Re: [PATCH v3 06/10] perf tools: Factor out dso__find_binary_type()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] perf: Deadlock in __dso__find_binary_type retry loop on non-recursive mutex
- [High] perf: Invalid memory access/crash in close_first_dso() due to missing emptiness check
- [High] perf: Loss of machine->root_dir context breaks guest/container path resolution
- [Medium] perf: Stale errno causes spurious EMFILE failure and retry exhaustion
--

commit 281cbe5b226891d2bef88be13adaa52f889f6f89
Author: Namhyung Kim <namhyung@kernel.org>

perf tools: Factor out dso__find_binary_type()

This commit factors out the binary type detection logic for a DSO into a
separate function, checking for a ".text" section under the DSO lock. It
also introduces retry logic to handle file descriptor limits by closing
an open DSO when EMFILE is encountered.

> diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
> index 913877b84b2d4..851b39c63ec86 100644
> --- a/tools/perf/util/dso.c
> +++ b/tools/perf/util/dso.c
[ ... ]
> @@ -833,34 +833,142 @@ void dso__data_close(struct dso *dso)
[ ... ]
>  	do {
>  		dso__set_binary_type(dso, binary_type_data[i++]);
>  
> -		dso_data->fd = open_dso(dso, machine);
> -		if (dso_data->fd >= 0)
> -			goto out;
> +		path = dso__get_filename(dso, "", &decomp, dso__binary_type(dso));

[Severity: High]
Does hardcoding an empty string instead of propagating machine->root_dir
break guest or container path resolution?

In the pre-existing code, try_to_open_dso() passed the machine down to
open_dso(), which correctly resolved paths using machine->root_dir.

Because dso__find_binary_type() does not accept a machine parameter, it drops
the root directory context. Any binary types requiring alternative root
directories, such as DSO_BINARY_TYPE__GUEST_KMODULE, might incorrectly
resolve against the host's root file system, preventing proper symbol
decoding.

> +		if (path == NULL) {
> +			if (errno == EMFILE)
> +				goto open_err;
> +			continue;
> +		}
> +
> +		found = filename__has_section(path, ".text");
> +		if (!found)
> +			saved_errno = errno;

[Severity: Medium]
Could this read a stale errno if filename__has_section() returns false
gracefully?

If the ELF file is parsed successfully but lacks a .text section,
filename__has_section() returns false without setting or clearing errno.

If a previous, completely unrelated system call set errno to EMFILE,
saved_errno would take that stale value.

> +		dso__put_filename(dso, path, decomp);
> +
> +		if (found)
> +			return dso__binary_type(dso);
> +		else if (saved_errno == EMFILE)
> +			goto open_err;

This might cause an incorrect detection of EMFILE, which could then be
mistakenly interpreted as a file descriptor exhaustion failure, triggering
the open_err retry loop.

[ ... ]
> +static int __dso__find_binary_type(struct dso *dso, bool needs_open_lock)
> +{
> +	int ret;
> +	int retry = 0;
> +
> +again:
> +	mutex_lock(dso__lock(dso));
> +	ret = dso__find_binary_type_locked(dso);
> +	mutex_unlock(dso__lock(dso));
> +
> +	if (ret == -1 && retry++ < FIND_MAX_RETRY) {
> +		if (needs_open_lock)
> +			mutex_lock(dso__data_open_lock());
> +		close_first_dso();

[Severity: High]
Is it safe to call close_first_dso() unconditionally here?

If the global file descriptor limit is exhausted outside of the perf DSO
cache, or if the EMFILE error is spuriously triggered by a stale errno on
the very first DSO parsed, the list could be completely empty.

Looking at close_first_dso(), it extracts the first entry from the
dso__data_open linked list via list_first_entry() and dereferences it
without verifying 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);
    ...
}

If the list is empty, list_first_entry() resolves to an invalid pointer,
causing a crash upon dereference. The pre-existing caller,
check_data_close(), safely verified limits before closing.

> +		goto again;
> +	}

[Severity: High]
Does the goto again bypass unlocking the dso__data_open_lock?

If needs_open_lock is true and dso__find_binary_type_locked() returns -1,
the retry loop executes mutex_lock(dso__data_open_lock()) and jumps to again
without releasing the lock.

If it fails a second time, the code evaluates if (needs_open_lock) again and
incorrectly calls mutex_lock(dso__data_open_lock()) a second time.

Because perf uses standard non-recursive mutexes by default, acquiring the
lock a second time from the same thread could cause an immediate, permanent
deadlock.

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012052601.13920-1-namhyung@kernel.org?part=6