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

[Athira Rajeev <atrajeev@linux.ibm.com>]

> On 29 Apr 2026, at 11:09 PM, Ian Rogers <irogers@google.com> wrote:
> 
> Address several segmentation fault vectors in `perf sched stats diff`:
> 
> 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
>   prevent out-of-bounds reads during parallel list traversal.
> 
> 2. In `show_schedstat_data()`, added NULL checks for `cd_map1` and `cd_map2`
>   to gracefully handle invalid or empty data files.
> 
> 3. Added parallel iteration termination checks using `list_is_last()` in
>   `show_schedstat_data()` for both domain and CPU lists to safely terminate
>   at the end of each list when files contain a different number of CPUs
>   or domains.
> 
> 4. Added CPU bounds checks (`cs1->cpu >= nr1` and `cs2->cpu >= nr2`) in
>   `show_schedstat_data()` to prevent out-of-bounds reads from `cd_map1` and
>   `cd_map2` when comparing files from machines with different CPU counts.
> 
> 5. Added NULL checks for `cd_info1` and `cd_info2` to prevent crashes when
>   a CPU has data samples but no corresponding domain info in the header.
> 
> 6. Added domain bounds checks (`ds1->domain >= cd_info1->nr_domains` and
>   `ds2->domain >= cd_info2->nr_domains`) to prevent out-of-bounds array
>   accesses in the domains array.
> 
> 7. Added NULL checks for `dinfo1` and `dinfo2` in `show_schedstat_data()`
>   to prevent crashes when a domain has no corresponding domain info.
> 
> 8. Zero-initialized the `perf_data` array in `perf_sched__schedstat_diff()`
>   to prevent stack garbage from causing `perf_data_file__fd()` to attempt
>   to use a NULL `fptr` when `use_stdio` happened to be non-zero.
> 
> Assisted-by: Gemini:gemini-3.1-pro-preview
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
> v2: Reduce indentation and variable scopes. Hopefully address feedback
>    from James Clark.
> 
> Previously this patch was part of a large perf script refactor:
> https://lore.kernel.org/lkml/20260425224951.174663-1-irogers@google.com/

Tested-by: Athira Rajeev <atrajeev@linux.ibm.com <mailto:atrajeev@linux.ibm.com>>

