Skip to contentSkip to conversation

Add CPU hotplug support for PAI PMU

PATCH Superseded Thomas Richter 3 participants 11 messages Sep 1

Subsystems (2)

v8 of 9 Superseded Latest is v9 →

lore
Share & tools
lore
#0 lore raw

v8 cover Add CPU hotplug support for PAI PMU

V8: Add patch version 8 (v8) to each subject line

V7: Add findings from Jan Polensky
- Fixed spelling in code and commit messages.
- Added correct Cc: stable tag.

V6: Add findings from Sashiko
- Remove cpus_read_lock() and cpus_read_unlock() invocations from CPU hotplug handlers in patch 3.

V5: Add findings from Sashiko
- Add cpus_read_lock() and cpus_read_unlock() in patch 3.

V4: Add findings from Sumanth and updated patch 3/3.
- Fix reference count for CPU hotplug remove while multiple per-task events are active.

V3: Add findings from Sumanth and updated patch 3/3.
- Refined Fixes: commit id and messages
- Handle cpuhp_setup_state() failure

V2: Add findings from Sumanth and updated patch 3/3.
- handle possible NULL pointer dereference in pai_event_destroy_cpu()
- handle possibe memory allocation failure in pai_online_cpu()
- add Reviewed-by: Sumanth Korikkar <[email protected]> to patch 1/3 and patch 2/3.

Add support for CPU hotplug add and remove for PAI PMU.

Patch 1: Change function argument list Patch 2: Move locking from CPU scope to event scope Patch 3: Add CPU hotplug handler

Thomas Richter (3):
s390/pai: Use PAI PMU index as parameter replacing event s390/pai: Move locking to event init and delete s390/pai: Support CPU hotplug for PMU PAI

 arch/s390/include/asm/pai.h |   1 -
 arch/s390/kernel/perf_pai.c | 182 +++++++++++++++++++++++++-----------
 2 files changed, 128 insertions(+), 55 deletions(-)
Signature · 1 line
2.55.0
#1 lore raw

PATCH v8 1/3 s390/pai: Use PAI PMU index as parameter replacing event

Use PAI PMU index value as function argument instead of pointer to struct perf_event. Only that index value is used inside functions pai_alloc_cpu() and pai_event_destroy_cpu(). No functional change.

---
 arch/s390/kernel/perf_pai.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)
arch/s390/kernel/perf_pai.c +12 -13
index cdb8006220ca..7c13f5586c79 100644
@@ -140,16 +140,14 @@ static void pai_free(struct pai_mapptr *mp)
 /* Adjust usage counters and remove allocated memory when all users are
  * gone.
  */
