Re: [PATCH v1 11/11] perf tools: Scope mount namespace entry in dso__load()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] perf symbol: dso__load() skips mountns entry for distro debuginfo types
- [Medium] perf symbol: dso__load() exits mountns before minimal dso__load_sym()
--

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

perf tools: Scope mount namespace entry in dso__load()

This commit narrows the scope of mount namespace entry in dso__load() so
that the target mount namespace is entered only when reading perf maps,
reading build-ids, or probing binary types where dso_binary_type__needs_ns()
returns true.

> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> index 807e433c77081..063e9da0878e9 100644
> --- a/tools/perf/util/symbol.c
> +++ b/tools/perf/util/symbol.c

[ ... ]

> @@ -1882,24 +1879,25 @@ int dso__load(struct dso *dso, struct map *map)
>  		struct symsrc *ss = &ss_[ss_pos];
>  		bool next_slot = false;
>  		bool is_reg;
> -		bool nsexit;
> +		bool nsenter;
>  		int bfdrc = -1;
>  		int sirc = -1;
>  
>  		enum dso_binary_type symtab_type = binary_type_symtab[i];
>  
> -		nsexit = (symtab_type == DSO_BINARY_TYPE__BUILD_ID_CACHE ||
> -		    symtab_type == DSO_BINARY_TYPE__BUILD_ID_CACHE_DEBUGINFO);
> -
>  		if (!dso__is_compatible_symtab_type(dso, kmod, symtab_type))
>  			continue;
>  
> +		nsenter = dso_binary_type__needs_ns(symtab_type);
> +		if (nsenter)
> +			nsinfo__mountns_enter(dso__nsinfo(dso), &nsc);

[Severity: Medium]
Could gating namespace entry on dso_binary_type__needs_ns() break separate
distro debuginfo lookup for processes in containers or chroots?

Previously, dso__load() stayed in the target mount namespace for all
binary_type_symtab[] entries except DSO_BINARY_TYPE__BUILD_ID_CACHE and
DSO_BINARY_TYPE__BUILD_ID_CACHE_DEBUGINFO.

In tools/perf/util/dso.h, dso_binary_type__needs_ns() returns false for
distro debuginfo types:

tools/perf/util/dso.h:dso_binary_type__needs_ns() {
	switch (type) {
	case DSO_BINARY_TYPE__SYSTEM_PATH_DSO:
	case DSO_BINARY_TYPE__SYSTEM_PATH_KMODULE:
	case DSO_BINARY_TYPE__SYSTEM_PATH_KMODULE_COMP:
	case DSO_BINARY_TYPE__DEBUGLINK:
	case DSO_BINARY_TYPE__GNU_DEBUGDATA:
		return true;
	...
	case DSO_BINARY_TYPE__FEDORA_DEBUGINFO:
	case DSO_BINARY_TYPE__UBUNTU_DEBUGINFO:
	case DSO_BINARY_TYPE__MIXEDUP_UBUNTU_DEBUGINFO:
	case DSO_BINARY_TYPE__BUILDID_DEBUGINFO:
	...
	case DSO_BINARY_TYPE__OPENEMBEDDED_DEBUGINFO:
	...
		return false;
	}
}

Just before this loop, dso__load() reads the build-id inside the target
mount namespace specifically for DSO_BINARY_TYPE__BUILDID_DEBUGINFO:

tools/perf/util/symbol.c:dso__load() {
	/*
	 * Read the build id if possible. This is required for
	 * DSO_BINARY_TYPE__BUILDID_DEBUGINFO to work.
	 */
	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_ns(name, &bid, dso__nsinfo(dso)) > 0)
			dso__set_build_id(dso, &bid);
	}
}

When dso_binary_type__needs_ns() returns false for those debuginfo types,
dso__read_binary_type_filename(), is_regular_file(), dso__load_bfd_symbols(),
and symsrc__init() will check the host filesystem instead of the target
container or chroot filesystem (unlike build_id_cache__find_debug() in
tools/perf/util/build-id.c, which enters the target mount namespace when
resolving /usr/lib/debug/.build-id/ paths).

> +
>  		if (dso__read_binary_type_filename(dso, symtab_type,
> -						   root_dir, name, PATH_MAX))
> +						   root_dir, name, PATH_MAX)) {
> +			if (nsenter)
> +				nsinfo__mountns_exit(&nsc);
>  			continue;
> -
> -		if (nsexit)
> -			nsinfo__mountns_exit(&nsc);
> +		}
>  
>  		is_reg = is_regular_file(name);
>  
> @@ -1910,8 +1908,8 @@ int dso__load(struct dso *dso, struct map *map)
>  		if (is_reg && bfdrc < 0)
>  			sirc = symsrc__init(ss, dso, name, symtab_type);
>  
> -		if (nsexit)
> -			nsinfo__mountns_enter(dso__nsinfo(dso), &nsc);
> +		if (nsenter)
> +			nsinfo__mountns_exit(&nsc);

[Severity: Medium]
In NO_LIBELF=1 builds, will exiting the mount namespace here cause the
minimal dso__load_sym() to overwrite the DSO build-id with the host binary's
build-id?

After this loop finishes and the target mount namespace has been exited,
dso__load() calls dso__load_sym():

tools/perf/util/symbol.c:dso__load() {
	if (syms_ss)
		ret = dso__load_sym(dso, map, syms_ss, runtime_ss, kmod);
}

In tools/perf/util/symbol-minimal.c, dso__load_sym() opens ss->name by path
without entering dso__nsinfo(dso):

tools/perf/util/symbol-minimal.c:dso__load_sym() {
	struct build_id bid = { .size = 0, };
	int ret;

	ret = fd__is_64_bit(ss->fd);
	if (ret >= 0)
		RC_CHK_ACCESS(dso)->is_64_bit = ret;

	if (filename__read_build_id(ss->name, &bid) > 0)
		dso__set_build_id(dso, &bid);
	return 0;
}

If the container DSO path in ss->name also exists on the host (for example,
/lib64/libc.so.6), wouldn't filename__read_build_id() read the host file and
clobber the build-id that filename__read_build_id_ns() read from the target
namespace earlier in dso__load()?

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