perf: Generic hotplug/cpumask for system PMUs

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [Critical] Taking a CPU offline triggers a NULL pointer dereference if a PMU defines init_cpu but not exit_cpu.
- [High] CPU online context migration passes the new CPU as both source and destination to perf_pmu_migrate_context, causing a self-deadlock and failing to migrate events.
- [High] When a PMU is unregistered, the perf core fails to invoke the pmu->exit_cpu() callback for currently online CPUs, leaking state.
- [High] Missing bounds check in perf_event_setup_cpumask leads to an out-of-bounds per-CPU pointer access during migration.
- [High] perf_pmu_register() invokes the PMU's init_cpu callback in atomic IPI context, contradicting its documented thread context.
- [Medium] Secondary CPU hotplug triggers a WARN_ON_ONCE because perf_event_setup_cpumask incorrectly iterates up to PERF_PMU_SCOPE_CPUMASK.
- [Medium] PMUs using PERF_PMU_SCOPE_CPUMASK with static masks but no hotplug callbacks will never have their context migrated on CPU offline.
- [Medium] If perf_pmu_register() fails during the IDR allocation step, it leaks the CPU-affine state initialized via pmu->init_cpu().
--

--- Patch [1]: [PATCH 1/2] perf: Generic hotplug/cpumask for system PMUs --- commit aa795c7bb07889a3ee4d3e9c388a299913298dba
Author: Robin Murphy <[email protected]>

perf: Generic hotplug/cpumask for system PMUs

This commit extends the generic uncore machinery to handle system PMUs by introducing a new PERF_PMU_SCOPE_CPUMASK scope. It allows the core code to retrieve a per-PMU cpumask dynamically and adds optional callbacks to manage CPU-affine state during hotplug operations.

Quoted 4 lines

kernel/events/core.c +0 -0

index ba5bd6a78fe7b..505cbda2ac41f 100644

[ ... ]

Quoted 4 lines

kernel/events/core.c +0 -0

@@ -12920,6 +12945,16 @@ int perf_pmu_register(struct pmu *_pmu, const char *name, int type)
  	INIT_LIST_HEAD(&pmu->events);
  	spin_lock_init(&pmu->events_lock);
 

kernel/events/core.c int perf_pmu_register(struct pmu *_pmu, const char *name, int type) · line 12957

Quoted 10 lines

kernel/events/core.c +10 -0

+	/*
+	 * Finally, if appropriate give the PMU a chance to initialise any
+	 * of its own CPU-affine state. Note that we're serialised against
+	 * perf_event_{init,exit}_cpu() themselves by virtue of pmus_lock.
+	 */
+	if (pmu->init_cpu) {
+		for_each_online_cpu(cpu)
+			smp_call_function_single(cpu, perf_scope_init_cpu, pmu, 1);
+	}
+

[Severity: High]
Does executing init_cpu via smp_call_function_single() conflict with the header documentation?

The documentation states init_cpu runs from CPUHP_AP_PERF_ONLINE context, which is preemptible.

However, smp_call_function_single() executes the
callback in atomic IPI context. If a driver sleeps here, will it cause a panic?

[Severity: Medium]
If idr_cmpxchg() fails shortly after this initialization loop, does this leak the newly allocated CPU-affine state?

There doesn't appear to be an error path to invoke exit_cpu on the online CPUs to unwind this initialization.

[Severity: High]
While not explicitly in this diff, since this function sets up CPU-affine state for a PMU during registration, is there a corresponding teardown step missing?

It appears perf_pmu_unregister() doesn't invoke exit_cpu across the online CPUs, which could leave this state dangling when a driver is unloaded.

Quoted 3 lines

kernel/events/core.c +0 -0

  	/*
  	 * Now that the PMU is complete, make it visible to perf_try_init_event().
  	 */

[ ... ]

Quoted 2 lines

kernel/events/core.c +0 -0

@@ -15189,10 +15225,19 @@ static void perf_event_clear_cpumask(unsigned int cpu)
 

kernel/events/core.c static void perf_event_clear_cpumask(unsigned int cpu) · line 15229

Quoted 7 lines

