Re: [PATCH v2 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] Modifying the affinity of a shared IRQ line during CPU hotplug violates local64_t locking rules.
- [High] Race condition during CPU hotplug migration violates local64_t locking rules.
--

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

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

The PMU driver pins its overflow interrupt to the managing CPU, but requests
it with IRQF_SHARED without IRQF_NOBALANCING. This allows irqbalance to
move the interrupt, leading to concurrent cross-CPU access of local64_t
event counts. This commit adds IRQF_NOBALANCING to ensure the CPU hotplug
pinning holds.

Link: https://sashiko.dev/#/patchset/[email protected]?part=1

> diff --git a/drivers/perf/cxl_pmu.c b/drivers/perf/cxl_pmu.c
> index d1e810601e363..8b89db8f4d689 100644
> --- a/drivers/perf/cxl_pmu.c
> +++ b/drivers/perf/cxl_pmu.c
> @@ -888,7 +888,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 isn't a bug introduced by this patch, but there appears to be a
pre-existing race condition in the CPU hotplug migration window that violates
local64_t locking rules.

In cxl_pmu_offline_cpu(), perf_pmu_migrate_context() moves the PMU events
to the target CPU and restarts them:

drivers/perf/cxl_pmu.c: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)));
	return 0;
}

Since perf_pmu_migrate_context() re-enables hardware interrupts, and
irq_set_affinity() is called after this migration, an interrupt firing
during this window will still be delivered to the old offlining CPU.

Concurrently, user-space reads of the migrated event will execute on the
new target CPU.

Could this lead to cross-CPU concurrent accesses to the local64_t event
counts, corrupting the data?

[Severity: High]
This is a pre-existing issue, but modifying the affinity of a shared IRQ
line during CPU hotplug appears to also violate local64_t locking rules.

Looking at the same cxl_pmu_offline_cpu() snippet above, the driver
registers its interrupt with IRQF_SHARED. Because affinity is a per-IRQ
property, calling irq_set_affinity() moves the interrupt for all PMU
instances sharing this IRQ line.

If another PMU instance is still managed by the old CPU (e.g. its offline
callback hasn't run yet), its interrupts will now fire on the new target CPU
while user-space reads concurrently execute on the old CPU.

Is there a risk that this cross-CPU access corrupts the local64_t counts?

>  	if (rc)
>  		return rc;

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