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 3 potential issue(s) to consider:

New issues:
- [High] Stale `errno` usage in `dso__get_filename()` and `__open_dso()` creates brittle logic, incorrect error propagation, and loop pollution.

Pre-existing issues:
- [High] `file_read()` attempts to extract the failure reason using `-errno` after several cleanup and namespace functions have clobbered the `errno` value.
- [High] Strict AB-BA deadlock between `_dso__data_open_lock` and `dso__lock` when `REFCNT_CHECKING` is enabled and the `EMFILE` limit is hit.
--

--- Patch [1]: [PATCH 1/5] perf dso: Guard against errno==0 when dso__get_filename() returns NULL ---
commit b3606e9e10d4ce3d99991eea12437267b852da75
Author: Arnaldo Carvalho de Melo <[email protected]>

perf dso: Guard against errno==0 when dso__get_filename() returns NULL

This commit fixes an issue where __open_dso() could incorrectly return 0
(stdin) when dso__get_filename() returns NULL without setting errno.
It forces errno to ENOENT if it is 0 upon failure, ensuring a negative
file descriptor is returned.

> --- tools/perf/util/dso.c
> +++ tools/perf/util/dso.c
> @@ -640,10 +640,13 @@ static int __open_dso(struct dso *dso, struct machine *machine)
>  	mutex_lock(dso__lock(dso));

[Severity: High]
This is a pre-existing issue, but can this lock ordering cause an AB-BA
deadlock when REFCNT_CHECKING is enabled?

If do_open() hits the EMFILE limit, close_first_dso() calls
dso__list_del(). Under REFCNT_CHECKING, dso__list_del() drops and
reacquires dso__data_open_lock() to satisfy constraints:

tools/perf/util/dso.c:dso__list_del() {
#ifdef REFCNT_CHECKING
    mutex_unlock(dso__data_open_lock());
    dso__put(dso__data(dso)->dso);
    mutex_lock(dso__data_open_lock());
#endif
}

Since dso__lock(dso) is held here in __open_dso(), if another thread
acquires dso__data_open_lock() and then attempts __open_dso() on the
same dso, will they deadlock waiting on each other?

>  
>  	name = dso__get_filename(dso, machine ? machine->root_dir : "", &decomp);
> -	if (name)
> +	if (name) {
>  		fd = do_open(name);
> -	else
> +	} else {
> +		if (errno == 0)
> +			errno = ENOENT;

[Severity: High]
Does setting errno to ENOENT here pollute the thread-local state for
subsequent loop iterations?

If a missing DSO file triggers a fallback loop like try_to_open_dso(),
and the next iteration evaluates a directory instead of an ELF file,
is_regular_file() will return false.

Since stat() succeeds on a directory, it won't change errno. The
stale ENOENT from here means dso__get_filename() evaluates
if (errno != ENOENT || ...) as false.

Will this cause the code to incorrectly bypass the error path,
proceed to dso__filename_with_chroot(), and successfully open a
directory instead of an ELF file?

> +		fd = -errno;
> +	}

[Severity: High]
This is a pre-existing issue, but does relying on -errno here and in
file_read() lead to incorrect error propagation?

When try_to_open_dso() fails, file_read() attempts to extract the failure
reason:

tools/perf/util/dso.c:file_read() {
    ...
    if (dso__data(dso)->fd < 0) {
        dso__data(dso)->status = DSO_DATA_STATUS_ERROR;
        ret = -errno;
        goto out;
    }
}

However, between the actual failure and this check, cleanup functions like
unlink(), mutex_unlock(), and nsinfo__mountns_exit() execute.

Will these calls clobber errno, causing file_read() to return an arbitrary
error code or 0, silently failing to report the true failure reason?

>  
>  	if (decomp)
>  		unlink(name);
>  	mutex_unlock(dso__lock(dso));
>  	free(name);
>  	return fd;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1