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

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

Pre-existing issues:
- [Medium] `list_for_each_entry_safe` in `pai_have_samples()` is insufficient to prevent list corruption (dereferencing `LIST_POISON1`) when group throttling removes sibling events concurrently.
- [Medium] The patch fails to prohibit setting `event_limit`, leaving the unsupported `pai_stop()` invocation pathway open via `PERF_EVENT_IOC_REFRESH`.
--

--- Patch [1]: [PATCH v4] s390/pai: Handle multiple PMU stop callback invocations ---
commit 8cb65fa5b6a22b922174b5e65e7e2b3d844b3c12
Author: Thomas Richter <[email protected]>

s390/pai: Handle multiple PMU stop callback invocations

This commit handles back-to-back PMU stop invocations during task
scheduling and event throttling. It also attempts to prevent the
PERF_EVENT_IOC_PERIOD ioctl for specific events and switches to safe
list iteration in pai_have_samples().

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

[ ... ]

> @@ -672,9 +680,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)

[Severity: Medium]
This is a pre-existing issue, but is list_for_each_entry_safe()
sufficient here if group throttling occurs concurrently?

Since this only protects against the removal of the current entry, if
perf_event_throttle_group() synchronously removes the sibling event
(which is temporarily held in e2), could this result in dereferencing
LIST_POISON1 on the next iteration?

>  		pai_have_sample(event, cpump);
>  }
>  
> @@ -691,6 +699,17 @@ static void paicrypt_sched_task(struct perf_event_pmu_context *pmu_ctx,
>  		pai_have_samples(PAI_PMU_CRYPTO);
>  }
>  
> +/* Prevent ioctl(fd, PERF_EVENT_IOC_PERIOD, ...) call.
> + * It sets perf_event::event_limit to a positive value and causes
> + * perf_event_overflow() to invoke pai_stop() call back function when
> + * perf_event::event_limit hits zero. This is not supported because the
> + * sample events CRYPTO_ALL and NNPA_ALL are always taken at schedule out
> + * of a task.
> + */
> +static int pai_check_period(struct perf_event *event, u64 value)
> +{
> +	return -EINVAL;
> +}

[Severity: Medium]
This isn't a bug introduced by this patch, but does this fully prevent
userspace from setting event_limit to a positive value?

While this correctly blocks PERF_EVENT_IOC_PERIOD, could a user still
trigger the unsupported behavior by using PERF_EVENT_IOC_REFRESH, which
also modifies event_limit and bypasses this check?

[ ... ]

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