[PATCH v1 00/11] perf tools: Enhance namespace and chroot handling

[PATCH v1 00/11] perf tools: Enhance namespace and chroot handling

[Namhyung Kim]

Hello,

This patchset improves namespace and chroot handling with regards to
file access.  Basically it reads the information from the proc file
system but it's only meaningful when you run the perf command on the
same machine and the target thread is still alive.

Also some file types don't need to namespace and chroot translation
because they are located in a global directory and expected to be in
the same namespace and root directory with the perf command itself.

Thanks,
Namhyung


Namhyung Kim (11):
  perf tools: Add HEADER_BOOT_ID feature
  perf tools: Add perf_env__is_same_boot()
  perf tools: Add nsinfo__new_thread() to track tid
  perf tools: Defer /proc lookup until nsinfo is used
  perf tools: Verify thread status and mount namespace inode
  perf tools: Check thread liveness when accessing /proc
  perf tools: Handle chroot inside nsinfo__mountns_enter()
  perf tools: Export filename__read_build_id_ns()
  perf tools: Remove redundant chroot handling
  perf tools: Only enter mount namespace if necessary
  perf tools: Scope mount namespace entry in dso__load()

 .../Documentation/perf.data-file-format.txt   |   5 +
 tools/perf/builtin-inject.c                   |  13 +-
 tools/perf/builtin-trace.c                    |   2 +-
 tools/perf/util/build-id.c                    |   6 +-
 tools/perf/util/capstone.c                    |   6 +-
 tools/perf/util/dso.c                         |  41 +---
 tools/perf/util/dso.h                         |  36 ++-
 tools/perf/util/dsos.c                        |  17 +-
 tools/perf/util/env.c                         |  65 +++++
 tools/perf/util/env.h                         |   3 +
 tools/perf/util/header.c                      |  28 +++
 tools/perf/util/header.h                      |   1 +
 tools/perf/util/machine.c                     |  12 +-
 tools/perf/util/namespaces.c                  | 230 +++++++++++++++---
 tools/perf/util/namespaces.h                  |  12 +
 tools/perf/util/symbol.c                      |  51 ++--
 tools/perf/util/symbol.h                      |   3 +
 tools/perf/util/thread.c                      |  39 ++-
 tools/perf/util/thread.h                      |   7 +-
 tools/perf/util/util.c                        |  31 ---
 tools/perf/util/util.h                        |   2 -
 21 files changed, 436 insertions(+), 174 deletions(-)

-- 
2.56.0.257.gf7497c7d27-goog

[PATCH v1 01/11] perf tools: Add HEADER_BOOT_ID feature

[Namhyung Kim]

Save /proc/sys/kernel/random/boot_id in the perf.data header and
struct perf_env so post-processing tools can identify if it's the
same kernel boot on which the data was recorded.

Assisted-by: LLM
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 .../Documentation/perf.data-file-format.txt   |  5 +++
 tools/perf/builtin-inject.c                   |  1 +
 tools/perf/util/env.c                         | 44 +++++++++++++++++++
 tools/perf/util/env.h                         |  2 +
 tools/perf/util/header.c                      | 28 ++++++++++++
 tools/perf/util/header.h                      |  1 +
 6 files changed, 81 insertions(+)

diff --git a/tools/perf/Documentation/perf.data-file-format.txt b/tools/perf/Documentation/perf.data-file-format.txt
index 1f8e7d8e9ae21d73..359d6ef0c80aac26 100644
--- a/tools/perf/Documentation/perf.data-file-format.txt
+++ b/tools/perf/Documentation/perf.data-file-format.txt
@@ -491,6 +491,11 @@ struct memory_range {
 	u8 remote_region_id;
 }; [nr_memory_ranges]
 
+	HEADER_BOOT_ID = 36,
+
+	A perf_header_string with the kernel boot ID read from
+	/proc/sys/kernel/random/boot_id.
+
 	other bits are reserved and should be ignored for now
 	HEADER_FEAT_BITS	= 256,
 
diff --git a/tools/perf/builtin-inject.c b/tools/perf/builtin-inject.c
index ce7562fc788c246e..ddb21d888e00d143 100644
--- a/tools/perf/builtin-inject.c
+++ b/tools/perf/builtin-inject.c
@@ -2307,6 +2307,7 @@ static bool keep_feat(struct perf_inject *inject, int feat)
 	case HEADER_CPU_DOMAIN_INFO:
 	case HEADER_CLN_SIZE:
 	case HEADER_MEMORY_RANGES:
+	case HEADER_BOOT_ID:
 		return true;
 	/* Information that can be updated */
 	case HEADER_BUILD_ID:
diff --git a/tools/perf/util/env.c b/tools/perf/util/env.c
index 50993b0dedc577c7..8ca0b677f3172b5f 100644
--- a/tools/perf/util/env.c
+++ b/tools/perf/util/env.c
@@ -12,6 +12,7 @@
 #include <linux/string.h>
 #include <linux/zalloc.h>
 #include "cgroup.h"
+#include <api/fs/fs.h>
 #include <errno.h>
 #include <sys/utsname.h>
 #include <stdlib.h>
@@ -258,6 +259,7 @@ void perf_env__exit(struct perf_env *env)
 	zfree(&env->os_release);
 	zfree(&env->version);
 	zfree(&env->arch);
+	zfree(&env->boot_id);
 	zfree(&env->cpu_desc);
 	zfree(&env->cpuid);
 	zfree(&env->cmdline);
@@ -400,6 +402,48 @@ const char *perf_env__os_release(struct perf_env *env)
 	return release;
 }
 
