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

[sashiko-bot@kernel.org]

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?

[ ... ]

> @@ -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.

[ ... ]

> @@ -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.

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?

> +	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.

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

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