Re: [PATCH 5/9] perf/cxl: Keep the overflow interrupt pinned to the managed CPU

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Race window during CPU offline allows interrupts to fire on the old CPU after events have migrated, causing cross-CPU concurrency on local64_t.
- [High] Using IRQF_SHARED with independent per-device CPU hotplug states allows PMU instances sharing an IRQ to drift onto different CPUs, breaking the local64_t same-CPU guarantee.
--

--- Patch [5]: [PATCH 5/9] perf/cxl: Keep the overflow interrupt pinned to the managed CPU ---
commit e894c4110202a2f84025491b6b4843a427420643
Author: Dave Jiang <[email protected]>

perf/cxl: Keep the overflow interrupt pinned to the managed CPU

The PMU pins its overflow interrupt to info->on_cpu in the hotplug
online/offline callbacks, but requests it with only IRQF_SHARED |
IRQF_NO_THREAD. Without IRQF_NOBALANCING, irqbalance or a userspace
smp_affinity write can move the interrupt to another CPU.

Fixes: 5d7107c72796 ("perf: CXL Performance Monitoring Unit driver")
Reported-by: [email protected]
Closes: https://sashiko.dev/#/patchset/[email protected]?part=1
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Dave Jiang <[email protected]>

> diff --git a/drivers/perf/cxl_pmu.c b/drivers/perf/cxl_pmu.c
> index 7db858727ac61..0c53f9e5ccb1d 100644
> --- a/drivers/perf/cxl_pmu.c
> +++ b/drivers/perf/cxl_pmu.c
> @@ -876,7 +876,8 @@ static int cxl_pmu_probe(struct device *dev)
>  	if (!irq_name)
>  		return -ENOMEM;
>  
> -	rc = devm_request_irq(dev, irq, cxl_pmu_irq, IRQF_SHARED | IRQF_NO_THREAD,
> +	rc = devm_request_irq(dev, irq, cxl_pmu_irq,
> +			      IRQF_SHARED | IRQF_NO_THREAD | IRQF_NOBALANCING,
>  			      irq_name, info);

[Severity: High]
This is a pre-existing issue, but does using IRQF_SHARED with independent
per-device CPU hotplug states allow PMU instances sharing an IRQ to drift onto
different CPUs?

If multiple PMU instances on a CXL device share the same MSI vector and request
it with IRQF_SHARED, each PMU registers its own independent CPU hotplug
instance.

If an administrator unbinds and rebinds one PMU instance, its
cxl_pmu_online_cpu() callback executes and sets the shared IRQ's affinity to
its newly chosen CPU.

The other PMU instances sharing the IRQ remain affine to their existing on_cpu,
but will now receive interrupts on the new CPU. When the IRQ fires, the kernel
executes cxl_pmu_irq() for all sharing PMUs on the new CPU. This causes
cxl_pmu_read() to execute on the wrong CPU concurrently with userspace reads on
the correct CPU, corrupting the local64_t counters.

While this patch adds IRQF_NOBALANCING to stop external irqbalance, does it
leave the driver vulnerable to its own internal hotplug divergence?

[Severity: High]
This isn't a bug introduced by this patch, but is there a race window during
CPU offline that allows interrupts to fire on the old CPU after events have
migrated?

Looking at cxl_pmu_offline_cpu():

	perf_pmu_migrate_context(&info->pmu, cpu, target);
	info->on_cpu = target;
	/*
	 * CPU HP lock is held so we should be guaranteed that this CPU hasn't yet
	 * gone away.
	 */
	WARN_ON(irq_set_affinity(info->irq, cpumask_of(target)));

Does perf_pmu_migrate_context() schedule the events out from the old CPU and
into the new CPU before the driver updates the IRQ affinity?

If an interrupt fires during this window, it would be routed to the old CPU.
cxl_pmu_irq() would execute on the old CPU and call cxl_pmu_read().
Concurrently, an unprivileged user reading the counter forces cxl_pmu_read() to
execute on the new CPU via IPI, corrupting the local64_t counters.

>  	if (rc)
>  		return rc;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5