Re: [PATCH v13 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-iommu: use-after-free on driver unbind due to devres LIFO order
- [Medium] perf/riscv-iommu: invalid write to read-only IPSR register
--

commit 2511b2dad68cf3d12e748952ca89116c8fc1ba49
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 driver introduces event
and cycle counters for the PMU, using a raw spinlock to serialize accesses.
It registers as an auxiliary device of the RISC-V IOMMU.

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 irqreturn_t riscv_iommu_pmu_irq_handler(int irq, void *dev_id)
> +{
> +	struct riscv_iommu_pmu *pmu = (struct riscv_iommu_pmu *)dev_id;

[ ... ]

> +	/* Clear performance monitoring interrupt pending bit */
> +	writel_relaxed(RISCV_IOMMU_IPSR_PMIP, pmu->reg + RISCV_IOMMU_REG_IPSR);

[Severity: Medium]
Will this write trigger a bus access fault on strict hardware implementations?

According to the RISC-V IOMMU specification, the IPSR (Interrupt Pending 
Status Register) is read-only and combinatorial. While some hardware might 
silently drop writes to read-only registers, writing to it here in the IRQ 
handler explicitly attempts to clear the performance-monitoring interrupt 
pending (pmip) bit. This could lead to a crash or unexpected behavior on 
strict hardware interconnects.

> +
> +	riscv_iommu_pmu_start_all(pmu, inhibit);
> +
> +	raw_spin_unlock_irqrestore(&pmu->lock, irqflags);
> +
> +	return IRQ_HANDLED;
> +}

[ ... ]

> +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 = devm_add_action_or_reset(&auxdev->dev,
> +				       riscv_iommu_pmu_remove_cpuhp_instance,
> +				       iommu_pmu);
> +	if (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);
> +		return ret;
> +	}
> +
> +	ret = devm_add_action_or_reset(&auxdev->dev,
> +				       riscv_iommu_pmu_do_unregister,
> +				       iommu_pmu);

[Severity: High]
Does this sequence create a use-after-free window during device unbind?

The devm cleanup actions operate in LIFO order. Because 
riscv_iommu_pmu_do_unregister() is added after 
riscv_iommu_pmu_remove_cpuhp_instance(), it will run first when the driver 
unbinds, unregistering the PMU context.

During this window, the CPU hotplug callback remains active. If a CPU offline 
event occurs before the hotplug instance is removed, the kernel will execute 
riscv_iommu_pmu_offline_cpu(), which dereferences the freed context:

perf_pmu_migrate_context(&iommu_pmu->pmu, cpu, target_cpu);

Could we reverse the registration order so the hotplug callback is safely 
removed before the PMU is unregistered?

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