On Wed, Sep 3, 2025 at 7:27 AM Leon Hwang <leon.hwang@xxxxxxxxx> wrote:
>
> 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.
we don't copy those details, so there is nothing to hide, so no, I
think it's just weird that we do this, unless there is some
non-obvious reasoning behind this
>
> >> + } 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.
separate patch as part of the patch set to show the value of this refactoring :)
>
> Thanks,
> Leon
