Re: [PATCH 3/5] perf sched: Fix thread reference leaks in timehist_get_thread()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Unbounded reference leak of struct thread in timehist_sched_change_event
- [High] Missing .attr and .feature callbacks break perf sched in pipe mode
--

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

perf sched: Fix thread reference leaks in timehist_get_thread()

timehist_get_thread() acquires a thread reference via
machine__findnew_thread() and an idle thread reference via
get_idle_thread() (which calls thread__get()).  Two error paths in
the idle_hist block return NULL without releasing these references.

> diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
> index 4aa7833cae6e3..7bd61028327b3 100644
> --- a/tools/perf/builtin-sched.c
> +++ b/tools/perf/builtin-sched.c
> @@ -2546,12 +2546,16 @@ static struct thread *timehist_get_thread(struct perf_sched *sched,
>  			idle = get_idle_thread(sample->cpu);
>  			if (idle == NULL) {
>  				pr_err("Failed to get idle thread for cpu %d.\n", sample->cpu);
> +				thread__put(thread);
>  				return NULL;
>  			}
>  
>  			itr = thread__priv(idle);
> -			if (itr == NULL)
> +			if (itr == NULL) {
> +				thread__put(idle);
> +				thread__put(thread);
>  				return NULL;
> +			}
>  
>  			thread__put(itr->last_thread);
>  			itr->last_thread = thread__get(thread);

[Severity: High]
This isn't a bug introduced by this patch, but since itr->last_thread is now
properly reference counted here, I noticed it is being cleared in
timehist_sched_change_event() without a thread__put():

tools/perf/builtin-sched.c:timehist_sched_change_event() {
    ...
            if (itr->cursor.nr)
                callchain_append(&itr->callchain, &itr->cursor, t - tprev);

            itr->last_thread = NULL;
        }

        if (!sched->summary_only)
            timehist_print_sample(sched, sample, &al, thread, t, state);
    ...
}

Does this permanently leak the thread reference on every idle-to-busy
transition when --idle-hist is active?


[Severity: High]
This is a pre-existing issue, but while reviewing the sched tool
initialization, I noticed that the .feature callback is omitted for both
perf_sched__timehist and cmd_sched, and .attr is additionally omitted for
cmd_sched:

tools/perf/builtin-sched.c:cmd_sched() {
    ...
    perf_tool__init(&sched.tool, /*ordered_events=*/true);
    sched.tool.sample    = perf_sched__process_tracepoint_sample;
    sched.tool.comm      = perf_sched__process_comm;
    sched.tool.namespaces = perf_event__process_namespaces;
    sched.tool.lost      = perf_event__process_lost;
    sched.tool.fork      = perf_sched__process_fork_event;
    ...
}

tools/perf/builtin-sched.c:perf_sched__timehist() {
    ...
    sched->tool.sample   = perf_timehist__process_sample;
    sched->tool.mmap     = perf_event__process_mmap;
    sched->tool.mmap2    = perf_event__process_mmap2;
    ...
    sched->tool.attr     = perf_event__process_attr;
    ...
}

In pipe mode, does dropping .attr prevent the creation of evlists entirely,
and does dropping .feature prevent proper initialization of perf_env,
breaking the event processing pipeline?

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