-static void pai_event_destroy_cpu(struct perf_event *event, int cpu)
+static void pai_event_destroy_cpu(int idx, int cpu)
 {
-	int idx = PAI_PMU_IDX(event);
 	struct pai_mapptr *mp = per_cpu_ptr(pai_root[idx].mapptr, cpu);
 	struct pai_map *cpump = mp->mapptr;
 
 	mutex_lock(&pai_reserve_mutex);
-	debug_sprintf_event(paidbg, 5, "%s event %#llx idx %d cpu %d users %d "
-			    "refcnt %u\n", __func__, event->attr.config, idx,
-			    event->cpu, cpump->active_events,
+	debug_sprintf_event(paidbg, 5, "%s users %d refcnt %u\n",
+			    __func__, cpump->active_events,
 			    refcount_read(&cpump->refcnt));
 	if (refcount_dec_and_test(&cpump->refcnt))
 		pai_free(mp);
@@ -159,17 +157,17 @@ static void pai_event_destroy_cpu(struct perf_event *event, int cpu)
 
 static void pai_event_destroy(struct perf_event *event)
 {
-	int cpu;
+	int cpu = 0, idx = PAI_PMU_IDX(event);
 
 	free_page(PAI_SAVE_AREA(event));
 	if (event->cpu == -1) {
 		struct cpumask *mask = PAI_CPU_MASK(event);
 
 		for_each_cpu(cpu, mask)
-			pai_event_destroy_cpu(event, cpu);
+			pai_event_destroy_cpu(idx, cpu);
 		kfree(mask);
 	} else {
-		pai_event_destroy_cpu(event, event->cpu);
+		pai_event_destroy_cpu(idx, event->cpu);
 	}
 }
 
@@ -241,12 +239,12 @@ static u64 paicrypt_getall(struct perf_event *event)
  *
  * Allocate the memory for the event.
  */
-static int pai_alloc_cpu(struct perf_event *event, int cpu)
+static int pai_alloc_cpu(int idx, int cpu)
 {
-	int rc, idx = PAI_PMU_IDX(event);
 	struct pai_map *cpump = NULL;
 	bool need_paiext_cb = false;
 	struct pai_mapptr *mp;
+	int rc;
 
 	mutex_lock(&pai_reserve_mutex);
 	/* Allocate root node */
@@ -318,6 +316,7 @@ static int pai_alloc_cpu(struct perf_event *event, int cpu)
 
 static int pai_alloc(struct perf_event *event)
 {
+	int idx = PAI_PMU_IDX(event);
 	struct cpumask *maskptr;
 	int cpu, rc = -ENOMEM;
 
@@ -326,10 +325,10 @@ static int pai_alloc(struct perf_event *event)
 		goto out;
 
 	for_each_online_cpu(cpu) {
-		rc = pai_alloc_cpu(event, cpu);
+		rc = pai_alloc_cpu(idx, cpu);
 		if (rc) {
 			for_each_cpu(cpu, maskptr)
-				pai_event_destroy_cpu(event, cpu);
+				pai_event_destroy_cpu(idx, cpu);
 			kfree(maskptr);
 			goto out;
 		}
@@ -392,7 +391,7 @@ static int pai_event_init(struct perf_event *event, int idx)
 	}
 
 	if (event->cpu >= 0)
-		rc = pai_alloc_cpu(event, event->cpu);
+		rc = pai_alloc_cpu(idx, event->cpu);
 	else
 		rc = pai_alloc(event);
 	if (rc) {
Signature · 1 line
2.55.0
#2 lore raw

Re: [PATCH 1/3 v8] s390/pai: Use PAI PMU index as parameter replacing event

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing bounds check on the hardware-reported NNPA counter count (ib.num_nnpa) leads to an out-of-bounds kernel heap read. --

commit a6f12edfe6f84375133dd3bb77d40f4e8c67385e
Author: Thomas Richter <[email protected]> s390/pai: Use PAI PMU index as parameter replacing event

This patch updates pai_alloc_cpu() and pai_event_destroy_cpu() to use the PAI PMU index value as a parameter instead of a struct perf_event pointer. Only the index value is actually needed inside these functions.

Quoted 4 lines
arch/s390/kernel/perf_pai.c +0 -0
index cdb8006220ca0..7c13f5586c793 100644

[ ... ]

arch/s390/kernel/perf_pai.c static u64 paicrypt_getall(struct perf_event *event) · line 247

Quoted 12 lines
arch/s390/kernel/perf_pai.c +2 -2
@@ -241,12 +239,12 @@ static u64 paicrypt_getall(struct perf_event *event)
  *
  * Allocate the memory for the event.
  */
-static int pai_alloc_cpu(struct perf_event *event, int cpu)
+static int pai_alloc_cpu(int idx, int cpu)
 {
-	int rc, idx = PAI_PMU_IDX(event);
 	struct pai_map *cpump = NULL;
 	bool need_paiext_cb = false;
 	struct pai_mapptr *mp;
+	int rc;

[Severity: High]
This is a pre-existing issue, but I have a question about how the buffer allocated in this function is accessed.

In paipmu_setup(), the hardware-reported counter count (ib.num_nnpa) can be up to 127. The code then artificially increments it for the software ALL event:

arch/s390/kernel/perf_pai.c:paipmu_setup() {
    ...
		case PAI_PMU_EXT:
			p->num_avail = ib.num_nnpa;
			break;
		}
		p->num_avail += 1;		/* Add xxx_ALL event */
    ...
}

This means p->num_avail can reach 128.

Later in pai_alloc_cpu(), cpump->area is allocated with a fixed size:

arch/s390/kernel/perf_pai.c:pai_alloc_cpu() {
    ...
			cpump->area = kzalloc(pai_pmu[idx].area_size, GFP_KERNEL);
    ...
}

For PAI_PMU_EXT, area_size is 1024 bytes, which holds exactly 128 elements.

However, pai_getdata() iterates up to p->num_avail:

arch/s390/kernel/perf_pai.c:pai_getdata() {
    ...
	for (i = 1; i <= pp->num_avail; i++) {
		u64 val = pai_getctr(cpump->area, i,
				     kernel ? pp->kernel_offset : 0);
    ...
}

If ib.num_nnpa is 127, p->num_avail becomes 128. The loop evaluates i = 128 and pai_getctr() accesses cpump->area[128] (the 129th element).

Can this cause an out-of-bounds heap read if cpump->area is strictly 1024 bytes?

Quoted 3 lines
arch/s390/kernel/perf_pai.c +0 -0
 	mutex_lock(&pai_reserve_mutex);
 	/* Allocate root node */
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
#3 lore raw

Re: [PATCH 1/3 v8] s390/pai: Use PAI PMU index as parameter replacing event

Quoted 5 lines · On 9/1/26 11:12, [email protected] wrote:

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing bounds check on the hardware-reported NNPA counter count (ib.num_nnpa) leads to an out-of-bounds kernel heap read. --

This is not correct. The value of ib.num_nnpa is read out from hadware and should be correct. It is assigned to member pai_pmu::num_avail.
Later in function attr_event_init() that value is compared to pai_pmu::num_named which contains the number of supported events. And the minimum of both values is taken:

In attr_event_init()
unsigned int min_attr = min_t(unsigned int, p->num_named, p->num_avail);

So this is protected.

Signature · 9 lines
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
#4 lore raw

PATCH v8 2/3 s390/pai: Move locking to event init and delete

In reply to Thomas Richter

Move mutex locking from per CPU allocation to event allocation. No functional change.

---
 arch/s390/kernel/perf_pai.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)
arch/s390/kernel/perf_pai.c +12 -13
index 7c13f5586c79..52d9f654346a 100644
@@ -138,21 +138,19 @@ static void pai_free(struct pai_mapptr *mp)
 }
 
 /* Adjust usage counters and remove allocated memory when all users are
- * gone.
+ * gone. Called under mutex_lock.
  */
 static void pai_event_destroy_cpu(int idx, int cpu)
 {
 	struct pai_mapptr *mp = per_cpu_ptr(pai_root[idx].mapptr, cpu);
 	struct pai_map *cpump = mp->mapptr;
 
-	mutex_lock(&pai_reserve_mutex);
 	debug_sprintf_event(paidbg, 5, "%s users %d refcnt %u\n",
 			    __func__, cpump->active_events,
 			    refcount_read(&cpump->refcnt));
 	if (refcount_dec_and_test(&cpump->refcnt))
 		pai_free(mp);
 	pai_root_free(idx);
-	mutex_unlock(&pai_reserve_mutex);
 }
 
 static void pai_event_destroy(struct perf_event *event)
@@ -160,6 +158,7 @@ static void pai_event_destroy(struct perf_event *event)
 	int cpu = 0, idx = PAI_PMU_IDX(event);
 
 	free_page(PAI_SAVE_AREA(event));
+	mutex_lock(&pai_reserve_mutex);
 	if (event->cpu == -1) {
 		struct cpumask *mask = PAI_CPU_MASK(event);
 
@@ -169,6 +168,7 @@ static void pai_event_destroy(struct perf_event *event)
 	} else {
 		pai_event_destroy_cpu(idx, event->cpu);
 	}
+	mutex_unlock(&pai_reserve_mutex);
 }
 
 static void paicrypt_event_destroy(struct perf_event *event)
@@ -232,12 +232,10 @@ static u64 paicrypt_getall(struct perf_event *event)
 	return sum;
 }
 
-/* Check concurrent access of counting and sampling for crypto events.
- * This function is called in process context and it is save to block.
- * When the event initialization functions fails, no other call back will
- * be invoked.
- *
- * Allocate the memory for the event.
+/* Allocate all per-CPU data structures. This function is called in
+ * process context and can block. In case of error all partly allocated
+ * memory is released and the reference counters adjusted correctly.
+ * Called under mutex_lock.
  */
 static int pai_alloc_cpu(int idx, int cpu)
 {
@@ -246,11 +244,10 @@ static int pai_alloc_cpu(int idx, int cpu)
 	struct pai_mapptr *mp;
 	int rc;
 
-	mutex_lock(&pai_reserve_mutex);
 	/* Allocate root node */
 	rc = pai_root_alloc(idx);
 	if (rc)
-		goto unlock;
+		goto out;
 
 	/* Allocate node for this event */
 	mp = per_cpu_ptr(pai_root[idx].mapptr, cpu);
@@ -308,12 +305,12 @@ static int pai_alloc_cpu(int idx, int cpu)
 		 */
 		pai_root_free(idx);
 	}
-unlock:
-	mutex_unlock(&pai_reserve_mutex);
+out:
 	/* If rc is non-zero, no increment of counter/sampler was done. */
 	return rc;
 }
 
+/* Called under mutex_lock */
 static int pai_alloc(struct perf_event *event)
 {
 	int idx = PAI_PMU_IDX(event);
@@ -390,10 +387,12 @@ static int pai_event_init(struct perf_event *event, int idx)
 		}
 	}
 
