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

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

[Thomas Richter]

V4: Fixed typos spotted by Sumanth

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 | 52 +++++++++++++++++++++------------
 1 file changed, 33 insertions(+), 19 deletions(-)

diff --git a/arch/s390/kernel/perf_cpum_cf.c b/arch/s390/kernel/perf_cpum_cf.c
index 2076ac22e2c4..bf23ae9a9fd9 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, 4, "%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));
 }
 
 /*
@@ -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);
 	} else {
 		rc = cpum_cf_alloc_cpu(cpu);
 	}
@@ -314,6 +317,7 @@ static void cpum_cf_free(int cpu)
 	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);
 	}
@@ -1026,6 +1030,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 +1049,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 +1104,17 @@ 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() 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 +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);
+	if (refcount_read(&cfset_opencnt)) {
 		if (!rc)
 			cfset_online_cpu(cpu);
 	}
@@ -1130,13 +1145,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(cpu);
 	mutex_unlock(&cfset_ctrset_mutex);
 	return 0;
 }
-- 
2.55.0

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

[sashiko-bot]

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

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

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

[Thomas Richter]

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 infrastructure
on that new CPU.  This ensures the infrastructure 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.5+
Signed-off-by: Thomas Richter <tmricht@linux.ibm.com>
Reviewed-by: Sumanth Korikkar <sumanthk@linux.ibm.com>
---
 arch/s390/kernel/perf_cpum_cf.c | 140 +++++++++++++++++++++++---------
 1 file changed, 100 insertions(+), 40 deletions(-)

diff --git a/arch/s390/kernel/perf_cpum_cf.c b/arch/s390/kernel/perf_cpum_cf.c
index 2076ac22e2c4..aa84ada13d8b 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, 4, "%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));
 }
 
 /*
@@ -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;
 	}
 	cpum_cf_free_root();
-out:
-	mutex_unlock(&pmc_reserve_mutex);
 }
 
 /* Allocate CPU counter data structure for a PMU. Called under mutex lock. */
@@ -245,10 +243,9 @@ 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;
+		goto out;
 	p = per_cpu_ptr(cpu_cf_root.cfptr, cpu);
 	cpuhw = p->cpucf;
 
@@ -271,8 +268,7 @@ static int cpum_cf_alloc_cpu(int cpu)
 		 */
 		cpum_cf_free_root();
 	}
-unlock:
-	mutex_unlock(&pmc_reserve_mutex);
+out:
 	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 */
@@ -924,6 +930,11 @@ static void cpumf_pmu_start(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;
 	if (!(hwc->state & PERF_HES_STOPPED))
 		return;
 
@@ -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;
+
 	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
@@ -1026,6 +1043,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 +1062,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,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);
+		/* 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();
+
+			cpuhw = p->cpucf;
+			refcount_set(&cpuhw->refcnt,
+				     atomic_read(&cpu_cf_root.tskcnt));
+		}
+	}
+	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);
 	mutex_unlock(&cfset_ctrset_mutex);
 	return 0;
 }
@@ -1313,6 +1365,8 @@ static void cfset_ioctl_off(void *parm)
 	struct cfset_call_on_cpu_parm *p = parm;
 	int rc;
 
+	if (!cpuhw)
+		return;
 	/* Check if any counter set used by /dev/hwctr */
 	for (rc = CPUMF_CTR_SET_BASIC; rc < CPUMF_CTR_SET_MAX; ++rc)
 		if ((p->sets & cpumf_ctr_ctl[rc])) {
@@ -1339,6 +1393,8 @@ static void cfset_ioctl_on(void *parm)
 	struct cfset_call_on_cpu_parm *p = parm;
 	int rc;
 
+	if (!cpuhw)
+		return;
 	cpuhw->flags |= PMU_F_IN_USE;
 	ctr_set_enable(&cpuhw->dev_state, p->sets);
 	ctr_set_start(&cpuhw->dev_state, p->sets);
@@ -1358,6 +1414,8 @@ static void cfset_release_cpu(void *p)
 	struct cpu_cf_events *cpuhw = this_cpu_cfhw();
 	int rc;
 
+	if (!cpuhw)
+		return;
 	cpuhw->dev_state = 0;
 	rc = lcctl(cpuhw->state);	/* Keep perf_event_open counter sets */
 	if (rc)
@@ -1530,6 +1588,8 @@ static void cfset_cpu_read(void *parm)
 	int set, set_size;
 	size_t space;
 
+	if (!cpuhw)
+		return;
 	/* No data saved yet */
 	cpuhw->used = 0;
 	cpuhw->sets = 0;
-- 
2.55.0

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

[sashiko-bot]

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

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

[Thomas Richter]

On 8/6/26 15:29, sashiko-bot@kernel.org 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 <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?

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