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

On 8/13/26 09:21, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] Unsafe iteration over `cpump->syswide_list` in `pai_have_samples()` leads to a kernel crash (LIST_POISON dereference) if an event overflows and is synchronously throttled during the loop.

This can not happen. The syswide_list is anchored by per-CPU pai_map::syswide_list and tracks
all events which are bound to that particular CPU. The event was installed only on that particular
CPU and does not move with the process/task. Each CPU has its own list anchored by
pai_map::syswide_list.

The call chains are either

 paicrypt_sched_task() or paiext_sched_task
 +--> pai_have_samples() iterates over syswide_list and calls
      +--> pai_have_sample
      
or    
 pai_stop()
 +--> list_del() to remove the event from that CPU.

 Since a CPU can only execute one given code path at any one time:
 - either schedule out
 - or stop the event
 both can not happen at the same time. PAI PMU does not install an interrupt handler,
 there is no supoprt for interrupts.

 I think this is safe.


> --
> 
> --- Patch [1]: [PATCH] s390/pai: Handle multiple PMU stop callback invocations ---
> commit 8c88016c23029bda1d214155e79ad2671b8e3eb5
> Author: Thomas Richter <[email protected]>
> 
> s390/pai: Handle multiple PMU stop callback invocations
> 
> The kernel protects itself against high sampling load by throttling it via
> perf_event_throttle(), calling PMU->stop(). Shortly after, the task may be
> scheduled out, resulting in a second PMU->stop() invocation. This patch adds
> a check for the PERF_HES_STOPPED bit to prevent duplicate handling.
> 
> Cc: [email protected] # v6.19+
> Fixes: ac03223f0723 ("s390/pai_crypto: Add common pai_stop() function")
> 
>> diff --git a/arch/s390/kernel/perf_pai.c b/arch/s390/kernel/perf_pai.c
>> index cdb8006220ca0..a377f04296563 100644
>> --- a/arch/s390/kernel/perf_pai.c
>> +++ b/arch/s390/kernel/perf_pai.c
> 
> [ ... ]
> 
>> @@ -510,6 +511,13 @@ 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;
> 
> [Severity: High]
> This is a pre-existing issue, but can this synchronous throttle event
> corrupt the list iteration during sample processing?
> 
> Looking at the iteration in pai_have_samples():
> 
> arch/s390/kernel/perf_pai.c:pai_have_samples() {
>     ...
>     list_for_each_entry(event, &cpump->syswide_list, hw.tp_list)
>         pai_have_sample(event, cpump);
>     ...
> }
> 
> If an event reaches its sample limit, perf_event_overflow() triggers
> synchronously, invoking perf_event_throttle() which then calls pai_stop().
> 
> When pai_stop() is called for a system-wide sampling event, it executes:
> 
>     list_del(PAI_SWLIST(event));
> 
> Will deleting the event currently being iterated leave the list pointer
> at a poison value, causing a panic when the loop in pai_have_samples()
> continues? Does pai_have_samples() need to use list_for_each_entry_safe()
> instead?
> 
>>  	if (!event->attr.sample_period) {	/* Counting */
>>  		pai_pmu[idx].pmu->read(event);
>>  	} else {				/* Sampling */
> 


-- 
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