+	mutex_lock(&pai_reserve_mutex);
 	if (event->cpu >= 0)
 		rc = pai_alloc_cpu(idx, event->cpu);
 	else
 		rc = pai_alloc(event);
+	mutex_unlock(&pai_reserve_mutex);
 	if (rc) {
 		free_page(PAI_SAVE_AREA(event));
 		goto out;
Signature · 1 line
2.55.0
#5 lore raw

Re: [PATCH 2/3 v8] s390/pai: Move locking to event init and delete

Sashiko has reviewed this patch and found no issues. It looks great!
Quoted 4 lines

Move mutex locking from per CPU allocation to event allocation. No functional change.

Sashiko has reviewed this patch and found no issues. It looks great!

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
#6 lore raw

PATCH v8 3/3 s390/pai: Support CPU hotplug for PMU PAI

In reply to Thomas Richter

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

Root cause is the missing allocation of per-CPU data structures for that new CPU. 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

Currently without a CPU hotplug handler, that new CPU has no per-CPU data infrastructure. The scheduler runs PMU call back function pai_add() to install the PMU support for that CPU before the task is being scheduled on that new CPU.
In pai_add() instructions

    mp = this_cpu_ptr(pai_root[idx].mapptr);
    cpump = mp->mapptr;

return a NULL pointer and the result is a kernel panic as variable cpump is used inside that function.

Add CPU hotplug support for CPU add and delete and create the necessary per-CPU data infrastructure during CPU hotplug add processing. Same for CPU hotplug remove.
This is done when the CPU is offline to ensure the data structures are available when CPU is made online and tasks are scheduled on it.

Fixes 582cc1b28e8c ("s390/pai_ext: Enable per-task and system-wide sampling event")
Fixes 9f66572f2889 ("s390/pai_crypto: Enable per-task and system-wide sampling event")
---
 arch/s390/include/asm/pai.h |   1 -
 arch/s390/kernel/perf_pai.c | 164 ++++++++++++++++++++++++++----------
 2 files changed, 120 insertions(+), 45 deletions(-)
arch/s390/include/asm/pai.h +0 -1
index 534d0320e2aa..a3456a36aaa7 100644
@@ -76,7 +76,6 @@ static __always_inline void pai_kernel_exit(struct pt_regs *regs)
 }
 
 #define PAI_SAVE_AREA(x)	((x)->hw.event_base)
