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

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

[Thomas Richter]

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.

Cc: stable@vger.kernel.org
Signed-off-by: Thomas Richter <tmricht@linux.ibm.com>
---
 arch/s390/kernel/perf_pai.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/arch/s390/kernel/perf_pai.c b/arch/s390/kernel/perf_pai.c
index cdb8006220ca..7c13f5586c79 100644
--- a/arch/s390/kernel/perf_pai.c
+++ b/arch/s390/kernel/perf_pai.c
@@ -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) {
-- 
2.55.0

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

[Thomas Richter]

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

Signed-off-by: Thomas Richter <tmricht@linux.ibm.com>
---
 arch/s390/kernel/perf_pai.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/s390/kernel/perf_pai.c b/arch/s390/kernel/perf_pai.c
index 7c13f5586c79..03c5bda53ccc 100644
--- a/arch/s390/kernel/perf_pai.c
+++ b/arch/s390/kernel/perf_pai.c
@@ -145,14 +145,12 @@ 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)
@@ -246,11 +246,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,8 +307,7 @@ 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;
 }
@@ -390,10 +388,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;
-- 
2.55.0

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

[sashiko-bot]

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

Pre-existing issues:
- [High] The PAI PMU lacks CPU hotplug state registration. When a per-task event is created, per-CPU data is allocated only for currently online CPUs. If a task migrates to a newly onlined CPU, PMU callbacks dereference a NULL per-CPU pointer, causing a kernel panic.
--

--- Patch [1]: [PATCH 1/2] s390/pai: Use PAI PMU index as parameter replacing event ---
commit a1e342cbbe1229dba8d0d235d003b35f9d061a5f
Author: Thomas Richter <tmricht@linux.ibm.com>

s390/pai: Use PAI PMU index as parameter replacing event

This patch updates the PAI PMU code to use the PAI index value directly
as a function argument instead of passing a pointer to struct perf_event.
This simplifies functions like pai_alloc_cpu() and pai_event_destroy_cpu()
without introducing any functional changes.

> diff --git a/arch/s390/kernel/perf_pai.c b/arch/s390/kernel/perf_pai.c
> index cdb8006220ca0..7c13f5586c793 100644
> --- a/arch/s390/kernel/perf_pai.c
> +++ b/arch/s390/kernel/perf_pai.c

[ ... ]

> @@ -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);

[Severity: High]
This is a pre-existing issue, but does this allocation strategy handle CPU
hotplug correctly?

Since memory for the PMU context is only allocated for currently online
CPUs, what happens if a task migrates to a newly onlined CPU?

Looking at pai_add(), if a task is scheduled on a newly hotplugged CPU
where no memory was allocated, it seems mp->mapptr will be NULL:

arch/s390/kernel/perf_pai.c:pai_add() {
    ...
    struct pai_map *cpump = mp->mapptr;
    struct paiext_cb *pcb = cpump->paiext_cb;
    ...
}

Can this lead to a NULL pointer dereference when accessing cpump->paiext_cb?

Should the PAI PMU register for CPU hotplug states to ensure memory is
allocated when new CPUs come online?

>  		if (rc) {
>  			for_each_cpu(cpu, maskptr)
> -				pai_event_destroy_cpu(event, cpu);
> +				pai_event_destroy_cpu(idx, cpu);
>  			kfree(maskptr);
>  			goto out;
>  		}

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

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

[sashiko-bot]

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

Pre-existing issues:
- [High] Missing CPU hotplug support causes a NULL pointer dereference in `pai_add()` when a task with an active PAI event migrates to a newly onlined CPU.
--

--- Patch [2]: [PATCH 2/2] s390/pai: Move locking to event init and delete ---
commit fa13d9fdfca46f4567335a92efab0ab04b313967
Author: Thomas Richter <tmricht@linux.ibm.com>

s390/pai: Move locking to event init and delete

This commit moves the mutex locking from the per-CPU allocation functions
to the higher-level event allocation and deletion routines. No functional
changes are introduced.

> diff --git a/arch/s390/kernel/perf_pai.c b/arch/s390/kernel/perf_pai.c
> index 7c13f5586c793..03c5bda53cccb 100644
> --- a/arch/s390/kernel/perf_pai.c
> +++ b/arch/s390/kernel/perf_pai.c

[ ... ]

> @@ -390,10 +388,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);

[Severity: High]
This is a pre-existing issue, but does the current design lack CPU hotplug
support?

Looking at pai_alloc() called here, it only allocates memory for currently
online CPUs:

pai_alloc() {
...
        for_each_online_cpu(cpu) {
                rc = pai_alloc_cpu(idx, cpu);
...

If a task-bound PAI event is created and a new CPU is hotplugged later, the
per-CPU mapptr for the new CPU appears to remain NULL.

When the task with the active event is scheduled onto the newly onlined
CPU, the perf core will call pai_add(), which seems to unconditionally
dereference cpump:

pai_add() {
        struct pai_map *cpump = mp->mapptr;
        struct paiext_cb *pcb = cpump->paiext_cb;

        if (++cpump->active_events == 1) {
...

Can this result in a kernel panic due to a NULL pointer dereference when
a task migrates to a newly onlined CPU?

> +	mutex_unlock(&pai_reserve_mutex);
>  	if (rc) {
>  		free_page(PAI_SAVE_AREA(event));
>  		goto out;

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