Re: [PATCH v14 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 3 potential issue(s) to consider:
- [High] perf: riscv-iommu: use-after-free on CPU hotplug due to missing cleanup in probe error path
- [High] perf: riscv-iommu: NULL pointer dereference in CPU offline race before PMU registration
- [Medium] perf: riscv-iommu: stale read on cross-CPU lock contention in riscv_iommu_pmu_read()
--

commit ade01fdbbb7c20954560c04dafd143c2c60bee2e
Author: Zong Li <zong.li@sifive.com>
Subject: drivers/perf: riscv-iommu: add risc-v iommu pmu driver

This commit adds a new driver to support the RISC-V IOMMU PMU, which is an
auxiliary device driver created by the parent RISC-V IOMMU driver. It includes
performance monitoring counters for specific device IDs/process IDs, hardware
detection of counters, and handles CPU hotplug operations.

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..56f25f725f1c4
> --- /dev/null
> +++ b/drivers/perf/riscv_iommu_pmu.c
> @@ -0,0 +1,1012 @@
[ ... ]
> +static void riscv_iommu_pmu_read(struct perf_event *event)
> +{
> +	struct riscv_iommu_pmu *pmu = to_riscv_iommu_pmu(event->pmu);
> +	unsigned long irqflags;
> +
> +	/*
> +	 * Use trylock, not lock: an NMI (e.g. a BPF program calling
> +	 * bpf_perf_event_read()) can land on this same CPU while it already
> +	 * holds pmu->lock in the irq handler, so blocking here could
> +	 * self-deadlock. Skipping the update on contention is safe -- the
> +	 * next successful read() or IRQ overflow will catch up.
> +	 */
> +	if (!raw_spin_trylock_irqsave(&pmu->lock, irqflags))
> +		return;

[Severity: Medium]
Will this trylock cause stale reads on cross-CPU lock contention?

If riscv_iommu_pmu_read() is called from a different CPU while the IRQ handler 
(running on the CPU to which the PMU is bound) holds the global pmu->lock, the 
trylock will fail. The function will then immediately return without calling 
riscv_iommu_pmu_update(), causing it to silently provide an un-updated, stale 
event->count to the caller.

Could we remove the lock here, since riscv_iommu_pmu_update() is inherently 
lockless and safely resolves concurrency using local64_cmpxchg() along with a 
retry loop?

> +	riscv_iommu_pmu_update(event);
> +	raw_spin_unlock_irqrestore(&pmu->lock, irqflags);
> +}
[ ... ]
> +static int riscv_iommu_pmu_offline_cpu(unsigned int cpu, struct hlist_node *node)
> +{
> +	struct riscv_iommu_pmu *iommu_pmu;
> +	unsigned int target_cpu;
[ ... ]
> +	target_cpu = riscv_iommu_pmu_bind_cpu(iommu_pmu, cpu);
> +	if (WARN_ON_ONCE(target_cpu >= nr_cpu_ids)) {
> +		WRITE_ONCE(iommu_pmu->on_cpu, -1);
> +	} else {
> +		WRITE_ONCE(iommu_pmu->on_cpu, target_cpu);
> +		/*
> +		 * perf_pmu_migrate_context() runs ->del() on cpu and ->add()
> +		 * on target_cpu with a synchronize_rcu() gap in between.
> +		 * riscv_iommu_pmu_irq_handler() can run concurrently with
> +		 * either step, on whichever CPU the interrupt physically
> +		 * lands on - pmu->lock serialises it against them instead of
> +		 * racing.
> +		 */
> +		perf_pmu_migrate_context(&iommu_pmu->pmu, cpu, target_cpu);

[Severity: High]
Can this migration call trigger a NULL pointer dereference?

In riscv_iommu_pmu_probe(), cpuhp_state_add_instance() is called before 
perf_pmu_register(). If a CPU goes offline in the narrow window between these 
two calls, this migration function will dereference pmu->cpu_pmu_context before 
it has been initialized, causing an immediate kernel oops.

> +	}
> +
> +	return 0;
> +}
[ ... ]
> +static int riscv_iommu_pmu_probe(struct auxiliary_device *auxdev,
> +				 const struct auxiliary_device_id *id)
> +{
[ ... ]
> +	ret = cpuhp_state_add_instance(cpuhp_state, &iommu_pmu->node);
> +	if (ret) {
> +		dev_err(&auxdev->dev, "Failed to register hotplug %s: %d\n", name, ret);
> +		return ret;
> +	}
> +
> +	ret = perf_pmu_register(&iommu_pmu->pmu, name, -1);
> +	if (ret) {
> +		dev_err(&auxdev->dev, "Failed to register %s: %d\n", name, ret);
> +		cpuhp_state_remove_instance_nocalls(cpuhp_state, &iommu_pmu->node);
> +		return ret;
> +	}

[Severity: High]
Is it possible to reorder cpuhp_state_add_instance() after 
perf_pmu_register() to close the race window mentioned above where hotplug 
events operate on an uninitialized PMU structure?

> +
> +	ret = devm_add_action_or_reset(&auxdev->dev,
> +				       riscv_iommu_pmu_do_unregister,
> +				       iommu_pmu);
> +	if (ret)
> +		return ret;

[Severity: High]
Does this error path leak the hotplug node?

If devm_add_action_or_reset() fails here (e.g. due to devres memory allocation 
failure), it executes riscv_iommu_pmu_do_unregister() and immediately returns. 
This skips the registration of the riscv_iommu_pmu_remove_cpuhp_instance 
cleanup action. The driver core unwinds the device and frees the iommu_pmu 
struct, but leaves a dangling iommu_pmu->node pointer in the global cpuhp_state 
list, leading to a use-after-free panic during the next CPU hotplug event.

> +
> +	/*
> +	 * Registered after do_unregister so it runs first (LIFO) on unbind:
> +	 * the cpuhp instance must be gone before perf_pmu_unregister() runs.
> +	 */
> +	ret = devm_add_action_or_reset(&auxdev->dev,
> +				       riscv_iommu_pmu_remove_cpuhp_instance,
> +				       iommu_pmu);
> +	if (ret)
> +		return ret;

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