[PATCH v3] s390/pai: Handle multiple PMU stop callback invocations

[PATCH v3] s390/pai: Handle multiple PMU stop callback invocations

[Thomas Richter]

Handle the following scenario:
The kernel protects itself against a very high sampling load and
throttles the sampling using:

  perf_event_throttle() --> PMU->stop()

Shortly later the scheduler may terminate the task and removes it from the
CPU. It again calls

  PMU->stop()

which results in two invocations of PMU->stop() called back to back.
Protect against this and check the PERF_HES_STOPPED bit on function
entry.  If it is already set return.
Clear bit PERF_HES_STOPPED in PMU->start().

Also fix unsafe iteration over syswide_list in pai_have_samples()
which might lead to a kernel crash (LIST_POISON dereference) if an
event overflows and is synchronously throttled during the loop.

Move calls to perf_sched_cb_inc() and perf_sched_cb_dec() to
functions pai_add() and pai_del() to avoid list corruption during
event throttling processing.

 #Cc: stable@vger.kernel.org # v6.19+
Fixes: 9f66572f2889 ("s390/pai_crypto: Enable per-task and system-wide sampling event")
Fixes: 582cc1b28e8c ("s390/pai_ext: Enable per-task and system-wide sampling event")

Signed-off-by: Thomas Richter <tmricht@linux.ibm.com>
Reviewed-by: Sumanth Korikkar <sumanthk@linux.ibm.com>
Suggested-by: Heiko Carstens <hca@linux.ibm.com>
---
 arch/s390/kernel/perf_pai.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/arch/s390/kernel/perf_pai.c b/arch/s390/kernel/perf_pai.c
index cdb8006220ca..388941d9aec1 100644
--- a/arch/s390/kernel/perf_pai.c
+++ b/arch/s390/kernel/perf_pai.c
@@ -456,14 +456,13 @@ static void pai_start(struct perf_event *event, int flags,
 		local64_set(&event->hw.prev_count, sum);
 	} else {				/* Sampling */
 		memcpy((void *)PAI_SAVE_AREA(event), cpump->area, pp->area_size);
-		/* Enable context switch callback for system-wide sampling */
 		if (!(event->attach_state & PERF_ATTACH_TASK)) {
 			list_add_tail(PAI_SWLIST(event), &cpump->syswide_list);
-			perf_sched_cb_inc(event->pmu);
 		} else {
 			cpump->event = event;
 		}
 	}
+	event->hw.state &= ~PERF_HES_STOPPED;
 }
 
 static void paicrypt_start(struct perf_event *event, int flags)
@@ -492,6 +491,9 @@ static int pai_add(struct perf_event *event, int flags)
 			local_ctl_set_bit(0, CR0_PAI_EXTENSION_BIT);
 		}
 	}
+	/* Enable context switch callback for system-wide sampling */
+	if (!(event->attach_state & PERF_ATTACH_TASK))
+		perf_sched_cb_inc(event->pmu);
 	if (flags & PERF_EF_START)
 		pai_pmu[idx].pmu->start(event, PERF_EF_RELOAD);
 	event->hw.state = 0;
@@ -510,11 +512,17 @@ static void pai_stop(struct perf_event *event, int flags)
 	struct pai_mapptr *mp = this_cpu_ptr(pai_root[idx].mapptr);
 	struct pai_map *cpump = mp->mapptr;
 
