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

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?

[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