Re: [PATCH 3/3] 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()` in `pai_alloc()` and `pai_event_destroy()` allows race conditions with CPU hotplug, leading to NULL pointer dereferences and memory leaks.
--

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

s390/pai: Support CPU hotplug for PMU PAI
    
This commit adds CPU hotplug support for the PAI PMU to prevent crashes
when CPUs are hotplugged during perf runs. It dynamically creates and
deletes per-CPU data structures during CPU offline and online states.

> 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]
Since this iterates over the online CPU mask and modifies tskctx here
without holding cpus_read_lock(), could this race with CPU hotplug teardown?

If pai_offline_cpu() executes concurrently with pai_event_destroy():

pai_offline_cpu() runs after the CPU is removed from cpu_online_mask.

pai_event_destroy() would skip the CPU since it is not in the mask,
missing the refcount decrement, but it still decrements the global tskctx.

When pai_offline_cpu() later subtracts this newly decremented tskctx from
the CPU's refcount, will it leave the CPU's refcount permanently above zero
and leak the associated memory?

>  	} 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]
Could this for_each_online_cpu() iteration and tskctx increment also race
with CPU bring-up since it doesn't hold cpus_read_lock()?

The CPU hotplug callbacks are registered at CPUHP_BP_PREPARE_DYN, which
runs on the boot processor before the target CPU is added to
cpu_online_mask.

If a CPU is coming online, pai_online_cpu() could observe tskctx == 0
and allocate nothing.

If pai_alloc() runs concurrently, it skips the new CPU because it isn't
in cpu_online_mask yet, but then increments tskctx.

When the new CPU fully comes online, it will have no cpump structure
allocated. Does this result in a NULL pointer dereference in pai_add()
when a task is scheduled on it?

Would wrapping the iterations in pai_alloc() and pai_event_destroy()
with cpus_read_lock() resolve this? (It looks like it would need to be
taken before pai_reserve_mutex to prevent AB-BA deadlocks).

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

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