Re: [PATCH v4 1/3] perf/cxlpmu: Fix 64-bit write to 32-bit HDM filter register

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The driver updates the counter configuration register using bitwise OR without first clearing the relevant fields, causing stale bits to accumulate from previous events.
- [High] The CPU hotplug instance is removed after the PMU is unregistered during driver teardown, causing a Use-After-Free if a CPU goes offline concurrently.
- [High] The `cxl_pmu_get_event_idx` function fails to set the `event_idx` output parameter for configurable counters, resulting in silently measuring the wrong events.
--

--- Patch [1]: [PATCH v4 1/3] perf/cxlpmu: Fix 64-bit write to 32-bit HDM filter register ---
commit 1a1b5ade4d63271a0649378648434d5656079fd5
Author: Davidlohr Bueso <dave@stgolabs.net>

perf/cxlpmu: Fix 64-bit write to 32-bit HDM filter register

The HDM decoder filter configuration register is 32 bits wide, but the
driver programs it with a 64-bit writeq(). The filter value never
exceeds 32 bits, so the upper half of the write is always zero and
lands in the adjacent Filter ID 1 (Channel/Rank/Bank) configuration
register at offset+4.

> diff --git a/drivers/perf/cxl_pmu.c b/drivers/perf/cxl_pmu.c
> index 68a54d97d2a8a..39b46550a5109 100644
> --- a/drivers/perf/cxl_pmu.c
> +++ b/drivers/perf/cxl_pmu.c
> @@ -635,7 +635,7 @@ static void cxl_pmu_event_start(struct perf_event *event, int flags)
>  			cfg = cxl_pmu_config2_get_hdm_decoder(event);
>  		else
>  			cfg = GENMASK(31, 0); /* No filtering if 0xFFFF_FFFF */
> -		writeq(cfg, base + CXL_PMU_FILTER_CFG_REG(hwc->idx, 0));
> +		writel(cfg, base + CXL_PMU_FILTER_CFG_REG(hwc->idx, 0));
>  	}
>  
>  	cfg = readq(base + CXL_PMU_COUNTER_CFG_REG(hwc->idx));
>  	cfg |= FIELD_PREP(CXL_PMU_COUNTER_CFG_EVENT_GRP_ID_IDX_MSK,
>  			  hwc->event_base);
>  	cfg |= FIELD_PREP(CXL_PMU_COUNTER_CFG_EVENTS_MSK,
>  			  cxl_pmu_config_get_mask(event));

[Severity: High]
This is a pre-existing issue, but does this bitwise OR accumulate stale bits
from previous events?

When setting CXL_PMU_COUNTER_CFG_EVENT_GRP_ID_IDX_MSK and
CXL_PMU_COUNTER_CFG_EVENTS_MSK, the fields are not cleared first. If a new
event is allocated to a previously used counter, it seems it will inherit
the old event's bits, resulting in a bitwise OR of both events.

Should the relevant masks be cleared before applying the new fields to prevent
incorrect statistics?


While reviewing this, I also noticed a few other pre-existing issues in
this file that might need addressing:

> drivers/perf/cxl_pmu.c:cxl_pmu_get_event_idx() {
> [ ... ]
> 		if (test_bit(i, info->conf_counter_bm) &&
> 		    test_bit(i, info->used_counter_bm)) {
> 			*counter_idx = i;
> 			return 0;
> 		}
> [ ... ]
> }

[Severity: High]
This isn't a bug introduced by this patch, but are we missing an assignment
to the event_idx output parameter here?

When a configurable counter is matched, *counter_idx is set, but it
appears *event_idx is omitted.

Since the caller initializes event_idx to 0, does this mean the counter
will always be programmed to count the event group at index 0, silently
ignoring the requested event group?


> drivers/perf/cxl_pmu.c:cxl_pmu_probe() {
> [ ... ]
> 	rc = cpuhp_state_add_instance(cxl_pmu_cpuhp_state_num, &info->node);
> 	if (rc)
> 		return rc;
> 
> 	rc = devm_add_action_or_reset(dev, cxl_pmu_cpuhp_remove, info);
> 	if (rc)
> 		return rc;
> 
> 	rc = perf_pmu_register(&info->pmu, info->pmu.name, -1);
> [ ... ]
> }

[Severity: High]
This is a pre-existing issue, but does the registration order here create a
use-after-free race during teardown?

The devm framework executes teardown actions in last-in first-out order.
Because the CPU hotplug instance is registered before the PMU, the PMU is
unregistered first during teardown.

When perf_pmu_unregister() frees the PMU's per-CPU context, if a CPU
hotplug offline event occurs concurrently:

cxl_pmu_offline_cpu()
   perf_pmu_migrate_context()

Could this access the freed per-CPU context since the hotplug instance
has not been removed yet?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260715191454.459673-1-dave@stgolabs.net?part=1