Re: [PATCH 9/9] perf dso: Set standard errno on decompression failure

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] If `dso__get_filename()` returns NULL without explicitly setting `errno` (e.g., when the target path is a directory and `is_regular_file()` returns false without altering `errno`, or when `dso__has_build_id()` is false), `__open_dso()` computes `fd = -errno`, resulting in `fd = 0`. The subsequent check `if (fd >= 0)` in `open_dso()` incorrectly treats this as a successful open and registers standard input (fd 0) as the DSO data file descriptor.
- [High] Under `REFCNT_CHECKING`, `dso__list_del()` removes a DSO from the open list and temporarily unlocks `_dso__data_open_lock` before decrementing `dso__data_open_cnt`. This breaks the invariant between the list length and the counter. If a concurrent thread calls `close_first_dso()`, it observes `dso__data_open_cnt > 0` but reads from an empty list.
--

commit 6181e27c050bdb8f633dfde54842ef5d8d538b5a
Author: Arnaldo Carvalho de Melo <acme@redhat.com>

perf dso: Set standard errno on decompression failure

dso__get_filename sets errno to a negative custom DSO_LOAD_ERRNO value
when kernel module decompression fails. The caller __open_dso then computes
fd = -errno, producing a large positive value that looks like a valid file
descriptor. This commit sets errno to EIO instead to avoid incorrectly
closing unrelated file descriptors.

Fixes: 1d6b3c9ba756a513 ("perf tools: Decompress kernel module when reading DSO data")
Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Assisted-by: Claude <noreply@anthropic.com>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>

> diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
> index 79f1a30f3683d..2309196d8df31 100644
> --- a/tools/perf/util/dso.c
> +++ b/tools/perf/util/dso.c
> @@ -600,7 +600,13 @@ static char *dso__get_filename(struct dso *dso, const char *root_dir,
>  		size_t len = sizeof(newpath);
>  
>  		if (dso__decompress_kmodule_path(dso, name, newpath, len) < 0) {
> -			errno = *dso__load_errno(dso);
> +			/*
> +			 * Use a standard errno value, not the negative custom
> +			 * DSO_LOAD_ERRNO stored in dso__load_errno(dso):
> +			 * __open_dso() computes fd = -errno, so a negative
> +			 * errno produces a positive fd that looks valid.
> +			 */
> +			errno = EIO;
>  			goto out;
>  		}

[Severity: High]
This isn't a bug introduced by this patch, but since we are fixing errno
handling here, could a similar issue occur earlier in dso__get_filename
when it fails without explicitly setting errno?

If stat succeeds on a directory, errno is 0 but S_ISREG fails.
is_regular_file returns false and errno remains 0, jumping to out:

tools/perf/util/dso.c:dso__get_filename() {
    ...
	if (!is_regular_file(name)) {
		char *new_name;

		if (errno != ENOENT || dso__nsinfo(dso) == NULL)
			goto out;
    ...
}

In this case, __open_dso computes fd = -errno, resulting in fd = 0:

tools/perf/util/dso.c:__open_dso() {
    ...
	name = dso__get_filename(dso, machine ? machine->root_dir : "", &decomp);
	if (name)
		fd = do_open(name);
	else
		fd = -errno;
    ...
}

Then open_dso incorrectly treats this as a successful open and registers
standard input as the DSO data file descriptor:

tools/perf/util/dso.c:open_dso() {
    ...
	fd = __open_dso(dso, machine);
	if (fd >= 0) {
		dso__list_add(dso);
    ...
}

Could this lead to resource hijacking or blocking on stdin?

[Severity: High]
This is another pre-existing issue, but I noticed a potential race condition
with the DSO lists.

Under REFCNT_CHECKING, dso__list_del removes a DSO from the open list and
temporarily unlocks dso__data_open_lock before decrementing the counter:

tools/perf/util/dso.c:dso__list_del() {
    ...
	list_del_init(&dso__data(dso)->open_entry);
#ifdef REFCNT_CHECKING
	mutex_unlock(dso__data_open_lock());
	dso__put(dso__data(dso)->dso);
	mutex_lock(dso__data_open_lock());
#endif
	WARN_ONCE(dso__data_open_cnt <= 0, ...);
	dso__data_open_cnt--;
    ...
}

If a concurrent thread calls close_first_dso, it checks the counter rather
than checking if the list is empty:

tools/perf/util/dso.c:close_first_dso() {
    ...
	dso_data = list_first_entry(&dso__data_open, struct dso_data, open_entry);
#ifdef REFCNT_CHECKING
	dso = dso_data->dso;
#else
	...
	close_dso(dso);
    ...
}

Because dso__data_open_cnt is greater than zero even when the list is
empty, list_first_entry computes an invalid pointer from the list head.
This causes dso_data->dso to evaluate to the address of
dso__data_open_cnt, treating the integer as a struct pointer and
dereferencing it.

Can this result in a concurrent segmentation fault under REFCNT_CHECKING
builds?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260616022715.5739-1-acme@kernel.org?part=9