Re: [PATCH bpf v2 1/2] bpf: Zero-fill other CPUs when BPF_F_CPU creates a per-cpu hash element

[Leon Hwang <leon.hwang@linux.dev>]

On 23/9/26 08:08, Donggeun Yoo wrote:
> pcpu_init_value() initializes the per-cpu area of a newly created
> [lru_]percpu_hash element.  The area is recycled, so when the value
> comes from a BPF program (onallcpus == false) it writes the running
> CPU's slot and zeroes the rest.
> 
> bpf_percpu_hash_update() passes onallcpus == true, which delegates to
> pcpu_copy_value().  Since BPF_F_CPU was added that writes only the CPU
> named in map_flags when the flag is set, so on the create path the other
> slots keep the recycled element's values:
> 
>   update(k1, 0xdeadc0de, BPF_F_ALL_CPUS)  every CPU holds 0xdeadc0de
>   delete(k1)                              element back on the freelist
>   update(k2, 0xc0ffee, BPF_F_CPU | 0)     creates, writes CPU 0 only
>   lookup(k2)                              CPU 0 0xc0ffee, rest 0xdeadc0de
> 
> Zero-fill the other CPUs on that arm too.
> 
> Fixes: c6936161fd55 ("bpf: Add BPF_F_CPU and BPF_F_ALL_CPUS flags support for percpu_hash and lru_percpu_hash maps")
> Signed-off-by: Donggeun Yoo <donggeunyoo.kernel@gmail.com>
> ---
> The merged arm no longer calls bpf_obj_cancel_fields() on the named
> CPU.  That call is inert here: it acts only on BPF_TIMER,
> BPF_WORKQUEUE and BPF_TASK_WORK, which map_check_btf() rejects for
> [lru_]percpu_hash.
> 
> v2: name the BPF_F_CPU entry condition in the block comment.
> 
>  kernel/bpf/hashtab.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
> index 4f495dcbf670c..f7a9c432d9b76 100644
> --- a/kernel/bpf/hashtab.c
> +++ b/kernel/bpf/hashtab.c
> @@ -1054,14 +1054,16 @@ static void pcpu_init_value(struct bpf_htab *htab, void __percpu *pptr,
>  	/* When not setting the initial value on all cpus, zero-fill element
>  	 * values for other cpus. Otherwise, bpf program has no way to ensure
>  	 * known initial values for cpus other than current one
> -	 * (onallcpus=false always when coming from bpf prog).
> +	 * (onallcpus=false always when coming from bpf prog,
> +	 *  map_flags & BPF_F_CPU when coming from syscall but setting
> +	 *  only one cpu).
>  	 */
> -	if (!onallcpus) {
> -		int current_cpu = raw_smp_processor_id();
> +	if (!onallcpus || (map_flags & BPF_F_CPU)) {
> +		int init_cpu = onallcpus ? map_flags >> 32 : raw_smp_processor_id();

'onallcpus ? map_flags >> 32' does not look reasonable.

Based on the 'if' above, either of these looks straightforward to me.

int init_cpu = !onallcpus ? raw_smp_processor_id() : map_flags >> 32;

or

int init_cpu = (map_flags & BPF_F_CPU) ? map_flags >> 32 :
raw_smp_processor_id();

Thanks,
Leon

>  		int cpu;
>  
>  		for_each_possible_cpu(cpu) {
> -			if (cpu == current_cpu)
> +			if (cpu == init_cpu)
>  				copy_map_value(&htab->map, per_cpu_ptr(pptr, cpu), value);
>  			else /* Since elem is preallocated, we cannot touch special fields */
>  				zero_map_value(&htab->map, per_cpu_ptr(pptr, cpu));