netdev - Re: [PATCH net-next v2 1/2] bpf_xdp_redirect_map: Add flag to return XDP

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, 6 Jun 2019 08:56:30 -0700
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Daniel Borkmann <daniel@...earbox.net>
Cc:     Toke Høiland-Jørgensen <toke@...hat.com>,
        David Miller <davem@...emloft.net>,
        Network Development <netdev@...r.kernel.org>,
        Jesper Dangaard Brouer <brouer@...hat.com>,
        Alexei Starovoitov <ast@...nel.org>
Subject: Re: [PATCH net-next v2 1/2] bpf_xdp_redirect_map: Add flag to return
 XDP_PASS on map lookup failure

On Thu, Jun 6, 2019 at 8:51 AM Daniel Borkmann <daniel@...earbox.net> wrote:
>
> On 06/06/2019 03:24 PM, Toke Høiland-Jørgensen wrote:
> > From: Toke Høiland-Jørgensen <toke@...hat.com>
> >
> > The bpf_redirect_map() helper used by XDP programs doesn't return any
> > indication of whether it can successfully redirect to the map index it was
> > given. Instead, BPF programs have to track this themselves, leading to
> > programs using duplicate maps to track which entries are populated in the
> > devmap.
> >
> > This patch adds a flag to the XDP version of the bpf_redirect_map() helper,
> > which makes the helper do a lookup in the map when called, and return
> > XDP_PASS if there is no value at the provided index.
> >
> > With this, a BPF program can check the return code from the helper call and
> > react if it is XDP_PASS (by, for instance, substituting a different
> > redirect). This works for any type of map used for redirect.
> >
> > Signed-off-by: Toke Høiland-Jørgensen <toke@...hat.com>
> > ---
> >  include/uapi/linux/bpf.h |    8 ++++++++
> >  net/core/filter.c        |   10 +++++++++-
> >  2 files changed, 17 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index 7c6aef253173..d57df4f0b837 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -3098,6 +3098,14 @@ enum xdp_action {
> >       XDP_REDIRECT,
> >  };
> >
> > +/* Flags for bpf_xdp_redirect_map helper */
> > +
> > +/* If set, the help will check if the entry exists in the map and return
> > + * XDP_PASS if it doesn't.
> > + */
> > +#define XDP_REDIRECT_F_PASS_ON_INVALID BIT(0)
> > +#define XDP_REDIRECT_ALL_FLAGS XDP_REDIRECT_F_PASS_ON_INVALID
> > +
> >  /* user accessible metadata for XDP packet hook
> >   * new fields must be added to the end of this structure
> >   */
> > diff --git a/net/core/filter.c b/net/core/filter.c
> > index 55bfc941d17a..2e532a9b2605 100644
> > --- a/net/core/filter.c
> > +++ b/net/core/filter.c
> > @@ -3755,9 +3755,17 @@ BPF_CALL_3(bpf_xdp_redirect_map, struct bpf_map *, map, u32, ifindex,
> >  {
> >       struct bpf_redirect_info *ri = this_cpu_ptr(&bpf_redirect_info);
> >
> > -     if (unlikely(flags))
> > +     if (unlikely(flags & ~XDP_REDIRECT_ALL_FLAGS))
> >               return XDP_ABORTED;
> >
> > +     if (flags & XDP_REDIRECT_F_PASS_ON_INVALID) {
> > +             void *val;
> > +
> > +             val = __xdp_map_lookup_elem(map, ifindex);
> > +             if (unlikely(!val))
> > +                     return XDP_PASS;
>
> Generally looks good to me, also the second part with the flag. Given we store into
> the per-CPU scratch space and function like xdp_do_redirect() pick this up again, we
> could even propagate val onwards and save a second lookup on the /same/ element (which
> also avoids a race if the val was dropped from the map in the meantime). Given this
> should all still be within RCU it should work. Perhaps it even makes sense to do the
> lookup unconditionally inside bpf_xdp_redirect_map() helper iff we manage to do it
> only once anyway?

+1

also I don't think we really need a new flag here.
Yes, it could be considered an uapi change, but it
looks more like bugfix in uapi to me.
Since original behavior was so clunky to use.