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

On 8/6/26 15:29, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
> 
> New issues:
> - [High] Unbounded reference count leak and memory leak on CPU hotplug for task events.
> - [High] Per-CPU events are permanently broken and suffer data loss when a CPU is offlined.
> - [High] Reference count overwrite on CPU online leads to Use-After-Free for per-CPU and hardware counter events.
> 
> Pre-existing issues:
> - [High] Data race in `cpum_cf_free_cpu` leads to Use-After-Free if interrupted by hardware counter alert.
> --
> 
> --- Patch [1]: [PATCH] s390/cpum_cf: Handle CPU hotplug add and delete ---
> commit 29becd857d549bdc0388325fd158df7d2f158812
> Author: Thomas Richter <[email protected]>
> 
> s390/cpum_cf: Handle CPU hotplug add and delete
>     
> The command 'perf stat -e cycles -- <command>' crashes the kernel when CPUs
> are hotplug added during that run. This commit tracks task-context users to
> ensure per-CPU structures are allocated when a new CPU is brought online.
> 
> Cc: <[email protected]> # v6.5+
> 
>> diff --git a/arch/s390/kernel/perf_cpum_cf.c b/arch/s390/kernel/perf_cpum_cf.c
>> index 2076ac22e2c49..9e8c3d2e71d57 100644
>> --- a/arch/s390/kernel/perf_cpum_cf.c
>> +++ b/arch/s390/kernel/perf_cpum_cf.c
> 
> [ ... ]
> 
>> @@ -206,20 +207,19 @@ static int cpum_cf_alloc_root(void)
>>  	return rc;
>>  }
>>  
>> -/* Free CPU counter data structure for a PMU */
>> +/* Free CPU counter data structure for a PMU. Called under mutex lock */
>>  static void cpum_cf_free_cpu(int cpu)
>>  {
>>  	struct cpu_cf_events *cpuhw;
>>  	struct cpu_cf_ptr *p;
>>  
>> -	mutex_lock(&pmc_reserve_mutex);
>>  	/*
>>  	 * When invoked via CPU hotplug handler, there might be no events
>>  	 * installed or that particular CPU might not have an
>>  	 * event installed. This anchor pointer can be NULL!
>>  	 */
>>  	if (!cpu_cf_root.cfptr)
>> -		goto out;
>> +		return;
>>  	p = per_cpu_ptr(cpu_cf_root.cfptr, cpu);
>>  	cpuhw = p->cpucf;
>>  	/*
>> @@ -227,15 +227,13 @@ static void cpum_cf_free_cpu(int cpu)
>>  	 * installed on that CPU, but on different CPUs.
>>  	 */
>>  	if (!cpuhw)
>> -		goto out;
>> +		return;
>>  
>>  	if (refcount_dec_and_test(&cpuhw->refcnt)) {
>>  		kfree(cpuhw);
>>  		p->cpucf = NULL;
> 
> [Severity: High]
> This is a pre-existing issue, but is it possible for an interrupt to
> dereference this pointer after it is freed?
> 
> Since cpum_cf_free_cpu() executes with interrupts enabled, if a CPUMF
> measurement alert interrupt triggers right after kfree(cpuhw) but before
> p->cpucf is set to NULL:
> 
> cpum_cf_free_cpu()
>   kfree(cpuhw);
>   
>   --- interrupt occurs ---
>   cpumf_measurement_alert()
>     this_cpu_cfhw() -> returns p->cpucf
>     ... accesses freed memory ...
> 
> Would it be safer to clear the pointer before calling kfree?

The interrupt handler is shared an protects against a NULL ptr:
        /*                                                                    
         * Measurement alerts are shared and might happen when the PMU        
         * is not reserved.  Ignore these alerts in this case.                
         */                                                                   
        cpuhw = this_cpu_cfhw();                                              
        if (!cpuhw)                                                           
                return;
> 
>>  	}
>>  	cpum_cf_free_root();
>> -out:
>> -	mutex_unlock(&pmc_reserve_mutex);
>>  }
> 
> [ ... ]
> 
>> @@ -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);
>>  			if (rc) {
>> @@ -303,20 +302,27 @@ static int cpum_cf_alloc(int cpu)
>>  			cpumask_set_cpu(cpu, mask);
>>  		}
>>  		free_cpumask_var(mask);
>> +		if (!rc)
>> +			atomic_inc(&cpu_cf_root.tskcnt);
>>  	} else {
>>  		rc = cpum_cf_alloc_cpu(cpu);
>>  	}
>> +out:
>> +	mutex_unlock(&pmc_reserve_mutex);
>>  	return rc;
>>  }
>>  
>>  static void cpum_cf_free(int cpu)
>>  {
>> +	mutex_lock(&pmc_reserve_mutex);
>>  	if (cpu == -1) {
>>  		for_each_online_cpu(cpu)
>>  			cpum_cf_free_cpu(cpu);
>> +		atomic_dec(&cpu_cf_root.tskcnt);
> 
> [Severity: High]
> Does this loop potentially leak the per-CPU data structure if a CPU is
> offline?
> 
> If task events are closed while a CPU is offline, iterating only over
> for_each_online_cpu(cpu) skips that offline CPU.
> 
> Will this permanently leave the cpuhw structure and root references orphaned
> for that CPU?

If the CPU is offline, the removal of that per-CPU variable is handled by the
CPU hotplug remove path.

> 
>>  	} else {
>>  		cpum_cf_free_cpu(cpu);
>>  	}
>> +	mutex_unlock(&pmc_reserve_mutex);
>>  }
> 
> [ ... ]
> 
>> @@ -992,6 +1003,12 @@ static void cpumf_pmu_stop(struct perf_event *event, int flags)
>>  	struct hw_perf_event *hwc = &event->hw;
>>  	int i;
>>  
>> +	/* Might be zero when a per-task context event is active. Happens
>> +	 * when CPUs are made offline and process migration takes place.
>> +	 */
>> +	if (!cpuhw)
>> +		return;
>> +
> 
> [Severity: High]
> Could this early return cause us to lose accumulated performance counter data?
> 
> Since CPU hotplug teardown in s390 executes CPUHP_AP_PERF_S390_CF_ONLINE
> (state 209) before the generic perf core's CPUHP_AP_PERF_ONLINE (state 204),
> the per-CPU data structure has already been freed by cpum_cf_offline_cpu().
> 
> When the perf core eventually calls cpumf_pmu_stop() to stop the event, this
> check evaluates to true, aborting the function without updating the hardware
> counter state.
> 

