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

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 <[email protected]>

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/[email protected]?part=1