Re: [PATCH v1 2/2] perf: RISC-V: store available counter mask as bitmap

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

New issues:
- [High] The patch fails to fully support matching 64 counters on RV32, as it hardcodes `cmask[0]` and `cbase = 0` in SBI counter match queries, effectively ignoring counters 32-63.

Pre-existing issues:
- [High] An out-of-bounds counter base index is passed to `SBI_EXT_PMU_COUNTER_START` in the snapshot overflow handler.
- [High] Global `cmask` is not cleared on probe failure, leading to a NULL pointer dereference in `riscv_pmu_get_hpm_info()`.
- [High] Incorrect snapshot array indexing and overflow mask shifting for PMU counters >= 32 on RV32.
--

--- Patch [2]: [PATCH v1 2/2] perf: RISC-V: store available counter mask as bitmap ---
commit 0c40d37d24749711315dcc35e091383a23b8eeb6
Author: Xixin Liu <[email protected]>
Subject: perf: RISC-V: store available counter mask as bitmap

The available-counter mask was previously a single unsigned long, which
caused out-of-bounds reads on RV32 when iterating up to RISCV_MAX_COUNTERS.
This commit transitions the mask to use DECLARE_BITMAP and related helpers
to prevent reads past the object and properly stop counters across words.

> diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
> --- a/drivers/perf/riscv_pmu_sbi.c
> +++ b/drivers/perf/riscv_pmu_sbi.c
> @@ -364,7 +364,7 @@ static void pmu_sbi_check_event(struct sbi_pmu_event_data *edata)
>  	struct sbiret ret;
>  
>  	ret = sbi_ecall(SBI_EXT_PMU, SBI_EXT_PMU_COUNTER_CFG_MATCH,
> -			0, cmask, 0, edata->event_idx, 0, 0);
> +			0, cmask[0], 0, edata->event_idx, 0, 0);

[Severity: High]
Does this code fail to match all 64 counters on RV32?

By hardcoding cmask[0] and a base of 0, it appears to ignore counters 32-63
during the SBI counter match queries.

Should this issue subsequent sbi_ecall queries for cmask[1] with cbase = 32
to ensure the upper half of the counters are checked?

[ ... ]

> @@ -541,7 +541,7 @@ static int pmu_sbi_ctr_get_idx(struct perf_event *event)
>  	struct cpu_hw_events *cpuc = this_cpu_ptr(rvpmu->hw_events);
>  	struct sbiret ret;
>  	int idx;
> -	uint64_t cbase = 0, cmask = rvpmu->cmask;
> +	uint64_t cbase = 0, cmask = rvpmu->cmask[0];

[Severity: High]
Does this code also fail to support matching counters >= 32 on RV32?

Similar to pmu_sbi_check_event(), initializing cmask to rvpmu->cmask[0] and
using it as the sole mask for the match call will result in -ENOENT for
events that could be satisfied by the upper counters.

[ ... ]

> @@ -1451,7 +1458,7 @@ static int pmu_sbi_device_probe(struct platform_device *pdev)
>  	}
>  
>  	/* cache all the information about counters now */
> -	if (pmu_sbi_get_ctrinfo(num_counters, &cmask))
> +	if (pmu_sbi_get_ctrinfo(num_counters, cmask))
>  		goto out_free;

[Severity: High]
This is a pre-existing issue, but does the global cmask get left populated
with stale data on probe failure?

If pmu_sbi_device_probe() fails after pmu_sbi_get_ctrinfo() (for instance,
during IRQ setup), the out_free error path frees and nullifies
pmu_ctr_list:

drivers/perf/riscv_pmu_sbi.c:pmu_sbi_device_probe() {
    ...
out_free:
    free_percpu(pmu->hw_events);
    kfree(pmu_ctr_list);
    pmu_ctr_list = NULL;
    ...
}

However, the global cmask bitmap retains its bits. Later, if KVM calls
riscv_pmu_get_hpm_info(), it will bypass the bitmap_empty() check,
iterate over the stale bits, and dereference the NULL pmu_ctr_list array.

[Severity: High]
This is a pre-existing issue, but can the PMU counters fail to restart
after an overflow when snapshots are enabled?

In pmu_sbi_start_ovf_ctrs_snapshot(), the loop-exhausted idx variable is
used to calculate the base counter index instead of the outer loop
variable i:

drivers/perf/riscv_pmu_sbi.c:pmu_sbi_start_ovf_ctrs_snapshot() {
    ...
    for_each_set_bit(idx, &cpu_hw_evt->used_hw_ctrs[i], BITS_PER_LONG) {
        ...
    }
    
    sbi_ecall(SBI_EXT_PMU, SBI_EXT_PMU_COUNTER_START, idx * BITS_PER_LONG,
              ...
}

The for_each_set_bit() loop terminates with idx equal to BITS_PER_LONG.
Passing idx * BITS_PER_LONG (e.g., 1024 on RV32) as the counter_idx_base
to SBI_EXT_PMU_COUNTER_START will cause the firmware to reject the
out-of-bounds base index.

[Severity: High]
This is a pre-existing issue, but does this code corrupt snapshot memory
for counters >= 32 on RV32?

When stopping hardware counters, the snapshot counter values are read using
relative indices instead of absolute ones:

drivers/perf/riscv_pmu_sbi.c:pmu_sbi_stop_hw_ctrs() {
    ...
    for_each_set_bit(idx, &cpu_hw_evt->used_hw_ctrs[i], BITS_PER_LONG)
        cpu_hw_evt->snapshot_cval_shcopy[i * BITS_PER_LONG + idx] =
                    sdata->ctr_values[idx];
    ...
}

Additionally, the absolute 64-bit overflow mask is incorrectly shifted:

    temp_ctr_overflow_mask |= sdata->ctr_overflow_mask << (i * BITS_PER_LONG);

For i = 1 (counters 32-63), this shifts the valid overflow bits out of
bounds.

Furthermore, in pmu_sbi_start_ovf_ctrs_snapshot(), the snapshot counter
values are written back using relative indices:

    sdata->ctr_values[idx] = ...

This overwrites the first 32 counters' values instead of writing to the
counters 32-63 region.

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