Re: [PATCH bpf-next 8/9] libbpf: add opt-in strict BPF program section name handling logic

From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
To: Stanislav Fomichev <sdf@google.com>
Cc: Andrii Nakryiko <andrii@kernel.org>, bpf <bpf@vger.kernel.org>,
	Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Kernel Team <kernel-team@fb.com>
Subject: Re: [PATCH bpf-next 8/9] libbpf: add opt-in strict BPF program section name handling logic
Date: Fri, 17 Sep 2021 16:21:14 -0700	[thread overview]
Message-ID: <CAEf4BzYg3Tdv3KjmwNYu=81ig=KeLOGvqA+zH_nC_VmJ3M6hjg@mail.gmail.com> (raw)
In-Reply-To: <YUUgj5kR1XA48Z3n@google.com>

On Fri, Sep 17, 2021 at 4:11 PM <sdf@google.com> wrote:
>
> On 09/17, Andrii Nakryiko wrote:
> > On Fri, Sep 17, 2021 at 10:26 AM <sdf@google.com> wrote:
> > >
> > > On 09/16, Andrii Nakryiko wrote:
> > > > Implement strict ELF section name handling for BPF programs. It
> > utilizes
> > > > `libbpf_set_strict_mode()` framework and adds new flag:
> > > > LIBBPF_STRICT_SEC_NAME.
> > >
> > > > If this flag is set, libbpf will enforce exact section name matching
> > for
> > > > a lot of program types that previously allowed just partial prefix
> > > > match. E.g., if previously SEC("xdp_whatever_i_want") was allowed, now
> > > > in strict mode only SEC("xdp") will be accepted, which makes SEC("")
> > > > definitions cleaner and more structured. SEC() now won't be used as
> > yet
> > > > another way to uniquely encode BPF program identifier (for that
> > > > C function name is better and is guaranteed to be unique within
> > > > bpf_object). Now SEC() is strictly BPF program type and, depending on
> > > > program type, extra load/attach parameter specification.
> > >
> > > > Libbpf completely supports multiple BPF programs in the same ELF
> > > > section, so multiple BPF programs of the same type/specification
> > easily
> > > > co-exist together within the same bpf_object scope.
> > >
> > > > Additionally, a new (for now internal) convention is introduced:
> > section
> > > > name that can be a stand-alone exact BPF program type specificator,
> > but
> > > > also could have extra parameters after '/' delimiter. An example of
> > such
> > > > section is "struct_ops", which can be specified by itself, but also
> > > > allows to specify the intended operation to be attached to, e.g.,
> > > > "struct_ops/dctcp_init". Note, that "struct_ops_some_op" is not
> > allowed.
> > > > Such section definition is specified as "struct_ops+".
> > >
> > > > This change is part of libbpf 1.0 effort ([0], [1]).
> > >
> > > >    [0] Closes: https://github.com/libbpf/libbpf/issues/271
> > > >    [1]
> > > >
> > https://github.com/libbpf/libbpf/wiki/Libbpf:-the-road-to-v1.0#stricter-and-more-uniform-bpf-program-section-name-sec-handling
> > >
> > > > Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> > > > ---
> > > >   tools/lib/bpf/libbpf.c        | 135
> > ++++++++++++++++++++++------------
> > > >   tools/lib/bpf/libbpf_legacy.h |   9 +++
> > > >   2 files changed, 98 insertions(+), 46 deletions(-)
> > >

[...]

> > > > +     /*
> > > > +      * Enforce strict BPF program section (SEC()) names.
> > > > +      * E.g., while prefiously SEC("xdp_whatever") or
> > SEC("perf_event_blah")
> > > > were
> > > > +      * allowed, with LIBBPF_STRICT_SEC_PREFIX this will become
> > > > +      * unrecognized by libbpf and would have to be just SEC("xdp")
> > and
> > > > +      * SEC("xdp") and SEC("perf_event").
> > > > +      */
> > > > +     LIBBPF_STRICT_SEC_NAME = 0x04,
> > >
> > > To clarify: I'm assuming, as discussed, we'll still support that old,
> > > non-conforming naming in libbpf 1.0, right? It just won't be enabled
> > > by default.
>
> > No, we won't. All those opt-in strict flags will be turned on
> > permanently in libbpf 1.0. But I'm adding an ability to provide custom
> > callbacks to handle whatever (reasonable) BPF program section names.
> > So if someone has a real important case needing custom handling, it's
> > not a big problem to implement that logic on their own. If someone is
> > just resisting making their code conforming, well... Stay on the old
> > fixed version, write a callback, or just do the mechanical rename, how
> > hard can that be? We are dropping bpf_program__find_program_by_title()
> > in libbpf 1.0, that API is meaningless with multiple programs per
> > section, so you'd have to update your logic to either skeleton or
> > bpf_program__find_program_by_name() anyways.
>
> I see. I was assuming some of them would stay, iirc Toke also was asking
> for this one to stay (or was it the old maps format?). FTR, I'm not
> resisting any changes, I'm willing to invest some time to update our
> callers, just trying to understand what my options are. We do have some
> cases where we depend on the section names, so maybe I should just
> switch from bpf_program__title to bpf_program__name (and do appropriate
> renaming).

Switching to name over title (section name) is a good idea for sure.

>
> RE skeleton: I'm not too eager to adopt it, I'll wait for version 2 :-)

Honest curiosity, what's wrong with the current version of skeleton?
Can you please expand on this?

>
>
> > >
> > > Btw, forgot to update you, I've enabled LIBBPF_STRICT_DIRECT_ERRS and
> > > LIBBPF_STRICT_CLEAN_PTRS and everything seems to be working fine 🤞
>
> > Great! The problem is that you would see the difference only when
> > actual runtime failure happens. So I'd still recommend auditing the
> > code, if possible.