netdev - Re: [RFC bpf-next 4/7] net: flow_dissector: handle no-skb use case

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]

Message-ID: <20190205204531.GE10769@mini-arch>
Date:   Tue, 5 Feb 2019 12:45:31 -0800
From:   Stanislav Fomichev <sdf@...ichev.me>
To:     Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc:     Stanislav Fomichev <sdf@...gle.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, Willem de Bruijn <willemb@...gle.com>
Subject: Re: [RFC bpf-next 4/7] net: flow_dissector: handle no-skb use case

On 02/05, Willem de Bruijn wrote:
> On Tue, Feb 5, 2019 at 12:57 PM Stanislav Fomichev <sdf@...gle.com> wrote:
> >
> > When flow_dissector is called without skb (with only data and hlen),
> > construct on-stack skb (which has a linear chunk of data passed
> > to the flow dissector). This should let us handle eth_get_headlen
> > case where only data is provided and we don't want to (yet) allocate
> > an skb.
> >
> > Since this on-stack skb doesn't allocate its own data, we can't
> > add shinfo and need to be careful to avoid any code paths that use
> > it. Flow dissector BPF programs can only call bpf_skb_load_bytes helper,
> > which doesn't touch shinfo in our case (skb->len is the length of the
> > linear header so it exits early).
> >
> > Signed-off-by: Stanislav Fomichev <sdf@...gle.com>
> > ---
> >  include/linux/skbuff.h    |  5 +++
> >  net/core/flow_dissector.c | 95 +++++++++++++++++++++++++++++----------
> >  2 files changed, 76 insertions(+), 24 deletions(-)
> >
> > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> > index aa9a9983de80..5f1c085cb34c 100644
> > --- a/include/linux/skbuff.h
> > +++ b/include/linux/skbuff.h
> > @@ -1227,6 +1227,11 @@ bool __skb_flow_bpf_dissect(struct bpf_prog *prog,
> >                             const struct sk_buff *skb,
> >                             struct flow_dissector *flow_dissector,
> >                             struct bpf_flow_keys *flow_keys);
> > +bool __flow_bpf_dissect(struct bpf_prog *prog,
> > +                       void *data, __be16 proto,
> > +                       int nhoff, int hlen,
> > +                       struct flow_dissector *flow_dissector,
> > +                       struct bpf_flow_keys *flow_keys);
> 
> nit: please use more descriptive name. Perhaps bpf_flow_dissect_raw
> and rename __skb_flow_bpf_dissect to bpf_flow_dissect_skb.
Agreed.

> > +bool __flow_bpf_dissect(struct bpf_prog *prog,
> > +                       void *data, __be16 proto,
> > +                       int nhoff, int hlen,
> > +                       struct flow_dissector *flow_dissector,
> > +                       struct bpf_flow_keys *flow_keys)
> > +{
> > +       struct bpf_skb_data_end *cb;
> > +       struct sk_buff skb;
> > +       u32 result;
> > +
> > +       __init_skb(&skb, data, hlen);
> > +       skb_put(&skb, hlen);
> > +       skb.protocol = proto;
> > +
> > +       init_flow_keys(flow_keys, &skb, nhoff);
> > +
> > +       cb = (struct bpf_skb_data_end *)skb.cb;
> > +       cb->data_meta = skb.data;
> > +       cb->data_end  = skb.data + skb_headlen(&skb);
> > +
> > +       result = BPF_PROG_RUN(prog, &skb);
> > +
> > +       clamp_flow_keys(flow_keys, hlen);
> >
> >         return result == BPF_OK;
> >  }
> 
> Can__flow_bpf_dissect just construct an skb and then call
> __skb_flow_bpf_dissect?
__skb_flow_bpf_dissect calls bpf_compute_data_pointers which calls
skb_metadata_len which touches shinfo. And I don't think I have a
clever way to handle that.

> 
> It will unnecessarily save and restore the control block, but that is
> a relatively small cost (compared to, say, zeroing the entire skb).