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

[sashiko-bot@kernel.org]

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