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

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

[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 | 70 +++++++++++++++++++++------------
 1 file changed, 44 insertions(+), 26 deletions(-)

diff --git a/arch/s390/kernel/perf_cpum_cf.c b/arch/s390/kernel/perf_cpum_cf.c
index 2076ac22e2c4..817978039744 100644
--- a/arch/s390/kernel/perf_cpum_cf.c
+++ b/arch/s390/kernel/perf_cpum_cf.c
@@ -175,7 +175,7 @@ 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\n",
 			    __func__, refcount_read(&cpu_cf_root.refcnt),
 			    !cpu_cf_root.cfptr);
 }
@@ -212,14 +212,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 +226,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 +242,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 +268,6 @@ static int cpum_cf_alloc_cpu(int cpu)
 		cpum_cf_free_root();
 	}
 unlock:
-	mutex_unlock(&pmc_reserve_mutex);
 	return rc;
 }
 
@@ -290,9 +285,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) {
@@ -306,17 +304,21 @@ static int cpum_cf_alloc(int cpu)
 	} 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);
 	} else {
 		cpum_cf_free_cpu(cpu);
 	}
+	mutex_unlock(&pmc_reserve_mutex);
 }
 
 #define	CF_DIAG_CTRSET_DEF		0xfeef	/* Counter set header mark */
@@ -1026,6 +1028,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 +1047,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 +1102,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 +1121,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 event task active. */
+	mutex_lock(&pmc_reserve_mutex);
+	if (refcount_read(&cpu_cf_root.refcnt))
 		rc = cpum_cf_alloc_cpu(cpu);
+	mutex_unlock(&pmc_reserve_mutex);
+	if (refcount_read(&cfset_opencnt)) {
 		if (!rc)
 			cfset_online_cpu(cpu);
 	}
@@ -1130,13 +1146,15 @@ 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);
+	mutex_lock(&pmc_reserve_mutex);
+	if (refcount_read(&cpu_cf_root.refcnt))
 		cpum_cf_free_cpu(cpu);
-	}
+	mutex_unlock(&pmc_reserve_mutex);
 	mutex_unlock(&cfset_ctrset_mutex);
 	return 0;
 }
-- 
2.55.0

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The CPU hotplug handler incorrectly initializes the per-CPU PMU structure refcount to 1, leading to premature freeing and subsequent UAF/NULL dereference for active events.
- [High] Kernel panic in cpumf_pmu_stop() during CPU offline teardown because cpuhw is freed before the event is stopped.
- [Medium] The CPU hotplug handler misinterprets `cpu_cf_root.refcnt` as a count of per-task events, erroneously allocating structures for hotplugged CPUs when only per-CPU events exist.
- [Medium] Concurrent per-task event initialization and CPU hotplug can double-allocate CPUMF references, leading to a permanent memory leak.
--

--- Patch [1]: [PATCH] s390/cpum_cf: Handle CPU hotplug add and delete 5 ---
commit 4991a250e6aaf68659c77722e073f5976ce4c570
Author: Thomas Richter <tmricht@linux.ibm.com>

s390/cpum_cf: Handle CPU hotplug add and delete 5

This patch addresses a kernel crash that occurs when CPUs are hotplugged
during a per-task perf session. It modifies the CPU hotplug add and delete
handlers to allocate and free the CPUMF per-CPU infrastructure when per-task
processes are active.

> diff --git a/arch/s390/kernel/perf_cpum_cf.c b/arch/s390/kernel/perf_cpum_cf.c
> index 2076ac22e2c49..817978039744f 100644
> --- a/arch/s390/kernel/perf_cpum_cf.c
> +++ b/arch/s390/kernel/perf_cpum_cf.c
[ ... ]
> @@ -1105,13 +1121,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 event task active. */
> +	mutex_lock(&pmc_reserve_mutex);
> +	if (refcount_read(&cpu_cf_root.refcnt))
>  		rc = cpum_cf_alloc_cpu(cpu);

[Severity: High]
Does this incorrectly initialize the per-CPU PMU structure refcount to 1?

If a user creates multiple per-task perf events, the global number of active
events is N > 1. When a new CPU is hotplugged online, cpum_cf_alloc_cpu()
allocates cpuhw for this CPU but sets its refcount to 1 instead of N.

When the user closes any single event, cpum_cf_free() iterates over all
online CPUs and decrements the refcount by 1. On the newly hotplugged CPU,
the refcount drops from 1 to 0 and cpuhw is freed, even though N-1 events
are still active.

When one of these remaining events is disabled, rotated, or scheduled out,
cpumf_pmu_stop() is called and dereferences the NULL cpuhw pointer, causing
a panic.

[Severity: Medium]
Does this misinterpret cpu_cf_root.refcnt as a count of per-task events?

The cpu_cf_root.refcnt counter tracks all CPUMF allocations, including
per-CPU events. If a user creates a per-CPU event on CPU 0, the root
refcount becomes > 0.

If CPU 1 is then hotplugged online, this check sees the non-zero refcount
and assumes per-task events exist, allocating a cpuhw on CPU 1.

When the per-CPU event on CPU 0 is eventually destroyed, cpum_cf_free()
only cleans up CPU 0, permanently leaking the cpuhw allocated on CPU 1.

[Severity: Medium]
Can concurrent per-task event initialization and CPU hotplug cause a double
allocation here?

When a new CPU comes online, it is added to the online mask before the
architecture-specific CPUHP callbacks are executed. If a per-task perf event
is initialized concurrently, cpum_cf_alloc() iterates over the online CPU
mask without holding hotplug locks:

	if (cpu == -1) {
		if (!zalloc_cpumask_var(&mask, GFP_KERNEL)) {
			rc = -ENOMEM;
			goto out;
		}
		for_each_online_cpu(cpu) {
			rc = cpum_cf_alloc_cpu(cpu);

It will observe the new CPU in the mask and allocate a cpuhw with a refcount
of 1. Shortly after, the CPU hotplug thread executes cpum_cf_online_cpu()
for the new CPU. It sees cpu_cf_root.refcnt > 0 and unconditionally calls
cpum_cf_alloc_cpu() a second time for the same CPU, incrementing the refcount
to 2.

When the single event is later destroyed, the refcount only drops by 1,
permanently leaking the cpuhw structure.

> +	mutex_unlock(&pmc_reserve_mutex);
> +	if (refcount_read(&cfset_opencnt)) {
>  		if (!rc)
>  			cfset_online_cpu(cpu);
>  	}
[ ... ]
> @@ -1130,13 +1146,15 @@ 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);
> +	mutex_lock(&pmc_reserve_mutex);
> +	if (refcount_read(&cpu_cf_root.refcnt))
>  		cpum_cf_free_cpu(cpu);

[Severity: High]
Does this prematurely drop the refcount and potentially free cpuhw before
the perf events are stopped?

During a CPU offline operation, teardown callbacks are executed in reverse
order. cpum_cf_offline_cpu() is registered at CPUHP_AP_PERF_S390_CF_ONLINE,
which runs before the perf core's teardown at CPUHP_AP_PERF_ONLINE.

This modified logic unconditionally calls cpum_cf_free_cpu() if the root
refcount is non-zero, which can free cpuhw if its refcount reaches 0.

Later, when the perf core attempts to stop active events or multiplex timers
fire during the offlining window, cpumf_pmu_stop() is invoked. Since it lacks
a NULL check, it dereferences the NULL cpuhw pointer, leading to a panic.

> -	}
> +	mutex_unlock(&pmc_reserve_mutex);
>  	mutex_unlock(&cfset_ctrset_mutex);
>  	return 0;
>  }

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