Re: [PATCH V3 2/8] perf/x86/intel/uncore: Fix refcnt and other cleanups

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Use-After-Free of uncore_box via active perf events and hrtimers after PCI PMU unregistration
- [High] Lockless race condition on `activeboxes` leading to concurrent perf PMU register/unregister
- [High] Missing memory barrier when publishing `box` allowing readers to observe partially initialized memory
- [High] Use-After-Free race between CPU hotplug context migration and PCI PMU unregistration
--

commit 372caa30bbc8374ec4e96cf8e02d24ffa69fe538
Author: Zide Chen <zide.chen@intel.com>
Subject: perf/x86/intel/uncore: Fix refcnt and other cleanups

This commit fixes a typo for the initialized flag and renames the 'id'
parameter to 'die' in uncore_box_{ref,unref}. It also adds reference
counting decrement logic in the PCI PMU unregister path to properly
clean up the uncore box.

> diff --git a/arch/x86/events/intel/uncore.c b/arch/x86/events/intel/uncore.c
> index b69b6a21d46bf..21c8ed1628cb2 100644
> --- a/arch/x86/events/intel/uncore.c
> +++ b/arch/x86/events/intel/uncore.c
> @@ -1255,8 +1255,10 @@ static void uncore_pci_pmu_unregister(struct intel_uncore_pmu *pmu, int die)
>  	pmu->boxes[die] = NULL;
>  	if (atomic_dec_return(&pmu->activeboxes) == 0)
>  		uncore_pmu_unregister(pmu);

[Severity: High]
This is a pre-existing issue, but does this lockless decrement of activeboxes
create a race condition with concurrent PMU registrations?

If Thread A (unregistering die 0) and Thread B (registering die 1) run
concurrently, Thread A might decrement activeboxes from 1 to 0 here, while
Thread B increments it from 0 to 1 in uncore_pci_pmu_register().

Both threads could then concurrently invoke uncore_pmu_unregister(pmu) and
perf_pmu_register(&pmu->pmu) on the exact same struct pmu object, potentially
corrupting the internal perf core PMU lists.

> -	uncore_box_exit(box);
> -	kfree(box);
> +	if (atomic_dec_return(&box->refcnt) == 0) {
> +		uncore_box_exit(box);
> +		kfree(box);
> +	}

[Severity: High]
This isn't a bug introduced by this patch, but does freeing the box here
leave dangling pointers in active perf events or hrtimers?

The perf core doesn't actively detach or destroy open events when the
underlying device is removed. If other dies still have this PMU active,
uncore_pmu_unregister(pmu) is bypassed.

Any armed hrtimer firing asynchronously in uncore_pmu_hrtimer() or userspace
reads invoking uncore_pmu_event_read() would then dereference the freed box
memory, leading to a use-after-free.

[Severity: High]
This is a pre-existing issue, but could this kfree(box) also race with CPU
hotplug context migration?

During CPU hotplug, uncore_change_type_ctx() iterates over pmu->boxes[die]
to migrate perf contexts. It fetches the box pointer and subsequently
accesses it (e.g., reading box->cpu or calling
uncore_pmu_cancel_hrtimer(box)).

If a PCI device removal executes uncore_pci_pmu_unregister() concurrently,
it nullifies pmu->boxes[die] and frees the box here.

If the hotplug thread reads the pointer before it is set to NULL but accesses
it after the memory is freed, it could result in a use-after-free.

[Severity: High]
This isn't a bug introduced by this patch, but is there a missing memory
barrier when publishing the box pointer during allocation?

In uncore_pci_pmu_register(), a newly allocated box is published to
pmu->boxes[die] using a plain store, lacking an smp_store_release().

Concurrently, a user opening a perf event triggers uncore_pmu_event_init(),
which looks up the box via uncore_pmu_to_box() using a plain load. Due to
CPU reordering, the reader may observe the non-NULL box pointer but read a
stale, uninitialized memory state (such as box->cpu == 0 instead of -1).

This would bypass the if (!box || box->cpu < 0) safety check and allow
the perf core to operate on partially initialized memory.

>  }

[ ... ]

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