Message-ID: <56d5758b-fa9f-c727-1219-5e1318e34ce6@iogearbox.net>
Date:   Sat, 29 Jun 2019 01:33:05 +0200
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Toke Høiland-Jørgensen <toke@...hat.com>,
        netdev@...r.kernel.org
Cc:     Jesper Dangaard Brouer <brouer@...hat.com>,
        Alexei Starovoitov <ast@...nel.org>,
        David Miller <davem@...emloft.net>,
        Jonathan Lemon <jonathan.lemon@...il.com>
Subject: Re: [PATCH bpf-next v6 0/5] xdp: Allow lookup into devmaps before
 redirect

On 06/28/2019 11:12 AM, Toke Høiland-Jørgensen 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:
> 
> v6:
>   - Factor out list handling in maps to a helper in list.h (new patch 1)
>   - Rename variables in struct bpf_redirect_info (new patch 3 + patch 4)
>   - Explain why we are clearing out the map in the info struct on lookup failure
>   - Remove unneeded check for forwarding target in tracepoint macro
> 
> 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 (5):
>       xskmap: Move non-standard list manipulation to helper
>       devmap/cpumap: Use flush list instead of bitmap
>       devmap: Rename ifindex member in bpf_redirect_info
>       bpf_xdp_redirect_map: Perform map lookup in eBPF helper
>       devmap: Allow map lookups from eBPF
> 
> 
>  include/linux/filter.h     |    3 +
>  include/linux/list.h       |   14 ++++++
>  include/trace/events/xdp.h |    5 +-
>  include/uapi/linux/bpf.h   |    7 ++-
>  kernel/bpf/cpumap.c        |  105 +++++++++++++++++++----------------------
>  kernel/bpf/devmap.c        |  112 ++++++++++++++++++++------------------------
>  kernel/bpf/verifier.c      |    7 +--
>  kernel/bpf/xskmap.c        |    3 -
>  net/core/filter.c          |   60 ++++++++++++------------
>  9 files changed, 157 insertions(+), 159 deletions(-)
> 

Applied, thanks!

Powered by blists - more mailing lists