Message-ID: <CAEf4Bzad70FY2mWxjQm4-Vx7=T3_S5VA5YdYbDxb1BmG2z6F-A@mail.gmail.com>
Date:   Tue, 27 Jul 2021 12:05:53 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     David Ahern <dsahern@...il.com>
Cc:     Hangbin Liu <haliu@...hat.com>, Martynas Pumputis <m@...bda.lt>,
        Networking <netdev@...r.kernel.org>,
        Stephen Hemminger <stephen@...workplumber.org>,
        Daniel Borkmann <daniel@...earbox.net>
Subject: Re: [PATCH iproute2] libbpf: fix attach of prog with multiple sections

On Mon, Jul 26, 2021 at 7:51 PM David Ahern <dsahern@...il.com> wrote:
>
> On 7/26/21 9:13 AM, Andrii Nakryiko wrote:
> > On Mon, Jul 26, 2021 at 6:58 AM David Ahern <dsahern@...il.com> wrote:
> >>
> >> On 7/23/21 6:25 PM, Andrii Nakryiko wrote:
> >>>>>>>> This is still problematic, because one section can have multiple BPF
> >>>>>>>> programs. I.e., it's possible two define two or more XDP BPF programs
> >>>>>>>> all with SEC("xdp") and libbpf works just fine with that. I suggest
> >>>>>>>> moving users to specify the program name (i.e., C function name
> >>>>>>>> representing the BPF program). All the xdp_mycustom_suffix namings are
> >>>>>>>> a hack and will be rejected by libbpf 1.0, so it would be great to get
> >>>>>>>> a head start on fixing this early on.
> >>>>>>>
> >>>>>>> Thanks for bringing this up. Currently, there is no way to specify a
> >>>>>>> function name with "tc exec bpf" (only a section name via the "sec" arg). So
> >>>>>>> probably, we should just add another arg to specify the function name.
> >>>>>>
> >>>>>> How about add a "prog" arg to load specified program name and mark
> >>>>>> "sec" as not recommended? To keep backwards compatibility we just load the
> >>>>>> first program in the section.
> >>>>>
> >>>>> Why not error out if there is more than one program with the same
> >>>>> section name? if there is just one (and thus section name is still
> >>>>> unique) -- then proceed. It seems much less confusing, IMO.
> >>>>>
> >>>>
> >>>> Let' see if I understand this correctly: libbpf 1.0 is not going to
> >>>> allow SEC("xdp_foo") or SEC("xdp_bar") kind of section names - which is
> >>>> the hint for libbpf to know program type. Instead only SEC("xdp") is
> >>>> allowed.
> >>>
> >>> Right.
> >>>
> >>>>
> >>>> Further, a single object file is not going to be allowed to have
> >>>> multiple SEC("xdp") instances for each program name.
> >>>
> >>> On the contrary. Libbpf already allows (and will keep allowing)
> >>> multiple BPF programs with SEC("xdp") in a single object file. Which
> >>> is why section_name is not a unique program identifier.
> >>>
> >>
> >> Does that require BTF? My attempts at loading an object file with 2
> >> SEC("xdp") programs failed. This is using bpftool from top of tree and
> >> loadall.
> >
> > You mean kernel BTF? Not if XDP programs themselves were built
> > requiring CO-RE. So if those programs use #include "vmlinux.h", or
> > there is BPF_CORE_READ() use somewhere in the code, or explicit
> > __attribute__((preserve_access_index)) is used on some of the used
> > structs, then yes, vmlinux BTF will be needed. But otherwise no. Do
> > you have verbose error logs? I think with bpftool you can get them
> > with -d argument.
> >
>
> xdp_l3fwd is built using an old school compile line - no CO-RE or BTF,
> just a basic compile line extracted from samples/bpf 2-3 years ago.
> Works fine for what I need and take this nothing more than an example to
> verify your comment
>
> "Libbpf already allows (and will keep allowing) multiple BPF programs
> with SEC("xdp") in a single object file."
>
>
> The bpftool command line to load the programs is:
>
> $ bpftool -ddd prog loadall xdp_l3fwd.o /sys/fs/bpf
>
> It fails because libbpf is trying to put 2 programs at the same path:
>
> libbpf: pinned program '/sys/fs/bpf/xdp'
> libbpf: failed to pin program: File exists
> libbpf: unpinned program '/sys/fs/bpf/xdp'
> Error: failed to pin all programs

Ok, I see, that's the problem with pinning path using section name as
an identifier (same wrong assumption made a long time ago). We have a
task to fix that ([0]) and use program name instead of section name
for this, but it's a backwards incompatible change, so users will have
to opt-in.

And we should fix bpftool as well, of course, though I never used
bpftool to load programs so I wasn't even aware it's doing pinning
like that.


  [0] https://github.com/libbpf/libbpf/issues/273

>
> The code that works is this:
>
> SEC("xdp_l3fwd")
> int xdp_l3fwd_prog(struct xdp_md *ctx)
> {
>         return xdp_l3fwd_flags(ctx, 0);
> }
>
> SEC("xdp_l3fwd_direct")
> int xdp_l3fwd_direct_prog(struct xdp_md *ctx)
> {
>         return xdp_l3fwd_flags(ctx, BPF_FIB_LOOKUP_DIRECT);
> }
>
> The code that fails is this:
>
> SEC("xdp")
> int xdp_l3fwd_prog(struct xdp_md *ctx)
> {
>         return xdp_l3fwd_flags(ctx, 0);
> }
>
> SEC("xdp")
> int xdp_l3fwd_direct_prog(struct xdp_md *ctx)
> {
>         return xdp_l3fwd_flags(ctx, BPF_FIB_LOOKUP_DIRECT);
> }
>
> which is what you said should work -- 2 programs with the same section name.
>

And I didn't lie, see progs/test_check_mtu.c as an example, it has 6
XDP programs with SEC("xdp"). The problem is in the pinning, which is
in general an area that was pretty ad-hoc and not very well thought
out in libbpf, growing organically. This hopefully will be addressed
and improved before libbpf 1.0 is finalized.

Right now users can override the default pin path by setting it
explicitly with bpf_program__set_pin_path(), which might be a good
idea to do for this new "prog" approach that Hangbin proposed.

> From a very quick check of bpftool vs libbpf, the former is calling
> bpf_object__pin_programs from the latter and passing the base path
> (/sys/fs/bpf in this example) and then bpf_object__pin_programs adds the
> pin_name for the prog - which must be the same for both programs since
> the second one fails.
>

Powered by blists - more mailing lists