Re: [PATCH bpf-next v4 5/8] bpf: Update per-CPU maps using BPF_F_ALL_CPUS flag

On 15/4/26 05:02, [email protected] wrote:
> Sashiko AI review found 1 potential issue(s):
[...]
>> +	/*
>> +	 * When updating per-CPU maps via the lightweight skeleton
>> +	 * loader, use a single value slot across all CPUs. This avoids
>> +	 * two potential issues when updating on an M-CPU kernel with
>> +	 * N cached slots (N < M), especially when N is much smaller
>> +	 * than M:
>> +	 * 1) The update may trigger a page fault when copying data from
>> +	 *    the last slot, as the read may go beyond the allocated
>> +	 *    buffer.
>> +	 * 2) The update may copy unexpected data from slots [N, M-1].
>> +	 */
>> +	if (bpfptr_is_kernel(uattr) && bpf_map_supports_cpu_flags(map->map_type) &&
>> +	    !(attr->flags & (BPF_F_CPU | BPF_F_ALL_CPUS)))
>> +		attr->flags |= BPF_F_ALL_CPUS;
> 
> Does this unintentionally break the bpf_sys_bpf() helper API for
> BPF_PROG_TYPE_SYSCALL programs?
> 
> Because bpf_sys_bpf() also wraps the pointer with KERNEL_BPFPTR,
> bpfptr_is_kernel(uattr) will evaluate to true for any eBPF program calling it.
> 
> Prior to this patch, a BPF_PROG_TYPE_SYSCALL program could legitimately
> update a per-CPU map with distinct values for each CPU by providing an array
> of num_possible_cpus() elements and omitting any CPU flags, which mirrors
> standard user-space API semantics.
> 
> With this change, it appears the kernel silently overrides attr->flags by
> setting BPF_F_ALL_CPUS. This truncates the internal value_size calculation
> down to a single element (map->value_size), reads only the first element
> of the BPF program's provided buffer, and incorrectly replicates it across
> all CPUs.
> 
> Should this workaround for the skeleton loader be constrained to
> bpftool gen skeleton instead of mutating the core syscall handler?
> 

It was my intent. Even though BPF_PROG_TYPE_SYSCALL progs could be
broken, it should be fixed when verifying the oob issue is true for
SYSCALL progs.

This patch will be dropped from this series. And, a separate patch will
be posted if the oob issue is true.

Thanks,
Leon