Re: [PATCH v3 6/9] perf/cxl: Don't share the overflow interrupt, and keep it pinned

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

Pre-existing issues:
- [High] CPU hotplug migration race causes cross-CPU data corruption on in-flight interrupts.
--

--- Patch [6]: [PATCH v3 6/9] perf/cxl: Don't share the overflow interrupt, and keep it pinned ---
commit 0c52a628eb5f3fcd46d4c3f0c11a1448687ffd14
Author: Dave Jiang <[email protected]>

perf/cxl: Don't share the overflow interrupt, and keep it pinned

The PMU pins its overflow interrupt to info->on_cpu in the hotplug
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. cxl_pmu_irq() then runs local64_cmpxchg() and
local64_add() on hwc->prev_count and event->count there, at the same time
as the managing CPU. local64_t is only atomic against same-CPU access, so
the counts get corrupted.

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

> diff --git a/drivers/perf/cxl_pmu.c b/drivers/perf/cxl_pmu.c
> index c9e30cb149dfa..6fdc66a01fb67 100644
> --- a/drivers/perf/cxl_pmu.c
> +++ b/drivers/perf/cxl_pmu.c

[ ... ]

> @@ -784,7 +784,7 @@ static irqreturn_t cxl_pmu_irq(int irq, void *data)
>  
>  	overflowed = readq(base + CXL_PMU_OVERFLOW_REG);
>  
> -	/* Interrupt may be shared, so maybe it isn't ours */
> +	/* Nothing overflowed, so the device did not raise this */
>  	if (!overflowed)
>  		return IRQ_NONE;

[Severity: High]
This is a pre-existing issue, but can this IRQ handler race with CPU hotplug
migration and cause cross-CPU data corruption?

Looking at cxl_pmu_offline_cpu(), the context is migrated to a new CPU before
the IRQ affinity is updated:

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)));
    ...
}

During installation on the new CPU, the hardware counters and interrupts are
re-enabled. If the hardware generates a PMU overflow interrupt in this
window before irq_set_affinity() completes, the interrupt is delivered to
the old CPU.

The old CPU's IRQ handler (cxl_pmu_irq()) will then see the newly installed
event in the globally shared info->hw_events[i] array:

cxl_pmu_irq() {
    ...
	for_each_set_bit(i, overflowedbm, info->num_counters) {
		struct perf_event *event = info->hw_events[i];
    ...
		__cxl_pmu_read(event, true);
	}
    ...
}

This causes the old CPU to execute non-atomic local64_cmpxchg() and
local64_add() operations concurrently with the new managing CPU. Since
local64_t operations are only safe for same-CPU access, won't this corrupt
the hardware event counts?

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