Re: [PATCH 3/3 v2] s390/pai: Support CPU hotplug for PMU PAI

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Missing `cpus_read_lock()` during PAI event allocation and destruction causes race conditions with the new CPU hotplug callbacks, leading to NULL pointer dereferences or memory leaks.
--

--- Patch [3]: [PATCH 3/3 v2] s390/pai: Support CPU hotplug for PMU PAI ---
commit 91620cb7064d1e27822672c053b2886bba4751a9
Author: Thomas Richter <[email protected]>

s390/pai: Support CPU hotplug for PMU PAI

This commit adds CPU hotplug support for the PAI PMU, creating per-CPU data
structures during CPU add/remove to prevent panics when CPUs are hotplugged
during perf runs.

> diff --git a/arch/s390/kernel/perf_pai.c b/arch/s390/kernel/perf_pai.c
> index 52d9f654346a2..04a9dafa8f371 100644
> --- a/arch/s390/kernel/perf_pai.c
> +++ b/arch/s390/kernel/perf_pai.c

[ ... ]

> @@ -160,13 +178,11 @@ static void pai_event_destroy(struct perf_event *event)
>  	free_page(PAI_SAVE_AREA(event));
>  	mutex_lock(&pai_reserve_mutex);
>  	if (event->cpu == -1) {
> -		struct cpumask *mask = PAI_CPU_MASK(event);
> -
> -		for_each_cpu(cpu, mask)
> -			pai_event_destroy_cpu(idx, cpu);
> -		kfree(mask);
> +		atomic_dec(&pai_root[idx].tskctx);
> +		for_each_online_cpu(cpu)
> +			pai_event_destroy_cpu(idx, cpu, false);

[Severity: High]
Does this code introduce a memory leak if a CPU is offlined concurrently?

If a CPU is offlined, it is removed from cpu_online_mask at CPUHP_BRINGUP_CPU.
If a concurrent perf event destruction runs, for_each_online_cpu() will skip
the offlining CPU, failing to decrement its refcnt.

When the offline process reaches pai_offline_cpu(), it reads tskctx as 0, so
refcount_sub_and_test(0, ...) does not drop the reference, leaving the memory
permanently leaked.

Should cpus_read_lock() be held during pai_event_destroy() to prevent this?

>  	} else {
> -		pai_event_destroy_cpu(idx, event->cpu);
> +		pai_event_destroy_cpu(idx, event->cpu, false);
>  	}
>  	mutex_unlock(&pai_reserve_mutex);
>  }

[ ... ]

> @@ -322,24 +362,20 @@ static int pai_alloc(struct perf_event *event)
>  		goto out;
>  
>  	for_each_online_cpu(cpu) {
> -		rc = pai_alloc_cpu(idx, cpu);
> +		rc = pai_alloc_cpu(idx, cpu, false);
>  		if (rc) {
>  			for_each_cpu(cpu, maskptr)
> -				pai_event_destroy_cpu(idx, cpu);
> -			kfree(maskptr);
> -			goto out;
> +				pai_event_destroy_cpu(idx, cpu, false);
> +			goto undo;
>  		}
>  		cpumask_set_cpu(cpu, maskptr);
>  	}
>  
> -	/*
> -	 * On error all cpumask are freed and all events have been destroyed.
> -	 * Save of which CPUs data structures have been allocated for.
> -	 * Release them in pai_event_destroy call back function
> -	 * for this event.
> -	 */
> -	PAI_CPU_MASK(event) = maskptr;
>  	rc = 0;
> +	/* Trace per-task events for CPU hotplug. */
> +	atomic_inc(&pai_root[idx].tskctx);

[Severity: High]
Can this race with CPU hotplug and lead to a NULL pointer dereference?

If CPU hotplug begins (e.g., via sysfs), pai_online_cpu() executes at
CPUHP_BP_PREPARE_DYN. Since no per-task events exist (tskctx == 0), it skips
allocation.

If a concurrent system-wide perf event is created here, pai_alloc() iterates
cpu_online_mask. Since the new CPU is not yet in the online mask (which
happens later at CPUHP_BRINGUP_CPU), it skips allocation for the new CPU, and
then increments tskctx to 1.

When the CPU completes bring-up and a task schedules on it, will pai_add()
dereference the unallocated per-CPU pointer and cause a kernel panic?

> +undo:
> +	kfree(maskptr);
>  out:
>  	return rc;
>  }

[ ... ]

> @@ -389,7 +425,7 @@ static int pai_event_init(struct perf_event *event, int idx)
>  
>  	mutex_lock(&pai_reserve_mutex);
>  	if (event->cpu >= 0)
> -		rc = pai_alloc_cpu(idx, event->cpu);
> +		rc = pai_alloc_cpu(idx, event->cpu, false);
>  	else
>  		rc = pai_alloc(event);
>  	mutex_unlock(&pai_reserve_mutex);

[Severity: High]
Does pai_event_init() need to hold cpus_read_lock() here?

Acquiring cpus_read_lock() before taking pai_reserve_mutex (to avoid ABBA
deadlocks with the hotplug writer lock) would serialize event allocation with
CPU hotplug operations, preventing the races mentioned above.

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