Re: [PATCH v1 1/2] perf: RISC-V: use BIT_ULL for u64 overflow masks

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Invalid SBI base index due to out-of-scope loop variable in pmu_sbi_start_ovf_ctrs_snapshot().
- [High] Incorrect mask arithmetic in pmu_sbi_start_ovf_ctrs_sbi() on RV32 mixes up counter indices.
- [High] Buffer over-read and undefined shift behavior due to global cmask being an unsigned long.
- [High] Early exit path in pmu_sbi_ovf_handler() triggers undefined shift and incorrect CSR access on Andes custom PMUs.
- [High] Absolute 64-bit SBI PMU snapshot shared memory structures are incorrectly shifted or indexed relative to cbase on RV32.
--

--- Patch [1]: [PATCH v1 1/2] perf: RISC-V: use BIT_ULL for u64 overflow masks ---
commit d452f12b31f1955f29f737a7fc0bc5084ebd71af
Author: Xixin Liu <liuxixin@kylinos.cn>
Subject: perf: RISC-V: use BIT_ULL for u64 overflow masks

This patch replaces BIT() with BIT_ULL() for u64 overflow status and
restart masks in the RISC-V SBI PMU driver. This fixes truncation and
wrapping issues on RV32 where BIT() resolves to an unsigned long shift.

> diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
> index 50220f7b46d9b..8ea5ae6173471 100644
> --- a/drivers/perf/riscv_pmu_sbi.c
> +++ b/drivers/perf/riscv_pmu_sbi.c
> @@ -1002,7 +1002,7 @@ static inline void pmu_sbi_start_ovf_ctrs_snapshot(struct cpu_hw_events *cpu_hw_
>  	struct riscv_pmu_snapshot_data *sdata = cpu_hw_evt->snapshot_addr;
>  
>  	for_each_set_bit(idx, cpu_hw_evt->used_hw_ctrs, RISCV_MAX_COUNTERS) {
> -		if (ctr_ovf_mask & BIT(idx)) {
> +		if (ctr_ovf_mask & BIT_ULL(idx)) {
>  			event = cpu_hw_evt->events[idx];
>  			hwc = &event->hw;
>  			max_period = riscv_pmu_ctr_get_width_mask(event);

[Severity: High]
This isn't a bug introduced by this patch, but since we're fixing issues in
pmu_sbi_start_ovf_ctrs_snapshot(), there is an SBI base index problem slightly
lower in this same function:

pmu_sbi_start_ovf_ctrs_snapshot() {
    ...
    for (i = 0; i < BITS_TO_LONGS(RISCV_MAX_COUNTERS); i++) {
        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,
                  cpu_hw_evt->used_hw_ctrs[i], flag, 0, 0, 0);
    }
}

Does this pass an invalid base index to the SBI firmware? The for_each_set_bit()
macro terminates with idx equal to BITS_PER_LONG. So idx * BITS_PER_LONG would
evaluate to something like 4096 on RV64, which seems like it would cause an
SBI_ERR_INVALID_PARAM error. Should this use i * BITS_PER_LONG instead?

[Severity: High]
This is another pre-existing issue in pmu_sbi_start_ovf_ctrs_snapshot() related
to RV32 shared memory structures:

pmu_sbi_start_ovf_ctrs_snapshot() {
    ...
    for (i = 0; i < BITS_TO_LONGS(RISCV_MAX_COUNTERS); i++) {
        for_each_set_bit(idx, &cpu_hw_evt->used_hw_ctrs[i], BITS_PER_LONG)
            sdata->ctr_values[idx] =
                cpu_hw_evt->snapshot_cval_shcopy[idx + i * BITS_PER_LONG];
    ...
}

Since ctr_values is a 64-element array indexed by absolute counter IDs, does
using the relative idx (0-31) incorrectly overwrite the states of counters 0-31
when processing chunk i=1 (counters 32-63)?

[ ... ]

> @@ -1109,14 +1109,14 @@ static irqreturn_t pmu_sbi_ovf_handler(int irq, void *dev)
>  			hidx = info->csr - CSR_CYCLE;
>  
>  		/* check if the corresponding bit is set in scountovf or overflow mask in shmem */
> -		if (!(overflow & BIT(hidx)))
> +		if (!(overflow & BIT_ULL(hidx)))
>  			continue;
>  
>  		/*
>  		 * Keep a track of overflowed counters so that they can be started
>  		 * with updated initial value.
>  		 */
> -		overflowed_ctrs |= BIT(lidx);
> +		overflowed_ctrs |= BIT_ULL(lidx);
>  		hw_evt = &event->hw;
>  		/* Update the event states here so that we know the state while reading */
>  		hw_evt->state |= PERF_HES_STOPPED;

[Severity: High]
This is a pre-existing issue, but while looking at pmu_sbi_ovf_handler(), the
early exit path near the beginning of the function caught my eye:

pmu_sbi_ovf_handler() {
    ...
    fidx = find_first_bit(cpu_hw_evt->used_hw_ctrs, RISCV_MAX_COUNTERS);
    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 can be assigned values like 274
(ANDES_SLI_CAUSE_BASE + ANDES_RV_IRQ_PMOVI). Does BIT(274) cause an undefined
out-of-bounds shift? Furthermore, does clearing CSR_SIP work for Andes PMUs
which use custom SLIE CSRs for these IRQs? Could this lead to an unacknowledged
interrupt storm?

[Severity: High]
This is a pre-existing issue, but I noticed another mask arithmetic problem on
RV32 that might cause broken PMU statistics. In pmu_sbi_start_ovf_ctrs_sbi():

pmu_sbi_start_ovf_ctrs_sbi() {
    ...
    for (i = 0; i < BITS_TO_LONGS(RISCV_MAX_COUNTERS); i++) {
        ...
        ctr_start_mask = cpu_hw_evt->used_hw_ctrs[i] & ~ctr_ovf_mask;
        ...
}

On RV32, when i = 1 (counters 32-63), used_hw_ctrs[1] is a 32-bit unsigned long,
but ctr_ovf_mask is a 64-bit absolute mask. Since ~ctr_ovf_mask isn't shifted
down by i * BITS_PER_LONG, doesn't this evaluate the usage bits for counters
32-63 against the overflow status of counters 0-31?

[Severity: High]
This is also a pre-existing issue, but related to 32-bit mask truncation, is the
global cmask variable affected?

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

pmu_sbi_get_hpm_info() {
    ...
    for_each_set_bit(i, &cmask, RISCV_MAX_COUNTERS) {
    ...
}

On RV32, unsigned long is 32 bits, but RISCV_MAX_COUNTERS is 64. Does calling
for_each_set_bit() here force a 64-bit read that accesses out-of-bounds memory
adjacent to cmask? Should cmask be declared with DECLARE_BITMAP()?

[Severity: High]
Finally, this is a pre-existing issue, but similar to the ctr_values array, the
overflow mask is also shifted out-of-bounds on RV32 in pmu_sbi_stop_hw_ctrs():

pmu_sbi_stop_hw_ctrs() {
    ...
    for (i = 0; i < BITS_TO_LONGS(RISCV_MAX_COUNTERS); i++) {
        ...
        temp_ctr_overflow_mask |= sdata->ctr_overflow_mask << (i * BITS_PER_LONG);
        ...
}

On RV32, shifting the 64-bit ctr_overflow_mask by 32 bits when i=1 pushes
the absolute bits 32-63 out of bounds. Doesn't this discard the overflow state
for counters 32-63?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1786092600.git.liuxixin@kylinos.cn?part=1