On Tue, Sep 21, 2021 at 6:53 PM Dave Marchevsky <davemarchevsky@xxxxxx> wrote:
>
> On 9/20/21 7:43 PM, 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@xxxxxxxxxx>
> > ---
> > tools/lib/bpf/libbpf.c | 135 ++++++++++++++++++++++------------
> > tools/lib/bpf/libbpf_legacy.h | 9 +++
> > 2 files changed, 98 insertions(+), 46 deletions(-)
> >
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index 56082865ceff..f0846f609e26 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -232,6 +232,7 @@ enum sec_def_flags {
> > SEC_ATTACHABLE_OPT = SEC_ATTACHABLE | SEC_EXP_ATTACH_OPT,
> > SEC_ATTACH_BTF = 4,
> > SEC_SLEEPABLE = 8,
> > + SEC_SLOPPY_PFX = 16, /* allow non-strict prefix matching */
> > };
> >
> > struct bpf_sec_def {
> > @@ -7976,15 +7977,15 @@ static struct bpf_link *attach_lsm(const struct bpf_program *prog, long cookie);
> > static struct bpf_link *attach_iter(const struct bpf_program *prog, long cookie);
> >
> > static const struct bpf_sec_def section_defs[] = {
> > - SEC_DEF("socket", SOCKET_FILTER, 0, SEC_NONE),
> > - SEC_DEF("sk_reuseport/migrate", SK_REUSEPORT, BPF_SK_REUSEPORT_SELECT_OR_MIGRATE, SEC_ATTACHABLE),
> > - SEC_DEF("sk_reuseport", SK_REUSEPORT, BPF_SK_REUSEPORT_SELECT, SEC_ATTACHABLE),
> > + SEC_DEF("socket", SOCKET_FILTER, 0, SEC_SLOPPY_PFX),
> > + SEC_DEF("sk_reuseport/migrate", SK_REUSEPORT, BPF_SK_REUSEPORT_SELECT_OR_MIGRATE, SEC_ATTACHABLE | SEC_SLOPPY_PFX),
> > + SEC_DEF("sk_reuseport", SK_REUSEPORT, BPF_SK_REUSEPORT_SELECT, SEC_ATTACHABLE | SEC_SLOPPY_PFX),
> > SEC_DEF("kprobe/", KPROBE, 0, SEC_NONE, attach_kprobe),
> > SEC_DEF("uprobe/", KPROBE, 0, SEC_NONE),
> > SEC_DEF("kretprobe/", KPROBE, 0, SEC_NONE, attach_kprobe),
> > SEC_DEF("uretprobe/", KPROBE, 0, SEC_NONE),
> > - SEC_DEF("classifier", SCHED_CLS, 0, SEC_NONE),
> > - SEC_DEF("action", SCHED_ACT, 0, SEC_NONE),
> > + SEC_DEF("classifier", SCHED_CLS, 0, SEC_SLOPPY_PFX),
>
> Feels like the mass SEC_NONE -> SEC_SLOPPY_PFX migration is obscuring some
> useful at-a-glance info. The equivalent SEC_NONE | SEC_SLOPPY_PFX would make
> reasoning about attach behavior easier IMO.
Sure, I can leave "SEC_NONE | SEC_SLOPPY_PFX" everywhere where there
are no other flags
>
> > + SEC_DEF("action", SCHED_ACT, 0, SEC_SLOPPY_PFX),
> > SEC_DEF("tracepoint/", TRACEPOINT, 0, SEC_NONE, attach_tp),
> > SEC_DEF("tp/", TRACEPOINT, 0, SEC_NONE, attach_tp),
>
> [...]
