On Thu Aug 28, 2025 at 7:18 AM +08, Andrii Nakryiko wrote:
> On Wed, Aug 27, 2025 at 9:45 AM Leon Hwang <leon.hwang@xxxxxxxxx> wrote:
>>
[...]
>>
>> +#ifdef CONFIG_BPF_SYSCALL
>> +static inline void bpf_percpu_copy_to_user(struct bpf_map *map, void __percpu *pptr, void *value,
>> + u32 size, u64 flags)
>> +{
>> + int current_cpu = raw_smp_processor_id();
>> + int cpu, off = 0;
>> +
>> + if (flags & BPF_F_CPU) {
>> + cpu = flags >> 32;
>> + copy_map_value_long(map, value, cpu != current_cpu ? per_cpu_ptr(pptr, cpu) :
>> + this_cpu_ptr(pptr));
>> + check_and_init_map_value(map, value);
>
> I'm not sure it's the question to you, but why would we
> "check_and_init_map_value" when copying data to user space?... this is
> so confusing...
>
After reading its code, I think it's to hide some kernel details from
user space, e.g. refcount, list nodes, rb nodes.
>> + } else {
>> + for_each_possible_cpu(cpu) {
>> + copy_map_value_long(map, value + off, per_cpu_ptr(pptr, cpu));
>> + check_and_init_map_value(map, value + off);
>> + off += size;
>> + }
>> + }
>> +}
>> +
>> +void bpf_obj_free_fields(const struct btf_record *rec, void *obj);
>> +
>> +static inline void bpf_percpu_copy_from_user(struct bpf_map *map, void __percpu *pptr, void *value,
>> + u32 size, u64 flags)
>> +{
[...]
>> +}
>> +#endif
>
> hm... these helpers are just here with no way to validate that they
> generalize existing logic correctly... Do a separate patch where you
> introduce this helper before adding per-CPU flags *and* make use of
> them in existing code? Then we can check that you didn't introduce any
> subtle differences? Then in this patch you can adjust helpers to
> handle BPF_F_CPU and BPF_F_ALL_CPUS?
>
Get it.
I'll send a separate patch later.
Thanks,
Leon
