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

[sashiko-bot@kernel.org]

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 <tmricht@linux.ibm.com>

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: <stable@vger.kernel.org> # 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?

>  	}
>  	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?

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

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

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

> +
> +			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?

> +		}
> +	}
> +	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?

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.

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260806131131.2073914-1-tmricht@linux.ibm.com?part=1