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

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

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1