Re: [PATCH v2] perf sched stats: Fix segmentation faults in diff mode

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Stale `ds2` pointer leads to invalid domain diff comparisons.
- [High] Missing NULL check for `summary_head->cpu_data` allocation leads to a segmentation fault.
- [High] Memory leaks on error paths in `get_all_cpu_stats()`.
- [High] Failure of `get_all_cpu_stats()` is silently ignored, leading to data corruption.
- [Medium] Commit message falsely claims fixing an uninitialized variable.
--

commit 9defc7ef4fdb95729f7675ddb4788cb55728c905
Author: Ian Rogers <irogers@google.com>
Subject: perf sched stats: Fix segmentation faults in diff mode

The patch addresses multiple segmentation fault vectors and out-of-bounds
reads in perf sched stats diff by adding bounds checks, NULL checks, list
termination checks, and zero-initialization.

> 1. In get_all_cpu_stats(), added assert(!list_empty(head)) to prevent
>    unsafe list_first_entry() calls on empty lists, fixed uninitialized
>    variable ret, and added list_is_last check when iterating domains to

This isn't a bug, but the commit message mentions fixing an uninitialized
variable ret. Looking at the original code, ret was explicitly initialized
to zero (int ret = 0;).

> diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
> index 555247568e7a..e67241415255 100644
> --- a/tools/perf/builtin-sched.c
> +++ b/tools/perf/builtin-sched.c

[ ... ]

> @@ -4170,40 +4170,40 @@ static void summarize_schedstat_domain(struct schedstat_domain *summary_domain,
>   */
>  static int get_all_cpu_stats(struct list_head *head)
>  {
[ ... ]
> -	if (cptr) {
> -		summary_head = zalloc(sizeof(*summary_head));
> -		if (!summary_head)
> -			return -ENOMEM;
> +	summary_head = zalloc(sizeof(*summary_head));
> +	if (!summary_head)
> +		return -ENOMEM;
>  
> -		summary_head->cpu_data = zalloc(sizeof(*cs));
> -		memcpy(summary_head->cpu_data, cptr->cpu_data, sizeof(*cs));
> +	summary_head->cpu_data = zalloc(sizeof(*summary_head->cpu_data));
> +	memcpy(summary_head->cpu_data, cptr->cpu_data, sizeof(*summary_head->cpu_data));

If zalloc() for summary_head->cpu_data fails and returns NULL, could this
memcpy() dereference the NULL pointer and cause a crash?

>  
> -		INIT_LIST_HEAD(&summary_head->domain_head);
> +	INIT_LIST_HEAD(&summary_head->domain_head);
>  
> -		list_for_each_entry(dptr, &cptr->domain_head, domain_list) {
> -			tdptr = zalloc(sizeof(*tdptr));
> -			if (!tdptr)
> -				return -ENOMEM;
> +	list_for_each_entry(dptr, &cptr->domain_head, domain_list) {
> +		struct schedstat_domain *tdptr = zalloc(sizeof(*tdptr));
>  
> -			tdptr->domain_data = zalloc(sizeof(*ds));
> -			if (!tdptr->domain_data)
> -				return -ENOMEM;
> +		if (!tdptr)
> +			return -ENOMEM;
> +
> +		tdptr->domain_data = zalloc(sizeof(*tdptr->domain_data));
> +		if (!tdptr->domain_data)
> +			return -ENOMEM;

If either of these zalloc() calls fails and the function returns -ENOMEM,
what happens to the memory previously allocated for summary_head and
summary_head->cpu_data? It seems they might leak since they haven't been
attached to the list yet.

Additionally, does the caller of get_all_cpu_stats() properly handle an
-ENOMEM return value? In show_schedstat_data(), if this error is ignored,
the summary node won't be prepended, which might cause the subsequent loop
to treat the first CPU's actual data as the summary.

[ ... ]

> @@ -4302,10 +4339,30 @@ static int show_schedstat_data(struct list_head *head1, struct cpu_domain_map **
>  			struct domain_info *dinfo1 = NULL, *dinfo2 = NULL;
>  
>  			ds1 = dptr1->domain_data;
[ ... ]
>  			if (dptr2) {
>  				ds2 = dptr2->domain_data;
[ ... ]
> @@ -4334,14 +4391,22 @@ static int show_schedstat_data(struct list_head *head1, struct cpu_domain_map **
>  			print_domain_stats(ds1, ds2, jiffies1, jiffies2);
>  			print_separator2(SEP_LEN, "", 0);
>  
> -			if (dptr2)
> -				dptr2 = list_next_entry(dptr2, domain_list);
> +			if (dptr2) {
> +				if (list_is_last(&dptr2->domain_list, &cptr2->domain_head))
> +					dptr2 = NULL;
> +				else
> +					dptr2 = list_next_entry(dptr2, domain_list);
> +			}

When dptr2 is explicitly set to NULL at the end of the second file's domain
list, the next iteration of the loop will skip the ds2 = dptr2->domain_data
assignment.

Does this leave ds2 holding the pointer from the previous iteration, causing
print_domain_stats() to perform a diff comparison against a stale, unrelated
domain?

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