Thanks
Athira
> ---
> tools/perf/builtin-sched.c | 144 +++++++++++++++++++++++++++----------
> 1 file changed, 106 insertions(+), 38 deletions(-)
> 
> 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)
> {
> - struct schedstat_cpu *cptr = list_first_entry(head, struct schedstat_cpu, cpu_list);
> - struct schedstat_cpu *summary_head = NULL;
> - struct perf_record_schedstat_domain *ds;
> - struct perf_record_schedstat_cpu *cs;
> - struct schedstat_domain *dptr, *tdptr;
> + struct schedstat_cpu *cptr, *summary_head;
> + struct schedstat_domain *dptr;
> bool is_last = false;
> int cnt = 1;
> - int ret = 0;
> 
> - if (cptr) {
> - summary_head = zalloc(sizeof(*summary_head));
> - if (!summary_head)
> - return -ENOMEM;
> + assert(!list_empty(head));
> + cptr = list_first_entry(head, struct schedstat_cpu, cpu_list);
> 
> - summary_head->cpu_data = zalloc(sizeof(*cs));
> - memcpy(summary_head->cpu_data, cptr->cpu_data, sizeof(*cs));
> + summary_head = zalloc(sizeof(*summary_head));
> + if (!summary_head)
> + return -ENOMEM;
> 
> - INIT_LIST_HEAD(&summary_head->domain_head);
> + summary_head->cpu_data = zalloc(sizeof(*summary_head->cpu_data));
> + memcpy(summary_head->cpu_data, cptr->cpu_data, sizeof(*summary_head->cpu_data));
> 
> - list_for_each_entry(dptr, &cptr->domain_head, domain_list) {
> - tdptr = zalloc(sizeof(*tdptr));
> - if (!tdptr)
> - return -ENOMEM;
> + INIT_LIST_HEAD(&summary_head->domain_head);
> 
> - tdptr->domain_data = zalloc(sizeof(*ds));
> - if (!tdptr->domain_data)
> - return -ENOMEM;
> + list_for_each_entry(dptr, &cptr->domain_head, domain_list) {
> + struct schedstat_domain *tdptr = zalloc(sizeof(*tdptr));
> 
> - memcpy(tdptr->domain_data, dptr->domain_data, sizeof(*ds));
> - list_add_tail(&tdptr->domain_list, &summary_head->domain_head);
> - }
> + if (!tdptr)
> + return -ENOMEM;
> +
> + tdptr->domain_data = zalloc(sizeof(*tdptr->domain_data));
> + if (!tdptr->domain_data)
> + return -ENOMEM;
> +
> + memcpy(tdptr->domain_data, dptr->domain_data, sizeof(*tdptr->domain_data));
> + list_add_tail(&tdptr->domain_list, &summary_head->domain_head);
> }
> 
> list_for_each_entry(cptr, head, cpu_list) {
> + struct schedstat_domain *tdptr;
> +
> if (list_is_first(&cptr->cpu_list, head))
> continue;
> 
> @@ -4212,32 +4212,47 @@ static int get_all_cpu_stats(struct list_head *head)
> 
> cnt++;
> summarize_schedstat_cpu(summary_head, cptr, cnt, is_last);
> + if (list_empty(&summary_head->domain_head))
> + continue;
> +
> tdptr = list_first_entry(&summary_head->domain_head, struct schedstat_domain,
> domain_list);
> 
> list_for_each_entry(dptr, &cptr->domain_head, domain_list) {
> summarize_schedstat_domain(tdptr, dptr, cnt, is_last);
> + if (list_is_last(&tdptr->domain_list, &summary_head->domain_head)) {
> + tdptr = NULL;
> + break;
> + }
> tdptr = list_next_entry(tdptr, domain_list);
> }
> }
> 
> list_add(&summary_head->cpu_list, head);
> - return ret;
> + return 0;
> }
> 
> -static int show_schedstat_data(struct list_head *head1, struct cpu_domain_map **cd_map1,
> -       struct list_head *head2, struct cpu_domain_map **cd_map2,
> +static int show_schedstat_data(struct list_head *head1, struct cpu_domain_map **cd_map1, int nr1,
> +       struct list_head *head2, struct cpu_domain_map **cd_map2, int nr2,
>       bool summary_only)
> {
> struct schedstat_cpu *cptr1 = list_first_entry(head1, struct schedstat_cpu, cpu_list);
> struct perf_record_schedstat_domain *ds1 = NULL, *ds2 = NULL;
> - struct perf_record_schedstat_cpu *cs1 = NULL, *cs2 = NULL;
> struct schedstat_domain *dptr1 = NULL, *dptr2 = NULL;
> struct schedstat_cpu *cptr2 = NULL;
> __u64 jiffies1 = 0, jiffies2 = 0;
> bool is_summary = true;
> int ret = 0;
> 
> + if (!cd_map1) {
> + pr_err("Error: CPU domain map 1 is missing.\n");
> + return -1;
> + }
> + if (head2 && !cd_map2) {
> + pr_err("Error: CPU domain map 2 is missing.\n");
> + return -1;
> + }
> +
> printf("Description\n");
> print_separator2(SEP_LEN, "", 0);
> printf("%-30s-> %s\n", "DESC", "Description of the field");
> @@ -4267,14 +4282,36 @@ static int show_schedstat_data(struct list_head *head1, struct cpu_domain_map **
> 
> list_for_each_entry(cptr1, head1, cpu_list) {
> struct cpu_domain_map *cd_info1 = NULL, *cd_info2 = NULL;
> + struct perf_record_schedstat_cpu *cs1 = cptr1->cpu_data;
> + struct perf_record_schedstat_cpu *cs2 = NULL;
> 
> - cs1 = cptr1->cpu_data;
> + dptr2 = NULL;
> + if (cs1->cpu >= (u32)nr1) {
> + pr_err("Error: CPU %d exceeds domain map size %d\n", cs1->cpu, nr1);
> + return -1;
> + }
> cd_info1 = cd_map1[cs1->cpu];
> + if (!cd_info1) {
> + pr_err("Error: CPU %d domain info is missing in map 1.\n",
> +       cs1->cpu);
> + return -1;
> + }
> if (cptr2) {
> cs2 = cptr2->cpu_data;
> + if (cs2->cpu >= (u32)nr2) {
> + pr_err("Error: CPU %d exceeds domain map size %d\n", cs2->cpu, nr2);
> + return -1;
> + }
> cd_info2 = cd_map2[cs2->cpu];
> - dptr2 = list_first_entry(&cptr2->domain_head, struct schedstat_domain,
> - domain_list);
> + if (!cd_info2) {
> + pr_err("Error: CPU %d domain info is missing in map 2.\n",
> +       cs2->cpu);
> + return -1;
> + }
> + if (!list_empty(&cptr2->domain_head))
> + dptr2 = list_first_entry(&cptr2->domain_head,
> + struct schedstat_domain,
> + domain_list);
> }
> 
> if (cs2 && cs1->cpu != cs2->cpu) {
> @@ -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 (ds1->domain >= cd_info1->nr_domains) {
> + pr_err("Error: Domain %d exceeds max domains %d for CPU %d in map 1.\n",
> +       ds1->domain, cd_info1->nr_domains, cs1->cpu);
> + return -1;
> + }
> dinfo1 = cd_info1->domains[ds1->domain];
> + if (!dinfo1) {
> + pr_err("Error: Domain %d info is missing for CPU %d in map 1.\n",
> +       ds1->domain, cs1->cpu);
> + return -1;
> + }
> if (dptr2) {
> ds2 = dptr2->domain_data;
> + if (ds2->domain >= cd_info2->nr_domains) {
> + pr_err("Error: Domain %d exceeds max domains %d for CPU %d in map 2.\n",
> +       ds2->domain, cd_info2->nr_domains, cs2->cpu);
> + return -1;
> + }
> dinfo2 = cd_info2->domains[ds2->domain];
> + if (!dinfo2) {
> + pr_err("Error: Domain %d info is missing for CPU %d in map 2.\n",
> +       ds2->domain, cs2->cpu);
> + return -1;
> + }
> }
> 
> if (dinfo2 && dinfo1->domain != dinfo2->domain) {
> @@ -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);
> + }
> }
> if (summary_only)
> break;
> 
> - if (cptr2)
> - cptr2 = list_next_entry(cptr2, cpu_list);
> + if (cptr2) {
> + if (list_is_last(&cptr2->cpu_list, head2))
> + cptr2 = NULL;
> + else
> + cptr2 = list_next_entry(cptr2, cpu_list);
> + }
> 
> is_summary = false;
> }
> @@ -4523,7 +4588,9 @@ static int perf_sched__schedstat_report(struct perf_sched *sched)
> }
> 
> cd_map = session->header.env.cpu_domain;
> - err = show_schedstat_data(&cpu_head, cd_map, NULL, NULL, false);
> + err = show_schedstat_data(&cpu_head, cd_map,
> +  session->header.env.nr_cpus_avail,
> +  NULL, NULL, 0, false);
> }
> 
> out:
> @@ -4538,7 +4605,7 @@ static int perf_sched__schedstat_diff(struct perf_sched *sched,
> struct cpu_domain_map **cd_map0 = NULL, **cd_map1 = NULL;
> struct list_head cpu_head_ses0, cpu_head_ses1;
> struct perf_session *session[2];
> - struct perf_data data[2];
> + struct perf_data data[2] = {0};
> int ret = 0, err = 0;
> static const char *defaults[] = {
> "perf.data.old",
> @@ -4610,7 +4677,8 @@ static int perf_sched__schedstat_diff(struct perf_sched *sched,
> goto out_delete_ses0;
> }
> 
> - show_schedstat_data(&cpu_head_ses0, cd_map0, &cpu_head_ses1, cd_map1, true);
> + 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);
> 
> out_delete_ses1:
> free_schedstat(&cpu_head_ses1);
> @@ -4720,7 +4788,7 @@ static int perf_sched__schedstat_live(struct perf_sched *sched,
> goto out;
> }
> 
> - show_schedstat_data(&cpu_head, cd_map, NULL, NULL, false);
> + show_schedstat_data(&cpu_head, cd_map, nr, NULL, NULL, 0, false);
> free_cpu_domain_info(cd_map, sv, nr);
> out:
> free_schedstat(&cpu_head);
> -- 
> 2.54.0.545.g6539524ca2-goog
>