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

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Firmware PMU counters are completely disabled due to being omitted from the PMU's main counter mask.
- [High] Using 32-bit `unsigned long` bitmasks with `BIT(i)` for up to 64 counters causes false ID collisions on 32-bit RISC-V platforms.
- [High] Counter delegation discovery is a stub, causing the driver to disable all hardware counters on supported systems and incorrectly use SBI for verification.
- [High] Memory leak and fatal UAF/NULL-dereference on platform driver re-probe due to freed global pointer.
--

--- Patch [12]: [PATCH v9 12/20] RISC-V: perf: Modify the counter discovery mechanism ---
commit e2768c5a0a20d7c61d402570f684ea95ebbdf2e6
Author: Atish Patra <[email protected]>

    RISC-V: perf: Modify the counter discovery mechanism

This commit modifies the counter discovery mechanism to accommodate hybrid
PMU schemes where counter delegation is used for hardware counters and SBI
PMU is used for firmware counters. It tracks firmware counters in a
separate bitmask and updates the informational logging to reflect the
underlying interfaces.

> diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
> index a8a197d341c3c..1a14300599096 100644
> --- a/drivers/perf/riscv_pmu_sbi.c
> +++ b/drivers/perf/riscv_pmu_sbi.c

[ ... ]

> @@ -867,34 +884,44 @@ static int rvpmu_sbi_find_num_ctrs(void)
>  		return sbi_err_map_linux_errno(ret.error);
>  }
>  
> -static int rvpmu_sbi_get_ctrinfo(int nctr, unsigned long *mask)
> +static u32 rvpmu_deleg_find_ctrs(void)
> +{
> +	/* TODO */
> +	return 0;
> +}

[Severity: High]
Does returning 0 from this stub cause the driver to disable all hardware
counters on systems where counter delegation is available?

When counter delegation is available, this stub returns 0 delegated
hardware counters, but the code in rvpmu_sbi_get_ctrinfo() also explicitly
ignores the SBI fallback for hardware counters:

    } else if (cinfo.type == SBI_PMU_CTR_TYPE_HW &&
               !riscv_pmu_cdeleg_available_boot()) {

Will this result in 0 hardware counters being registered and all standard
hardware events being marked as unsupported when they are checked against
the empty mask?

> +
> +static int rvpmu_sbi_get_ctrinfo(u32 nsbi_ctr, u32 *num_fw_ctr, u32 *num_hw_ctr)
>  {
>  	struct sbiret ret;
> -	int i, num_hw_ctr = 0, num_fw_ctr = 0;
> +	int i;
>  	union sbi_pmu_ctr_info cinfo;
>  
> -	pmu_ctr_list = kzalloc_objs(*pmu_ctr_list, nctr);
> -	if (!pmu_ctr_list)
> -		return -ENOMEM;
> -
> -	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) {
> +			/* FW counters use SBI logical ids; skip any that collide */
> +			if (cmask & BIT(i)) {

[Severity: High]
Could this bitwise check cause false ID collisions on 32-bit RISC-V platforms?

Since cmask is an unsigned long, on RV32 it is 32 bits wide. RISC-V supports
up to 64 PMU counters. When processing counters >= 32, BIT(i) will wrap and
alias to BIT(i % 32), which could incorrectly trigger this warning and drop
valid firmware counters.

> +				pr_warn_once("SBI firmware counter %d overlaps a delegated hardware counter, ignoring it\n",
> +					     i);
> +				continue;
> +			}
> +			/* Track firmware counters in a different mask */
> +			firmware_cmask |= BIT(i);
> +			pmu_ctr_list[i].value = cinfo.value;
> +			*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);

[Severity: High]
Are firmware PMU counters inadvertently disabled by tracking them separately
from the main counter mask?

Here firmware_cmask tracks the firmware counters, but later in
rvpmu_device_probe(), the driver initializes pmu->cmask strictly using
cmask:

    pmu->cmask = cmask;

When a firmware event is requested, rvpmu_sbi_ctr_get_idx() validates it
against the main mask:

    if (!test_bit(idx, &rvpmu->cmask) || !pmu_ctr_list[idx].value)
        return -ENOENT;

Will this cause the validation to unconditionally fail for all firmware
counters?

> +			pmu_ctr_list[i].value = cinfo.value;
> +		}
>  	}

[ ... ]

> @@ -1158,16 +1185,54 @@ static void rvpmu_ctr_stop(struct perf_event *event, unsigned long flag)
>  	/* TODO: Counter delegation implementation */
>  }
>  
> -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]
Does this allocation leak memory and potentially cause a fatal crash if the
driver is re-probed?

If a user unbinds and rebinds the driver via sysfs, this probe will
unconditionally allocate pmu_ctr_list without cleaning up the previous
allocation, leaking the memory.

Then, when perf_pmu_register() fails because the PMU is already registered,
the error path in rvpmu_device_probe() will free the pointer:

    out_free:
        free_percpu(pmu->hw_events);
        kfree(pmu_ctr_list);
        pmu_ctr_list = NULL;

Since the original PMU remains active in the subsystem, would any
subsequent perf event operation dereference this freed and NULLed
pmu_ctr_list pointer?

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

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