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

[Ian Rogers]

On Tue, Jun 9, 2026 at 11:44 AM Chun-Tse Shao <ctshao@xxxxxxxxxx> wrote:
>
> Ping for attention.

I suspect Arnaldo and Namhyung are waiting for feedback from Intel on
these patches. If someone could check them it would be appreciated.

Thanks,
Ian

> 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