Re: [PATCH v3 3/9] perf/cxl: Fix the counter overflow delta fixup

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [Medium] A race condition between a polled read and the overflow IRQ handler causes a double-counting of a full period if the polled read executes after the counter wraps but before the IRQ handler.
- [Medium] Missing validation for `info->counter_width` allows a non-compliant or buggy CXL device to trigger Undefined Behavior when `GENMASK_ULL` is evaluated.
--

--- Patch [3]: [PATCH v3 3/9] perf/cxl: Fix the counter overflow delta fixup ---
commit 333d6be6b59d292e876b5c4888ab33d0abc8cc03
Author: Dave Jiang <dave.jiang@intel.com>

perf/cxl: Fix the counter overflow delta fixup

This commit alters the CXL PMU counter overflow logic to condition the fixup
on new_cnt >= prev_cnt, aiming to correctly handle cases where the subtraction
cannot express the span over a period boundary, while accommodating events
that increment by more than 1 per cycle. It also modifies the period addition
to use mask + 1 to avoid undefined behavior on 64-bit counters.

Link: https://sashiko.dev/#/patchset/20260715191454.459673-1-dave@stgolabs.net?part=1

> diff --git a/drivers/perf/cxl_pmu.c b/drivers/perf/cxl_pmu.c
> index b16e2e4090a39..3511d049aea40 100644
> --- a/drivers/perf/cxl_pmu.c
> +++ b/drivers/perf/cxl_pmu.c
> @@ -689,7 +689,7 @@ static void __cxl_pmu_read(struct perf_event *event, bool overflow)
>  {
>  	struct cxl_pmu_info *info = pmu_to_cxl_pmu_info(event->pmu);
>  	struct hw_perf_event *hwc = &event->hw;
> -	u64 new_cnt, prev_cnt, delta;
> +	u64 new_cnt, prev_cnt, delta, mask;
>  
>  	do {
>  		prev_cnt = local64_read(&hwc->prev_count);

[ ... ]

>  	/*
> -	 * If we know an overflow occur then take that into account.
> -	 * Note counter is not reset as that would lose events
> +	 * The mask discards exactly the bit that says the counter wrapped, so a
> +	 * delta of one whole period reads back as 0, the same as no events at
> +	 * all. Only the overflow status distinguishes them, and new_cnt >=
> +	 * prev_cnt is that case, so add the period back. mask + 1 is
> +	 * 2^counter_width, which is 0 for a 64-bit counter - the right value to
> +	 * add once the top bit is gone, and no undefined 1 << 64.
>  	 */
> -	delta = (new_cnt - prev_cnt) & GENMASK_ULL(info->counter_width - 1, 0);
> -	if (overflow && delta < GENMASK_ULL(info->counter_width - 1, 0))
> -		delta += (1UL << info->counter_width);
> +	mask = GENMASK_ULL(info->counter_width - 1, 0);

[Severity: Medium]
This is a pre-existing issue, but could a buggy or malicious CXL device
reporting a counter width of 0 (or > 64) trigger Undefined Behavior here?

If the device reports 0 for the counter width in the capability registers,
the driver reads this into info->counter_width without bounds validation.

When GENMASK_ULL(info->counter_width - 1, 0) is evaluated, it results in a
negative shift amount which leads to Undefined Behavior and corrupted PMU
masks.

> +	delta = (new_cnt - prev_cnt) & mask;
> +	if (overflow && new_cnt >= prev_cnt)
> +		delta += mask + 1;

[Severity: Medium]
This isn't a bug introduced by this patch, but does this sequence allow a 
race condition between a polled read and the overflow IRQ handler to
double-count a full period?

If the hardware counter wraps, setting the overflow bit, a polled read
(such as via perf stat -I) might read the counter before the hardware IRQ
is delivered.

The polled read would compute the delta, correctly including the period
because new_cnt is wrapped and smaller than prev_cnt, and then update
prev_count to the small post-wrap value.

When the delayed overflow IRQ handler subsequently runs, it reads the
overflow bit and calls __cxl_pmu_read(event, true). Because prev_cnt is
now the small post-wrap value, new_cnt >= prev_cnt evaluates to true.

The handler would then execute delta += mask + 1, incorrectly adding a
second full period because it assumes the wrap wasn't already consumed.

>  
>  	local64_add(delta, &event->count);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260731232827.401447-1-dave.jiang@intel.com?part=3