Re: [PATCH v3 08/10] perf symbol: Do not set binary type from symtab type

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] perf/dso: out-of-bounds access in close_first_dso() on empty list
- [Medium] perf/dso: deadlock in __dso__find_binary_type() during EMFILE retry
- [Medium] perf: stale errno causes false failure in dso__find_binary_type_locked()
--

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

perf symbol: Do not set binary type from symtab type

The DSO binary type is intended to access binary data, while the symtab
type is for the symbol table. This commit separates their tracking by
leaving the binary type info intact and only updating it when actually
opening the DSO.

> diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
> index 851b39c63ec86..a8e531766f1b5 100644
> --- a/tools/perf/util/dso.c
> +++ b/tools/perf/util/dso.c
> @@ -1485,9 +1485,10 @@ uint16_t dso__e_machine_endian(struct dso *dso, struct machine *machine, uint32_
>  			       bool *is_big_endian)
>  {
>  	uint16_t e_machine = EM_NONE;
> +	enum dso_binary_type dbt = dso__find_binary_type(dso);

[Severity: Medium]
This new call to dso__find_binary_type() can expose a potential deadlock.

If dso__find_binary_type_locked() returns -1 due to an open file limit,
__dso__find_binary_type() acquires dso__data_open_lock() but does not release
it before jumping back:

tools/perf/util/dso.c:__dso__find_binary_type() {
    ...
	if (ret == -1 && retry++ < FIND_MAX_RETRY) {
		if (needs_open_lock)
			mutex_lock(dso__data_open_lock());
		close_first_dso();
		goto again;
	}
    ...
}

Could this cause a double-lock deadlock on dso__data_open_lock() if
ret == -1 happens again on the next iteration?

[Severity: High]
When __dso__find_binary_type() encounters EMFILE, it unconditionally calls
close_first_dso():

tools/perf/util/dso.c:__dso__find_binary_type() {
    ...
		close_first_dso();
    ...
}

Does close_first_dso() extract the first entry from the dso__data_open list
without verifying if the list is actually populated?

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, couldn't this yield an invalid pointer and lead to
out-of-bounds memory corruption when closing the fd?

[Severity: Medium]
Inside dso__find_binary_type_locked(), it appears we read errno unconditionally
when filename__has_section() returns false:

tools/perf/util/dso.c:dso__find_binary_type_locked() {
    ...
		found = filename__has_section(path, ".text");
		if (!found)
			saved_errno = errno;
		dso__put_filename(dso, path, decomp);

		if (found)
			return dso__binary_type(dso);
		else if (saved_errno == EMFILE)
			goto open_err;
    ...
}

Since a false return legitimately occurs when the .text section is missing
(without updating errno), could a stale EMFILE value be erroneously captured
here and cause false failures?

>  	int fd;
>  
> -	switch (dso__binary_type(dso)) {
> +	switch (dbt) {
>  	case DSO_BINARY_TYPE__KALLSYMS:
>  	case DSO_BINARY_TYPE__GUEST_KALLSYMS:
>  	case DSO_BINARY_TYPE__VMLINUX:

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