kernel/events/core.c +5 -0

  	/* migrate */
  	list_for_each_entry(pmu, &pmus, entry) {
+		bool cpumask_migrate = false;
+
+		if (pmu->init_cpu && (!pmu->cpumask || cpumask_test_cpu(cpu, pmu->cpumask)))
+			cpumask_migrate = pmu->exit_cpu(pmu, cpu);
+

[Severity: Critical]
Is there a risk of a NULL pointer dereference here?

This code checks if pmu->init_cpu is present, but then invokes pmu->exit_cpu. Because both callbacks are optional, a PMU driver might define init_cpu but leave exit_cpu as NULL.

kernel/events/core.c static void perf_event_clear_cpumask(unsigned int cpu) · line 15237

Quoted 8 lines

kernel/events/core.c +4 -0

  		if (pmu->scope == PERF_PMU_SCOPE_NONE ||
  		    WARN_ON_ONCE(pmu->scope >= PERF_PMU_MAX_SCOPE))
  			continue;
 
+		target[PERF_PMU_SCOPE_CPUMASK] = -1;
+		if (cpumask_migrate && !WARN_ON_ONCE(!pmu->cpumask))
+			target[PERF_PMU_SCOPE_CPUMASK] = cpumask_any_but(pmu->cpumask, cpu);
+

[Severity: Medium]
Does this strand PMU contexts for PMUs that use a static cpumask without providing hotplug callbacks?

Since cpumask_migrate defaults to false and is only set if a callback is invoked, target[PERF_PMU_SCOPE_CPUMASK] remains -1 and the migration seems to be bypassed, leaving the context orphaned on the offline CPU.

Quoted 4 lines

kernel/events/core.c +0 -0

  		if (target[pmu->scope] >= 0 && target[pmu->scope] < nr_cpu_ids)
  			perf_pmu_migrate_context(pmu, cpu, target[pmu->scope]);
  	}

[ ... ]

Quoted 4 lines

kernel/events/core.c +0 -0

@@ -15246,7 +15292,7 @@ static void perf_event_setup_cpumask(unsigned int cpu)
  		goto end;
  	}
 

kernel/events/core.c static void perf_event_setup_cpumask(unsigned int cpu) · line 15292

Quoted 2 lines

kernel/events/core.c +1 -1

-	for (scope = PERF_PMU_SCOPE_NONE + 1; scope < PERF_PMU_MAX_SCOPE; scope++) {
+	for (scope = PERF_PMU_SCOPE_NONE + 1; scope <= PERF_PMU_SCOPE_CPUMASK; scope++) {

[Severity: Medium]
Does iterating up to PERF_PMU_SCOPE_CPUMASK unconditionally trigger a warning during secondary CPU hotplug?

For this scope, perf_scope_cpumask() explicitly returns NULL, which would immediately trip the WARN_ON_ONCE(!pmu_cpumask || !cpumask) check later in this loop.

Quoted 3 lines

kernel/events/core.c +0 -0

  		const struct cpumask *cpumask = perf_scope_cpu_topology_cpumask(scope, cpu);
 
  		pmu_cpumask = perf_scope_cpumask(scope);

[ ... ]

Quoted 4 lines

kernel/events/core.c +0 -0

@@ -15258,6 +15304,16 @@ static void perf_event_setup_cpumask(unsigned int cpu)
  		    cpumask_any_and(pmu_cpumask, cpumask) >= nr_cpu_ids)
  			cpumask_set_cpu(cpu, pmu_cpumask);
  	}

kernel/events/core.c static void perf_event_setup_cpumask(unsigned int cpu) · line 15316

Quoted 10 lines

kernel/events/core.c +10 -0

+
+	/* Allow migrating if the new CPU is preferable (e.g. NUMA locality) */
+	list_for_each_entry(pmu, &pmus, entry) {
+		bool cpumask_migrate = false;
+
+		if (pmu->init_cpu && (!pmu->cpumask || !cpumask_test_cpu(cpu, pmu->cpumask)))
+			cpumask_migrate = pmu->init_cpu(pmu, cpu);
+		if (cpumask_migrate && !WARN_ON_ONCE(!pmu->cpumask))
+			perf_pmu_migrate_context(pmu, cpu, cpumask_any(pmu->cpumask));
+	}

[Severity: High]
Will this cause a self-deadlock in perf_pmu_migrate_context()?

The newly onlined CPU is passed as both the source CPU (cpu) and potentially the destination CPU (via cpumask_any). This would lock the same mutex twice in perf_pmu_migrate_context():

kernel/events/core.c:perf_pmu_migrate_context() {
     ...
     mutex_lock_double(&src_ctx->mutex, &dst_ctx->mutex);
     ...
}

Should this be migrating from the old CPU to the new CPU instead?

[Severity: High]
What happens if the PMU cpumask is empty here?

If init_cpu returns true but the PMU's mask is empty (for example, if it defaulted to cpu_none_mask), cpumask_any() could return a value greater than or equal to nr_cpu_ids. Passing this out-of-bounds value to perf_pmu_migrate_context() might result in corrupting memory when it accesses the per-CPU pointers.

Quoted 3 lines

kernel/events/core.c +0 -0

  end:
  	cpumask_set_cpu(cpu, perf_online_mask);
  }