Re: [PATCH 1/2] perf: Generic hotplug/cpumask for system PMUs

[sashiko-bot@kernel.org]

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 <robin.murphy@arm.com>

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.

> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index ba5bd6a78fe7b..505cbda2ac41f 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c

[ ... ]

> @@ -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);
>  
> +	/*
> +	 * 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.

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

[ ... ]

> @@ -15189,10 +15225,19 @@ static void perf_event_clear_cpumask(unsigned int cpu)
>  
>  	/* 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.

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

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

[ ... ]

> @@ -15246,7 +15292,7 @@ static void perf_event_setup_cpumask(unsigned int cpu)
>  		goto end;
>  	}
>  
> -	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.

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

[ ... ]

> @@ -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);
>  	}
> +
> +	/* 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.

>  end:
>  	cpumask_set_cpu(cpu, perf_online_mask);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1784653319.git.robin.murphy@arm.com?part=1