-#define PAI_CPU_MASK(x)		((x)->hw.addr_filters)
 #define PAI_PMU_IDX(x)		((x)->hw.last_tag)
 #define PAI_SWLIST(x)		(&(x)->hw.tp_list)
 
arch/s390/kernel/perf_pai.c +120 -44

271 lines not on this page · raw message

Signature · 1 line
2.55.0
#7 lore raw

Re: [PATCH 3/3 v8] s390/pai: Support CPU hotplug for PMU PAI

Sashiko has reviewed this patch and found no issues. It looks great!
Quoted 15 lines

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

Root cause is the missing allocation of per-CPU data structures for that new CPU. 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
Quoted 6 lines

[ ... ]

Fixes 582cc1b28e8c ("s390/pai_ext: Enable per-task and system-wide sampling event")
Fixes 9f66572f2889 ("s390/pai_crypto: Enable per-task and system-wide sampling event")

Sashiko has reviewed this patch and found no issues. It looks great!

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3
maintainer
#8 lore raw

Re: [PATCH 3/3 v8] s390/pai: Support CPU hotplug for PMU PAI

In reply to Thomas Richter
Quoted 4 lines · Thomas Richter
(quoted diff) +4 -0
+static int pai_online_cpu(unsigned int cpu)
+{
+	int rc;
+

Review

Quoted 10 lines
(quoted diff) +10 -0
+	mutex_lock(&pai_reserve_mutex);
+	rc = pai_alloc_cpu(PAI_PMU_CRYPTO, cpu, true);
+	if (!rc) {
+		rc = pai_alloc_cpu(PAI_PMU_EXT, cpu, true);
+		if (rc)
+			pai_event_destroy_cpu(PAI_PMU_CRYPTO, cpu, true);
+	}
+	mutex_unlock(&pai_reserve_mutex);
+	return rc;
+}

