Re: [PATCH v15 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:
- [High] perf/riscv: Torn read race between PMU read and IRQ handler corrupts event count
- [Medium] perf/riscv: UAF in CPU hotplug callback during PMU probe error path
--

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

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

This commit introduces a new driver for the RISC-V IOMMU PMU, created as an
auxiliary device by the parent RISC-V IOMMU driver. The PMU provides event
and cycle counters with filtering support to collect performance metrics.

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
> index 0000000000000..25f217ac97e7f
> --- /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 = pmu->cntr_mask[hwc->idx];
> +	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);
> +}

[ ... ]

> +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.
> +	 * 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 code allow a lockless read to calculate a torn update delta?

The commit message notes that IRQCHIP_MOVE_DEFERRED can cause the PMU interrupt
to be routed to a different CPU than the event is bound to.

If riscv_iommu_pmu_read() is called locklessly on CPU B, it can race with
riscv_iommu_pmu_irq_handler() running on CPU A. In riscv_iommu_pmu_set_period(),
the hardware counter is written before the shadow copy prev_count.

If CPU B reads the old near-overflow prev_count, and then CPU A resets the
hardware counter, CPU B could read the new reset hardware counter. Because
CPU A hasn't reset the software prev_count yet, CPU B's loop validation
(prev == local64_read()) would succeed, producing a massive wrap-around spike
in the event count.

[ ... ]

> +static int riscv_iommu_pmu_probe(struct auxiliary_device *auxdev,
> +				 const struct auxiliary_device_id *id)
> +{

[ ... ]

> +	ret = devm_add_action_or_reset(&auxdev->dev,
> +				       riscv_iommu_pmu_do_unregister,
> +				       iommu_pmu);
> +	if (ret) {
> +		/*
> +		 * The action above has already run, so the pmu is unregistered
> +		 * and pmu->cpu_pmu_context is freed. Drop the cpuhp instance
> +		 * with _nocalls() so the teardown callback cannot reach it, and
> +		 * so it does not outlive iommu_pmu on the global cpuhp list.
> +		 */
> +		cpuhp_state_remove_instance_nocalls(cpuhp_state, &iommu_pmu->node);
> +		return ret;
> +	}

[Severity: Medium]
Can this sequence result in a use-after-free during a concurrent CPU offline
event?

If devm_add_action_or_reset() fails to allocate a devres node, it will
immediately execute riscv_iommu_pmu_do_unregister(), which calls
perf_pmu_unregister() and frees the PMU per-CPU contexts.

Because the CPU hotplug instance is not removed before the teardown action
executes, a concurrent CPU hotplug callback (riscv_iommu_pmu_offline_cpu)
might be invoked in this window. Could this cause perf_pmu_migrate_context()
to be called on the already-freed contexts?

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