True, this can cause some counter to miss the last read.
Should be addressed in a follow-on patch.

>>  	if (!(hwc->state & PERF_HES_STOPPED)) {
>>  		/* Decrement reference count for this counter set and if this
>>  		 * is the last used counter in the set, clear activation
> 
> [ ... ]
> 
>> @@ -1090,28 +1117,62 @@ static refcount_t cfset_opencnt = REFCOUNT_INIT(0);	/* Access count */
>>  static DEFINE_MUTEX(cfset_ctrset_mutex);
>>  
>>  /*
>> - * CPU hotplug handles only /dev/hwctr device.
>> - * For perf_event_open() the CPU hotplug handling is done on kernel common
>> - * code:
>> - * - CPU add: Nothing is done since a file descriptor can not be created
>> - *   and returned to the user.
>> - * - CPU delete: Handled by common code via pmu_disable(), pmu_stop() and
>> - *   pmu_delete(). The event itself is removed when the file descriptor is
>> - *   closed.
>> + * CPU hotplug handles /dev/hwctr device.
>> + *
>> + * For perf_event_open() the CPU hotplug handler needs to check the number
>> + * of per-task context events currently active. A per-task context event
>> + * needs per-CPU data structures. The scheduler might schedule the task on
>> + * the new CPU and then the CPUMF per-CPU infrastructure must be available.
>> + * Common code relies on that and calls cpumf_pmu_add(), cpumf_pmu_start(),
>> + * cpumf_pmu_stop() and cpumf_pmu_del() to install PMU backend functions on
>> + * the new CPU.
>> + *
>> + * If no per-task context event has been installed, the events are per-CPU
>> + * and do not care about a new CPU.
>>   */
>>  static int cfset_online_cpu(unsigned int cpu);
>>  
>>  static int cpum_cf_online_cpu(unsigned int cpu)
>>  {
>> -	int rc = 0;
>> +	int i, 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]
> Are we failing to reallocate the data structure for CPU-bound events here?
> 
> Since per-CPU events do not increment the task context count (tskcnt), this
> check is bypassed.
> 
> Will subsequent attempts to interact with an existing CPU-bound event
> encounter a NULL pointer on this newly onlined CPU?

per-CPU bound events can not be bound to an offline CPU. New hotplug CPUs
are only used when a per-task event is active. Because the scheduler enables
performance measurement only when the event has per-process context.
When a new event is created with CPU-context, the per-CPU data is allocated
with perf_event_open() for that CPU.

> 
>> +		/* Adjust reference counts:
>> +		 * CPU X is offline
>> +		 *   perf_event_open() task event E1: tskcnt = 1, no cpuhw for CPU X
>> +		 *   perf_event_open() task event E2: tskcnt = 2
>> +		 *   CPU X set online
>> +		 *     cpum_cf_online_cpu()
>> +		 *       cpum_cf_alloc_cpu(X): cpuhw->refcnt = 1
>> +		 * E1 closed
>> +		 *   hw_perf_event_destroy()
>> +		 *     cpum_cf_free(-1)
>> +		 *       cpum_cf_free_cpu(X): refcnt 1 -> 0, kfree(cpuhw)
>> +		 * E2's task runs on CPU X
>> +		 *   cpumf_pmu_add(): this_cpu_cfhw() == NULL, -ENODEV
>> +		 *
>> +		 * If tskcnt > 1, adjust the reference counts to the number
>> +		 * of process-context tasks.
>> +		 */
>> +		if (!rc && atomic_read(&cpu_cf_root.tskcnt) > 1) {
>> +			struct cpu_cf_events *cpuhw;
>> +			struct cpu_cf_ptr *p;
>> +
>> +			p = per_cpu_ptr(cpu_cf_root.cfptr, cpu);
>> +			for (i = 1; i < atomic_read(&cpu_cf_root.tskcnt); ++i)
>> +				cpum_cf_alloc_root();
> 
> [Severity: High]
> Is there a risk of an unbounded reference count leak on the root structure
> here?
> 
> When a CPU goes offline, cpum_cf_offline_cpu() drops a single reference to
> cpuhw and cpu_cf_root.refcnt.
> 
> However, when brought back online with multiple active task events
> (tskcnt > 1), this loop adds tskcnt - 1 new references to
> cpu_cf_root.refcnt, causing it to grow indefinitely on every offline/online
> cycle.

No, when a CPU is hotplug added with multiple per-process tasks active, each
of these tasks will close the event and event deletion on a per-process task
uses for_each_online_cpu(). So when 3 per-process tasks were active at hotplug
add, 3 tasks will close the event and call cpumf_cf_free_cpu() 3 time. Which
is in sync with the reference count.

> 
>> +
>> +			cpuhw = p->cpucf;
>> +			refcount_set(&cpuhw->refcnt,
>> +				     atomic_read(&cpu_cf_root.tskcnt));
> 
> [Severity: High]
> Does forcefully setting the reference count here destroy references held by
> other active subsystems?
> 
> The cpuhw->refcnt field tracks all users on a CPU, including task events,
> per-CPU events, and /dev/hwctr sessions.
> 
> By overwriting it with tskcnt, wouldn't this erase any references held by
> other active sessions, causing a premature free of cpuhw later and leading
> to a Use-After-Free?

No the adjustment is done to match the per-process tasks which close events
with for_each_online_cpu() loops.

> 
>> +		}
>> +	}
>> +	mutex_unlock(&pmc_reserve_mutex);
>> +	if (refcount_read(&cfset_opencnt)) {
>>  		if (!rc)
>>  			cfset_online_cpu(cpu);
>>  	}
>> @@ -1123,20 +1184,11 @@ static int cfset_offline_cpu(unsigned int cpu);
>>  
>>  static int cpum_cf_offline_cpu(unsigned int cpu)
>>  {
>> -	/*
>> -	 * During task exit processing of grouped perf events triggered by CPU
>> -	 * hotplug processing, pmu_disable() is called as part of perf context
>> -	 * removal process. Therefore do not trigger event removal now for
>> -	 * perf_event_open() created events. Perf common code triggers event
>> -	 * destruction when the event file descriptor is closed.
>> -	 *
>> -	 * Handle only /dev/hwctr device sessions.
>> -	 */
>>  	mutex_lock(&cfset_ctrset_mutex);
>> -	if (refcount_read(&cfset_opencnt)) {
>> +	/* Handle /dev/hwctr device sessions */
>> +	if (refcount_read(&cfset_opencnt))
>>  		cfset_offline_cpu(cpu);
>> -		cpum_cf_free_cpu(cpu);
>> -	}
>> +	cpum_cf_free(cpu);
> 
> [Severity: High]
> Does unconditionally freeing the data structure here break active per-CPU
> events?

No the CPU is gone, we remove that CPU's data

> 
> As noted earlier, this function runs before the generic perf core can stop
> the event. Unconditionally dropping the refcount to 0 frees cpuhw while the
> hardware counter is still active.

As discussed above, the call backs protect themselves against NULL pointers.

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


-- 
Thomas Richter, Dept 3303, IBM s390 Linux Development, Boeblingen, Germany
--
IBM Deutschland Research & Development GmbH

Vorsitzender des Aufsichtsrats: Wolfgang Wendt

Geschäftsführung: David Faller

Sitz der Gesellschaft: Böblingen / Registergericht: Amtsgericht Stuttgart, HRB 243294