Re: [PATCH 5/7] perf/x86/intel/uncore: Introduce PMU flags and broken state

["Chen, Zide" <zide.chen@intel.com>]

On 5/13/2026 11:27 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
> - [Critical] Intentionally leaking `die_refcnt` on PCI box setup failure prevents PMU unregistration, leading to a Use-After-Free.
> - [High] Global `PMU_BROKEN` state disables new event creation and breaks CPU context migration for fully functional dies.
> - [Medium] Confused lifecycle assumptions lead to contradictory error handling and dead code between PCI and MSR/MMIO PMUs.
> - [Medium] Data race between plain store and atomic bitwise operation on `pmu->flags`.
> --
> 
> commit 553c891e098baf8dc3f2b474849cc4642dfe9be6
> Author: Zide Chen <zide.chen@intel.com>
> 
> perf/x86/intel/uncore: Introduce PMU flags and broken state
> 
> This commit replaces the 'registered' field in intel_uncore_pmu with a
> 'flags' field, introducing a PMU_BROKEN flag to track box setup failures.
> If a box fails to initialize, the PMU is marked broken, which rejects new
> event assignments and skips future box setup attempts.
> 
>> diff --git a/arch/x86/events/intel/uncore.c b/arch/x86/events/intel/uncore.c
>> index 617a68c1d76c3..4d31dcaf5405b 100644
>> --- a/arch/x86/events/intel/uncore.c
>> +++ b/arch/x86/events/intel/uncore.c
> 
> [ ... ]
> 
>> @@ -738,7 +738,7 @@ static int uncore_pmu_event_init(struct perf_event *event)
>>  
>>  	pmu = uncore_event_to_pmu(event);
>>  	/* no device found for this pmu */
>> -	if (!pmu->registered)
>> +	if (!uncore_pmu_available(pmu))
>>  		return -ENOENT;
> 
> Since PMU_BROKEN is a global flag across all dies and packages, if one die
> fails initialization, the entire PMU is marked broken. Does this cause
> uncore_pmu_event_init() to incorrectly reject new event assignments on fully
> functional packages?

This is by design. The PMU represents a single logical device spanning
all dies, so if any box fails initialization, the entire PMU is marked
broken to avoid partial and potentially inconsistent monitoring state.
Allowing some dies to remain active while others are broken could lead
to misleading perf data.

