Re: [PATCH v4] s390/cpum_cf: Handle CPU hotplug add and delete

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Premature freeing of per-CPU PMU data leading to NULL pointer dereference and kernel panic.
- [High] Per-CPU perf events are permanently broken after their target CPU undergoes an offline/online cycle.
- [Medium] Memory leak of `cpuhw` structures due to double-allocation race between event initialization and CPU hotplug.
--

--- Patch [1]: [PATCH v4] s390/cpum_cf: Handle CPU hotplug add and delete ---
commit fb5dfe8e1024f1616e4e0be3383c2356fabd9b3e
Author: Thomas Richter <[email protected]>

s390/cpum_cf: Handle CPU hotplug add and delete

This patch attempts to resolve a kernel panic occurring when CPUs are
hotplugged during a perf stat run. It does so by counting active
per-task processes and allocating the CPUMF per-CPU infrastructure on
newly online CPUs if there are active per-task contexts.

> diff --git a/arch/s390/kernel/perf_cpum_cf.c b/arch/s390/kernel/perf_cpum_cf.c
> index 2076ac22e2c49..f24b2145a0dfd 100644
> --- a/arch/s390/kernel/perf_cpum_cf.c
> +++ b/arch/s390/kernel/perf_cpum_cf.c
[ ... ]
> @@ -290,9 +286,12 @@ static int cpum_cf_alloc(int cpu)
>  	cpumask_var_t mask;
>  	int rc;
>  
> +	mutex_lock(&pmc_reserve_mutex);
>  	if (cpu == -1) {
> -		if (!zalloc_cpumask_var(&mask, GFP_KERNEL))
> -			return -ENOMEM;
> +		if (!zalloc_cpumask_var(&mask, GFP_KERNEL)) {
> +			rc = -ENOMEM;
> +			goto out;
> +		}
>  		for_each_online_cpu(cpu) {
>  			rc = cpum_cf_alloc_cpu(cpu);

[Severity: Medium]
Is there a missing hotplug lock here?

Since for_each_online_cpu() is called without cpus_read_lock(), can this
race with CPU hotplug and cause a memory leak?

If a new CPU is added to cpu_online_mask before the hotplug callbacks
execute, this loop will allocate a cpuhw structure for it and increment
tskcnt. Concurrently, when cpum_cf_online_cpu() runs for the new CPU, it
will see tskcnt > 0 and call cpum_cf_alloc_cpu() again, bumping the
refcount to 2.

When the perf event is later destroyed, cpum_cf_free() will only drop the
refcount by 1, permanently leaking the structure.

[ ... ]
> @@ -1105,13 +1125,13 @@ static int cpum_cf_online_cpu(unsigned int cpu)
>  {
>  	int rc = 0;
>  
> -	/*
> -	 * Ignore notification for perf_event_open().
> -	 * Handle only /dev/hwctr device sessions.
> -	 */
>  	mutex_lock(&cfset_ctrset_mutex);
> -	if (refcount_read(&cfset_opencnt)) {
> +	/* Allocate per-CPU infrastructure when per-task context active. */
> +	mutex_lock(&pmc_reserve_mutex);
> +	if (atomic_read(&cpu_cf_root.tskcnt))
>  		rc = cpum_cf_alloc_cpu(cpu);

[Severity: High]
If there are multiple active task-context events, does this correctly track
the reference counts?

When a CPU is hotplugged online and tskcnt is greater than 1 (for example,
if userspace has opened two task-context perf events), cpum_cf_alloc_cpu()
is only called once. This initializes the new CPU's refcnt to 1, regardless
of the actual number of active contexts.

If userspace then closes just one of the perf events, cpum_cf_free() will
decrement the refcnt for all CPUs by 1. For the newly onlined CPU, the
refcnt drops to 0 and its cpuhw structure is freed.

However, the second perf event is still active. When the task is scheduled
out from this CPU, perf core calls cpumf_pmu_stop():

cpumf_pmu_stop() {
   cpuhw = this_cpu_cfhw();
   ...
   if (!atomic_dec_return(&cpuhw->ctr_set[i]))
}

Since cpuhw is now NULL, won't this dereference a NULL pointer and cause a
kernel panic?

> +	mutex_unlock(&pmc_reserve_mutex);
> +	if (refcount_read(&cfset_opencnt)) {
>  		if (!rc)
>  			cfset_online_cpu(cpu);
>  	}
> @@ -1130,13 +1150,12 @@ static int cpum_cf_offline_cpu(unsigned int cpu)
>  	 * perf_event_open() created events. Perf common code triggers event
>  	 * destruction when the event file descriptor is closed.
>  	 *
> -	 * Handle only /dev/hwctr device sessions.
> +	 * Handle /dev/hwctr device sessions.
>  	 */
>  	mutex_lock(&cfset_ctrset_mutex);
> -	if (refcount_read(&cfset_opencnt)) {
> +	if (refcount_read(&cfset_opencnt))
>  		cfset_offline_cpu(cpu);
> -		cpum_cf_free_cpu(cpu);
> -	}
> +	cpum_cf_free(cpu);

[Severity: High]
Does unconditionally freeing the per-CPU data break active per-CPU events?

If a per-CPU event is active (which does not increment tskcnt), and its
target CPU is hotplugged offline, this cpum_cf_free() call will drop the
reference count to 0 and free the cpuhw structure.

When the CPU is brought back online, cpum_cf_online_cpu() checks if
tskcnt > 0. Since this is a per-CPU event, tskcnt is 0, and the cpuhw
structure is not reallocated.

The CPU will come online with a NULL cpuhw pointer. When the active per-CPU
event is scheduled on this CPU, cpumf_pmu_add() will evaluate cpuhw == NULL
and return -ENODEV, permanently preventing the event from counting.

>  	mutex_unlock(&cfset_ctrset_mutex);
>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1