Re: [PATCH v3] perf sched stats: Fix segmentation faults, memory leaks, and stale pointers in diff mode

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Resource and memory leak of the second perf session when the first session's data is empty.
- [Medium] Stranded elements in the global `cpu_head` list are leaked when `perf_session__process_events()` fails.
--

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

perf sched stats: Fix segmentation faults, memory leaks, and stale pointers in diff mode

This commit addresses multiple crash vectors and memory leaks in perf sched
stats diff mode by adding bounds checks, NULL checks, proper error
propagation, and robust memory cleanup.

> diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
> --- a/tools/perf/builtin-sched.c
> +++ b/tools/perf/builtin-sched.c

[ ... ]

> @@ -4632,7 +4704,10 @@ static int perf_sched__schedstat_diff(struct perf_sched *sched,
>  	err = perf_session__process_events(session[0]);
>  	if (err)
>  		goto out_delete_ses0;

Is this code leaking the events added to the global cpu_head list?

The schedstat callbacks dynamically allocate and add parsed elements to the
global cpu_head list during perf_session__process_events(). However, the
transfer to cpu_head_ses0 via list_replace_init() hasn't happened yet at
this point. Since out_delete_ses0 only frees cpu_head_ses0, it looks like
any items added to cpu_head before the error occurred would be stranded.

The same pattern appears to exist for session[1] and cpu_head_ses1.

[ ... ]

>  	if (list_empty(&cpu_head_ses1)) {
>  		pr_err("Data is not available\n");
>  		ret = -1;
>  		goto out_delete_ses1;
>  	}
> 
>  	if (list_empty(&cpu_head_ses0)) {
>  		pr_err("Data is not available\n");
>  		ret = -1;
>  		goto out_delete_ses0;
>  	}

Does jumping to out_delete_ses0 here bypass the cleanup for the second
session?

Since session[1] has already been successfully opened, parsed, and its
events transferred into cpu_head_ses1, skipping the out_delete_ses1 block
would mean free_schedstat(&cpu_head_ses1) and
perf_session__delete(session[1]) are never called.

> -	show_schedstat_data(&cpu_head_ses0, cd_map0, &cpu_head_ses1, cd_map1, true);
> +	ret = show_schedstat_data(&cpu_head_ses0, cd_map0, session[0]->header.env.nr_cpus_avail,
> +				  &cpu_head_ses1, cd_map1, session[1]->header.env.nr_cpus_avail, true);
> +	if (ret)
> +		goto out_delete_ses1;
>  
>  out_delete_ses1:
>  	free_schedstat(&cpu_head_ses1);
>  	if (!IS_ERR(session[1]))
>  		perf_session__delete(session[1]);
> 
>  out_delete_ses0:
>  	free_schedstat(&cpu_head_ses0);
>  	if (!IS_ERR(session[0]))
>  		perf_session__delete(session[0]);

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