Re: [PATCH v11 1/2] drivers/perf: riscv-iommu: add risc-v iommu pmu driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Missing lock in riscv_iommu_pmu_read() causes an ABA problem leading to PMU counter corruption.
- [Medium] Race condition between riscv_iommu_pmu_stop() and the PMU interrupt handler corrupts the event count.
--

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

drivers/perf: riscv-iommu: add risc-v iommu pmu driver

Add a new driver to support the RISC-V IOMMU PMU. This is an auxiliary
device driver created by the parent RISC-V IOMMU driver.

Link: https://lore.kernel.org/linux-riscv/20260618143634.7f3dd6c5@pumpkin/

> diff --git a/drivers/perf/riscv_iommu_pmu.c b/drivers/perf/riscv_iommu_pmu.c
> new file mode 100644
> index 0000000000000..b4906c44d2a08
> --- /dev/null
> +++ b/drivers/perf/riscv_iommu_pmu.c
[ ... ]
> +static void riscv_iommu_pmu_set_period(struct perf_event *event)
> +{
> +	struct riscv_iommu_pmu *pmu = to_riscv_iommu_pmu(event->pmu);
> +	struct hw_perf_event *hwc = &event->hw;
> +	u64 counter_mask = hwc->idx ? pmu->event_cntr_mask : pmu->cycle_cntr_mask;
> +	u64 period;
> +
> +	/*
> +	 * Limit the maximum period to prevent the counter value
> +	 * from overtaking the one we are about to program.
> +	 * In effect we are reducing max_period to account for
> +	 * interrupt latency (and we are being very conservative).
> +	 */
> +	period = counter_mask >> 1;
> +	riscv_iommu_pmu_set_counter(pmu, hwc->idx, period);
> +	local64_set(&hwc->prev_count, period);

[Severity: Medium]
If riscv_iommu_pmu_stop()'s update executes concurrently with this, can it
compute a spurious massive delta?

This appears to be a vulnerability window for a race condition. If an overflow
interrupt occurs just before or during riscv_iommu_pmu_stop(), the IRQ handler
running on another CPU could execute this concurrently with the unprotected
riscv_iommu_pmu_update() in riscv_iommu_pmu_stop(). This would cause an ABA
data corruption on prev_count, breaking the counter statistics for userspace
applications.

> +}
[ ... ]
> +static void riscv_iommu_pmu_stop(struct perf_event *event, int flags)
> +{
> +	struct riscv_iommu_pmu *pmu = to_riscv_iommu_pmu(event->pmu);
> +	struct hw_perf_event *hwc = &event->hw;
> +	unsigned long irqflags;
> +	int idx = hwc->idx;
> +
> +	if (hwc->state & PERF_HES_STOPPED)
> +		return;
> +
> +	raw_spin_lock_irqsave(&pmu->lock, irqflags);
> +	riscv_iommu_pmu_disable_counter(pmu, idx);
> +	raw_spin_unlock_irqrestore(&pmu->lock, irqflags);
> +
> +	if ((flags & PERF_EF_UPDATE) && !(hwc->state & PERF_HES_UPTODATE))
> +		riscv_iommu_pmu_update(event);
> +
> +	hwc->state |= PERF_HES_STOPPED | PERF_HES_UPTODATE;

[Severity: Medium]
Could this state update cause a race condition with the PMU interrupt handler?

The hwc->state is updated outside the pmu->lock critical section in
riscv_iommu_pmu_stop(). This creates a vulnerability window where a concurrent
ISR running on another CPU still sees the event as active.

> +}
[ ... ]
> +static void riscv_iommu_pmu_read(struct perf_event *event)
> +{
> +	riscv_iommu_pmu_update(event);

[Severity: Medium]
Is it possible to expose the local64_cmpxchg() loop in riscv_iommu_pmu_update()
to a cross-CPU ABA race condition here?

Because riscv_iommu_pmu_read() does not hold pmu->lock, and because of
IRQCHIP_MOVE_DEFERRED, the PMU interrupt can fire on a different CPU than the
one handling the perf read.

If the IRQ handler processes an overflow concurrently, it resets hwc->prev_count
to the period. The concurrent cmpxchg in riscv_iommu_pmu_read() could see the
period as unchanged and succeed, double-counting the overflow. The subsequent
read would then see a massive spurious delta, resulting in incorrect statistics
for perf userspace tools.

> +}
[ ... ]
> +static irqreturn_t riscv_iommu_pmu_irq_handler(int irq, void *dev_id)
> +{
[ ... ]
> +			/*
> +			 * pmu->events[idx] only means the counter is allocated,
> +			 * not that it is counting. A counter which has not been
> +			 * started has no valid prev_count to compute a delta
> +			 * against, and one which has been stopped must not be
> +			 * reprogrammed here. The overflow bit still has to be
> +			 * cleared below in either case, including when the event
> +			 * was already removed by riscv_iommu_pmu_del(),
> +			 * otherwise the interrupt would stay pending forever.
> +			 */
> +			if (event && !(event->hw.state & PERF_HES_STOPPED)) {
> +				riscv_iommu_pmu_update(event);
> +				riscv_iommu_pmu_set_period(event);
> +			}

[Severity: Medium]
Since PERF_HES_STOPPED is set outside the lock in riscv_iommu_pmu_stop(),
can the ISR incorrectly enter this block and mutate the hardware counter?

The ISR relies on PERF_HES_STOPPED to avoid touching a counter being stopped.
Because the flag is set too late in riscv_iommu_pmu_stop(), it might enter
this block and mutate the hardware counter while it is actively being stopped.

> +
> +			riscv_iommu_pmu_clear_ovf(pmu, idx);
> +		}

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