Re: [PATCH v3 9/9] perf/cxl: Don't log through pmu.dev in the overflow interrupt handler

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

Pre-existing issues:
- [High] Double counting and cross-event corruption of performance events due to uncleared pending overflows.
--

--- Patch [9]: [PATCH v3 9/9] perf/cxl: Don't log through pmu.dev in the overflow interrupt handler ---
commit d6b58846ef576f998be916a54f3a5a31363d2d44
Author: Dave Jiang <[email protected]>

perf/cxl: Don't log through pmu.dev in the overflow interrupt handler

This commit fixes a use-after-free in the CXL PMU overflow interrupt handler
by changing the logging device to the parent and clears any stale overflow
status before requesting the IRQ.

Closes: https://sashiko.dev/#/patchset/[email protected]?part=1

[ ... ]

> --- a/drivers/perf/cxl_pmu.c
> +++ b/drivers/perf/cxl_pmu.c

[ ... ]

> @@ -903,6 +903,15 @@ static int cxl_pmu_probe(struct device *dev)
>  	if (!irq_name)
>  		return -ENOMEM;
>  
> +	/*
> +	 * Clear any overflow status left set by firmware or a previous kernel
> +	 * before the handler goes live, so it cannot mistake a stale bit for an
> +	 * overflow on a counter no event owns yet. The register is RW1C, and
> +	 * bits above the implemented counters are RsvdZ, so only write those.
> +	 */
> +	writeq(GENMASK_ULL(info->num_counters - 1, 0),
> +	       info->base + CXL_PMU_OVERFLOW_REG);
> +

[Severity: High]
This isn't a bug introduced by this patch, but since we are clearing stale 
overflow bits here, does cxl_pmu_event_stop() also need to clear the overflow 
bit when stopping an event?

Currently, cxl_pmu_event_stop() in drivers/perf/cxl_pmu.c disables the counter 
but doesn't clear any pending overflow bit in CXL_PMU_OVERFLOW_REG. If an 
event overflows just before being stopped (e.g., during PMU multiplexing), 
and a new event is started on the same counter or reset via 
PERF_EVENT_IOC_RESET, the hardware overflow bit remains set.

When IRQs are re-enabled, cxl_pmu_irq() handles the pending IRQ and calls
__cxl_pmu_read() with overflow=true. Because the counter was just reset
by cxl_pmu_event_start(), both new_cnt and prev_cnt will be 0. 

Since new_cnt (0) >= prev_cnt (0), would __cxl_pmu_read() falsely apply the 
wraparound fixup:

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

Could this erroneously add a full period (e.g., 2^64 ticks) to the newly
started or reset event and corrupt the counter statistics?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=9