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

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

[Thomas Richter]

V4: Addressed comments from Sumanth
    Fixed typos
    Reworked mutex locking to avoid race

V3: Implements Heiko's option 3
    Count per-task processes and only install CPUMF per-CPU
    infratructure when a per-task process is running during
    CPU hot plug add.

The command 'perf stat -e cycles -- <command> crashes the kernel
when CPUs are hotplug added during that run.

Root cause is the allocation of struct cpu_cf_events at first
event initialization. The allocation is dynamic and the first
event that has task context creates such a structure for
each online CPU. This is not sufficient. CPUs may be offline
during event creation and can be set online during the
perf run time. For example commands

 # echo 0 > /sys/devices/system/cpu/cpu1/online
 # perf stat -e cycles -i -- stress-ng -t10s --matrix X
 # sleep 1
 # echo 1 > /sys/devices/system/cpu/cpu1/online

create an event for CPUs 0,2-X. Since the events are created with
task-context, the scheduler will eventually schedule the program
on CPU1. This CPU has not created and initialized any per
CPU event infrastructure as that CPU was not online at the time
of the perf invocation. Thus when the scheduler runs stress-ng
on CPU1, the function cpumf_pmu_add() refers to a NULL pointer:

 struct cpu_cf_events *cpuhw = this_cpu_cfhw();

This function call is invoked after the task stress-ng has been
made runnable on CPU1. And this_cpu_cfhw() returns NULL.

The result is a panic:
[1148608.961663] Unable to handle kernel pointer dereference in virtual kernel address space
[1148608.961677] Failing address: 0000000000000000 TEID: 0000000000000483
[1148608.961679] Fault in home space mode while using kernel ASCE.
[1148608.961683] AS:000000ff318dc007 R3:000000fffd5a8007 S:000000fffd5a7801 P:000000000000013d
[1148608.961733] Oops: 0004 ilc:3 [#1]SMP
[1148608.961738] Modules linked in: nf_tables ib_core vhost_net vhost
                 ....
[1148608.961797] Hardware name: IBM 9175 ML1 400 (LPAR)
[1148608.961799] Krnl PSW : 0404d00180000000 000003ef8291fd0c (cpumf_pmu_add+0x3c/0x80)
[1148608.961813]            R:0 T:1 IO:0 EX:0 Key:0 M:1 W:0 P:0 AS:3 CC:1 PM:0 RI:0 EA:3
[1148608.961816] Krnl GPRS: 0000000000000026 0000000000040000 0000000000000000 0000000000000004
[1148608.961818]            0000000000000130 fffffd105615b000 0000000000000000 0000000085324300
[1148608.961820]            0000000000000000 00000000857edc80 0000000000000001 0000000290090000
[1148608.961821]            000003ff9ad094c0 0000000290090000 000003ef8291fcfa 0000036f86ccb3e0
[1148608.961830] Krnl Code: 000003ef8291fcfa: e330b1880004      lg      %r3,392(%r11)
           000003ef8291fd00: e31020180004       lg      %r1,24(%r2)
          #000003ef8291fd06: ec13002f1056       rosbg   %r1,%r3,0,47,16
          >000003ef8291fd0c: e31020180024       stg     %r1,24(%r2)
           000003ef8291fd12: e54cb1f00003       mvhi    496(%r11),3
           000003ef8291fd18: a7a10001           tmll    %r10,1
           000003ef8291fd1c: a774000a           brc     7,000003ef8291fd30
           000003ef8291fd20: a7290000           lghi    %r2,0
[1148608.961880] Call Trace:
[1148608.961881]  [<000003ef8291fd0c>] cpumf_pmu_add+0x3c/0x80
[1148608.961885]  [<000003ef82bb5e3e>] event_sched_in+0xae/0x190
[1148608.961890]  [<000003ef82bb60d6>] merge_sched_in+0x1b6/0x390
[1148608.961892]  [<000003ef82bb65b8>] visit_groups_merge.constprop.0.isra.0+0x308/0x5b0
[1148608.961894]  [<000003ef82bb689a>] pmu_groups_sched_in+0x3a/0x50
[1148608.961896]  [<000003ef82bb6a30>] ctx_sched_in+0x180/0x260
[1148608.961898]  [<000003ef82bb780c>] perf_event_context_sched_in+0x11c/0x2d0
[1148608.961900]  [<000003ef82bb79ee>] __perf_event_task_sched_in+0x2e/0xc0
[1148608.961902]  [<000003ef82994834>] finish_task_switch.isra.0+0x1a4/0x250
[1148608.961907]  [<000003ef8340b5e6>] __schedule+0x376/0x750
[1148608.961914]  [<000003ef8340b9fe>] schedule+0x3e/0xd0
[1148608.961916]  [<000003ef83412ca2>] schedule_hrtimeout_range_clock+0xc2/0x110
[1148608.961919]  [<000003ef82d0d36c>] poll_schedule_timeout.constprop.0+0x5c/0xb0
[1148608.961926]  [<000003ef82d0e276>] do_poll+0x286/0x3b0
[1148608.961928]  [<000003ef82d0e5a8>] do_sys_poll+0x208/0x2f0
[1148608.961931]  [<000003ef82d0f230>] __s390x_sys_poll+0xe0/0x160
[1148608.961934]  [<000003ef83408028>] __do_syscall+0x168/0x290
[1148608.961936]  [<000003ef83413754>] system_call+0x74/0x98
[1148608.961938] Last Breaking-Event-Address:
[1148608.961939]  [<000003ef8291f1d8>] this_cpu_cfhw+0x38/0x40
[1148608.961943] Kernel panic - not syncing: Fatal exception: panic_on_oops

The issue arises only in per-task context when the CPUMF facility is
used and the scheduler picks a random CPU for such a process to run on.
The scheduler enables the CPUMF infrastructure via PMU callback
functions pmu::add() and pmu::del().
Now count all per-task processes currently running and active.
When a CPU is hotplug added, check for running per-task context
processes. If one or more are active, install the CPUMF instructure
on that new CPU.  This ensures the intrastructure is available when
new CPU is selected to run the per-task context process.

Fixes: 9b9cf3c77e7e ("s390/cpum_cf: rework PER_CPU_DEFINE of struct cpu_cf_events")
Cc: <Stable@vger.kernel.org> # v6.10+
Signed-off-by: Thomas Richter <tmricht@linux.ibm.com>
---
 arch/s390/kernel/perf_cpum_cf.c | 75 +++++++++++++++++++++------------
 1 file changed, 47 insertions(+), 28 deletions(-)

diff --git a/arch/s390/kernel/perf_cpum_cf.c b/arch/s390/kernel/perf_cpum_cf.c
index 2076ac22e2c4..f24b2145a0df 100644
--- a/arch/s390/kernel/perf_cpum_cf.c
+++ b/arch/s390/kernel/perf_cpum_cf.c
@@ -110,6 +110,7 @@ struct cpu_cf_ptr {
 
 static struct cpu_cf_root {		/* Anchor to per CPU data */
 	refcount_t refcnt;		/* Overall active events */
+	atomic_t tskcnt;		/* Number task-context users */
 	struct cpu_cf_ptr __percpu *cfptr;
 } cpu_cf_root;
 
@@ -175,9 +176,9 @@ static void cpum_cf_free_root(void)
 	cpu_cf_root.cfptr = NULL;
 	irq_subclass_unregister(IRQ_SUBCLASS_MEASUREMENT_ALERT);
 	on_each_cpu(cpum_cf_reset_cpu, NULL, 1);
-	debug_sprintf_event(cf_dbg, 4, "%s root.refcnt %u cfptr %d\n",
+	debug_sprintf_event(cf_dbg, 3, "%s root.refcnt %u cfptr %d tskcnt %d\n",
 			    __func__, refcount_read(&cpu_cf_root.refcnt),
-			    !cpu_cf_root.cfptr);
+			    !cpu_cf_root.cfptr, atomic_read(&cpu_cf_root.tskcnt));
 }
 
 /*
@@ -212,14 +213,13 @@ 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;
 	}
 	cpum_cf_free_root();
-out:
-	mutex_unlock(&pmc_reserve_mutex);
 }
 
 /* Allocate CPU counter data structure for a PMU. Called under mutex lock. */
@@ -245,7 +243,6 @@ static int cpum_cf_alloc_cpu(int cpu)
 	struct cpu_cf_ptr *p;
 	int rc;
 
-	mutex_lock(&pmc_reserve_mutex);
 	rc = cpum_cf_alloc_root();
 	if (rc)
 		goto unlock;
@@ -272,7 +269,6 @@ static int cpum_cf_alloc_cpu(int cpu)
 		cpum_cf_free_root();
 	}
 unlock:
