Re: [PATCH] perf: RISC-V: check cpu_hw_evt before dereference in overflow IRQ

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

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but skipping non-sampling events during overflow handling fails to clear their hardware overflow state, causing an interrupt storm.
- [High] This problem wasn't introduced by this patch, but the `BIT()` macro is improperly used with 64-bit masks, causing undefined behavior and corruption on RV32 systems when counter indices are >= 32.
- [High] This problem wasn't introduced by this patch, but incorrect interrupt pending bit clearance bypasses alternative instructions, causing interrupt storms on custom PMUs (e.g., Andes).
- [High] This problem wasn't introduced by this patch, but global variable `cmask` is defined as a 32-bit `unsigned long` on RV32 but is processed as a 64-bit bitmap, causing out-of-bounds memory reads.
- [High] This problem wasn't introduced by this patch, but restarting overflowed hardware counters incorrectly mixes `unsigned long` arrays with `u64` masks, breaking counter isolation for indices >= 32 on RV32 systems.
--

--- Patch [1]: [PATCH] perf: RISC-V: check cpu_hw_evt before dereference in overflow IRQ ---
commit 4fa941d0eec411e362522077608e4fec275e7349
Author: Xixin Liu <[email protected]>

perf: RISC-V: check cpu_hw_evt before dereference in overflow IRQ

The overflow IRQ handler dereferences cpu_hw_evt before the null check.
Move the check first. Defensive only; the cookie is valid on the normal
path today.

> diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
> index 50220f7b46d9b..cb7148d0c87ad 100644
> --- a/drivers/perf/riscv_pmu_sbi.c
> +++ b/drivers/perf/riscv_pmu_sbi.c
> @@ -1050,11 +1050,13 @@ static irqreturn_t pmu_sbi_ovf_handler(int irq, void *dev)
>  	u64 overflowed_ctrs = 0;
>  	struct cpu_hw_events *cpu_hw_evt = dev;
>  	u64 start_clock = sched_clock();
> -	struct riscv_pmu_snapshot_data *sdata = cpu_hw_evt->snapshot_addr;
> +	struct riscv_pmu_snapshot_data *sdata;
>  
>  	if (WARN_ON_ONCE(!cpu_hw_evt))
>  		return IRQ_NONE;
>  
> +	sdata = cpu_hw_evt->snapshot_addr;
> +
>  	/* Firmware counter don't support overflow yet */
>  	fidx = find_first_bit(cpu_hw_evt->used_hw_ctrs, RISCV_MAX_COUNTERS);
>  	if (fidx == RISCV_MAX_COUNTERS) {

[Severity: High]
This isn't a bug introduced by this patch, but does clearing the interrupt
pending bit manually here bypass alternative instructions?

For the Andes custom PMU, riscv_pmu_irq_num evaluates to 274. Using
BIT(274) on a 64-bit integer overflows and wraps to BIT(18), which would
erroneously clear bit 18 in the standard CSR_SIP register instead of using
the custom register.

Could this cause an interrupt storm on custom PMUs like Andes?


[Severity: High]
This problem wasn't introduced by this patch, but looking further down in
pmu_sbi_ovf_handler(), does skipping non-sampling events during overflow
handling fail to clear their hardware overflow state?

drivers/perf/riscv_pmu_sbi.c:pmu_sbi_ovf_handler() {
    ...
		/* Skip if invalid event or user did not request a sampling */
		if (!event || !is_sampling_event(event))
			continue;
    ...
}

When a non-sampling counting event overflows, its bit isn't added to
overflowed_ctrs. 

Consequently, when restarting the counter, the
SBI_PMU_START_FLAG_SET_INIT_VALUE flag isn't set, which means the hardware
overflow state (scountovf) isn't cleared and the interrupt is immediately
re-asserted.

Will this cause an interrupt storm?


[Severity: High]
This is a pre-existing issue, but does using the BIT() macro against 64-bit
variables like overflow and overflowed_ctrs cause undefined behavior on
32-bit RISC-V systems?

drivers/perf/riscv_pmu_sbi.c:pmu_sbi_ovf_handler() {
    ...
		/* check if the corresponding bit is set in scountovf or overflow mask in shmem */
		if (!(overflow & BIT(hidx)))
			continue;

		/*
		 * Keep a track of overflowed counters so that they can be started
		 * with updated initial value.
		 */
		overflowed_ctrs |= BIT(lidx);
    ...
}

On RV32, BIT() resolves to a 32-bit unsigned long, so shifting by 32 or more
results in truncation or wrapping. 

Should this use BIT_ULL() instead?


[Severity: High]
This problem wasn't introduced by this patch, but is cmask undersized
for a 64-bit bitmap on 32-bit architectures?

drivers/perf/riscv_pmu_sbi.c:
...
static unsigned long cmask;
...

drivers/perf/riscv_pmu_sbi.c:pmu_sbi_event_map() {
    ...
	for_each_set_bit(i, &cmask, RISCV_MAX_COUNTERS) {
    ...
}

On RV32, unsigned long is 4 bytes, but for_each_set_bit() with
RISCV_MAX_COUNTERS (64) expects an 8-byte buffer, which would cause an
out-of-bounds read into adjacent memory. 

Should this be defined using DECLARE_BITMAP(cmask, RISCV_MAX_COUNTERS)?


[Severity: High]
This is a pre-existing issue, but does this bitwise operation incorrectly mix
a 32-bit unsigned long element (on RV32) with a 64-bit u64 mask?

drivers/perf/riscv_pmu_sbi.c:pmu_sbi_start_ovf_ctrs_sbi() {
    ...
		ctr_start_mask = cpu_hw_evt->used_hw_ctrs[i] & ~ctr_ovf_mask;
    ...
}

For i=1 (handling counters 32-63), the 64-bit result is truncated when
assigned to the 32-bit ctr_start_mask, erroneously evaluating against the
lower 32 bits of ctr_ovf_mask instead of the upper 32 bits.

Could this break counter isolation for indices >= 32?

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