Re: [RESEND PATCH v4 09/11] perf/cxl: Validate the hardware-reported counter width

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

Pre-existing issues:
- [High] Race condition between manual perf event reads and the PMU overflow interrupt handler causes spurious full-period additions.
- [High] Missing cleanup of the hardware overflow status bit when stopping or starting a counter leads to massive overcounting for subsequently scheduled events.
--

--- Patch [9]: [RESEND PATCH v4 09/11] perf/cxl: Validate the hardware-reported counter width ---
commit 194e381f35d23ddbc6cc781235e1bc6b38fb673a
Author: Dave Jiang <[email protected]>

perf/cxl: Validate the hardware-reported counter width

cxl_pmu_parse_caps() takes the Counter Width straight from the CPMU
Capability register without a bounds check. Reject a counter width
outside 1..64 at probe to prevent shifting out of range.

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

> diff --git a/drivers/perf/cxl_pmu.c b/drivers/perf/cxl_pmu.c
> index a081fcba69917..84ebc7da181d5 100644
> --- a/drivers/perf/cxl_pmu.c
> +++ b/drivers/perf/cxl_pmu.c
> @@ -142,6 +142,15 @@ static int cxl_pmu_parse_caps(struct device *dev, struct cxl_pmu_info *info)
>  
>  	info->num_counters = FIELD_GET(CXL_PMU_CAP_NUM_COUNTERS_MSK, val) + 1;
>  	info->counter_width = FIELD_GET(CXL_PMU_CAP_COUNTER_WIDTH_MSK, val);
> +	/*
> +	 * The Counter Data register is 64 bits wide, so a Counter Width of 0 or
> +	 * >64 is invalid. Reject it rather than let GENMASK_ULL(width - 1, 0) in
> +	 * the read path shift out of range.
> +	 */
> +	if (info->counter_width == 0 || info->counter_width > 64) {
> +		dev_err(dev, "Invalid counter width %d\n", info->counter_width);
> +		return -ENODEV;
> +	}
>  	info->num_event_capabilities = FIELD_GET(CXL_PMU_CAP_NUM_EVN_CAP_REG_SUP_MSK, val) + 1;
>  
>  	info->filter_hdm = FIELD_GET(CXL_PMU_CAP_FILTERS_SUP_MSK, val) & CXL_PMU_FILTER_HDM;

[Severity: High]
This is a pre-existing issue, but can a race condition between manual perf
event reads and the PMU overflow interrupt handler cause spurious additions?

drivers/perf/cxl_pmu.c:__cxl_pmu_read() {
    ...
	mask = GENMASK_ULL(info->counter_width - 1, 0);
	delta = (new_cnt - prev_cnt) & mask;
	if (overflow && new_cnt >= prev_cnt)
		delta += mask + 1;
    ...
}

If a hardware counter wraps and asserts the PMU interrupt, and a manual read
occurs before the interrupt handler executes, the manual read handles the wrap
natively via 2's complement subtraction and updates prev_count.

When the IRQ handler subsequently runs, it sees the stale hardware overflow
bit still set and evaluates new_cnt >= prev_cnt. Will it then spuriously
add a full period (mask + 1) to the event count?


[Severity: High]
This is a pre-existing issue, but does missing cleanup of the hardware overflow
status bit when stopping or starting a counter lead to overcounting for
subsequently scheduled events?

drivers/perf/cxl_pmu.c:cxl_pmu_event_stop() {
    ...
	cfg &= ~(FIELD_PREP(CXL_PMU_COUNTER_CFG_INT_ON_OVRFLW, 1) |
		 FIELD_PREP(CXL_PMU_COUNTER_CFG_ENABLE, 1));
	writeq(cfg, base + CXL_PMU_COUNTER_CFG_REG(hwc->idx));
    ...
}

If cxl_pmu_event_stop disables a counter's interrupt but does not clear the
hardware overflow status bit in CXL_PMU_OVERFLOW_REG, the bit remains set.

drivers/perf/cxl_pmu.c:cxl_pmu_event_start() {
    ...
	cfg |= FIELD_PREP(CXL_PMU_COUNTER_CFG_INT_ON_OVRFLW, 1);
	cfg |= FIELD_PREP(CXL_PMU_COUNTER_CFG_FREEZE_ON_OVRFLW, 1);
	cfg |= FIELD_PREP(CXL_PMU_COUNTER_CFG_ENABLE, 1);
    ...
}

When cxl_pmu_event_start later reuses the counter index for a new event, it
initializes the counter to 0 and re-enables the interrupt without clearing the
pending overflow bit.

Could the IRQ handler process the stale overflow bit and incorrectly add mask + 1
to the new event's count?

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