Re: [PATCH 1/5] perf dso: Guard against errno==0 when dso__get_filename() returns NULL

On Sun, Aug 02, 2026 at 02:58:28PM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> New issues:
> - [Medium] Incomplete fix in `__open_dso()` causes error swallowing in callers by returning negative fd without setting `errno`.
> 
> Pre-existing issues:
> - [Medium] Data race on adjacent `struct dso` bitfields leads to corruption and a TOCTOU mount namespace leak.
> --
> 
> --- Patch [1]: [PATCH 1/5] perf dso: Guard against errno==0 when dso__get_filename() returns NULL ---
> commit db31953321ccfc3f2f28e5db31230d5fef57578f
> Author: Arnaldo Carvalho de Melo <[email protected]>
> 
> perf dso: Guard against errno==0 when dso__get_filename() returns NULL
> 
> This commit updates __open_dso() to fall back to ENOENT when
> dso__get_filename() returns NULL but leaves errno as 0. This ensures fd
> is always negative on failure.
> 
> > diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
> > index 2309196d8df31..e087a89066bdb 100644
> > --- a/tools/perf/util/dso.c
> > +++ b/tools/perf/util/dso.c
> > @@ -643,7 +643,7 @@ static int __open_dso(struct dso *dso, struct machine *machine)
> >  	if (name)
> >  		fd = do_open(name);
> >  	else
> > -		fd = -errno;
> > +		fd = errno ? -errno : -ENOENT;
> 
> [Severity: Medium]
> Does this code need to explicitly set errno to ENOENT as well?
> 
> Callers of __open_dso() appear to check if the returned file descriptor is
> negative, and then compute their return value using errno. For example, in
> file_read():
> 
> tools/perf/util/dso.c:file_read() {
>     ...
>     fd = data_file_open(dso, machine);
>     if (fd < 0)
>         return -errno;
>     ...
> }
> 
> Since errno is left as 0, callers like file_read() will evaluate -errno
> to 0, which incorrectly reports EOF instead of an open failure. Should we
> update errno directly?

Agreed, will fix for v2.

- Arnaldo
 
> [Severity: Medium]
> This is a pre-existing issue, but can the adjacent bitfields in struct dso
> cause a data race that leaks mount namespaces?
> 
> Looking at the struct dso definition:
> 
> tools/perf/util/dso.h:struct dso {
>     ...
>     enum dso_binary_type symtab_type:8;
>     enum dso_binary_type binary_type:8;
>     ...
> }
> 
> If one thread calls try_to_open_dso() and locklessly modifies binary_type,
> while another concurrently modifies symtab_type holding dso__lock(dso),
> the concurrent read-modify-write on the shared memory word can corrupt
> binary_type.
> 
> This corruption appears to break the namespace exit check in open_dso():
> 
> tools/perf/util/dso.c:open_dso() {
>     ...
>     if (dso__binary_type(dso) != DSO_BINARY_TYPE__BUILD_ID_CACHE) {
>         mutex_lock(dso__lock(dso));
>         nsinfo__mountns_enter(dso__nsinfo(dso), &nsc);
>         mutex_unlock(dso__lock(dso));
>     }
>     fd = __open_dso(dso, machine);
>     if (dso__binary_type(dso) != DSO_BINARY_TYPE__BUILD_ID_CACHE)
>         nsinfo__mountns_exit(&nsc);
>     ...
> }
> 
> If the bitfield gets corrupted during __open_dso(), the exit check might
> fail, skipping nsinfo__mountns_exit() and permanently stranding the thread
> in the wrong mount namespace. Should access to these bitfields be protected
> by dso__lock, or should they be separated to avoid sharing memory bytes?
> 
> >  
> >  	if (decomp)
> >  		unlink(name);
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1