Re: [PATCH RESEND v7 3/3] drivers/perf: riscv-iommu: protect shared state with a raw spinlock

[sashiko-bot@kernel.org]

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

New issues:
- [High] Taking a raw spinlock in a PMU's read callback can lead to an NMI deadlock.

Pre-existing issues:
- [High] The IRQ handler updates the count for newly added but stopped events, leading to massive data corruption.
--

commit 1a00f5bb6a14fe9e1b35b9a9aa789a513bd04f90
Author: Zong Li <zong.li@sifive.com>

drivers/perf: riscv-iommu: protect shared state with a raw spinlock

This commit introduces a raw spinlock to protect shared PMU state, ensuring
that the interrupt handler doesn't conflict with perf callbacks on the same
CPU.

> diff --git a/drivers/perf/riscv_iommu_pmu.c b/drivers/perf/riscv_iommu_pmu.c
> index f6acd56f2f616..ee2f6d1fbece4 100644
> --- a/drivers/perf/riscv_iommu_pmu.c
> +++ b/drivers/perf/riscv_iommu_pmu.c

[ ... ]

> @@ -556,18 +586,26 @@ static int riscv_iommu_pmu_add(struct perf_event *event, int flags)
>  
>  static void riscv_iommu_pmu_read(struct perf_event *event)
>  {
> +	struct riscv_iommu_pmu *pmu = to_riscv_iommu_pmu(event->pmu);
> +	unsigned long irqflags;
> +
> +	raw_spin_lock_irqsave(&pmu->lock, irqflags);
>  	riscv_iommu_pmu_update(event);
> +	raw_spin_unlock_irqrestore(&pmu->lock, irqflags);
>  }

[Severity: High]
Can taking a raw spinlock in a PMU's read callback lead to an NMI deadlock?

If a user puts an IOMMU PMU event in a BPF perf event array map, a BPF program
attached to NMI-context events (like hardware perf counter overflows) can call
bpf_perf_event_read(), which invokes this pmu->read() callback from within the
NMI context.

If this NMI interrupts the same CPU while it is executing another PMU function
that already holds pmu->lock (such as riscv_iommu_pmu_irq_handler() or
riscv_iommu_pmu_add()), the read callback will spin forever attempting to
acquire the same lock.

Since riscv_iommu_pmu_update() already uses a lock-free local64_cmpxchg() to
handle reentrancy, does this function need the spinlock?

[ ... ]

> @@ -672,6 +722,8 @@ static irqreturn_t riscv_iommu_pmu_irq_handler(int irq, void *dev_id)
>  
>  	riscv_iommu_pmu_start_all(pmu, inhibit);
>  
> +	raw_spin_unlock_irqrestore(&pmu->lock, irqflags);
> +
>  	return IRQ_HANDLED;
>  }

[Severity: High]
This isn't a bug introduced by this patch, but does the IRQ handler update
the count for newly added but stopped events, leading to data corruption?

Inside riscv_iommu_pmu_irq_handler(), if an overflow bit is set, the handler
updates the event:

	if (ovf) {
		bitmap_from_u64(ovf_bitmap, ovf);
		for_each_set_bit(idx, ovf_bitmap, pmu->num_counters) {
			struct perf_event *event = pmu->events[idx];

			if (event) {
				riscv_iommu_pmu_update(event);
				riscv_iommu_pmu_set_period(event);
			}

If Event A on counter X overflows and is then deleted via
riscv_iommu_pmu_del(), the counter is disabled but its OF bit remains set.

If Event B is subsequently added via riscv_iommu_pmu_add() without
PERF_EF_START, it is assigned the same counter X with prev_count
initialized to 0.

When the delayed interrupt is handled, the handler reads the set overflow bit
for counter X, retrieves Event B, and blindly updates it since there is no
check for whether the event is stopped (e.g., !(event->hw.state &
PERF_HES_STOPPED)).

Because prev_count is 0, riscv_iommu_pmu_update() computes delta = now - prev,
applying the full hardware counter value to Event B's count.

Should this sequence check if the event is stopped before updating?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260828085819.4076449-1-zong.li@sifive.com?part=3