Re: [PATCH 4/5] perf sched: Cap max_cpu at MAX_CPUS in timehist sample processing

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] perf_sched__timehist omits the .feature tool callback, breaking perf_env population in pipe mode.
- [High] Leaked callchain nodes and violation of reference counting rules during idle threads teardown.
--

commit fa7c274dda31324797cc9c2df04b0e7a2f0a40d2
Author: Arnaldo Carvalho de Melo <acme@redhat.com>

perf sched: Cap max_cpu at MAX_CPUS in timehist sample processing

This commit adds bounds checking to sched->max_cpu updates from the
sample CPU. It caps env->nr_cpus_online initialization of max_cpu to
MAX_CPUS to prevent out-of-bounds array accesses on large systems or
recordings with high CPU IDs.

> diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
> index 7bd61028327b3..87a1f4cf8760e 100644
> --- a/tools/perf/builtin-sched.c
> +++ b/tools/perf/builtin-sched.c

[ ... ]

> @@ -3385,8 +3387,8 @@ static int perf_sched__timehist(struct perf_sched *sched)
>  		perf_session__set_tracepoints_handlers(session, migrate_handlers))
>  		goto out;
>  
> -	/* pre-allocate struct for per-CPU idle stats */
> -	sched->max_cpu.cpu = env->nr_cpus_online;
> +	/* pre-allocate struct for per-CPU idle stats; cap to array bounds */
> +	sched->max_cpu.cpu = min(env->nr_cpus_online, MAX_CPUS);

[Severity: High]
This is a pre-existing issue, but does perf_sched__timehist() correctly
populate perf_env in pipe mode?

Looking at perf_sched__timehist() in tools/perf/builtin-sched.c, the tool
callbacks are initialized without a .feature callback:

    sched->tool.sample       = perf_timehist__process_sample;
    sched->tool.mmap         = perf_event__process_mmap;
    ...
    sched->tool.build_id     = perf_event__process_build_id;

Without registering perf_event__process_feature, piped perf.data streams
might silently drop non-seekable PERF_RECORD_HEADER_FEATURE events.

Does this leave perf_env uninitialized and break environment metadata lookups
like the env->nr_cpus_online field accessed here?

[Severity: High]
This is another pre-existing issue, but is there a reference counting leak
during teardown in free_idle_threads()?

When cleaning up the idle threads array created below by init_idle_threads(),
the cleanup code uses the internal destructor instead of the lifecycle helper:

    free_idle_threads() {
        ...
        thread__delete(idle);
    }

Since reference handles allocated via thread__new() must be strictly paired
with thread__put(), does calling thread__delete() directly bypass the
reference wrapper lifecycle and leak the pointer handle, triggering
REFCNT_CHECKING traps? 

Also, does destroying the thread without proper cleanup leak the dynamically
allocated nodes inside itr->callchain?

>  	if (sched->max_cpu.cpu == 0)
>  		sched->max_cpu.cpu = 4;
>  	if (init_idle_threads(sched->max_cpu.cpu))

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