Re: [PATCH RESEND v7 1/2] perf pmu intel: Generalize SNC cpumask adjustment for multiple platforms

[Chun-Tse Shao]

Ping for attention.

Thanks,
CT

On Thu, May 28, 2026 at 2:31 PM Chun-Tse Shao <ctshao@xxxxxxxxxx> wrote:
>
> On Wed, May 27, 2026 at 5:11 PM <sashiko-bot@xxxxxxxxxx> wrote:
> >
> > Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> >
> > Pre-existing issues:
> > - [Critical] An integer overflow in `uncore_cha_snc()` can result in a negative Sub-NUMA Clustering (SNC) node index. This bypasses the upper-bound check against `MAX_SNCS` and causes subsequent out-of-bounds array accesses.
> > --
> >
> > commit be394e8b7ddbe730c3fc40fb608337fe19c7b33b
> > Author: Chun-Tse Shao <ctshao@xxxxxxxxxx>
> >
> > perf pmu intel: Generalize SNC cpumask adjustment for multiple platforms
> >
> > This commit generalizes the Sub-NUMA Clustering (SNC) logic previously
> > specific to Granite Rapids to support multiple Intel platforms. It uses
> > CPUID to determine SNC support and caches various PMU configurations
> > to improve thread safety.
> >
> > > diff --git a/tools/perf/arch/x86/util/pmu.c b/tools/perf/arch/x86/util/pmu.c
> > > index 7c9d238922a6c..9b00d5720fb7a 100644
> > > --- a/tools/perf/arch/x86/util/pmu.c
> > > +++ b/tools/perf/arch/x86/util/pmu.c
> >
> > [ ... ]
> >
> > > @@ -121,46 +142,73 @@ static int uncore_cha_snc(struct perf_pmu *pmu)
> > >               return 0;
> > >       }
> > >       chas_per_node = num_cha / snc_nodes;
> > > +     if (chas_per_node == 0) {
> > > +             pr_warning("Unexpected: chas_per_node is 0 (num_cha=%d, snc_nodes=%d)\n",
> > > +                        num_cha, snc_nodes);
> > > +             return 0;
> > > +     }
> > >       cha_snc = cha_num / chas_per_node;
> > >
> > >       /* Range check cha_snc. for unexpected out of bounds. */
> > >       return cha_snc >= MAX_SNCS ? 0 : cha_snc;
> >
> > [Severity: Critical]
> > This is a pre-existing issue, but does this range check safely handle very
> > large values for cha_num?
> >
> > Since cha_num is parsed as an unsigned int via sscanf() from the PMU name,
> > and cha_snc is a signed int, a sufficiently large cha_num could cause the
> > division result to overflow into a negative value.
>
> I think it is very unlikely we would see cha_num > INT_MAX.
> Since that is a pre-existing issue, I prefer to fix it separately.
>
> Thanks,
> CT
>
> >
> > If cha_snc becomes negative, the cha_snc >= MAX_SNCS check evaluates to
> > false, allowing the function to return a negative index.
> >
> > Could this negative index then cause out-of-bounds array accesses when used
> > later in uncore_cha_imc_compute_cpu_adjust()?
> >
> > uncore_cha_imc_compute_cpu_adjust() {
> >     static bool checked_cpu_adjust[MAX_SNCS];
> >     ...
> >     if (checked_cpu_adjust[pmu_snc])
> >     ...
> > }
> >
> > --
> > Sashiko AI review · https://sashiko.dev/#/patchset/20260527221934.3830896-1-ctshao@xxxxxxxxxx?part=1