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

[sashiko-bot@kernel.org]

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