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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 8 Feb 2023 15:22:50 -0800
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Joanne Koong <joannelkoong@...il.com>
Cc:     bpf <bpf@...r.kernel.org>, Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        Martin KaFai Lau <martin.lau@...nel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Network Development <netdev@...r.kernel.org>,
        Kumar Kartikeya Dwivedi <memxor@...il.com>,
        Kernel Team <kernel-team@...com>
Subject: Re: [PATCH v9 bpf-next 3/5] bpf: Add skb dynptrs

On Wed, Feb 8, 2023 at 1:47 PM Joanne Koong <joannelkoong@...il.com> wrote:
>
> On Tue, Jan 31, 2023 at 9:54 AM Joanne Koong <joannelkoong@...il.com> wrote:
> >
> > On Mon, Jan 30, 2023 at 9:36 PM Alexei Starovoitov
> > <alexei.starovoitov@...il.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@...il.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.

bpf_skb_load_bytes() is a performance critical function.
I'm worried about the cost of the extra call.
Will compiler be smart enough to inline __bpf_skb_load_bytes()
in both cases? Probably not if they're in different .c files.
Not sure how to solve it. Make it a static inline in skbuff.h ?