+static char *read_host_boot_id(void)
+{
+	char *buf = NULL;
+	size_t sz;
+
+	if (procfs__read_str("sys/kernel/random/boot_id", &buf, &sz) < 0)
+		return NULL;
+
+	strim(buf);
+	return buf;
+}
+
+const char *perf_env__boot_id(struct perf_env *env)
+{
+	const char *boot_id;
+
+	if (!env)
+		return NULL;
+
+	mutex_lock(&env->lock);
+	if (env->boot_id) {
+		boot_id = env->boot_id;
+		goto out;
+	}
+
+	/*
+	 * If env->arch or env->hostname is set, this is an offline target
+	 * environment. If the boot_id is not populated in the file, do not
+	 * fall back to the host's boot_id.
+	 */
+	if (env->arch || env->hostname) {
+		boot_id = NULL;
+		goto out;
+	}
+
+	env->boot_id = read_host_boot_id();
+	boot_id = env->boot_id;
+out:
+	mutex_unlock(&env->lock);
+	return boot_id;
+}
+
 int perf_env__set_cmdline(struct perf_env *env, int argc, const char *argv[])
 {
 	int i;
diff --git a/tools/perf/util/env.h b/tools/perf/util/env.h
index 23d35425efe38b24..772d11eed1bc2a7c 100644
--- a/tools/perf/util/env.h
+++ b/tools/perf/util/env.h
@@ -81,6 +81,7 @@ struct perf_env {
 	char			*os_release;
 	char			*version;
 	char			*arch;
+	char			*boot_id;
 	/* e_machine expanded from 16 to 32-bits for alignment. */
 	u32			e_machine;
 	u32			e_flags;
@@ -187,6 +188,7 @@ void perf_env__exit(struct perf_env *env);
 int perf_env__kernel_is_64_bit(struct perf_env *env);
 bool perf_arch_is_big_endian(const char *arch);
 const char *perf_env__os_release(struct perf_env *env);
+const char *perf_env__boot_id(struct perf_env *env);
 
 int perf_env__set_cmdline(struct perf_env *env, int argc, const char *argv[]);
 
diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index 9eec6deee190ff21..3e0b6fce55973dd1 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -494,6 +494,27 @@ static int write_version(struct feat_fd *ff,
 	return do_write_string(ff, perf_version_string);
 }
 
+static int write_boot_id(struct feat_fd *ff, struct evlist *evlist)
+{
+	const char *boot_id = NULL;
+	char *buf = NULL;
+	size_t sz;
+	int ret;
+
+	if (evlist__session(evlist))
+		boot_id = perf_env__boot_id(perf_session__env(evlist__session(evlist)));
+
+	if (!boot_id) {
+		if (procfs__read_str("sys/kernel/random/boot_id", &buf, &sz) < 0)
+			return -1;
+		boot_id = strim(buf);
+	}
+
+	ret = do_write_string(ff, boot_id);
+	free(buf);
+	return ret;
+}
+
 static int __write_cpudesc(struct feat_fd *ff, const char *cpuinfo_proc)
 {
 	FILE *file;
@@ -2079,6 +2100,11 @@ static void print_version(struct feat_fd *ff, FILE *fp)
 	fprintf(fp, "# perf version : %s\n", ff->ph->env.version);
 }
 
+static void print_boot_id(struct feat_fd *ff, FILE *fp)
+{
+	fprintf(fp, "# boot id : %s\n", ff->ph->env.boot_id);
+}
+
 static void print_cmdline(struct feat_fd *ff, FILE *fp)
 {
 	int nr, i;
@@ -3022,6 +3048,7 @@ FEAT_PROCESS_STR_FUN(osrelease, os_release);
 FEAT_PROCESS_STR_FUN(version, version);
 FEAT_PROCESS_STR_FUN(cpudesc, cpu_desc);
 FEAT_PROCESS_STR_FUN(cpuid, cpuid);
+FEAT_PROCESS_STR_FUN(boot_id, boot_id);
 
 static int process_arch(struct feat_fd *ff, void *data __maybe_unused)
 {
@@ -4496,6 +4523,7 @@ const struct perf_header_feature_ops feat_ops[HEADER_LAST_FEATURE] = {
 	FEAT_OPR(E_MACHINE,	e_machine,	false),
 	FEAT_OPR(CLN_SIZE,	cln_size,	false),
 	FEAT_OPR(MEMORY_RANGES,	memory_ranges,	false),
+	FEAT_OPR(BOOT_ID,	boot_id,	false),
 };
 
 struct header_print_data {
diff --git a/tools/perf/util/header.h b/tools/perf/util/header.h
index 765037762758ca11..5bbf16342b4d26d8 100644
--- a/tools/perf/util/header.h
+++ b/tools/perf/util/header.h
@@ -57,6 +57,7 @@ enum {
 	HEADER_E_MACHINE,
 	HEADER_CLN_SIZE,
 	HEADER_MEMORY_RANGES,
+	HEADER_BOOT_ID,
 	HEADER_LAST_FEATURE,
 	HEADER_FEAT_BITS	= 256,
 };
-- 
2.56.0.257.gf7497c7d27-goog

Re: [PATCH v1 01/11] perf tools: Add HEADER_BOOT_ID feature

[sashiko-bot]

> Save /proc/sys/kernel/random/boot_id in the perf.data header and
> struct perf_env so post-processing tools can identify if it's the
> same kernel boot on which the data was recorded.
> 
> Assisted-by: LLM
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Sashiko has reviewed this patch and found no issues.

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

[PATCH v1 02/11] perf tools: Add perf_env__is_same_boot()

[Namhyung Kim]

Add perf_env__is_same_boot() to compare the recorded boot_id in struct
perf_env against the running host's /proc/sys/kernel/random/boot_id.
For live sessions where no offline header was populated, it falls back
to reading the host boot_id and returns true.

Assisted-by: LLM
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/env.c | 21 +++++++++++++++++++++
 tools/perf/util/env.h |  1 +
 2 files changed, 22 insertions(+)

diff --git a/tools/perf/util/env.c b/tools/perf/util/env.c
index 8ca0b677f3172b5f..735db21d66a019a5 100644
--- a/tools/perf/util/env.c
+++ b/tools/perf/util/env.c
@@ -444,6 +444,27 @@ const char *perf_env__boot_id(struct perf_env *env)
 	return boot_id;
 }
 
+bool perf_env__is_same_boot(struct perf_env *env)
+{
+	static char *host_boot_id;
+	const char *env_boot_id;
+
+	if (!host_boot_id) {
+		host_boot_id = read_host_boot_id();
+		if (!host_boot_id)
+			return false;
+	}
+
+	if (!env)
+		return true;
+
+	env_boot_id = perf_env__boot_id(env);
+	if (!env_boot_id)
+		return false;
+
+	return !strcmp(env_boot_id, host_boot_id);
+}
+
 int perf_env__set_cmdline(struct perf_env *env, int argc, const char *argv[])
 {
 	int i;
diff --git a/tools/perf/util/env.h b/tools/perf/util/env.h
index 772d11eed1bc2a7c..df1d3d1ac3376e1e 100644
--- a/tools/perf/util/env.h
+++ b/tools/perf/util/env.h
@@ -189,6 +189,7 @@ int perf_env__kernel_is_64_bit(struct perf_env *env);
 bool perf_arch_is_big_endian(const char *arch);
 const char *perf_env__os_release(struct perf_env *env);
 const char *perf_env__boot_id(struct perf_env *env);
+bool perf_env__is_same_boot(struct perf_env *env);
 
 int perf_env__set_cmdline(struct perf_env *env, int argc, const char *argv[]);
 
-- 
2.56.0.257.gf7497c7d27-goog

Re: [PATCH v1 02/11] perf tools: Add perf_env__is_same_boot()

[sashiko-bot]

> Add perf_env__is_same_boot() to compare the recorded boot_id in struct
> perf_env against the running host's /proc/sys/kernel/random/boot_id.
> For live sessions where no offline header was populated, it falls back
> to reading the host boot_id and returns true.
> 
> Assisted-by: LLM
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Sashiko has reviewed this patch and found no issues.

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

[PATCH v1 03/11] perf tools: Add nsinfo__new_thread() to track tid

[Namhyung Kim]

Record the thread ID (tid) in struct nsinfo via nsinfo__new_thread() and
access /proc/<pid>/task/<tid>/ status and ns/mnt paths instead of only
/proc/<pid>/.  Also recreate thread->nsinfo when the pid of the thread
is updated.

Assisted-by: LLM
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/machine.c    |  3 +++
 tools/perf/util/namespaces.c | 22 ++++++++++++++++++----
 tools/perf/util/namespaces.h |  3 +++
 tools/perf/util/thread.c     |  2 +-
 4 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index 1d9d3bf57720008e..14f52234d4d99fb6 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -19,6 +19,7 @@
 #include "branch.h"
 #include "mem-events.h"
 #include "mem-info.h"
+#include "namespaces.h"
 #include "path.h"
 #include "srcline.h"
 #include "symbol.h"
@@ -495,6 +496,8 @@ static void machine__update_thread_pid(struct machine *machine,
 		return;
 
 	thread__set_pid(th, pid);
+	nsinfo__zput(RC_CHK_ACCESS(th)->nsinfo);
+	RC_CHK_ACCESS(th)->nsinfo = nsinfo__new_thread(pid, thread__tid(th));
 
 	if (thread__pid(th) == thread__tid(th))
 		return;
diff --git a/tools/perf/util/namespaces.c b/tools/perf/util/namespaces.c
index 01502570b32d02ab..f426eba04d6ef449 100644
--- a/tools/perf/util/namespaces.c
+++ b/tools/perf/util/namespaces.c
@@ -106,7 +106,8 @@ int nsinfo__init(struct nsinfo *nsi)
 	if (snprintf(oldns, PATH_MAX, "/proc/self/ns/mnt") >= PATH_MAX)
 		return rv;
 
-	if (asprintf(&newns, "/proc/%d/ns/mnt", nsinfo__pid(nsi)) == -1)
+	if (asprintf(&newns, "/proc/%d/task/%d/ns/mnt",
+		     nsinfo__pid(nsi), nsinfo__tid(nsi)) == -1)
 		return rv;
 
 	if (stat(oldns, &old_stat) < 0)
@@ -127,7 +128,8 @@ int nsinfo__init(struct nsinfo *nsi)
 	/* If we're dealing with a process that is in a different PID namespace,
 	 * attempt to work out the innermost tgid for the process.
 	 */
-	if (snprintf(spath, PATH_MAX, "/proc/%d/status", nsinfo__pid(nsi)) >= PATH_MAX)
+	if (snprintf(spath, PATH_MAX, "/proc/%d/task/%d/status",
+		     nsinfo__pid(nsi), nsinfo__tid(nsi)) >= PATH_MAX)
 		goto out;
 
 	rv = nsinfo__get_nspid(&RC_CHK_ACCESS(nsi)->tgid, &RC_CHK_ACCESS(nsi)->nstgid,
@@ -150,11 +152,11 @@ static struct nsinfo *nsinfo__alloc(void)
 	return res;
 }
 
-struct nsinfo *nsinfo__new(pid_t pid)
+struct nsinfo *nsinfo__new_thread(pid_t pid, pid_t tid)
 {
 	struct nsinfo *nsi;
 
-	if (pid == 0)
+	if (pid <= 0 || tid <= 0)
 		return NULL;
 
 	nsi = nsinfo__alloc();
@@ -162,6 +164,7 @@ struct nsinfo *nsinfo__new(pid_t pid)
 		return NULL;
 
 	RC_CHK_ACCESS(nsi)->pid = pid;
+	RC_CHK_ACCESS(nsi)->tid = tid;
 	RC_CHK_ACCESS(nsi)->tgid = pid;
 	RC_CHK_ACCESS(nsi)->nstgid = pid;
 	nsinfo__clear_need_setns(nsi);
@@ -176,6 +179,11 @@ struct nsinfo *nsinfo__new(pid_t pid)
 	return nsi;
 }
 
+struct nsinfo *nsinfo__new(pid_t pid)
+{
+	return nsinfo__new_thread(pid, pid);
+}
+
 static const char *nsinfo__mntns_path(const struct nsinfo *nsi)
 {
 	return RC_CHK_ACCESS(nsi)->mntns_path;
@@ -193,6 +201,7 @@ struct nsinfo *nsinfo__copy(const struct nsinfo *nsi)
 		return NULL;
 
 	RC_CHK_ACCESS(nnsi)->pid = nsinfo__pid(nsi);
+	RC_CHK_ACCESS(nnsi)->tid = nsinfo__tid(nsi);
 	RC_CHK_ACCESS(nnsi)->tgid = nsinfo__tgid(nsi);
 	RC_CHK_ACCESS(nnsi)->nstgid = nsinfo__nstgid(nsi);
 	RC_CHK_ACCESS(nnsi)->need_setns = nsinfo__need_setns(nsi);
@@ -265,6 +274,11 @@ pid_t nsinfo__pid(const struct nsinfo  *nsi)
 	return RC_CHK_ACCESS(nsi)->pid;
 }
 
+pid_t nsinfo__tid(const struct nsinfo  *nsi)
+{
+	return RC_CHK_ACCESS(nsi)->tid;
+}
+
 bool nsinfo__in_pidns(const struct nsinfo *nsi)
 {
 	return RC_CHK_ACCESS(nsi)->in_pidns;
diff --git a/tools/perf/util/namespaces.h b/tools/perf/util/namespaces.h
index e95c79b80e27c8fc..ea645954944de4be 100644
--- a/tools/perf/util/namespaces.h
+++ b/tools/perf/util/namespaces.h
@@ -32,6 +32,7 @@ void namespaces__free(struct namespaces *namespaces);
 
 DECLARE_RC_STRUCT(nsinfo) {
 	pid_t			pid;
+	pid_t			tid;
 	pid_t			tgid;
 	pid_t			nstgid;
 	bool			need_setns;
@@ -48,6 +49,7 @@ struct nscookie {
 
 int nsinfo__init(struct nsinfo *nsi);
 struct nsinfo *nsinfo__new(pid_t pid);
+struct nsinfo *nsinfo__new_thread(pid_t pid, pid_t tid);
 struct nsinfo *nsinfo__copy(const struct nsinfo *nsi);
 
 struct nsinfo *nsinfo__get(struct nsinfo *nsi);
@@ -58,6 +60,7 @@ void nsinfo__clear_need_setns(struct nsinfo *nsi);
 pid_t nsinfo__tgid(const struct nsinfo  *nsi);
 pid_t nsinfo__nstgid(const struct nsinfo  *nsi);
 pid_t nsinfo__pid(const struct nsinfo  *nsi);
+pid_t nsinfo__tid(const struct nsinfo  *nsi);
 bool nsinfo__in_pidns(const struct nsinfo  *nsi);
 void nsinfo__set_in_pidns(struct nsinfo *nsi);
 
diff --git a/tools/perf/util/thread.c b/tools/perf/util/thread.c
index 91118c301913b834..d12eb6bc63313b00 100644
--- a/tools/perf/util/thread.c
+++ b/tools/perf/util/thread.c
@@ -71,7 +71,7 @@ struct thread *thread__new(pid_t pid, pid_t tid)
 		list_add(&comm->list, thread__comm_list(thread));
 		refcount_set(thread__refcnt(thread), 1);
 		/* Thread holds first ref to nsdata. */
-		RC_CHK_ACCESS(thread)->nsinfo = nsinfo__new(pid);
+		RC_CHK_ACCESS(thread)->nsinfo = nsinfo__new_thread(pid, tid);
 		srccode_state_init(thread__srccode_state(thread));
 	}
 
-- 
2.56.0.257.gf7497c7d27-goog

Re: [PATCH v1 03/11] perf tools: Add nsinfo__new_thread() to track tid

[sashiko-bot]

> Record the thread ID (tid) in struct nsinfo via nsinfo__new_thread() and
> access /proc/<pid>/task/<tid>/ status and ns/mnt paths instead of only
> /proc/<pid>/.  Also recreate thread->nsinfo when the pid of the thread
> is updated.
> 
> Assisted-by: LLM
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Sashiko has reviewed this patch and found no issues.

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

[PATCH v1 04/11] perf tools: Defer /proc lookup until nsinfo is used

[Namhyung Kim]

Do not read /proc/<pid>/task/<tid>/ eagerly in nsinfo__new_thread().
Instead, initialize struct nsinfo on demand via nsinfo__ensure_init()
when namespace information is queried or entered.

Also reset the initialization state on exec and PERF_RECORD_NAMESPACES
events so namespace changes are picked up.

Assisted-by: LLM
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/namespaces.c | 46 ++++++++++++++++++++++++++++--------
 tools/perf/util/namespaces.h |  2 ++
 tools/perf/util/thread.c     |  4 ++++
 3 files changed, 42 insertions(+), 10 deletions(-)

diff --git a/tools/perf/util/namespaces.c b/tools/perf/util/namespaces.c
index f426eba04d6ef449..843d075d30409029 100644
--- a/tools/perf/util/namespaces.c
+++ b/tools/perf/util/namespaces.c
@@ -103,6 +103,11 @@ int nsinfo__init(struct nsinfo *nsi)
 	struct stat new_stat;
 	int rv = -1;
 
+	if (!nsi)
+		return -1;
+
+	RC_CHK_ACCESS(nsi)->inited = true;
+
 	if (snprintf(oldns, PATH_MAX, "/proc/self/ns/mnt") >= PATH_MAX)
 		return rv;
 
@@ -140,6 +145,12 @@ int nsinfo__init(struct nsinfo *nsi)
 	return rv;
 }
 
+static void nsinfo__ensure_init(struct nsinfo *nsi)
+{
+	if (nsi && !RC_CHK_ACCESS(nsi)->inited)
+		nsinfo__init(nsi);
+}
+
 static struct nsinfo *nsinfo__alloc(void)
 {
 	struct nsinfo *res;
@@ -167,15 +178,6 @@ struct nsinfo *nsinfo__new_thread(pid_t pid, pid_t tid)
 	RC_CHK_ACCESS(nsi)->tid = tid;
 	RC_CHK_ACCESS(nsi)->tgid = pid;
 	RC_CHK_ACCESS(nsi)->nstgid = pid;
-	nsinfo__clear_need_setns(nsi);
-	RC_CHK_ACCESS(nsi)->in_pidns = false;
-	/* Init may fail if the process exits while we're trying to look at its
-	 * proc information. In that case, save the pid but don't try to enter
-	 * the namespace.
-	 */
-	if (nsinfo__init(nsi) == -1)
-		nsinfo__clear_need_setns(nsi);
-
 	return nsi;
 }
 
@@ -196,6 +198,8 @@ struct nsinfo *nsinfo__copy(const struct nsinfo *nsi)
 	if (nsi == NULL)
 		return NULL;
 
+	nsinfo__ensure_init((struct nsinfo *)nsi);
+
 	nnsi = nsinfo__alloc();
 	if (!nnsi)
 		return NULL;
@@ -204,7 +208,8 @@ struct nsinfo *nsinfo__copy(const struct nsinfo *nsi)
 	RC_CHK_ACCESS(nnsi)->tid = nsinfo__tid(nsi);
 	RC_CHK_ACCESS(nnsi)->tgid = nsinfo__tgid(nsi);
 	RC_CHK_ACCESS(nnsi)->nstgid = nsinfo__nstgid(nsi);
-	RC_CHK_ACCESS(nnsi)->need_setns = nsinfo__need_setns(nsi);
+	RC_CHK_ACCESS(nnsi)->inited = RC_CHK_ACCESS(nsi)->inited;
+	RC_CHK_ACCESS(nnsi)->need_setns = RC_CHK_ACCESS(nsi)->need_setns;
 	RC_CHK_ACCESS(nnsi)->in_pidns = nsinfo__in_pidns(nsi);
 	if (nsinfo__mntns_path(nsi)) {
 		RC_CHK_ACCESS(nnsi)->mntns_path = strdup(nsinfo__mntns_path(nsi));
@@ -251,21 +256,41 @@ void nsinfo__put(struct nsinfo *nsi)
 
 bool nsinfo__need_setns(const struct nsinfo *nsi)
 {
+	if (!nsi)
+		return false;
+
+	nsinfo__ensure_init((struct nsinfo *)nsi);
 	return RC_CHK_ACCESS(nsi)->need_setns;
 }
 
 void nsinfo__clear_need_setns(struct nsinfo *nsi)
 {
+	if (!nsi)
+		return;
+
+	RC_CHK_ACCESS(nsi)->inited = true;
 	RC_CHK_ACCESS(nsi)->need_setns = false;
+	zfree(&RC_CHK_ACCESS(nsi)->mntns_path);
+}
+
+void nsinfo__reset_init(struct nsinfo *nsi)
+{
+	if (!nsi)
+		return;
+
+	nsinfo__clear_need_setns(nsi);
+	RC_CHK_ACCESS(nsi)->inited = false;
 }
 
 pid_t nsinfo__tgid(const struct nsinfo  *nsi)
 {
+	nsinfo__ensure_init((struct nsinfo *)nsi);
 	return RC_CHK_ACCESS(nsi)->tgid;
 }
 
 pid_t nsinfo__nstgid(const struct nsinfo  *nsi)
 {
+	nsinfo__ensure_init((struct nsinfo *)nsi);
 	return RC_CHK_ACCESS(nsi)->nstgid;
 }
 
@@ -281,6 +306,7 @@ pid_t nsinfo__tid(const struct nsinfo  *nsi)
 
 bool nsinfo__in_pidns(const struct nsinfo *nsi)
 {
+	nsinfo__ensure_init((struct nsinfo *)nsi);
 	return RC_CHK_ACCESS(nsi)->in_pidns;
 }
 
diff --git a/tools/perf/util/namespaces.h b/tools/perf/util/namespaces.h
index ea645954944de4be..77fe6f0eaf2f0618 100644
--- a/tools/perf/util/namespaces.h
+++ b/tools/perf/util/namespaces.h
@@ -35,6 +35,7 @@ DECLARE_RC_STRUCT(nsinfo) {
 	pid_t			tid;
 	pid_t			tgid;
 	pid_t			nstgid;
+	bool			inited;
 	bool			need_setns;
 	bool			in_pidns;
 	char			*mntns_path;
@@ -57,6 +58,7 @@ void nsinfo__put(struct nsinfo *nsi);
 
 bool nsinfo__need_setns(const struct nsinfo *nsi);
 void nsinfo__clear_need_setns(struct nsinfo *nsi);
+void nsinfo__reset_init(struct nsinfo *nsi);
 pid_t nsinfo__tgid(const struct nsinfo  *nsi);
 pid_t nsinfo__nstgid(const struct nsinfo  *nsi);
 pid_t nsinfo__pid(const struct nsinfo  *nsi);
diff --git a/tools/perf/util/thread.c b/tools/perf/util/thread.c
index d12eb6bc63313b00..377c39ec624bb7bb 100644
--- a/tools/perf/util/thread.c
+++ b/tools/perf/util/thread.c
@@ -199,6 +199,8 @@ int thread__set_namespaces(struct thread *thread, u64 timestamp,
 	down_write(thread__namespaces_lock(thread));
 	ret = __thread__set_namespaces(thread, timestamp, event);
 	up_write(thread__namespaces_lock(thread));
+	if (!ret && event)
+		nsinfo__reset_init(thread__nsinfo(thread));
 	return ret;
 }
 
@@ -282,6 +284,8 @@ int __thread__set_comm(struct thread *thread, const char *str, u64 timestamp,
 	down_write(thread__comm_lock(thread));
 	ret = ____thread__set_comm(thread, str, timestamp, exec);
 	up_write(thread__comm_lock(thread));
+	if (!ret && exec)
+		nsinfo__reset_init(thread__nsinfo(thread));
 	return ret;
 }
 
-- 
2.56.0.257.gf7497c7d27-goog

Re: [PATCH v1 04/11] perf tools: Defer /proc lookup until nsinfo is used

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] perf namespaces: eager /proc lookup in nsinfo__copy() on vdso maps
- [Medium] perf namespaces: stale PID namespace fields after nsinfo__reset_init()
- [Medium] perf namespaces: unsynchronized in-place mutation of shared nsinfo
--

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

perf tools: Defer /proc lookup until nsinfo is used

This commit defers reading /proc/<pid>/task/<tid>/ in nsinfo__new_thread()
until namespace information is queried or entered via nsinfo__ensure_init(),
and resets the initialization state on exec and PERF_RECORD_NAMESPACES events.

> diff --git a/tools/perf/util/namespaces.c b/tools/perf/util/namespaces.c
> index f426eba04d6ef..843d075d30409 100644
> --- a/tools/perf/util/namespaces.c
> +++ b/tools/perf/util/namespaces.c

[ ... ]

> @@ -196,6 +198,8 @@ struct nsinfo *nsinfo__copy(const struct nsinfo *nsi)
>  	if (nsi == NULL)
>  		return NULL;
>  
> +	nsinfo__ensure_init((struct nsinfo *)nsi);
> +
>  	nnsi = nsinfo__alloc();
>  	if (!nnsi)
>  		return NULL;
> @@ -204,7 +208,8 @@ struct nsinfo *nsinfo__copy(const struct nsinfo *nsi)
>  	RC_CHK_ACCESS(nnsi)->tid = nsinfo__tid(nsi);
>  	RC_CHK_ACCESS(nnsi)->tgid = nsinfo__tgid(nsi);
>  	RC_CHK_ACCESS(nnsi)->nstgid = nsinfo__nstgid(nsi);
> -	RC_CHK_ACCESS(nnsi)->need_setns = nsinfo__need_setns(nsi);
> +	RC_CHK_ACCESS(nnsi)->inited = RC_CHK_ACCESS(nsi)->inited;
> +	RC_CHK_ACCESS(nnsi)->need_setns = RC_CHK_ACCESS(nsi)->need_setns;
>  	RC_CHK_ACCESS(nnsi)->in_pidns = nsinfo__in_pidns(nsi);
>  	if (nsinfo__mntns_path(nsi)) {
>  		RC_CHK_ACCESS(nnsi)->mntns_path = strdup(nsinfo__mntns_path(nsi));

[Severity: Medium]
Should nsinfo__copy() copy the raw fields of struct nsinfo without calling
nsinfo__ensure_init(), nsinfo__tgid(), nsinfo__nstgid(), and
nsinfo__in_pidns()?

When a PERF_RECORD_MMAP or PERF_RECORD_MMAP2 event for a [vdso] mapping is
processed in tools/perf/util/map.c:map__new() or
tools/perf/builtin-inject.c:dso__findnew_id(), perf copies the thread's
nsinfo and immediately clears need_setns on the copy:

	if (vdso) {
		nnsi = nsinfo__copy(nsi);
		if (nnsi) {
			nsinfo__put(nsi);
			nsinfo__clear_need_setns(nnsi);
			nsi = nnsi;
		}
	}

Because nsinfo__copy() calls nsinfo__ensure_init() on the source nsinfo (as
well as the getter helpers that also call nsinfo__ensure_init()), any process
that maps [vdso] will still trigger an eager /proc/<pid>/task/<tid>/ns/mnt
and /proc/<pid>/task/<tid>/status lookup and duplicate mntns_path only for
nsinfo__clear_need_setns() to immediately free it, even when namespace
information is never subsequently used.

> @@ -251,21 +256,41 @@ void nsinfo__put(struct nsinfo *nsi)
>  
>  bool nsinfo__need_setns(const struct nsinfo *nsi)
>  {
> +	if (!nsi)
> +		return false;
> +
> +	nsinfo__ensure_init((struct nsinfo *)nsi);
>  	return RC_CHK_ACCESS(nsi)->need_setns;
>  }
>  
>  void nsinfo__clear_need_setns(struct nsinfo *nsi)
>  {
> +	if (!nsi)
> +		return;
> +
> +	RC_CHK_ACCESS(nsi)->inited = true;
>  	RC_CHK_ACCESS(nsi)->need_setns = false;
> +	zfree(&RC_CHK_ACCESS(nsi)->mntns_path);
> +}
> +
> +void nsinfo__reset_init(struct nsinfo *nsi)
> +{
> +	if (!nsi)
> +		return;
> +
> +	nsinfo__clear_need_setns(nsi);
> +	RC_CHK_ACCESS(nsi)->inited = false;
>  }

[Severity: Medium]
Does nsinfo__reset_init() leave stale PID namespace values in in_pidns, tgid,
and nstgid?

When nsinfo__new_thread() allocates a struct nsinfo, tgid and nstgid are set
to pid and in_pidns is false. Once nsinfo__init() (or nsinfo__set_in_pidns())
populates tgid, nstgid, and in_pidns for a task in a PID namespace, a
subsequent exec or PERF_RECORD_NAMESPACES event calls nsinfo__reset_init(),
which only clears need_setns, mntns_path, and inited.

If the task exits or becomes inaccessible in /proc before the next
nsinfo__ensure_init() call, nsinfo__init() sets inited = true at the start of
the function and fails before or inside nsinfo__get_nspid():

tools/perf/util/namespaces.c:nsinfo__init() {
	...
	RC_CHK_ACCESS(nsi)->inited = true;
	...
	if (stat(newns, &new_stat) < 0)
		goto out;
	...
	rv = nsinfo__get_nspid(&RC_CHK_ACCESS(nsi)->tgid, &RC_CHK_ACCESS(nsi)->nstgid,
			       &RC_CHK_ACCESS(nsi)->in_pidns, spath);
out:
	free(newns);
	return rv;
}

In that case, in_pidns, tgid, and nstgid will retain their stale pre-reset
values instead of reverting to false and pid.

> diff --git a/tools/perf/util/thread.c b/tools/perf/util/thread.c
> index d12eb6bc63313..377c39ec624bb 100644
> --- a/tools/perf/util/thread.c
> +++ b/tools/perf/util/thread.c
> @@ -199,6 +199,8 @@ int thread__set_namespaces(struct thread *thread, u64 timestamp,
>  	down_write(thread__namespaces_lock(thread));
>  	ret = __thread__set_namespaces(thread, timestamp, event);
>  	up_write(thread__namespaces_lock(thread));
> +	if (!ret && event)
> +		nsinfo__reset_init(thread__nsinfo(thread));
>  	return ret;
>  }
>  
> @@ -282,6 +284,8 @@ int __thread__set_comm(struct thread *thread, const char *str, u64 timestamp,
>  	down_write(thread__comm_lock(thread));
>  	ret = ____thread__set_comm(thread, str, timestamp, exec);
>  	up_write(thread__comm_lock(thread));
> +	if (!ret && exec)
> +		nsinfo__reset_init(thread__nsinfo(thread));
>  	return ret;
>  }

[Severity: Medium]
Can mutating thread__nsinfo(thread) in-place via nsinfo__reset_init() and
nsinfo__ensure_init() cause a use-after-free on mntns_path or corrupt DSO
namespace state?

In tools/perf/util/map.c:map__new() and
tools/perf/builtin-inject.c:dso__findnew_id(), dso->nsinfo takes a reference
to thread__nsinfo(thread) via nsinfo__get(), so the same struct nsinfo
instance is shared between the thread and its DSOs.

In multithreaded modes such as perf top (where display_thread resolves
symbols via nsinfo__mountns_enter() while process_thread handles
PERF_RECORD_COMM and PERF_RECORD_NAMESPACES events) or multithreaded event
synthesis, one thread can check nsinfo__need_setns() and then pass
nsinfo__mntns_path(nsi) to open() or strdup():

tools/perf/util/namespaces.c:nsinfo__mountns_enter() {
	if (!nsi || !nsinfo__need_setns(nsi))
		return;
	...
	newns = open(nsinfo__mntns_path(nsi), O_RDONLY);
}

while another thread calls thread__set_namespaces() or __thread__set_comm()
-> nsinfo__reset_init() -> nsinfo__clear_need_setns() ->
zfree(&RC_CHK_ACCESS(nsi)->mntns_path) without synchronization, freeing and
clearing mntns_path concurrently.

Similarly, nsinfo__init() sets RC_CHK_ACCESS(nsi)->inited = true at the
beginning of the function before setting need_setns, mntns_path, tgid,
nstgid, and in_pidns, so a concurrent caller of nsinfo__ensure_init() can
observe inited == true and read partially initialized fields (or two threads
can enter nsinfo__init() concurrently and leak the first mntns_path
allocation).

Furthermore, even in single-threaded mode, resetting thread__nsinfo(thread)
in-place mutates the struct nsinfo already attached to DSOs mapped by the
thread prior to the namespace change or exec. Would it be safer to replace
thread->nsinfo with a newly allocated struct nsinfo on namespace changes and
synchronize lazy initialization inside nsinfo__ensure_init()?

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

[PATCH v1 05/11] perf tools: Verify thread status and mount namespace inode

[Namhyung Kim]

Verify /proc/<pid>/task/<tid>/status exists and has Tgid == pid before
reading or entering a thread's namespaces to guard against PID/TID
recycling.

Also record the mount namespace inode (mntns_ino) during initialization
and verify via fstat() that the opened namespace file descriptor still
refers to the same mount namespace.

Assisted-by: LLM
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/namespaces.c | 120 +++++++++++++++++++++++++----------
 tools/perf/util/namespaces.h |   4 ++
 2 files changed, 91 insertions(+), 33 deletions(-)

diff --git a/tools/perf/util/namespaces.c b/tools/perf/util/namespaces.c
index 843d075d30409029..bb119a6cc6064d89 100644
--- a/tools/perf/util/namespaces.c
+++ b/tools/perf/util/namespaces.c
@@ -65,6 +65,7 @@ static int nsinfo__get_nspid(pid_t *tgid, pid_t *nstgid, bool *in_pidns, const c
 	char *statln = NULL;
 	size_t linesz = 0;
 	char *nspid;
+	bool got_tgid = false;
 
 	f = fopen(path, "r");
 	if (f == NULL)
@@ -75,6 +76,7 @@ static int nsinfo__get_nspid(pid_t *tgid, pid_t *nstgid, bool *in_pidns, const c
 		if (strstr(statln, "Tgid:") != NULL) {
 			*tgid = (pid_t)strtol(strrchr(statln, '\t'), NULL, 10);
 			*nstgid = *tgid;
+			got_tgid = true;
 		}
 
 		if (strstr(statln, "NStgid:") != NULL) {
@@ -91,7 +93,7 @@ static int nsinfo__get_nspid(pid_t *tgid, pid_t *nstgid, bool *in_pidns, const c
 
 	fclose(f);
 	free(statln);
-	return 0;
+	return got_tgid ? 0 : -1;
 }
 
 int nsinfo__init(struct nsinfo *nsi)
@@ -103,17 +105,34 @@ int nsinfo__init(struct nsinfo *nsi)
 	struct stat new_stat;
 	int rv = -1;
 
-	if (!nsi)
+	if (!nsi || RC_CHK_ACCESS(nsi)->exited)
 		return -1;
 
 	RC_CHK_ACCESS(nsi)->inited = true;
 
+	/*
+	 * Verify that the thread (pid, tid) still exists in /proc and its Tgid
+	 * matches pid (guarding against PID/TID recycling), while resolving the
+	 * innermost tgid if the process is in a different PID namespace.
+	 */
+	if (snprintf(spath, PATH_MAX, "/proc/%d/task/%d/status",
+		     nsinfo__pid(nsi), nsinfo__tid(nsi)) >= PATH_MAX)
+		goto out;
+
+	rv = nsinfo__get_nspid(&RC_CHK_ACCESS(nsi)->tgid, &RC_CHK_ACCESS(nsi)->nstgid,
+			       &RC_CHK_ACCESS(nsi)->in_pidns, spath);
+	if (rv < 0 || RC_CHK_ACCESS(nsi)->tgid != nsinfo__pid(nsi)) {
+		nsinfo__set_exited(nsi);
+		rv = -1;
+		goto out;
+	}
+
 	if (snprintf(oldns, PATH_MAX, "/proc/self/ns/mnt") >= PATH_MAX)
-		return rv;
+		return -1;
 
 	if (asprintf(&newns, "/proc/%d/task/%d/ns/mnt",
 		     nsinfo__pid(nsi), nsinfo__tid(nsi)) == -1)
-		return rv;
+		return -1;
 
 	if (stat(oldns, &old_stat) < 0)
 		goto out;
@@ -127,19 +146,10 @@ int nsinfo__init(struct nsinfo *nsi)
 	if (old_stat.st_ino != new_stat.st_ino) {
 		RC_CHK_ACCESS(nsi)->need_setns = true;
 		RC_CHK_ACCESS(nsi)->mntns_path = newns;
+		RC_CHK_ACCESS(nsi)->mntns_ino = new_stat.st_ino;
 		newns = NULL;
 	}
 
-	/* If we're dealing with a process that is in a different PID namespace,
-	 * attempt to work out the innermost tgid for the process.
-	 */
-	if (snprintf(spath, PATH_MAX, "/proc/%d/task/%d/status",
-		     nsinfo__pid(nsi), nsinfo__tid(nsi)) >= PATH_MAX)
-		goto out;
-
-	rv = nsinfo__get_nspid(&RC_CHK_ACCESS(nsi)->tgid, &RC_CHK_ACCESS(nsi)->nstgid,
-			       &RC_CHK_ACCESS(nsi)->in_pidns, spath);
-
 out:
 	free(newns);
 	return rv;
@@ -147,7 +157,7 @@ int nsinfo__init(struct nsinfo *nsi)
 
 static void nsinfo__ensure_init(struct nsinfo *nsi)
 {
-	if (nsi && !RC_CHK_ACCESS(nsi)->inited)
+	if (nsi && !RC_CHK_ACCESS(nsi)->inited && !RC_CHK_ACCESS(nsi)->exited)
 		nsinfo__init(nsi);
 }
 
@@ -208,7 +218,9 @@ struct nsinfo *nsinfo__copy(const struct nsinfo *nsi)
 	RC_CHK_ACCESS(nnsi)->tid = nsinfo__tid(nsi);
 	RC_CHK_ACCESS(nnsi)->tgid = nsinfo__tgid(nsi);
 	RC_CHK_ACCESS(nnsi)->nstgid = nsinfo__nstgid(nsi);
+	RC_CHK_ACCESS(nnsi)->mntns_ino = RC_CHK_ACCESS(nsi)->mntns_ino;
 	RC_CHK_ACCESS(nnsi)->inited = RC_CHK_ACCESS(nsi)->inited;
+	RC_CHK_ACCESS(nnsi)->exited = RC_CHK_ACCESS(nsi)->exited;
 	RC_CHK_ACCESS(nnsi)->need_setns = RC_CHK_ACCESS(nsi)->need_setns;
 	RC_CHK_ACCESS(nnsi)->in_pidns = nsinfo__in_pidns(nsi);
 	if (nsinfo__mntns_path(nsi)) {
@@ -260,7 +272,7 @@ bool nsinfo__need_setns(const struct nsinfo *nsi)
 		return false;
 
 	nsinfo__ensure_init((struct nsinfo *)nsi);
-	return RC_CHK_ACCESS(nsi)->need_setns;
+	return !RC_CHK_ACCESS(nsi)->exited && RC_CHK_ACCESS(nsi)->need_setns;
 }
 
 void nsinfo__clear_need_setns(struct nsinfo *nsi)
@@ -273,15 +285,44 @@ void nsinfo__clear_need_setns(struct nsinfo *nsi)
 	zfree(&RC_CHK_ACCESS(nsi)->mntns_path);
 }
 
-void nsinfo__reset_init(struct nsinfo *nsi)
+void nsinfo__set_exited(struct nsinfo *nsi)
 {
 	if (!nsi)
 		return;
 
+	RC_CHK_ACCESS(nsi)->exited = true;
+	nsinfo__clear_need_setns(nsi);
+}
+
+void nsinfo__reset_init(struct nsinfo *nsi)
+{
+	if (!nsi || RC_CHK_ACCESS(nsi)->exited)
+		return;
+
 	nsinfo__clear_need_setns(nsi);
 	RC_CHK_ACCESS(nsi)->inited = false;
 }
 
+bool nsinfo__check_proc(struct nsinfo *nsi)
+{
+	char spath[64];
+	pid_t tgid = 0, nstgid = 0;
+	bool in_pidns = false;
+
+	if (!nsi || RC_CHK_ACCESS(nsi)->exited)
+		return false;
+
+	scnprintf(spath, sizeof(spath), "/proc/%d/task/%d/status",
+		  nsinfo__pid(nsi), nsinfo__tid(nsi));
+	if (nsinfo__get_nspid(&tgid, &nstgid, &in_pidns, spath) < 0 ||
+	    tgid != nsinfo__pid(nsi)) {
+		nsinfo__set_exited(nsi);
+		return false;
+	}
+
+	return true;
+}
+
 pid_t nsinfo__tgid(const struct nsinfo  *nsi)
 {
 	nsinfo__ensure_init((struct nsinfo *)nsi);
@@ -328,10 +369,14 @@ void nsinfo__mountns_enter(struct nsinfo *nsi,
 
 	nc->oldns = -1;
 	nc->newns = -1;
+	nc->oldcwd = NULL;
 
 	if (!nsi || !nsinfo__need_setns(nsi))
 		return;
 
+	if (!nsinfo__check_proc(nsi))
+		return;
+
 	if (!getcwd(curpath, sizeof(curpath)))
 		return;
 
@@ -339,16 +384,26 @@ void nsinfo__mountns_enter(struct nsinfo *nsi,
 	if (!oldcwd)
 		return;
 
-	oldns = open("/proc/self/ns/mnt", O_RDONLY);
-	if (oldns < 0)
-		goto errout;
+	if (RC_CHK_ACCESS(nsi)->need_setns) {
+		struct stat st;
+
+		oldns = open("/proc/self/ns/mnt", O_RDONLY | O_CLOEXEC);
+		if (oldns < 0)
+			goto errout;
 
-	newns = open(nsinfo__mntns_path(nsi), O_RDONLY);
-	if (newns < 0)
-		goto errout;
+		newns = open(nsinfo__mntns_path(nsi), O_RDONLY | O_CLOEXEC);
+		if (newns < 0)
+			goto errout;
 
-	if (setns(newns, CLONE_NEWNS) < 0)
-		goto errout;
+		if (fstat(newns, &st) < 0 ||
+		    st.st_ino != RC_CHK_ACCESS(nsi)->mntns_ino) {
+			nsinfo__set_exited(nsi);
+			goto errout;
+		}
+
+		if (setns(newns, CLONE_NEWNS) < 0)
+			goto errout;
+	}
 
 	nc->oldcwd = oldcwd;
 	nc->oldns = oldns;
@@ -365,17 +420,11 @@ void nsinfo__mountns_enter(struct nsinfo *nsi,
 
 void nsinfo__mountns_exit(struct nscookie *nc)
 {
-	if (nc == NULL || nc->oldns == -1 || nc->newns == -1 || !nc->oldcwd)
+	if (nc == NULL || !nc->oldcwd)
 		return;
 
-	setns(nc->oldns, CLONE_NEWNS);
-
-	if (nc->oldcwd) {
-		WARN_ON_ONCE(chdir(nc->oldcwd));
-		zfree(&nc->oldcwd);
-	}
-
 	if (nc->oldns > -1) {
+		setns(nc->oldns, CLONE_NEWNS);
 		close(nc->oldns);
 		nc->oldns = -1;
 	}
@@ -384,6 +433,11 @@ void nsinfo__mountns_exit(struct nscookie *nc)
 		close(nc->newns);
 		nc->newns = -1;
 	}
+
+	if (nc->oldcwd) {
+		WARN_ON_ONCE(chdir(nc->oldcwd));
+		zfree(&nc->oldcwd);
+	}
 }
 
 char *nsinfo__realpath(const char *path, struct nsinfo *nsi)
diff --git a/tools/perf/util/namespaces.h b/tools/perf/util/namespaces.h
index 77fe6f0eaf2f0618..165e9c9994461595 100644
--- a/tools/perf/util/namespaces.h
+++ b/tools/perf/util/namespaces.h
@@ -35,7 +35,9 @@ DECLARE_RC_STRUCT(nsinfo) {
 	pid_t			tid;
 	pid_t			tgid;
 	pid_t			nstgid;
+	ino_t			mntns_ino;
 	bool			inited;
+	bool			exited;
 	bool			need_setns;
 	bool			in_pidns;
 	char			*mntns_path;
@@ -58,7 +60,9 @@ void nsinfo__put(struct nsinfo *nsi);
 
 bool nsinfo__need_setns(const struct nsinfo *nsi);
 void nsinfo__clear_need_setns(struct nsinfo *nsi);
+void nsinfo__set_exited(struct nsinfo *nsi);
 void nsinfo__reset_init(struct nsinfo *nsi);
+bool nsinfo__check_proc(struct nsinfo *nsi);
 pid_t nsinfo__tgid(const struct nsinfo  *nsi);
 pid_t nsinfo__nstgid(const struct nsinfo  *nsi);
 pid_t nsinfo__pid(const struct nsinfo  *nsi);
-- 
2.56.0.257.gf7497c7d27-goog

Re: [PATCH v1 05/11] perf tools: Verify thread status and mount namespace inode

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] perf namespaces: nsinfo__init() clobbers tgid and pidns on TID recycling
- [Medium] perf namespaces: nsinfo__mountns_enter() sets exited on mntns inode change
--

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

perf tools: Verify thread status and mount namespace inode

This commit verifies that /proc/<pid>/task/<tid>/status exists and has a
matching Tgid before accessing a thread's namespaces to guard against PID/TID
recycling, and records the mount namespace inode to verify it via fstat() when
entering the namespace.

> diff --git a/tools/perf/util/namespaces.c b/tools/perf/util/namespaces.c
> index 843d075d30409..bb119a6cc6064 100644
> --- a/tools/perf/util/namespaces.c
> +++ b/tools/perf/util/namespaces.c

[ ... ]

> @@ -103,17 +105,34 @@ int nsinfo__init(struct nsinfo *nsi)
>  	struct stat new_stat;
>  	int rv = -1;
>  
> -	if (!nsi)
> +	if (!nsi || RC_CHK_ACCESS(nsi)->exited)
>  		return -1;
>  
>  	RC_CHK_ACCESS(nsi)->inited = true;
>  
> +	/*
> +	 * Verify that the thread (pid, tid) still exists in /proc and its Tgid
> +	 * matches pid (guarding against PID/TID recycling), while resolving the
> +	 * innermost tgid if the process is in a different PID namespace.
> +	 */
> +	if (snprintf(spath, PATH_MAX, "/proc/%d/task/%d/status",
> +		     nsinfo__pid(nsi), nsinfo__tid(nsi)) >= PATH_MAX)
> +		goto out;
> +
> +	rv = nsinfo__get_nspid(&RC_CHK_ACCESS(nsi)->tgid, &RC_CHK_ACCESS(nsi)->nstgid,
> +			       &RC_CHK_ACCESS(nsi)->in_pidns, spath);
> +	if (rv < 0 || RC_CHK_ACCESS(nsi)->tgid != nsinfo__pid(nsi)) {
> +		nsinfo__set_exited(nsi);
> +		rv = -1;
> +		goto out;
> +	}

[Severity: Medium]
Can this overwrite tgid, nstgid, and in_pidns with an unrelated process's
values when PID/TID recycling is detected?

In nsinfo__new_thread(pid, tid), tgid and nstgid are initialized to pid and
in_pidns to false as fallback values in case the thread exits before /proc is
queried.

If a process with pid == tid exits before lazy initialization in
nsinfo__init(), and its ID is recycled as a non-leader thread of an unrelated
process P2, /proc/<pid>/task/<tid>/status still resolves in procfs because
both refer to the same thread group.

In that case, nsinfo__get_nspid() succeeds and writes P2's Tgid, NStgid, and
in_pidns directly into nsi before the tgid != nsinfo__pid(nsi) check detects
the mismatch. Calling nsinfo__set_exited(nsi) marks exited = true and clears
need_setns, but leaves the overwritten tgid, nstgid, and in_pidns in place.

Downstream callers that query an exited nsinfo via nsinfo__tgid(),
nsinfo__nstgid(), nsinfo__in_pidns(), or nsinfo__copy(), such as
dso__find_perf_map() in tools/perf/util/symbol.c:

	if (!nsinfo__need_setns(nsi)) {
		...
		nsinfo__clear_need_setns(nnsi);
		snprintf(filebuf, bufsz, "/tmp/perf-%d.map", nsinfo__tgid(nnsi));

as well as jr_entry_pid() and jit_detect() in tools/perf/util/jitdump.c, will
then use P2's identifiers instead of the original process's pid.

Should nsinfo__init() read into local variables first, similar to
nsinfo__check_proc(), and only update nsi after verifying that tgid matches
nsinfo__pid(nsi)?

[ ... ]

> @@ -273,15 +285,44 @@ void nsinfo__clear_need_setns(struct nsinfo *nsi)
>  	zfree(&RC_CHK_ACCESS(nsi)->mntns_path);
>  }
>  
> -void nsinfo__reset_init(struct nsinfo *nsi)
> +void nsinfo__set_exited(struct nsinfo *nsi)
>  {
>  	if (!nsi)
>  		return;
>  
> +	RC_CHK_ACCESS(nsi)->exited = true;
> +	nsinfo__clear_need_setns(nsi);
> +}
> +
> +void nsinfo__reset_init(struct nsinfo *nsi)
> +{
> +	if (!nsi || RC_CHK_ACCESS(nsi)->exited)
> +		return;
> +
>  	nsinfo__clear_need_setns(nsi);
>  	RC_CHK_ACCESS(nsi)->inited = false;
>  }

[ ... ]

> @@ -339,16 +384,26 @@ void nsinfo__mountns_enter(struct nsinfo *nsi,
>  	if (!oldcwd)
>  		return;
>  
> -	oldns = open("/proc/self/ns/mnt", O_RDONLY);
> -	if (oldns < 0)
> -		goto errout;
> +	if (RC_CHK_ACCESS(nsi)->need_setns) {
> +		struct stat st;
> +
> +		oldns = open("/proc/self/ns/mnt", O_RDONLY | O_CLOEXEC);
> +		if (oldns < 0)
> +			goto errout;
>  
> -	newns = open(nsinfo__mntns_path(nsi), O_RDONLY);
> -	if (newns < 0)
> -		goto errout;
> +		newns = open(nsinfo__mntns_path(nsi), O_RDONLY | O_CLOEXEC);
> +		if (newns < 0)
> +			goto errout;
>  
> -	if (setns(newns, CLONE_NEWNS) < 0)
> -		goto errout;
> +		if (fstat(newns, &st) < 0 ||
> +		    st.st_ino != RC_CHK_ACCESS(nsi)->mntns_ino) {
> +			nsinfo__set_exited(nsi);
> +			goto errout;
> +		}

[Severity: Medium]
What happens if a live thread changes its mount namespace via
unshare(CLONE_NEWNS) or setns(..., CLONE_NEWNS)?

If a thread in a non-root mount namespace is initialized via nsinfo__init()
and later changes its mount namespace while still alive, procfs immediately
updates the inode of /proc/<pid>/task/<tid>/ns/mnt.

If perf symbolizes an earlier sample or loads a DSO via
dso__load() -> nsinfo__mountns_enter() before the corresponding
PERF_RECORD_NAMESPACES or exec PERF_RECORD_COMM event is processed,
nsinfo__check_proc(nsi) succeeds because the thread is still alive with
Tgid == pid, but fstat(newns, &st) sees st.st_ino != nsi->mntns_ino and calls
nsinfo__set_exited(nsi).

Because dso->nsinfo shares the thread's struct nsinfo via
nsinfo__get(thread__nsinfo(thread)) in map__new(), setting exited = true
causes subsequent calls to nsinfo__reset_init() in thread__set_namespaces()
and __thread__set_comm() to return early without resetting inited, permanently
disabling mount namespace entry and nsinfo__check_proc() checks for the rest
of the thread's lifetime.

Should a mount namespace inode mismatch here call nsinfo__clear_need_setns(nsi)
instead of nsinfo__set_exited(nsi) so that a subsequent namespace or exec
event can still re-initialize the thread's nsinfo?

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

[PATCH v1 06/11] perf tools: Check thread liveness when accessing /proc

[Namhyung Kim]

Add thread__exited() and thread__is_alive(), and mark threads as exited
on PERF_RECORD_EXIT and replaced threads on PERF_RECORD_FORK.

Propagate thread exit and boot ID mismatch to thread->nsinfo, and guard
/proc accesses with thread__is_alive().

Assisted-by: LLM
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-trace.c |  2 +-
 tools/perf/util/machine.c  |  9 ++++++---
 tools/perf/util/thread.c   | 33 ++++++++++++++++++++++++++++++++-
 tools/perf/util/thread.h   |  7 +++++--
 4 files changed, 44 insertions(+), 7 deletions(-)

diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
index 4094141461f91c32..a21bbf5926617633 100644
--- a/tools/perf/builtin-trace.c
+++ b/tools/perf/builtin-trace.c
@@ -1858,7 +1858,7 @@ static const char *thread__fd_path(struct thread *thread, int fd,
 		return NULL;
 
 	if ((fd > ttrace->files.max || ttrace->files.table[fd].pathname == NULL)) {
-		if (!trace->live)
+		if (!trace->live || !thread__is_alive(thread))
 			return NULL;
 		++trace->stats.proc_getname;
 		if (thread__read_fd_path(thread, fd))
diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index 14f52234d4d99fb6..8f6298aff681f3fc 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -498,6 +498,9 @@ static void machine__update_thread_pid(struct machine *machine,
 	thread__set_pid(th, pid);
 	nsinfo__zput(RC_CHK_ACCESS(th)->nsinfo);
 	RC_CHK_ACCESS(th)->nsinfo = nsinfo__new_thread(pid, thread__tid(th));
+	if (thread__exited(th) || !machine__is_host(machine) ||
+	    !perf_env__is_same_boot(machine->env))
+		nsinfo__set_exited(thread__nsinfo(th));
 
 	if (thread__pid(th) == thread__tid(th))
 		return;
@@ -1954,6 +1957,7 @@ int machine__process_fork_event(struct machine *machine, union perf_event *event
 
 	/* if a thread currently exists for the thread id remove it */
 	if (thread != NULL) {
+		thread__set_exited(thread, /*exited=*/true);
 		machine__remove_thread(machine, thread);
 		thread__put(thread);
 	}
@@ -2001,9 +2005,8 @@ int machine__process_exit_event(struct machine *machine, union perf_event *event
 	/* There is no context switch out before exit, so we decrement here. */
 	machine->parallelism--;
 	if (thread != NULL) {
-		if (symbol_conf.keep_exited_threads)
-			thread__set_exited(thread, /*exited=*/true);
-		else
+		thread__set_exited(thread, /*exited=*/true);
+		if (!symbol_conf.keep_exited_threads)
 			machine__remove_thread(machine, thread);
 	}
 	thread__put(thread);
diff --git a/tools/perf/util/thread.c b/tools/perf/util/thread.c
index 377c39ec624bb7bb..f75eafb25bfc06cd 100644
--- a/tools/perf/util/thread.c
+++ b/tools/perf/util/thread.c
@@ -37,6 +37,10 @@ int thread__init_maps(struct thread *thread, struct machine *machine)
 		}
 	}
 
+	if (!machine || !machine__is_host(machine) ||
+	    !perf_env__is_same_boot(machine->env))
+		nsinfo__set_exited(thread__nsinfo(thread));
+
 	return thread__maps(thread) ? 0 : -1;
 }
 
@@ -289,6 +293,30 @@ int __thread__set_comm(struct thread *thread, const char *str, u64 timestamp,
 	return ret;
 }
 
+void thread__set_exited(struct thread *thread, bool exited)
+{
+	RC_CHK_ACCESS(thread)->exited = exited;
+	if (exited)
+		nsinfo__set_exited(thread__nsinfo(thread));
+}
+
+bool thread__is_alive(struct thread *thread)
+{
+	struct maps *maps;
+	struct machine *machine;
+
+	if (!thread || thread__exited(thread))
+		return false;
+
+	maps = thread__maps(thread);
+	machine = maps ? maps__machine(maps) : NULL;
+	if (machine && (!machine__is_host(machine) ||
+			!perf_env__is_same_boot(machine->env)))
+		return false;
+
+	return nsinfo__check_proc(thread__nsinfo(thread));
+}
+
 int thread__set_comm_from_proc(struct thread *thread)
 {
 	char path[64];
@@ -296,6 +324,9 @@ int thread__set_comm_from_proc(struct thread *thread)
 	size_t sz;
 	int err = -1;
 
+	if (!thread__is_alive(thread))
+		return -1;
+
 	if (!(snprintf(path, sizeof(path), "%d/task/%d/comm",
 		       thread__pid(thread), thread__tid(thread)) >= (int)sizeof(path)) &&
 	    procfs__read_str(path, &comm, &sz) == 0) {
@@ -541,7 +572,7 @@ uint16_t thread__e_machine_endian(struct thread *thread, struct machine *machine
 			is_live = !session || !session->data;
 		}
 		/* Read from /proc/pid/exe if live. */
-		if (is_live) {
+		if (is_live && thread__is_alive(thread)) {
 			e_machine = read_proc_e_machine_for_pid(pid, &local_e_flags,
 								&args.is_big_endian);
 		} else if (machine && machine->env) {
diff --git a/tools/perf/util/thread.h b/tools/perf/util/thread.h
index d82fce8173aefedc..d6edbe9a840454f1 100644
--- a/tools/perf/util/thread.h
+++ b/tools/perf/util/thread.h
@@ -205,11 +205,14 @@ static inline refcount_t *thread__refcnt(struct thread *thread)
 	return &RC_CHK_ACCESS(thread)->refcnt;
 }
 
-static inline void thread__set_exited(struct thread *thread, bool exited)
+static inline bool thread__exited(const struct thread *thread)
 {
-	RC_CHK_ACCESS(thread)->exited = exited;
+	return RC_CHK_ACCESS(thread)->exited;
 }
 
+void thread__set_exited(struct thread *thread, bool exited);
+bool thread__is_alive(struct thread *thread);
+
 static inline bool thread__comm_set(const struct thread *thread)
 {
 	return RC_CHK_ACCESS(thread)->comm_set;
-- 
2.56.0.257.gf7497c7d27-goog

Re: [PATCH v1 06/11] perf tools: Check thread liveness when accessing /proc

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Medium] perf: mark replaced parent exited in machine__process_fork_event()
- [Medium] perf tools: missing thread__set_exited() in perf_event__exit_del_thread()
- [Medium] perf tools: premature perf_env__is_same_boot() call poisons env->boot_id
- [Medium] perf tools: unsynchronized nsinfo mutation in thread__set_exited()
- [Medium] perf thread: thread__e_machine_endian() bypasses thread__is_alive() when pid != tid
--

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

perf tools: Check thread liveness when accessing /proc

This commit adds thread__exited() and thread__is_alive(), marks threads and
their nsinfo as exited upon PERF_RECORD_EXIT, PERF_RECORD_FORK replacement,
or boot ID mismatch, and guards /proc accesses with thread__is_alive().

> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> index 14f52234d4d99..8f6298aff681f 100644
> --- a/tools/perf/util/machine.c
> +++ b/tools/perf/util/machine.c
[ ... ]
> @@ -1954,6 +1957,7 @@ int machine__process_fork_event(struct machine *machine, union perf_event *event
>  
>  	/* if a thread currently exists for the thread id remove it */
>  	if (thread != NULL) {
> +		thread__set_exited(thread, /*exited=*/true);
>  		machine__remove_thread(machine, thread);
>  		thread__put(thread);
>  	}

[Severity: Medium]
Should the replaced parent thread just above in machine__process_fork_event()
also be marked exited before it is removed?

tools/perf/util/machine.c:machine__process_fork_event() {
	if (parent != NULL &&
	    thread__pid(parent) != (pid_t)event->fork.ppid) {
		dump_printf("removing erroneous parent thread %d/%d\n",
			    thread__pid(parent), thread__tid(parent));
		machine__remove_thread(machine, parent);
		thread__put(parent);
		parent = machine__findnew_thread(machine, event->fork.ppid,
						 event->fork.ptid);
	}
}

When a prior exit or fork event was lost and event->fork.ptid now belongs to
a different process, parent is removed from machine->threads without calling
thread__set_exited(parent, true).

As a result, parent->exited and parent->nsinfo->exited remain false (and
nsinfo__clear_need_setns() is not called on parent->nsinfo). Any remaining
references to parent or to parent->nsinfo (such as dso->nsinfo shared via
map__new()) will continue to treat the removed thread's nsinfo as live, and
if the TID is recycled again into the original process, nsinfo__check_proc()
and thread__is_alive() can succeed against the wrong thread.

> @@ -2001,9 +2005,8 @@ int machine__process_exit_event(struct machine *machine, union perf_event *event
>  	/* There is no context switch out before exit, so we decrement here. */
>  	machine->parallelism--;
>  	if (thread != NULL) {
> -		if (symbol_conf.keep_exited_threads)
> -			thread__set_exited(thread, /*exited=*/true);
> -		else
> +		thread__set_exited(thread, /*exited=*/true);
> +		if (!symbol_conf.keep_exited_threads)
>  			machine__remove_thread(machine, thread);
>  	}
>  	thread__put(thread);

[Severity: Medium]
Does perf_event__exit_del_thread() in tools/perf/util/event.c also need a
call to thread__set_exited(thread, true) before removing the thread?

tools/perf/util/event.c:perf_event__exit_del_thread() {
	if (thread) {
		machine__remove_thread(machine, thread);
		thread__put(thread);
	}
}

In cmd_buildid_list() -> perf_session__list_build_ids(),
build_id__mark_dso_hit_ops sets .exit = perf_event__exit_del_thread. When a
thread maps a DSO via perf_event__process_mmap2() -> map__new(),
thread__nsinfo(thread) is attached to the DSO via dso__set_nsinfo().

When that thread later exits, perf_event__exit_del_thread() removes the
thread without calling thread__set_exited(thread, true), leaving
nsi->exited false on the surviving dso->nsinfo reference. If a subsequent
sample with DWARF callchain data is processed by build_id__mark_dso_hit() ->
sample__for_each_callchain_node() -> __thread__resolve_callchain() ->
thread__resolve_callchain_unwind() -> unwind__get_entries(), loading the DSO
calls nsinfo__mountns_enter(dso__nsinfo(dso), &nsc) with nsi->exited still
false, which can inspect /proc/<pid>/task/<tid>/ of a recycled PID on the
same boot.

> diff --git a/tools/perf/util/thread.c b/tools/perf/util/thread.c
> index 377c39ec624bb..f75eafb25bfc0 100644
> --- a/tools/perf/util/thread.c
> +++ b/tools/perf/util/thread.c
> @@ -37,6 +37,10 @@ int thread__init_maps(struct thread *thread, struct machine *machine)
>  		}
>  	}
>  
> +	if (!machine || !machine__is_host(machine) ||
> +	    !perf_env__is_same_boot(machine->env))
> +		nsinfo__set_exited(thread__nsinfo(thread));
> +
>  	return thread__maps(thread) ? 0 : -1;
>  }

[Severity: Medium]
Can calling perf_env__is_same_boot(machine->env) here during idle thread
registration prematurely populate machine->env->boot_id with the local host's
boot ID in pipe mode or featureless recordings?

In perf_session__process_events(), perf_session__register_idle_thread() runs
before __perf_session__process_pipe_events() reads any
PERF_RECORD_HEADER_FEATURE events from the pipe:

tools/perf/util/session.c:perf_session__process_events() {
	if (perf_session__register_idle_thread(session) < 0)
		return -ENOMEM;

	if (perf_data__is_pipe(session->data))
		return __perf_session__process_pipe_events(session);
}

perf_session__register_idle_thread() creates the idle thread (0, 0) via
machine__idle_thread() -> machine__findnew_thread() -> thread__init_maps().
Even though thread__nsinfo(thread) is NULL for tid 0, thread__init_maps()
evaluates perf_env__is_same_boot(machine->env) -> perf_env__boot_id():

tools/perf/util/env.c:perf_env__boot_id() {
	if (env->arch || env->hostname) {
		boot_id = NULL;
		goto out;
	}

	env->boot_id = read_host_boot_id();
	boot_id = env->boot_id;
}

Because env->arch, env->hostname, and env->boot_id are all still NULL before
feature events are processed (or in interrupted recordings where
f_header.data.size == 0), perf_env__boot_id() assumes a live environment and
caches read_host_boot_id() in env->boot_id.

If the stream lacks HEADER_BOOT_ID or is processed by a tool that does not
populate session->header.env from feature events (such as perf inject or
offline perf trace), env->boot_id remains set to the local host's boot ID,
causing perf_env__is_same_boot(machine->env) to return true in
thread__init_maps(), machine__update_thread_pid(), and thread__is_alive()
during offline analysis.

> @@ -289,6 +293,30 @@ int __thread__set_comm(struct thread *thread, const char *str, u64 timestamp,
>  	return ret;
>  }
>  
> +void thread__set_exited(struct thread *thread, bool exited)
> +{
> +	RC_CHK_ACCESS(thread)->exited = exited;
> +	if (exited)
> +		nsinfo__set_exited(thread__nsinfo(thread));
> +}

[Severity: Medium]
Can calling nsinfo__set_exited(thread__nsinfo(thread)) on every exit and
fork replacement race with concurrent nsinfo users in multithreaded modes
such as perf top?

A struct nsinfo instance is shared without an internal lock between
thread->nsinfo and one or more dso->nsinfo pointers via map__new() ->
dso__set_nsinfo(). Calling thread__set_exited() invokes nsinfo__set_exited()
-> nsinfo__clear_need_setns():

tools/perf/util/namespaces.c:nsinfo__clear_need_setns() {
	if (!nsi)
		return;

	RC_CHK_ACCESS(nsi)->inited = true;
	RC_CHK_ACCESS(nsi)->need_setns = false;
	zfree(&RC_CHK_ACCESS(nsi)->mntns_path);
}

In perf top, where process_thread processes PERF_RECORD_EXIT and
PERF_RECORD_FORK events concurrently with display_thread resolving symbols
and reading DSOs:

1. nsinfo__mountns_enter() checks RC_CHK_ACCESS(nsi)->need_setns and then
   calls open(nsinfo__mntns_path(nsi), ...), which can race with
   nsinfo__clear_need_setns() freeing and zeroing mntns_path.

2. nsinfo__copy() checks if (nsinfo__mntns_path(nsi)) and then calls
   strdup(nsinfo__mntns_path(nsi)), which can pass NULL or a freed pointer
   to strdup() if nsinfo__clear_need_setns() runs concurrently.

3. Concurrent calls to nsinfo__set_exited() (for example, process_thread
   handling PERF_RECORD_EXIT while display_thread calls
   thread__e_machine_endian() -> thread__is_alive() -> nsinfo__check_proc()
   or nsinfo__mountns_enter() -> nsinfo__check_proc()) can race in
   zfree(&RC_CHK_ACCESS(nsi)->mntns_path) and double-free mntns_path.

[ ... ]
> @@ -541,7 +572,7 @@ uint16_t thread__e_machine_endian(struct thread *thread, struct machine *machine
>  			is_live = !session || !session->data;
>  		}
>  		/* Read from /proc/pid/exe if live. */
> -		if (is_live) {
> +		if (is_live && thread__is_alive(thread)) {
>  			e_machine = read_proc_e_machine_for_pid(pid, &local_e_flags,
>  								&args.is_big_endian);
>  		} else if (machine && machine->env) {

[Severity: Medium]
What happens in thread__e_machine_endian() when thread is a non-leader
sub-thread (pid != tid) whose thread or leader has already exited?

Earlier in thread__e_machine_endian(), before reaching the
thread__is_alive(thread) check, a sub-thread delegates to its group leader:

tools/perf/util/thread.c:thread__e_machine_endian() {
	tid = thread__tid(thread);
	pid = thread__pid(thread);
	if (pid != tid) {
		struct thread *parent = machine__findnew_thread(machine, pid, pid);

		if (parent) {
			e_machine = thread__e_machine_endian(parent, machine, &local_e_flags,
							     &args.is_big_endian);
			thread__put(parent);
			goto out;
		}
		/* Something went wrong, fallback. */
	}
}

If the leader thread (pid, pid) has already exited and been removed from
machine->threads (when symbol_conf.keep_exited_threads is false),
machine__findnew_thread(machine, pid, pid) allocates a fresh parent thread
with parent->exited = false and a new nsinfo for (pid, pid), and recurses on
parent.

In the recursive call, thread__is_alive(parent) is checked instead of
thread__is_alive(thread). Even if the sub-thread had thread->exited == true,
parent->exited is false, and nsinfo__check_proc(parent->nsinfo) only checks
whether /proc/<pid>/task/<pid>/status has Tgid == <pid>, which will succeed
for any new process that recycled <pid> and cause
read_proc_e_machine_for_pid(pid, ...) to read /proc/<pid>/exe of the
recycled process.

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

[PATCH v1 07/11] perf tools: Handle chroot inside nsinfo__mountns_enter()

[Namhyung Kim]

Read /proc/<pid>/task/<tid>/root in nsinfo__init() and enter/exit the
chroot directory alongside the mount namespace inside
nsinfo__mountns_enter() and nsinfo__mountns_exit().

Assisted-by: LLM
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/namespaces.c | 60 +++++++++++++++++++++++++++++++++++-
 tools/perf/util/namespaces.h |  3 ++
 2 files changed, 62 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/namespaces.c b/tools/perf/util/namespaces.c
index bb119a6cc6064d89..1ec6b1ed4db23109 100644
--- a/tools/perf/util/namespaces.c
+++ b/tools/perf/util/namespaces.c
@@ -100,10 +100,12 @@ int nsinfo__init(struct nsinfo *nsi)
 {
 	char oldns[PATH_MAX];
 	char spath[PATH_MAX];
+	char rpath[64];
 	char *newns = NULL;
 	struct stat old_stat;
 	struct stat new_stat;
 	int rv = -1;
+	ssize_t ret;
 
 	if (!nsi || RC_CHK_ACCESS(nsi)->exited)
 		return -1;
@@ -150,6 +152,19 @@ int nsinfo__init(struct nsinfo *nsi)
 		newns = NULL;
 	}
 
+	/* Check if the task is in a chroot directory. */
+	scnprintf(rpath, sizeof(rpath), "/proc/%d/task/%d/root",
+		  nsinfo__pid(nsi), nsinfo__tid(nsi));
+	ret = readlink(rpath, spath, sizeof(spath) - 1);
+	if (ret > 0) {
+		spath[ret] = '\0';
+		if (strcmp(spath, "/") && !strstr(spath, "(deleted)")) {
+			RC_CHK_ACCESS(nsi)->chroot = strdup(spath);
+			if (RC_CHK_ACCESS(nsi)->chroot)
+				RC_CHK_ACCESS(nsi)->in_chroot = true;
+		}
+	}
+
 out:
 	free(newns);
 	return rv;
@@ -201,6 +216,11 @@ static const char *nsinfo__mntns_path(const struct nsinfo *nsi)
 	return RC_CHK_ACCESS(nsi)->mntns_path;
 }
 
+static const char *nsinfo__chroot(const struct nsinfo *nsi)
+{
+	return RC_CHK_ACCESS(nsi)->chroot;
+}
+
 struct nsinfo *nsinfo__copy(const struct nsinfo *nsi)
 {
 	struct nsinfo *nnsi;
@@ -222,6 +242,7 @@ struct nsinfo *nsinfo__copy(const struct nsinfo *nsi)
 	RC_CHK_ACCESS(nnsi)->inited = RC_CHK_ACCESS(nsi)->inited;
 	RC_CHK_ACCESS(nnsi)->exited = RC_CHK_ACCESS(nsi)->exited;
 	RC_CHK_ACCESS(nnsi)->need_setns = RC_CHK_ACCESS(nsi)->need_setns;
+	RC_CHK_ACCESS(nnsi)->in_chroot = RC_CHK_ACCESS(nsi)->in_chroot;
 	RC_CHK_ACCESS(nnsi)->in_pidns = nsinfo__in_pidns(nsi);
 	if (nsinfo__mntns_path(nsi)) {
 		RC_CHK_ACCESS(nnsi)->mntns_path = strdup(nsinfo__mntns_path(nsi));
@@ -230,6 +251,13 @@ struct nsinfo *nsinfo__copy(const struct nsinfo *nsi)
 			return NULL;
 		}
 	}
+	if (nsinfo__chroot(nsi)) {
+		RC_CHK_ACCESS(nnsi)->chroot = strdup(nsinfo__chroot(nsi));
+		if (!RC_CHK_ACCESS(nnsi)->chroot) {
+			nsinfo__put(nnsi);
+			return NULL;
+		}
+	}
 
 	return nnsi;
 }
@@ -244,6 +272,7 @@ static void nsinfo__delete(struct nsinfo *nsi)
 	if (nsi) {
 		WARN_ONCE(refcount_read(nsinfo__refcnt(nsi)) != 0, "nsinfo refcnt unbalanced\n");
 		zfree(&RC_CHK_ACCESS(nsi)->mntns_path);
+		zfree(&RC_CHK_ACCESS(nsi)->chroot);
 		RC_CHK_FREE(nsi);
 	}
 }
@@ -272,7 +301,8 @@ bool nsinfo__need_setns(const struct nsinfo *nsi)
 		return false;
 
 	nsinfo__ensure_init((struct nsinfo *)nsi);
-	return !RC_CHK_ACCESS(nsi)->exited && RC_CHK_ACCESS(nsi)->need_setns;
+	return !RC_CHK_ACCESS(nsi)->exited &&
+	       (RC_CHK_ACCESS(nsi)->need_setns || RC_CHK_ACCESS(nsi)->in_chroot);
 }
 
 void nsinfo__clear_need_setns(struct nsinfo *nsi)
@@ -282,7 +312,9 @@ void nsinfo__clear_need_setns(struct nsinfo *nsi)
 
 	RC_CHK_ACCESS(nsi)->inited = true;
 	RC_CHK_ACCESS(nsi)->need_setns = false;
+	RC_CHK_ACCESS(nsi)->in_chroot = false;
 	zfree(&RC_CHK_ACCESS(nsi)->mntns_path);
+	zfree(&RC_CHK_ACCESS(nsi)->chroot);
 }
 
 void nsinfo__set_exited(struct nsinfo *nsi)
@@ -362,6 +394,7 @@ void nsinfo__mountns_enter(struct nsinfo *nsi,
 	char curpath[PATH_MAX];
 	int oldns = -1;
 	int newns = -1;
+	int oldroot = -1;
 	char *oldcwd = NULL;
 
 	if (nc == NULL)
@@ -369,6 +402,7 @@ void nsinfo__mountns_enter(struct nsinfo *nsi,
 
 	nc->oldns = -1;
 	nc->newns = -1;
+	nc->oldroot = -1;
 	nc->oldcwd = NULL;
 
 	if (!nsi || !nsinfo__need_setns(nsi))
@@ -405,13 +439,30 @@ void nsinfo__mountns_enter(struct nsinfo *nsi,
 			goto errout;
 	}
 
+	if (RC_CHK_ACCESS(nsi)->in_chroot) {
+		oldroot = open("/", O_RDONLY | O_DIRECTORY | O_CLOEXEC);
+		if (oldroot < 0)
+			goto errout_setns;
+
+		if (chroot(nsinfo__chroot(nsi)) < 0)
+			goto errout_setns;
+	}
+
 	nc->oldcwd = oldcwd;
 	nc->oldns = oldns;
 	nc->newns = newns;
+	nc->oldroot = oldroot;
 	return;
 
+errout_setns:
+	if (oldns > -1) {
+		setns(oldns, CLONE_NEWNS);
+		WARN_ON_ONCE(chdir(oldcwd));
+	}
 errout:
 	free(oldcwd);
+	if (oldroot > -1)
+		close(oldroot);
 	if (oldns > -1)
 		close(oldns);
 	if (newns > -1)
@@ -423,6 +474,13 @@ void nsinfo__mountns_exit(struct nscookie *nc)
 	if (nc == NULL || !nc->oldcwd)
 		return;
 
+	if (nc->oldroot > -1) {
+		WARN_ON_ONCE(fchdir(nc->oldroot));
+		WARN_ON_ONCE(chroot("."));
+		close(nc->oldroot);
+		nc->oldroot = -1;
+	}
+
 	if (nc->oldns > -1) {
 		setns(nc->oldns, CLONE_NEWNS);
 		close(nc->oldns);
diff --git a/tools/perf/util/namespaces.h b/tools/perf/util/namespaces.h
index 165e9c9994461595..640ce537295a88e5 100644
--- a/tools/perf/util/namespaces.h
+++ b/tools/perf/util/namespaces.h
@@ -39,14 +39,17 @@ DECLARE_RC_STRUCT(nsinfo) {
 	bool			inited;
 	bool			exited;
 	bool			need_setns;
+	bool			in_chroot;
 	bool			in_pidns;
 	char			*mntns_path;
+	char			*chroot;
 	refcount_t		refcnt;
 };
 
 struct nscookie {
 	int			oldns;
 	int			newns;
+	int			oldroot;
 	char			*oldcwd;
 };
 
-- 
2.56.0.257.gf7497c7d27-goog

Re: [PATCH v1 07/11] perf tools: Handle chroot inside nsinfo__mountns_enter()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] perf namespaces: missing chdir() after chroot() in nsinfo__mountns_enter()
- [Medium] perf tools: chroot in nsinfo__mountns_enter() breaks symbol__disassemble()
- [Medium] perf symbols: dso__load() stores unprefixed chroot path in symsrc_filename
- [Medium] perf namespaces: chroot() in nsinfo__mountns_enter() breaks DSO path lookup
--

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

perf tools: Handle chroot inside nsinfo__mountns_enter()

This commit reads /proc/<pid>/task/<tid>/root in nsinfo__init() and enters
and exits the chroot directory alongside the mount namespace in
nsinfo__mountns_enter() and nsinfo__mountns_exit().

> diff --git a/tools/perf/util/namespaces.c b/tools/perf/util/namespaces.c
> index bb119a6cc6064..1ec6b1ed4db23 100644
> --- a/tools/perf/util/namespaces.c
> +++ b/tools/perf/util/namespaces.c

[ ... ]

> @@ -405,13 +439,30 @@ void nsinfo__mountns_enter(struct nsinfo *nsi,
>  			goto errout;
>  	}
>  
> +	if (RC_CHK_ACCESS(nsi)->in_chroot) {
> +		oldroot = open("/", O_RDONLY | O_DIRECTORY | O_CLOEXEC);
> +		if (oldroot < 0)
> +			goto errout_setns;
> +
> +		if (chroot(nsinfo__chroot(nsi)) < 0)
> +			goto errout_setns;
> +	}

[Severity: Medium]
Should nsinfo__mountns_enter() in tools/perf/util/namespaces.c also call
chdir("/") after chroot(nsinfo__chroot(nsi))?

Calling chroot() updates the process root directory without changing the
current working directory. When need_setns is false, the working directory
remains at oldcwd on the host filesystem, and when need_setns is true,
setns(newns, CLONE_NEWNS) sets the working directory to "/" of the mount
namespace, which is still outside the chroot subdirectory.

When a caller such as opt_set_target() -> nsinfo__realpath() in
tools/perf/util/namespaces.c resolves a relative path for a chrooted target:

tools/perf/util/namespaces.c:nsinfo__realpath() {
	nsinfo__mountns_enter(nsi, &nsc);
	rpath = realpath(path, NULL);
	nsinfo__mountns_exit(&nsc);
}

the kernel getcwd() syscall prepends "(unreachable)" because the working
directory is outside the new root, causing glibc realpath() to fail with
ENOENT, while relative paths passed to nsinfo__stat() resolve against the
directory outside the chroot. Since nsinfo__mountns_enter() already saves
oldcwd and nsinfo__mountns_exit() restores it via chdir(nc->oldcwd), calling
chdir("/") right after chroot() would keep the working directory inside the
new root.

[Severity: Medium]
Does entering the chroot in nsinfo__mountns_enter() break symbol disassembly
in symbol__disassemble() in tools/perf/util/disasm.c when profiling a
chrooted process?

In symbol__disassemble(), dso__find_binary_type() and dso__get_filename() are
called outside nsinfo__mountns_enter():

tools/perf/util/disasm.c:symbol__disassemble() {
	...
	mutex_lock(dso__lock(dso));
	symfs_filename = dso__get_filename(dso, "", &decomp, dbt);
	mutex_unlock(dso__lock(dso));
}

At this commit, when a chrooted binary does not exist at the same path in the
host root, dso__get_filename() in tools/perf/util/dso.c falls back to
dso__filename_with_chroot() and returns a path that already has the chroot
prefix prepended. When symbol__disassemble() then calls
symbol__disassemble_llvm(), symbol__disassemble_capstone(), or
symbol__disassemble_capstone_powerpc(), __dso__read_symbol() in
tools/perf/util/dso.c and symbol__disassemble_capstone_powerpc() in
tools/perf/util/capstone.c enter the chroot via nsinfo__mountns_enter() and
call open(symfs_filename, ...), resulting in a double chroot prefix and
failing with ENOENT (or failing to open the host build-id cache path when the
DSO comes from DSO_BINARY_TYPE__BUILD_ID_CACHE).

In the later commit in the series ("perf tools: Remove redundant chroot
handling"), dso__filename_with_chroot() is removed from dso__get_filename(),
while dso__find_binary_type_locked(), symbol__disassemble(),
dso__find_dbginfo_type(), and debuginfo__new() still invoke
dso__get_filename() outside nsinfo__mountns_enter(), causing
is_regular_file(name) in dso__get_filename() to check the unprefixed path in
the host root and return NULL.

[Severity: Medium]
When dso__load() in tools/perf/util/symbol.c runs inside
nsinfo__mountns_enter() for a chrooted process, how will downstream callers
that open dso__symsrc_filename(dso) outside nsinfo__mountns_enter() locate
the binary?

Inside dso__load(), because nsinfo__mountns_enter() has already chrooted into
the target root directory, is_regular_file(name) succeeds on the unprefixed
in-chroot path instead of falling back to dso__filename_with_chroot() (which
is later removed in "perf tools: Remove redundant chroot handling"):

tools/perf/util/symbol.c:dso__load() {
	...
	is_reg = is_regular_file(name);
	if (!is_reg && errno == ENOENT && dso__nsinfo(dso)) {
		char *new_name = dso__filename_with_chroot(dso, name);
		...
	}
	...
	dso__set_symsrc_filename(dso, strdup(name));
}

As a result, dso__set_symsrc_filename() records the unprefixed in-chroot path
before nsinfo__mountns_exit() restores the host root. When
dso__symsrc_filename(dso) is subsequently used outside
nsinfo__mountns_enter() by dso__libdw_dwfl() in tools/perf/util/libdw.c,
srcline_dso_name() in tools/perf/util/srcline.c (for cmd__addr2line() and
libbfd__addr2line()), __find_debuginfo() in tools/perf/util/unwind-libdw.c,
and read_unwind_spec_debug_frame() / __libunwind__find_proc_info() in
tools/perf/util/unwind-libunwind.c, opening that path in the host root will
either fail with ENOENT or open a different host binary.

[Severity: Medium]
Can calling chroot(nsinfo__chroot(nsi)) in nsinfo__mountns_enter() also break
DSO path lookups for synthesized MMAP2 events and for processes that chroot
after mapping libraries?

First, when perf_event__synthesize_mmap_events() synthesizes MMAP2 events by
reading /proc/<pid>/task/<pid>/maps from perf's own context, the kernel's
show_map_vma() -> d_path() resolves each VMA file path relative to perf's root
(the root of the mount namespace), so event->mmap2.filename and
dso__long_name(dso) already contain the chroot prefix (for example,
"/var/chroot/usr/bin/app"). When perf_record_mmap2__read_build_id() in
tools/perf/util/synthetic-events.c, dsos__read_build_ids_cb(), open_dso(),
dso__load(), or dso__cache_build_id() enters nsinfo__mountns_enter() and
chroots into "/var/chroot", opening that path resolves to
"/var/chroot/var/chroot/usr/bin/app" and fails with ENOENT.

Second, for daemons that map their executable and shared libraries before
calling chroot() into an empty directory such as "/var/empty", both
/proc/<pid>/task/<pid>/maps and kernel-generated PERF_RECORD_MMAP2 events
record host-root-relative paths such as "/usr/sbin/sshd" and
"/lib64/libc.so.6". Entering chroot("/var/empty") in nsinfo__mountns_enter()
causes lookups of those pre-chroot paths to search inside "/var/empty" and
fail with ENOENT.

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

[PATCH v1 08/11] perf tools: Export filename__read_build_id_ns()

[Namhyung Kim]

Export filename__read_build_id_ns() and use it in the dsos callback
and perf-inject's dso__read_build_id() instead of open-coding
nsinfo__mountns_enter() and dso__filename_with_chroot() fallbacks.

Assisted-by: LLM
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-inject.c | 12 +-----------
 tools/perf/util/build-id.c  |  6 +++---
 tools/perf/util/dsos.c      | 17 +++--------------
 tools/perf/util/symbol.h    |  3 +++
 4 files changed, 10 insertions(+), 28 deletions(-)

diff --git a/tools/perf/builtin-inject.c b/tools/perf/builtin-inject.c
index ddb21d888e00d143..2f044aef0bb32b94 100644
--- a/tools/perf/builtin-inject.c
+++ b/tools/perf/builtin-inject.c
@@ -894,24 +894,14 @@ static int perf_event__repipe_tracing_data(const struct perf_tool *tool,
 
 static int dso__read_build_id(struct dso *dso)
 {
-	struct nscookie nsc;
 	struct build_id bid = { .size = 0, };
 
 	if (dso__has_build_id(dso))
 		return 0;
 
 	mutex_lock(dso__lock(dso));
-	nsinfo__mountns_enter(dso__nsinfo(dso), &nsc);
-	if (filename__read_build_id(dso__long_name(dso), &bid) > 0)
+	if (filename__read_build_id_ns(dso__long_name(dso), &bid, dso__nsinfo(dso)) > 0)
 		dso__set_build_id(dso, &bid);
-	else if (dso__nsinfo(dso)) {
-		char *new_name = dso__filename_with_chroot(dso, dso__long_name(dso));
-
-		if (new_name && filename__read_build_id(new_name, &bid) > 0)
-			dso__set_build_id(dso, &bid);
-		free(new_name);
-	}
-	nsinfo__mountns_exit(&nsc);
 	mutex_unlock(dso__lock(dso));
 
 	return dso__has_build_id(dso) ? 0 : -1;
diff --git a/tools/perf/util/build-id.c b/tools/perf/util/build-id.c
index 02233ef0eafff2c2..db5ba9e91fba4d2d 100644
--- a/tools/perf/util/build-id.c
+++ b/tools/perf/util/build-id.c
@@ -847,9 +847,9 @@ int build_id_cache__remove_s(const char *sbuild_id)
 	return err;
 }
 
-static int filename__read_build_id_ns(const char *filename,
-				      struct build_id *bid,
-				      struct nsinfo *nsi)
+int filename__read_build_id_ns(const char *filename,
+			       struct build_id *bid,
+			       struct nsinfo *nsi)
 {
 	struct nscookie nsc;
 	int ret;
diff --git a/tools/perf/util/dsos.c b/tools/perf/util/dsos.c
index e927e707abace9c6..a97fed55286aefca 100644
--- a/tools/perf/util/dsos.c
+++ b/tools/perf/util/dsos.c
@@ -4,11 +4,11 @@
 #include "dso.h"
 #include "util.h"
 #include "vdso.h"
-#include "namespaces.h"
 #include <errno.h>
+#include <linux/perf_event.h>
 #include <stdlib.h>
 #include <string.h>
-#include <symbol.h> // filename__read_build_id
+#include <symbol.h> // filename__read_build_id_ns
 #include <unistd.h>
 
 void dsos__init(struct dsos *dsos)
@@ -70,7 +70,6 @@ struct dsos__read_build_ids_cb_args {
 static int dsos__read_build_ids_cb(struct dso *dso, void *data)
 {
 	struct dsos__read_build_ids_cb_args *args = data;
-	struct nscookie nsc;
 	struct build_id bid = { .size = 0, };
 
 	if (args->with_hits && !dso__hit(dso) && !dso__is_vdso(dso))
@@ -79,20 +78,10 @@ static int dsos__read_build_ids_cb(struct dso *dso, void *data)
 		args->have_build_id = true;
 		return 0;
 	}
-	nsinfo__mountns_enter(dso__nsinfo(dso), &nsc);
-	if (filename__read_build_id(dso__long_name(dso), &bid) > 0) {
+	if (filename__read_build_id_ns(dso__long_name(dso), &bid, dso__nsinfo(dso)) > 0) {
 		dso__set_build_id(dso, &bid);
 		args->have_build_id = true;
-	} else if (errno == ENOENT && dso__nsinfo(dso)) {
-		char *new_name = dso__filename_with_chroot(dso, dso__long_name(dso));
-
-		if (new_name && filename__read_build_id(new_name, &bid) > 0) {
-			dso__set_build_id(dso, &bid);
-			args->have_build_id = true;
-		}
-		free(new_name);
 	}
-	nsinfo__mountns_exit(&nsc);
 	return 0;
 }
 
diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
index d0bac824c79c8283..9d86d2eb694298f7 100644
--- a/tools/perf/util/symbol.h
+++ b/tools/perf/util/symbol.h
@@ -28,6 +28,7 @@
 struct dso;
 struct map;
 struct maps;
+struct nsinfo;
 struct option;
 struct build_id;
 struct perf_env;
@@ -221,6 +222,8 @@ struct symbol *dso__next_symbol(struct symbol *sym);
 enum dso_type dso__type_fd(int fd);
 
 int filename__read_build_id(const char *filename, struct build_id *id);
+int filename__read_build_id_ns(const char *filename, struct build_id *bid,
+			       struct nsinfo *nsi);
 int sysfs__read_build_id(const char *filename, struct build_id *bid);
 int modules__parse(const char *filename, void *arg,
 		   int (*process_module)(void *arg, const char *name,
-- 
2.56.0.257.gf7497c7d27-goog

Re: [PATCH v1 08/11] perf tools: Export filename__read_build_id_ns()

[sashiko-bot]

> Export filename__read_build_id_ns() and use it in the dsos callback
> and perf-inject's dso__read_build_id() instead of open-coding
> nsinfo__mountns_enter() and dso__filename_with_chroot() fallbacks.
> 
> Assisted-by: LLM
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Sashiko has reviewed this patch and found no issues.

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

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

[Namhyung Kim]

Now that nsinfo__mountns_enter() enters the target process's chroot
alongside its mount namespace, callers inside a mountns context no
longer need to manually prepend the target's root directory path via
dso__filename_with_chroot() or filename_with_chroot().

Remove dso__filename_with_chroot() and filename_with_chroot() and use
the DSO path directly within the mountns scope.

Assisted-by: LLM
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/dso.c    | 29 ++---------------------------
 tools/perf/util/dso.h    |  2 --
 tools/perf/util/symbol.c |  8 --------
 tools/perf/util/util.c   | 31 -------------------------------
 tools/perf/util/util.h   |  2 --
 5 files changed, 2 insertions(+), 70 deletions(-)

diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
index ce98613868e7d371..4331b48505b2b10e 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;
 
 	if (dso__needs_decompress(dso)) {
 		char newpath[KMOD_DECOMP_LEN];
diff --git a/tools/perf/util/dso.h b/tools/perf/util/dso.h
index 3329f417810c25a4..e53c83c23c578918 100644
--- a/tools/perf/util/dso.h
+++ b/tools/perf/util/dso.h
@@ -826,8 +826,6 @@ static inline bool dso__has_symbols(const struct dso *dso)
 	return !RB_EMPTY_ROOT(&RC_CHK_ACCESS(dso)->symbols.rb_root);
 }
 
-char *dso__filename_with_chroot(const struct dso *dso, const char *filename);
-
 bool dso__sorted_by_name(const struct dso *dso);
 void dso__set_sorted_by_name(struct dso *dso);
 void dso__sort_by_name(struct dso *dso);
diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index b61a574ce9a30721..807e433c770812a4 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -1902,14 +1902,6 @@ int dso__load(struct dso *dso, struct map *map)
 			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);
-			}
-		}
 
 #ifdef HAVE_LIBBFD_SUPPORT
 		if (is_reg)
diff --git a/tools/perf/util/util.c b/tools/perf/util/util.c
index 8f7cd32f524dc10e..443b1d2b4748bfe4 100644
--- a/tools/perf/util/util.c
+++ b/tools/perf/util/util.c
@@ -451,37 +451,6 @@ void perf_debuginfod_setup(struct perf_debuginfod *di)
 #endif
 }
 
-/*
- * Return a new filename prepended with task's root directory if it's in
- * a chroot.  Callers should free the returned string.
- */
-char *filename_with_chroot(int pid, const char *filename)
-{
-	char buf[PATH_MAX];
-	char proc_root[32];
-	char *new_name = NULL;
-	int ret;
-
-	scnprintf(proc_root, sizeof(proc_root), "/proc/%d/root", pid);
-	ret = readlink(proc_root, buf, sizeof(buf) - 1);
-	if (ret <= 0)
-		return NULL;
-
-	/* readlink(2) does not append a null byte to buf */
-	buf[ret] = '\0';
-
-	if (!strcmp(buf, "/"))
-		return NULL;
-
-	if (strstr(buf, "(deleted)"))
-		return NULL;
-
-	if (asprintf(&new_name, "%s/%s", buf, filename) < 0)
-		return NULL;
-
-	return new_name;
-}
-
 /*
  * Reallocate an array *arr of size *arr_sz so that it is big enough to contain
  * x elements of size msz, initializing new entries to *init_val or zero if
diff --git a/tools/perf/util/util.h b/tools/perf/util/util.h
index f8f1ff603c728086..b9adb58df976f29e 100644
--- a/tools/perf/util/util.h
+++ b/tools/perf/util/util.h
@@ -84,8 +84,6 @@ void perf_debuginfod_setup(struct perf_debuginfod *di);
 
 const char *perf_basename(const char *path);
 
-char *filename_with_chroot(int pid, const char *filename);
-
 int do_realloc_array_as_needed(void **arr, size_t *arr_sz, size_t x,
 			       size_t msz, const void *init_val);
 
-- 
2.56.0.257.gf7497c7d27-goog

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

[sashiko-bot]

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

[PATCH v1 10/11] perf tools: Only enter mount namespace if necessary

[Namhyung Kim]

Add dso_binary_type__needs_ns() to distinguish DSO binary types that
reside inside the target process's mount namespace and chroot from
host-side paths.

Gate mount namespace entry in dso__data_file_size() and do_open() using
dso_binary_type__needs_ns().  And remove redundant outer mountns entry
in around dso__data_get_fd().

Assisted-by: LLM
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/capstone.c |  6 ++++--
 tools/perf/util/dso.c      | 12 ++++++++----
 tools/perf/util/dso.h      | 34 ++++++++++++++++++++++++++++++++++
 3 files changed, 46 insertions(+), 6 deletions(-)

diff --git a/tools/perf/util/capstone.c b/tools/perf/util/capstone.c
index 95881f15324aa614..9a8646ff026d92a4 100644
--- a/tools/perf/util/capstone.c
+++ b/tools/perf/util/capstone.c
@@ -653,9 +653,11 @@ int symbol__disassemble_capstone_powerpc(const char *filename __maybe_unused,
 	if (args->options->objdump_path)
 		return -1;
 
-	nsinfo__mountns_enter(dso__nsinfo(dso), &nsc);
+	if (dso_binary_type__needs_ns(dso__binary_type(dso)))
+		nsinfo__mountns_enter(dso__nsinfo(dso), &nsc);
 	fd = open(filename, O_RDONLY);
-	nsinfo__mountns_exit(&nsc);
+	if (dso_binary_type__needs_ns(dso__binary_type(dso)))
+		nsinfo__mountns_exit(&nsc);
 	if (fd < 0)
 		return -1;
 
diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
index 4331b48505b2b10e..50dd215f81f6737c 100644
--- a/tools/perf/util/dso.c
+++ b/tools/perf/util/dso.c
@@ -682,14 +682,15 @@ static int open_dso(struct dso *dso, struct machine *machine)
 {
 	int fd;
 	struct nscookie nsc;
+	bool nsenter = dso_binary_type__needs_ns(dso__binary_type(dso));
 
-	if (dso__binary_type(dso) != DSO_BINARY_TYPE__BUILD_ID_CACHE) {
+	if (nsenter) {
 		mutex_lock(dso__lock(dso));
 		nsinfo__mountns_enter(dso__nsinfo(dso), &nsc);
 		mutex_unlock(dso__lock(dso));
 	}
 	fd = __open_dso(dso, machine);
-	if (dso__binary_type(dso) != DSO_BINARY_TYPE__BUILD_ID_CACHE)
+	if (nsenter)
 		nsinfo__mountns_exit(&nsc);
 
 	if (fd >= 0) {
@@ -2081,11 +2082,14 @@ static const u8 *__dso__read_symbol(struct dso *dso, const char *symfs_filename,
 	};
 	u8 *code_buf = NULL;
 	int saved_errno;
+	bool nsenter = dso_binary_type__needs_ns(dso__binary_type(dso));
 
-	nsinfo__mountns_enter(dso__nsinfo(dso), &nsc);
+	if (nsenter)
+		nsinfo__mountns_enter(dso__nsinfo(dso), &nsc);
 	fd = open(symfs_filename, O_RDONLY | O_CLOEXEC);
 	saved_errno = errno;
-	nsinfo__mountns_exit(&nsc);
+	if (nsenter)
+		nsinfo__mountns_exit(&nsc);
 	if (fd < 0) {
 		errno = saved_errno;
 		return NULL;
diff --git a/tools/perf/util/dso.h b/tools/perf/util/dso.h
index e53c83c23c578918..c4ff19f575b88a9e 100644
--- a/tools/perf/util/dso.h
+++ b/tools/perf/util/dso.h
@@ -141,6 +141,40 @@ enum dso_binary_type {
 	DSO_BINARY_TYPE__NOT_FOUND,
 };
 
+static inline bool dso_binary_type__needs_ns(enum dso_binary_type type)
+{
+	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__KALLSYMS:
+	case DSO_BINARY_TYPE__GUEST_KALLSYMS:
+	case DSO_BINARY_TYPE__VMLINUX:
+	case DSO_BINARY_TYPE__GUEST_VMLINUX:
+	case DSO_BINARY_TYPE__JAVA_JIT:
+	case DSO_BINARY_TYPE__BUILD_ID_CACHE:
+	case DSO_BINARY_TYPE__BUILD_ID_CACHE_DEBUGINFO:
+	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__GUEST_KMODULE:
+	case DSO_BINARY_TYPE__GUEST_KMODULE_COMP:
+	case DSO_BINARY_TYPE__KCORE:
+	case DSO_BINARY_TYPE__GUEST_KCORE:
+	case DSO_BINARY_TYPE__OPENEMBEDDED_DEBUGINFO:
+	case DSO_BINARY_TYPE__BPF_PROG_INFO:
+	case DSO_BINARY_TYPE__BPF_IMAGE:
+	case DSO_BINARY_TYPE__OOL:
+	case DSO_BINARY_TYPE__NOT_FOUND:
+	default:
+		return false;
+	}
+}
+
 enum dso_space_type {
 	DSO_SPACE__USER = 0,
 	DSO_SPACE__KERNEL,
-- 
2.56.0.257.gf7497c7d27-goog

Re: [PATCH v1 10/11] perf tools: Only enter mount namespace if necessary

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] perf tools: dso_binary_type__needs_ns() omits distro debuginfo types
- [Medium] perf tools: commit message describes functions not modified by patch
- [Medium] perf dso: missing mountns entry in dso__find_binary_type_locked() and callers
--

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

perf tools: Only enter mount namespace if necessary

This commit adds dso_binary_type__needs_ns() to identify DSO binary types
that reside in a target process's mount namespace and chroot, and uses it
to gate nsinfo__mountns_enter() and nsinfo__mountns_exit() calls.

>     perf tools: Only enter mount namespace if necessary
>     
>     Add dso_binary_type__needs_ns() to distinguish DSO binary types that
>     reside inside the target process's mount namespace and chroot from
>     host-side paths.
>     
>     Gate mount namespace entry in dso__data_file_size() and do_open() using
>     dso_binary_type__needs_ns().  And remove redundant outer mountns entry
>     in around dso__data_get_fd().
>     
>     Assisted-by: LLM
>     Signed-off-by: Namhyung Kim <namhyung@kernel.org>

[Severity: Medium]
Does the commit message match the functions modified in this patch?

The description mentions gating mount namespace entry in
dso__data_file_size() and do_open(), and removing an outer mountns entry
around dso__data_get_fd(), but the patch actually modifies open_dso(),
__dso__read_symbol(), and symbol__disassemble_capstone_powerpc().

[ ... ]

> diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
> index 4331b48505b2b..50dd215f81f67 100644
> --- a/tools/perf/util/dso.c
> +++ b/tools/perf/util/dso.c
> @@ -682,14 +682,15 @@ static int open_dso(struct dso *dso, struct machine *machine)
>  {
>  	int fd;
>  	struct nscookie nsc;
> +	bool nsenter = dso_binary_type__needs_ns(dso__binary_type(dso));
>  
> -	if (dso__binary_type(dso) != DSO_BINARY_TYPE__BUILD_ID_CACHE) {
> +	if (nsenter) {
>  		mutex_lock(dso__lock(dso));
>  		nsinfo__mountns_enter(dso__nsinfo(dso), &nsc);
>  		mutex_unlock(dso__lock(dso));
>  	}
>  	fd = __open_dso(dso, machine);
> -	if (dso__binary_type(dso) != DSO_BINARY_TYPE__BUILD_ID_CACHE)
> +	if (nsenter)
>  		nsinfo__mountns_exit(&nsc);

[Severity: Medium]
Will dso__binary_type(dso) ever be set to a namespace-dependent type if
dso__find_binary_type_locked() does not enter the mount namespace while
probing candidate paths?

In the call chain try_to_open_dso() -> __dso__find_binary_type() ->
dso__find_binary_type_locked(), each candidate type in binary_type_data[]
is tested before open_dso() is called:

tools/perf/util/dso.c:dso__find_binary_type_locked() {
	...
	do {
		dso__set_binary_type(dso, binary_type_data[i++]);

		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__find_binary_type_locked() does not call
nsinfo__mountns_enter(dso__nsinfo(dso), &nsc) when
dso_binary_type__needs_ns() is true (and an earlier commit in this series,
"perf tools: Remove redundant chroot handling", removed the
dso__filename_with_chroot() fallback from dso__get_filename()), probing a
binary that only exists inside the target's mount namespace or chroot will
fail to find the file on the host and leave dso->binary_type set to
DSO_BINARY_TYPE__NOT_FOUND. When open_dso() then checks
dso_binary_type__needs_ns(DSO_BINARY_TYPE__NOT_FOUND), nsenter will be
false and __open_dso() will fail with -ENOENT (or inspect a host file if
one happens to share the same path).

Similarly, symbol__disassemble() in tools/perf/util/disasm.c calls
dso__find_binary_type(dso) and dso__get_filename(dso, "", &decomp, dbt)
outside the mount namespace before reaching __dso__read_symbol() or
symbol__disassemble_capstone_powerpc(), and dso__find_dbginfo_type() and
debuginfo__new() also call dso__get_filename() and inspect ELF sections
without entering the target's mount namespace.

[ ... ]

> diff --git a/tools/perf/util/dso.h b/tools/perf/util/dso.h
> index e53c83c23c578..c4ff19f575b88 100644
> --- a/tools/perf/util/dso.h
> +++ b/tools/perf/util/dso.h
> @@ -141,6 +141,40 @@ enum dso_binary_type {
>  	DSO_BINARY_TYPE__NOT_FOUND,
>  };
>  
> +static inline bool dso_binary_type__needs_ns(enum dso_binary_type type)
> +{
> +	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__KALLSYMS:
> +	case DSO_BINARY_TYPE__GUEST_KALLSYMS:
> +	case DSO_BINARY_TYPE__VMLINUX:
> +	case DSO_BINARY_TYPE__GUEST_VMLINUX:
> +	case DSO_BINARY_TYPE__JAVA_JIT:
> +	case DSO_BINARY_TYPE__BUILD_ID_CACHE:
> +	case DSO_BINARY_TYPE__BUILD_ID_CACHE_DEBUGINFO:
> +	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__GUEST_KMODULE:
> +	case DSO_BINARY_TYPE__GUEST_KMODULE_COMP:
> +	case DSO_BINARY_TYPE__KCORE:
> +	case DSO_BINARY_TYPE__GUEST_KCORE:
> +	case DSO_BINARY_TYPE__OPENEMBEDDED_DEBUGINFO:
> +	case DSO_BINARY_TYPE__BPF_PROG_INFO:
> +	case DSO_BINARY_TYPE__BPF_IMAGE:
> +	case DSO_BINARY_TYPE__OOL:
> +	case DSO_BINARY_TYPE__NOT_FOUND:
> +	default:
> +		return false;
> +	}
> +}

[Severity: Medium]
Should dso_binary_type__needs_ns() also return true for
DSO_BINARY_TYPE__FEDORA_DEBUGINFO, DSO_BINARY_TYPE__UBUNTU_DEBUGINFO,
DSO_BINARY_TYPE__MIXEDUP_UBUNTU_DEBUGINFO,
DSO_BINARY_TYPE__BUILDID_DEBUGINFO, and
DSO_BINARY_TYPE__OPENEMBEDDED_DEBUGINFO?

In dso__read_binary_type_filename(), those five distribution debuginfo
types construct target-filesystem paths via __symbol__join_symfs() (such as
/usr/lib/debug/... or <dso_dir>/.debug/...), rather than host build-id
cache paths like DSO_BINARY_TYPE__BUILD_ID_CACHE and
DSO_BINARY_TYPE__BUILD_ID_CACHE_DEBUGINFO.

In tools/perf/util/symbol.c, dso__load() only exits the target's mount
namespace for DSO_BINARY_TYPE__BUILD_ID_CACHE and
DSO_BINARY_TYPE__BUILD_ID_CACHE_DEBUGINFO:

	nsexit = (symtab_type == DSO_BINARY_TYPE__BUILD_ID_CACHE ||
	    symtab_type == DSO_BINARY_TYPE__BUILD_ID_CACHE_DEBUGINFO);

When dso__find_binary_type_locked() falls back to dso__symtab_type(dso) in
binary_type_data[] (or when a later commit in this series, "perf tools:
Scope mount namespace entry in dso__load()", replaces the nsexit check in
dso__load() with dso_binary_type__needs_ns(symtab_type)), returning false
for these five debuginfo types causes perf to skip entering the target
container's mount namespace and chroot when resolving or opening them.

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

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

[Namhyung Kim]

Enter the target mount namespace only for the specific binary types
that need it.

Assisted-by: LLM
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/symbol.c | 43 +++++++++++++++++++---------------------
 1 file changed, 20 insertions(+), 23 deletions(-)

diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index 807e433c770812a4..063e9da0878e93bc 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -1811,17 +1811,6 @@ int dso__load(struct dso *dso, struct map *map)
 	const char *map_path = dso__long_name(dso);
 
 	mutex_lock(dso__lock(dso));
-	perfmap = is_perf_pid_map_name(map_path);
-
-	if (perfmap) {
-		if (dso__nsinfo(dso) &&
-		    (dso__find_perf_map(newmapname, sizeof(newmapname),
-					dso__nsinfo_ptr(dso)) == 0)) {
-			map_path = newmapname;
-		}
-	}
-
-	nsinfo__mountns_enter(dso__nsinfo(dso), &nsc);
 
 	/* check again under the dso->lock */
 	if (dso__loaded(dso)) {
@@ -1845,8 +1834,16 @@ int dso__load(struct dso *dso, struct map *map)
 
 	dso__set_adjust_symbols(dso, false);
 
+	perfmap = is_perf_pid_map_name(map_path);
 	if (perfmap) {
+		if (dso__nsinfo(dso) &&
+		    (dso__find_perf_map(newmapname, sizeof(newmapname),
+					dso__nsinfo_ptr(dso)) == 0)) {
+			map_path = newmapname;
+		}
+		nsinfo__mountns_enter(dso__nsinfo(dso), &nsc);
 		ret = dso__load_perf_map(map_path, dso);
+		nsinfo__mountns_exit(&nsc);
 		dso__set_binary_type(dso, DSO_BINARY_TYPE__JAVA_JIT);
 		dso__set_symtab_type(dso, ret > 0
 				? DSO_BINARY_TYPE__JAVA_JIT
@@ -1869,7 +1866,7 @@ int dso__load(struct dso *dso, struct map *map)
 		struct build_id bid = { .size = 0, };
 
 		__symbol__join_symfs(name, PATH_MAX, dso__long_name(dso));
-		if (filename__read_build_id(name, &bid) > 0)
+		if (filename__read_build_id_ns(name, &bid, dso__nsinfo(dso)) > 0)
 			dso__set_build_id(dso, &bid);
 	}
 
@@ -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);
+
 		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);
 
 		if (bfdrc > 0) {
 			dso__set_symtab_type(dso, symtab_type);
@@ -1978,7 +1976,6 @@ int dso__load(struct dso *dso, struct map *map)
 out:
 	dso__set_loaded(dso);
 	mutex_unlock(dso__lock(dso));
-	nsinfo__mountns_exit(&nsc);
 
 	return ret;
 }
-- 
2.56.0.257.gf7497c7d27-goog

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

[sashiko-bot]

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