Re: [PATCH bpf-next 8/9] libbpf: add opt-in strict BPF program section name handling logic
From: <hidden>
Date: 2021-09-17 23:11:16
On 09/17, Andrii Nakryiko wrote:
On Fri, Sep 17, 2021 at 10:26 AM [off-list ref] wrote:quoted
On 09/16, Andrii Nakryiko wrote:quoted
Implement strict ELF section name handling for BPF programs. Itutilizesquoted
quoted
`libbpf_set_strict_mode()` framework and adds new flag: LIBBPF_STRICT_SEC_NAME.quoted
If this flag is set, libbpf will enforce exact section name matchingforquoted
quoted
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 asyetquoted
quoted
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.quoted
Libbpf completely supports multiple BPF programs in the same ELF section, so multiple BPF programs of the same type/specificationeasilyquoted
quoted
co-exist together within the same bpf_object scope.quoted
Additionally, a new (for now internal) convention is introduced:sectionquoted
quoted
name that can be a stand-alone exact BPF program type specificator,butquoted
quoted
also could have extra parameters after '/' delimiter. An example ofsuchquoted
quoted
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 notallowed.quoted
quoted
Such section definition is specified as "struct_ops+".quoted
This change is part of libbpf 1.0 effort ([0], [1]).quoted
[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-handlingquoted
quoted
Signed-off-by: Andrii Nakryiko <andrii@kernel.org> --- tools/lib/bpf/libbpf.c | 135++++++++++++++++++++++------------quoted
quoted
tools/lib/bpf/libbpf_legacy.h | 9 +++ 2 files changed, 98 insertions(+), 46 deletions(-)quoted
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 */ };quoted
struct bpf_sec_def {@@ -7976,15 +7977,15 @@ static struct bpf_link *attach_lsm(conststructquoted
quoted
bpf_program *prog, long cookie); static struct bpf_link *attach_iter(const struct bpf_program *prog,longquoted
quoted
cookie);quoted
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,quoted
quoted
SEC_ATTACHABLE), + SEC_DEF("socket", SOCKET_FILTER, 0,SEC_SLOPPY_PFX),quoted
quoted
+ 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,quoted
quoted
SEC_ATTACHABLE | SEC_SLOPPY_PFX), SEC_DEF("kprobe/", KPROBE, 0, SEC_NONE,attach_kprobe),quoted
quoted
SEC_DEF("uprobe/", KPROBE, 0, SEC_NONE), SEC_DEF("kretprobe/", KPROBE, 0, SEC_NONE,attach_kprobe),quoted
quoted
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), + SEC_DEF("action", SCHED_ACT, 0, SEC_SLOPPY_PFX), SEC_DEF("tracepoint/", TRACEPOINT, 0, SEC_NONE,attach_tp),quoted
quoted
SEC_DEF("tp/", TRACEPOINT, 0, SEC_NONE,attach_tp),quoted
quoted
SEC_DEF("raw_tracepoint/", RAW_TRACEPOINT, 0, SEC_NONE,attach_raw_tp),quoted
quoted
@@ -8003,44 +8004,44 @@ static const struct bpf_sec_defsection_defs[] = {quoted
quoted
SEC_DEF("syscall", SYSCALL, 0, SEC_SLEEPABLE), SEC_DEF("xdp_devmap/", XDP, BPF_XDP_DEVMAP,SEC_ATTACHABLE),quoted
quoted
SEC_DEF("xdp_cpumap/", XDP, BPF_XDP_CPUMAP,SEC_ATTACHABLE),quoted
quoted
- SEC_DEF("xdp", XDP, BPF_XDP,SEC_ATTACHABLE_OPT),quoted
quoted
- SEC_DEF("perf_event", PERF_EVENT, 0, SEC_NONE), - SEC_DEF("lwt_in", LWT_IN, 0, SEC_NONE), - SEC_DEF("lwt_out", LWT_OUT, 0, SEC_NONE), - SEC_DEF("lwt_xmit", LWT_XMIT, 0, SEC_NONE), - SEC_DEF("lwt_seg6local", LWT_SEG6LOCAL, 0, SEC_NONE), - SEC_DEF("cgroup_skb/ingress", CGROUP_SKB,BPF_CGROUP_INET_INGRESS,quoted
quoted
SEC_ATTACHABLE_OPT), - SEC_DEF("cgroup_skb/egress", CGROUP_SKB,BPF_CGROUP_INET_EGRESS,quoted
quoted
SEC_ATTACHABLE_OPT), - SEC_DEF("cgroup/skb", CGROUP_SKB, 0, SEC_NONE), - SEC_DEF("cgroup/sock_create", CGROUP_SOCK,BPF_CGROUP_INET_SOCK_CREATE,quoted
quoted
SEC_ATTACHABLE), - SEC_DEF("cgroup/sock_release", CGROUP_SOCK, BPF_CGROUP_INET_SOCK_RELEASE, SEC_ATTACHABLE), - SEC_DEF("cgroup/sock", CGROUP_SOCK,BPF_CGROUP_INET_SOCK_CREATE,quoted
quoted
SEC_ATTACHABLE_OPT), - SEC_DEF("cgroup/post_bind4", CGROUP_SOCK,BPF_CGROUP_INET4_POST_BIND,quoted
quoted
SEC_ATTACHABLE), - SEC_DEF("cgroup/post_bind6", CGROUP_SOCK,BPF_CGROUP_INET6_POST_BIND,quoted
quoted
SEC_ATTACHABLE), - SEC_DEF("cgroup/dev", CGROUP_DEVICE,BPF_CGROUP_DEVICE,quoted
quoted
SEC_ATTACHABLE_OPT), - SEC_DEF("sockops", SOCK_OPS, BPF_CGROUP_SOCK_OPS,SEC_ATTACHABLE_OPT),quoted
quoted
- SEC_DEF("sk_skb/stream_parser", SK_SKB,BPF_SK_SKB_STREAM_PARSER,quoted
quoted
SEC_ATTACHABLE_OPT), - SEC_DEF("sk_skb/stream_verdict",SK_SKB,BPF_SK_SKB_STREAM_VERDICT,quoted
quoted
SEC_ATTACHABLE_OPT), - SEC_DEF("sk_skb", SK_SKB, 0, SEC_NONE), - SEC_DEF("sk_msg", SK_MSG, BPF_SK_MSG_VERDICT,SEC_ATTACHABLE_OPT),quoted
quoted
- SEC_DEF("lirc_mode2", LIRC_MODE2, BPF_LIRC_MODE2,SEC_ATTACHABLE_OPT),quoted
quoted
- SEC_DEF("flow_dissector", FLOW_DISSECTOR,BPF_FLOW_DISSECTOR,quoted
quoted
SEC_ATTACHABLE_OPT), - SEC_DEF("cgroup/bind4", CGROUP_SOCK_ADDR,BPF_CGROUP_INET4_BIND,quoted
quoted
SEC_ATTACHABLE), - SEC_DEF("cgroup/bind6", CGROUP_SOCK_ADDR,BPF_CGROUP_INET6_BIND,quoted
quoted
SEC_ATTACHABLE), - SEC_DEF("cgroup/connect4", CGROUP_SOCK_ADDR,BPF_CGROUP_INET4_CONNECT,quoted
quoted
SEC_ATTACHABLE), - SEC_DEF("cgroup/connect6", CGROUP_SOCK_ADDR,BPF_CGROUP_INET6_CONNECT,quoted
quoted
SEC_ATTACHABLE), - SEC_DEF("cgroup/sendmsg4", CGROUP_SOCK_ADDR,BPF_CGROUP_UDP4_SENDMSG,quoted
quoted
SEC_ATTACHABLE), - SEC_DEF("cgroup/sendmsg6", CGROUP_SOCK_ADDR,BPF_CGROUP_UDP6_SENDMSG,quoted
quoted
SEC_ATTACHABLE), - SEC_DEF("cgroup/recvmsg4", CGROUP_SOCK_ADDR,BPF_CGROUP_UDP4_RECVMSG,quoted
quoted
SEC_ATTACHABLE), - SEC_DEF("cgroup/recvmsg6", CGROUP_SOCK_ADDR,BPF_CGROUP_UDP6_RECVMSG,quoted
quoted
SEC_ATTACHABLE), - SEC_DEF("cgroup/getpeername4", CGROUP_SOCK_ADDR, BPF_CGROUP_INET4_GETPEERNAME, SEC_ATTACHABLE), - SEC_DEF("cgroup/getpeername6", CGROUP_SOCK_ADDR, BPF_CGROUP_INET6_GETPEERNAME, SEC_ATTACHABLE), - SEC_DEF("cgroup/getsockname4", CGROUP_SOCK_ADDR, BPF_CGROUP_INET4_GETSOCKNAME, SEC_ATTACHABLE), - SEC_DEF("cgroup/getsockname6", CGROUP_SOCK_ADDR, BPF_CGROUP_INET6_GETSOCKNAME, SEC_ATTACHABLE), - SEC_DEF("cgroup/sysctl", CGROUP_SYSCTL,BPF_CGROUP_SYSCTL,quoted
quoted
SEC_ATTACHABLE), - SEC_DEF("cgroup/getsockopt", CGROUP_SOCKOPT,BPF_CGROUP_GETSOCKOPT,quoted
quoted
SEC_ATTACHABLE), - SEC_DEF("cgroup/setsockopt", CGROUP_SOCKOPT,BPF_CGROUP_SETSOCKOPT,quoted
quoted
SEC_ATTACHABLE), - SEC_DEF("struct_ops", STRUCT_OPS, 0, SEC_NONE), + SEC_DEF("xdp", XDP, BPF_XDP,SEC_ATTACHABLE_OPT | SEC_SLOPPY_PFX),quoted
quoted
+ SEC_DEF("perf_event", PERF_EVENT, 0, SEC_SLOPPY_PFX), + SEC_DEF("lwt_in", LWT_IN, 0, SEC_SLOPPY_PFX), + SEC_DEF("lwt_out", LWT_OUT, 0, SEC_SLOPPY_PFX), + SEC_DEF("lwt_xmit", LWT_XMIT, 0, SEC_SLOPPY_PFX), + SEC_DEF("lwt_seg6local", LWT_SEG6LOCAL, 0,SEC_SLOPPY_PFX),quoted
quoted
+ SEC_DEF("cgroup_skb/ingress", CGROUP_SKB,BPF_CGROUP_INET_INGRESS,quoted
quoted
SEC_ATTACHABLE_OPT | SEC_SLOPPY_PFX), + SEC_DEF("cgroup_skb/egress", CGROUP_SKB,BPF_CGROUP_INET_EGRESS,quoted
quoted
SEC_ATTACHABLE_OPT | SEC_SLOPPY_PFX), + SEC_DEF("cgroup/skb", CGROUP_SKB, 0, SEC_SLOPPY_PFX), + SEC_DEF("cgroup/sock_create", CGROUP_SOCK,BPF_CGROUP_INET_SOCK_CREATE,quoted
quoted
SEC_ATTACHABLE | SEC_SLOPPY_PFX), + SEC_DEF("cgroup/sock_release", CGROUP_SOCK, BPF_CGROUP_INET_SOCK_RELEASE, SEC_ATTACHABLE | SEC_SLOPPY_PFX), + SEC_DEF("cgroup/sock", CGROUP_SOCK,BPF_CGROUP_INET_SOCK_CREATE,quoted
quoted
SEC_ATTACHABLE_OPT | SEC_SLOPPY_PFX), + SEC_DEF("cgroup/post_bind4", CGROUP_SOCK,BPF_CGROUP_INET4_POST_BIND,quoted
quoted
SEC_ATTACHABLE | SEC_SLOPPY_PFX), + SEC_DEF("cgroup/post_bind6", CGROUP_SOCK,BPF_CGROUP_INET6_POST_BIND,quoted
quoted
SEC_ATTACHABLE | SEC_SLOPPY_PFX), + SEC_DEF("cgroup/dev", CGROUP_DEVICE,BPF_CGROUP_DEVICE,quoted
quoted
SEC_ATTACHABLE_OPT | SEC_SLOPPY_PFX), + SEC_DEF("sockops", SOCK_OPS, BPF_CGROUP_SOCK_OPS,SEC_ATTACHABLE_OPT |quoted
quoted
SEC_SLOPPY_PFX), + SEC_DEF("sk_skb/stream_parser", SK_SKB,BPF_SK_SKB_STREAM_PARSER,quoted
quoted
SEC_ATTACHABLE_OPT | SEC_SLOPPY_PFX), + SEC_DEF("sk_skb/stream_verdict",SK_SKB,BPF_SK_SKB_STREAM_VERDICT,quoted
quoted
SEC_ATTACHABLE_OPT | SEC_SLOPPY_PFX), + SEC_DEF("sk_skb", SK_SKB, 0, SEC_SLOPPY_PFX), + SEC_DEF("sk_msg", SK_MSG, BPF_SK_MSG_VERDICT,SEC_ATTACHABLE_OPT |quoted
quoted
SEC_SLOPPY_PFX), + SEC_DEF("lirc_mode2", LIRC_MODE2, BPF_LIRC_MODE2,SEC_ATTACHABLE_OPT |quoted
quoted
SEC_SLOPPY_PFX), + SEC_DEF("flow_dissector", FLOW_DISSECTOR,BPF_FLOW_DISSECTOR,quoted
quoted
SEC_ATTACHABLE_OPT | SEC_SLOPPY_PFX), + SEC_DEF("cgroup/bind4", CGROUP_SOCK_ADDR,BPF_CGROUP_INET4_BIND,quoted
quoted
SEC_ATTACHABLE | SEC_SLOPPY_PFX), + SEC_DEF("cgroup/bind6", CGROUP_SOCK_ADDR,BPF_CGROUP_INET6_BIND,quoted
quoted
SEC_ATTACHABLE | SEC_SLOPPY_PFX), + SEC_DEF("cgroup/connect4", CGROUP_SOCK_ADDR,BPF_CGROUP_INET4_CONNECT,quoted
quoted
SEC_ATTACHABLE | SEC_SLOPPY_PFX), + SEC_DEF("cgroup/connect6", CGROUP_SOCK_ADDR,BPF_CGROUP_INET6_CONNECT,quoted
quoted
SEC_ATTACHABLE | SEC_SLOPPY_PFX), + SEC_DEF("cgroup/sendmsg4", CGROUP_SOCK_ADDR,BPF_CGROUP_UDP4_SENDMSG,quoted
quoted
SEC_ATTACHABLE | SEC_SLOPPY_PFX), + SEC_DEF("cgroup/sendmsg6", CGROUP_SOCK_ADDR,BPF_CGROUP_UDP6_SENDMSG,quoted
quoted
SEC_ATTACHABLE | SEC_SLOPPY_PFX), + SEC_DEF("cgroup/recvmsg4", CGROUP_SOCK_ADDR,BPF_CGROUP_UDP4_RECVMSG,quoted
quoted
SEC_ATTACHABLE | SEC_SLOPPY_PFX), + SEC_DEF("cgroup/recvmsg6", CGROUP_SOCK_ADDR,BPF_CGROUP_UDP6_RECVMSG,quoted
quoted
SEC_ATTACHABLE | SEC_SLOPPY_PFX), + SEC_DEF("cgroup/getpeername4", CGROUP_SOCK_ADDR, BPF_CGROUP_INET4_GETPEERNAME, SEC_ATTACHABLE | SEC_SLOPPY_PFX), + SEC_DEF("cgroup/getpeername6", CGROUP_SOCK_ADDR, BPF_CGROUP_INET6_GETPEERNAME, SEC_ATTACHABLE | SEC_SLOPPY_PFX), + SEC_DEF("cgroup/getsockname4", CGROUP_SOCK_ADDR, BPF_CGROUP_INET4_GETSOCKNAME, SEC_ATTACHABLE | SEC_SLOPPY_PFX), + SEC_DEF("cgroup/getsockname6", CGROUP_SOCK_ADDR, BPF_CGROUP_INET6_GETSOCKNAME, SEC_ATTACHABLE | SEC_SLOPPY_PFX), + SEC_DEF("cgroup/sysctl", CGROUP_SYSCTL,BPF_CGROUP_SYSCTL,quoted
quoted
SEC_ATTACHABLE | SEC_SLOPPY_PFX), + SEC_DEF("cgroup/getsockopt", CGROUP_SOCKOPT,BPF_CGROUP_GETSOCKOPT,quoted
quoted
SEC_ATTACHABLE | SEC_SLOPPY_PFX), + SEC_DEF("cgroup/setsockopt", CGROUP_SOCKOPT,BPF_CGROUP_SETSOCKOPT,quoted
quoted
SEC_ATTACHABLE | SEC_SLOPPY_PFX), + SEC_DEF("struct_ops+", STRUCT_OPS, 0, SEC_NONE), SEC_DEF("sk_lookup/", SK_LOOKUP, BPF_SK_LOOKUP,SEC_ATTACHABLE),quoted
quoted
};quoted
@@ -8048,11 +8049,53 @@ static const struct bpf_sec_defsection_defs[] = {quoted
quoted
static const struct bpf_sec_def *find_sec_def(const char *sec_name) { - int i, n = ARRAY_SIZE(section_defs); + const struct bpf_sec_def *sec_def; + enum sec_def_flags sec_flags; + int i, n = ARRAY_SIZE(section_defs), len; + bool strict = libbpf_mode & LIBBPF_STRICT_SEC_NAME;quoted
for (i = 0; i < n; i++) { - if (str_has_pfx(sec_name, section_defs[i].sec)) - return §ion_defs[i]; + sec_def = §ion_defs[i]; + sec_flags = sec_def->cookie; + len = strlen(sec_def->sec); + + /* "type/" always has to have proper SEC("type/extras")form */quoted
quoted
+ if (sec_def->sec[len - 1] == '/') { + if (str_has_pfx(sec_name, sec_def->sec)) + return sec_def; + continue; + } + + /* "type+" means it can be either exact SEC("type") or + * well-formed SEC("type/extras") with proper '/'separatorquoted
quoted
+ */ + if (sec_def->sec[len - 1] == '+') { + len--; + /* not even a prefix */ + if (strncmp(sec_name, sec_def->sec, len) != 0) + continue; + /* exact match or has '/' separator */ + if (sec_name[len] == '\0' || sec_name[len]== '/')quoted
quoted
+ return sec_def; + continue; + } + + /* SEC_SLOPPY_PFX definitions are allowed to be justprefixquoted
quoted
+ * matches, unless strict section name mode + * (LIBBPF_STRICT_SEC_NAME) is enabled, in which casethequoted
quoted
+ * match has to be exact. + */ + if ((sec_flags & SEC_SLOPPY_PFX) && !strict) { + if (str_has_pfx(sec_name, sec_def->sec)) + return sec_def; + continue; + } + + /* Definitions not marked SEC_SLOPPY_PFX (e.g., + * SEC("syscall")) are exact matches in both modes. + */ + if (strcmp(sec_name, sec_def->sec) == 0) + return sec_def; } return NULL; }diff --git a/tools/lib/bpf/libbpf_legacy.hb/tools/lib/bpf/libbpf_legacy.hquoted
quoted
index df0d03dcffab..74e6f860f703 100644--- a/tools/lib/bpf/libbpf_legacy.h +++ b/tools/lib/bpf/libbpf_legacy.h@@ -46,6 +46,15 @@ enum libbpf_strict_mode { */ LIBBPF_STRICT_DIRECT_ERRS = 0x02,quoted
+ /* + * Enforce strict BPF program section (SEC()) names. + * E.g., while prefiously SEC("xdp_whatever") orSEC("perf_event_blah")quoted
quoted
were + * allowed, with LIBBPF_STRICT_SEC_PREFIX this will become + * unrecognized by libbpf and would have to be just SEC("xdp")andquoted
quoted
+ * 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). RE skeleton: I'm not too eager to adopt it, I'll wait for version 2 :-)
quoted
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.