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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] perf: Guest kernel modules fail to open due to hardcoded root_dir
- [High] perf: Strict .text section check breaks data-only DSOs and NO_LIBELF builds
- [High] perf: Concurrent Read-Modify-Write data race on struct dso bitfields
- [High] perf: Use-After-Free of dso->nsinfo in __dso__find_binary_type due to missing dso__lock
- [Medium] perf: Explicitly resolved binary types are unconditionally overwritten
- [Medium] perf: Bypassing do_open() permanently caches EMFILE failures as NOT_FOUND
- [Low] perf: Redundant double decompression of compressed kernel modules
--

commit 8b41de1355b0e2f1d37b604c80238237fc833f0f
Author: Namhyung Kim <namhyung@kernel.org>

perf tools: Factor out dso__find_binary_type()

Separate out the logic to detect binary type of a DSO.  It checks if the
candidate file contains ".text" section.  It should be called under the
_dso__data_open_lock.

Introduce bintype_searched flag and it accessors to prevent repeated
(failed) search.

> diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
> index 913877b84b2d4..f5054c18a2e93 100644
> --- a/tools/perf/util/dso.c
> +++ b/tools/perf/util/dso.c
> @@ -833,7 +833,7 @@ void dso__data_close(struct dso *dso)
>  	mutex_unlock(dso__data_open_lock());
>  }
>  
> -static void try_to_open_dso(struct dso *dso, struct machine *machine)
> +static enum dso_binary_type __dso__find_binary_type(struct dso *dso)
>  	EXCLUSIVE_LOCKS_REQUIRED(_dso__data_open_lock)
>  {
>  	enum dso_binary_type binary_type_data[] = {
> @@ -842,25 +842,103 @@ static void try_to_open_dso(struct dso *dso, struct machine *machine)
>  		DSO_BINARY_TYPE__NOT_FOUND,
>  	};
>  	int i = 0;
> -	struct dso_data *dso_data = dso__data(dso);
> +	char *path;
> +	bool found = false;
> +	bool decomp = false;
>  
> -	if (dso_data->fd >= 0)
> -		return;
> +	if (dso__bintype_searched(dso))
> +		return dso__binary_type(dso);
> +	dso__set_bintype_searched(dso);

[Severity: High]
Does this code introduce a data race on the struct dso bitfields?

The dso__set_bintype_searched() function modifies the bintype_searched
bitfield under the global _dso__data_open_lock. However, adjacent bitfields
like debuginfo_searched and long_name_allocated share the same memory
location and are modified under different locks (dso__lock(dso) and
dsos->lock).

Concurrent execution could cause compiler-generated read-modify-write
operations to overlap, resulting in dropped flags or memory leaks if
long_name_allocated is clobbered.

[ ... ]

> +	case DSO_BINARY_TYPE__SYSTEM_PATH_DSO:
> +	case DSO_BINARY_TYPE__BUILD_ID_CACHE:
> +		/* We don't expect these are set; fall through */
> +	case DSO_BINARY_TYPE__NOT_FOUND:
> +		/* Let's find it out (for user DSOs or kernel modules) */
> +		break;

[Severity: Medium]
Does this fall-through discard valid binary types that have already been
resolved elsewhere in the system?

When symbol.c successfully resolves and sets the binary type (such as
DSO_BINARY_TYPE__SYSTEM_PATH_DSO or DSO_BINARY_TYPE__BUILD_ID_CACHE),
this switch statement deliberately falls through. The loop below then
unconditionally overwrites the previously valid binary type.

If the fallback types lack a .text section (such as data-only DSOs), the
state is permanently left as NOT_FOUND, preventing perf from reading them.

[ ... ]

>  	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 the root directory to an empty string break path resolution
for guest kernel modules?

When the fallback logic looks up types like DSO_BINARY_TYPE__GUEST_KMODULE,
it passes "" to dso__get_filename() instead of machine->root_dir.

This causes dso__get_filename() to construct an invalid path missing the
guest's root directory. The file cannot be found, filename__has_section()
fails, and subsequent calls to open_dso() will fail.

[Severity: High]
Does calling dso__get_filename() without holding dso__lock(dso) cause a
use-after-free of dso->nsinfo?

The new __dso__find_binary_type() helper executes with only the global
_dso__data_open_lock held. It calls dso__get_filename(), which invokes
dso__filename_with_chroot() and dereferences dso->nsinfo to read the
process ID.

Concurrently, another thread could call dso__set_nsinfo() (which acquires
dso__lock(dso)), replacing the nsinfo pointer and freeing the old one via
nsinfo__put(). Since this read path ignores dso__lock(dso), it can read
the freed pointer.

> +		if (path == NULL)
> +			continue;
> +
> +		found = filename__has_section(path, ".text");

[Severity: High]
Does this strict .text section check break data-only DSOs and NO_LIBELF
builds?

Prior to this patch, try_to_open_dso() discovered the DSO by calling
open_dso(). If the file existed and was readable, the open succeeded. This
patch introduces a strict requirement that a DSO must contain a .text
section.

Perf commonly profiles memory data accesses (like perf mem or perf c2c)
hitting data-only DSOs. For these files, filename__has_section() evaluates
to false, forcing the binary type to NOT_FOUND. Furthermore, if perf is
compiled without libelf (NO_LIBELF=1), filename__has_section() is stubbed
to unconditionally return false, breaking user DSO loading entirely.

[Severity: Medium]
Can this bypass of do_open() permanently cache EMFILE failures as NOT_FOUND?

The filename__has_section() function uses a raw open() without the FD limit
(EMFILE) handling and LRU eviction logic normally provided by do_open() and
close_first_dso().

If perf hits RLIMIT_NOFILE, open() returns -1, and filename__has_section()
silently returns false. __dso__find_binary_type() then assumes the binary
is invalid, sets bintype_searched = 1, and permanently caches
DSO_BINARY_TYPE__NOT_FOUND, breaking data access to the DSO for the rest
of the execution.

[ ... ]

> +static void try_to_open_dso(struct dso *dso, struct machine *machine)
> +	EXCLUSIVE_LOCKS_REQUIRED(_dso__data_open_lock)
> +{
> +	struct dso_data *dso_data = dso__data(dso);
> +
> +	if (dso_data->fd >= 0)
> +		return;
> +
> +	__dso__find_binary_type(dso);
> +	dso_data->fd = open_dso(dso, machine);

[Severity: Low]
Does this result in redundant double decompression of compressed kernel
modules?

The call to dso__get_filename() in __dso__find_binary_type() decompresses
a compressed kernel module to a temporary file. After checking for a .text
section, dso__put_filename() unlinks this temporary file.

Immediately after returning, try_to_open_dso() calls open_dso(), which
ultimately calls dso__get_filename() again, forcing a second decompression
of the exact same kernel module.

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