Re: [PATCH v2 2/3] perf unwind: Find separate debuginfo without loading symbols

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] perf dso: dso__find_debuginfo() returns unverified first regular file
- [Medium] perf dso: dso__find_debuginfo() ignores symfs when reading build-id
- [Medium] perf dso: dso__find_debuginfo() skips mountns and chroot lookup
- [Medium] perf unwind: __find_debuginfo() skips fallback when symsrc is main binary
--

commit c3719700a5c64b73424eb58a041d6711fca9b8d4
Author: Matthew Wood <thepacketgeek@gmail.com>

perf unwind: Find separate debuginfo without loading symbols

This commit adds dso__find_debuginfo() to locate separate debuginfo files
for user space DSOs without loading symbol tables, and wires it into the
libdw and libunwind DWARF unwinders.

> diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
> index e03e2cb9212b1..36f5ab94c7e70 100644
> --- a/tools/perf/util/dso.c
> +++ b/tools/perf/util/dso.c
> @@ -598,6 +598,37 @@ int dso__read_build_id(struct dso *dso)
>  	return dso__has_build_id(dso) ? 0 : -1;
>  }
>  
> +/*
> + * Look for a separate debuginfo file for a user space DSO in the places
> + * dso__load() would, without loading its symbols. Returns an allocated path,
> + * or NULL if none was found.
> + */
> +char *dso__find_debuginfo(struct dso *dso)
> +{
> +	static const enum dso_binary_type debuginfo_types[] = {
> +		DSO_BINARY_TYPE__DEBUGLINK,
> +		DSO_BINARY_TYPE__BUILD_ID_CACHE_DEBUGINFO,
> +		DSO_BINARY_TYPE__FEDORA_DEBUGINFO,
> +		DSO_BINARY_TYPE__UBUNTU_DEBUGINFO,
> +		DSO_BINARY_TYPE__BUILDID_DEBUGINFO,
> +		DSO_BINARY_TYPE__OPENEMBEDDED_DEBUGINFO,
> +		DSO_BINARY_TYPE__MIXEDUP_UBUNTU_DEBUGINFO,
> +	};
> +	char filename[PATH_MAX];
> +
> +	/* The build-id cache and /usr/lib/debug/.build-id need the build-id. */
> +	dso__read_build_id(dso);

[Severity: Medium]
Does calling dso__read_build_id() here ignore symbol_conf.symfs when
--symfs is used and the DSO does not yet have a recorded build-id?

In tools/perf/util/symbol.c:dso__load(), perf joins symfs before reading
the build-id:

	if (!dso__has_build_id(dso)) {
		struct build_id bid = { .size = 0, };

		__symbol__join_symfs(name, PATH_MAX, dso__long_name(dso));
		if (filename__read_build_id(name, &bid) > 0)
			dso__set_build_id(dso, &bid);
	}

In contrast, tools/perf/util/dso.c:dso__read_build_id() passes
dso__long_name(dso) directly to filename__read_build_id() without
__symbol__join_symfs():

	if (filename__read_build_id(dso__long_name(dso), &bid) > 0)
		dso__set_build_id(dso, &bid);

If the binary only exists under symfs, dso__read_build_id() fails to read
the build-id. If a different version of the binary exists at
dso__long_name(dso) on the host root filesystem, dso__read_build_id()
populates the DSO with the host binary's build-id, causing subsequent
dso__load() calls to reject the symfs binary with
DSO_LOAD_ERRNO__MISMATCHING_BUILDID.

> +
> +	for (size_t i = 0; i < ARRAY_SIZE(debuginfo_types); i++) {
> +		if (dso__read_binary_type_filename(dso, debuginfo_types[i], /*root_dir=*/"",
> +						   filename, sizeof(filename)))
> +			continue;
> +		if (is_regular_file(filename))
> +			return strdup(filename);
> +	}

[Severity: Medium]
Can this loop return the stripped DSO itself or a file with a mismatched
build-id before reaching the remaining candidates in debuginfo_types[]?

When checking DSO_BINARY_TYPE__DEBUGLINK,
tools/perf/util/dso.c:dso__read_binary_type_filename() tests
debuglink_paths[] using is_regular_file():

		for (i = 0; i < ARRAY_SIZE(debuglink_paths); i++) {
			snprintf(filename, size,
					debuglink_paths[i], dso_dir, symfile);
			if (is_regular_file(filename)) {
				ret = 0;
				break;
			}
		}

When .gnu_debuglink has the same basename as the DSO itself (such as on
Debian/Ubuntu where /usr/lib/libfoo.so has separate debuginfo at
/usr/lib/debug/usr/lib/libfoo.so), debuglink_paths[1] ("%s/%s") resolves to
the stripped DSO binary itself. Similarly, for
DSO_BINARY_TYPE__BUILD_ID_CACHE_DEBUGINFO, __dso__build_id_filename()
ignores is_debug=true for vDSOs or flat build-id cache links and returns
the cached main binary.

In dso__load(), each candidate is opened via symsrc__init(), which checks
that the candidate's build-id matches the DSO:

	if (dso__has_build_id(dso) && !symbol_conf.ignore_vmlinux_buildid) {
		...
		if (!dso__build_id_equal(dso, &bid)) {
			pr_debug("%s: build id mismatch for %s.\n", __func__, name);
			*dso__load_errno(dso) = DSO_LOAD_ERRNO__MISMATCHING_BUILDID;
			goto out_elf_end;
		}
	}

and verifies symsrc__has_symtab() before stopping, allowing the loop to
continue to BUILD_ID_CACHE_DEBUGINFO, UBUNTU_DEBUGINFO, and
BUILDID_DEBUGINFO.

Because dso__find_debuginfo() returns the very first regular file without
checking if it is distinct from the DSO, if its build-id matches, or if it
contains debug/unwind sections:
- In __find_debuginfo(), if the returned path equals file_name, it is freed
  and libdw receives no debuginfo without trying later debuginfo_types[]
  entries.
- If a path-based candidate (DEBUGLINK, FEDORA_DEBUGINFO, UBUNTU_DEBUGINFO)
  on the host has a mismatched build-id while the matching debuginfo is in
  BUILD_ID_CACHE_DEBUGINFO or BUILDID_DEBUGINFO, libdwfl rejects the
  mismatched file and never calls __find_debuginfo() again for that module,
  while libunwind's read_unwind_spec_debug_frame() reads .debug_frame from
  the mismatched file and overwrites dso->symsrc_filename with it.

[Severity: Medium]
Also in this loop, how does dso__find_debuginfo() find debuginfo files
located inside a target process's mount namespace or chroot?

In tools/perf/util/symbol.c:dso__load(), perf enters dso__nsinfo(dso)'s
mount namespace (exiting for the build-id cache types) and falls back to
dso__filename_with_chroot() when is_regular_file() fails with ENOENT:

		if (nsexit)
			nsinfo__mountns_exit(&nsc);

		is_reg = is_regular_file(name);
		if (!is_reg && errno == ENOENT && dso__nsinfo(dso)) {
			char *new_name = dso__filename_with_chroot(dso, name);
			if (new_name) {
				is_reg = is_regular_file(new_name);
				strlcpy(name, new_name, PATH_MAX);
				free(new_name);
			}
		}

Here, dso__read_build_id(dso) enters and immediately exits the mount
namespace, so the debuginfo_types[] loop runs in perf's own mount namespace
without mountns or chroot handling.

> +	return NULL;
> +}
> +
>  static char *dso__get_filename(struct dso *dso, const char *root_dir,
>  			       bool *decomp)
>  {

[ ... ]

> diff --git a/tools/perf/util/unwind-libdw.c b/tools/perf/util/unwind-libdw.c
> index 63a5c2253174f..a2e70e4f206f7 100644
> --- a/tools/perf/util/unwind-libdw.c
> +++ b/tools/perf/util/unwind-libdw.c

[ ... ]

> @@ -39,11 +40,20 @@ static int __find_debuginfo(Dwfl_Module *mod __maybe_unused, void **userdata,
>  			    const char *file_name, const char *debuglink_file __maybe_unused,
>  			    GElf_Word debuglink_crc __maybe_unused, char **debuginfo_file_name)
>  {
> -	const struct dso *dso = *userdata;
> +	struct dso *dso = *userdata;
>  
>  	assert(dso);
> -	if (dso__symsrc_filename(dso) && strcmp(file_name, dso__symsrc_filename(dso)))
> -		*debuginfo_file_name = strdup(dso__symsrc_filename(dso));
> +	if (dso__symsrc_filename(dso)) {
> +		if (strcmp(file_name, dso__symsrc_filename(dso)))
> +			*debuginfo_file_name = strdup(dso__symsrc_filename(dso));
> +	} else {
> +		char *debuginfo = dso__find_debuginfo(dso);
> +
> +		if (debuginfo && strcmp(file_name, debuginfo))
> +			*debuginfo_file_name = debuginfo;
> +		else
> +			free(debuginfo);
> +	}
>  	return -1;
>  }

[Severity: Medium]
Should __find_debuginfo() also fall back to dso__find_debuginfo(dso) when
dso__symsrc_filename(dso) is equal to file_name?

If symbols were loaded from the main binary itself (for example, when the
binary retains a .symtab after strip --strip-debug, has .gnu_debugdata
MiniDebugInfo, or matched DSO_BINARY_TYPE__BUILD_ID_CACHE in dso__load()),
dso__symsrc_filename(dso) is non-NULL and equal to file_name while
.debug_frame is in a separate debuginfo file.

With the if/else structure above, strcmp(file_name, dso__symsrc_filename(dso))
is 0, so *debuginfo_file_name is not set and the else branch calling
dso__find_debuginfo(dso) is skipped.

[ ... ]

> diff --git a/tools/perf/util/unwind-libunwind.c b/tools/perf/util/unwind-libunwind.c
> index 73d191ce51a55..d92f1d4852851 100644
> --- a/tools/perf/util/unwind-libunwind.c
> +++ b/tools/perf/util/unwind-libunwind.c

[ ... ]

> @@ -301,19 +302,10 @@ static int read_unwind_spec_debug_frame(struct dso *dso,
>  		}
>  
>  		if (ofs <= 0) {
> -			char *debuglink = malloc(PATH_MAX);
> -			int ret = 0;
> -
> -			if (debuglink == NULL) {
> -				pr_err("unwind: Can't read unwind spec debug frame.\n");
> -				return -ENOMEM;
> -			}
> +			char *debuginfo = dso__find_debuginfo(dso);
>  
> -			ret = dso__read_binary_type_filename(
> -				dso, DSO_BINARY_TYPE__DEBUGLINK,
> -				machine->root_dir, debuglink, PATH_MAX);
> -			if (!ret) {
> -				fd = open(debuglink, O_RDONLY);
> +			if (debuginfo) {
> +				fd = open(debuginfo, O_RDONLY);
>  				if (fd >= 0) {
>  					ofs = elf_section_offset(fd,
>  							".debug_frame");
> @@ -326,12 +318,12 @@ static int read_unwind_spec_debug_frame(struct dso *dso,
>  						"%s: overwrite symsrc(%s,%s)\n",
>  							__func__,
>  							dso__symsrc_filename(dso),
> -							debuglink);
> +							debuginfo);
>  					dso__free_symsrc_filename(dso);
>  				}
> -				dso__set_symsrc_filename(dso, debuglink);
> +				dso__set_symsrc_filename(dso, debuginfo);
>  			} else {
> -				free(debuglink);
> +				free(debuginfo);
>  			}
>  		}
>  

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261017000158.1178269-1-thepacketgeek@gmail.com?part=2