On Fri, Nov 15, 2024 at 2:09 PM Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> wrote:
>
> 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.
Good point. I'll split this up into multiple patches.
Note I really didn't add new selftests, I only changed an existing
selftest that expected multiple links attaching to an XDP device to
fail - that now works. I'll add a more complete selftest to the v2
patch set that makes use of all the relevant BPF_F_* multiprog flags.
>
> 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?
So I ended up splitting these because I thought it was harder to read,
especially now that netlink doesn't support mprog and link does. :) I
tried to combine a lot of the logic in verify_dev_xdp_attach but maybe
that wasn't enough. I'll try recombining dev_xdp_attach() again and
see how it looks.
>
> 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(-)
>
> [...]
