Re: [PATCH V3 1/8] perf/x86/intel/uncore: Fix PCI PMU cleanup on setup failure

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Unjustified removal of `WARN_ON_ONCE` safety check based on a misunderstanding of the Linux device model.

Pre-existing issues:
- [High] Active perf events will dereference a freed `intel_uncore_box` when a PCI device is forcefully unbound.
- [High] Lockless update of `pmu->boxes[die]` in PCI probe/remove paths races with CPU hotplug callbacks, leading to Use-After-Free.
--

commit 1ebe3c291f253a0ba73f6277fd61a8e1138e1725
Author: Zide Chen <zide.chen@intel.com>
perf/x86/intel/uncore: Fix PCI PMU cleanup on setup failure

This commit attempts to fix a cleanup issue when uncore_pci_pmu_register()
fails by explicitly managing pmu->boxes[die] and activeboxes. It also removes
a WARN_ON_ONCE() under the assumption that a NULL driver data pointer could
result from a registration failure during probe.

Link: https://sashiko.dev/#/patchset/20260512233048.9577-1-zide.chen@intel.com?part=1

> diff --git a/arch/x86/events/intel/uncore.c b/arch/x86/events/intel/uncore.c
> index 7857959c6e823..b69b6a21d46bf 100644
> --- a/arch/x86/events/intel/uncore.c
> +++ b/arch/x86/events/intel/uncore.c

[ ... ]

> @@ -1248,6 +1249,9 @@ static void uncore_pci_pmu_unregister(struct intel_uncore_pmu *pmu, int die)
>  {
>  	struct intel_uncore_box *box = pmu->boxes[die];
>  
> +	if (!box)
> +		return;
> +
>  	pmu->boxes[die] = NULL;
>  	if (atomic_dec_return(&pmu->activeboxes) == 0)
>  		uncore_pmu_unregister(pmu);

[Severity: High]
This is a pre-existing issue, but when a PCI device is forcefully unbound,
does unconditionally freeing the box memory here lead to a use-after-free if
there are active perf events?

If active perf events are currently open for this PMU, the perf core retains
them. Subsequent perf operations will dereference the freed memory:

uncore_pmu_event_read() {
    struct intel_uncore_box *box = uncore_event_to_box(event);
    uncore_perf_event_update(box, event);
    ...
}

[Severity: High]
This is also a pre-existing issue, but is there a race condition between
setting pmu->boxes[die] to NULL here and concurrent CPU hotplug callbacks?

CPU hotplug callbacks run under cpus_read_lock(), but PCI bind/unbind
operations can execute concurrently without it. If a CPU hotplug event reads
the box right before it is cleared:

CPU1
uncore_change_type_ctx()
    box = pmu->boxes[die];

CPU2 would then execute uncore_pci_pmu_unregister():
    pmu->boxes[die] = NULL;
    ...
    kfree(box);

Would CPU1 then dereference the freed box memory when it subsequently accesses
box->cpu?

[ ... ]

> @@ -1272,7 +1276,6 @@ static void uncore_pci_remove(struct pci_dev *pdev)
>  				break;
>  			}
>  		}
> -		WARN_ON_ONCE(i >= UNCORE_EXTRA_PCI_DEV_MAX);
>  		return;
>  	}

[Severity: Medium]
The commit message states that since pci_get_drvdata() returns NULL on
registration failure, uncore_pci_remove() can no longer treat NULL drvdata as
an indicator of an auxiliary PCI device. Is this assumption accurate?

If PMU registration fails during uncore_pci_probe(), the probe function
returns an error code. The PCI driver core responds by aborting the bind
process and will never invoke the driver's remove callback for that device.

Because uncore_pci_remove() is never called as a result of a registration
failure, does removing this check silently mask bugs where a successfully
bound primary device unexpectedly loses its driver data?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260611160033.66760-1-zide.chen@intel.com?part=1