Re: [PATCH v12 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 1 potential issue(s) to consider:
- [High] perf: data corruption due to missing lock in pmu->read()
--

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

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

This commit adds a new driver to support the RISC-V IOMMU PMU, an auxiliary
device driver created by the parent RISC-V IOMMU driver. It implements
hardware counter detection and separates the cycle counter from event
counters. Raw spin locks are used to avoid CPU hotplug races and
IRQCHIP_MOVE_DEFERRED issues.

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..ac4d225cca85f
> --- /dev/null
> +++ b/drivers/perf/riscv_iommu_pmu.c

[ ... ]

> +static void riscv_iommu_pmu_update(struct perf_event *event)
> +{
> +	struct hw_perf_event *hwc = &event->hw;
> +	struct riscv_iommu_pmu *pmu = to_riscv_iommu_pmu(event->pmu);
> +	u64 delta, prev, now;
> +	u32 idx = hwc->idx;
> +	u64 counter_mask = idx ? pmu->event_cntr_mask : pmu->cycle_cntr_mask;
> +
> +	/*
> +	 * riscv_iommu_pmu_set_period() resets the hardware counter and
> +	 * prev_count as two separate writes.
> +	 * Pairing a "prev" read from before that reset with a "now" read from
> +	 * after it would produce a nonsensical, possibly huge, delta.
> +	 * Re-checking prev_count after the hardware read detects that torn
> +	 * pairing and retries.
> +	 */
> +	do {
> +		prev = local64_read(&hwc->prev_count);
> +		now = riscv_iommu_pmu_get_counter(pmu, idx);
> +	} while (prev != local64_read(&hwc->prev_count) ||
> +		 local64_cmpxchg(&hwc->prev_count, prev, now) != prev);
> +
> +	delta = (now - prev) & counter_mask;
> +	local64_add(delta, &event->count);
> +}

[ ... ]

> +static void riscv_iommu_pmu_read(struct perf_event *event)
> +{
> +	riscv_iommu_pmu_update(event);

[Severity: High]
Does this missing pmu->lock expose the lockless update loop to a race with
the IRQ handler?

The commit message explicitly notes that due to PCI MSI/MSI-X on IMSIC
setting IRQCHIP_MOVE_DEFERRED, the IRQ handler can run on a different CPU
than the CPU the events are bound to.

If the IRQ handler on CPU A calls riscv_iommu_pmu_set_period() concurrently,
it writes the new hardware counter value and then updates prev_count. If an
unprivileged user reads the perf event fd on CPU B, triggering this
riscv_iommu_pmu_read() call without holding pmu->lock, the lockless loop in
riscv_iommu_pmu_update() can read the old prev_count and the newly updated
hardware counter.

This would allow the cmpxchg to succeed before the IRQ handler updates
prev_count, resulting in a massive bogus delta being calculated and added
to the performance event count.

> +}

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