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] `dso__get_filename()` can fail and return `NULL` without setting `errno` to a non-zero value, leading `__open_dso()` to mistakenly treat the error as success or yield an invalid state (like caching `stdin` as a DSO file descriptor).
- [Low] `close()` is called unconditionally on a potentially negative file descriptor in `dso__decompress_kmodule_path()`.
--

commit 0a5c1c7faf0a0df19aeca89ce7afb8679748c9b8
Author: Arnaldo Carvalho de Melo <acme@redhat.com>

perf dso: Set standard errno on decompression failure

The commit fixes an issue where a negative custom error value was used
to set errno on decompression failure. This caused the caller to compute
a valid-looking positive file descriptor, which could lead to closing
unrelated file descriptors.

> 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);
>  

[Severity: High]
This is a pre-existing issue, but since __open_dso() relies on errno to
compute the file descriptor when dso__get_filename() fails:

tools/perf/util/dso.c:__open_dso() {
...
    if (name)
        fd = do_open(name);
    else
        fd = -errno;
...
}

Does dso__get_filename() always set errno when returning NULL?

Earlier in dso__get_filename(), if dso__read_binary_type_filename()
returns -1, it jumps to the out label and returns NULL, but errno might
remain 0. If errno is 0, __open_dso() computes fd = -0, which is standard
input.

Could this cause the tool to mistakenly cache stdin as the DSO data file
descriptor and later unintentionally close it when it reaches the open file
limit?

>  		if (dso__decompress_kmodule_path(dso, name, newpath, len) < 0) {

[Severity: Low]
This isn't a bug introduced by this patch, but does
dso__decompress_kmodule_path() handle negative file descriptors correctly?

If the kernel module doesn't need decompression, decompress_kmodule()
returns -1:

tools/perf/util/dso.c:dso__decompress_kmodule_path() {
...
    int fd = decompress_kmodule(dso, name, pathname, len);

    close(fd);
...
}

Calling close(-1) will fail and set errno to EBADF. Could this clobber the
errno value unnecessarily?

> -			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;
>  		}

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