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

From: Dave Marchevsky <davemarchevsky@fb.com>
To: Andrii Nakryiko <andrii@kernel.org>, <bpf@vger.kernel.org>,
	<ast@kernel.org>, <daniel@iogearbox.net>
Cc: <kernel-team@fb.com>
Subject: Re: [PATCH v2 bpf-next 8/9] libbpf: add opt-in strict BPF program section name handling logic
Date: Tue, 21 Sep 2021 21:53:05 -0400	[thread overview]
Message-ID: <1f3336f0-c789-71ab-1974-b280ce28da06@fb.com> (raw)
In-Reply-To: <20210920234320.3312820-9-andrii@kernel.org>

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@kernel.org>
> ---
>  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.

> +	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),

[...]