Re: [PATCH v1 09/11] perf tools: Remove redundant chroot handling

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] perf: dso__get_filename() breaks chroot resolution outside mountns scope
--

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

perf tools: Remove redundant chroot handling

This commit removes dso__filename_with_chroot() and filename_with_chroot()
along with their fallback callers in dso__get_filename() and dso__load(),
relying on nsinfo__mountns_enter() to enter the target process's chroot.

> diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
> index ce98613868e7d..4331b48505b2b 100644
> --- a/tools/perf/util/dso.c
> +++ b/tools/perf/util/dso.c
> @@ -585,11 +585,6 @@ static int do_open(char *name) EXCLUSIVE_LOCKS_REQUIRED(_dso__data_open_lock)
>  	return -1;
>  }
>  
> -char *dso__filename_with_chroot(const struct dso *dso, const char *filename)
> -{
> -	return filename_with_chroot(nsinfo__pid(dso__nsinfo_const(dso)), filename);
> -}
> -
>  char *dso__get_filename(struct dso *dso, const char *root_dir, bool *decomp,
>  			enum dso_binary_type type)
>  {
> @@ -603,28 +598,8 @@ char *dso__get_filename(struct dso *dso, const char *root_dir, bool *decomp,
>  	if (dso__read_binary_type_filename(dso, type, root_dir, name, PATH_MAX))
>  		goto out;
>  
> -	if (!is_regular_file(name)) {
> -		struct stat st;
> -		char *new_name;
> -
> -		/*
> -		 * errno only reflects the failure reason when stat() itself
> -		 * failed: a successful stat() on a non-regular file (e.g. a
> -		 * directory) leaves a stale errno, which a previous failed
> -		 * iteration of the try_to_open_dso() fallback loop may have
> -		 * set to ENOENT.
> -		 */
> -		if (stat(name, &st) == 0 || errno != ENOENT ||
> -		    dso__nsinfo(dso) == NULL)
> -			goto out;
> -
> -		new_name = dso__filename_with_chroot(dso, name);
> -		if (!new_name)
> -			goto out;
> -
> -		free(name);
> -		name = new_name;
> -	}
> +	if (!is_regular_file(name))
> +		goto out;

[Severity: Medium]
Are all callers of dso__get_filename() guaranteed to execute inside an
nsinfo__mountns_enter() scope?

While open_dso() -> __open_dso() enters the mount namespace and chroot, the
other callers of dso__get_filename() run outside nsinfo__mountns_enter().
When profiling a chrooted workload whose binaries are not already populated
in the host build-id cache (such as in pipe mode, live mode, or with
perf record -N), is_regular_file(name) in dso__get_filename() checks the
host root filesystem and fails with ENOENT.

Specifically, try_to_open_dso() resolves the binary type before entering the
mount namespace in open_dso():

tools/perf/util/dso.c:try_to_open_dso() {
	...
	__dso__find_binary_type(dso);
	dso_data->fd = open_dso(dso, machine);
	...
}

__dso__find_binary_type() -> dso__find_binary_type_locked() calls
dso__get_filename() and filename__has_section(path, ".text") without
calling nsinfo__mountns_enter():

tools/perf/util/dso.c:dso__find_binary_type_locked() {
	...
		errno = 0;
		path = dso__get_filename(dso, "", &decomp, dso__binary_type(dso));
		if (path == NULL) {
			if (errno == EMFILE)
				goto open_err;
			continue;
		}

		errno = 0;
		found = filename__has_section(path, ".text");
	...
}

Because dso__get_filename() returns NULL outside the chroot,
dso__find_binary_type_locked() sets dso->binary_type to
DSO_BINARY_TYPE__NOT_FOUND, causing the subsequent open_dso() call in
try_to_open_dso() to fail as well.

Similarly, symbol__disassemble() in tools/perf/util/disasm.c:

	dbt = dso__find_binary_type(dso);

	if (dbt == DSO_BINARY_TYPE__KALLSYMS)
		return SYMBOL_ANNOTATE_ERRNO__NO_VMLINUX;

	mutex_lock(dso__lock(dso));
	symfs_filename = dso__get_filename(dso, "", &decomp, dbt);
	mutex_unlock(dso__lock(dso));

and dso__debuginfo() -> debuginfo__new() -> dso__find_dbginfo_type() in
tools/perf/util/debuginfo.c and tools/perf/util/dso.c:

	dso__find_dbginfo_type(dso);
	path = dso__get_filename(dso, "", &decomp, dso__dbginfo_type(dso));
	if (path == NULL)
		return NULL;

	dinfo = __debuginfo__new(path);

also invoke dso__get_filename(), filename__has_section(), and
__debuginfo__new() without entering nsinfo__mountns_enter().

Should those callers enter the mount namespace via nsinfo__mountns_enter(),
or does dso__get_filename() still need chroot path resolution for callers
that use the returned path outside a mountns scope?

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