Re: [PATCH v3 19/20] perf symbol: Use session allocation wrappers in symbol, DSO, thread, and hist paths

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] perf machine: NULL deref in __machine__findnew_thread() on
  thread OOM
- [Low] perf symbol: NULL session for split kernel DSOs in
  maps__split_kallsyms()
--

commit ae77bbd4b11902101ad3db4f72bc23f1b4972f1f
Author: Ian Rogers <irogers@google.com>

perf symbol: Use session allocation wrappers in symbol, DSO, thread, and hist paths

This commit wires session-scoped allocation wrappers and shrink-and-retry
handling into symbol, DSO, thread, and hist_entry allocation paths.

> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> index c89f47ba16f2c..489395b53c2e3 100644
> --- a/tools/perf/util/machine.c
> +++ b/tools/perf/util/machine.c

[ ... ]

> @@ -598,7 +599,12 @@ static struct thread *__machine__findnew_thread(struct machine *machine,
>  
>  struct thread *machine__findnew_thread(struct machine *machine, pid_t pid, pid_t tid)
>  {
> -	return __machine__findnew_thread(machine, pid, tid, /*create=*/true);
> +	struct thread *th = __machine__findnew_thread(machine, pid, tid, /*create=*/true);
> +
> +	if (!th && perf_session__shrink(machine__session(machine)) > 0)
> +		th = __machine__findnew_thread(machine, pid, tid, /*create=*/true);
> +
> +	return th;
>  }

[Severity: Medium]
Can __machine__findnew_thread() dereference a NULL th before returning to
this retry path when thread allocation fails?

When threads__findnew() fails to allocate a thread, it returns NULL with
created == false:

tools/perf/util/machine.c:__machine__findnew_thread() {
    ...
	th = threads__findnew(&machine->threads, pid, tid, &created);
	if (created) {
		if (thread__init_maps(th, machine)) {
			pr_err("Thread init failed thread %d\n", pid);
			threads__remove(&machine->threads, th);
			thread__put(th);
			return NULL;
		}
	} else
		machine__update_thread_pid(machine, th, pid);
    ...
}

The else branch then calls machine__update_thread_pid(machine, NULL, pid),
which immediately dereferences th in thread__pid(th) instead of returning
NULL so machine__findnew_thread() can shrink the session and retry.

> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> index 9d7c87bcbbea8..21f012252dcd5 100644
> --- a/tools/perf/util/symbol.c
> +++ b/tools/perf/util/symbol.c

[ ... ]

> @@ -1717,7 +1727,8 @@ static int maps__split_kallsyms(struct maps *kmaps, struct dso *dso, u64 delta,
>  		if (!RC_CHK_EQUAL(curr_map, initial_map)) {
>  			struct dso *curr_map_dso = map__dso(curr_map);
>  
> -			if (symbols__insert(dso__symbols(curr_map_dso), pos) == 0)
> +			if (symbols__insert_session(dso__symbols(curr_map_dso), pos,
> +						    dso__session(curr_map_dso)) == 0)
>  				++moved;
>  		} else {
>  			old_symbols[write_idx++] = pos;

[Severity: Low]
Does dso__session(curr_map_dso) return NULL for split kernel range DSOs
created earlier in maps__split_kallsyms()?

When a non-module kernel symbol falls outside initial_map, ndso is created
with dso__new(dso_name) and inserted into kmaps via map__new2(), but it is
never added to machine->dsos.  Because ndso->dsos remains NULL,
dso__session(curr_map_dso) evaluates to NULL and symbol array growth for
those maps bypasses perf_session__shrink(); passing dso__session(dso)
would preserve the session.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110220803.2503416-1-irogers@google.com?part=19