Re: [PATCH v3 4/9] perf/cxl: Split the MSI vector out of info->irq

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

Pre-existing issues:
- [High] The check `info->msi_vec <= 0` incorrectly rejects valid MSI vector index 0.
- [High] Incorrect registration order of the PMU and CPU hotplug instance leads to a Use-After-Free or NULL pointer dereference during hotplug events.
- [High] PMU hardware is not frozen during CPU hotplug migration, and the interrupt is shared, leading to concurrent SMP execution of `local64_t` operations without SMP safety.
- [High] The driver dynamically changes the CPU affinity of an interrupt requested with IRQF_SHARED, forcibly breaking affinity for other devices sharing the IRQ.
- [Medium] The PMU firmware's counter width is used without validation, which can cause undefined behavior via out-of-bounds bit shifts.
- [Medium] Silent truncation of user-provided PMU threshold values.
--

--- Patch [4]: [PATCH v3 4/9] perf/cxl: Split the MSI vector out of info->irq ---
commit ad9898396611ac9af6c4819fa48aba52e6dbeb08
Author: Dave Jiang <[email protected]>
Subject: perf/cxl: Split the MSI vector out of info->irq

The commit separates the MSI/MSI-X message number from the Linux virq by
introducing a new msi_vec field in struct cxl_pmu_info. This clarifies
the use of the field during probe before it gets overwritten with the virq.

> diff --git a/drivers/perf/cxl_pmu.c b/drivers/perf/cxl_pmu.c
> index 3511d049aea40..56e6ae7bebf7c 100644
> --- a/drivers/perf/cxl_pmu.c
> +++ b/drivers/perf/cxl_pmu.c

[ ... ]

> @@ -874,10 +875,10 @@ static int cxl_pmu_probe(struct device *dev)
>  		.capabilities = PERF_PMU_CAP_NO_EXCLUDE,
>  	};
>  
> -	if (info->irq <= 0)
> +	if (info->msi_vec <= 0)

[Severity: High]
This is a pre-existing issue, but does this incorrectly reject a valid MSI
vector index of 0?

The CXL PMU capability register provides a 0-based MSI vector number (0-15).
If the firmware specifies MSI message number 0, info->msi_vec becomes 0.
cxl_pmu_probe() then erroneously aborts with -EINVAL, causing valid
devices using MSI vector 0 to fail probing completely.

>  		return -EINVAL;
>  
> -	rc = pci_irq_vector(pdev, info->irq);
> +	rc = pci_irq_vector(pdev, info->msi_vec);
>  	if (rc < 0)
>  		return rc;
>  	irq = rc;

I also noticed a few other pre-existing issues in this file:

[Severity: High]
This isn't a bug introduced by this patch, but could the PMU registration
order cause a NULL pointer dereference or use-after-free during hotplug?

In cxl_pmu_probe(), cpuhp_state_add_instance() is called before
perf_pmu_register(). If a CPU hotplug event triggers immediately:

cxl_pmu_offline_cpu()
  perf_pmu_migrate_context()

Since perf_pmu_register() hasn't allocated pmu->pmu_cpu_context yet,
could this result in a NULL pointer dereference? 

Furthermore, upon module unload, devm executes teardown in reverse order.
perf_pmu_unregister() is called before removing the hotplug instance,
creating a window where a CPU offline event could trigger a use-after-free
on the unregistered PMU context.

[Severity: High]
This is a pre-existing issue, but is it safe to use local64_cmpxchg() when
the PMU interrupt is shared and not disabled during migration?

cxl_pmu_offline_cpu() migrates PMU events without disabling the PMU hardware.
Because the interrupt is shared (IRQF_SHARED), it can fire on any CPU at any
time due to other devices sharing the line:

cxl_pmu_irq()
  __cxl_pmu_read()
    local64_cmpxchg()

Concurrently, the new CPU can start or read events, also invoking
__cxl_pmu_read(). Since local64_t operations are only safe for single-CPU
access, could cross-CPU concurrent access corrupt the performance counters?

[Severity: Medium]
This isn't a bug introduced by this patch, but could an unvalidated counter
width from firmware cause undefined behavior?

In cxl_pmu_parse_caps(), info->counter_width is read directly from the hardware
capability register. If firmware reports a width of 0, __cxl_pmu_read()
computes GENMASK_ULL(0 - 1, 0).

Since 0 - 1 promotes to an int value of -1, passing it to GENMASK_ULL
evaluates to a negative bit shift (~0ULL >> 64), which is undefined behavior.

[Severity: High]
This is a pre-existing issue, but does this driver dynamically alter the
affinity of a shared interrupt?

cxl_pmu_probe() requests the PMU interrupt with the IRQF_SHARED flag.
However, during CPU hotplug events, cxl_pmu_online_cpu() and
cxl_pmu_offline_cpu() unconditionally call irq_set_affinity() to bind the
interrupt to a specific CPU.

Dynamically changing the hardware affinity of a shared interrupt can reroute
interrupts for all other unrelated devices sharing that IRQ line, violating
system isolation and potentially causing performance regressions or timeouts
for other drivers.

[Severity: Medium]
This isn't a bug introduced by this patch, but does the configuration
accessor truncate user-provided threshold values?

The format attribute threshold is exposed via sysfs as config1:0-15 (16 bits).
However, cxl_pmu_config1_get_threshold() uses a u8 return type:

static u8 cxl_pmu_config1_get_threshold(struct perf_event *event)
{
	return FIELD_GET(CXL_PMU_ATTR_CONFIG1_THRESHOLD_MSK, event->attr.config1);
}

This silently truncates any 16-bit user input to 8 bits, resulting in the PMU
being configured with an incorrect threshold without returning any error to
userspace.

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