> 
> [ ... ]
> 
>> @@ -934,16 +934,16 @@ static int uncore_pmu_register(struct intel_uncore_pmu *pmu)
>>  
>>  	ret = perf_pmu_register(&pmu->pmu, pmu->name, -1);
>>  	if (!ret)
>> -		pmu->registered = true;
>> +		uncore_pmu_set_registered(pmu);
>>  	return ret;
>>  }
>>  
>>  static void uncore_pmu_unregister(struct intel_uncore_pmu *pmu)
>>  {
>> -	if (!pmu->registered)
>> +	if (!uncore_pmu_registered(pmu))
>>  		return;
>>  	perf_pmu_unregister(&pmu->pmu);
>> -	pmu->registered = false;
>> +	WRITE_ONCE(pmu->flags, 0);
> 
> Can this plain store data race with concurrent atomic bitwise operations?
> 
> If uncore_box_setup() executes on another CPU (e.g., probing a new failing
> die while removing a successful die), it uses the atomic read-modify-write
> instruction set_bit(PMU_BROKEN_BIT, &(pmu)->flags). Mixing plain stores and
> atomic bitwise operations on the same memory location could lead to
> clobbered updates.

Actually, I'm thinking the atomic operations are not needed at all. On
the MSR/MMIO side, CPU hotplug callbacks are serialized, so plain bit
operations are safe. For PCI PMUs, the PCI device is per-die, so there
is no way that box_setup() and pmu_unregister() will happen concurrently.

> [ ... ]
> 
>> @@ -1136,7 +1136,13 @@ static int uncore_box_setup(struct intel_uncore_pmu *pmu,
>>  
>>  	/* die_refcnt tracks online dies, not only functioning boxes. */
>>  	dies = atomic_inc_return(&pmu->die_refcnt);
>> -	uncore_box_init(box);
>> +
>> +	if (uncore_pmu_broken(pmu))
>> +		return -ENODEV;
>> +
>> +	ret = uncore_box_init(box);
>> +	if (ret)
>> +		goto err;
>>  
>>  	/* First active box registers the pmu. */
>>  	if (dies > 1)
>> @@ -1148,6 +1154,19 @@ static int uncore_box_setup(struct intel_uncore_pmu *pmu,
>>  
>>  	return 0;
>>  err:
>> +	/*
>> +	 * On failure on any box, mark the per-package PMU as broken regardless
>> +	 * of whether it was registered or not.
>> +	 *
>> +	 * Don't decrement die_refcnt to prevent any future CPU online
>> +	 * event or PCI probe, from retrying the failed PMU registration.
>> +	 *
>> +	 * Don't decrement cpu_refcnt to avoid other in-die CPUs from
>> +	 * trying to set up the PMU box again.
>> +	 *
>> +	 * Don't kfree box; MSR and MMIO boxes are freed at module exit only.
>> +	 */
> 
> Is there a risk of a use-after-free resulting from intentionally leaking
> die_refcnt here?
> 
> For PCI PMUs, the caller uncore_pci_pmu_register() executes kfree(box) on error
> and fails the PCI probe, meaning uncore_pci_remove() is never called to clean
> up this leaked reference.

You are right — in that case, uncore_pci_pmu_unregister() may not be
called if the box is NULL in uncore_pci_remove(). That said, die_refcnt
should be incremented only when pmu->boxes[die] is not NULL, for both
the PCI and MSR/MMIO cases.

In this patch, die_refcnt should be decremented if uncore_box_setup()
fails in the PCI case. There is also a pre-existing bug where die_refcnt
could be unexpectedly decremented via the uncore_bus_notify() path.


> If another successfully probed PCI box for this PMU is later removed,
> uncore_pci_pmu_unregister() decrements die_refcnt, but it will not reach 0 due
> to the earlier leak. Consequently, uncore_pmu_unregister(pmu) is skipped,
> leaving the PMU and its associated perf events active.
> 
> When uncore_pci_pmu_unregister() then frees the successful box, active
> perf_event instances could still hold event->pmu_private pointing to this freed
> box, potentially triggering a use-after-free on subsequent event reads or
> context switches.
> 
> Additionally, this comment claims MSR and MMIO boxes are freed at module exit
> only. Does this conflict with PCI PMUs which unconditionally execute kfree(box)
> on error, immediately destroying the box?

There is no conflicts here.
>> +	uncore_pmu_set_broken(pmu);
>>  	uncore_box_exit(box);
>>  	return ret;
>>  }
> 
> [ ... ]
> 
>> @@ -1483,7 +1502,8 @@ static void uncore_change_type_ctx(struct intel_uncore_type *type, int old_cpu,
>>  
>>  		if (old_cpu < 0) {
>>  			WARN_ON_ONCE(box->cpu != -1);
>> -			if (uncore_die_has_box(type, die, pmu->pmu_idx)) {
>> +			if (uncore_die_has_box(type, die, pmu->pmu_idx) &&
>> +			    !uncore_pmu_broken(pmu)) {
>>  				box->cpu = new_cpu;
>>  				cpumask_set_cpu(new_cpu, &pmu->cpu_mask);
>>  			}
>> @@ -1493,7 +1513,7 @@ static void uncore_change_type_ctx(struct intel_uncore_type *type, int old_cpu,
>>  		WARN_ON_ONCE(box->cpu != -1 && box->cpu != old_cpu);
>>  		box->cpu = -1;
>>  		cpumask_clear_cpu(old_cpu, &pmu->cpu_mask);
>> -		if (new_cpu < 0)
>> +		if (new_cpu < 0 || uncore_pmu_broken(pmu))
>>  			continue;
> 
> If uncore_pmu_broken(pmu) causes this to skip, does this strand active hardware
> events on offlined CPUs for fully functional dies?
> 
> The commit message states the goal is to avoid disrupting in-flight events on
> other boxes, but skipping uncore_pmu_cancel_hrtimer() and
> perf_pmu_migrate_context() here seems to break CPU context migration for those
> events.

You are right, checking the broken flag here could disrupt in-flight
events. Instead, checking box->cpu == -1 is appropriate to skip broken
boxes.

@@ -1522,9 +1522,15 @@ static void uncore_change_type_ctx(struct
intel_uncore_type *type, int old_cpu,
                }

                WARN_ON_ONCE(box->cpu != -1 && box->cpu != old_cpu);

+               if (box->cpu == -1)
+                       continue;
+
                box->cpu = -1;
                cpumask_clear_cpu(old_cpu, &pmu->cpu_mask);
-               if (new_cpu < 0 || uncore_pmu_broken(pmu))
+               if (new_cpu < 0)
                        continue;

>>  
>>  		if (!uncore_die_has_box(type, die, pmu->pmu_idx))
> 
> [ ... ]
> 
>> @@ -1573,7 +1593,7 @@ static int allocate_boxes(struct intel_uncore_type **types,
>>  		type = *types;
>>  		pmu = type->pmus;
>>  		for (i = 0; i < type->num_boxes; i++, pmu++) {
>> -			if (pmu->boxes[die])
>> +			if (pmu->boxes[die] || uncore_pmu_broken(pmu))
>>  				continue;
> 
> Is this check dead code? allocate_boxes() is exclusively used by MSR/MMIO
> PMUs, but those PMUs ignore hardware initialization failures and never call
> uncore_pmu_set_broken(pmu).

Not quite true. MSR/MMIO PMUs do not ignore hardware initialization
failures — they call uncore_pmu_set_broken() in uncore_box_setup() when
setup fails.