This unusual coding style. Please change to something like:

	mutex_lock(&pai_reserve_mutex);
	rc = pai_alloc_cpu(PAI_PMU_CRYPTO, cpu, true);
	if (rc)
		goto out;
	rc = pai_alloc_cpu(PAI_PMU_EXT, cpu, true);
	if (rc)
		pai_event_destroy_cpu(PAI_PMU_CRYPTO, cpu, true);
out:
	mutex_unlock(&pai_reserve_mutex);
Quoted 4 lines
(quoted diff) +4 -0
+	/* CPUHP_BP_PREPARE_DYN --> before CPU is brought online */
+	rc = cpuhp_setup_state(CPUHP_BP_PREPARE_DYN, "perf/pai:prepare",
+			       pai_online_cpu, pai_offline_cpu);
+	if (rc < 0) {

Review

Quoted 10 lines
(quoted diff) +6 -0
+		debug_unregister_view(paidbg, &debug_sprintf_view);
+		debug_unregister(paidbg);
+		return rc;
+	}
+
 	if (!paipmu_setup()) {
+		cpuhp_remove_state(rc);
 		/* No PMU registration, no need for debug buffer */
 		debug_unregister_view(paidbg, &debug_sprintf_view);
 		debug_unregister(paidbg);

Code duplication. Can't we have _something_ like:

arch/s390/kernel/perf_pai.c +12 -13
index 72023b35e70f..e1134e50f3e9 100644
@@ -1316,20 +1316,19 @@ static int __init pai_init(void)
 	/* CPUHP_BP_PREPARE_DYN --> before CPU is brought online */
 	rc = cpuhp_setup_state(CPUHP_BP_PREPARE_DYN, "perf/pai:prepare",
 			       pai_online_cpu, pai_offline_cpu);
-	if (rc < 0) {
-		debug_unregister_view(paidbg, &debug_sprintf_view);
-		debug_unregister(paidbg);
-		return rc;
-	}
-
-	if (!paipmu_setup()) {
-		cpuhp_remove_state(rc);
-		/* No PMU registration, no need for debug buffer */
-		debug_unregister_view(paidbg, &debug_sprintf_view);
-		debug_unregister(paidbg);
-		return -ENODEV;
-	}
+	if (rc < 0)
+		goto out_debug;
+	rc = -ENODEV;
+	if (!paipmu_setup())
+		goto out_cpuhp;
 	return 0;
+
+out_cpuhp:
+	cpuhp_remove_state(rc);
+out_debug:
+	debug_unregister_view(paidbg, &debug_sprintf_view);
+	debug_unregister(paidbg);
+	return rc;
 }
 
 device_initcall(pai_init);
maintainer
#9 lore raw
In reply to Thomas Richter
Quoted 6 lines · Thomas Richter

Add support for CPU hotplug add and remove for PAI PMU.

Patch 1: Change function argument list Patch 2: Move locking from CPU scope to event scope Patch 3: Add CPU hotplug handler

Quoted 8 lines

Thomas Richter (3):
s390/pai: Use PAI PMU index as parameter replacing event s390/pai: Move locking to event init and delete s390/pai: Support CPU hotplug for PMU PAI

 arch/s390/include/asm/pai.h |   1 -
 arch/s390/kernel/perf_pai.c | 182 +++++++++++++++++++++++++-----------
 2 files changed, 128 insertions(+), 55 deletions(-)

Series applied, thanks!

maintainer
#10 lore raw
Quoted 12 lines · Heiko Carstens
Quoted 11 lines · Thomas Richter

Add support for CPU hotplug add and remove for PAI PMU.

Patch 1: Change function argument list Patch 2: Move locking from CPU scope to event scope Patch 3: Add CPU hotplug handler

Thomas Richter (3):
s390/pai: Use PAI PMU index as parameter replacing event s390/pai: Move locking to event init and delete s390/pai: Support CPU hotplug for PMU PAI

Quoted 5 lines
Quoted 3 lines
 arch/s390/include/asm/pai.h |   1 -
 arch/s390/kernel/perf_pai.c | 182 +++++++++++++++++++++++++-----------
 2 files changed, 128 insertions(+), 55 deletions(-)

Series applied, thanks!

This was supposed to be a reply to v9.