Re: [PATCH 5/8] perf sched: Clean up idle_threads entry on init failure

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Negative array index out-of-bounds read and write in `get_idle_thread()` due to unchecked `sample->cpu`.
- [Medium] Missing `thread__put()` causes reference count and handle leaks for both `idle` and `thread` variables in `timehist_get_thread()`.
- [Medium] A NULL pointer dereference occurs in `latency_runtime_event()` if thread allocation fails.
- [Medium] Leaked `callchain_cursor_node` structs inside `idle_thread_runtime` when the thread is destroyed.
--

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

    perf sched: Clean up idle_threads entry on init failure
    
    get_idle_thread() allocates a thread via thread__new() and stores it in
    idle_threads[cpu], then calls init_idle_thread() to set up the private
    data. If init_idle_thread() fails, the partially initialized thread is
    left in idle_threads[cpu], causing subsequent calls to return a thread
    without private data. This patch resets the slot on init failure.

> diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
> index 555247568e7a6..e47db1c5c6020 100644
> --- a/tools/perf/builtin-sched.c
> +++ b/tools/perf/builtin-sched.c
> @@ -2475,8 +2475,11 @@ static struct thread *get_idle_thread(int cpu)

[Severity: High]
This is a pre-existing issue, but does this function properly validate the
cpu index? The sample->cpu field is read from the untrusted perf.data file.
If a sample lacks a CPU, it typically defaults to (u32)-1. When passed to
get_idle_thread() as an int, cpu becomes -1.

The upper-bounds check will evaluate to false:

        if ((cpu >= idle_max_cpu) || (idle_threads == NULL)) {

And then a negative index is used to read and write memory out of bounds:

        if (idle_threads[cpu] == NULL) {
                idle_threads[cpu] = thread__new(0, 0);

Could this unchecked value lead to heap metadata corruption?

>  		idle_threads[cpu] = thread__new(0, 0);
>  
>  		if (idle_threads[cpu]) {
> -			if (init_idle_thread(idle_threads[cpu]) < 0)
> +			if (init_idle_thread(idle_threads[cpu]) < 0) {
> +				/* clean up so next call doesn't find a half-initialized thread */
> +				thread__zput(idle_threads[cpu]);
>  				return NULL;
> +			}
>  		}
>  	}

[Severity: Medium]
This isn't a bug introduced by this patch, but doesn't the
timehist_get_thread() function leak the thread and idle references on error
paths?

tools/perf/builtin-sched.c:timehist_get_thread() {
        ...
        idle = get_idle_thread(sample->cpu);
        if (idle == NULL) {
            ...
        }

        itr = thread__priv(idle);
        if (itr == NULL)
            return NULL;
        ...
}

It appears get_idle_thread() returns a reference to the idle thread that is
never released with thread__put(). Also, returning NULL when itr is NULL
leaks both the newly acquired idle thread and the primary thread reference.
Could this cause a memory leak over time during large traces?

[Severity: Medium]
This is a pre-existing issue, but does latency_runtime_event() dereference
a potentially NULL thread pointer before checking it?

tools/perf/builtin-sched.c:latency_runtime_event() {
        ...
        struct thread *thread = machine__findnew_thread(machine, -1, pid);
        struct work_atoms *atoms = thread_atoms_search(&sched->atom_root,
                                                       thread, &sched->cmp_pid);
        ...
        if (thread == NULL)
            return -1;
        ...
}

If thread allocation fails, machine__findnew_thread() returns NULL, which is
then passed to thread_atoms_search() and dereferenced before the check
if (thread == NULL) is reached. Is it possible to move the NULL check above
the call to thread_atoms_search()?

[Severity: Medium]
This isn't a bug introduced by this patch, but does the idle_thread_runtime
structure permanently leak its callchain_cursor nodes?

When init_idle_thread() sets up the idle thread:

tools/perf/builtin-sched.c:init_idle_thread() {
        ...
        callchain_cursor_reset(&itr->cursor);
        thread__set_priv(thread, itr);
        ...
}

During execution, save_idle_callchain() allocates nodes dynamically via
callchain_cursor__copy(). However, the destructor for thread private data is
typically just free(), meaning the dynamically allocated callchain_cursor_node
elements within itr->cursor are never deep-freed when the idle thread is
destroyed. Should a proper destructor be registered to free these nodes?

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