Re: [PATCH v16 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/riscv-iommu: massive bogus counter delta due to lockless read race
--

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

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

This commit adds a new auxiliary device driver to support the RISC-V IOMMU
Performance Monitor. It introduces hardware counter management, event
configuration, and handles counter overflow interrupts.

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
> --- /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 = pmu->cntr_mask[idx];
> +
> +	/*
> +	 * riscv_iommu_pmu_set_period() resets the hardware counter and
> +	 * prev_count as two separate writes, and deliberately updates
> +	 * prev_count first.
[ ... ]
> +	 * so 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);
> +}

[Severity: High]
Does this lockless loop fully protect against a torn update from
riscv_iommu_pmu_set_period?

The commit notes that the IRQ handler can run on a different CPU than the
event's bound CPU due to IRQCHIP_MOVE_DEFERRED.

If riscv_iommu_pmu_set_period executes concurrently, it first updates
prev_count to the new period, and then resets the hardware counter.

If this lockless loop executes exactly between those two writes, it will
read prev as the newly updated period, and now as the old hardware counter
that has not yet been reset.

Since prev_count is not modified during the loop itself, the check for
prev != local64_read(&hwc->prev_count) will evaluate to false. The
cmpxchg will succeed, corrupting prev_count with the old hardware counter
value, and calculating a very large bogus delta:

delta = (old_hw_counter - period) & counter_mask;

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

[Severity: High]
Since riscv_iommu_pmu_update is exposed to the race described above, should
riscv_iommu_pmu_read acquire pmu->lock to serialize against the IRQ handler?

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