netdev - Re: [bpf-next, v3 1/5] flow_dissector: implements flow dissector BPF hook

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:   Thu, 13 Sep 2018 16:51:49 -0400
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc:     Petar Penkov <peterpenkov96@...il.com>,
        Network Development <netdev@...r.kernel.org>,
        David Miller <davem@...emloft.net>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        simon.horman@...ronome.com, ecree@...arflare.com,
        songliubraving@...com, Tom Herbert <tom@...bertland.com>,
        Petar Penkov <ppenkov@...gle.com>,
        Willem de Bruijn <willemb@...gle.com>
Subject: Re: [bpf-next, v3 1/5] flow_dissector: implements flow dissector BPF hook

On Thu, Sep 13, 2018 at 3:45 PM Alexei Starovoitov
<alexei.starovoitov@...il.com> wrote:
>
> On Thu, Sep 13, 2018 at 10:45:53AM -0700, Petar Penkov wrote:
> > From: Petar Penkov <ppenkov@...gle.com>
> >
> > Adds a hook for programs of type BPF_PROG_TYPE_FLOW_DISSECTOR and
> > attach type BPF_FLOW_DISSECTOR that is executed in the flow dissector
> > path. The BPF program is per-network namespace.
> >
> > Signed-off-by: Petar Penkov <ppenkov@...gle.com>
> > Signed-off-by: Willem de Bruijn <willemb@...gle.com>
> ...
> > @@ -2333,6 +2335,7 @@ struct __sk_buff {
> >       /* ... here. */
> >
> >       __u32 data_meta;
> > +     struct bpf_flow_keys *flow_keys;
>
> the bpf prog form patch 4 looks much better now. Thanks!
>
> >  };
> >
> >  struct bpf_tunnel_key {
> > @@ -2778,4 +2781,27 @@ enum bpf_task_fd_type {
> >       BPF_FD_TYPE_URETPROBE,          /* filename + offset */
> >  };
> >
> > +struct bpf_flow_keys {
> > +     __u16   nhoff;
> > +     __u16   thoff;
> > +     __u16   addr_proto;                     /* ETH_P_* of valid addrs */
> > +     __u8    is_frag;
> > +     __u8    is_first_frag;
> > +     __u8    is_encap;
> > +     __be16  n_proto;
> > +     __u8    ip_proto;
> > +     union {
> > +             struct {
> > +                     __be32  ipv4_src;
> > +                     __be32  ipv4_dst;
> > +             };
> > +             struct {
> > +                     __u32   ipv6_src[4];    /* in6_addr; network order */
> > +                     __u32   ipv6_dst[4];    /* in6_addr; network order */
> > +             };
> > +     };
> > +     __be16  sport;
> > +     __be16  dport;
> > +};
>
> can you please pack it?
> struct bpf_flow_keys {
>         __u16                      nhoff;                /*     0     2 */
>         __u16                      thoff;                /*     2     2 */
>         __u16                      addr_proto;           /*     4     2 */
>         __u8                       is_frag;              /*     6     1 */
>         __u8                       is_first_frag;        /*     7     1 */
>         __u8                       is_encap;             /*     8     1 */
>
>         /* XXX 1 byte hole, try to pack */
>
>         __be16                     n_proto;              /*    10     2 */
>         __u8                       ip_proto;             /*    12     1 */
>
>         /* XXX 3 bytes hole, try to pack */
>
>         union {
>
> also is_frag and other fields are not used by the kernel and
> only used by the prog to pass data between tail_calls ?

No, these are mapped directly onto fields in struct flow_keys
on return from the BPF program in __skb_flow_bpf_to_target.
For is_frag, for instance:

       if (flow_keys->is_frag)
               key_control->flags |= FLOW_DIS_IS_FRAGMENT;

This is true for all fields in the struct except nhoff.

> In such case reserve some space in bpf_flow_keys similar to skb->cb
> so it can contain any fields and accommodate for inevitable changes
> to bpf flow dissector prog in the future.

Do you mean a second scratch space akin to cb[], just a few
reserved padding bytes?

We have given some thought to forward compatibility. The existing
fields cannot be changed, but it should be fine if we need to expand
the struct later.