Re: [PATCH v7 12/22] RISC-V: perf: Modify the counter discovery mechanism

[sashiko-bot@kernel.org]

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

New issues:
- [High] Hardware counter physical indices and SBI logical counter indices for firmware counters can overlap, causing array overwrites and state clobbering in `pmu_ctr_list` and the per-CPU `events` array.
- [Medium] Firmware counters are not stopped in `rvpmu_sbi_stop_all()` when counter delegation is disabled, leaving them running unexpectedly.

Pre-existing issues:
- [High] `cmask` and `firmware_cmask` are defined as `unsigned long` and accessed via `BIT(i)` and `for_each_set_bit()`, causing shift overflow and out-of-bounds memory reads on 32-bit architectures.
- [High] Missing `.remove` hook combined with global `pmu_ctr_list` overwrite causes a memory leak and a subsequent NULL pointer dereference on re-probe failure.
--

commit 98634f15c972a5ad2282dff7c8f2da3bfc47efa8
Author: Atish Patra <atishp@rivosinc.com>

RISC-V: perf: Modify the counter discovery mechanism

If both counter delegation and SBI PMU are present, counter delegation
is used for hardware counters while the SBI PMU is used for firmware
counters. The driver is updated to probe via SBI PMU to distinguish
the firmware counters and cache the discovery info.

> diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
> index 80c7249ee1662..d44f47613aa1a 100644
> --- a/drivers/perf/riscv_pmu_sbi.c
> +++ b/drivers/perf/riscv_pmu_sbi.c
[ ... ]
> @@ -101,6 +116,8 @@ static unsigned int riscv_pmu_irq;
>  
>  /* Cache the available counters in a bitmask */
>  static unsigned long cmask;
> +/* Cache the available firmware counters in another bitmask */
> +static unsigned long firmware_cmask;

[Severity: High]
This is a pre-existing issue, but on 32-bit architectures (RV32), unsigned
long is 32 bits wide, while RISCV_MAX_COUNTERS is 64.

Does this cause shift overflows when accessed via BIT(i) where i can reach
up to 63?

Additionally, does using for_each_set_bit() with this 32-bit mask cause
out-of-bounds memory reads, as the macro expects an array large enough for
64 bits?

[ ... ]
> @@ -868,34 +885,38 @@ static int rvpmu_sbi_find_num_ctrs(void)
[ ... ]
> -static int rvpmu_sbi_get_ctrinfo(int nctr, unsigned long *mask)
> +static int rvpmu_sbi_get_ctrinfo(u32 nsbi_ctr, u32 *num_fw_ctr, u32 *num_hw_ctr)
>  {
[ ... ]
> -	for (i = 0; i < nctr; i++) {
> +	for (i = 0; i < nsbi_ctr; i++) {
>  		ret = sbi_ecall(SBI_EXT_PMU, SBI_EXT_PMU_COUNTER_GET_INFO, i, 0, 0, 0, 0, 0);
>  		if (ret.error)
>  			/* The logical counter ids are not expected to be contiguous */
>  			continue;
>  
> -		*mask |= BIT(i);
> -
>  		cinfo.value = ret.value;
> -		if (cinfo.type == SBI_PMU_CTR_TYPE_FW)
> -			num_fw_ctr++;
> -		else
> -			num_hw_ctr++;
> -		pmu_ctr_list[i].value = cinfo.value;
> +		if (cinfo.type == SBI_PMU_CTR_TYPE_FW) {
> +			/* Track firmware counters in a different mask */
> +			firmware_cmask |= BIT(i);
> +			pmu_ctr_list[i].value = cinfo.value;

[Severity: High]
Can the hardware counter physical indices and the SBI logical counter
indices overlap, causing array overwrites in pmu_ctr_list?

When both counter delegation and the SBI PMU are available, pmu_ctr_list
is first populated using hardware physical CSR indices (0-31) in
rvpmu_deleg_find_ctrs().

Then, rvpmu_sbi_get_ctrinfo() populates firmware counters using the SBI
logical index i. Because the SBI specification allows logical indices to
start at 0, an SBI implementation might assign logical indices to firmware
counters that overlap with the physical CSR indices.

Could this silently overwrite the hardware counter definitions in
pmu_ctr_list[i], and later corrupt the active cpuc->events[idx] array when
both types of counters share overlapping indices?

[Severity: Medium]
Does splitting firmware counters into firmware_cmask and intentionally
excluding them from cmask leave firmware counters running unexpectedly?

In rvpmu_sbi_stop_all(), the code only stops counters present in pmu->cmask:

static inline void rvpmu_sbi_stop_all(struct riscv_pmu *pmu)
{
	...
	sbi_ecall(SBI_EXT_PMU, SBI_EXT_PMU_COUNTER_STOP,
		  0, pmu->cmask, SBI_PMU_STOP_FLAG_RESET, 0, 0, 0);
}

When counter delegation is disabled, the fallback path calls
rvpmu_sbi_stop_all(pmu), which now exclusively stops hardware counters.

Could this leave firmware counters running across CPU hotplug and PMU
initialization, potentially leading to stale counts and unexpected overflows?

> +			*num_fw_ctr = *num_fw_ctr + 1;
> +		} else if (cinfo.type == SBI_PMU_CTR_TYPE_HW &&
> +			   !riscv_pmu_cdeleg_available_boot()) {
> +			*num_hw_ctr = *num_hw_ctr + 1;
> +			cmask |= BIT(i);
> +			pmu_ctr_list[i].value = cinfo.value;
> +		}
>  	}
[ ... ]
> @@ -1159,16 +1180,48 @@ static void rvpmu_ctr_stop(struct perf_event *event, unsigned long flag)
[ ... ]
> -static int rvpmu_find_num_ctrs(void)
> +static int rvpmu_find_ctrs(void)
>  {
> -	return rvpmu_sbi_find_num_ctrs();
> -	/* TODO: Counter delegation implementation */
> -}
> +	int num_sbi_counters = 0;
> +	u32 num_deleg_counters = 0;
> +	u32 num_hw_ctr = 0, num_fw_ctr = 0, num_ctr = 0;
> +	/*
> +	 * We don't know how many firmware counters are available. Just allocate
> +	 * for maximum counters the driver can support. The default is 64 anyways.
> +	 */
> +	pmu_ctr_list = kcalloc(RISCV_MAX_COUNTERS, sizeof(*pmu_ctr_list),
> +			       GFP_KERNEL);

[Severity: High]
This is a pre-existing issue, but does missing a .remove hook combined with
this global pmu_ctr_list overwrite cause a memory leak and subsequent NULL
pointer dereference on re-probe failure?

The platform driver lacks a .remove callback and does not set
suppress_bind_attrs, meaning it can be unbound via sysfs, leaving the PMU
registered and active.

If the driver is rebound, rvpmu_device_probe() executes again and calls
rvpmu_find_ctrs(), which unconditionally allocates pmu_ctr_list via kcalloc,
overwriting the global pointer and leaking the original memory.

When perf_pmu_register() then fails because the PMU is already registered,
the error path in rvpmu_device_probe() calls kfree(pmu_ctr_list) and sets it
to NULL.

Could the still-active PMU from the first probe then dereference this NULL
pointer during any subsequent perf event operations?

> +	if (!pmu_ctr_list)
> +		return -ENOMEM;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260622-counter_delegation-v7-0-0ba2fd34614e@meta.com?part=12