Re: [PATCH v5 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 7 potential issue(s) to consider:
- [High] The auxiliary driver lacks a `.remove` callback, permanently leaking the registered PMU and CPU hotplug state, and leading to a use-after-free upon device removal.
- [High] The IRQ is requested on the parent IOMMU device instead of the auxiliary device, causing the IRQ lifecycle to outlive the `pmu` pointer passed as `dev_id`.
- [High] The custom hi-lo-hi 64-bit counter read logic fails to detect wraps if `lo_prev` was sampled at a small value.
- [High] The PMU driver requests its interrupt without `IRQF_SHARED`, which will fail and break the PMU on systems where the IOMMU shares a single interrupt vector.
- [Medium] The driver assigns the PMU's CPU and registers the CPU hotplug instance without holding `cpus_read_lock()`, which can permanently pin the PMU to an offline CPU.
- [Medium] Software group leaders incorrectly consume a hardware counter slot during event initialization.
- [Medium] Deleting a PMU event does not clear pending hardware overflows, leading to a teardown race that triggers a spurious `WARN_ON_ONCE`.
--

--- Patch [1]: [PATCH v5 1/2] drivers/perf: riscv-iommu: add risc-v iommu pmu driver ---
commit 76e5b588d50530adc2e8af05625ed2c20f89b431
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. It is
implemented as an auxiliary device driver created by the parent IOMMU
driver.

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..984ddb1c7f399
> --- /dev/null
> +++ b/drivers/perf/riscv_iommu_pmu.c
[ ... ]
> +static u64 riscv_iommu_pmu_get_counter(struct riscv_iommu_pmu *pmu, u32 idx)
> +{
> +	void __iomem *addr = pmu->reg + RISCV_IOMMU_REG_IOHPMCTR(idx);
> +	u64 value, counter_mask = idx ? pmu->event_cntr_mask : pmu->cycle_cntr_mask;
> +	u32 hi, lo;
> +
> +	hi = readl(addr + 4);
> +	lo = readl(addr);
> +
> +	if (lo <= pmu->lo_prev[idx] || hi != pmu->hi_prev[idx]) {

[Severity: High]
Can this miss a wrap if lo_prev was sampled at a small value?

If the counter wraps between reading the high and low halves, but the new
low value is still greater than a stale lo_prev, wouldn't the wrap check
evaluate to false and return a torn read?

> +		u32 hi_tmp = readl(addr + 4);
[ ... ]
> +static int riscv_iommu_pmu_event_init(struct perf_event *event)
> +{
[ ... ]
> +	if (!is_cycle_event(get_event(event)))
> +		if (++counters > total_event_counters)
> +			return -EINVAL;
> +
> +	if (!is_cycle_event(get_event(event->group_leader)))
> +		if (++counters > total_event_counters)
> +			return -EINVAL;

[Severity: Medium]
Does this incorrectly consume a hardware counter slot if the group leader
is a software event?

Should this check is_software_event() for the group leader before
incrementing the counter requirement?

> +
> +	for_each_sibling_event(sibling, event->group_leader) {
[ ... ]
> +static void riscv_iommu_pmu_del(struct perf_event *event, int flags)
> +{
> +	struct riscv_iommu_pmu *pmu = to_riscv_iommu_pmu(event->pmu);
> +	struct hw_perf_event *hwc = &event->hw;
> +	int idx = hwc->idx;
> +
> +	riscv_iommu_pmu_stop(event, PERF_EF_UPDATE);
> +	pmu->events[idx] = NULL;

[Severity: Medium]
Can this trigger the WARN_ON_ONCE() in the IRQ handler?

If an event overflows and is immediately deleted, the hardware overflow bit
is left set while the event pointer is cleared to NULL here. When the
pending interrupt is subsequently handled, wouldn't it detect the overflow,
find a NULL event, and trigger the WARN_ON_ONCE(!event)?

> +	clear_bit(idx, pmu->used_counters);
> +
> +	perf_event_update_userpage(event);
> +}
[ ... ]
> +static int riscv_iommu_pmu_request_irq(struct riscv_iommu_device *iommu,
> +				       struct riscv_iommu_pmu *pmu)
> +{
> +	return devm_request_irq(iommu->dev, pmu->irq, riscv_iommu_pmu_irq_handler,
> +				IRQF_NOBALANCING, dev_name(iommu->dev), pmu);

[Severity: High]
Is it safe to request the IRQ on the parent iommu->dev instead of the
auxiliary device?

If probe fails later, the auxiliary device memory for the pmu object will
be freed, but the IRQ handler would remain registered on the parent device,
potentially leading to a use-after-free on the next interrupt.

[Severity: High]
Will this fail to probe on systems that share a single interrupt vector?

The parent IOMMU driver likely requests the IRQ with IRQF_SHARED.
Shouldn't this request also include IRQF_SHARED to avoid a flag mismatch
rejection?

> +}
> +
> +static int riscv_iommu_pmu_probe(struct auxiliary_device *auxdev,
> +				 const struct auxiliary_device_id *id)
> +{
[ ... ]
> +	/*
> +	 * Bind all events to the same cpu context to avoid race enabling.
> +	 * Pick a local CPU from the numa node of this iommu for the locality.
> +	 */
> +	iommu_pmu->numa_node = dev_to_node(iommu_dev->dev);
> +	iommu_pmu->on_cpu = cpumask_local_spread(0, iommu_pmu->numa_node);
> +	iommu_pmu->irq = riscv_iommu_pmu_get_irq_num(iommu_dev);
> +	ret = irq_set_affinity(iommu_pmu->irq, cpumask_of(iommu_pmu->on_cpu));
> +	if (ret)
> +		dev_dbg(&auxdev->dev, "failed to set irq %u affinity to cpu %u: %d\n",
> +			iommu_pmu->irq, iommu_pmu->on_cpu, ret);
> +
> +	ret = riscv_iommu_pmu_request_irq(iommu_dev, iommu_pmu);
> +	if (ret) {
> +		dev_err(&auxdev->dev, "Failed to request irq %s: %d\n", name, ret);
> +		return ret;
> +	}
> +
> +	ret = cpuhp_state_add_instance_nocalls(cpuhp_state, &iommu_pmu->node);

[Severity: Medium]
Can the selected CPU go offline right before the hotplug instance is
registered?

Since cpus_read_lock() is not held around the CPU selection and
cpuhp_state_add_instance_nocalls() registration, the selected CPU could
potentially go offline in between. Wouldn't this permanently pin the PMU to
an offline CPU?

> +	if (ret) {
> +		dev_err(&auxdev->dev, "Failed to register hotplug %s: %d\n", name, ret);
> +		return ret;
> +	}
[ ... ]
> +static struct auxiliary_driver iommu_pmu_driver = {
> +	.driver = {
> +		.suppress_bind_attrs = true,
> +	},
> +	.probe		= riscv_iommu_pmu_probe,
> +	.id_table	= riscv_iommu_pmu_id_table,
> +};

[Severity: High]
Does this auxiliary driver need a .remove callback?

The probe function registers with the perf and cpuhp subsystems using
non-devm APIs. Without a remove function to unregister them, unbinding the
parent driver could leave active subsystem references to freed memory.

> +
> +static int __init riscv_iommu_pmu_init(void)

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