-	mutex_unlock(&pmc_reserve_mutex);
 	return rc;
 }
 
@@ -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);
 	} else {
 		cpum_cf_free_cpu(cpu);
 	}
+	mutex_unlock(&pmc_reserve_mutex);
 }
 
 #define	CF_DIAG_CTRSET_DEF		0xfeef	/* Counter set header mark */
@@ -1026,6 +1032,11 @@ static int cpumf_pmu_add(struct perf_event *event, int flags)
 {
 	struct cpu_cf_events *cpuhw = this_cpu_cfhw();
 
+	/* 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 -ENODEV;
 	ctr_set_enable(&cpuhw->state, event->hw.config_base);
 	event->hw.state = PERF_HES_UPTODATE | PERF_HES_STOPPED;
 
@@ -1040,6 +1051,11 @@ static void cpumf_pmu_del(struct perf_event *event, int flags)
 	struct cpu_cf_events *cpuhw = this_cpu_cfhw();
 	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;
 	cpumf_pmu_stop(event, PERF_EF_UPDATE);
 
 	/* Check if any counter in the counter set is still used.  If not used,
@@ -1090,14 +1106,18 @@ 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 cpum_cf_add(), cpum_cf_start(),
+ * cpum_cf_stop() and cpum_cf_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);
 
@@ -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);
+	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);
 	mutex_unlock(&cfset_ctrset_mutex);
 	return 0;
 }
-- 
2.55.0

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

[sashiko-bot]

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

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/20260803091940.1318933-1-tmricht@linux.ibm.com?part=1