Re: [PATCH 06/11] perf sched: Fix idle-hist callchain display using wrong rb_first variant

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] In `perf sched map`, using the `--color-pids` option causes a segmentation fault and an invalid free due to type confusion on `thread__priv()`.
--

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

perf sched: Fix idle-hist callchain display using wrong rb_first variant

This commit fixes a bug in the idle-hist callchain display where
rb_first_cached() was used on a tree populated with rb_insert_color(). The
tree lacks the leftmost cache, so rb_first_cached() returned NULL and dropped
the callchain summary. The fix restores the original behavior by using
rb_first() directly on the rb_root.

> diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
> index ded511d851880..85f11d388392d 100644
> --- a/tools/perf/builtin-sched.c
> +++ b/tools/perf/builtin-sched.c
> @@ -3130,7 +3130,8 @@ static size_t timehist_print_idlehist_callchain(struct rb_root_cached *root)
>  	size_t ret = 0;
>  	FILE *fp = stdout;
>  	struct callchain_node *chain;
> -	struct rb_node *rb_node = rb_first_cached(root);
> +	/* sort() uses rb_insert_color() on rb_root, not rb_root_cached */
> +	struct rb_node *rb_node = rb_first(&root->rb_root);

[Severity: High]
This isn't a bug introduced by this patch, but does the --color-pids
option in perf sched map cause a segmentation fault and invalid free
due to type confusion on thread__priv()?

When --color-pids is used, map__findnew_thread() sets
thread__priv(thread) to the scalar value (void*)1 to mark the thread
for coloring:

tools/perf/builtin-sched.c:map__findnew_thread() {
    ...
    if (thread_map__has(sched->map.color_pids, tid))
        color = true;

    thread__set_priv(thread, color ? ((void*)1) : NULL);
    ...
}

However, the rest of the perf sched code expects thread__priv() to
hold a valid pointer to a struct thread_runtime. When
map_switch_event() calls thread__get_runtime(), it retrieves the
(void*)1 pointer and skips initialization because it is not NULL:

tools/perf/builtin-sched.c:thread__get_runtime() {
    ...
    tr = thread__priv(thread);
    if (tr == NULL) {
        tr = thread__init_runtime(thread);
    ...
}

Then map_switch_event() attempts to read tr->shortname[0], which
dereferences the invalid address and causes an immediate segmentation
fault:

tools/perf/builtin-sched.c:map_switch_event() {
    ...
    tr = thread__get_runtime(sched_in);
    if (tr == NULL)
        goto out;
    ...
    if (!tr->shortname[0]) {
    ...
}

Furthermore, because cmd_sched() registers free() as the destructor
for thread priv data:

tools/perf/builtin-sched.c:cmd_sched() {
    ...
    thread__set_priv_destructor(free);
    ...
}

Tearing down the session will attempt to call free((void*)1),
corrupting the heap.

Could struct thread_runtime be expanded to include a coloring flag,
or could the color state be tracked outside the thread__priv pointer
to prevent this type confusion?

>  
>  	printf("  %16s  %8s  %s\n", "Idle time (msec)", "Count", "Callchains");
>  	printf("  %.16s  %.8s  %.50s\n", graph_dotted_line, graph_dotted_line,

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