netdev - Re: [PATCH bpf-next v5 0/3] xdp: Allow lookup into devmaps before redirect

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: <CAEf4BzZPb2YsrvOfshJAboY9KE3dCa_FZsTkxvQyPquzDChz+w@mail.gmail.com>
Date:   Mon, 24 Jun 2019 13:49:01 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Toke Høiland-Jørgensen <toke@...hat.com>
Cc:     Networking <netdev@...r.kernel.org>,
        Jesper Dangaard Brouer <brouer@...hat.com>,
        Daniel Borkmann <daniel@...earbox.net>,
        Alexei Starovoitov <ast@...nel.org>,
        David Miller <davem@...emloft.net>,
        Jonathan Lemon <jonathan.lemon@...il.com>
Subject: Re: [PATCH bpf-next v5 0/3] xdp: Allow lookup into devmaps before redirect

On Mon, Jun 24, 2019 at 12:38 PM Toke Høiland-Jørgensen <toke@...hat.com> wrote:
>
> Andrii Nakryiko <andrii.nakryiko@...il.com> writes:
>
> > On Sat, Jun 22, 2019 at 7:19 PM Toke Høiland-Jørgensen <toke@...hat.com> wrote:
> >>
> >> When using the bpf_redirect_map() helper to redirect packets from XDP, the eBPF
> >> program cannot currently know whether the redirect will succeed, which makes it
> >> impossible to gracefully handle errors. To properly fix this will probably
> >> require deeper changes to the way TX resources are allocated, but one thing that
> >> is fairly straight forward to fix is to allow lookups into devmaps, so programs
> >> can at least know when a redirect is *guaranteed* to fail because there is no
> >> entry in the map. Currently, programs work around this by keeping a shadow map
> >> of another type which indicates whether a map index is valid.
> >>
> >> This series contains two changes that are complementary ways to fix this issue:
> >>
> >> - Moving the map lookup into the bpf_redirect_map() helper (and caching the
> >>   result), so the helper can return an error if no value is found in the map.
> >>   This includes a refactoring of the devmap and cpumap code to not care about
> >>   the index on enqueue.
> >>
> >> - Allowing regular lookups into devmaps from eBPF programs, using the read-only
> >>   flag to make sure they don't change the values.
> >>
> >> The performance impact of the series is negligible, in the sense that I cannot
> >> measure it because the variance between test runs is higher than the difference
> >> pre/post series.
> >>
> >> Changelog:
> >>
> >> v5:
> >>   - Rebase on latest bpf-next.
> >>   - Update documentation for bpf_redirect_map() with the new meaning of flags.
> >>
> >> v4:
> >>   - Fix a few nits from Andrii
> >>   - Lose the #defines in bpf.h and just compare the flags argument directly to
> >>     XDP_TX in bpf_xdp_redirect_map().
> >>
> >> v3:
> >>   - Adopt Jonathan's idea of using the lower two bits of the flag value as the
> >>     return code.
> >>   - Always do the lookup, and cache the result for use in xdp_do_redirect(); to
> >>     achieve this, refactor the devmap and cpumap code to get rid the bitmap for
> >>     selecting which devices to flush.
> >>
> >> v2:
> >>   - For patch 1, make it clear that the change works for any map type.
> >>   - For patch 2, just use the new BPF_F_RDONLY_PROG flag to make the return
> >>     value read-only.
> >>
> >> ---
> >>
> >> Toke Høiland-Jørgensen (3):
> >>       devmap/cpumap: Use flush list instead of bitmap
> >>       bpf_xdp_redirect_map: Perform map lookup in eBPF helper
> >>       devmap: Allow map lookups from eBPF
> >>
> >>
> >>  include/linux/filter.h   |    1
> >>  include/uapi/linux/bpf.h |    7 ++-
> >>  kernel/bpf/cpumap.c      |  106 ++++++++++++++++++++-----------------------
> >>  kernel/bpf/devmap.c      |  113 ++++++++++++++++++++++------------------------
> >>  kernel/bpf/verifier.c    |    7 +--
> >>  net/core/filter.c        |   29 +++++-------
> >>  6 files changed, 123 insertions(+), 140 deletions(-)
> >>
> >
> >
> > Looks like you forgot to add my Acked-by's for your patches?
>
> Ah yes, did not carry those forward for the individual patches, my
> apologies. Could you perhaps be persuaded to send a new one (I believe a
> response to the cover letter acking the whole series would suffice)?
> I'll make sure to add the carrying forward of acks into my workflow in
> the future :)

I don't think patchworks captures ack's from cover letter, but let's
give it a go:

Acked-by: Andrii Nakryiko <andriin@...com>

>
> -Toke