Re: [PATCH v9 bpf-next 3/5] bpf: Add skb dynptrs

[Joanne Koong <joannelkoong@gmail.com>]

On Tue, Jan 31, 2023 at 9:54 AM Joanne Koong <joannelkoong@gmail.com> wrote:
>
> On Mon, Jan 30, 2023 at 9:36 PM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > On Mon, Jan 30, 2023 at 04:44:12PM -0800, Joanne Koong wrote:
> > > On Sun, Jan 29, 2023 at 3:39 PM Alexei Starovoitov
> > > <alexei.starovoitov@gmail.com> wrote:
> > > >
> > > > On Fri, Jan 27, 2023 at 11:17:01AM -0800, Joanne Koong wrote:
[...]
> > > > > diff --git a/net/core/filter.c b/net/core/filter.c
> > > > > index 6da78b3d381e..ddb47126071a 100644
> > > > > --- a/net/core/filter.c
> > > > > +++ b/net/core/filter.c
> > > > > @@ -1684,8 +1684,8 @@ static inline void bpf_pull_mac_rcsum(struct sk_buff *skb)
> > > > >               skb_postpull_rcsum(skb, skb_mac_header(skb), skb->mac_len);
> > > > >  }
> > > > >
> > > > > -BPF_CALL_5(bpf_skb_store_bytes, struct sk_buff *, skb, u32, offset,
> > > > > -        const void *, from, u32, len, u64, flags)
> > > > > +int __bpf_skb_store_bytes(struct sk_buff *skb, u32 offset, const void *from,
> > > > > +                       u32 len, u64 flags)
> > > >
> > > > This change is just to be able to call __bpf_skb_store_bytes() ?
> > > > If so, it's unnecessary.
> > > > See:
> > > > BPF_CALL_4(sk_reuseport_load_bytes,
> > > >            const struct sk_reuseport_kern *, reuse_kern, u32, offset,
> > > >            void *, to, u32, len)
> > > > {
> > > >         return ____bpf_skb_load_bytes(reuse_kern->skb, offset, to, len);
> > > > }
> > > >
> > >
> > > There was prior feedback [0] that using four underscores to call a
> > > helper function is confusing and makes it ungreppable
> >
> > There are plenty of ungreppable funcs in the kernel.
> > Try finding where folio_test_dirty() is defined.
> > mm subsystem is full of such 'features'.
> > Not friendly for casual kernel code reader, but useful.
> >
> > Since quadruple underscore is already used in the code base
> > I see no reason to sacrifice bpf_skb_load_bytes performance with extra call.
>
> I don't have a preference either way, I'll change it to use the
> quadruple underscore in the next version

I think we still need these extra __bpf_skb_store/load_bytes()
functions, because BPF_CALL_x static inlines the
bpf_skb_store/load_bytes helpers in net/core/filter.c, and we need to
call these bpf_skb_store/load_bytes helpers from another file
(kernel/bpf/helpers.c). I think the only other alternative is moving
the BPF_CALL_x declaration of bpf_skb_store/load bytes to
include/linux/filter.h, but I think having the extra
__bpf_skb_store/load_bytes() is cleaner.