Re: [PATCH] perf: riscv: fix PMU overflow counter restart state

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Accumulation of bits in `ctr_overflow_mask` causes phantom PMU overflows and infinite loops.
- [High] Batch PMU operations clobber stopped counter snapshot values on RV32.
- [High] Using BIT() for a potentially large IRQ number causes undefined behavior and fails to clear the interrupt.
- [High] The 'cmask' variable is undersized on RV32, causing out-of-bounds reads and undefined behavior.
- [Medium] Mapping firmware counters unintentionally enables/disables userspace access to the hardware cycle counter.
--

commit 5c2027925f34b7a9d276d3526dd841300147c576
Author: Pengpeng Hou <pengpeng@iscas.ac.cn>

perf: riscv: fix PMU overflow counter restart state

This commit fixes PMU overflow counter restart states on RISC-V by applying
the correct overflow-mask word to each used-counter word. It updates the
snapshot path and overflow handler to use 64-bit bit operations for global
counter indices. Additionally, it ensures the snapshot start base is correctly
derived from the outer word index.

> diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
> index 50220f7b46d9b..6cfb1fc0ec6e1 100644
> --- a/drivers/perf/riscv_pmu_sbi.c
> +++ b/drivers/perf/riscv_pmu_sbi.c
> @@ -962,7 +962,9 @@ static inline void pmu_sbi_start_ovf_ctrs_sbi(struct cpu_hw_events *cpu_hw_evt,
>  	u64 init_val = 0;
>  
>  	for (i = 0; i < BITS_TO_LONGS(RISCV_MAX_COUNTERS); i++) {
> -		ctr_start_mask = cpu_hw_evt->used_hw_ctrs[i] & ~ctr_ovf_mask;
> +		unsigned long word_ovf_mask =
> +			ctr_ovf_mask >> (i * BITS_PER_LONG);
> +		ctr_start_mask = cpu_hw_evt->used_hw_ctrs[i] & ~word_ovf_mask;
>  		/* Start all the counters that did not overflow in a single shot */
>  		if (ctr_start_mask) {
>  			sbi_ecall(SBI_EXT_PMU, SBI_EXT_PMU_COUNTER_START, i * BITS_PER_LONG,

[ ... ]

[Severity: High]
This is a pre-existing issue, but can the 64-bit mask corruption cause phantom
PMU overflows and infinite loops?

On RV32, when stopping counters across multiple words:

drivers/perf/riscv_pmu_sbi.c:pmu_sbi_stop_hw_ctrs() {
...
    temp_ctr_overflow_mask |= sdata->ctr_overflow_mask << (i * BITS_PER_LONG);
...
    sdata->ctr_overflow_mask = temp_ctr_overflow_mask;
...
}

The OS fails to clear the 64-bit mask sdata->ctr_overflow_mask before calling
SBI_EXT_PMU_COUNTER_STOP for i=1. Firmware ORs relative overflow bits into
this uncleared mask, mixing base 0 and base 32 overflows.

Does the kernel then shift and OR this corrupted mask into
temp_ctr_overflow_mask, accumulating stale bits indefinitely and causing
phantom interrupts?

[Severity: High]
This is a pre-existing issue, but does this batch PMU operation clobber
stopped counter snapshot values on RV32?

If all counters in used_hw_ctrs[0] are already stopped,
SBI_EXT_PMU_COUNTER_STOP returns an error, skipping the save of their
snapshot values to snapshot_cval_shcopy:

drivers/perf/riscv_pmu_sbi.c:pmu_sbi_stop_hw_ctrs() {
...
    ret = sbi_ecall(SBI_EXT_PMU, SBI_EXT_PMU_COUNTER_STOP, i * BITS_PER_LONG,
                    cpu_hw_evt->used_hw_ctrs[i], flag, 0, 0, 0);
...
    sdata->ctr_values[idx] = cpu_hw_evt->snapshot_cval_shcopy[idx];
...
}

When i=1 runs for counters 32..63, does the firmware write their snapshot into
sdata->ctr_values[0..31], clobbering the values for counters 0..31?

Does the kernel then restore snapshot_cval_shcopy[0..31] (which remains 0)
into sdata->ctr_values, destroying the saved snapshot of the already stopped
counters?

[Severity: Medium]
This is a pre-existing issue, but does mapping firmware counters
unintentionally toggle userspace access to the hardware cycle counter?

When userspace maps a firmware counter, pmu_sbi_event_mapped() unconditionally
calls pmu_sbi_set_scounteren():

drivers/perf/riscv_pmu_sbi.c:pmu_sbi_set_scounteren() {
...
    csr_write(CSR_SCOUNTEREN,
              csr_read(CSR_SCOUNTEREN) | BIT(pmu_sbi_csr_index(event)));
...
}

This calls pmu_sbi_csr_index(), which subtracts CSR_CYCLE from the
uninitialized csr field of the firmware counter:

drivers/perf/riscv_pmu_sbi.c:pmu_sbi_csr_index() {
...
    return pmu_ctr_list[event->hw.idx].csr - CSR_CYCLE;
...
}

Since the csr field is 0, the result -3072 truncates to 0 in uint8_t.

Does this erroneously toggle SCOUNTEREN[0], inadvertently enabling or
disabling hardware access to the cycle counter?

[Severity: High]
This is a pre-existing issue, but does using BIT() for a potentially large IRQ
number cause undefined behavior here?

drivers/perf/riscv_pmu_sbi.c:pmu_sbi_ovf_handler() {
...
    if (fidx == RISCV_MAX_COUNTERS) {
        csr_clear(CSR_SIP, BIT(riscv_pmu_irq_num));
        return IRQ_NONE;
    }
...
}

For the ANDES custom PMU, riscv_pmu_irq_num is 274. If this path executes,
does shifting by 274 trigger undefined behavior?

Does this also fail to clear the pending IRQ since it targets CSR_SIP instead
of the custom ANDES_CSR_SLIP register correctly handled by
ALT_SBI_PMU_OVF_CLEAR_PENDING?

[Severity: High]
This is a pre-existing issue, but is the 'cmask' variable undersized on RV32,
causing out-of-bounds reads and undefined behavior?

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

In pmu_sbi_get_ctrinfo(), BIT(i) is used for i up to 63:

drivers/perf/riscv_pmu_sbi.c:pmu_sbi_get_ctrinfo() {
...
    *mask |= BIT(i);
...
}

And in riscv_pmu_get_hpm_info():

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

Since cmask is an unsigned long, it is 32 bits on RV32.

Does for_each_set_bit() read 64 bits, overrunning the 32-bit cmask variable
into adjacent memory and causing undefined behavior when i >= 32?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260828142120.12616-1-pengpeng@iscas.ac.cn?part=1