Re: [PATCH 1/2] s390/pai: Use PAI PMU index as parameter replacing event

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

Pre-existing issues:
- [High] The PAI PMU lacks CPU hotplug state registration. When a per-task event is created, per-CPU data is allocated only for currently online CPUs. If a task migrates to a newly onlined CPU, PMU callbacks dereference a NULL per-CPU pointer, causing a kernel panic.
--

--- Patch [1]: [PATCH 1/2] s390/pai: Use PAI PMU index as parameter replacing event ---
commit a1e342cbbe1229dba8d0d235d003b35f9d061a5f
Author: Thomas Richter <[email protected]>

s390/pai: Use PAI PMU index as parameter replacing event

This patch updates the PAI PMU code to use the PAI index value directly
as a function argument instead of passing a pointer to struct perf_event.
This simplifies functions like pai_alloc_cpu() and pai_event_destroy_cpu()
without introducing any functional changes.

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

[ ... ]

> @@ -318,6 +316,7 @@ static int pai_alloc_cpu(struct perf_event *event, int cpu)
>  
>  static int pai_alloc(struct perf_event *event)
>  {
> +	int idx = PAI_PMU_IDX(event);
>  	struct cpumask *maskptr;
>  	int cpu, rc = -ENOMEM;
>  
> @@ -326,10 +325,10 @@ static int pai_alloc(struct perf_event *event)
>  		goto out;
>  
>  	for_each_online_cpu(cpu) {
> -		rc = pai_alloc_cpu(event, cpu);
> +		rc = pai_alloc_cpu(idx, cpu);

[Severity: High]
This is a pre-existing issue, but does this allocation strategy handle CPU
hotplug correctly?

Since memory for the PMU context is only allocated for currently online
CPUs, what happens if a task migrates to a newly onlined CPU?

Looking at pai_add(), if a task is scheduled on a newly hotplugged CPU
where no memory was allocated, it seems mp->mapptr will be NULL:

arch/s390/kernel/perf_pai.c:pai_add() {
    ...
    struct pai_map *cpump = mp->mapptr;
    struct paiext_cb *pcb = cpump->paiext_cb;
    ...
}

Can this lead to a NULL pointer dereference when accessing cpump->paiext_cb?

Should the PAI PMU register for CPU hotplug states to ensure memory is
allocated when new CPUs come online?

>  		if (rc) {
>  			for_each_cpu(cpu, maskptr)
> -				pai_event_destroy_cpu(event, cpu);
> +				pai_event_destroy_cpu(idx, cpu);
>  			kfree(maskptr);
>  			goto out;
>  		}

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