On Thu, Nov 14, 2024 at 9:07 AM Ryan Wilson <ryantimwilson@xxxxxxxxx> wrote:
>
> Currently, network devices only support a single XDP program. However,
> there are use cases for multiple XDP programs per device. For example,
> at Meta, we have XDP programs for firewalls, DDOS and logging that must
> all run in a specific order. To work around the lack of multi-program
> support, a single daemon loads all programs and uses bpf_tail_call()
> in a loop to jump to each program contained in a BPF map.
>
> This patch proposes allowing multiple XDP programs per device based
> on the mprog API from the below commit:
>
> Commit 053c8e1f235d ("bpf: Add generic attach/detach/query API for
> multi-progs")
>
> This patch adds support for multi-progs for generic XDP and the tunnel
> driver, as it shares many APIs with generic XDP. Each driver can be
> migrated by:
> 1. Return 0 for command = XDP_QUERY_MPROG_SUPPORT
> 2. Codemod xdp.prog -> xdp.array in driver code
> 3. Run programs with bpf_mprog_run_xdp()
> 4. Change bpf_prog_put() to bpf_mprog_array_put()
>
> Note non-migrated driver are currently backwards compatible. They will
> receive a single program object but attachment will fail if user tries
> to attach multiple BPF programs.
>
> Note this patch is more complex than its TCX counterpart
>
> Commit e420bed02507 ("bpf: Add fd-based tcx multi-prog infra with
> link support")
>
> This is because XDP program attachment/detachment is done via BPF links and
> netlink socket unlike bpf_prog_attach() syscall. We only add multi-progs
> support for BPF links in this commit but ensure the netlink socket is
> backwards compatible with single program attachment/detachment/queries.
>
> Unlike TCX, each driver needs to own a reference to the BPF programs it
> runs. Thus, we introduce struct bpf_mprog_array to copy from
> bpf_mprog_entry owned by the network device. The BPF driver then will
> release the array via bpf_mprog_array_put() which will decrement each
> BPF program refcount and free the array memory.
>
> Furthermore, we typically set the BPF multi-prog flags via
> link_create.flags e.g. BPF_F_BEFORE. However, there are already XDP_*
> flags set via link_create.flags e.g. XDP_FLAGS_SKB_MODE. For example,
> BPF_F_REPLACE and XDP_FLAGS_DRV_MODE both have the same value. Thus to
> allow for setting both XDP_* mode and BPF_* multi-prog flags when using
> BPF links, we introduce link_create.xdp.flags for setting BPF_* flags
> specifically. However, feedback is needed on this approach to make sure
> its compatible with libbpf.
>
> Note I am in the process of verifying no/minimal performance overhead
> for a real driver but I am sending this patch for feedback on the overall
> approach.
>
> Signed-off-by: Ryan Wilson <ryantimwilson@xxxxxxxxx>
> ---
So I looked through this once, but it's a bit too much code to grok at
once, so please bear with me if I'm proposing something stupid. But
just a few initial thoughts.
First, you bundled everything into a single patch, which makes it
harder to review. At the very least, split out selftests. If you can
break it further, say into mprog_array abstraction patch, then generic
XDP infrastructure patch, then tun-specific patch, etc, that would
make it much easier to follow and review.
Looking at tun.c, I like how straightforward the conversion is thanks
to bpf_mprog_array, nice job with that! Converting the other 50 use
cases should be easy ;)
There seems to be dev_xdp_attach_netlink() duplication, is it
absolutely necessary or can we keep a common dev_xdp_attach()
implementation between BPF link and netlink implementations?
But overall everything seems to make sense, but again, a bit much
code, so I'll need few more passes (maybe next week).
> drivers/net/tun.c | 54 +--
> include/linux/bpf_mprog.h | 72 +++
> include/linux/netdevice.h | 23 +-
> include/linux/skbuff.h | 3 +-
> include/net/xdp.h | 94 ++++
> include/uapi/linux/bpf.h | 12 +
> net/core/dev.c | 422 ++++++++++++++----
> net/core/rtnetlink.c | 16 +-
> net/core/skbuff.c | 11 +-
> .../selftests/bpf/prog_tests/xdp_link.c | 15 +-
> 10 files changed, 571 insertions(+), 151 deletions(-)
[...]
