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

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Premature freeing of per-CPU PMU structures leading to Use-After-Free and kernel panics on CPU hotplug.
- [Medium] Missing lock during PMU allocation allows a concurrent CPU hotplug event to silently miss CPUMF initialization.
--

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

s390/cpum_cf: Handle CPU hotplug add and delete

This commit tracks the number of active per-task processes using
a new tskcnt counter. When a CPU is hotplug added, it checks if there
are running per-task processes and installs the CPUMF infrastructure
on the new CPU.

> diff --git a/arch/s390/kernel/perf_cpum_cf.c b/arch/s390/kernel/perf_cpum_cf.c
> index 2076ac22e2c49..bf23ae9a9fd93 100644
> --- a/arch/s390/kernel/perf_cpum_cf.c
> +++ b/arch/s390/kernel/perf_cpum_cf.c

[ ... ]

> @@ -303,6 +304,8 @@ static int cpum_cf_alloc(int cpu)
>  			cpumask_set_cpu(cpu, mask);
>  		}
>  		free_cpumask_var(mask);
> +		if (!rc)
> +			atomic_inc(&cpu_cf_root.tskcnt);

[Severity: Medium]
Could a concurrent CPU hotplug event happen between the for_each_online_cpu()
loop in this function and this atomic_inc()?

Since cpus_read_lock() is not held, a newly hotplugged CPU could observe
tskcnt == 0 in cpum_cf_online_cpu() and skip allocating its per-CPU
infrastructure.

Once the atomic_inc() completes, tskcnt is > 0 but the new CPU remains
permanently without cpuhw, which could cause perf events on that CPU to
silently fail. Is CPU hotplug synchronization needed here?

>  	} else {
>  		rc = cpum_cf_alloc_cpu(cpu);
>  	}

[ ... ]

> @@ -1105,13 +1122,11 @@ 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. */
> +	if (atomic_read(&cpu_cf_root.tskcnt))
>  		rc = cpum_cf_alloc_cpu(cpu);

[Severity: Critical]
If there are multiple active task-context events (e.g., tskcnt > 1), does
allocating the new CPU infrastructure here correctly account for them?

Since cpum_cf_alloc_cpu() initializes the new cpuhw->refcnt to 1 regardless
of the actual tskcnt, could a single task closing later decrement the refcnt
to 0 and free cpuhw prematurely in cpum_cf_free_cpu()?

If other remaining active tasks are running on this newly hotplugged CPU or
trigger PMU callbacks, could they trigger a use-after-free when accessing
the freed cpuhw?

> +	if (refcount_read(&cfset_opencnt)) {
>  		if (!rc)
>  			cfset_online_cpu(cpu);
>  	}

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