+	/* Cope with multiple invocations:
+	 *   1. perf_event_throttle() --> PMU->stop()
+	 *   2. task schedules out --> PMU->stop()
+	 * Check for event already stopped.
+	 */
+	if (event->hw.state & PERF_HES_STOPPED)
+		return;
 	if (!event->attr.sample_period) {	/* Counting */
 		pai_pmu[idx].pmu->read(event);
 	} else {				/* Sampling */
 		if (!(event->attach_state & PERF_ATTACH_TASK)) {
-			perf_sched_cb_dec(event->pmu);
 			list_del(PAI_SWLIST(event));
 		} else {
 			pai_have_sample(event, cpump);
@@ -537,6 +545,9 @@ static void pai_del(struct perf_event *event, int flags)
 	struct paiext_cb *pcb = cpump->paiext_cb;
 
 	pai_pmu[idx].pmu->stop(event, PERF_EF_UPDATE);
+	/* Disable context switch callback for system-wide sampling */
+	if (!(event->attach_state & PERF_ATTACH_TASK))
+		perf_sched_cb_dec(event->pmu);
 	if (--cpump->active_events == 0) {
 		if (!pcb) {		/* PAI crypto */
 			local_ctl_clear_bit(0, CR0_CRYPTOGRAPHY_COUNTER_BIT);
@@ -672,9 +683,9 @@ static void pai_have_samples(int idx)
 {
 	struct pai_mapptr *mp = this_cpu_ptr(pai_root[idx].mapptr);
 	struct pai_map *cpump = mp->mapptr;
-	struct perf_event *event;
+	struct perf_event *event, *e2;
 
-	list_for_each_entry(event, &cpump->syswide_list, hw.tp_list)
+	list_for_each_entry_safe(event, e2, &cpump->syswide_list, hw.tp_list)
 		pai_have_sample(event, cpump);
 }
 
-- 
2.55.0

Re: [PATCH v3] s390/pai: Handle multiple PMU stop callback invocations

[sashiko-bot]

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

New issues:
- [Medium] Unnecessarily enabled context switch callback for system-wide counting events.

Pre-existing issues:
- [High] LIST_POISON dereference crash during list iteration due to group throttling.
- [High] Uninitialized list deletion causing a NULL pointer dereference.
--

--- Patch [1]: [PATCH v3] s390/pai: Handle multiple PMU stop callback invocations ---
commit 56c4e8b2eb99bf60f554c282666c15e39068225e
Author: Thomas Richter <tmricht@linux.ibm.com>

s390/pai: Handle multiple PMU stop callback invocations

This commit handles back-to-back invocations of the PMU stop callback caused
by perf_event_throttle() followed by the task scheduling out. It adds a check
for PERF_HES_STOPPED on entry to pai_stop(), and moves context switch callback
registration into pai_add() and pai_del(). It also attempts to fix an unsafe
iteration in pai_have_samples().

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

[ ... ]

> @@ -492,6 +491,9 @@ static int pai_add(struct perf_event *event, int flags)
>  			local_ctl_set_bit(0, CR0_PAI_EXTENSION_BIT);
>  		}
>  	}
> +	/* Enable context switch callback for system-wide sampling */
> +	if (!(event->attach_state & PERF_ATTACH_TASK))
> +		perf_sched_cb_inc(event->pmu);

[Severity: Medium]
When moving perf_sched_cb_inc() into pai_add(), the check for
event->attr.sample_period was dropped. Does this unnecessarily enable the
context switch callback for system-wide counting events?

The core scheduler might pointlessly iterate over the callback list for events
that do not need it, adding overhead on every context switch.

>  	if (flags & PERF_EF_START)
>  		pai_pmu[idx].pmu->start(event, PERF_EF_RELOAD);
>  	event->hw.state = 0;

[Severity: High]
This is a pre-existing issue, but does setting event->hw.state = 0
unconditionally bypass the new PERF_HES_STOPPED protection in pai_stop()?

If PERF_EF_START is not passed (for instance, an inactive sibling in a group
scheduled in via group_sched_in), the event is never added to syswide_list.

When pai_stop() is called later, the early return check for PERF_HES_STOPPED
fails, and list_del() is called on an uninitialized list head.

[ ... ]

> @@ -672,9 +683,9 @@ static void pai_have_samples(int idx)
>  {
>  	struct pai_mapptr *mp = this_cpu_ptr(pai_root[idx].mapptr);
>  	struct pai_map *cpump = mp->mapptr;
> -	struct perf_event *event;
> +	struct perf_event *event, *e2;
>  
> -	list_for_each_entry(event, &cpump->syswide_list, hw.tp_list)
> +	list_for_each_entry_safe(event, e2, &cpump->syswide_list, hw.tp_list)
>  		pai_have_sample(event, cpump);

[Severity: High]
This isn't a bug introduced by this patch, but does this safe iteration
protect against group throttling deleting the cached e2 element?

Pushing a sample in pai_have_sample() can trigger perf_event_overflow(),
which invokes perf_event_throttle_group(). This synchronously stops all
siblings, calling pai_stop() on each.

If the adjacent list sibling (e2) is in the same group, pai_stop() removes
it from syswide_list via list_del(). When list_for_each_entry_safe() advances
to e2, its next pointer would be LIST_POISON1, potentially crashing the
kernel.

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