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

[Namhyung Kim <namhyung@kernel.org>]

On Mon, Oct 12, 2026 at 05:38:32AM +0000, sashiko-bot@kernel.org wrote:
> 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.

Ok, I'll clear errno before calling filename__has_section().

> 
> > +		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.

Ok, I'll check the open count first.

> 
> > +		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.

Oops, right.  I'll make sure it's call only once.

Thanks,
Namhyung