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

[Chun-Tse Shao]

Thank you Ian for bringing this back. Zide gave a Reviewed-by tag on
v6, and v7 mainly addresses the Sashiko review.

To Arnaldo and Namhyung, Is this patch ready to be merged?

Thanks,
CT

On Fri, Aug 28, 2026 at 1:11 AM Mi, Dapeng <dapeng1.mi@xxxxxxxxxxxxxxx> wrote:
>
>
> On 8/28/2026 1:28 PM, Ian Rogers wrote:
> > 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.
>
> I suppose Zide has checked these patches and gave the "reviewed-by" tags. :)
>
>
> >
> > 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