All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 bpf-next 0/9] libbpf: stricter BPF program section name handling
@ 2021-09-20 23:43 Andrii Nakryiko
  2021-09-20 23:43 ` [PATCH v2 bpf-next 1/9] selftests/bpf: normalize XDP section names in selftests Andrii Nakryiko
                   ` (8 more replies)
  0 siblings, 9 replies; 27+ messages in thread
From: Andrii Nakryiko @ 2021-09-20 23:43 UTC (permalink / raw)
  To: bpf, ast, daniel; +Cc: andrii, kernel-team

Implement opt-in stricter BPF program section name (SEC()) handling logic. For
a lot of supported ELF section names, enforce exact section name match with no
arbitrary characters added at the end. See patch #8 for more details.

To allow this, first three patches clean up and preventively fix selftests,
normalizing existing SEC() usage across multiple selftests. While at it those
patches also reduce the amount of remaining bpf_object__find_program_by_title()
uses, which should be completely removed soon, given it's an API with
ambiguous semantics and will be deprecated and eventually removed in libbpf 1.0.

Last patch is also fixing "sk_lookup/" definition to not require and not allow
extra "/blah" parts after it, which serve no meaning.

All the other patches are gradual internal libbpf changes to:
  - allow this optional strict logic for ELF section name handling;
  - allow new use case (for now for "struct_ops", but that could be extended
    to, say, freplace definitions), in which it can be used stand-alone to
    specify just type (SEC("struct_ops")), or also accept extra parameters
    which can be utilized by libbpf to either get more data or double-check
    valid use (e.g., SEC("struct_ops/dctcp_init") to specify desired
    struct_ops operation that is supposed to be implemented);
  - get libbpf's internal logic ready to allow other libraries and
    applications to specify their custom handlers for ELF section name for BPF
    programs. All the pieces are in place, the only thing preventing making
    this as public libbpf API is reliance on internal type for specifying BPF
    program load attributes. The work is planned to revamp related low-level
    libbpf APIs, at which point it will be possible to just re-use such new
    types for coordination between libbpf and custom handlers.

These changes are a part of libbpf 1.0 effort ([0]). They are also intended to
be applied on top of the previous preparatory series [1], so currently CI will
be failing to apply them to bpf-next until that patch set is landed. Once it
is landed, kernel-patches daemon will automatically retest this patch set.

  [0] https://github.com/libbpf/libbpf/wiki/Libbpf:-the-road-to-v1.0#stricter-and-more-uniform-bpf-program-section-name-sec-handling
  [1] https://patchwork.kernel.org/project/netdevbpf/list/?series=547675&state=*

v1->v2:
  - rebase onto latest bpf-next and resolve merge conflicts w/ Dave's changes.

Andrii Nakryiko (9):
  selftests/bpf: normalize XDP section names in selftests
  selftests/bpf: normalize SEC("classifier") usage
  selftests/bpf: normalize all the rest SEC() uses
  libbpf: refactor internal sec_def handling to enable pluggability
  libbpf: reduce reliance of attach_fns on sec_def internals
  libbpf: refactor ELF section handler definitions
  libbpf: complete SEC() table unification for
    BPF_APROG_SEC/BPF_EAPROG_SEC
  libbpf: add opt-in strict BPF program section name handling logic
  selftests/bpf: switch sk_lookup selftests to strict SEC("sk_lookup")
    use

 tools/lib/bpf/libbpf.c                        | 506 +++++++++---------
 tools/lib/bpf/libbpf_internal.h               |   7 +
 tools/lib/bpf/libbpf_legacy.h                 |   9 +
 .../selftests/bpf/prog_tests/flow_dissector.c |   4 +-
 .../bpf/prog_tests/reference_tracking.c       |  22 +-
 .../selftests/bpf/prog_tests/sk_assign.c      |   2 +-
 .../selftests/bpf/prog_tests/sockopt_multi.c  |  30 +-
 .../selftests/bpf/prog_tests/tailcalls.c      |  58 +-
 tools/testing/selftests/bpf/progs/bpf_flow.c  |   3 +-
 .../bpf/progs/cg_storage_multi_isolated.c     |   4 +-
 .../bpf/progs/cg_storage_multi_shared.c       |   4 +-
 .../testing/selftests/bpf/progs/skb_pkt_end.c |   2 +-
 .../selftests/bpf/progs/sockopt_multi.c       |   5 +-
 tools/testing/selftests/bpf/progs/tailcall1.c |   5 +-
 tools/testing/selftests/bpf/progs/tailcall2.c |  21 +-
 tools/testing/selftests/bpf/progs/tailcall3.c |   5 +-
 tools/testing/selftests/bpf/progs/tailcall4.c |   5 +-
 tools/testing/selftests/bpf/progs/tailcall5.c |   5 +-
 tools/testing/selftests/bpf/progs/tailcall6.c |   4 +-
 .../selftests/bpf/progs/tailcall_bpf2bpf1.c   |   5 +-
 .../selftests/bpf/progs/tailcall_bpf2bpf2.c   |   5 +-
 .../selftests/bpf/progs/tailcall_bpf2bpf3.c   |   9 +-
 .../selftests/bpf/progs/tailcall_bpf2bpf4.c   |  13 +-
 .../bpf/progs/test_btf_skc_cls_ingress.c      |   2 +-
 .../selftests/bpf/progs/test_cgroup_link.c    |   4 +-
 .../selftests/bpf/progs/test_cls_redirect.c   |   2 +-
 .../selftests/bpf/progs/test_global_data.c    |   2 +-
 .../selftests/bpf/progs/test_global_func1.c   |   2 +-
 .../selftests/bpf/progs/test_global_func3.c   |   2 +-
 .../selftests/bpf/progs/test_global_func5.c   |   2 +-
 .../selftests/bpf/progs/test_global_func6.c   |   2 +-
 .../selftests/bpf/progs/test_global_func7.c   |   2 +-
 .../selftests/bpf/progs/test_map_in_map.c     |   2 +-
 .../bpf/progs/test_misc_tcp_hdr_options.c     |   2 +-
 .../selftests/bpf/progs/test_pkt_access.c     |   2 +-
 .../selftests/bpf/progs/test_pkt_md_access.c  |   4 +-
 .../selftests/bpf/progs/test_sk_assign.c      |   3 +-
 .../selftests/bpf/progs/test_sk_lookup.c      |  44 +-
 .../selftests/bpf/progs/test_sk_lookup_kern.c |  37 +-
 .../selftests/bpf/progs/test_skb_helpers.c    |   2 +-
 .../selftests/bpf/progs/test_sockmap_listen.c |   2 +-
 .../progs/test_sockmap_skb_verdict_attach.c   |   2 +-
 .../selftests/bpf/progs/test_sockmap_update.c |   2 +-
 .../selftests/bpf/progs/test_tc_neigh.c       |   6 +-
 .../selftests/bpf/progs/test_tc_neigh_fib.c   |   6 +-
 .../selftests/bpf/progs/test_tc_peer.c        |  10 +-
 .../bpf/progs/test_tcp_check_syncookie_kern.c |   4 +-
 .../bpf/progs/test_tcp_hdr_options.c          |   2 +-
 tools/testing/selftests/bpf/progs/test_xdp.c  |   2 +-
 .../bpf/progs/test_xdp_adjust_tail_grow.c     |   2 +-
 .../bpf/progs/test_xdp_adjust_tail_shrink.c   |   4 +-
 .../bpf/progs/test_xdp_devmap_helpers.c       |   2 +-
 .../selftests/bpf/progs/test_xdp_link.c       |   2 +-
 .../selftests/bpf/progs/test_xdp_loop.c       |   2 +-
 .../selftests/bpf/progs/test_xdp_noinline.c   |   4 +-
 .../bpf/progs/test_xdp_with_cpumap_helpers.c  |   4 +-
 .../bpf/progs/test_xdp_with_devmap_helpers.c  |   4 +-
 tools/testing/selftests/bpf/progs/xdp_dummy.c |   2 +-
 .../bpf/progs/xdp_redirect_multi_kern.c       |   4 +-
 .../testing/selftests/bpf/progs/xdping_kern.c |   4 +-
 .../selftests/bpf/test_tcp_check_syncookie.sh |   4 +-
 .../selftests/bpf/test_xdp_redirect.sh        |   4 +-
 .../selftests/bpf/test_xdp_redirect_multi.sh  |   2 +-
 tools/testing/selftests/bpf/test_xdp_veth.sh  |   4 +-
 tools/testing/selftests/bpf/xdping.c          |   6 +-
 65 files changed, 463 insertions(+), 476 deletions(-)

-- 
2.30.2


^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PATCH v2 bpf-next 1/9] selftests/bpf: normalize XDP section names in selftests
  2021-09-20 23:43 [PATCH v2 bpf-next 0/9] libbpf: stricter BPF program section name handling Andrii Nakryiko
@ 2021-09-20 23:43 ` Andrii Nakryiko
  2021-09-21  4:55   ` Dave Marchevsky
  2021-09-20 23:43 ` [PATCH v2 bpf-next 2/9] selftests/bpf: normalize SEC("classifier") usage Andrii Nakryiko
                   ` (7 subsequent siblings)
  8 siblings, 1 reply; 27+ messages in thread
From: Andrii Nakryiko @ 2021-09-20 23:43 UTC (permalink / raw)
  To: bpf, ast, daniel; +Cc: andrii, kernel-team

Convert almost all SEC("xdp_blah") uses to strict SEC("xdp") to comply
with strict libbpf 1.0 logic of exact section name match for XDP program
types. There is only one exception, which is only tested through
iproute2 and defines multiple XDP programs within the same BPF object.
Given iproute2 still works in non-strict libbpf mode and it doesn't have
means to specify XDP programs by its name (not section name/title),
leave that single file alone for now until iproute2 gains lookup by
function/program name.

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 tools/testing/selftests/bpf/progs/test_map_in_map.c         | 2 +-
 .../selftests/bpf/progs/test_tcp_check_syncookie_kern.c     | 2 +-
 tools/testing/selftests/bpf/progs/test_xdp.c                | 2 +-
 .../testing/selftests/bpf/progs/test_xdp_adjust_tail_grow.c | 2 +-
 .../selftests/bpf/progs/test_xdp_adjust_tail_shrink.c       | 4 +---
 tools/testing/selftests/bpf/progs/test_xdp_devmap_helpers.c | 2 +-
 tools/testing/selftests/bpf/progs/test_xdp_link.c           | 2 +-
 tools/testing/selftests/bpf/progs/test_xdp_loop.c           | 2 +-
 tools/testing/selftests/bpf/progs/test_xdp_noinline.c       | 4 ++--
 .../selftests/bpf/progs/test_xdp_with_cpumap_helpers.c      | 4 ++--
 .../selftests/bpf/progs/test_xdp_with_devmap_helpers.c      | 4 ++--
 tools/testing/selftests/bpf/progs/xdp_dummy.c               | 2 +-
 tools/testing/selftests/bpf/progs/xdp_redirect_multi_kern.c | 4 ++--
 tools/testing/selftests/bpf/progs/xdping_kern.c             | 4 ++--
 tools/testing/selftests/bpf/test_tcp_check_syncookie.sh     | 2 +-
 tools/testing/selftests/bpf/test_xdp_redirect.sh            | 4 ++--
 tools/testing/selftests/bpf/test_xdp_redirect_multi.sh      | 2 +-
 tools/testing/selftests/bpf/test_xdp_veth.sh                | 4 ++--
 tools/testing/selftests/bpf/xdping.c                        | 6 +++---
 19 files changed, 28 insertions(+), 30 deletions(-)

diff --git a/tools/testing/selftests/bpf/progs/test_map_in_map.c b/tools/testing/selftests/bpf/progs/test_map_in_map.c
index 1cfeb940cf9f..5f0e0bfc151e 100644
--- a/tools/testing/selftests/bpf/progs/test_map_in_map.c
+++ b/tools/testing/selftests/bpf/progs/test_map_in_map.c
@@ -23,7 +23,7 @@ struct {
 	__uint(value_size, sizeof(__u32));
 } mim_hash SEC(".maps");
 
-SEC("xdp_mimtest")
+SEC("xdp")
 int xdp_mimtest0(struct xdp_md *ctx)
 {
 	int value = 123;
diff --git a/tools/testing/selftests/bpf/progs/test_tcp_check_syncookie_kern.c b/tools/testing/selftests/bpf/progs/test_tcp_check_syncookie_kern.c
index 47cbe2eeae43..fac7ef99f9a6 100644
--- a/tools/testing/selftests/bpf/progs/test_tcp_check_syncookie_kern.c
+++ b/tools/testing/selftests/bpf/progs/test_tcp_check_syncookie_kern.c
@@ -156,7 +156,7 @@ int check_syncookie_clsact(struct __sk_buff *skb)
 	return TC_ACT_OK;
 }
 
-SEC("xdp/check_syncookie")
+SEC("xdp")
 int check_syncookie_xdp(struct xdp_md *ctx)
 {
 	check_syncookie(ctx, (void *)(long)ctx->data,
diff --git a/tools/testing/selftests/bpf/progs/test_xdp.c b/tools/testing/selftests/bpf/progs/test_xdp.c
index 31f9bce37491..e6aa2fc6ce6b 100644
--- a/tools/testing/selftests/bpf/progs/test_xdp.c
+++ b/tools/testing/selftests/bpf/progs/test_xdp.c
@@ -210,7 +210,7 @@ static __always_inline int handle_ipv6(struct xdp_md *xdp)
 	return XDP_TX;
 }
 
-SEC("xdp_tx_iptunnel")
+SEC("xdp")
 int _xdp_tx_iptunnel(struct xdp_md *xdp)
 {
 	void *data_end = (void *)(long)xdp->data_end;
diff --git a/tools/testing/selftests/bpf/progs/test_xdp_adjust_tail_grow.c b/tools/testing/selftests/bpf/progs/test_xdp_adjust_tail_grow.c
index 3d66599eee2e..199c61b7d062 100644
--- a/tools/testing/selftests/bpf/progs/test_xdp_adjust_tail_grow.c
+++ b/tools/testing/selftests/bpf/progs/test_xdp_adjust_tail_grow.c
@@ -2,7 +2,7 @@
 #include <linux/bpf.h>
 #include <bpf/bpf_helpers.h>
 
-SEC("xdp_adjust_tail_grow")
+SEC("xdp")
 int _xdp_adjust_tail_grow(struct xdp_md *xdp)
 {
 	void *data_end = (void *)(long)xdp->data_end;
diff --git a/tools/testing/selftests/bpf/progs/test_xdp_adjust_tail_shrink.c b/tools/testing/selftests/bpf/progs/test_xdp_adjust_tail_shrink.c
index 22065a9cfb25..b7448253d135 100644
--- a/tools/testing/selftests/bpf/progs/test_xdp_adjust_tail_shrink.c
+++ b/tools/testing/selftests/bpf/progs/test_xdp_adjust_tail_shrink.c
@@ -9,9 +9,7 @@
 #include <linux/if_ether.h>
 #include <bpf/bpf_helpers.h>
 
-int _version SEC("version") = 1;
-
-SEC("xdp_adjust_tail_shrink")
+SEC("xdp")
 int _xdp_adjust_tail_shrink(struct xdp_md *xdp)
 {
 	void *data_end = (void *)(long)xdp->data_end;
diff --git a/tools/testing/selftests/bpf/progs/test_xdp_devmap_helpers.c b/tools/testing/selftests/bpf/progs/test_xdp_devmap_helpers.c
index b360ba2bd441..807bf895f42c 100644
--- a/tools/testing/selftests/bpf/progs/test_xdp_devmap_helpers.c
+++ b/tools/testing/selftests/bpf/progs/test_xdp_devmap_helpers.c
@@ -5,7 +5,7 @@
 #include <linux/bpf.h>
 #include <bpf/bpf_helpers.h>
 
-SEC("xdp_dm_log")
+SEC("xdp")
 int xdpdm_devlog(struct xdp_md *ctx)
 {
 	char fmt[] = "devmap redirect: dev %u -> dev %u len %u\n";
diff --git a/tools/testing/selftests/bpf/progs/test_xdp_link.c b/tools/testing/selftests/bpf/progs/test_xdp_link.c
index eb93ea95d1d8..ee7d6ac0f615 100644
--- a/tools/testing/selftests/bpf/progs/test_xdp_link.c
+++ b/tools/testing/selftests/bpf/progs/test_xdp_link.c
@@ -5,7 +5,7 @@
 
 char LICENSE[] SEC("license") = "GPL";
 
-SEC("xdp/handler")
+SEC("xdp")
 int xdp_handler(struct xdp_md *xdp)
 {
 	return 0;
diff --git a/tools/testing/selftests/bpf/progs/test_xdp_loop.c b/tools/testing/selftests/bpf/progs/test_xdp_loop.c
index fcabcda30ba3..27eb52dda92c 100644
--- a/tools/testing/selftests/bpf/progs/test_xdp_loop.c
+++ b/tools/testing/selftests/bpf/progs/test_xdp_loop.c
@@ -206,7 +206,7 @@ static __always_inline int handle_ipv6(struct xdp_md *xdp)
 	return XDP_TX;
 }
 
-SEC("xdp_tx_iptunnel")
+SEC("xdp")
 int _xdp_tx_iptunnel(struct xdp_md *xdp)
 {
 	void *data_end = (void *)(long)xdp->data_end;
diff --git a/tools/testing/selftests/bpf/progs/test_xdp_noinline.c b/tools/testing/selftests/bpf/progs/test_xdp_noinline.c
index 3a67921f62b5..596c4e71bf3a 100644
--- a/tools/testing/selftests/bpf/progs/test_xdp_noinline.c
+++ b/tools/testing/selftests/bpf/progs/test_xdp_noinline.c
@@ -797,7 +797,7 @@ static int process_packet(void *data, __u64 off, void *data_end,
 	return XDP_DROP;
 }
 
-SEC("xdp-test-v4")
+SEC("xdp")
 int balancer_ingress_v4(struct xdp_md *ctx)
 {
 	void *data = (void *)(long)ctx->data;
@@ -816,7 +816,7 @@ int balancer_ingress_v4(struct xdp_md *ctx)
 		return XDP_DROP;
 }
 
-SEC("xdp-test-v6")
+SEC("xdp")
 int balancer_ingress_v6(struct xdp_md *ctx)
 {
 	void *data = (void *)(long)ctx->data;
diff --git a/tools/testing/selftests/bpf/progs/test_xdp_with_cpumap_helpers.c b/tools/testing/selftests/bpf/progs/test_xdp_with_cpumap_helpers.c
index 59ee4f182ff8..532025057711 100644
--- a/tools/testing/selftests/bpf/progs/test_xdp_with_cpumap_helpers.c
+++ b/tools/testing/selftests/bpf/progs/test_xdp_with_cpumap_helpers.c
@@ -12,13 +12,13 @@ struct {
 	__uint(max_entries, 4);
 } cpu_map SEC(".maps");
 
-SEC("xdp_redir")
+SEC("xdp")
 int xdp_redir_prog(struct xdp_md *ctx)
 {
 	return bpf_redirect_map(&cpu_map, 1, 0);
 }
 
-SEC("xdp_dummy")
+SEC("xdp")
 int xdp_dummy_prog(struct xdp_md *ctx)
 {
 	return XDP_PASS;
diff --git a/tools/testing/selftests/bpf/progs/test_xdp_with_devmap_helpers.c b/tools/testing/selftests/bpf/progs/test_xdp_with_devmap_helpers.c
index 0ac086497722..1e6b9c38ea6d 100644
--- a/tools/testing/selftests/bpf/progs/test_xdp_with_devmap_helpers.c
+++ b/tools/testing/selftests/bpf/progs/test_xdp_with_devmap_helpers.c
@@ -9,7 +9,7 @@ struct {
 	__uint(max_entries, 4);
 } dm_ports SEC(".maps");
 
-SEC("xdp_redir")
+SEC("xdp")
 int xdp_redir_prog(struct xdp_md *ctx)
 {
 	return bpf_redirect_map(&dm_ports, 1, 0);
@@ -18,7 +18,7 @@ int xdp_redir_prog(struct xdp_md *ctx)
 /* invalid program on DEVMAP entry;
  * SEC name means expected attach type not set
  */
-SEC("xdp_dummy")
+SEC("xdp")
 int xdp_dummy_prog(struct xdp_md *ctx)
 {
 	return XDP_PASS;
diff --git a/tools/testing/selftests/bpf/progs/xdp_dummy.c b/tools/testing/selftests/bpf/progs/xdp_dummy.c
index ea25e8881992..d988b2e0cee8 100644
--- a/tools/testing/selftests/bpf/progs/xdp_dummy.c
+++ b/tools/testing/selftests/bpf/progs/xdp_dummy.c
@@ -4,7 +4,7 @@
 #include <linux/bpf.h>
 #include <bpf/bpf_helpers.h>
 
-SEC("xdp_dummy")
+SEC("xdp")
 int xdp_dummy_prog(struct xdp_md *ctx)
 {
 	return XDP_PASS;
diff --git a/tools/testing/selftests/bpf/progs/xdp_redirect_multi_kern.c b/tools/testing/selftests/bpf/progs/xdp_redirect_multi_kern.c
index 880debcbcd65..8395782b6e0a 100644
--- a/tools/testing/selftests/bpf/progs/xdp_redirect_multi_kern.c
+++ b/tools/testing/selftests/bpf/progs/xdp_redirect_multi_kern.c
@@ -34,7 +34,7 @@ struct {
 	__uint(max_entries, 128);
 } mac_map SEC(".maps");
 
-SEC("xdp_redirect_map_multi")
+SEC("xdp")
 int xdp_redirect_map_multi_prog(struct xdp_md *ctx)
 {
 	void *data_end = (void *)(long)ctx->data_end;
@@ -63,7 +63,7 @@ int xdp_redirect_map_multi_prog(struct xdp_md *ctx)
 }
 
 /* The following 2 progs are for 2nd devmap prog testing */
-SEC("xdp_redirect_map_ingress")
+SEC("xdp")
 int xdp_redirect_map_all_prog(struct xdp_md *ctx)
 {
 	return bpf_redirect_map(&map_egress, 0,
diff --git a/tools/testing/selftests/bpf/progs/xdping_kern.c b/tools/testing/selftests/bpf/progs/xdping_kern.c
index 6b9ca40bd1f4..4ad73847b8a5 100644
--- a/tools/testing/selftests/bpf/progs/xdping_kern.c
+++ b/tools/testing/selftests/bpf/progs/xdping_kern.c
@@ -86,7 +86,7 @@ static __always_inline int icmp_check(struct xdp_md *ctx, int type)
 	return XDP_TX;
 }
 
-SEC("xdpclient")
+SEC("xdp")
 int xdping_client(struct xdp_md *ctx)
 {
 	void *data_end = (void *)(long)ctx->data_end;
@@ -150,7 +150,7 @@ int xdping_client(struct xdp_md *ctx)
 	return XDP_TX;
 }
 
-SEC("xdpserver")
+SEC("xdp")
 int xdping_server(struct xdp_md *ctx)
 {
 	void *data_end = (void *)(long)ctx->data_end;
diff --git a/tools/testing/selftests/bpf/test_tcp_check_syncookie.sh b/tools/testing/selftests/bpf/test_tcp_check_syncookie.sh
index 9b3617d770a5..fed765157c53 100755
--- a/tools/testing/selftests/bpf/test_tcp_check_syncookie.sh
+++ b/tools/testing/selftests/bpf/test_tcp_check_syncookie.sh
@@ -77,7 +77,7 @@ TEST_IF=lo
 MAX_PING_TRIES=5
 BPF_PROG_OBJ="${DIR}/test_tcp_check_syncookie_kern.o"
 CLSACT_SECTION="clsact/check_syncookie"
-XDP_SECTION="xdp/check_syncookie"
+XDP_SECTION="xdp"
 BPF_PROG_ID=0
 PROG="${DIR}/test_tcp_check_syncookie_user"
 
diff --git a/tools/testing/selftests/bpf/test_xdp_redirect.sh b/tools/testing/selftests/bpf/test_xdp_redirect.sh
index c033850886f4..57c8db9972a6 100755
--- a/tools/testing/selftests/bpf/test_xdp_redirect.sh
+++ b/tools/testing/selftests/bpf/test_xdp_redirect.sh
@@ -52,8 +52,8 @@ test_xdp_redirect()
 		return 0
 	fi
 
-	ip -n ns1 link set veth11 $xdpmode obj xdp_dummy.o sec xdp_dummy &> /dev/null
-	ip -n ns2 link set veth22 $xdpmode obj xdp_dummy.o sec xdp_dummy &> /dev/null
+	ip -n ns1 link set veth11 $xdpmode obj xdp_dummy.o sec xdp &> /dev/null
+	ip -n ns2 link set veth22 $xdpmode obj xdp_dummy.o sec xdp &> /dev/null
 	ip link set dev veth1 $xdpmode obj test_xdp_redirect.o sec redirect_to_222 &> /dev/null
 	ip link set dev veth2 $xdpmode obj test_xdp_redirect.o sec redirect_to_111 &> /dev/null
 
diff --git a/tools/testing/selftests/bpf/test_xdp_redirect_multi.sh b/tools/testing/selftests/bpf/test_xdp_redirect_multi.sh
index 1538373157e3..351955c2bdfd 100755
--- a/tools/testing/selftests/bpf/test_xdp_redirect_multi.sh
+++ b/tools/testing/selftests/bpf/test_xdp_redirect_multi.sh
@@ -88,7 +88,7 @@ setup_ns()
 		# Add a neigh entry for IPv4 ping test
 		ip -n ns$i neigh add 192.0.2.253 lladdr 00:00:00:00:00:01 dev veth0
 		ip -n ns$i link set veth0 $mode obj \
-			xdp_dummy.o sec xdp_dummy &> /dev/null || \
+			xdp_dummy.o sec xdp &> /dev/null || \
 			{ test_fail "Unable to load dummy xdp" && exit 1; }
 		IFACES="$IFACES veth$i"
 		veth_mac[$i]=$(ip link show veth$i | awk '/link\/ether/ {print $2}')
diff --git a/tools/testing/selftests/bpf/test_xdp_veth.sh b/tools/testing/selftests/bpf/test_xdp_veth.sh
index 995278e684b6..a3a1eaee26ea 100755
--- a/tools/testing/selftests/bpf/test_xdp_veth.sh
+++ b/tools/testing/selftests/bpf/test_xdp_veth.sh
@@ -107,9 +107,9 @@ ip link set dev veth1 xdp pinned $BPF_DIR/progs/redirect_map_0
 ip link set dev veth2 xdp pinned $BPF_DIR/progs/redirect_map_1
 ip link set dev veth3 xdp pinned $BPF_DIR/progs/redirect_map_2
 
-ip -n ns1 link set dev veth11 xdp obj xdp_dummy.o sec xdp_dummy
+ip -n ns1 link set dev veth11 xdp obj xdp_dummy.o sec xdp
 ip -n ns2 link set dev veth22 xdp obj xdp_tx.o sec xdp
-ip -n ns3 link set dev veth33 xdp obj xdp_dummy.o sec xdp_dummy
+ip -n ns3 link set dev veth33 xdp obj xdp_dummy.o sec xdp
 
 trap cleanup EXIT
 
diff --git a/tools/testing/selftests/bpf/xdping.c b/tools/testing/selftests/bpf/xdping.c
index 842d9155d36c..f9798ead20a9 100644
--- a/tools/testing/selftests/bpf/xdping.c
+++ b/tools/testing/selftests/bpf/xdping.c
@@ -178,9 +178,9 @@ int main(int argc, char **argv)
 		return 1;
 	}
 
-	main_prog = bpf_object__find_program_by_title(obj,
-						      server ? "xdpserver" :
-							       "xdpclient");
+	main_prog = bpf_object__find_program_by_name(obj,
+						      server ? "xdping_server" :
+							       "xdping_client");
 	if (main_prog)
 		prog_fd = bpf_program__fd(main_prog);
 	if (!main_prog || prog_fd < 0) {
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH v2 bpf-next 2/9] selftests/bpf: normalize SEC("classifier") usage
  2021-09-20 23:43 [PATCH v2 bpf-next 0/9] libbpf: stricter BPF program section name handling Andrii Nakryiko
  2021-09-20 23:43 ` [PATCH v2 bpf-next 1/9] selftests/bpf: normalize XDP section names in selftests Andrii Nakryiko
@ 2021-09-20 23:43 ` Andrii Nakryiko
  2021-09-21  5:20   ` Dave Marchevsky
  2021-09-20 23:43 ` [PATCH v2 bpf-next 3/9] selftests/bpf: normalize all the rest SEC() uses Andrii Nakryiko
                   ` (6 subsequent siblings)
  8 siblings, 1 reply; 27+ messages in thread
From: Andrii Nakryiko @ 2021-09-20 23:43 UTC (permalink / raw)
  To: bpf, ast, daniel; +Cc: andrii, kernel-team

Convert all SEC("classifier*") uses to strict SEC("classifier") with no
extra characters. In reference_tracking selftests also drop the usage of
broken bpf_program__load(). Along the way switch from ambiguous searching by
program title (section name) to non-ambiguous searching by name in some
selftests, getting closer to completely removing
bpf_object__find_program_by_title().

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 .../bpf/prog_tests/reference_tracking.c       | 22 ++++---
 .../selftests/bpf/prog_tests/sk_assign.c      |  2 +-
 .../selftests/bpf/prog_tests/tailcalls.c      | 58 +++++++++----------
 .../testing/selftests/bpf/progs/skb_pkt_end.c |  2 +-
 tools/testing/selftests/bpf/progs/tailcall1.c |  5 +-
 tools/testing/selftests/bpf/progs/tailcall2.c | 21 ++++---
 tools/testing/selftests/bpf/progs/tailcall3.c |  5 +-
 tools/testing/selftests/bpf/progs/tailcall4.c |  5 +-
 tools/testing/selftests/bpf/progs/tailcall5.c |  5 +-
 tools/testing/selftests/bpf/progs/tailcall6.c |  4 +-
 .../selftests/bpf/progs/tailcall_bpf2bpf1.c   |  5 +-
 .../selftests/bpf/progs/tailcall_bpf2bpf2.c   |  5 +-
 .../selftests/bpf/progs/tailcall_bpf2bpf3.c   |  9 ++-
 .../selftests/bpf/progs/tailcall_bpf2bpf4.c   | 13 ++---
 .../bpf/progs/test_btf_skc_cls_ingress.c      |  2 +-
 .../selftests/bpf/progs/test_cls_redirect.c   |  2 +-
 .../selftests/bpf/progs/test_global_data.c    |  2 +-
 .../selftests/bpf/progs/test_global_func1.c   |  2 +-
 .../selftests/bpf/progs/test_global_func3.c   |  2 +-
 .../selftests/bpf/progs/test_global_func5.c   |  2 +-
 .../selftests/bpf/progs/test_global_func6.c   |  2 +-
 .../selftests/bpf/progs/test_global_func7.c   |  2 +-
 .../selftests/bpf/progs/test_pkt_access.c     |  2 +-
 .../selftests/bpf/progs/test_pkt_md_access.c  |  4 +-
 .../selftests/bpf/progs/test_sk_assign.c      |  3 +-
 .../selftests/bpf/progs/test_sk_lookup_kern.c | 37 ++++++------
 .../selftests/bpf/progs/test_skb_helpers.c    |  2 +-
 .../selftests/bpf/progs/test_sockmap_update.c |  2 +-
 .../selftests/bpf/progs/test_tc_neigh.c       |  6 +-
 .../selftests/bpf/progs/test_tc_neigh_fib.c   |  6 +-
 .../selftests/bpf/progs/test_tc_peer.c        | 10 ++--
 31 files changed, 117 insertions(+), 132 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/reference_tracking.c b/tools/testing/selftests/bpf/prog_tests/reference_tracking.c
index ded2dc8ddd79..836b8bf17fff 100644
--- a/tools/testing/selftests/bpf/prog_tests/reference_tracking.c
+++ b/tools/testing/selftests/bpf/prog_tests/reference_tracking.c
@@ -2,14 +2,14 @@
 #include <test_progs.h>
 
 static void toggle_object_autoload_progs(const struct bpf_object *obj,
-					 const char *title_load)
+					 const char *name_load)
 {
 	struct bpf_program *prog;
 
 	bpf_object__for_each_program(prog, obj) {
-		const char *title = bpf_program__section_name(prog);
+		const char *name = bpf_program__name(prog);
 
-		if (!strcmp(title_load, title))
+		if (!strcmp(name_load, name))
 			bpf_program__set_autoload(prog, true);
 		else
 			bpf_program__set_autoload(prog, false);
@@ -39,23 +39,20 @@ void test_reference_tracking(void)
 		goto cleanup;
 
 	bpf_object__for_each_program(prog, obj_iter) {
-		const char *title;
+		const char *name;
 
 		/* Ignore .text sections */
-		title = bpf_program__section_name(prog);
-		if (strstr(title, ".text") != NULL)
-			continue;
-
-		if (!test__start_subtest(title))
+		name = bpf_program__name(prog);
+		if (!test__start_subtest(name))
 			continue;
 
 		obj = bpf_object__open_file(file, &open_opts);
 		if (!ASSERT_OK_PTR(obj, "obj_open_file"))
 			goto cleanup;
 
-		toggle_object_autoload_progs(obj, title);
+		toggle_object_autoload_progs(obj, name);
 		/* Expect verifier failure if test name has 'err' */
-		if (strstr(title, "err_") != NULL) {
+		if (strncmp(name, "err_", sizeof("err_") - 1) == 0) {
 			libbpf_print_fn_t old_print_fn;
 
 			old_print_fn = libbpf_set_print(NULL);
@@ -64,7 +61,8 @@ void test_reference_tracking(void)
 		} else {
 			err = bpf_object__load(obj);
 		}
-		CHECK(err, title, "\n");
+		ASSERT_OK(err, name);
+
 		bpf_object__close(obj);
 		obj = NULL;
 	}
diff --git a/tools/testing/selftests/bpf/prog_tests/sk_assign.c b/tools/testing/selftests/bpf/prog_tests/sk_assign.c
index 3a469099f30d..7ad3262ea339 100644
--- a/tools/testing/selftests/bpf/prog_tests/sk_assign.c
+++ b/tools/testing/selftests/bpf/prog_tests/sk_assign.c
@@ -48,7 +48,7 @@ configure_stack(void)
 		return false;
 	sprintf(tc_cmd, "%s %s %s %s", "tc filter add dev lo ingress bpf",
 		       "direct-action object-file ./test_sk_assign.o",
-		       "section classifier/sk_assign_test",
+		       "section classifier",
 		       (env.verbosity < VERBOSE_VERY) ? " 2>/dev/null" : "verbose");
 	if (CHECK(system(tc_cmd), "BPF load failed;",
 		  "run with -vv for more info\n"))
diff --git a/tools/testing/selftests/bpf/prog_tests/tailcalls.c b/tools/testing/selftests/bpf/prog_tests/tailcalls.c
index 7bf3a7a97d7b..9825f1f7bfcc 100644
--- a/tools/testing/selftests/bpf/prog_tests/tailcalls.c
+++ b/tools/testing/selftests/bpf/prog_tests/tailcalls.c
@@ -21,7 +21,7 @@ static void test_tailcall_1(void)
 	if (CHECK_FAIL(err))
 		return;
 
-	prog = bpf_object__find_program_by_title(obj, "classifier");
+	prog = bpf_object__find_program_by_name(obj, "entry");
 	if (CHECK_FAIL(!prog))
 		goto out;
 
@@ -38,9 +38,9 @@ static void test_tailcall_1(void)
 		goto out;
 
 	for (i = 0; i < bpf_map__def(prog_array)->max_entries; i++) {
-		snprintf(prog_name, sizeof(prog_name), "classifier/%i", i);
+		snprintf(prog_name, sizeof(prog_name), "classifier_%d", i);
 
-		prog = bpf_object__find_program_by_title(obj, prog_name);
+		prog = bpf_object__find_program_by_name(obj, prog_name);
 		if (CHECK_FAIL(!prog))
 			goto out;
 
@@ -70,9 +70,9 @@ static void test_tailcall_1(void)
 	      err, errno, retval);
 
 	for (i = 0; i < bpf_map__def(prog_array)->max_entries; i++) {
-		snprintf(prog_name, sizeof(prog_name), "classifier/%i", i);
+		snprintf(prog_name, sizeof(prog_name), "classifier_%d", i);
 
-		prog = bpf_object__find_program_by_title(obj, prog_name);
+		prog = bpf_object__find_program_by_name(obj, prog_name);
 		if (CHECK_FAIL(!prog))
 			goto out;
 
@@ -92,9 +92,9 @@ static void test_tailcall_1(void)
 
 	for (i = 0; i < bpf_map__def(prog_array)->max_entries; i++) {
 		j = bpf_map__def(prog_array)->max_entries - 1 - i;
-		snprintf(prog_name, sizeof(prog_name), "classifier/%i", j);
+		snprintf(prog_name, sizeof(prog_name), "classifier_%d", j);
 
-		prog = bpf_object__find_program_by_title(obj, prog_name);
+		prog = bpf_object__find_program_by_name(obj, prog_name);
 		if (CHECK_FAIL(!prog))
 			goto out;
 
@@ -159,7 +159,7 @@ static void test_tailcall_2(void)
 	if (CHECK_FAIL(err))
 		return;
 
-	prog = bpf_object__find_program_by_title(obj, "classifier");
+	prog = bpf_object__find_program_by_name(obj, "entry");
 	if (CHECK_FAIL(!prog))
 		goto out;
 
@@ -176,9 +176,9 @@ static void test_tailcall_2(void)
 		goto out;
 
 	for (i = 0; i < bpf_map__def(prog_array)->max_entries; i++) {
-		snprintf(prog_name, sizeof(prog_name), "classifier/%i", i);
+		snprintf(prog_name, sizeof(prog_name), "classifier_%d", i);
 
-		prog = bpf_object__find_program_by_title(obj, prog_name);
+		prog = bpf_object__find_program_by_name(obj, prog_name);
 		if (CHECK_FAIL(!prog))
 			goto out;
 
@@ -233,7 +233,7 @@ static void test_tailcall_count(const char *which)
 	if (CHECK_FAIL(err))
 		return;
 
-	prog = bpf_object__find_program_by_title(obj, "classifier");
+	prog = bpf_object__find_program_by_name(obj, "entry");
 	if (CHECK_FAIL(!prog))
 		goto out;
 
@@ -249,7 +249,7 @@ static void test_tailcall_count(const char *which)
 	if (CHECK_FAIL(map_fd < 0))
 		goto out;
 
-	prog = bpf_object__find_program_by_title(obj, "classifier/0");
+	prog = bpf_object__find_program_by_name(obj, "classifier_0");
 	if (CHECK_FAIL(!prog))
 		goto out;
 
@@ -329,7 +329,7 @@ static void test_tailcall_4(void)
 	if (CHECK_FAIL(err))
 		return;
 
-	prog = bpf_object__find_program_by_title(obj, "classifier");
+	prog = bpf_object__find_program_by_name(obj, "entry");
 	if (CHECK_FAIL(!prog))
 		goto out;
 
@@ -354,9 +354,9 @@ static void test_tailcall_4(void)
 		return;
 
 	for (i = 0; i < bpf_map__def(prog_array)->max_entries; i++) {
-		snprintf(prog_name, sizeof(prog_name), "classifier/%i", i);
+		snprintf(prog_name, sizeof(prog_name), "classifier_%d", i);
 
-		prog = bpf_object__find_program_by_title(obj, prog_name);
+		prog = bpf_object__find_program_by_name(obj, prog_name);
 		if (CHECK_FAIL(!prog))
 			goto out;
 
@@ -417,7 +417,7 @@ static void test_tailcall_5(void)
 	if (CHECK_FAIL(err))
 		return;
 
-	prog = bpf_object__find_program_by_title(obj, "classifier");
+	prog = bpf_object__find_program_by_name(obj, "entry");
 	if (CHECK_FAIL(!prog))
 		goto out;
 
@@ -442,9 +442,9 @@ static void test_tailcall_5(void)
 		return;
 
 	for (i = 0; i < bpf_map__def(prog_array)->max_entries; i++) {
-		snprintf(prog_name, sizeof(prog_name), "classifier/%i", i);
+		snprintf(prog_name, sizeof(prog_name), "classifier_%d", i);
 
-		prog = bpf_object__find_program_by_title(obj, prog_name);
+		prog = bpf_object__find_program_by_name(obj, prog_name);
 		if (CHECK_FAIL(!prog))
 			goto out;
 
@@ -503,7 +503,7 @@ static void test_tailcall_bpf2bpf_1(void)
 	if (CHECK_FAIL(err))
 		return;
 
-	prog = bpf_object__find_program_by_title(obj, "classifier");
+	prog = bpf_object__find_program_by_name(obj, "entry");
 	if (CHECK_FAIL(!prog))
 		goto out;
 
@@ -521,9 +521,9 @@ static void test_tailcall_bpf2bpf_1(void)
 
 	/* nop -> jmp */
 	for (i = 0; i < bpf_map__def(prog_array)->max_entries; i++) {
-		snprintf(prog_name, sizeof(prog_name), "classifier/%i", i);
+		snprintf(prog_name, sizeof(prog_name), "classifier_%d", i);
 
-		prog = bpf_object__find_program_by_title(obj, prog_name);
+		prog = bpf_object__find_program_by_name(obj, prog_name);
 		if (CHECK_FAIL(!prog))
 			goto out;
 
@@ -587,7 +587,7 @@ static void test_tailcall_bpf2bpf_2(void)
 	if (CHECK_FAIL(err))
 		return;
 
-	prog = bpf_object__find_program_by_title(obj, "classifier");
+	prog = bpf_object__find_program_by_name(obj, "entry");
 	if (CHECK_FAIL(!prog))
 		goto out;
 
@@ -603,7 +603,7 @@ static void test_tailcall_bpf2bpf_2(void)
 	if (CHECK_FAIL(map_fd < 0))
 		goto out;
 
-	prog = bpf_object__find_program_by_title(obj, "classifier/0");
+	prog = bpf_object__find_program_by_name(obj, "classifier_0");
 	if (CHECK_FAIL(!prog))
 		goto out;
 
@@ -665,7 +665,7 @@ static void test_tailcall_bpf2bpf_3(void)
 	if (CHECK_FAIL(err))
 		return;
 
-	prog = bpf_object__find_program_by_title(obj, "classifier");
+	prog = bpf_object__find_program_by_name(obj, "entry");
 	if (CHECK_FAIL(!prog))
 		goto out;
 
@@ -682,9 +682,9 @@ static void test_tailcall_bpf2bpf_3(void)
 		goto out;
 
 	for (i = 0; i < bpf_map__def(prog_array)->max_entries; i++) {
-		snprintf(prog_name, sizeof(prog_name), "classifier/%i", i);
+		snprintf(prog_name, sizeof(prog_name), "classifier_%d", i);
 
-		prog = bpf_object__find_program_by_title(obj, prog_name);
+		prog = bpf_object__find_program_by_name(obj, prog_name);
 		if (CHECK_FAIL(!prog))
 			goto out;
 
@@ -762,7 +762,7 @@ static void test_tailcall_bpf2bpf_4(bool noise)
 	if (CHECK_FAIL(err))
 		return;
 
-	prog = bpf_object__find_program_by_title(obj, "classifier");
+	prog = bpf_object__find_program_by_name(obj, "entry");
 	if (CHECK_FAIL(!prog))
 		goto out;
 
@@ -779,9 +779,9 @@ static void test_tailcall_bpf2bpf_4(bool noise)
 		goto out;
 
 	for (i = 0; i < bpf_map__def(prog_array)->max_entries; i++) {
-		snprintf(prog_name, sizeof(prog_name), "classifier/%i", i);
+		snprintf(prog_name, sizeof(prog_name), "classifier_%d", i);
 
-		prog = bpf_object__find_program_by_title(obj, prog_name);
+		prog = bpf_object__find_program_by_name(obj, prog_name);
 		if (CHECK_FAIL(!prog))
 			goto out;
 
diff --git a/tools/testing/selftests/bpf/progs/skb_pkt_end.c b/tools/testing/selftests/bpf/progs/skb_pkt_end.c
index 7f2eaa2f89f8..e8e7f3b89509 100644
--- a/tools/testing/selftests/bpf/progs/skb_pkt_end.c
+++ b/tools/testing/selftests/bpf/progs/skb_pkt_end.c
@@ -25,7 +25,7 @@ static INLINE struct iphdr *get_iphdr(struct __sk_buff *skb)
 	return ip;
 }
 
-SEC("classifier/cls")
+SEC("classifier")
 int main_prog(struct __sk_buff *skb)
 {
 	struct iphdr *ip = NULL;
diff --git a/tools/testing/selftests/bpf/progs/tailcall1.c b/tools/testing/selftests/bpf/progs/tailcall1.c
index 7115bcefbe8a..3820ab77d0c0 100644
--- a/tools/testing/selftests/bpf/progs/tailcall1.c
+++ b/tools/testing/selftests/bpf/progs/tailcall1.c
@@ -11,8 +11,8 @@ struct {
 } jmp_table SEC(".maps");
 
 #define TAIL_FUNC(x) 				\
-	SEC("classifier/" #x)			\
-	int bpf_func_##x(struct __sk_buff *skb)	\
+	SEC("classifier")			\
+	int classifier_##x(struct __sk_buff *skb)	\
 	{					\
 		return x;			\
 	}
@@ -45,4 +45,3 @@ int entry(struct __sk_buff *skb)
 }
 
 char __license[] SEC("license") = "GPL";
-int _version SEC("version") = 1;
diff --git a/tools/testing/selftests/bpf/progs/tailcall2.c b/tools/testing/selftests/bpf/progs/tailcall2.c
index 0431e4fe7efd..9d4e4701a597 100644
--- a/tools/testing/selftests/bpf/progs/tailcall2.c
+++ b/tools/testing/selftests/bpf/progs/tailcall2.c
@@ -10,35 +10,35 @@ struct {
 	__uint(value_size, sizeof(__u32));
 } jmp_table SEC(".maps");
 
-SEC("classifier/0")
-int bpf_func_0(struct __sk_buff *skb)
+SEC("classifier")
+int classifier_0(struct __sk_buff *skb)
 {
 	bpf_tail_call_static(skb, &jmp_table, 1);
 	return 0;
 }
 
-SEC("classifier/1")
-int bpf_func_1(struct __sk_buff *skb)
+SEC("classifier")
+int classifier_1(struct __sk_buff *skb)
 {
 	bpf_tail_call_static(skb, &jmp_table, 2);
 	return 1;
 }
 
-SEC("classifier/2")
-int bpf_func_2(struct __sk_buff *skb)
+SEC("classifier")
+int classifier_2(struct __sk_buff *skb)
 {
 	return 2;
 }
 
-SEC("classifier/3")
-int bpf_func_3(struct __sk_buff *skb)
+SEC("classifier")
+int classifier_3(struct __sk_buff *skb)
 {
 	bpf_tail_call_static(skb, &jmp_table, 4);
 	return 3;
 }
 
-SEC("classifier/4")
-int bpf_func_4(struct __sk_buff *skb)
+SEC("classifier")
+int classifier_4(struct __sk_buff *skb)
 {
 	bpf_tail_call_static(skb, &jmp_table, 3);
 	return 4;
@@ -56,4 +56,3 @@ int entry(struct __sk_buff *skb)
 }
 
 char __license[] SEC("license") = "GPL";
-int _version SEC("version") = 1;
diff --git a/tools/testing/selftests/bpf/progs/tailcall3.c b/tools/testing/selftests/bpf/progs/tailcall3.c
index 910858fe078a..423e6809a6da 100644
--- a/tools/testing/selftests/bpf/progs/tailcall3.c
+++ b/tools/testing/selftests/bpf/progs/tailcall3.c
@@ -12,8 +12,8 @@ struct {
 
 int count = 0;
 
-SEC("classifier/0")
-int bpf_func_0(struct __sk_buff *skb)
+SEC("classifier")
+int classifier_0(struct __sk_buff *skb)
 {
 	count++;
 	bpf_tail_call_static(skb, &jmp_table, 0);
@@ -28,4 +28,3 @@ int entry(struct __sk_buff *skb)
 }
 
 char __license[] SEC("license") = "GPL";
-int _version SEC("version") = 1;
diff --git a/tools/testing/selftests/bpf/progs/tailcall4.c b/tools/testing/selftests/bpf/progs/tailcall4.c
index bd4be135c39d..82bf8713f1bb 100644
--- a/tools/testing/selftests/bpf/progs/tailcall4.c
+++ b/tools/testing/selftests/bpf/progs/tailcall4.c
@@ -13,8 +13,8 @@ struct {
 int selector = 0;
 
 #define TAIL_FUNC(x)				\
-	SEC("classifier/" #x)			\
-	int bpf_func_##x(struct __sk_buff *skb)	\
+	SEC("classifier")			\
+	int classifier_##x(struct __sk_buff *skb)	\
 	{					\
 		return x;			\
 	}
@@ -30,4 +30,3 @@ int entry(struct __sk_buff *skb)
 }
 
 char __license[] SEC("license") = "GPL";
-int _version SEC("version") = 1;
diff --git a/tools/testing/selftests/bpf/progs/tailcall5.c b/tools/testing/selftests/bpf/progs/tailcall5.c
index adf30a33064e..6925ef193cc5 100644
--- a/tools/testing/selftests/bpf/progs/tailcall5.c
+++ b/tools/testing/selftests/bpf/progs/tailcall5.c
@@ -13,8 +13,8 @@ struct {
 int selector = 0;
 
 #define TAIL_FUNC(x)				\
-	SEC("classifier/" #x)			\
-	int bpf_func_##x(struct __sk_buff *skb)	\
+	SEC("classifier")			\
+	int classifier_##x(struct __sk_buff *skb)	\
 	{					\
 		return x;			\
 	}
@@ -37,4 +37,3 @@ int entry(struct __sk_buff *skb)
 }
 
 char __license[] SEC("license") = "GPL";
-int _version SEC("version") = 1;
diff --git a/tools/testing/selftests/bpf/progs/tailcall6.c b/tools/testing/selftests/bpf/progs/tailcall6.c
index 0f4a811cc028..8505981c7f2d 100644
--- a/tools/testing/selftests/bpf/progs/tailcall6.c
+++ b/tools/testing/selftests/bpf/progs/tailcall6.c
@@ -12,8 +12,8 @@ struct {
 
 int count, which;
 
-SEC("classifier/0")
-int bpf_func_0(struct __sk_buff *skb)
+SEC("classifier")
+int classifier_0(struct __sk_buff *skb)
 {
 	count++;
 	if (__builtin_constant_p(which))
diff --git a/tools/testing/selftests/bpf/progs/tailcall_bpf2bpf1.c b/tools/testing/selftests/bpf/progs/tailcall_bpf2bpf1.c
index 0103f3dd9f02..ce7886c9207b 100644
--- a/tools/testing/selftests/bpf/progs/tailcall_bpf2bpf1.c
+++ b/tools/testing/selftests/bpf/progs/tailcall_bpf2bpf1.c
@@ -10,8 +10,8 @@ struct {
 } jmp_table SEC(".maps");
 
 #define TAIL_FUNC(x) 				\
-	SEC("classifier/" #x)			\
-	int bpf_func_##x(struct __sk_buff *skb)	\
+	SEC("classifier")			\
+	int classifier_##x(struct __sk_buff *skb)	\
 	{					\
 		return x;			\
 	}
@@ -35,4 +35,3 @@ int entry(struct __sk_buff *skb)
 }
 
 char __license[] SEC("license") = "GPL";
-int _version SEC("version") = 1;
diff --git a/tools/testing/selftests/bpf/progs/tailcall_bpf2bpf2.c b/tools/testing/selftests/bpf/progs/tailcall_bpf2bpf2.c
index 3cc4c12817b5..bbe350b2ba45 100644
--- a/tools/testing/selftests/bpf/progs/tailcall_bpf2bpf2.c
+++ b/tools/testing/selftests/bpf/progs/tailcall_bpf2bpf2.c
@@ -22,8 +22,8 @@ int subprog_tail(struct __sk_buff *skb)
 
 int count = 0;
 
-SEC("classifier/0")
-int bpf_func_0(struct __sk_buff *skb)
+SEC("classifier")
+int classifier_0(struct __sk_buff *skb)
 {
 	count++;
 	return subprog_tail(skb);
@@ -38,4 +38,3 @@ int entry(struct __sk_buff *skb)
 }
 
 char __license[] SEC("license") = "GPL";
-int _version SEC("version") = 1;
diff --git a/tools/testing/selftests/bpf/progs/tailcall_bpf2bpf3.c b/tools/testing/selftests/bpf/progs/tailcall_bpf2bpf3.c
index 0d5482bea6c9..fc00e26502d5 100644
--- a/tools/testing/selftests/bpf/progs/tailcall_bpf2bpf3.c
+++ b/tools/testing/selftests/bpf/progs/tailcall_bpf2bpf3.c
@@ -33,16 +33,16 @@ int subprog_tail(struct __sk_buff *skb)
 	return skb->len * 2;
 }
 
-SEC("classifier/0")
-int bpf_func_0(struct __sk_buff *skb)
+SEC("classifier")
+int classifier_0(struct __sk_buff *skb)
 {
 	volatile char arr[128] = {};
 
 	return subprog_tail2(skb);
 }
 
-SEC("classifier/1")
-int bpf_func_1(struct __sk_buff *skb)
+SEC("classifier")
+int classifier_1(struct __sk_buff *skb)
 {
 	volatile char arr[128] = {};
 
@@ -58,4 +58,3 @@ int entry(struct __sk_buff *skb)
 }
 
 char __license[] SEC("license") = "GPL";
-int _version SEC("version") = 1;
diff --git a/tools/testing/selftests/bpf/progs/tailcall_bpf2bpf4.c b/tools/testing/selftests/bpf/progs/tailcall_bpf2bpf4.c
index e89368a50b97..01e95a5c0bb8 100644
--- a/tools/testing/selftests/bpf/progs/tailcall_bpf2bpf4.c
+++ b/tools/testing/selftests/bpf/progs/tailcall_bpf2bpf4.c
@@ -50,21 +50,21 @@ int subprog_tail(struct __sk_buff *skb)
 	return skb->len;
 }
 
-SEC("classifier/1")
-int bpf_func_1(struct __sk_buff *skb)
+SEC("classifier")
+int classifier_1(struct __sk_buff *skb)
 {
 	return subprog_tail_2(skb);
 }
 
-SEC("classifier/2")
-int bpf_func_2(struct __sk_buff *skb)
+SEC("classifier")
+int classifier_2(struct __sk_buff *skb)
 {
 	count++;
 	return subprog_tail_2(skb);
 }
 
-SEC("classifier/0")
-int bpf_func_0(struct __sk_buff *skb)
+SEC("classifier")
+int classifier_0(struct __sk_buff *skb)
 {
 	return subprog_tail_1(skb);
 }
@@ -76,4 +76,3 @@ int entry(struct __sk_buff *skb)
 }
 
 char __license[] SEC("license") = "GPL";
-int _version SEC("version") = 1;
diff --git a/tools/testing/selftests/bpf/progs/test_btf_skc_cls_ingress.c b/tools/testing/selftests/bpf/progs/test_btf_skc_cls_ingress.c
index 9a6b85dd52d2..56aa9389a6c8 100644
--- a/tools/testing/selftests/bpf/progs/test_btf_skc_cls_ingress.c
+++ b/tools/testing/selftests/bpf/progs/test_btf_skc_cls_ingress.c
@@ -145,7 +145,7 @@ static int handle_ip6_tcp(struct ipv6hdr *ip6h, struct __sk_buff *skb)
 	return TC_ACT_OK;
 }
 
-SEC("classifier/ingress")
+SEC("classifier")
 int cls_ingress(struct __sk_buff *skb)
 {
 	struct ipv6hdr *ip6h;
diff --git a/tools/testing/selftests/bpf/progs/test_cls_redirect.c b/tools/testing/selftests/bpf/progs/test_cls_redirect.c
index e2a5acc4785c..f1f7213f6170 100644
--- a/tools/testing/selftests/bpf/progs/test_cls_redirect.c
+++ b/tools/testing/selftests/bpf/progs/test_cls_redirect.c
@@ -928,7 +928,7 @@ static INLINING verdict_t process_ipv6(buf_t *pkt, metrics_t *metrics)
 	}
 }
 
-SEC("classifier/cls_redirect")
+SEC("classifier")
 int cls_redirect(struct __sk_buff *skb)
 {
 	metrics_t *metrics = get_global_metrics();
diff --git a/tools/testing/selftests/bpf/progs/test_global_data.c b/tools/testing/selftests/bpf/progs/test_global_data.c
index 1319be1c54ba..3f435a27f183 100644
--- a/tools/testing/selftests/bpf/progs/test_global_data.c
+++ b/tools/testing/selftests/bpf/progs/test_global_data.c
@@ -68,7 +68,7 @@ static struct foo struct3 = {
 		bpf_map_update_elem(&result_##map, &key, var, 0);	\
 	} while (0)
 
-SEC("classifier/static_data_load")
+SEC("classifier")
 int load_static_data(struct __sk_buff *skb)
 {
 	static const __u64 bar = ~0;
diff --git a/tools/testing/selftests/bpf/progs/test_global_func1.c b/tools/testing/selftests/bpf/progs/test_global_func1.c
index 880260f6d536..2511f9391da9 100644
--- a/tools/testing/selftests/bpf/progs/test_global_func1.c
+++ b/tools/testing/selftests/bpf/progs/test_global_func1.c
@@ -38,7 +38,7 @@ int f3(int val, struct __sk_buff *skb, int var)
 	return skb->ifindex * val * var;
 }
 
-SEC("classifier/test")
+SEC("classifier")
 int test_cls(struct __sk_buff *skb)
 {
 	return f0(1, skb) + f1(skb) + f2(2, skb) + f3(3, skb, 4);
diff --git a/tools/testing/selftests/bpf/progs/test_global_func3.c b/tools/testing/selftests/bpf/progs/test_global_func3.c
index 86f0ecb304fc..84ef0df0adf7 100644
--- a/tools/testing/selftests/bpf/progs/test_global_func3.c
+++ b/tools/testing/selftests/bpf/progs/test_global_func3.c
@@ -54,7 +54,7 @@ int f8(struct __sk_buff *skb)
 }
 #endif
 
-SEC("classifier/test")
+SEC("classifier")
 int test_cls(struct __sk_buff *skb)
 {
 #ifndef NO_FN8
diff --git a/tools/testing/selftests/bpf/progs/test_global_func5.c b/tools/testing/selftests/bpf/progs/test_global_func5.c
index 260c25b827ef..70e660084aed 100644
--- a/tools/testing/selftests/bpf/progs/test_global_func5.c
+++ b/tools/testing/selftests/bpf/progs/test_global_func5.c
@@ -24,7 +24,7 @@ int f3(int val, struct __sk_buff *skb)
 	return skb->ifindex * val;
 }
 
-SEC("classifier/test")
+SEC("classifier")
 int test_cls(struct __sk_buff *skb)
 {
 	return f1(skb) + f2(2, skb) + f3(3, skb);
diff --git a/tools/testing/selftests/bpf/progs/test_global_func6.c b/tools/testing/selftests/bpf/progs/test_global_func6.c
index 69e19c64e10b..c886f91bb560 100644
--- a/tools/testing/selftests/bpf/progs/test_global_func6.c
+++ b/tools/testing/selftests/bpf/progs/test_global_func6.c
@@ -24,7 +24,7 @@ int f3(int val, struct __sk_buff *skb)
 	return skb->ifindex * val;
 }
 
-SEC("classifier/test")
+SEC("classifier")
 int test_cls(struct __sk_buff *skb)
 {
 	return f1(skb) + f2(2, skb) + f3(3, skb);
diff --git a/tools/testing/selftests/bpf/progs/test_global_func7.c b/tools/testing/selftests/bpf/progs/test_global_func7.c
index 309b3f6136bd..23bb224f8d1a 100644
--- a/tools/testing/selftests/bpf/progs/test_global_func7.c
+++ b/tools/testing/selftests/bpf/progs/test_global_func7.c
@@ -10,7 +10,7 @@ void foo(struct __sk_buff *skb)
 	skb->tc_index = 0;
 }
 
-SEC("classifier/test")
+SEC("classifier")
 int test_cls(struct __sk_buff *skb)
 {
 	foo(skb);
diff --git a/tools/testing/selftests/bpf/progs/test_pkt_access.c b/tools/testing/selftests/bpf/progs/test_pkt_access.c
index 852051064507..b10c473aba26 100644
--- a/tools/testing/selftests/bpf/progs/test_pkt_access.c
+++ b/tools/testing/selftests/bpf/progs/test_pkt_access.c
@@ -97,7 +97,7 @@ int test_pkt_write_access_subprog(struct __sk_buff *skb, __u32 off)
 	return 0;
 }
 
-SEC("classifier/test_pkt_access")
+SEC("classifier")
 int test_pkt_access(struct __sk_buff *skb)
 {
 	void *data_end = (void *)(long)skb->data_end;
diff --git a/tools/testing/selftests/bpf/progs/test_pkt_md_access.c b/tools/testing/selftests/bpf/progs/test_pkt_md_access.c
index 610c74ea9f64..a770f3562b9d 100644
--- a/tools/testing/selftests/bpf/progs/test_pkt_md_access.c
+++ b/tools/testing/selftests/bpf/progs/test_pkt_md_access.c
@@ -7,8 +7,6 @@
 #include <linux/pkt_cls.h>
 #include <bpf/bpf_helpers.h>
 
-int _version SEC("version") = 1;
-
 #if  __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
 #define TEST_FIELD(TYPE, FIELD, MASK)					\
 	{								\
@@ -27,7 +25,7 @@ int _version SEC("version") = 1;
 	}
 #endif
 
-SEC("classifier/test_pkt_md_access")
+SEC("classifier")
 int test_pkt_md_access(struct __sk_buff *skb)
 {
 	TEST_FIELD(__u8,  len, 0xFF);
diff --git a/tools/testing/selftests/bpf/progs/test_sk_assign.c b/tools/testing/selftests/bpf/progs/test_sk_assign.c
index 1ecd987005d2..dfc81c18deff 100644
--- a/tools/testing/selftests/bpf/progs/test_sk_assign.c
+++ b/tools/testing/selftests/bpf/progs/test_sk_assign.c
@@ -36,7 +36,6 @@ struct {
 	.pinning = PIN_GLOBAL_NS,
 };
 
-int _version SEC("version") = 1;
 char _license[] SEC("license") = "GPL";
 
 /* Fill 'tuple' with L3 info, and attempt to find L4. On fail, return NULL. */
@@ -159,7 +158,7 @@ handle_tcp(struct __sk_buff *skb, struct bpf_sock_tuple *tuple, bool ipv4)
 	return ret;
 }
 
-SEC("classifier/sk_assign_test")
+SEC("classifier")
 int bpf_sk_assign_test(struct __sk_buff *skb)
 {
 	struct bpf_sock_tuple *tuple, ln = {0};
diff --git a/tools/testing/selftests/bpf/progs/test_sk_lookup_kern.c b/tools/testing/selftests/bpf/progs/test_sk_lookup_kern.c
index 8249075f088f..5b2642c21bae 100644
--- a/tools/testing/selftests/bpf/progs/test_sk_lookup_kern.c
+++ b/tools/testing/selftests/bpf/progs/test_sk_lookup_kern.c
@@ -15,7 +15,6 @@
 #include <bpf/bpf_helpers.h>
 #include <bpf/bpf_endian.h>
 
-int _version SEC("version") = 1;
 char _license[] SEC("license") = "GPL";
 
 /* Fill 'tuple' with L3 info, and attempt to find L4. On fail, return NULL. */
@@ -53,8 +52,8 @@ static struct bpf_sock_tuple *get_tuple(void *data, __u64 nh_off,
 	return result;
 }
 
-SEC("classifier/sk_lookup_success")
-int bpf_sk_lookup_test0(struct __sk_buff *skb)
+SEC("classifier")
+int sk_lookup_success(struct __sk_buff *skb)
 {
 	void *data_end = (void *)(long)skb->data_end;
 	void *data = (void *)(long)skb->data;
@@ -79,8 +78,8 @@ int bpf_sk_lookup_test0(struct __sk_buff *skb)
 	return sk ? TC_ACT_OK : TC_ACT_UNSPEC;
 }
 
-SEC("classifier/sk_lookup_success_simple")
-int bpf_sk_lookup_test1(struct __sk_buff *skb)
+SEC("classifier")
+int sk_lookup_success_simple(struct __sk_buff *skb)
 {
 	struct bpf_sock_tuple tuple = {};
 	struct bpf_sock *sk;
@@ -91,8 +90,8 @@ int bpf_sk_lookup_test1(struct __sk_buff *skb)
 	return 0;
 }
 
-SEC("classifier/err_use_after_free")
-int bpf_sk_lookup_uaf(struct __sk_buff *skb)
+SEC("classifier")
+int err_use_after_free(struct __sk_buff *skb)
 {
 	struct bpf_sock_tuple tuple = {};
 	struct bpf_sock *sk;
@@ -106,8 +105,8 @@ int bpf_sk_lookup_uaf(struct __sk_buff *skb)
 	return family;
 }
 
-SEC("classifier/err_modify_sk_pointer")
-int bpf_sk_lookup_modptr(struct __sk_buff *skb)
+SEC("classifier")
+int err_modify_sk_pointer(struct __sk_buff *skb)
 {
 	struct bpf_sock_tuple tuple = {};
 	struct bpf_sock *sk;
@@ -121,8 +120,8 @@ int bpf_sk_lookup_modptr(struct __sk_buff *skb)
 	return 0;
 }
 
-SEC("classifier/err_modify_sk_or_null_pointer")
-int bpf_sk_lookup_modptr_or_null(struct __sk_buff *skb)
+SEC("classifier")
+int err_modify_sk_or_null_pointer(struct __sk_buff *skb)
 {
 	struct bpf_sock_tuple tuple = {};
 	struct bpf_sock *sk;
@@ -135,8 +134,8 @@ int bpf_sk_lookup_modptr_or_null(struct __sk_buff *skb)
 	return 0;
 }
 
-SEC("classifier/err_no_release")
-int bpf_sk_lookup_test2(struct __sk_buff *skb)
+SEC("classifier")
+int err_no_release(struct __sk_buff *skb)
 {
 	struct bpf_sock_tuple tuple = {};
 
@@ -144,8 +143,8 @@ int bpf_sk_lookup_test2(struct __sk_buff *skb)
 	return 0;
 }
 
-SEC("classifier/err_release_twice")
-int bpf_sk_lookup_test3(struct __sk_buff *skb)
+SEC("classifier")
+int err_release_twice(struct __sk_buff *skb)
 {
 	struct bpf_sock_tuple tuple = {};
 	struct bpf_sock *sk;
@@ -156,8 +155,8 @@ int bpf_sk_lookup_test3(struct __sk_buff *skb)
 	return 0;
 }
 
-SEC("classifier/err_release_unchecked")
-int bpf_sk_lookup_test4(struct __sk_buff *skb)
+SEC("classifier")
+int err_release_unchecked(struct __sk_buff *skb)
 {
 	struct bpf_sock_tuple tuple = {};
 	struct bpf_sock *sk;
@@ -173,8 +172,8 @@ void lookup_no_release(struct __sk_buff *skb)
 	bpf_sk_lookup_tcp(skb, &tuple, sizeof(tuple), BPF_F_CURRENT_NETNS, 0);
 }
 
-SEC("classifier/err_no_release_subcall")
-int bpf_sk_lookup_test5(struct __sk_buff *skb)
+SEC("classifier")
+int err_no_release_subcall(struct __sk_buff *skb)
 {
 	lookup_no_release(skb);
 	return 0;
diff --git a/tools/testing/selftests/bpf/progs/test_skb_helpers.c b/tools/testing/selftests/bpf/progs/test_skb_helpers.c
index bb3fbf1a29e3..e17bb3a039e3 100644
--- a/tools/testing/selftests/bpf/progs/test_skb_helpers.c
+++ b/tools/testing/selftests/bpf/progs/test_skb_helpers.c
@@ -14,7 +14,7 @@ struct {
 
 char _license[] SEC("license") = "GPL";
 
-SEC("classifier/test_skb_helpers")
+SEC("classifier")
 int test_skb_helpers(struct __sk_buff *skb)
 {
 	struct task_struct *task;
diff --git a/tools/testing/selftests/bpf/progs/test_sockmap_update.c b/tools/testing/selftests/bpf/progs/test_sockmap_update.c
index 9d0c9f28cab2..6ac05bd5b827 100644
--- a/tools/testing/selftests/bpf/progs/test_sockmap_update.c
+++ b/tools/testing/selftests/bpf/progs/test_sockmap_update.c
@@ -24,7 +24,7 @@ struct {
 	__type(value, __u64);
 } dst_sock_hash SEC(".maps");
 
-SEC("classifier/copy_sock_map")
+SEC("classifier")
 int copy_sock_map(void *ctx)
 {
 	struct bpf_sock *sk;
diff --git a/tools/testing/selftests/bpf/progs/test_tc_neigh.c b/tools/testing/selftests/bpf/progs/test_tc_neigh.c
index 0c93d326a663..03145db5da2e 100644
--- a/tools/testing/selftests/bpf/progs/test_tc_neigh.c
+++ b/tools/testing/selftests/bpf/progs/test_tc_neigh.c
@@ -70,7 +70,7 @@ static __always_inline bool is_remote_ep_v6(struct __sk_buff *skb,
 	return v6_equal(ip6h->daddr, addr);
 }
 
-SEC("classifier/chk_egress")
+SEC("classifier")
 int tc_chk(struct __sk_buff *skb)
 {
 	void *data_end = ctx_ptr(skb->data_end);
@@ -83,7 +83,7 @@ int tc_chk(struct __sk_buff *skb)
 	return !raw[0] && !raw[1] && !raw[2] ? TC_ACT_SHOT : TC_ACT_OK;
 }
 
-SEC("classifier/dst_ingress")
+SEC("classifier")
 int tc_dst(struct __sk_buff *skb)
 {
 	__u8 zero[ETH_ALEN * 2];
@@ -108,7 +108,7 @@ int tc_dst(struct __sk_buff *skb)
 	return bpf_redirect_neigh(IFINDEX_SRC, NULL, 0, 0);
 }
 
-SEC("classifier/src_ingress")
+SEC("classifier")
 int tc_src(struct __sk_buff *skb)
 {
 	__u8 zero[ETH_ALEN * 2];
diff --git a/tools/testing/selftests/bpf/progs/test_tc_neigh_fib.c b/tools/testing/selftests/bpf/progs/test_tc_neigh_fib.c
index f7ab69cf018e..c3d1d1f38232 100644
--- a/tools/testing/selftests/bpf/progs/test_tc_neigh_fib.c
+++ b/tools/testing/selftests/bpf/progs/test_tc_neigh_fib.c
@@ -75,7 +75,7 @@ static __always_inline int fill_fib_params_v6(struct __sk_buff *skb,
 	return 0;
 }
 
-SEC("classifier/chk_egress")
+SEC("classifier")
 int tc_chk(struct __sk_buff *skb)
 {
 	void *data_end = ctx_ptr(skb->data_end);
@@ -143,13 +143,13 @@ static __always_inline int tc_redir(struct __sk_buff *skb)
 /* these are identical, but keep them separate for compatibility with the
  * section names expected by test_tc_redirect.sh
  */
-SEC("classifier/dst_ingress")
+SEC("classifier")
 int tc_dst(struct __sk_buff *skb)
 {
 	return tc_redir(skb);
 }
 
-SEC("classifier/src_ingress")
+SEC("classifier")
 int tc_src(struct __sk_buff *skb)
 {
 	return tc_redir(skb);
diff --git a/tools/testing/selftests/bpf/progs/test_tc_peer.c b/tools/testing/selftests/bpf/progs/test_tc_peer.c
index fe818cd5f010..7d0256d7db82 100644
--- a/tools/testing/selftests/bpf/progs/test_tc_peer.c
+++ b/tools/testing/selftests/bpf/progs/test_tc_peer.c
@@ -16,31 +16,31 @@ volatile const __u32 IFINDEX_DST;
 static const __u8 src_mac[] = {0x00, 0x11, 0x22, 0x33, 0x44, 0x55};
 static const __u8 dst_mac[] = {0x00, 0x22, 0x33, 0x44, 0x55, 0x66};
 
-SEC("classifier/chk_egress")
+SEC("classifier")
 int tc_chk(struct __sk_buff *skb)
 {
 	return TC_ACT_SHOT;
 }
 
-SEC("classifier/dst_ingress")
+SEC("classifier")
 int tc_dst(struct __sk_buff *skb)
 {
 	return bpf_redirect_peer(IFINDEX_SRC, 0);
 }
 
-SEC("classifier/src_ingress")
+SEC("classifier")
 int tc_src(struct __sk_buff *skb)
 {
 	return bpf_redirect_peer(IFINDEX_DST, 0);
 }
 
-SEC("classifier/dst_ingress_l3")
+SEC("classifier")
 int tc_dst_l3(struct __sk_buff *skb)
 {
 	return bpf_redirect(IFINDEX_SRC, 0);
 }
 
-SEC("classifier/src_ingress_l3")
+SEC("classifier")
 int tc_src_l3(struct __sk_buff *skb)
 {
 	__u16 proto = skb->protocol;
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH v2 bpf-next 3/9] selftests/bpf: normalize all the rest SEC() uses
  2021-09-20 23:43 [PATCH v2 bpf-next 0/9] libbpf: stricter BPF program section name handling Andrii Nakryiko
  2021-09-20 23:43 ` [PATCH v2 bpf-next 1/9] selftests/bpf: normalize XDP section names in selftests Andrii Nakryiko
  2021-09-20 23:43 ` [PATCH v2 bpf-next 2/9] selftests/bpf: normalize SEC("classifier") usage Andrii Nakryiko
@ 2021-09-20 23:43 ` Andrii Nakryiko
  2021-09-21  5:41   ` Dave Marchevsky
  2021-09-20 23:43 ` [PATCH v2 bpf-next 4/9] libbpf: refactor internal sec_def handling to enable pluggability Andrii Nakryiko
                   ` (5 subsequent siblings)
  8 siblings, 1 reply; 27+ messages in thread
From: Andrii Nakryiko @ 2021-09-20 23:43 UTC (permalink / raw)
  To: bpf, ast, daniel; +Cc: andrii, kernel-team

Normalize all the other non-conforming SEC() usages across all
selftests. This is in preparation for libbpf to start to enforce
stricter SEC() rules in libbpf 1.0 mode.

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 .../selftests/bpf/prog_tests/flow_dissector.c |  4 +--
 .../selftests/bpf/prog_tests/sockopt_multi.c  | 30 +++++++++----------
 tools/testing/selftests/bpf/progs/bpf_flow.c  |  3 +-
 .../bpf/progs/cg_storage_multi_isolated.c     |  4 +--
 .../bpf/progs/cg_storage_multi_shared.c       |  4 +--
 .../selftests/bpf/progs/sockopt_multi.c       |  5 ++--
 .../selftests/bpf/progs/test_cgroup_link.c    |  4 +--
 .../bpf/progs/test_misc_tcp_hdr_options.c     |  2 +-
 .../selftests/bpf/progs/test_sk_lookup.c      |  6 ++--
 .../selftests/bpf/progs/test_sockmap_listen.c |  2 +-
 .../progs/test_sockmap_skb_verdict_attach.c   |  2 +-
 .../bpf/progs/test_tcp_check_syncookie_kern.c |  2 +-
 .../bpf/progs/test_tcp_hdr_options.c          |  2 +-
 .../selftests/bpf/test_tcp_check_syncookie.sh |  2 +-
 14 files changed, 35 insertions(+), 37 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/flow_dissector.c b/tools/testing/selftests/bpf/prog_tests/flow_dissector.c
index 225714f71ac6..ac54e3f91d42 100644
--- a/tools/testing/selftests/bpf/prog_tests/flow_dissector.c
+++ b/tools/testing/selftests/bpf/prog_tests/flow_dissector.c
@@ -458,9 +458,9 @@ static int init_prog_array(struct bpf_object *obj, struct bpf_map *prog_array)
 		return -1;
 
 	for (i = 0; i < bpf_map__def(prog_array)->max_entries; i++) {
-		snprintf(prog_name, sizeof(prog_name), "flow_dissector/%i", i);
+		snprintf(prog_name, sizeof(prog_name), "flow_dissector_%d", i);
 
-		prog = bpf_object__find_program_by_title(obj, prog_name);
+		prog = bpf_object__find_program_by_name(obj, prog_name);
 		if (!prog)
 			return -1;
 
diff --git a/tools/testing/selftests/bpf/prog_tests/sockopt_multi.c b/tools/testing/selftests/bpf/prog_tests/sockopt_multi.c
index 51fac975b316..bc34f7773444 100644
--- a/tools/testing/selftests/bpf/prog_tests/sockopt_multi.c
+++ b/tools/testing/selftests/bpf/prog_tests/sockopt_multi.c
@@ -2,7 +2,7 @@
 #include <test_progs.h>
 #include "cgroup_helpers.h"
 
-static int prog_attach(struct bpf_object *obj, int cgroup_fd, const char *title)
+static int prog_attach(struct bpf_object *obj, int cgroup_fd, const char *title, const char *name)
 {
 	enum bpf_attach_type attach_type;
 	enum bpf_prog_type prog_type;
@@ -15,23 +15,23 @@ static int prog_attach(struct bpf_object *obj, int cgroup_fd, const char *title)
 		return -1;
 	}
 
-	prog = bpf_object__find_program_by_title(obj, title);
+	prog = bpf_object__find_program_by_name(obj, name);
 	if (!prog) {
-		log_err("Failed to find %s BPF program", title);
+		log_err("Failed to find %s BPF program", name);
 		return -1;
 	}
 
 	err = bpf_prog_attach(bpf_program__fd(prog), cgroup_fd,
 			      attach_type, BPF_F_ALLOW_MULTI);
 	if (err) {
-		log_err("Failed to attach %s BPF program", title);
+		log_err("Failed to attach %s BPF program", name);
 		return -1;
 	}
 
 	return 0;
 }
 
-static int prog_detach(struct bpf_object *obj, int cgroup_fd, const char *title)
+static int prog_detach(struct bpf_object *obj, int cgroup_fd, const char *title, const char *name)
 {
 	enum bpf_attach_type attach_type;
 	enum bpf_prog_type prog_type;
@@ -42,7 +42,7 @@ static int prog_detach(struct bpf_object *obj, int cgroup_fd, const char *title)
 	if (err)
 		return -1;
 
-	prog = bpf_object__find_program_by_title(obj, title);
+	prog = bpf_object__find_program_by_name(obj, name);
 	if (!prog)
 		return -1;
 
@@ -89,7 +89,7 @@ static int run_getsockopt_test(struct bpf_object *obj, int cg_parent,
 	 * - child:  0x80 -> 0x90
 	 */
 
-	err = prog_attach(obj, cg_child, "cgroup/getsockopt/child");
+	err = prog_attach(obj, cg_child, "cgroup/getsockopt", "_getsockopt_child");
 	if (err)
 		goto detach;
 
@@ -113,7 +113,7 @@ static int run_getsockopt_test(struct bpf_object *obj, int cg_parent,
 	 * - parent: 0x90 -> 0xA0
 	 */
 
-	err = prog_attach(obj, cg_parent, "cgroup/getsockopt/parent");
+	err = prog_attach(obj, cg_parent, "cgroup/getsockopt", "_getsockopt_parent");
 	if (err)
 		goto detach;
 
@@ -157,7 +157,7 @@ static int run_getsockopt_test(struct bpf_object *obj, int cg_parent,
 	 * - parent: unexpected 0x40, EPERM
 	 */
 
-	err = prog_detach(obj, cg_child, "cgroup/getsockopt/child");
+	err = prog_detach(obj, cg_child, "cgroup/getsockopt", "_getsockopt_child");
 	if (err) {
 		log_err("Failed to detach child program");
 		goto detach;
@@ -198,8 +198,8 @@ static int run_getsockopt_test(struct bpf_object *obj, int cg_parent,
 	}
 
 detach:
-	prog_detach(obj, cg_child, "cgroup/getsockopt/child");
-	prog_detach(obj, cg_parent, "cgroup/getsockopt/parent");
+	prog_detach(obj, cg_child, "cgroup/getsockopt", "_getsockopt_child");
+	prog_detach(obj, cg_parent, "cgroup/getsockopt", "_getsockopt_parent");
 
 	return err;
 }
@@ -236,7 +236,7 @@ static int run_setsockopt_test(struct bpf_object *obj, int cg_parent,
 
 	/* Attach child program and make sure it adds 0x10. */
 
-	err = prog_attach(obj, cg_child, "cgroup/setsockopt");
+	err = prog_attach(obj, cg_child, "cgroup/setsockopt", "_setsockopt");
 	if (err)
 		goto detach;
 
@@ -263,7 +263,7 @@ static int run_setsockopt_test(struct bpf_object *obj, int cg_parent,
 
 	/* Attach parent program and make sure it adds another 0x10. */
 
-	err = prog_attach(obj, cg_parent, "cgroup/setsockopt");
+	err = prog_attach(obj, cg_parent, "cgroup/setsockopt", "_setsockopt");
 	if (err)
 		goto detach;
 
@@ -289,8 +289,8 @@ static int run_setsockopt_test(struct bpf_object *obj, int cg_parent,
 	}
 
 detach:
-	prog_detach(obj, cg_child, "cgroup/setsockopt");
-	prog_detach(obj, cg_parent, "cgroup/setsockopt");
+	prog_detach(obj, cg_child, "cgroup/setsockopt", "_setsockopt");
+	prog_detach(obj, cg_parent, "cgroup/setsockopt", "_setsockopt");
 
 	return err;
 }
diff --git a/tools/testing/selftests/bpf/progs/bpf_flow.c b/tools/testing/selftests/bpf/progs/bpf_flow.c
index 95a5a0778ed7..f266c757b3df 100644
--- a/tools/testing/selftests/bpf/progs/bpf_flow.c
+++ b/tools/testing/selftests/bpf/progs/bpf_flow.c
@@ -19,9 +19,8 @@
 #include <bpf/bpf_helpers.h>
 #include <bpf/bpf_endian.h>
 
-int _version SEC("version") = 1;
 #define PROG(F) PROG_(F, _##F)
-#define PROG_(NUM, NAME) SEC("flow_dissector/"#NUM) int bpf_func##NAME
+#define PROG_(NUM, NAME) SEC("flow_dissector") int flow_dissector_##NUM
 
 /* These are the identifiers of the BPF programs that will be used in tail
  * calls. Name is limited to 16 characters, with the terminating character and
diff --git a/tools/testing/selftests/bpf/progs/cg_storage_multi_isolated.c b/tools/testing/selftests/bpf/progs/cg_storage_multi_isolated.c
index a25373002055..3f81ff92184c 100644
--- a/tools/testing/selftests/bpf/progs/cg_storage_multi_isolated.c
+++ b/tools/testing/selftests/bpf/progs/cg_storage_multi_isolated.c
@@ -20,7 +20,7 @@ struct {
 
 __u32 invocations = 0;
 
-SEC("cgroup_skb/egress/1")
+SEC("cgroup_skb/egress")
 int egress1(struct __sk_buff *skb)
 {
 	struct cgroup_value *ptr_cg_storage =
@@ -32,7 +32,7 @@ int egress1(struct __sk_buff *skb)
 	return 1;
 }
 
-SEC("cgroup_skb/egress/2")
+SEC("cgroup_skb/egress")
 int egress2(struct __sk_buff *skb)
 {
 	struct cgroup_value *ptr_cg_storage =
diff --git a/tools/testing/selftests/bpf/progs/cg_storage_multi_shared.c b/tools/testing/selftests/bpf/progs/cg_storage_multi_shared.c
index a149f33bc533..d662db27fe4a 100644
--- a/tools/testing/selftests/bpf/progs/cg_storage_multi_shared.c
+++ b/tools/testing/selftests/bpf/progs/cg_storage_multi_shared.c
@@ -20,7 +20,7 @@ struct {
 
 __u32 invocations = 0;
 
-SEC("cgroup_skb/egress/1")
+SEC("cgroup_skb/egress")
 int egress1(struct __sk_buff *skb)
 {
 	struct cgroup_value *ptr_cg_storage =
@@ -32,7 +32,7 @@ int egress1(struct __sk_buff *skb)
 	return 1;
 }
 
-SEC("cgroup_skb/egress/2")
+SEC("cgroup_skb/egress")
 int egress2(struct __sk_buff *skb)
 {
 	struct cgroup_value *ptr_cg_storage =
diff --git a/tools/testing/selftests/bpf/progs/sockopt_multi.c b/tools/testing/selftests/bpf/progs/sockopt_multi.c
index 9d8c212dde9f..177a59069dae 100644
--- a/tools/testing/selftests/bpf/progs/sockopt_multi.c
+++ b/tools/testing/selftests/bpf/progs/sockopt_multi.c
@@ -4,9 +4,8 @@
 #include <bpf/bpf_helpers.h>
 
 char _license[] SEC("license") = "GPL";
-__u32 _version SEC("version") = 1;
 
-SEC("cgroup/getsockopt/child")
+SEC("cgroup/getsockopt")
 int _getsockopt_child(struct bpf_sockopt *ctx)
 {
 	__u8 *optval_end = ctx->optval_end;
@@ -29,7 +28,7 @@ int _getsockopt_child(struct bpf_sockopt *ctx)
 	return 1;
 }
 
-SEC("cgroup/getsockopt/parent")
+SEC("cgroup/getsockopt")
 int _getsockopt_parent(struct bpf_sockopt *ctx)
 {
 	__u8 *optval_end = ctx->optval_end;
diff --git a/tools/testing/selftests/bpf/progs/test_cgroup_link.c b/tools/testing/selftests/bpf/progs/test_cgroup_link.c
index 77e47b9e4446..4faba88e45a5 100644
--- a/tools/testing/selftests/bpf/progs/test_cgroup_link.c
+++ b/tools/testing/selftests/bpf/progs/test_cgroup_link.c
@@ -6,14 +6,14 @@
 int calls = 0;
 int alt_calls = 0;
 
-SEC("cgroup_skb/egress1")
+SEC("cgroup_skb/egress")
 int egress(struct __sk_buff *skb)
 {
 	__sync_fetch_and_add(&calls, 1);
 	return 1;
 }
 
-SEC("cgroup_skb/egress2")
+SEC("cgroup_skb/egress")
 int egress_alt(struct __sk_buff *skb)
 {
 	__sync_fetch_and_add(&alt_calls, 1);
diff --git a/tools/testing/selftests/bpf/progs/test_misc_tcp_hdr_options.c b/tools/testing/selftests/bpf/progs/test_misc_tcp_hdr_options.c
index 6077a025092c..2c121c5d66a7 100644
--- a/tools/testing/selftests/bpf/progs/test_misc_tcp_hdr_options.c
+++ b/tools/testing/selftests/bpf/progs/test_misc_tcp_hdr_options.c
@@ -293,7 +293,7 @@ static int handle_passive_estab(struct bpf_sock_ops *skops)
 	return check_active_hdr_in(skops);
 }
 
-SEC("sockops/misc_estab")
+SEC("sockops")
 int misc_estab(struct bpf_sock_ops *skops)
 {
 	int true_val = 1;
diff --git a/tools/testing/selftests/bpf/progs/test_sk_lookup.c b/tools/testing/selftests/bpf/progs/test_sk_lookup.c
index ac6f7f205e25..6c4d32c56765 100644
--- a/tools/testing/selftests/bpf/progs/test_sk_lookup.c
+++ b/tools/testing/selftests/bpf/progs/test_sk_lookup.c
@@ -84,13 +84,13 @@ int lookup_drop(struct bpf_sk_lookup *ctx)
 	return SK_DROP;
 }
 
-SEC("sk_reuseport/reuse_pass")
+SEC("sk_reuseport")
 int reuseport_pass(struct sk_reuseport_md *ctx)
 {
 	return SK_PASS;
 }
 
-SEC("sk_reuseport/reuse_drop")
+SEC("sk_reuseport")
 int reuseport_drop(struct sk_reuseport_md *ctx)
 {
 	return SK_DROP;
@@ -194,7 +194,7 @@ int select_sock_a_no_reuseport(struct bpf_sk_lookup *ctx)
 	return err ? SK_DROP : SK_PASS;
 }
 
-SEC("sk_reuseport/select_sock_b")
+SEC("sk_reuseport")
 int select_sock_b(struct sk_reuseport_md *ctx)
 {
 	__u32 key = KEY_SERVER_B;
diff --git a/tools/testing/selftests/bpf/progs/test_sockmap_listen.c b/tools/testing/selftests/bpf/progs/test_sockmap_listen.c
index a1cc58b10c7c..00f1456aaeda 100644
--- a/tools/testing/selftests/bpf/progs/test_sockmap_listen.c
+++ b/tools/testing/selftests/bpf/progs/test_sockmap_listen.c
@@ -56,7 +56,7 @@ int prog_stream_verdict(struct __sk_buff *skb)
 	return verdict;
 }
 
-SEC("sk_skb/skb_verdict")
+SEC("sk_skb")
 int prog_skb_verdict(struct __sk_buff *skb)
 {
 	unsigned int *count;
diff --git a/tools/testing/selftests/bpf/progs/test_sockmap_skb_verdict_attach.c b/tools/testing/selftests/bpf/progs/test_sockmap_skb_verdict_attach.c
index 2d31f66e4f23..3c69aa971738 100644
--- a/tools/testing/selftests/bpf/progs/test_sockmap_skb_verdict_attach.c
+++ b/tools/testing/selftests/bpf/progs/test_sockmap_skb_verdict_attach.c
@@ -9,7 +9,7 @@ struct {
 	__type(value, __u64);
 } sock_map SEC(".maps");
 
-SEC("sk_skb/skb_verdict")
+SEC("sk_skb")
 int prog_skb_verdict(struct __sk_buff *skb)
 {
 	return SK_DROP;
diff --git a/tools/testing/selftests/bpf/progs/test_tcp_check_syncookie_kern.c b/tools/testing/selftests/bpf/progs/test_tcp_check_syncookie_kern.c
index fac7ef99f9a6..6be6d12cc729 100644
--- a/tools/testing/selftests/bpf/progs/test_tcp_check_syncookie_kern.c
+++ b/tools/testing/selftests/bpf/progs/test_tcp_check_syncookie_kern.c
@@ -148,7 +148,7 @@ static __always_inline void check_syncookie(void *ctx, void *data,
 	bpf_sk_release(sk);
 }
 
-SEC("clsact/check_syncookie")
+SEC("classifier")
 int check_syncookie_clsact(struct __sk_buff *skb)
 {
 	check_syncookie(skb, (void *)(long)skb->data,
diff --git a/tools/testing/selftests/bpf/progs/test_tcp_hdr_options.c b/tools/testing/selftests/bpf/progs/test_tcp_hdr_options.c
index 678bd0fad29e..5f4e87ee949a 100644
--- a/tools/testing/selftests/bpf/progs/test_tcp_hdr_options.c
+++ b/tools/testing/selftests/bpf/progs/test_tcp_hdr_options.c
@@ -594,7 +594,7 @@ static int handle_parse_hdr(struct bpf_sock_ops *skops)
 	return CG_OK;
 }
 
-SEC("sockops/estab")
+SEC("sockops")
 int estab(struct bpf_sock_ops *skops)
 {
 	int true_val = 1;
diff --git a/tools/testing/selftests/bpf/test_tcp_check_syncookie.sh b/tools/testing/selftests/bpf/test_tcp_check_syncookie.sh
index fed765157c53..47c6ff7b519b 100755
--- a/tools/testing/selftests/bpf/test_tcp_check_syncookie.sh
+++ b/tools/testing/selftests/bpf/test_tcp_check_syncookie.sh
@@ -76,7 +76,7 @@ DIR=$(dirname $0)
 TEST_IF=lo
 MAX_PING_TRIES=5
 BPF_PROG_OBJ="${DIR}/test_tcp_check_syncookie_kern.o"
-CLSACT_SECTION="clsact/check_syncookie"
+CLSACT_SECTION="classifier"
 XDP_SECTION="xdp"
 BPF_PROG_ID=0
 PROG="${DIR}/test_tcp_check_syncookie_user"
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH v2 bpf-next 4/9] libbpf: refactor internal sec_def handling to enable pluggability
  2021-09-20 23:43 [PATCH v2 bpf-next 0/9] libbpf: stricter BPF program section name handling Andrii Nakryiko
                   ` (2 preceding siblings ...)
  2021-09-20 23:43 ` [PATCH v2 bpf-next 3/9] selftests/bpf: normalize all the rest SEC() uses Andrii Nakryiko
@ 2021-09-20 23:43 ` Andrii Nakryiko
  2021-09-22  0:42   ` Dave Marchevsky
  2021-09-20 23:43 ` [PATCH v2 bpf-next 5/9] libbpf: reduce reliance of attach_fns on sec_def internals Andrii Nakryiko
                   ` (4 subsequent siblings)
  8 siblings, 1 reply; 27+ messages in thread
From: Andrii Nakryiko @ 2021-09-20 23:43 UTC (permalink / raw)
  To: bpf, ast, daniel; +Cc: andrii, kernel-team

Refactor internals of libbpf to allow adding custom SEC() handling logic
easily from outside of libbpf. To that effect, each SEC()-handling
registration sets mandatory program type/expected attach type for
a given prefix and can provide three callbacks called at different
points of BPF program lifetime:

  - init callback for right after bpf_program is initialized and
  prog_type/expected_attach_type is set. This happens during
  bpf_object__open() step, close to the very end of constructing
  bpf_object, so all the libbpf APIs for querying and updating
  bpf_program properties should be available;

  - pre-load callback is called right before BPF_PROG_LOAD command is
  called in the kernel. This callbacks has ability to set both
  bpf_program properties, as well as program load attributes, overriding
  and augmenting the standard libbpf handling of them;

  - optional auto-attach callback, which makes a given SEC() handler
  support auto-attachment of a BPF program through bpf_program__attach()
  API and/or BPF skeletons <skel>__attach() method.

Each callbacks gets a `long cookie` parameter passed in, which is
specified during SEC() handling. This can be used by callbacks to lookup
whatever additional information is necessary.

This is not yet completely ready to be exposed to the outside world,
mainly due to non-public nature of struct bpf_prog_load_params. Instead
of making it part of public API, we'll wait until the planned low-level
libbpf API improvements for BPF_PROG_LOAD and other typical bpf()
syscall APIs, at which point we'll have a public, probably OPTS-based,
way to fully specify BPF program load parameters, which will be used as
an interface for custom pre-load callbacks.

But this change itself is already a good first step to unify the BPF
program hanling logic even within the libbpf itself. As one example, all
the extra per-program type handling (sleepable bit, attach_btf_id
resolution, unsetting optional expected attach type) is now more obvious
and is gathered in one place.

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 tools/lib/bpf/libbpf.c | 129 +++++++++++++++++++++++++++--------------
 1 file changed, 87 insertions(+), 42 deletions(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index da65a1666a5e..8ab2edbf7a3b 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -220,7 +220,9 @@ struct reloc_desc {
 
 struct bpf_sec_def;
 
-typedef struct bpf_link *(*attach_fn_t)(const struct bpf_program *prog);
+typedef int (*init_fn_t)(struct bpf_program *prog, long cookie);
+typedef int (*preload_fn_t)(struct bpf_program *prog, struct bpf_prog_load_params *attr, long cookie);
+typedef struct bpf_link *(*attach_fn_t)(const struct bpf_program *prog, long cookie);
 
 struct bpf_sec_def {
 	const char *sec;
@@ -231,7 +233,11 @@ struct bpf_sec_def {
 	bool is_attachable;
 	bool is_attach_btf;
 	bool is_sleepable;
+
+	init_fn_t init_fn;
+	preload_fn_t preload_fn;
 	attach_fn_t attach_fn;
+	long cookie;
 };
 
 /*
@@ -6094,6 +6100,44 @@ static int bpf_object__sanitize_prog(struct bpf_object *obj, struct bpf_program
 	return 0;
 }
 
+static int libbpf_find_attach_btf_id(struct bpf_program *prog, int *btf_obj_fd, int *btf_type_id);
+
+/* this is called as prog->sec_def->preload_fn for libbpf-supported sec_defs */
+static int libbpf_preload_prog(struct bpf_program *prog,
+			       struct bpf_prog_load_params *attr, long cookie)
+{
+	/* old kernels might not support specifying expected_attach_type */
+	if (prog->sec_def->is_exp_attach_type_optional &&
+	    !kernel_supports(prog->obj, FEAT_EXP_ATTACH_TYPE))
+		attr->expected_attach_type = 0;
+
+	if (prog->sec_def->is_sleepable)
+		attr->prog_flags |= BPF_F_SLEEPABLE;
+
+	if ((prog->type == BPF_PROG_TYPE_TRACING ||
+	     prog->type == BPF_PROG_TYPE_LSM ||
+	     prog->type == BPF_PROG_TYPE_EXT) && !prog->attach_btf_id) {
+		int btf_obj_fd = 0, btf_type_id = 0, err;
+
+		err = libbpf_find_attach_btf_id(prog, &btf_obj_fd, &btf_type_id);
+		if (err)
+			return err;
+
+		/* cache resolved BTF FD and BTF type ID in the prog */
+		prog->attach_btf_obj_fd = btf_obj_fd;
+		prog->attach_btf_id = btf_type_id;
+
+		/* but by now libbpf common logic is not utilizing
+		 * prog->atach_btf_obj_fd/prog->attach_btf_id anymore because
+		 * this callback is called after attrs were populated by
+		 * libbpf, so this callback has to update attr explicitly here
+		 */
+		attr->attach_btf_obj_fd = btf_obj_fd;
+		attr->attach_btf_id = btf_type_id;
+	}
+	return 0;
+}
+
 static int
 load_program(struct bpf_program *prog, struct bpf_insn *insns, int insns_cnt,
 	     char *license, __u32 kern_version, int *pfd)
@@ -6102,7 +6146,7 @@ load_program(struct bpf_program *prog, struct bpf_insn *insns, int insns_cnt,
 	char *cp, errmsg[STRERR_BUFSIZE];
 	size_t log_buf_size = 0;
 	char *log_buf = NULL;
-	int btf_fd, ret;
+	int btf_fd, ret, err;
 
 	if (prog->type == BPF_PROG_TYPE_UNSPEC) {
 		/*
@@ -6118,22 +6162,15 @@ load_program(struct bpf_program *prog, struct bpf_insn *insns, int insns_cnt,
 		return -EINVAL;
 
 	load_attr.prog_type = prog->type;
-	/* old kernels might not support specifying expected_attach_type */
-	if (!kernel_supports(prog->obj, FEAT_EXP_ATTACH_TYPE) && prog->sec_def &&
-	    prog->sec_def->is_exp_attach_type_optional)
-		load_attr.expected_attach_type = 0;
-	else
-		load_attr.expected_attach_type = prog->expected_attach_type;
+	load_attr.expected_attach_type = prog->expected_attach_type;
 	if (kernel_supports(prog->obj, FEAT_PROG_NAME))
 		load_attr.name = prog->name;
 	load_attr.insns = insns;
 	load_attr.insn_cnt = insns_cnt;
 	load_attr.license = license;
 	load_attr.attach_btf_id = prog->attach_btf_id;
-	if (prog->attach_prog_fd)
-		load_attr.attach_prog_fd = prog->attach_prog_fd;
-	else
-		load_attr.attach_btf_obj_fd = prog->attach_btf_obj_fd;
+	load_attr.attach_prog_fd = prog->attach_prog_fd;
+	load_attr.attach_btf_obj_fd = prog->attach_btf_obj_fd;
 	load_attr.attach_btf_id = prog->attach_btf_id;
 	load_attr.kern_version = kern_version;
 	load_attr.prog_ifindex = prog->prog_ifindex;
@@ -6152,6 +6189,16 @@ load_program(struct bpf_program *prog, struct bpf_insn *insns, int insns_cnt,
 	load_attr.log_level = prog->log_level;
 	load_attr.prog_flags = prog->prog_flags;
 
+	/* adjust load_attr if sec_def provides custom preload callback */
+	if (prog->sec_def && prog->sec_def->preload_fn) {
+		err = prog->sec_def->preload_fn(prog, &load_attr, prog->sec_def->cookie);
+		if (err < 0) {
+			pr_warn("prog '%s': failed to prepare load attributes: %d\n",
+				prog->name, err);
+			return err;
+		}
+	}
+
 	if (prog->obj->gen_loader) {
 		bpf_gen__prog_load(prog->obj->gen_loader, &load_attr,
 				   prog - prog->obj->programs);
@@ -6267,8 +6314,6 @@ static int bpf_program__record_externs(struct bpf_program *prog)
 	return 0;
 }
 
-static int libbpf_find_attach_btf_id(struct bpf_program *prog, int *btf_obj_fd, int *btf_type_id);
-
 int bpf_program__load(struct bpf_program *prog, char *license, __u32 kern_ver)
 {
 	int err = 0, fd, i;
@@ -6278,19 +6323,6 @@ int bpf_program__load(struct bpf_program *prog, char *license, __u32 kern_ver)
 		return libbpf_err(-EINVAL);
 	}
 
-	if ((prog->type == BPF_PROG_TYPE_TRACING ||
-	     prog->type == BPF_PROG_TYPE_LSM ||
-	     prog->type == BPF_PROG_TYPE_EXT) && !prog->attach_btf_id) {
-		int btf_obj_fd = 0, btf_type_id = 0;
-
-		err = libbpf_find_attach_btf_id(prog, &btf_obj_fd, &btf_type_id);
-		if (err)
-			return libbpf_err(err);
-
-		prog->attach_btf_obj_fd = btf_obj_fd;
-		prog->attach_btf_id = btf_type_id;
-	}
-
 	if (prog->instances.nr < 0 || !prog->instances.fds) {
 		if (prog->preprocessor) {
 			pr_warn("Internal error: can't load program '%s'\n",
@@ -6400,6 +6432,7 @@ static const struct bpf_sec_def *find_sec_def(const char *sec_name);
 static int bpf_object_init_progs(struct bpf_object *obj, const struct bpf_object_open_opts *opts)
 {
 	struct bpf_program *prog;
+	int err;
 
 	bpf_object__for_each_program(prog, obj) {
 		prog->sec_def = find_sec_def(prog->sec_name);
@@ -6410,8 +6443,6 @@ static int bpf_object_init_progs(struct bpf_object *obj, const struct bpf_object
 			continue;
 		}
 
-		if (prog->sec_def->is_sleepable)
-			prog->prog_flags |= BPF_F_SLEEPABLE;
 		bpf_program__set_type(prog, prog->sec_def->prog_type);
 		bpf_program__set_expected_attach_type(prog, prog->sec_def->expected_attach_type);
 
@@ -6421,6 +6452,18 @@ static int bpf_object_init_progs(struct bpf_object *obj, const struct bpf_object
 		    prog->sec_def->prog_type == BPF_PROG_TYPE_EXT)
 			prog->attach_prog_fd = OPTS_GET(opts, attach_prog_fd, 0);
 #pragma GCC diagnostic pop
+
+		/* sec_def can have custom callback which should be called
+		 * after bpf_program is initialized to adjust its properties
+		 */
+		if (prog->sec_def->init_fn) {
+			err = prog->sec_def->init_fn(prog, prog->sec_def->cookie);
+			if (err < 0) {
+				pr_warn("prog '%s': failed to initialize: %d\n",
+					prog->name, err);
+				return err;
+			}
+		}
 	}
 
 	return 0;
@@ -7918,6 +7961,7 @@ void bpf_program__set_expected_attach_type(struct bpf_program *prog,
 		.is_exp_attach_type_optional = eatype_optional,		    \
 		.is_attachable = attachable,				    \
 		.is_attach_btf = attach_btf,				    \
+		.preload_fn = libbpf_preload_prog,			    \
 	}
 
 /* Programs that can NOT be attached. */
@@ -7944,15 +7988,16 @@ void bpf_program__set_expected_attach_type(struct bpf_program *prog,
 	.sec = sec_pfx,							    \
 	.len = sizeof(sec_pfx) - 1,					    \
 	.prog_type = BPF_PROG_TYPE_##ptype,				    \
+	.preload_fn = libbpf_preload_prog,				    \
 	__VA_ARGS__							    \
 }
 
-static struct bpf_link *attach_kprobe(const struct bpf_program *prog);
-static struct bpf_link *attach_tp(const struct bpf_program *prog);
-static struct bpf_link *attach_raw_tp(const struct bpf_program *prog);
-static struct bpf_link *attach_trace(const struct bpf_program *prog);
-static struct bpf_link *attach_lsm(const struct bpf_program *prog);
-static struct bpf_link *attach_iter(const struct bpf_program *prog);
+static struct bpf_link *attach_kprobe(const struct bpf_program *prog, long cookie);
+static struct bpf_link *attach_tp(const struct bpf_program *prog, long cookie);
+static struct bpf_link *attach_raw_tp(const struct bpf_program *prog, long cookie);
+static struct bpf_link *attach_trace(const struct bpf_program *prog, long cookie);
+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[] = {
 	BPF_PROG_SEC("socket",			BPF_PROG_TYPE_SOCKET_FILTER),
@@ -9400,7 +9445,7 @@ struct bpf_link *bpf_program__attach_kprobe(const struct bpf_program *prog,
 	return bpf_program__attach_kprobe_opts(prog, func_name, &opts);
 }
 
-static struct bpf_link *attach_kprobe(const struct bpf_program *prog)
+static struct bpf_link *attach_kprobe(const struct bpf_program *prog, long cookie)
 {
 	DECLARE_LIBBPF_OPTS(bpf_kprobe_opts, opts);
 	unsigned long offset = 0;
@@ -9573,7 +9618,7 @@ struct bpf_link *bpf_program__attach_tracepoint(const struct bpf_program *prog,
 	return bpf_program__attach_tracepoint_opts(prog, tp_category, tp_name, NULL);
 }
 
-static struct bpf_link *attach_tp(const struct bpf_program *prog)
+static struct bpf_link *attach_tp(const struct bpf_program *prog, long cookie)
 {
 	char *sec_name, *tp_cat, *tp_name;
 	struct bpf_link *link;
@@ -9627,7 +9672,7 @@ struct bpf_link *bpf_program__attach_raw_tracepoint(const struct bpf_program *pr
 	return link;
 }
 
-static struct bpf_link *attach_raw_tp(const struct bpf_program *prog)
+static struct bpf_link *attach_raw_tp(const struct bpf_program *prog, long cookie)
 {
 	const char *tp_name = prog->sec_name + prog->sec_def->len;
 
@@ -9674,12 +9719,12 @@ struct bpf_link *bpf_program__attach_lsm(const struct bpf_program *prog)
 	return bpf_program__attach_btf_id(prog);
 }
 
-static struct bpf_link *attach_trace(const struct bpf_program *prog)
+static struct bpf_link *attach_trace(const struct bpf_program *prog, long cookie)
 {
 	return bpf_program__attach_trace(prog);
 }
 
-static struct bpf_link *attach_lsm(const struct bpf_program *prog)
+static struct bpf_link *attach_lsm(const struct bpf_program *prog, long cookie)
 {
 	return bpf_program__attach_lsm(prog);
 }
@@ -9810,7 +9855,7 @@ bpf_program__attach_iter(const struct bpf_program *prog,
 	return link;
 }
 
-static struct bpf_link *attach_iter(const struct bpf_program *prog)
+static struct bpf_link *attach_iter(const struct bpf_program *prog, long cookie)
 {
 	return bpf_program__attach_iter(prog, NULL);
 }
@@ -9820,7 +9865,7 @@ struct bpf_link *bpf_program__attach(const struct bpf_program *prog)
 	if (!prog->sec_def || !prog->sec_def->attach_fn)
 		return libbpf_err_ptr(-ESRCH);
 
-	return prog->sec_def->attach_fn(prog);
+	return prog->sec_def->attach_fn(prog, prog->sec_def->cookie);
 }
 
 static int bpf_link__detach_struct_ops(struct bpf_link *link)
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH v2 bpf-next 5/9] libbpf: reduce reliance of attach_fns on sec_def internals
  2021-09-20 23:43 [PATCH v2 bpf-next 0/9] libbpf: stricter BPF program section name handling Andrii Nakryiko
                   ` (3 preceding siblings ...)
  2021-09-20 23:43 ` [PATCH v2 bpf-next 4/9] libbpf: refactor internal sec_def handling to enable pluggability Andrii Nakryiko
@ 2021-09-20 23:43 ` Andrii Nakryiko
  2021-09-22  1:00   ` Dave Marchevsky
  2021-09-20 23:43 ` [PATCH v2 bpf-next 6/9] libbpf: refactor ELF section handler definitions Andrii Nakryiko
                   ` (3 subsequent siblings)
  8 siblings, 1 reply; 27+ messages in thread
From: Andrii Nakryiko @ 2021-09-20 23:43 UTC (permalink / raw)
  To: bpf, ast, daniel; +Cc: andrii, kernel-team

Move closer to not relying on bpf_sec_def internals that won't be part
of public API, when pluggable SEC() handlers will be allowed. Drop
pre-calculated prefix length, and in various helpers don't rely on this
prefix length availability. Also minimize reliance on knowing
bpf_sec_def's prefix for few places where section prefix shortcuts are
supported (e.g., tp vs tracepoint, raw_tp vs raw_tracepoint).

Given checking some string for having a given string-constant prefix is
such a common operation and so annoying to be done with pure C code, add
a small macro helper, str_has_pfx(), and reuse it throughout libbpf.c
where prefix comparison is performed. With __builtin_constant_p() it's
possible to have a convenient helper that checks some string for having
a given prefix, where prefix is either string literal (or compile-time
known string due to compiler optimization) or just a runtime string
pointer, which is quite convenient and saves a lot of typing and string
literal duplication.

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 tools/lib/bpf/libbpf.c          | 41 ++++++++++++++++++---------------
 tools/lib/bpf/libbpf_internal.h |  7 ++++++
 2 files changed, 30 insertions(+), 18 deletions(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 8ab2edbf7a3b..52c398fae0af 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -226,7 +226,6 @@ typedef struct bpf_link *(*attach_fn_t)(const struct bpf_program *prog, long coo
 
 struct bpf_sec_def {
 	const char *sec;
-	size_t len;
 	enum bpf_prog_type prog_type;
 	enum bpf_attach_type expected_attach_type;
 	bool is_exp_attach_type_optional;
@@ -1671,7 +1670,7 @@ static int bpf_object__process_kconfig_line(struct bpf_object *obj,
 	void *ext_val;
 	__u64 num;
 
-	if (strncmp(buf, "CONFIG_", 7))
+	if (!str_has_pfx(buf, "CONFIG_"))
 		return 0;
 
 	sep = strchr(buf, '=');
@@ -2919,7 +2918,7 @@ static Elf_Data *elf_sec_data(const struct bpf_object *obj, Elf_Scn *scn)
 static bool is_sec_name_dwarf(const char *name)
 {
 	/* approximation, but the actual list is too long */
-	return strncmp(name, ".debug_", sizeof(".debug_") - 1) == 0;
+	return str_has_pfx(name, ".debug_");
 }
 
 static bool ignore_elf_section(GElf_Shdr *hdr, const char *name)
@@ -2941,7 +2940,7 @@ static bool ignore_elf_section(GElf_Shdr *hdr, const char *name)
 	if (is_sec_name_dwarf(name))
 		return true;
 
-	if (strncmp(name, ".rel", sizeof(".rel") - 1) == 0) {
+	if (str_has_pfx(name, ".rel")) {
 		name += sizeof(".rel") - 1;
 		/* DWARF section relocations */
 		if (is_sec_name_dwarf(name))
@@ -6890,8 +6889,7 @@ static int bpf_object__resolve_externs(struct bpf_object *obj,
 			if (err)
 				return err;
 			pr_debug("extern (kcfg) %s=0x%x\n", ext->name, kver);
-		} else if (ext->type == EXT_KCFG &&
-			   strncmp(ext->name, "CONFIG_", 7) == 0) {
+		} else if (ext->type == EXT_KCFG && str_has_pfx(ext->name, "CONFIG_")) {
 			need_config = true;
 		} else if (ext->type == EXT_KSYM) {
 			if (ext->ksym.type_id)
@@ -7955,7 +7953,6 @@ void bpf_program__set_expected_attach_type(struct bpf_program *prog,
 			  attachable, attach_btf)			    \
 	{								    \
 		.sec = string,						    \
-		.len = sizeof(string) - 1,				    \
 		.prog_type = ptype,					    \
 		.expected_attach_type = eatype,				    \
 		.is_exp_attach_type_optional = eatype_optional,		    \
@@ -7986,7 +7983,6 @@ void bpf_program__set_expected_attach_type(struct bpf_program *prog,
 
 #define SEC_DEF(sec_pfx, ptype, ...) {					    \
 	.sec = sec_pfx,							    \
-	.len = sizeof(sec_pfx) - 1,					    \
 	.prog_type = BPF_PROG_TYPE_##ptype,				    \
 	.preload_fn = libbpf_preload_prog,				    \
 	__VA_ARGS__							    \
@@ -8160,10 +8156,8 @@ static const struct bpf_sec_def *find_sec_def(const char *sec_name)
 	int i, n = ARRAY_SIZE(section_defs);
 
 	for (i = 0; i < n; i++) {
-		if (strncmp(sec_name,
-			    section_defs[i].sec, section_defs[i].len))
-			continue;
-		return &section_defs[i];
+		if (str_has_pfx(sec_name, section_defs[i].sec))
+			return &section_defs[i];
 	}
 	return NULL;
 }
@@ -8517,7 +8511,7 @@ static int libbpf_find_attach_btf_id(struct bpf_program *prog, int *btf_obj_fd,
 			prog->sec_name);
 		return -ESRCH;
 	}
-	attach_name = prog->sec_name + prog->sec_def->len;
+	attach_name = prog->sec_name + strlen(prog->sec_def->sec);
 
 	/* BPF program's BTF ID */
 	if (attach_prog_fd) {
@@ -9454,8 +9448,11 @@ static struct bpf_link *attach_kprobe(const struct bpf_program *prog, long cooki
 	char *func;
 	int n, err;
 
-	func_name = prog->sec_name + prog->sec_def->len;
-	opts.retprobe = strcmp(prog->sec_def->sec, "kretprobe/") == 0;
+	opts.retprobe = str_has_pfx(prog->sec_name, "kretprobe/");
+	if (opts.retprobe)
+		func_name = prog->sec_name + sizeof("kretprobe/") - 1;
+	else
+		func_name = prog->sec_name + sizeof("kprobe/") - 1;
 
 	n = sscanf(func_name, "%m[a-zA-Z0-9_.]+%li", &func, &offset);
 	if (n < 1) {
@@ -9627,8 +9624,11 @@ static struct bpf_link *attach_tp(const struct bpf_program *prog, long cookie)
 	if (!sec_name)
 		return libbpf_err_ptr(-ENOMEM);
 
-	/* extract "tp/<category>/<name>" */
-	tp_cat = sec_name + prog->sec_def->len;
+	/* extract "tp/<category>/<name>" or "tracepoint/<category>/<name>" */
+	if (str_has_pfx(prog->sec_name, "tp/"))
+		tp_cat = sec_name + sizeof("tp/") - 1;
+	else
+		tp_cat = sec_name + sizeof("tracepoint/") - 1;
 	tp_name = strchr(tp_cat, '/');
 	if (!tp_name) {
 		free(sec_name);
@@ -9674,7 +9674,12 @@ struct bpf_link *bpf_program__attach_raw_tracepoint(const struct bpf_program *pr
 
 static struct bpf_link *attach_raw_tp(const struct bpf_program *prog, long cookie)
 {
-	const char *tp_name = prog->sec_name + prog->sec_def->len;
+	const char *tp_name;
+
+	if (str_has_pfx(prog->sec_name, "raw_tp/"))
+		tp_name = prog->sec_name + sizeof("raw_tp/") - 1;
+	else
+		tp_name = prog->sec_name + sizeof("raw_tracepoint/") - 1;
 
 	return bpf_program__attach_raw_tracepoint(prog, tp_name);
 }
diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
index ceb0c98979bc..ec79400517d4 100644
--- a/tools/lib/bpf/libbpf_internal.h
+++ b/tools/lib/bpf/libbpf_internal.h
@@ -89,6 +89,13 @@
 	(offsetof(TYPE, FIELD) + sizeof(((TYPE *)0)->FIELD))
 #endif
 
+/* Check whether a string `str` has prefix `pfx`, regardless if `pfx` is
+ * a string literal known at compilation time or char * pointer known only at
+ * runtime.
+ */
+#define str_has_pfx(str, pfx) \
+	(strncmp(str, pfx, __builtin_constant_p(pfx) ? sizeof(pfx) - 1 : strlen(pfx)) == 0)
+
 /* Symbol versioning is different between static and shared library.
  * Properly versioned symbols are needed for shared library, but
  * only the symbol of the new version is needed for static library.
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH v2 bpf-next 6/9] libbpf: refactor ELF section handler definitions
  2021-09-20 23:43 [PATCH v2 bpf-next 0/9] libbpf: stricter BPF program section name handling Andrii Nakryiko
                   ` (4 preceding siblings ...)
  2021-09-20 23:43 ` [PATCH v2 bpf-next 5/9] libbpf: reduce reliance of attach_fns on sec_def internals Andrii Nakryiko
@ 2021-09-20 23:43 ` Andrii Nakryiko
  2021-09-22  1:34   ` Dave Marchevsky
  2021-09-20 23:43 ` [PATCH v2 bpf-next 7/9] libbpf: complete SEC() table unification for BPF_APROG_SEC/BPF_EAPROG_SEC Andrii Nakryiko
                   ` (2 subsequent siblings)
  8 siblings, 1 reply; 27+ messages in thread
From: Andrii Nakryiko @ 2021-09-20 23:43 UTC (permalink / raw)
  To: bpf, ast, daniel; +Cc: andrii, kernel-team

Refactor ELF section handler definitions table to use a set of flags and
unified SEC_DEF() macro. This allows for more succinct and table-like
set of definitions, and allows to more easily extend the logic without
adding more verbosity (this is utilized in later patches in the series).

This approach is also making libbpf-internal program pre-load callback
not rely on bpf_sec_def definition, which demonstrates that future
pluggable ELF section handlers will be able to achieve similar level of
integration without libbpf having to expose extra types and APIs.

For starters, update SEC_DEF() definitions and make them more succinct.
Also convert BPF_PROG_SEC() and BPF_APROG_COMPAT() definitions to
a common SEC_DEF() use.

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 tools/lib/bpf/libbpf.c | 183 ++++++++++++++++-------------------------
 1 file changed, 73 insertions(+), 110 deletions(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 52c398fae0af..734be7dc52a0 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -224,19 +224,25 @@ typedef int (*init_fn_t)(struct bpf_program *prog, long cookie);
 typedef int (*preload_fn_t)(struct bpf_program *prog, struct bpf_prog_load_params *attr, long cookie);
 typedef struct bpf_link *(*attach_fn_t)(const struct bpf_program *prog, long cookie);
 
+/* stored as sec_def->cookie for all libbpf-supported SEC()s */
+enum sec_def_flags {
+	SEC_NONE = 0,
+	SEC_EXP_ATTACH_OPT = 1,
+	SEC_ATTACHABLE = 2,
+	SEC_ATTACHABLE_OPT = SEC_ATTACHABLE | SEC_EXP_ATTACH_OPT,
+	SEC_ATTACH_BTF = 4,
+	SEC_SLEEPABLE = 8,
+};
+
 struct bpf_sec_def {
 	const char *sec;
 	enum bpf_prog_type prog_type;
 	enum bpf_attach_type expected_attach_type;
-	bool is_exp_attach_type_optional;
-	bool is_attachable;
-	bool is_attach_btf;
-	bool is_sleepable;
+	long cookie;
 
 	init_fn_t init_fn;
 	preload_fn_t preload_fn;
 	attach_fn_t attach_fn;
-	long cookie;
 };
 
 /*
@@ -6099,26 +6105,30 @@ static int bpf_object__sanitize_prog(struct bpf_object *obj, struct bpf_program
 	return 0;
 }
 
-static int libbpf_find_attach_btf_id(struct bpf_program *prog, int *btf_obj_fd, int *btf_type_id);
+static int libbpf_find_attach_btf_id(struct bpf_program *prog, const char *attach_name,
+				     int *btf_obj_fd, int *btf_type_id);
 
 /* this is called as prog->sec_def->preload_fn for libbpf-supported sec_defs */
 static int libbpf_preload_prog(struct bpf_program *prog,
 			       struct bpf_prog_load_params *attr, long cookie)
 {
+	enum sec_def_flags def = cookie;
+
 	/* old kernels might not support specifying expected_attach_type */
-	if (prog->sec_def->is_exp_attach_type_optional &&
-	    !kernel_supports(prog->obj, FEAT_EXP_ATTACH_TYPE))
+	if ((def & SEC_EXP_ATTACH_OPT) && !kernel_supports(prog->obj, FEAT_EXP_ATTACH_TYPE))
 		attr->expected_attach_type = 0;
 
-	if (prog->sec_def->is_sleepable)
+	if (def & SEC_SLEEPABLE)
 		attr->prog_flags |= BPF_F_SLEEPABLE;
 
 	if ((prog->type == BPF_PROG_TYPE_TRACING ||
 	     prog->type == BPF_PROG_TYPE_LSM ||
 	     prog->type == BPF_PROG_TYPE_EXT) && !prog->attach_btf_id) {
 		int btf_obj_fd = 0, btf_type_id = 0, err;
+		const char *attach_name;
 
-		err = libbpf_find_attach_btf_id(prog, &btf_obj_fd, &btf_type_id);
+		attach_name = strchr(prog->sec_name, '/') + 1;
+		err = libbpf_find_attach_btf_id(prog, attach_name, &btf_obj_fd, &btf_type_id);
 		if (err)
 			return err;
 
@@ -7955,15 +7965,14 @@ void bpf_program__set_expected_attach_type(struct bpf_program *prog,
 		.sec = string,						    \
 		.prog_type = ptype,					    \
 		.expected_attach_type = eatype,				    \
-		.is_exp_attach_type_optional = eatype_optional,		    \
-		.is_attachable = attachable,				    \
-		.is_attach_btf = attach_btf,				    \
+		.cookie = (long) (					    \
+			(eatype_optional ? SEC_EXP_ATTACH_OPT : 0) |   \
+			(attachable ? SEC_ATTACHABLE : 0) |		    \
+			(attach_btf ? SEC_ATTACH_BTF : 0)		    \
+		),							    \
 		.preload_fn = libbpf_preload_prog,			    \
 	}
 
-/* Programs that can NOT be attached. */
-#define BPF_PROG_SEC(string, ptype) BPF_PROG_SEC_IMPL(string, ptype, 0, 0, 0, 0)
-
 /* Programs that can be attached. */
 #define BPF_APROG_SEC(string, ptype, atype) \
 	BPF_PROG_SEC_IMPL(string, ptype, atype, true, 1, 0)
@@ -7976,14 +7985,11 @@ void bpf_program__set_expected_attach_type(struct bpf_program *prog,
 #define BPF_PROG_BTF(string, ptype, eatype) \
 	BPF_PROG_SEC_IMPL(string, ptype, eatype, false, 0, 1)
 
-/* Programs that can be attached but attach type can't be identified by section
- * name. Kept for backward compatibility.
- */
-#define BPF_APROG_COMPAT(string, ptype) BPF_PROG_SEC(string, ptype)
-
-#define SEC_DEF(sec_pfx, ptype, ...) {					    \
+#define SEC_DEF(sec_pfx, ptype, atype, flags, ...) {			    \
 	.sec = sec_pfx,							    \
 	.prog_type = BPF_PROG_TYPE_##ptype,				    \
+	.expected_attach_type = atype,					    \
+	.cookie = (long)(flags),					    \
 	.preload_fn = libbpf_preload_prog,				    \
 	__VA_ARGS__							    \
 }
@@ -7996,92 +8002,49 @@ 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[] = {
-	BPF_PROG_SEC("socket",			BPF_PROG_TYPE_SOCKET_FILTER),
+	SEC_DEF("socket",		SOCKET_FILTER, 0, SEC_NONE),
 	BPF_EAPROG_SEC("sk_reuseport/migrate",	BPF_PROG_TYPE_SK_REUSEPORT,
 						BPF_SK_REUSEPORT_SELECT_OR_MIGRATE),
 	BPF_EAPROG_SEC("sk_reuseport",		BPF_PROG_TYPE_SK_REUSEPORT,
 						BPF_SK_REUSEPORT_SELECT),
-	SEC_DEF("kprobe/", KPROBE,
-		.attach_fn = attach_kprobe),
-	BPF_PROG_SEC("uprobe/",			BPF_PROG_TYPE_KPROBE),
-	SEC_DEF("kretprobe/", KPROBE,
-		.attach_fn = attach_kprobe),
-	BPF_PROG_SEC("uretprobe/",		BPF_PROG_TYPE_KPROBE),
-	BPF_PROG_SEC("classifier",		BPF_PROG_TYPE_SCHED_CLS),
-	BPF_PROG_SEC("action",			BPF_PROG_TYPE_SCHED_ACT),
-	SEC_DEF("tracepoint/", TRACEPOINT,
-		.attach_fn = attach_tp),
-	SEC_DEF("tp/", TRACEPOINT,
-		.attach_fn = attach_tp),
-	SEC_DEF("raw_tracepoint/", RAW_TRACEPOINT,
-		.attach_fn = attach_raw_tp),
-	SEC_DEF("raw_tp/", RAW_TRACEPOINT,
-		.attach_fn = attach_raw_tp),
-	SEC_DEF("tp_btf/", TRACING,
-		.expected_attach_type = BPF_TRACE_RAW_TP,
-		.is_attach_btf = true,
-		.attach_fn = attach_trace),
-	SEC_DEF("fentry/", TRACING,
-		.expected_attach_type = BPF_TRACE_FENTRY,
-		.is_attach_btf = true,
-		.attach_fn = attach_trace),
-	SEC_DEF("fmod_ret/", TRACING,
-		.expected_attach_type = BPF_MODIFY_RETURN,
-		.is_attach_btf = true,
-		.attach_fn = attach_trace),
-	SEC_DEF("fexit/", TRACING,
-		.expected_attach_type = BPF_TRACE_FEXIT,
-		.is_attach_btf = true,
-		.attach_fn = attach_trace),
-	SEC_DEF("fentry.s/", TRACING,
-		.expected_attach_type = BPF_TRACE_FENTRY,
-		.is_attach_btf = true,
-		.is_sleepable = true,
-		.attach_fn = attach_trace),
-	SEC_DEF("fmod_ret.s/", TRACING,
-		.expected_attach_type = BPF_MODIFY_RETURN,
-		.is_attach_btf = true,
-		.is_sleepable = true,
-		.attach_fn = attach_trace),
-	SEC_DEF("fexit.s/", TRACING,
-		.expected_attach_type = BPF_TRACE_FEXIT,
-		.is_attach_btf = true,
-		.is_sleepable = true,
-		.attach_fn = attach_trace),
-	SEC_DEF("freplace/", EXT,
-		.is_attach_btf = true,
-		.attach_fn = attach_trace),
-	SEC_DEF("lsm/", LSM,
-		.is_attach_btf = true,
-		.expected_attach_type = BPF_LSM_MAC,
-		.attach_fn = attach_lsm),
-	SEC_DEF("lsm.s/", LSM,
-		.is_attach_btf = true,
-		.is_sleepable = true,
-		.expected_attach_type = BPF_LSM_MAC,
-		.attach_fn = attach_lsm),
-	SEC_DEF("iter/", TRACING,
-		.expected_attach_type = BPF_TRACE_ITER,
-		.is_attach_btf = true,
-		.attach_fn = attach_iter),
-	SEC_DEF("syscall", SYSCALL,
-		.is_sleepable = true),
+	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("tracepoint/",		TRACEPOINT, 0, SEC_NONE, attach_tp),
+	SEC_DEF("tp/",			TRACEPOINT, 0, SEC_NONE, attach_tp),
+	SEC_DEF("raw_tracepoint/",	RAW_TRACEPOINT, 0, SEC_NONE, attach_raw_tp),
+	SEC_DEF("raw_tp/",		RAW_TRACEPOINT, 0, SEC_NONE, attach_raw_tp),
+	SEC_DEF("tp_btf/",		TRACING, BPF_TRACE_RAW_TP, SEC_ATTACH_BTF, attach_trace),
+	SEC_DEF("fentry/",		TRACING, BPF_TRACE_FENTRY, SEC_ATTACH_BTF, attach_trace),
+	SEC_DEF("fmod_ret/",		TRACING, BPF_MODIFY_RETURN, SEC_ATTACH_BTF, attach_trace),
+	SEC_DEF("fexit/",		TRACING, BPF_TRACE_FEXIT, SEC_ATTACH_BTF, attach_trace),
+	SEC_DEF("fentry.s/",		TRACING, BPF_TRACE_FENTRY, SEC_ATTACH_BTF | SEC_SLEEPABLE, attach_trace),
+	SEC_DEF("fmod_ret.s/",		TRACING, BPF_MODIFY_RETURN, SEC_ATTACH_BTF | SEC_SLEEPABLE, attach_trace),
+	SEC_DEF("fexit.s/",		TRACING, BPF_TRACE_FEXIT, SEC_ATTACH_BTF | SEC_SLEEPABLE, attach_trace),
+	SEC_DEF("freplace/",		EXT, 0, SEC_ATTACH_BTF, attach_trace),
+	SEC_DEF("lsm/",			LSM, BPF_LSM_MAC, SEC_ATTACH_BTF, attach_lsm),
+	SEC_DEF("lsm.s/",		LSM, BPF_LSM_MAC, SEC_ATTACH_BTF | SEC_SLEEPABLE, attach_lsm),
+	SEC_DEF("iter/",		TRACING, BPF_TRACE_ITER, SEC_ATTACH_BTF, attach_iter),
+	SEC_DEF("syscall",		SYSCALL, 0, SEC_SLEEPABLE),
 	BPF_EAPROG_SEC("xdp_devmap/",		BPF_PROG_TYPE_XDP,
 						BPF_XDP_DEVMAP),
 	BPF_EAPROG_SEC("xdp_cpumap/",		BPF_PROG_TYPE_XDP,
 						BPF_XDP_CPUMAP),
 	BPF_APROG_SEC("xdp",			BPF_PROG_TYPE_XDP,
 						BPF_XDP),
-	BPF_PROG_SEC("perf_event",		BPF_PROG_TYPE_PERF_EVENT),
-	BPF_PROG_SEC("lwt_in",			BPF_PROG_TYPE_LWT_IN),
-	BPF_PROG_SEC("lwt_out",			BPF_PROG_TYPE_LWT_OUT),
-	BPF_PROG_SEC("lwt_xmit",		BPF_PROG_TYPE_LWT_XMIT),
-	BPF_PROG_SEC("lwt_seg6local",		BPF_PROG_TYPE_LWT_SEG6LOCAL),
+	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),
 	BPF_APROG_SEC("cgroup_skb/ingress",	BPF_PROG_TYPE_CGROUP_SKB,
 						BPF_CGROUP_INET_INGRESS),
 	BPF_APROG_SEC("cgroup_skb/egress",	BPF_PROG_TYPE_CGROUP_SKB,
 						BPF_CGROUP_INET_EGRESS),
-	BPF_APROG_COMPAT("cgroup/skb",		BPF_PROG_TYPE_CGROUP_SKB),
+	SEC_DEF("cgroup/skb",		CGROUP_SKB, 0, SEC_NONE),
 	BPF_EAPROG_SEC("cgroup/sock_create",	BPF_PROG_TYPE_CGROUP_SOCK,
 						BPF_CGROUP_INET_SOCK_CREATE),
 	BPF_EAPROG_SEC("cgroup/sock_release",	BPF_PROG_TYPE_CGROUP_SOCK,
@@ -8100,7 +8063,7 @@ static const struct bpf_sec_def section_defs[] = {
 						BPF_SK_SKB_STREAM_PARSER),
 	BPF_APROG_SEC("sk_skb/stream_verdict",	BPF_PROG_TYPE_SK_SKB,
 						BPF_SK_SKB_STREAM_VERDICT),
-	BPF_APROG_COMPAT("sk_skb",		BPF_PROG_TYPE_SK_SKB),
+	SEC_DEF("sk_skb",		SK_SKB, 0, SEC_NONE),
 	BPF_APROG_SEC("sk_msg",			BPF_PROG_TYPE_SK_MSG,
 						BPF_SK_MSG_VERDICT),
 	BPF_APROG_SEC("lirc_mode2",		BPF_PROG_TYPE_LIRC_MODE2,
@@ -8137,16 +8100,14 @@ static const struct bpf_sec_def section_defs[] = {
 						BPF_CGROUP_GETSOCKOPT),
 	BPF_EAPROG_SEC("cgroup/setsockopt",	BPF_PROG_TYPE_CGROUP_SOCKOPT,
 						BPF_CGROUP_SETSOCKOPT),
-	BPF_PROG_SEC("struct_ops",		BPF_PROG_TYPE_STRUCT_OPS),
+	SEC_DEF("struct_ops",		STRUCT_OPS, 0, SEC_NONE),
 	BPF_EAPROG_SEC("sk_lookup/",		BPF_PROG_TYPE_SK_LOOKUP,
 						BPF_SK_LOOKUP),
 };
 
 #undef BPF_PROG_SEC_IMPL
-#undef BPF_PROG_SEC
 #undef BPF_APROG_SEC
 #undef BPF_EAPROG_SEC
-#undef BPF_APROG_COMPAT
 #undef SEC_DEF
 
 #define MAX_TYPE_NAME_SIZE 32
@@ -8174,8 +8135,15 @@ static char *libbpf_get_type_names(bool attach_type)
 	buf[0] = '\0';
 	/* Forge string buf with all available names */
 	for (i = 0; i < ARRAY_SIZE(section_defs); i++) {
-		if (attach_type && !section_defs[i].is_attachable)
-			continue;
+		const struct bpf_sec_def *sec_def = &section_defs[i];
+
+		if (attach_type) {
+			if (sec_def->preload_fn != libbpf_preload_prog)
+				continue;
+
+			if (!(sec_def->cookie & SEC_ATTACHABLE))
+				continue;
+		}
 
 		if (strlen(buf) + strlen(section_defs[i].sec) + 2 > len) {
 			free(buf);
@@ -8499,20 +8467,13 @@ static int find_kernel_btf_id(struct bpf_object *obj, const char *attach_name,
 	return -ESRCH;
 }
 
-static int libbpf_find_attach_btf_id(struct bpf_program *prog, int *btf_obj_fd, int *btf_type_id)
+static int libbpf_find_attach_btf_id(struct bpf_program *prog, const char *attach_name,
+				     int *btf_obj_fd, int *btf_type_id)
 {
 	enum bpf_attach_type attach_type = prog->expected_attach_type;
 	__u32 attach_prog_fd = prog->attach_prog_fd;
-	const char *attach_name;
 	int err = 0;
 
-	if (!prog->sec_def || !prog->sec_def->is_attach_btf) {
-		pr_warn("failed to identify BTF ID based on ELF section name '%s'\n",
-			prog->sec_name);
-		return -ESRCH;
-	}
-	attach_name = prog->sec_name + strlen(prog->sec_def->sec);
-
 	/* BPF program's BTF ID */
 	if (attach_prog_fd) {
 		err = libbpf_find_prog_btf_id(attach_name, attach_prog_fd);
@@ -8562,7 +8523,9 @@ int libbpf_attach_type_by_name(const char *name,
 		return libbpf_err(-EINVAL);
 	}
 
-	if (!sec_def->is_attachable)
+	if (sec_def->preload_fn != libbpf_preload_prog)
+		return libbpf_err(-EINVAL);
+	if (!(sec_def->cookie & SEC_ATTACHABLE))
 		return libbpf_err(-EINVAL);
 
 	*attach_type = sec_def->expected_attach_type;
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH v2 bpf-next 7/9] libbpf: complete SEC() table unification for BPF_APROG_SEC/BPF_EAPROG_SEC
  2021-09-20 23:43 [PATCH v2 bpf-next 0/9] libbpf: stricter BPF program section name handling Andrii Nakryiko
                   ` (5 preceding siblings ...)
  2021-09-20 23:43 ` [PATCH v2 bpf-next 6/9] libbpf: refactor ELF section handler definitions Andrii Nakryiko
@ 2021-09-20 23:43 ` Andrii Nakryiko
  2021-09-22  1:42   ` Dave Marchevsky
  2021-09-20 23:43 ` [PATCH v2 bpf-next 8/9] libbpf: add opt-in strict BPF program section name handling logic Andrii Nakryiko
  2021-09-20 23:43 ` [PATCH v2 bpf-next 9/9] selftests/bpf: switch sk_lookup selftests to strict SEC("sk_lookup") use Andrii Nakryiko
  8 siblings, 1 reply; 27+ messages in thread
From: Andrii Nakryiko @ 2021-09-20 23:43 UTC (permalink / raw)
  To: bpf, ast, daniel; +Cc: andrii, kernel-team

Complete SEC() table refactoring towards unified form by rewriting
BPF_APROG_SEC and BPF_EAPROG_SEC definitions with
SEC_DEF(SEC_ATTACHABLE_OPT) (for optional expected_attach_type) and
SEC_DEF(SEC_ATTACHABLE) (mandatory expected_attach_type), respectively.
Drop BPF_APROG_SEC, BPF_EAPROG_SEC, and BPF_PROG_SEC_IMPL macros after
that, leaving SEC_DEF() macro as the only one used.

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 tools/lib/bpf/libbpf.c | 136 +++++++++++------------------------------
 1 file changed, 35 insertions(+), 101 deletions(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 734be7dc52a0..56082865ceff 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -7959,32 +7959,6 @@ void bpf_program__set_expected_attach_type(struct bpf_program *prog,
 	prog->expected_attach_type = type;
 }
 
-#define BPF_PROG_SEC_IMPL(string, ptype, eatype, eatype_optional,	    \
-			  attachable, attach_btf)			    \
-	{								    \
-		.sec = string,						    \
-		.prog_type = ptype,					    \
-		.expected_attach_type = eatype,				    \
-		.cookie = (long) (					    \
-			(eatype_optional ? SEC_EXP_ATTACH_OPT : 0) |   \
-			(attachable ? SEC_ATTACHABLE : 0) |		    \
-			(attach_btf ? SEC_ATTACH_BTF : 0)		    \
-		),							    \
-		.preload_fn = libbpf_preload_prog,			    \
-	}
-
-/* Programs that can be attached. */
-#define BPF_APROG_SEC(string, ptype, atype) \
-	BPF_PROG_SEC_IMPL(string, ptype, atype, true, 1, 0)
-
-/* Programs that must specify expected attach type at load time. */
-#define BPF_EAPROG_SEC(string, ptype, eatype) \
-	BPF_PROG_SEC_IMPL(string, ptype, eatype, false, 1, 0)
-
-/* Programs that use BTF to identify attach point */
-#define BPF_PROG_BTF(string, ptype, eatype) \
-	BPF_PROG_SEC_IMPL(string, ptype, eatype, false, 0, 1)
-
 #define SEC_DEF(sec_pfx, ptype, atype, flags, ...) {			    \
 	.sec = sec_pfx,							    \
 	.prog_type = BPF_PROG_TYPE_##ptype,				    \
@@ -8003,10 +7977,8 @@ 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),
-	BPF_EAPROG_SEC("sk_reuseport/migrate",	BPF_PROG_TYPE_SK_REUSEPORT,
-						BPF_SK_REUSEPORT_SELECT_OR_MIGRATE),
-	BPF_EAPROG_SEC("sk_reuseport",		BPF_PROG_TYPE_SK_REUSEPORT,
-						BPF_SK_REUSEPORT_SELECT),
+	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("kprobe/",		KPROBE,	0, SEC_NONE, attach_kprobe),
 	SEC_DEF("uprobe/",		KPROBE,	0, SEC_NONE),
 	SEC_DEF("kretprobe/",		KPROBE, 0, SEC_NONE, attach_kprobe),
@@ -8029,87 +8001,49 @@ static const struct bpf_sec_def section_defs[] = {
 	SEC_DEF("lsm.s/",		LSM, BPF_LSM_MAC, SEC_ATTACH_BTF | SEC_SLEEPABLE, attach_lsm),
 	SEC_DEF("iter/",		TRACING, BPF_TRACE_ITER, SEC_ATTACH_BTF, attach_iter),
 	SEC_DEF("syscall",		SYSCALL, 0, SEC_SLEEPABLE),
-	BPF_EAPROG_SEC("xdp_devmap/",		BPF_PROG_TYPE_XDP,
-						BPF_XDP_DEVMAP),
-	BPF_EAPROG_SEC("xdp_cpumap/",		BPF_PROG_TYPE_XDP,
-						BPF_XDP_CPUMAP),
-	BPF_APROG_SEC("xdp",			BPF_PROG_TYPE_XDP,
-						BPF_XDP),
+	SEC_DEF("xdp_devmap/",		XDP, BPF_XDP_DEVMAP, SEC_ATTACHABLE),
+	SEC_DEF("xdp_cpumap/",		XDP, BPF_XDP_CPUMAP, SEC_ATTACHABLE),
+	SEC_DEF("xdp",			XDP, BPF_XDP, SEC_ATTACHABLE_OPT),
 	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),
-	BPF_APROG_SEC("cgroup_skb/ingress",	BPF_PROG_TYPE_CGROUP_SKB,
-						BPF_CGROUP_INET_INGRESS),
-	BPF_APROG_SEC("cgroup_skb/egress",	BPF_PROG_TYPE_CGROUP_SKB,
-						BPF_CGROUP_INET_EGRESS),
+	SEC_DEF("cgroup_skb/ingress",	CGROUP_SKB, BPF_CGROUP_INET_INGRESS, SEC_ATTACHABLE_OPT),
+	SEC_DEF("cgroup_skb/egress",	CGROUP_SKB, BPF_CGROUP_INET_EGRESS, SEC_ATTACHABLE_OPT),
 	SEC_DEF("cgroup/skb",		CGROUP_SKB, 0, SEC_NONE),
-	BPF_EAPROG_SEC("cgroup/sock_create",	BPF_PROG_TYPE_CGROUP_SOCK,
-						BPF_CGROUP_INET_SOCK_CREATE),
-	BPF_EAPROG_SEC("cgroup/sock_release",	BPF_PROG_TYPE_CGROUP_SOCK,
-						BPF_CGROUP_INET_SOCK_RELEASE),
-	BPF_APROG_SEC("cgroup/sock",		BPF_PROG_TYPE_CGROUP_SOCK,
-						BPF_CGROUP_INET_SOCK_CREATE),
-	BPF_EAPROG_SEC("cgroup/post_bind4",	BPF_PROG_TYPE_CGROUP_SOCK,
-						BPF_CGROUP_INET4_POST_BIND),
-	BPF_EAPROG_SEC("cgroup/post_bind6",	BPF_PROG_TYPE_CGROUP_SOCK,
-						BPF_CGROUP_INET6_POST_BIND),
-	BPF_APROG_SEC("cgroup/dev",		BPF_PROG_TYPE_CGROUP_DEVICE,
-						BPF_CGROUP_DEVICE),
-	BPF_APROG_SEC("sockops",		BPF_PROG_TYPE_SOCK_OPS,
-						BPF_CGROUP_SOCK_OPS),
-	BPF_APROG_SEC("sk_skb/stream_parser",	BPF_PROG_TYPE_SK_SKB,
-						BPF_SK_SKB_STREAM_PARSER),
-	BPF_APROG_SEC("sk_skb/stream_verdict",	BPF_PROG_TYPE_SK_SKB,
-						BPF_SK_SKB_STREAM_VERDICT),
+	SEC_DEF("cgroup/sock_create",	CGROUP_SOCK, BPF_CGROUP_INET_SOCK_CREATE, 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, SEC_ATTACHABLE_OPT),
+	SEC_DEF("cgroup/post_bind4",	CGROUP_SOCK, BPF_CGROUP_INET4_POST_BIND, SEC_ATTACHABLE),
+	SEC_DEF("cgroup/post_bind6",	CGROUP_SOCK, BPF_CGROUP_INET6_POST_BIND, SEC_ATTACHABLE),
+	SEC_DEF("cgroup/dev",		CGROUP_DEVICE, BPF_CGROUP_DEVICE, SEC_ATTACHABLE_OPT),
+	SEC_DEF("sockops",		SOCK_OPS, BPF_CGROUP_SOCK_OPS, SEC_ATTACHABLE_OPT),
+	SEC_DEF("sk_skb/stream_parser",	SK_SKB, BPF_SK_SKB_STREAM_PARSER, SEC_ATTACHABLE_OPT),
+	SEC_DEF("sk_skb/stream_verdict",SK_SKB, BPF_SK_SKB_STREAM_VERDICT, SEC_ATTACHABLE_OPT),
 	SEC_DEF("sk_skb",		SK_SKB, 0, SEC_NONE),
-	BPF_APROG_SEC("sk_msg",			BPF_PROG_TYPE_SK_MSG,
-						BPF_SK_MSG_VERDICT),
-	BPF_APROG_SEC("lirc_mode2",		BPF_PROG_TYPE_LIRC_MODE2,
-						BPF_LIRC_MODE2),
-	BPF_APROG_SEC("flow_dissector",		BPF_PROG_TYPE_FLOW_DISSECTOR,
-						BPF_FLOW_DISSECTOR),
-	BPF_EAPROG_SEC("cgroup/bind4",		BPF_PROG_TYPE_CGROUP_SOCK_ADDR,
-						BPF_CGROUP_INET4_BIND),
-	BPF_EAPROG_SEC("cgroup/bind6",		BPF_PROG_TYPE_CGROUP_SOCK_ADDR,
-						BPF_CGROUP_INET6_BIND),
-	BPF_EAPROG_SEC("cgroup/connect4",	BPF_PROG_TYPE_CGROUP_SOCK_ADDR,
-						BPF_CGROUP_INET4_CONNECT),
-	BPF_EAPROG_SEC("cgroup/connect6",	BPF_PROG_TYPE_CGROUP_SOCK_ADDR,
-						BPF_CGROUP_INET6_CONNECT),
-	BPF_EAPROG_SEC("cgroup/sendmsg4",	BPF_PROG_TYPE_CGROUP_SOCK_ADDR,
-						BPF_CGROUP_UDP4_SENDMSG),
-	BPF_EAPROG_SEC("cgroup/sendmsg6",	BPF_PROG_TYPE_CGROUP_SOCK_ADDR,
-						BPF_CGROUP_UDP6_SENDMSG),
-	BPF_EAPROG_SEC("cgroup/recvmsg4",	BPF_PROG_TYPE_CGROUP_SOCK_ADDR,
-						BPF_CGROUP_UDP4_RECVMSG),
-	BPF_EAPROG_SEC("cgroup/recvmsg6",	BPF_PROG_TYPE_CGROUP_SOCK_ADDR,
-						BPF_CGROUP_UDP6_RECVMSG),
-	BPF_EAPROG_SEC("cgroup/getpeername4",	BPF_PROG_TYPE_CGROUP_SOCK_ADDR,
-						BPF_CGROUP_INET4_GETPEERNAME),
-	BPF_EAPROG_SEC("cgroup/getpeername6",	BPF_PROG_TYPE_CGROUP_SOCK_ADDR,
-						BPF_CGROUP_INET6_GETPEERNAME),
-	BPF_EAPROG_SEC("cgroup/getsockname4",	BPF_PROG_TYPE_CGROUP_SOCK_ADDR,
-						BPF_CGROUP_INET4_GETSOCKNAME),
-	BPF_EAPROG_SEC("cgroup/getsockname6",	BPF_PROG_TYPE_CGROUP_SOCK_ADDR,
-						BPF_CGROUP_INET6_GETSOCKNAME),
-	BPF_EAPROG_SEC("cgroup/sysctl",		BPF_PROG_TYPE_CGROUP_SYSCTL,
-						BPF_CGROUP_SYSCTL),
-	BPF_EAPROG_SEC("cgroup/getsockopt",	BPF_PROG_TYPE_CGROUP_SOCKOPT,
-						BPF_CGROUP_GETSOCKOPT),
-	BPF_EAPROG_SEC("cgroup/setsockopt",	BPF_PROG_TYPE_CGROUP_SOCKOPT,
-						BPF_CGROUP_SETSOCKOPT),
+	SEC_DEF("sk_msg",		SK_MSG, BPF_SK_MSG_VERDICT, SEC_ATTACHABLE_OPT),
+	SEC_DEF("lirc_mode2",		LIRC_MODE2, BPF_LIRC_MODE2, SEC_ATTACHABLE_OPT),
+	SEC_DEF("flow_dissector",	FLOW_DISSECTOR, BPF_FLOW_DISSECTOR, SEC_ATTACHABLE_OPT),
+	SEC_DEF("cgroup/bind4",		CGROUP_SOCK_ADDR, BPF_CGROUP_INET4_BIND, SEC_ATTACHABLE),
+	SEC_DEF("cgroup/bind6",		CGROUP_SOCK_ADDR, BPF_CGROUP_INET6_BIND, SEC_ATTACHABLE),
+	SEC_DEF("cgroup/connect4",	CGROUP_SOCK_ADDR, BPF_CGROUP_INET4_CONNECT, SEC_ATTACHABLE),
+	SEC_DEF("cgroup/connect6",	CGROUP_SOCK_ADDR, BPF_CGROUP_INET6_CONNECT, SEC_ATTACHABLE),
+	SEC_DEF("cgroup/sendmsg4",	CGROUP_SOCK_ADDR, BPF_CGROUP_UDP4_SENDMSG, SEC_ATTACHABLE),
+	SEC_DEF("cgroup/sendmsg6",	CGROUP_SOCK_ADDR, BPF_CGROUP_UDP6_SENDMSG, SEC_ATTACHABLE),
+	SEC_DEF("cgroup/recvmsg4",	CGROUP_SOCK_ADDR, BPF_CGROUP_UDP4_RECVMSG, SEC_ATTACHABLE),
+	SEC_DEF("cgroup/recvmsg6",	CGROUP_SOCK_ADDR, BPF_CGROUP_UDP6_RECVMSG, 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, SEC_ATTACHABLE),
+	SEC_DEF("cgroup/getsockopt",	CGROUP_SOCKOPT, BPF_CGROUP_GETSOCKOPT, SEC_ATTACHABLE),
+	SEC_DEF("cgroup/setsockopt",	CGROUP_SOCKOPT, BPF_CGROUP_SETSOCKOPT, SEC_ATTACHABLE),
 	SEC_DEF("struct_ops",		STRUCT_OPS, 0, SEC_NONE),
-	BPF_EAPROG_SEC("sk_lookup/",		BPF_PROG_TYPE_SK_LOOKUP,
-						BPF_SK_LOOKUP),
+	SEC_DEF("sk_lookup/",		SK_LOOKUP, BPF_SK_LOOKUP, SEC_ATTACHABLE),
 };
 
-#undef BPF_PROG_SEC_IMPL
-#undef BPF_APROG_SEC
-#undef BPF_EAPROG_SEC
-#undef SEC_DEF
-
 #define MAX_TYPE_NAME_SIZE 32
 
 static const struct bpf_sec_def *find_sec_def(const char *sec_name)
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH v2 bpf-next 8/9] libbpf: add opt-in strict BPF program section name handling logic
  2021-09-20 23:43 [PATCH v2 bpf-next 0/9] libbpf: stricter BPF program section name handling Andrii Nakryiko
                   ` (6 preceding siblings ...)
  2021-09-20 23:43 ` [PATCH v2 bpf-next 7/9] libbpf: complete SEC() table unification for BPF_APROG_SEC/BPF_EAPROG_SEC Andrii Nakryiko
@ 2021-09-20 23:43 ` Andrii Nakryiko
  2021-09-22  1:53   ` Dave Marchevsky
  2021-09-20 23:43 ` [PATCH v2 bpf-next 9/9] selftests/bpf: switch sk_lookup selftests to strict SEC("sk_lookup") use Andrii Nakryiko
  8 siblings, 1 reply; 27+ messages in thread
From: Andrii Nakryiko @ 2021-09-20 23:43 UTC (permalink / raw)
  To: bpf, ast, daniel; +Cc: andrii, kernel-team

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),
+	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),
 	SEC_DEF("raw_tracepoint/",	RAW_TRACEPOINT, 0, SEC_NONE, attach_raw_tp),
@@ -8003,44 +8004,44 @@ static const struct bpf_sec_def section_defs[] = {
 	SEC_DEF("syscall",		SYSCALL, 0, SEC_SLEEPABLE),
 	SEC_DEF("xdp_devmap/",		XDP, BPF_XDP_DEVMAP, SEC_ATTACHABLE),
 	SEC_DEF("xdp_cpumap/",		XDP, BPF_XDP_CPUMAP, SEC_ATTACHABLE),
-	SEC_DEF("xdp",			XDP, BPF_XDP, SEC_ATTACHABLE_OPT),
-	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, SEC_ATTACHABLE_OPT),
-	SEC_DEF("cgroup_skb/egress",	CGROUP_SKB, BPF_CGROUP_INET_EGRESS, SEC_ATTACHABLE_OPT),
-	SEC_DEF("cgroup/skb",		CGROUP_SKB, 0, SEC_NONE),
-	SEC_DEF("cgroup/sock_create",	CGROUP_SOCK, BPF_CGROUP_INET_SOCK_CREATE, 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, SEC_ATTACHABLE_OPT),
-	SEC_DEF("cgroup/post_bind4",	CGROUP_SOCK, BPF_CGROUP_INET4_POST_BIND, SEC_ATTACHABLE),
-	SEC_DEF("cgroup/post_bind6",	CGROUP_SOCK, BPF_CGROUP_INET6_POST_BIND, SEC_ATTACHABLE),
-	SEC_DEF("cgroup/dev",		CGROUP_DEVICE, BPF_CGROUP_DEVICE, SEC_ATTACHABLE_OPT),
-	SEC_DEF("sockops",		SOCK_OPS, BPF_CGROUP_SOCK_OPS, SEC_ATTACHABLE_OPT),
-	SEC_DEF("sk_skb/stream_parser",	SK_SKB, BPF_SK_SKB_STREAM_PARSER, SEC_ATTACHABLE_OPT),
-	SEC_DEF("sk_skb/stream_verdict",SK_SKB, BPF_SK_SKB_STREAM_VERDICT, 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),
-	SEC_DEF("lirc_mode2",		LIRC_MODE2, BPF_LIRC_MODE2, SEC_ATTACHABLE_OPT),
-	SEC_DEF("flow_dissector",	FLOW_DISSECTOR, BPF_FLOW_DISSECTOR, SEC_ATTACHABLE_OPT),
-	SEC_DEF("cgroup/bind4",		CGROUP_SOCK_ADDR, BPF_CGROUP_INET4_BIND, SEC_ATTACHABLE),
-	SEC_DEF("cgroup/bind6",		CGROUP_SOCK_ADDR, BPF_CGROUP_INET6_BIND, SEC_ATTACHABLE),
-	SEC_DEF("cgroup/connect4",	CGROUP_SOCK_ADDR, BPF_CGROUP_INET4_CONNECT, SEC_ATTACHABLE),
-	SEC_DEF("cgroup/connect6",	CGROUP_SOCK_ADDR, BPF_CGROUP_INET6_CONNECT, SEC_ATTACHABLE),
-	SEC_DEF("cgroup/sendmsg4",	CGROUP_SOCK_ADDR, BPF_CGROUP_UDP4_SENDMSG, SEC_ATTACHABLE),
-	SEC_DEF("cgroup/sendmsg6",	CGROUP_SOCK_ADDR, BPF_CGROUP_UDP6_SENDMSG, SEC_ATTACHABLE),
-	SEC_DEF("cgroup/recvmsg4",	CGROUP_SOCK_ADDR, BPF_CGROUP_UDP4_RECVMSG, SEC_ATTACHABLE),
-	SEC_DEF("cgroup/recvmsg6",	CGROUP_SOCK_ADDR, BPF_CGROUP_UDP6_RECVMSG, 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, SEC_ATTACHABLE),
-	SEC_DEF("cgroup/getsockopt",	CGROUP_SOCKOPT, BPF_CGROUP_GETSOCKOPT, SEC_ATTACHABLE),
-	SEC_DEF("cgroup/setsockopt",	CGROUP_SOCKOPT, BPF_CGROUP_SETSOCKOPT, SEC_ATTACHABLE),
-	SEC_DEF("struct_ops",		STRUCT_OPS, 0, SEC_NONE),
+	SEC_DEF("xdp",			XDP, BPF_XDP, SEC_ATTACHABLE_OPT | SEC_SLOPPY_PFX),
+	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),
+	SEC_DEF("cgroup_skb/ingress",	CGROUP_SKB, BPF_CGROUP_INET_INGRESS, SEC_ATTACHABLE_OPT | SEC_SLOPPY_PFX),
+	SEC_DEF("cgroup_skb/egress",	CGROUP_SKB, BPF_CGROUP_INET_EGRESS, 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, 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, SEC_ATTACHABLE_OPT | SEC_SLOPPY_PFX),
+	SEC_DEF("cgroup/post_bind4",	CGROUP_SOCK, BPF_CGROUP_INET4_POST_BIND, SEC_ATTACHABLE | SEC_SLOPPY_PFX),
+	SEC_DEF("cgroup/post_bind6",	CGROUP_SOCK, BPF_CGROUP_INET6_POST_BIND, SEC_ATTACHABLE | SEC_SLOPPY_PFX),
+	SEC_DEF("cgroup/dev",		CGROUP_DEVICE, BPF_CGROUP_DEVICE, SEC_ATTACHABLE_OPT | SEC_SLOPPY_PFX),
+	SEC_DEF("sockops",		SOCK_OPS, BPF_CGROUP_SOCK_OPS, SEC_ATTACHABLE_OPT | SEC_SLOPPY_PFX),
+	SEC_DEF("sk_skb/stream_parser",	SK_SKB, BPF_SK_SKB_STREAM_PARSER, SEC_ATTACHABLE_OPT | SEC_SLOPPY_PFX),
+	SEC_DEF("sk_skb/stream_verdict",SK_SKB, BPF_SK_SKB_STREAM_VERDICT, 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 | SEC_SLOPPY_PFX),
+	SEC_DEF("lirc_mode2",		LIRC_MODE2, BPF_LIRC_MODE2, SEC_ATTACHABLE_OPT | SEC_SLOPPY_PFX),
+	SEC_DEF("flow_dissector",	FLOW_DISSECTOR, BPF_FLOW_DISSECTOR, SEC_ATTACHABLE_OPT | SEC_SLOPPY_PFX),
+	SEC_DEF("cgroup/bind4",		CGROUP_SOCK_ADDR, BPF_CGROUP_INET4_BIND, SEC_ATTACHABLE | SEC_SLOPPY_PFX),
+	SEC_DEF("cgroup/bind6",		CGROUP_SOCK_ADDR, BPF_CGROUP_INET6_BIND, SEC_ATTACHABLE | SEC_SLOPPY_PFX),
+	SEC_DEF("cgroup/connect4",	CGROUP_SOCK_ADDR, BPF_CGROUP_INET4_CONNECT, SEC_ATTACHABLE | SEC_SLOPPY_PFX),
+	SEC_DEF("cgroup/connect6",	CGROUP_SOCK_ADDR, BPF_CGROUP_INET6_CONNECT, SEC_ATTACHABLE | SEC_SLOPPY_PFX),
+	SEC_DEF("cgroup/sendmsg4",	CGROUP_SOCK_ADDR, BPF_CGROUP_UDP4_SENDMSG, SEC_ATTACHABLE | SEC_SLOPPY_PFX),
+	SEC_DEF("cgroup/sendmsg6",	CGROUP_SOCK_ADDR, BPF_CGROUP_UDP6_SENDMSG, SEC_ATTACHABLE | SEC_SLOPPY_PFX),
+	SEC_DEF("cgroup/recvmsg4",	CGROUP_SOCK_ADDR, BPF_CGROUP_UDP4_RECVMSG, SEC_ATTACHABLE | SEC_SLOPPY_PFX),
+	SEC_DEF("cgroup/recvmsg6",	CGROUP_SOCK_ADDR, BPF_CGROUP_UDP6_RECVMSG, 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, SEC_ATTACHABLE | SEC_SLOPPY_PFX),
+	SEC_DEF("cgroup/getsockopt",	CGROUP_SOCKOPT, BPF_CGROUP_GETSOCKOPT, SEC_ATTACHABLE | SEC_SLOPPY_PFX),
+	SEC_DEF("cgroup/setsockopt",	CGROUP_SOCKOPT, BPF_CGROUP_SETSOCKOPT, 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),
 };
 
@@ -8048,11 +8049,53 @@ static const struct bpf_sec_def section_defs[] = {
 
 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;
 
 	for (i = 0; i < n; i++) {
-		if (str_has_pfx(sec_name, section_defs[i].sec))
-			return &section_defs[i];
+		sec_def = &section_defs[i];
+		sec_flags = sec_def->cookie;
+		len = strlen(sec_def->sec);
+
+		/* "type/" always has to have proper SEC("type/extras") form */
+		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 '/' separator
+		 */
+		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] == '/')
+				return sec_def;
+			continue;
+		}
+
+		/* SEC_SLOPPY_PFX definitions are allowed to be just prefix
+		 * matches, unless strict section name mode
+		 * (LIBBPF_STRICT_SEC_NAME) is enabled, in which case the
+		 * 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.h b/tools/lib/bpf/libbpf_legacy.h
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,
 
+	/*
+	 * 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,
+
 	__LIBBPF_STRICT_LAST,
 };
 
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH v2 bpf-next 9/9] selftests/bpf: switch sk_lookup selftests to strict SEC("sk_lookup") use
  2021-09-20 23:43 [PATCH v2 bpf-next 0/9] libbpf: stricter BPF program section name handling Andrii Nakryiko
                   ` (7 preceding siblings ...)
  2021-09-20 23:43 ` [PATCH v2 bpf-next 8/9] libbpf: add opt-in strict BPF program section name handling logic Andrii Nakryiko
@ 2021-09-20 23:43 ` Andrii Nakryiko
  2021-09-22  2:37   ` Dave Marchevsky
  8 siblings, 1 reply; 27+ messages in thread
From: Andrii Nakryiko @ 2021-09-20 23:43 UTC (permalink / raw)
  To: bpf, ast, daniel; +Cc: andrii, kernel-team

Update "sk_lookup/" definition to be a stand-alone type specifier,
with backwards-compatible prefix match logic in non-libbpf-1.0 mode.

Currently in selftests all the "sk_lookup/<whatever>" uses just use
<whatever> for duplicated unique name encoding, which is redundant as
BPF program's name (C function name) uniquely and descriptively
identifies the intended use for such BPF programs.

With libbpf's SEC_DEF("sk_lookup") definition updated, switch existing
sk_lookup programs to use "unqualified" SEC("sk_lookup") section names,
with no random text after it.

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 tools/lib/bpf/libbpf.c                        |  2 +-
 .../selftests/bpf/progs/test_sk_lookup.c      | 38 +++++++++----------
 2 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index f0846f609e26..8c70f02a4666 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -8042,7 +8042,7 @@ static const struct bpf_sec_def section_defs[] = {
 	SEC_DEF("cgroup/getsockopt",	CGROUP_SOCKOPT, BPF_CGROUP_GETSOCKOPT, SEC_ATTACHABLE | SEC_SLOPPY_PFX),
 	SEC_DEF("cgroup/setsockopt",	CGROUP_SOCKOPT, BPF_CGROUP_SETSOCKOPT, 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),
+	SEC_DEF("sk_lookup",		SK_LOOKUP, BPF_SK_LOOKUP, SEC_ATTACHABLE | SEC_SLOPPY_PFX),
 };
 
 #define MAX_TYPE_NAME_SIZE 32
diff --git a/tools/testing/selftests/bpf/progs/test_sk_lookup.c b/tools/testing/selftests/bpf/progs/test_sk_lookup.c
index 6c4d32c56765..48534d810391 100644
--- a/tools/testing/selftests/bpf/progs/test_sk_lookup.c
+++ b/tools/testing/selftests/bpf/progs/test_sk_lookup.c
@@ -72,13 +72,13 @@ static const __u16 DST_PORT = 7007; /* Host byte order */
 static const __u32 DST_IP4 = IP4(127, 0, 0, 1);
 static const __u32 DST_IP6[] = IP6(0xfd000000, 0x0, 0x0, 0x00000001);
 
-SEC("sk_lookup/lookup_pass")
+SEC("sk_lookup")
 int lookup_pass(struct bpf_sk_lookup *ctx)
 {
 	return SK_PASS;
 }
 
-SEC("sk_lookup/lookup_drop")
+SEC("sk_lookup")
 int lookup_drop(struct bpf_sk_lookup *ctx)
 {
 	return SK_DROP;
@@ -97,7 +97,7 @@ int reuseport_drop(struct sk_reuseport_md *ctx)
 }
 
 /* Redirect packets destined for port DST_PORT to socket at redir_map[0]. */
-SEC("sk_lookup/redir_port")
+SEC("sk_lookup")
 int redir_port(struct bpf_sk_lookup *ctx)
 {
 	struct bpf_sock *sk;
@@ -116,7 +116,7 @@ int redir_port(struct bpf_sk_lookup *ctx)
 }
 
 /* Redirect packets destined for DST_IP4 address to socket at redir_map[0]. */
-SEC("sk_lookup/redir_ip4")
+SEC("sk_lookup")
 int redir_ip4(struct bpf_sk_lookup *ctx)
 {
 	struct bpf_sock *sk;
@@ -139,7 +139,7 @@ int redir_ip4(struct bpf_sk_lookup *ctx)
 }
 
 /* Redirect packets destined for DST_IP6 address to socket at redir_map[0]. */
-SEC("sk_lookup/redir_ip6")
+SEC("sk_lookup")
 int redir_ip6(struct bpf_sk_lookup *ctx)
 {
 	struct bpf_sock *sk;
@@ -164,7 +164,7 @@ int redir_ip6(struct bpf_sk_lookup *ctx)
 	return err ? SK_DROP : SK_PASS;
 }
 
-SEC("sk_lookup/select_sock_a")
+SEC("sk_lookup")
 int select_sock_a(struct bpf_sk_lookup *ctx)
 {
 	struct bpf_sock *sk;
@@ -179,7 +179,7 @@ int select_sock_a(struct bpf_sk_lookup *ctx)
 	return err ? SK_DROP : SK_PASS;
 }
 
-SEC("sk_lookup/select_sock_a_no_reuseport")
+SEC("sk_lookup")
 int select_sock_a_no_reuseport(struct bpf_sk_lookup *ctx)
 {
 	struct bpf_sock *sk;
@@ -205,7 +205,7 @@ int select_sock_b(struct sk_reuseport_md *ctx)
 }
 
 /* Check that bpf_sk_assign() returns -EEXIST if socket already selected. */
-SEC("sk_lookup/sk_assign_eexist")
+SEC("sk_lookup")
 int sk_assign_eexist(struct bpf_sk_lookup *ctx)
 {
 	struct bpf_sock *sk;
@@ -238,7 +238,7 @@ int sk_assign_eexist(struct bpf_sk_lookup *ctx)
 }
 
 /* Check that bpf_sk_assign(BPF_SK_LOOKUP_F_REPLACE) can override selection. */
-SEC("sk_lookup/sk_assign_replace_flag")
+SEC("sk_lookup")
 int sk_assign_replace_flag(struct bpf_sk_lookup *ctx)
 {
 	struct bpf_sock *sk;
@@ -270,7 +270,7 @@ int sk_assign_replace_flag(struct bpf_sk_lookup *ctx)
 }
 
 /* Check that bpf_sk_assign(sk=NULL) is accepted. */
-SEC("sk_lookup/sk_assign_null")
+SEC("sk_lookup")
 int sk_assign_null(struct bpf_sk_lookup *ctx)
 {
 	struct bpf_sock *sk = NULL;
@@ -313,7 +313,7 @@ int sk_assign_null(struct bpf_sk_lookup *ctx)
 }
 
 /* Check that selected sk is accessible through context. */
-SEC("sk_lookup/access_ctx_sk")
+SEC("sk_lookup")
 int access_ctx_sk(struct bpf_sk_lookup *ctx)
 {
 	struct bpf_sock *sk1 = NULL, *sk2 = NULL;
@@ -379,7 +379,7 @@ int access_ctx_sk(struct bpf_sk_lookup *ctx)
  * are not covered because they give bogus results, that is the
  * verifier ignores the offset.
  */
-SEC("sk_lookup/ctx_narrow_access")
+SEC("sk_lookup")
 int ctx_narrow_access(struct bpf_sk_lookup *ctx)
 {
 	struct bpf_sock *sk;
@@ -553,7 +553,7 @@ int ctx_narrow_access(struct bpf_sk_lookup *ctx)
 }
 
 /* Check that sk_assign rejects SERVER_A socket with -ESOCKNOSUPPORT */
-SEC("sk_lookup/sk_assign_esocknosupport")
+SEC("sk_lookup")
 int sk_assign_esocknosupport(struct bpf_sk_lookup *ctx)
 {
 	struct bpf_sock *sk;
@@ -578,28 +578,28 @@ int sk_assign_esocknosupport(struct bpf_sk_lookup *ctx)
 	return ret;
 }
 
-SEC("sk_lookup/multi_prog_pass1")
+SEC("sk_lookup")
 int multi_prog_pass1(struct bpf_sk_lookup *ctx)
 {
 	bpf_map_update_elem(&run_map, &KEY_PROG1, &PROG_DONE, BPF_ANY);
 	return SK_PASS;
 }
 
-SEC("sk_lookup/multi_prog_pass2")
+SEC("sk_lookup")
 int multi_prog_pass2(struct bpf_sk_lookup *ctx)
 {
 	bpf_map_update_elem(&run_map, &KEY_PROG2, &PROG_DONE, BPF_ANY);
 	return SK_PASS;
 }
 
-SEC("sk_lookup/multi_prog_drop1")
+SEC("sk_lookup")
 int multi_prog_drop1(struct bpf_sk_lookup *ctx)
 {
 	bpf_map_update_elem(&run_map, &KEY_PROG1, &PROG_DONE, BPF_ANY);
 	return SK_DROP;
 }
 
-SEC("sk_lookup/multi_prog_drop2")
+SEC("sk_lookup")
 int multi_prog_drop2(struct bpf_sk_lookup *ctx)
 {
 	bpf_map_update_elem(&run_map, &KEY_PROG2, &PROG_DONE, BPF_ANY);
@@ -623,7 +623,7 @@ static __always_inline int select_server_a(struct bpf_sk_lookup *ctx)
 	return SK_PASS;
 }
 
-SEC("sk_lookup/multi_prog_redir1")
+SEC("sk_lookup")
 int multi_prog_redir1(struct bpf_sk_lookup *ctx)
 {
 	int ret;
@@ -633,7 +633,7 @@ int multi_prog_redir1(struct bpf_sk_lookup *ctx)
 	return SK_PASS;
 }
 
-SEC("sk_lookup/multi_prog_redir2")
+SEC("sk_lookup")
 int multi_prog_redir2(struct bpf_sk_lookup *ctx)
 {
 	int ret;
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 bpf-next 1/9] selftests/bpf: normalize XDP section names in selftests
  2021-09-20 23:43 ` [PATCH v2 bpf-next 1/9] selftests/bpf: normalize XDP section names in selftests Andrii Nakryiko
@ 2021-09-21  4:55   ` Dave Marchevsky
  2021-09-21 23:08     ` Andrii Nakryiko
  0 siblings, 1 reply; 27+ messages in thread
From: Dave Marchevsky @ 2021-09-21  4:55 UTC (permalink / raw)
  To: Andrii Nakryiko, bpf, ast, daniel; +Cc: kernel-team

On 9/20/21 7:43 PM, Andrii Nakryiko wrote:   
> Convert almost all SEC("xdp_blah") uses to strict SEC("xdp") to comply
> with strict libbpf 1.0 logic of exact section name match for XDP program
> types. There is only one exception, which is only tested through
> iproute2 and defines multiple XDP programs within the same BPF object.
> Given iproute2 still works in non-strict libbpf mode and it doesn't have
> means to specify XDP programs by its name (not section name/title),
> leave that single file alone for now until iproute2 gains lookup by
> function/program name.
> 
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> ---

Aside from a checkpatch nit which you didn't cause, LGTM. Some general
comments follow as well, but aren't directly related to the patch.

Acked-by: Dave Marchevsky <davemarchevsky@fb.com>

>  tools/testing/selftests/bpf/progs/test_map_in_map.c         | 2 +-
>  .../selftests/bpf/progs/test_tcp_check_syncookie_kern.c     | 2 +-
>  tools/testing/selftests/bpf/progs/test_xdp.c                | 2 +-
>  .../testing/selftests/bpf/progs/test_xdp_adjust_tail_grow.c | 2 +-
>  .../selftests/bpf/progs/test_xdp_adjust_tail_shrink.c       | 4 +---
>  tools/testing/selftests/bpf/progs/test_xdp_devmap_helpers.c | 2 +-
>  tools/testing/selftests/bpf/progs/test_xdp_link.c           | 2 +-
>  tools/testing/selftests/bpf/progs/test_xdp_loop.c           | 2 +-
>  tools/testing/selftests/bpf/progs/test_xdp_noinline.c       | 4 ++--
>  .../selftests/bpf/progs/test_xdp_with_cpumap_helpers.c      | 4 ++--
>  .../selftests/bpf/progs/test_xdp_with_devmap_helpers.c      | 4 ++--
>  tools/testing/selftests/bpf/progs/xdp_dummy.c               | 2 +-
>  tools/testing/selftests/bpf/progs/xdp_redirect_multi_kern.c | 4 ++--
>  tools/testing/selftests/bpf/progs/xdping_kern.c             | 4 ++--
>  tools/testing/selftests/bpf/test_tcp_check_syncookie.sh     | 2 +-
>  tools/testing/selftests/bpf/test_xdp_redirect.sh            | 4 ++--
>  tools/testing/selftests/bpf/test_xdp_redirect_multi.sh      | 2 +-
>  tools/testing/selftests/bpf/test_xdp_veth.sh                | 4 ++--
>  tools/testing/selftests/bpf/xdping.c                        | 6 +++---

Doesn't look like the test_...sh's here are run by the CI. Confirmed they
(as well as test_xdping.sh) all passed for me. My test VM isn't doing anything
special networking-wise, so maybe it's not too difficult to add these to CI.

>  19 files changed, 28 insertions(+), 30 deletions(-)
> 
> diff --git a/tools/testing/selftests/bpf/progs/test_map_in_map.c b/tools/testing/selftests/bpf/progs/test_map_in_map.c
> index 1cfeb940cf9f..5f0e0bfc151e 100644
> --- a/tools/testing/selftests/bpf/progs/test_map_in_map.c
> +++ b/tools/testing/selftests/bpf/progs/test_map_in_map.c
> @@ -23,7 +23,7 @@ struct {
>  	__uint(value_size, sizeof(__u32));
>  } mim_hash SEC(".maps");
>  
> -SEC("xdp_mimtest")
> +SEC("xdp")
>  int xdp_mimtest0(struct xdp_md *ctx)
>  {
>  	int value = 123;
> diff --git a/tools/testing/selftests/bpf/progs/test_tcp_check_syncookie_kern.c b/tools/testing/selftests/bpf/progs/test_tcp_check_syncookie_kern.c
> index 47cbe2eeae43..fac7ef99f9a6 100644
> --- a/tools/testing/selftests/bpf/progs/test_tcp_check_syncookie_kern.c
> +++ b/tools/testing/selftests/bpf/progs/test_tcp_check_syncookie_kern.c
> @@ -156,7 +156,7 @@ int check_syncookie_clsact(struct __sk_buff *skb)
>  	return TC_ACT_OK;
>  }
>  
> -SEC("xdp/check_syncookie")
> +SEC("xdp")
>  int check_syncookie_xdp(struct xdp_md *ctx)
>  {
>  	check_syncookie(ctx, (void *)(long)ctx->data,
> diff --git a/tools/testing/selftests/bpf/progs/test_xdp.c b/tools/testing/selftests/bpf/progs/test_xdp.c
> index 31f9bce37491..e6aa2fc6ce6b 100644
> --- a/tools/testing/selftests/bpf/progs/test_xdp.c
> +++ b/tools/testing/selftests/bpf/progs/test_xdp.c
> @@ -210,7 +210,7 @@ static __always_inline int handle_ipv6(struct xdp_md *xdp)
>  	return XDP_TX;
>  }
>  
> -SEC("xdp_tx_iptunnel")
> +SEC("xdp")
>  int _xdp_tx_iptunnel(struct xdp_md *xdp)
>  {
>  	void *data_end = (void *)(long)xdp->data_end;
> diff --git a/tools/testing/selftests/bpf/progs/test_xdp_adjust_tail_grow.c b/tools/testing/selftests/bpf/progs/test_xdp_adjust_tail_grow.c
> index 3d66599eee2e..199c61b7d062 100644
> --- a/tools/testing/selftests/bpf/progs/test_xdp_adjust_tail_grow.c
> +++ b/tools/testing/selftests/bpf/progs/test_xdp_adjust_tail_grow.c
> @@ -2,7 +2,7 @@
>  #include <linux/bpf.h>
>  #include <bpf/bpf_helpers.h>
>  
> -SEC("xdp_adjust_tail_grow")
> +SEC("xdp")
>  int _xdp_adjust_tail_grow(struct xdp_md *xdp)
>  {
>  	void *data_end = (void *)(long)xdp->data_end;
> diff --git a/tools/testing/selftests/bpf/progs/test_xdp_adjust_tail_shrink.c b/tools/testing/selftests/bpf/progs/test_xdp_adjust_tail_shrink.c
> index 22065a9cfb25..b7448253d135 100644
> --- a/tools/testing/selftests/bpf/progs/test_xdp_adjust_tail_shrink.c
> +++ b/tools/testing/selftests/bpf/progs/test_xdp_adjust_tail_shrink.c
> @@ -9,9 +9,7 @@
>  #include <linux/if_ether.h>
>  #include <bpf/bpf_helpers.h>
>  
> -int _version SEC("version") = 1;
> -

Didn't realize this was meant to specify kernel version for compat, and that
it no longer does anything anyways. Maybe this should be removed from all 
selftests + examples to make this more obvious?

> -SEC("xdp_adjust_tail_shrink")
> +SEC("xdp")
>  int _xdp_adjust_tail_shrink(struct xdp_md *xdp)
>  {
>  	void *data_end = (void *)(long)xdp->data_end;
> diff --git a/tools/testing/selftests/bpf/progs/test_xdp_devmap_helpers.c b/tools/testing/selftests/bpf/progs/test_xdp_devmap_helpers.c
> index b360ba2bd441..807bf895f42c 100644
> --- a/tools/testing/selftests/bpf/progs/test_xdp_devmap_helpers.c
> +++ b/tools/testing/selftests/bpf/progs/test_xdp_devmap_helpers.c
> @@ -5,7 +5,7 @@
>  #include <linux/bpf.h>
>  #include <bpf/bpf_helpers.h>
>  
> -SEC("xdp_dm_log")
> +SEC("xdp")
>  int xdpdm_devlog(struct xdp_md *ctx)
>  {
>  	char fmt[] = "devmap redirect: dev %u -> dev %u len %u\n";
> diff --git a/tools/testing/selftests/bpf/progs/test_xdp_link.c b/tools/testing/selftests/bpf/progs/test_xdp_link.c
> index eb93ea95d1d8..ee7d6ac0f615 100644
> --- a/tools/testing/selftests/bpf/progs/test_xdp_link.c
> +++ b/tools/testing/selftests/bpf/progs/test_xdp_link.c
> @@ -5,7 +5,7 @@
>  
>  char LICENSE[] SEC("license") = "GPL";
>  
> -SEC("xdp/handler")
> +SEC("xdp")
>  int xdp_handler(struct xdp_md *xdp)
>  {
>  	return 0;
> diff --git a/tools/testing/selftests/bpf/progs/test_xdp_loop.c b/tools/testing/selftests/bpf/progs/test_xdp_loop.c
> index fcabcda30ba3..27eb52dda92c 100644
> --- a/tools/testing/selftests/bpf/progs/test_xdp_loop.c
> +++ b/tools/testing/selftests/bpf/progs/test_xdp_loop.c
> @@ -206,7 +206,7 @@ static __always_inline int handle_ipv6(struct xdp_md *xdp)
>  	return XDP_TX;
>  }
>  
> -SEC("xdp_tx_iptunnel")
> +SEC("xdp")
>  int _xdp_tx_iptunnel(struct xdp_md *xdp)
>  {
>  	void *data_end = (void *)(long)xdp->data_end;
> diff --git a/tools/testing/selftests/bpf/progs/test_xdp_noinline.c b/tools/testing/selftests/bpf/progs/test_xdp_noinline.c
> index 3a67921f62b5..596c4e71bf3a 100644
> --- a/tools/testing/selftests/bpf/progs/test_xdp_noinline.c
> +++ b/tools/testing/selftests/bpf/progs/test_xdp_noinline.c
> @@ -797,7 +797,7 @@ static int process_packet(void *data, __u64 off, void *data_end,
>  	return XDP_DROP;
>  }
>  
> -SEC("xdp-test-v4")
> +SEC("xdp")
>  int balancer_ingress_v4(struct xdp_md *ctx)
>  {
>  	void *data = (void *)(long)ctx->data;
> @@ -816,7 +816,7 @@ int balancer_ingress_v4(struct xdp_md *ctx)
>  		return XDP_DROP;
>  }
>  
> -SEC("xdp-test-v6")
> +SEC("xdp")
>  int balancer_ingress_v6(struct xdp_md *ctx)
>  {
>  	void *data = (void *)(long)ctx->data;
> diff --git a/tools/testing/selftests/bpf/progs/test_xdp_with_cpumap_helpers.c b/tools/testing/selftests/bpf/progs/test_xdp_with_cpumap_helpers.c
> index 59ee4f182ff8..532025057711 100644
> --- a/tools/testing/selftests/bpf/progs/test_xdp_with_cpumap_helpers.c
> +++ b/tools/testing/selftests/bpf/progs/test_xdp_with_cpumap_helpers.c
> @@ -12,13 +12,13 @@ struct {
>  	__uint(max_entries, 4);
>  } cpu_map SEC(".maps");
>  
> -SEC("xdp_redir")
> +SEC("xdp")
>  int xdp_redir_prog(struct xdp_md *ctx)
>  {
>  	return bpf_redirect_map(&cpu_map, 1, 0);
>  }
>  
> -SEC("xdp_dummy")
> +SEC("xdp")
>  int xdp_dummy_prog(struct xdp_md *ctx)
>  {
>  	return XDP_PASS;
> diff --git a/tools/testing/selftests/bpf/progs/test_xdp_with_devmap_helpers.c b/tools/testing/selftests/bpf/progs/test_xdp_with_devmap_helpers.c
> index 0ac086497722..1e6b9c38ea6d 100644
> --- a/tools/testing/selftests/bpf/progs/test_xdp_with_devmap_helpers.c
> +++ b/tools/testing/selftests/bpf/progs/test_xdp_with_devmap_helpers.c
> @@ -9,7 +9,7 @@ struct {
>  	__uint(max_entries, 4);
>  } dm_ports SEC(".maps");
>  
> -SEC("xdp_redir")
> +SEC("xdp")
>  int xdp_redir_prog(struct xdp_md *ctx)
>  {
>  	return bpf_redirect_map(&dm_ports, 1, 0);
> @@ -18,7 +18,7 @@ int xdp_redir_prog(struct xdp_md *ctx)
>  /* invalid program on DEVMAP entry;
>   * SEC name means expected attach type not set
>   */
> -SEC("xdp_dummy")
> +SEC("xdp")
>  int xdp_dummy_prog(struct xdp_md *ctx)
>  {
>  	return XDP_PASS;
> diff --git a/tools/testing/selftests/bpf/progs/xdp_dummy.c b/tools/testing/selftests/bpf/progs/xdp_dummy.c
> index ea25e8881992..d988b2e0cee8 100644
> --- a/tools/testing/selftests/bpf/progs/xdp_dummy.c
> +++ b/tools/testing/selftests/bpf/progs/xdp_dummy.c
> @@ -4,7 +4,7 @@
>  #include <linux/bpf.h>
>  #include <bpf/bpf_helpers.h>
>  
> -SEC("xdp_dummy")
> +SEC("xdp")
>  int xdp_dummy_prog(struct xdp_md *ctx)
>  {
>  	return XDP_PASS;
> diff --git a/tools/testing/selftests/bpf/progs/xdp_redirect_multi_kern.c b/tools/testing/selftests/bpf/progs/xdp_redirect_multi_kern.c
> index 880debcbcd65..8395782b6e0a 100644
> --- a/tools/testing/selftests/bpf/progs/xdp_redirect_multi_kern.c
> +++ b/tools/testing/selftests/bpf/progs/xdp_redirect_multi_kern.c
> @@ -34,7 +34,7 @@ struct {
>  	__uint(max_entries, 128);
>  } mac_map SEC(".maps");
>  
> -SEC("xdp_redirect_map_multi")
> +SEC("xdp")
>  int xdp_redirect_map_multi_prog(struct xdp_md *ctx)
>  {
>  	void *data_end = (void *)(long)ctx->data_end;
> @@ -63,7 +63,7 @@ int xdp_redirect_map_multi_prog(struct xdp_md *ctx)
>  }
>  
>  /* The following 2 progs are for 2nd devmap prog testing */
> -SEC("xdp_redirect_map_ingress")
> +SEC("xdp")
>  int xdp_redirect_map_all_prog(struct xdp_md *ctx)
>  {
>  	return bpf_redirect_map(&map_egress, 0,
> diff --git a/tools/testing/selftests/bpf/progs/xdping_kern.c b/tools/testing/selftests/bpf/progs/xdping_kern.c
> index 6b9ca40bd1f4..4ad73847b8a5 100644
> --- a/tools/testing/selftests/bpf/progs/xdping_kern.c
> +++ b/tools/testing/selftests/bpf/progs/xdping_kern.c
> @@ -86,7 +86,7 @@ static __always_inline int icmp_check(struct xdp_md *ctx, int type)
>  	return XDP_TX;
>  }
>  
> -SEC("xdpclient")
> +SEC("xdp")
>  int xdping_client(struct xdp_md *ctx)
>  {
>  	void *data_end = (void *)(long)ctx->data_end;
> @@ -150,7 +150,7 @@ int xdping_client(struct xdp_md *ctx)
>  	return XDP_TX;
>  }
>  
> -SEC("xdpserver")
> +SEC("xdp")
>  int xdping_server(struct xdp_md *ctx)
>  {
>  	void *data_end = (void *)(long)ctx->data_end;
> diff --git a/tools/testing/selftests/bpf/test_tcp_check_syncookie.sh b/tools/testing/selftests/bpf/test_tcp_check_syncookie.sh
> index 9b3617d770a5..fed765157c53 100755
> --- a/tools/testing/selftests/bpf/test_tcp_check_syncookie.sh
> +++ b/tools/testing/selftests/bpf/test_tcp_check_syncookie.sh
> @@ -77,7 +77,7 @@ TEST_IF=lo
>  MAX_PING_TRIES=5
>  BPF_PROG_OBJ="${DIR}/test_tcp_check_syncookie_kern.o"
>  CLSACT_SECTION="clsact/check_syncookie"
> -XDP_SECTION="xdp/check_syncookie"
> +XDP_SECTION="xdp"
>  BPF_PROG_ID=0
>  PROG="${DIR}/test_tcp_check_syncookie_user"
>  
> diff --git a/tools/testing/selftests/bpf/test_xdp_redirect.sh b/tools/testing/selftests/bpf/test_xdp_redirect.sh
> index c033850886f4..57c8db9972a6 100755
> --- a/tools/testing/selftests/bpf/test_xdp_redirect.sh
> +++ b/tools/testing/selftests/bpf/test_xdp_redirect.sh
> @@ -52,8 +52,8 @@ test_xdp_redirect()
>  		return 0
>  	fi
>  
> -	ip -n ns1 link set veth11 $xdpmode obj xdp_dummy.o sec xdp_dummy &> /dev/null
> -	ip -n ns2 link set veth22 $xdpmode obj xdp_dummy.o sec xdp_dummy &> /dev/null
> +	ip -n ns1 link set veth11 $xdpmode obj xdp_dummy.o sec xdp &> /dev/null
> +	ip -n ns2 link set veth22 $xdpmode obj xdp_dummy.o sec xdp &> /dev/null
>  	ip link set dev veth1 $xdpmode obj test_xdp_redirect.o sec redirect_to_222 &> /dev/null
>  	ip link set dev veth2 $xdpmode obj test_xdp_redirect.o sec redirect_to_111 &> /dev/null
>  
> diff --git a/tools/testing/selftests/bpf/test_xdp_redirect_multi.sh b/tools/testing/selftests/bpf/test_xdp_redirect_multi.sh
> index 1538373157e3..351955c2bdfd 100755
> --- a/tools/testing/selftests/bpf/test_xdp_redirect_multi.sh
> +++ b/tools/testing/selftests/bpf/test_xdp_redirect_multi.sh
> @@ -88,7 +88,7 @@ setup_ns()
>  		# Add a neigh entry for IPv4 ping test
>  		ip -n ns$i neigh add 192.0.2.253 lladdr 00:00:00:00:00:01 dev veth0
>  		ip -n ns$i link set veth0 $mode obj \
> -			xdp_dummy.o sec xdp_dummy &> /dev/null || \
> +			xdp_dummy.o sec xdp &> /dev/null || \
>  			{ test_fail "Unable to load dummy xdp" && exit 1; }
>  		IFACES="$IFACES veth$i"
>  		veth_mac[$i]=$(ip link show veth$i | awk '/link\/ether/ {print $2}')
> diff --git a/tools/testing/selftests/bpf/test_xdp_veth.sh b/tools/testing/selftests/bpf/test_xdp_veth.sh
> index 995278e684b6..a3a1eaee26ea 100755
> --- a/tools/testing/selftests/bpf/test_xdp_veth.sh
> +++ b/tools/testing/selftests/bpf/test_xdp_veth.sh
> @@ -107,9 +107,9 @@ ip link set dev veth1 xdp pinned $BPF_DIR/progs/redirect_map_0
>  ip link set dev veth2 xdp pinned $BPF_DIR/progs/redirect_map_1
>  ip link set dev veth3 xdp pinned $BPF_DIR/progs/redirect_map_2
>  
> -ip -n ns1 link set dev veth11 xdp obj xdp_dummy.o sec xdp_dummy
> +ip -n ns1 link set dev veth11 xdp obj xdp_dummy.o sec xdp
>  ip -n ns2 link set dev veth22 xdp obj xdp_tx.o sec xdp
> -ip -n ns3 link set dev veth33 xdp obj xdp_dummy.o sec xdp_dummy
> +ip -n ns3 link set dev veth33 xdp obj xdp_dummy.o sec xdp
>  
>  trap cleanup EXIT
>  
> diff --git a/tools/testing/selftests/bpf/xdping.c b/tools/testing/selftests/bpf/xdping.c
> index 842d9155d36c..f9798ead20a9 100644
> --- a/tools/testing/selftests/bpf/xdping.c
> +++ b/tools/testing/selftests/bpf/xdping.c
> @@ -178,9 +178,9 @@ int main(int argc, char **argv)
>  		return 1;
>  	}
>  
> -	main_prog = bpf_object__find_program_by_title(obj,
> -						      server ? "xdpserver" :
> -							       "xdpclient");
> +	main_prog = bpf_object__find_program_by_name(obj,
> +						      server ? "xdping_server" :
> +							       "xdping_client");

checkpatch doesn't like the text alignment here, not that you changed it

>  	if (main_prog)
>  		prog_fd = bpf_program__fd(main_prog);
>  	if (!main_prog || prog_fd < 0) {
> 



^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 bpf-next 2/9] selftests/bpf: normalize SEC("classifier") usage
  2021-09-20 23:43 ` [PATCH v2 bpf-next 2/9] selftests/bpf: normalize SEC("classifier") usage Andrii Nakryiko
@ 2021-09-21  5:20   ` Dave Marchevsky
  2021-09-21 23:10     ` Andrii Nakryiko
  0 siblings, 1 reply; 27+ messages in thread
From: Dave Marchevsky @ 2021-09-21  5:20 UTC (permalink / raw)
  To: Andrii Nakryiko, bpf, ast, daniel; +Cc: kernel-team

On 9/20/21 7:43 PM, Andrii Nakryiko wrote:   
> Convert all SEC("classifier*") uses to strict SEC("classifier") with no
> extra characters. In reference_tracking selftests also drop the usage of
> broken bpf_program__load(). Along the way switch from ambiguous searching by
> program title (section name) to non-ambiguous searching by name in some
> selftests, getting closer to completely removing
> bpf_object__find_program_by_title().
> 
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> ---

Looks like you feel similarly about the SEC("version") comment from patch 1.

>  .../bpf/prog_tests/reference_tracking.c       | 22 ++++---
>  .../selftests/bpf/prog_tests/sk_assign.c      |  2 +-
>  .../selftests/bpf/prog_tests/tailcalls.c      | 58 +++++++++----------
>  .../testing/selftests/bpf/progs/skb_pkt_end.c |  2 +-
>  tools/testing/selftests/bpf/progs/tailcall1.c |  5 +-
>  tools/testing/selftests/bpf/progs/tailcall2.c | 21 ++++---
>  tools/testing/selftests/bpf/progs/tailcall3.c |  5 +-
>  tools/testing/selftests/bpf/progs/tailcall4.c |  5 +-
>  tools/testing/selftests/bpf/progs/tailcall5.c |  5 +-
>  tools/testing/selftests/bpf/progs/tailcall6.c |  4 +-
>  .../selftests/bpf/progs/tailcall_bpf2bpf1.c   |  5 +-
>  .../selftests/bpf/progs/tailcall_bpf2bpf2.c   |  5 +-
>  .../selftests/bpf/progs/tailcall_bpf2bpf3.c   |  9 ++-
>  .../selftests/bpf/progs/tailcall_bpf2bpf4.c   | 13 ++---
>  .../bpf/progs/test_btf_skc_cls_ingress.c      |  2 +-
>  .../selftests/bpf/progs/test_cls_redirect.c   |  2 +-
>  .../selftests/bpf/progs/test_global_data.c    |  2 +-
>  .../selftests/bpf/progs/test_global_func1.c   |  2 +-
>  .../selftests/bpf/progs/test_global_func3.c   |  2 +-
>  .../selftests/bpf/progs/test_global_func5.c   |  2 +-
>  .../selftests/bpf/progs/test_global_func6.c   |  2 +-
>  .../selftests/bpf/progs/test_global_func7.c   |  2 +-
>  .../selftests/bpf/progs/test_pkt_access.c     |  2 +-
>  .../selftests/bpf/progs/test_pkt_md_access.c  |  4 +-
>  .../selftests/bpf/progs/test_sk_assign.c      |  3 +-
>  .../selftests/bpf/progs/test_sk_lookup_kern.c | 37 ++++++------
>  .../selftests/bpf/progs/test_skb_helpers.c    |  2 +-
>  .../selftests/bpf/progs/test_sockmap_update.c |  2 +-
>  .../selftests/bpf/progs/test_tc_neigh.c       |  6 +-
>  .../selftests/bpf/progs/test_tc_neigh_fib.c   |  6 +-
>  .../selftests/bpf/progs/test_tc_peer.c        | 10 ++--
>  31 files changed, 117 insertions(+), 132 deletions(-)
> 
> diff --git a/tools/testing/selftests/bpf/prog_tests/reference_tracking.c b/tools/testing/selftests/bpf/prog_tests/reference_tracking.c
> index ded2dc8ddd79..836b8bf17fff 100644
> --- a/tools/testing/selftests/bpf/prog_tests/reference_tracking.c
> +++ b/tools/testing/selftests/bpf/prog_tests/reference_tracking.c
> @@ -2,14 +2,14 @@
>  #include <test_progs.h>
>  
>  static void toggle_object_autoload_progs(const struct bpf_object *obj,
> -					 const char *title_load)
> +					 const char *name_load)
>  {
>  	struct bpf_program *prog;
>  
>  	bpf_object__for_each_program(prog, obj) {
> -		const char *title = bpf_program__section_name(prog);
> +		const char *name = bpf_program__name(prog);
>  
> -		if (!strcmp(title_load, title))
> +		if (!strcmp(name_load, name))
>  			bpf_program__set_autoload(prog, true);
>  		else
>  			bpf_program__set_autoload(prog, false);
> @@ -39,23 +39,20 @@ void test_reference_tracking(void)
>  		goto cleanup;
>  
>  	bpf_object__for_each_program(prog, obj_iter) {
> -		const char *title;
> +		const char *name;
>  
>  		/* Ignore .text sections */

I think this comment should go as well, no longer helps understand the test

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 bpf-next 3/9] selftests/bpf: normalize all the rest SEC() uses
  2021-09-20 23:43 ` [PATCH v2 bpf-next 3/9] selftests/bpf: normalize all the rest SEC() uses Andrii Nakryiko
@ 2021-09-21  5:41   ` Dave Marchevsky
  2021-09-21 23:12     ` Andrii Nakryiko
  0 siblings, 1 reply; 27+ messages in thread
From: Dave Marchevsky @ 2021-09-21  5:41 UTC (permalink / raw)
  To: Andrii Nakryiko, bpf, ast, daniel; +Cc: kernel-team

On 9/20/21 7:43 PM, Andrii Nakryiko wrote:   
> Normalize all the other non-conforming SEC() usages across all
> selftests. This is in preparation for libbpf to start to enforce
> stricter SEC() rules in libbpf 1.0 mode.
> 
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> ---
>  .../selftests/bpf/prog_tests/flow_dissector.c |  4 +--
>  .../selftests/bpf/prog_tests/sockopt_multi.c  | 30 +++++++++----------
>  tools/testing/selftests/bpf/progs/bpf_flow.c  |  3 +-
>  .../bpf/progs/cg_storage_multi_isolated.c     |  4 +--
>  .../bpf/progs/cg_storage_multi_shared.c       |  4 +--
>  .../selftests/bpf/progs/sockopt_multi.c       |  5 ++--
>  .../selftests/bpf/progs/test_cgroup_link.c    |  4 +--
>  .../bpf/progs/test_misc_tcp_hdr_options.c     |  2 +-
>  .../selftests/bpf/progs/test_sk_lookup.c      |  6 ++--
>  .../selftests/bpf/progs/test_sockmap_listen.c |  2 +-
>  .../progs/test_sockmap_skb_verdict_attach.c   |  2 +-
>  .../bpf/progs/test_tcp_check_syncookie_kern.c |  2 +-
>  .../bpf/progs/test_tcp_hdr_options.c          |  2 +-
>  .../selftests/bpf/test_tcp_check_syncookie.sh |  2 +-

Ran test_tcp_check_syncookie.sh as CI suite doesn't - works.

checkpatch has some line length complaints, otherwise LGTM

Acked-by: Dave Marchevsky <davemarchevsky@fb.com>

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 bpf-next 1/9] selftests/bpf: normalize XDP section names in selftests
  2021-09-21  4:55   ` Dave Marchevsky
@ 2021-09-21 23:08     ` Andrii Nakryiko
  0 siblings, 0 replies; 27+ messages in thread
From: Andrii Nakryiko @ 2021-09-21 23:08 UTC (permalink / raw)
  To: Dave Marchevsky
  Cc: Andrii Nakryiko, bpf, Alexei Starovoitov, Daniel Borkmann, Kernel Team

On Mon, Sep 20, 2021 at 9:55 PM Dave Marchevsky <davemarchevsky@fb.com> wrote:
>
> On 9/20/21 7:43 PM, Andrii Nakryiko wrote:
> > Convert almost all SEC("xdp_blah") uses to strict SEC("xdp") to comply
> > with strict libbpf 1.0 logic of exact section name match for XDP program
> > types. There is only one exception, which is only tested through
> > iproute2 and defines multiple XDP programs within the same BPF object.
> > Given iproute2 still works in non-strict libbpf mode and it doesn't have
> > means to specify XDP programs by its name (not section name/title),
> > leave that single file alone for now until iproute2 gains lookup by
> > function/program name.
> >
> > Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> > ---
>
> Aside from a checkpatch nit which you didn't cause, LGTM. Some general
> comments follow as well, but aren't directly related to the patch.
>
> Acked-by: Dave Marchevsky <davemarchevsky@fb.com>
>
> >  tools/testing/selftests/bpf/progs/test_map_in_map.c         | 2 +-
> >  .../selftests/bpf/progs/test_tcp_check_syncookie_kern.c     | 2 +-
> >  tools/testing/selftests/bpf/progs/test_xdp.c                | 2 +-
> >  .../testing/selftests/bpf/progs/test_xdp_adjust_tail_grow.c | 2 +-
> >  .../selftests/bpf/progs/test_xdp_adjust_tail_shrink.c       | 4 +---
> >  tools/testing/selftests/bpf/progs/test_xdp_devmap_helpers.c | 2 +-
> >  tools/testing/selftests/bpf/progs/test_xdp_link.c           | 2 +-
> >  tools/testing/selftests/bpf/progs/test_xdp_loop.c           | 2 +-
> >  tools/testing/selftests/bpf/progs/test_xdp_noinline.c       | 4 ++--
> >  .../selftests/bpf/progs/test_xdp_with_cpumap_helpers.c      | 4 ++--
> >  .../selftests/bpf/progs/test_xdp_with_devmap_helpers.c      | 4 ++--
> >  tools/testing/selftests/bpf/progs/xdp_dummy.c               | 2 +-
> >  tools/testing/selftests/bpf/progs/xdp_redirect_multi_kern.c | 4 ++--
> >  tools/testing/selftests/bpf/progs/xdping_kern.c             | 4 ++--
> >  tools/testing/selftests/bpf/test_tcp_check_syncookie.sh     | 2 +-
> >  tools/testing/selftests/bpf/test_xdp_redirect.sh            | 4 ++--
> >  tools/testing/selftests/bpf/test_xdp_redirect_multi.sh      | 2 +-
> >  tools/testing/selftests/bpf/test_xdp_veth.sh                | 4 ++--
> >  tools/testing/selftests/bpf/xdping.c                        | 6 +++---
>
> Doesn't look like the test_...sh's here are run by the CI. Confirmed they
> (as well as test_xdping.sh) all passed for me. My test VM isn't doing anything
> special networking-wise, so maybe it's not too difficult to add these to CI.

Thanks for confirming. Yes, they are not run in CI, we only run
test_progs, test_verifier and test_maps. Instead of adding all those
small scripts to be run by CI, we are encouraging everyone to converge
on test_progs, because that's where we invest efforts to make it a
universal test runner for BPF needs. So I'd say let's convert them to
test_progs framework instead.

>
> >  19 files changed, 28 insertions(+), 30 deletions(-)
> >
> > diff --git a/tools/testing/selftests/bpf/progs/test_map_in_map.c b/tools/testing/selftests/bpf/progs/test_map_in_map.c
> > index 1cfeb940cf9f..5f0e0bfc151e 100644
> > --- a/tools/testing/selftests/bpf/progs/test_map_in_map.c
> > +++ b/tools/testing/selftests/bpf/progs/test_map_in_map.c
> > @@ -23,7 +23,7 @@ struct {

[...]

> >       void *data_end = (void *)(long)xdp->data_end;
> > diff --git a/tools/testing/selftests/bpf/progs/test_xdp_adjust_tail_shrink.c b/tools/testing/selftests/bpf/progs/test_xdp_adjust_tail_shrink.c
> > index 22065a9cfb25..b7448253d135 100644
> > --- a/tools/testing/selftests/bpf/progs/test_xdp_adjust_tail_shrink.c
> > +++ b/tools/testing/selftests/bpf/progs/test_xdp_adjust_tail_shrink.c
> > @@ -9,9 +9,7 @@
> >  #include <linux/if_ether.h>
> >  #include <bpf/bpf_helpers.h>
> >
> > -int _version SEC("version") = 1;
> > -
>
> Didn't realize this was meant to specify kernel version for compat, and that
> it no longer does anything anyways. Maybe this should be removed from all
> selftests + examples to make this more obvious?

Yes, it should, but perhaps in a separate clean up series. Except I'd
leave it for BPF static linker testing, of course.

>
> > -SEC("xdp_adjust_tail_shrink")
> > +SEC("xdp")
> >  int _xdp_adjust_tail_shrink(struct xdp_md *xdp)
> >  {
> >       void *data_end = (void *)(long)xdp->data_end;
> > diff --git a/tools/testing/selftests/bpf/progs/test_xdp_devmap_helpers.c b/tools/testing/selftests/bpf/progs/test_xdp_devmap_helpers.c
> > index b360ba2bd441..807bf895f42c 100644
> > --- a/tools/testing/selftests/bpf/progs/test_xdp_devmap_helpers.c

Note, it's generally a good idea to trim irrelevant parts in your
reply making it easier to see one's replies among the wall of text,
especially outside of gmail UI.

[...]

> > diff --git a/tools/testing/selftests/bpf/xdping.c b/tools/testing/selftests/bpf/xdping.c
> > index 842d9155d36c..f9798ead20a9 100644
> > --- a/tools/testing/selftests/bpf/xdping.c
> > +++ b/tools/testing/selftests/bpf/xdping.c
> > @@ -178,9 +178,9 @@ int main(int argc, char **argv)
> >               return 1;
> >       }
> >
> > -     main_prog = bpf_object__find_program_by_title(obj,
> > -                                                   server ? "xdpserver" :
> > -                                                            "xdpclient");
> > +     main_prog = bpf_object__find_program_by_name(obj,
> > +                                                   server ? "xdping_server" :
> > +                                                            "xdping_client");
>
> checkpatch doesn't like the text alignment here, not that you changed it

yeah, me neither, but not worth re-spin just for this :)

>
> >       if (main_prog)
> >               prog_fd = bpf_program__fd(main_prog);
> >       if (!main_prog || prog_fd < 0) {
> >
>
>

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 bpf-next 2/9] selftests/bpf: normalize SEC("classifier") usage
  2021-09-21  5:20   ` Dave Marchevsky
@ 2021-09-21 23:10     ` Andrii Nakryiko
  0 siblings, 0 replies; 27+ messages in thread
From: Andrii Nakryiko @ 2021-09-21 23:10 UTC (permalink / raw)
  To: Dave Marchevsky
  Cc: Andrii Nakryiko, bpf, Alexei Starovoitov, Daniel Borkmann, Kernel Team

On Mon, Sep 20, 2021 at 10:21 PM Dave Marchevsky <davemarchevsky@fb.com> wrote:
>
> On 9/20/21 7:43 PM, Andrii Nakryiko wrote:
> > Convert all SEC("classifier*") uses to strict SEC("classifier") with no
> > extra characters. In reference_tracking selftests also drop the usage of
> > broken bpf_program__load(). Along the way switch from ambiguous searching by
> > program title (section name) to non-ambiguous searching by name in some
> > selftests, getting closer to completely removing
> > bpf_object__find_program_by_title().
> >
> > Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> > ---
>
> Looks like you feel similarly about the SEC("version") comment from patch 1.
>
> >  .../bpf/prog_tests/reference_tracking.c       | 22 ++++---
> >  .../selftests/bpf/prog_tests/sk_assign.c      |  2 +-
> >  .../selftests/bpf/prog_tests/tailcalls.c      | 58 +++++++++----------
> >  .../testing/selftests/bpf/progs/skb_pkt_end.c |  2 +-
> >  tools/testing/selftests/bpf/progs/tailcall1.c |  5 +-
> >  tools/testing/selftests/bpf/progs/tailcall2.c | 21 ++++---
> >  tools/testing/selftests/bpf/progs/tailcall3.c |  5 +-
> >  tools/testing/selftests/bpf/progs/tailcall4.c |  5 +-
> >  tools/testing/selftests/bpf/progs/tailcall5.c |  5 +-
> >  tools/testing/selftests/bpf/progs/tailcall6.c |  4 +-
> >  .../selftests/bpf/progs/tailcall_bpf2bpf1.c   |  5 +-
> >  .../selftests/bpf/progs/tailcall_bpf2bpf2.c   |  5 +-
> >  .../selftests/bpf/progs/tailcall_bpf2bpf3.c   |  9 ++-
> >  .../selftests/bpf/progs/tailcall_bpf2bpf4.c   | 13 ++---
> >  .../bpf/progs/test_btf_skc_cls_ingress.c      |  2 +-
> >  .../selftests/bpf/progs/test_cls_redirect.c   |  2 +-
> >  .../selftests/bpf/progs/test_global_data.c    |  2 +-
> >  .../selftests/bpf/progs/test_global_func1.c   |  2 +-
> >  .../selftests/bpf/progs/test_global_func3.c   |  2 +-
> >  .../selftests/bpf/progs/test_global_func5.c   |  2 +-
> >  .../selftests/bpf/progs/test_global_func6.c   |  2 +-
> >  .../selftests/bpf/progs/test_global_func7.c   |  2 +-
> >  .../selftests/bpf/progs/test_pkt_access.c     |  2 +-
> >  .../selftests/bpf/progs/test_pkt_md_access.c  |  4 +-
> >  .../selftests/bpf/progs/test_sk_assign.c      |  3 +-
> >  .../selftests/bpf/progs/test_sk_lookup_kern.c | 37 ++++++------
> >  .../selftests/bpf/progs/test_skb_helpers.c    |  2 +-
> >  .../selftests/bpf/progs/test_sockmap_update.c |  2 +-
> >  .../selftests/bpf/progs/test_tc_neigh.c       |  6 +-
> >  .../selftests/bpf/progs/test_tc_neigh_fib.c   |  6 +-
> >  .../selftests/bpf/progs/test_tc_peer.c        | 10 ++--
> >  31 files changed, 117 insertions(+), 132 deletions(-)
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/reference_tracking.c b/tools/testing/selftests/bpf/prog_tests/reference_tracking.c
> > index ded2dc8ddd79..836b8bf17fff 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/reference_tracking.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/reference_tracking.c
> > @@ -2,14 +2,14 @@
> >  #include <test_progs.h>
> >
> >  static void toggle_object_autoload_progs(const struct bpf_object *obj,
> > -                                      const char *title_load)
> > +                                      const char *name_load)
> >  {
> >       struct bpf_program *prog;
> >
> >       bpf_object__for_each_program(prog, obj) {
> > -             const char *title = bpf_program__section_name(prog);
> > +             const char *name = bpf_program__name(prog);
> >
> > -             if (!strcmp(title_load, title))
> > +             if (!strcmp(name_load, name))
> >                       bpf_program__set_autoload(prog, true);
> >               else
> >                       bpf_program__set_autoload(prog, false);
> > @@ -39,23 +39,20 @@ void test_reference_tracking(void)
> >               goto cleanup;
> >
> >       bpf_object__for_each_program(prog, obj_iter) {
> > -             const char *title;
> > +             const char *name;
> >
> >               /* Ignore .text sections */
>
> I think this comment should go as well, no longer helps understand the test

yep, I originally removed it but then it was accidentally resurrected
when I was resolving merge conflict with your vprintk() patches. I'll
drop if I need to re-spin, but otherwise we can get to this in follow
up patches.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 bpf-next 3/9] selftests/bpf: normalize all the rest SEC() uses
  2021-09-21  5:41   ` Dave Marchevsky
@ 2021-09-21 23:12     ` Andrii Nakryiko
  0 siblings, 0 replies; 27+ messages in thread
From: Andrii Nakryiko @ 2021-09-21 23:12 UTC (permalink / raw)
  To: Dave Marchevsky
  Cc: Andrii Nakryiko, bpf, Alexei Starovoitov, Daniel Borkmann, Kernel Team

On Mon, Sep 20, 2021 at 10:41 PM Dave Marchevsky <davemarchevsky@fb.com> wrote:
>
> On 9/20/21 7:43 PM, Andrii Nakryiko wrote:
> > Normalize all the other non-conforming SEC() usages across all
> > selftests. This is in preparation for libbpf to start to enforce
> > stricter SEC() rules in libbpf 1.0 mode.
> >
> > Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> > ---
> >  .../selftests/bpf/prog_tests/flow_dissector.c |  4 +--
> >  .../selftests/bpf/prog_tests/sockopt_multi.c  | 30 +++++++++----------
> >  tools/testing/selftests/bpf/progs/bpf_flow.c  |  3 +-
> >  .../bpf/progs/cg_storage_multi_isolated.c     |  4 +--
> >  .../bpf/progs/cg_storage_multi_shared.c       |  4 +--
> >  .../selftests/bpf/progs/sockopt_multi.c       |  5 ++--
> >  .../selftests/bpf/progs/test_cgroup_link.c    |  4 +--
> >  .../bpf/progs/test_misc_tcp_hdr_options.c     |  2 +-
> >  .../selftests/bpf/progs/test_sk_lookup.c      |  6 ++--
> >  .../selftests/bpf/progs/test_sockmap_listen.c |  2 +-
> >  .../progs/test_sockmap_skb_verdict_attach.c   |  2 +-
> >  .../bpf/progs/test_tcp_check_syncookie_kern.c |  2 +-
> >  .../bpf/progs/test_tcp_hdr_options.c          |  2 +-
> >  .../selftests/bpf/test_tcp_check_syncookie.sh |  2 +-
>
> Ran test_tcp_check_syncookie.sh as CI suite doesn't - works.
>

Thanks for the due diligence!

> checkpatch has some line length complaints, otherwise LGTM

Keep in mind that checkpatch is not the absolute authority. I think it
also didn't get the memo about the 100 character line limit :) But
generally yes, we should try to keep it more or less happy.

>
> Acked-by: Dave Marchevsky <davemarchevsky@fb.com>

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 bpf-next 4/9] libbpf: refactor internal sec_def handling to enable pluggability
  2021-09-20 23:43 ` [PATCH v2 bpf-next 4/9] libbpf: refactor internal sec_def handling to enable pluggability Andrii Nakryiko
@ 2021-09-22  0:42   ` Dave Marchevsky
  2021-09-22 22:06     ` Andrii Nakryiko
  0 siblings, 1 reply; 27+ messages in thread
From: Dave Marchevsky @ 2021-09-22  0:42 UTC (permalink / raw)
  To: Andrii Nakryiko, bpf, ast, daniel; +Cc: kernel-team

On 9/20/21 7:43 PM, Andrii Nakryiko wrote:   
> Refactor internals of libbpf to allow adding custom SEC() handling logic
> easily from outside of libbpf. To that effect, each SEC()-handling
> registration sets mandatory program type/expected attach type for
> a given prefix and can provide three callbacks called at different
> points of BPF program lifetime:
> 
>   - init callback for right after bpf_program is initialized and
>   prog_type/expected_attach_type is set. This happens during
>   bpf_object__open() step, close to the very end of constructing
>   bpf_object, so all the libbpf APIs for querying and updating
>   bpf_program properties should be available;

Do you have a usecase in mind that would set this? USDT? 

>   - pre-load callback is called right before BPF_PROG_LOAD command is
>   called in the kernel. This callbacks has ability to set both
>   bpf_program properties, as well as program load attributes, overriding
>   and augmenting the standard libbpf handling of them;

[...]

> @@ -6094,6 +6100,44 @@ static int bpf_object__sanitize_prog(struct bpf_object *obj, struct bpf_program
>  	return 0;
>  }
>  
> +static int libbpf_find_attach_btf_id(struct bpf_program *prog, int *btf_obj_fd, int *btf_type_id);
> +
> +/* this is called as prog->sec_def->preload_fn for libbpf-supported sec_defs */
> +static int libbpf_preload_prog(struct bpf_program *prog,
> +			       struct bpf_prog_load_params *attr, long cookie)
> +{
> +	/* old kernels might not support specifying expected_attach_type */
> +	if (prog->sec_def->is_exp_attach_type_optional &&
> +	    !kernel_supports(prog->obj, FEAT_EXP_ATTACH_TYPE))
> +		attr->expected_attach_type = 0;
> +
> +	if (prog->sec_def->is_sleepable)
> +		attr->prog_flags |= BPF_F_SLEEPABLE;
> +
> +	if ((prog->type == BPF_PROG_TYPE_TRACING ||
> +	     prog->type == BPF_PROG_TYPE_LSM ||
> +	     prog->type == BPF_PROG_TYPE_EXT) && !prog->attach_btf_id) {
> +		int btf_obj_fd = 0, btf_type_id = 0, err;
> +
> +		err = libbpf_find_attach_btf_id(prog, &btf_obj_fd, &btf_type_id);
> +		if (err)
> +			return err;
> +
> +		/* cache resolved BTF FD and BTF type ID in the prog */
> +		prog->attach_btf_obj_fd = btf_obj_fd;
> +		prog->attach_btf_id = btf_type_id;
> +
> +		/* but by now libbpf common logic is not utilizing
> +		 * prog->atach_btf_obj_fd/prog->attach_btf_id anymore because
> +		 * this callback is called after attrs were populated by
> +		 * libbpf, so this callback has to update attr explicitly here
> +		 */
> +		attr->attach_btf_obj_fd = btf_obj_fd;
> +		attr->attach_btf_id = btf_type_id;
> +	}
> +	return 0;
> +}
> +
We talked on VC about some general approach questions I had here, will
summarize. Discussion touched on changes in patches 5 and 6 as well. I thought 
the pulling of these chunks into libbpf_preload_prog made sense, but wondered
whether some of this prog-type specific functionality would also be useful to
"average" custom sec_def writer even if it's not considered 'standard libbpf
handling', e.g. custom sec_def writer whose SEC produces a PROG_TYPE_TRACING
is likely to want the find_attach_btf_id niceness as well. So perhaps something
like the ability to chain the callbacks so that sec_def writer can use libbpf's
would be useful.

Your response was that you explicitly wanted to avoid doing this because this
would result in libbpf's callbacks becoming part of the API and stability 
requirements following from that. Furthermore, you don't anticipate libbpf's
preload callback becoming very complicated and expect that the average 
custom sec_def writer will be familiar enough with libbpf to be able to pull
out whatever they need.

Response made sense to me, LGTM

Acked-by: Dave Marchevsky <davemarchevsky@fb.com>

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 bpf-next 5/9] libbpf: reduce reliance of attach_fns on sec_def internals
  2021-09-20 23:43 ` [PATCH v2 bpf-next 5/9] libbpf: reduce reliance of attach_fns on sec_def internals Andrii Nakryiko
@ 2021-09-22  1:00   ` Dave Marchevsky
  0 siblings, 0 replies; 27+ messages in thread
From: Dave Marchevsky @ 2021-09-22  1:00 UTC (permalink / raw)
  To: Andrii Nakryiko, bpf, ast, daniel; +Cc: kernel-team

On 9/20/21 7:43 PM, Andrii Nakryiko wrote:   
> Move closer to not relying on bpf_sec_def internals that won't be part
> of public API, when pluggable SEC() handlers will be allowed. Drop
> pre-calculated prefix length, and in various helpers don't rely on this
> prefix length availability. Also minimize reliance on knowing
> bpf_sec_def's prefix for few places where section prefix shortcuts are
> supported (e.g., tp vs tracepoint, raw_tp vs raw_tracepoint).
> 
> Given checking some string for having a given string-constant prefix is
> such a common operation and so annoying to be done with pure C code, add
> a small macro helper, str_has_pfx(), and reuse it throughout libbpf.c
> where prefix comparison is performed. With __builtin_constant_p() it's
> possible to have a convenient helper that checks some string for having
> a given prefix, where prefix is either string literal (or compile-time
> known string due to compiler optimization) or just a runtime string
> pointer, which is quite convenient and saves a lot of typing and string
> literal duplication.
> 
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> ---

Acked-by: Dave Marchevsky <davemarchevsky@fb.com>

>  tools/lib/bpf/libbpf.c          | 41 ++++++++++++++++++---------------
>  tools/lib/bpf/libbpf_internal.h |  7 ++++++
>  2 files changed, 30 insertions(+), 18 deletions(-)
> 

[...]

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 bpf-next 6/9] libbpf: refactor ELF section handler definitions
  2021-09-20 23:43 ` [PATCH v2 bpf-next 6/9] libbpf: refactor ELF section handler definitions Andrii Nakryiko
@ 2021-09-22  1:34   ` Dave Marchevsky
  2021-09-22 21:54     ` Andrii Nakryiko
  0 siblings, 1 reply; 27+ messages in thread
From: Dave Marchevsky @ 2021-09-22  1:34 UTC (permalink / raw)
  To: Andrii Nakryiko, bpf, ast, daniel; +Cc: kernel-team

On 9/20/21 7:43 PM, Andrii Nakryiko wrote:   
> Refactor ELF section handler definitions table to use a set of flags and
> unified SEC_DEF() macro. This allows for more succinct and table-like
> set of definitions, and allows to more easily extend the logic without
> adding more verbosity (this is utilized in later patches in the series).
> 
> This approach is also making libbpf-internal program pre-load callback
> not rely on bpf_sec_def definition, which demonstrates that future
> pluggable ELF section handlers will be able to achieve similar level of
> integration without libbpf having to expose extra types and APIs.
> 
> For starters, update SEC_DEF() definitions and make them more succinct.
> Also convert BPF_PROG_SEC() and BPF_APROG_COMPAT() definitions to
> a common SEC_DEF() use.
> 
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> ---
>  tools/lib/bpf/libbpf.c | 183 ++++++++++++++++-------------------------
>  1 file changed, 73 insertions(+), 110 deletions(-)

To summarize VC convo we had about this patch, you don't expect custom sec_def
writers to necessarily follow your sec_def_flags approach, but it's a good
demonstration that a long's worth of flags is plenty for enabling custom
functionality. And custom sec_def writers can treat the cookie as a ptr to a
config struct if they need something more complicated, without imposing the
struct format on all other sec_defs.

[...]

> @@ -7955,15 +7965,14 @@ void bpf_program__set_expected_attach_type(struct bpf_program *prog,
>  		.sec = string,						    \
>  		.prog_type = ptype,					    \
>  		.expected_attach_type = eatype,				    \
> -		.is_exp_attach_type_optional = eatype_optional,		    \
> -		.is_attachable = attachable,				    \
> -		.is_attach_btf = attach_btf,				    \
> +		.cookie = (long) (					    \
> +			(eatype_optional ? SEC_EXP_ATTACH_OPT : 0) |   \
> +			(attachable ? SEC_ATTACHABLE : 0) |		    \
> +			(attach_btf ? SEC_ATTACH_BTF : 0)		    \
> +		),							    \
>  		.preload_fn = libbpf_preload_prog,			    \
>  	}
>  
> -/* Programs that can NOT be attached. */

I found this comment and APROG_COMPAT comment useful. Not as clear to me what
SEC_NONE implies without some comment explaining or giving example. The other 
flags are more obvious to me but might be worth being explicit there as well.

> -#define BPF_PROG_SEC(string, ptype) BPF_PROG_SEC_IMPL(string, ptype, 0, 0, 0, 0)
> -
>  /* Programs that can be attached. */
>  #define BPF_APROG_SEC(string, ptype, atype) \
>  	BPF_PROG_SEC_IMPL(string, ptype, atype, true, 1, 0)
> @@ -7976,14 +7985,11 @@ void bpf_program__set_expected_attach_type(struct bpf_program *prog,
>  #define BPF_PROG_BTF(string, ptype, eatype) \
>  	BPF_PROG_SEC_IMPL(string, ptype, eatype, false, 0, 1)
>  
> -/* Programs that can be attached but attach type can't be identified by section
> - * name. Kept for backward compatibility.
> - */
> -#define BPF_APROG_COMPAT(string, ptype) BPF_PROG_SEC(string, ptype)
> -
> -#define SEC_DEF(sec_pfx, ptype, ...) {					    \
> +#define SEC_DEF(sec_pfx, ptype, atype, flags, ...) {			    \
>  	.sec = sec_pfx,							    \
>  	.prog_type = BPF_PROG_TYPE_##ptype,				    \
> +	.expected_attach_type = atype,					    \
> +	.cookie = (long)(flags),					    \
>  	.preload_fn = libbpf_preload_prog,				    \
>  	__VA_ARGS__							    \
>  }
> @@ -7996,92 +8002,49 @@ 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[] = {
> -	BPF_PROG_SEC("socket",			BPF_PROG_TYPE_SOCKET_FILTER),
> +	SEC_DEF("socket",		SOCKET_FILTER, 0, SEC_NONE),

Didn't know how strictly you felt about checkpatch line-length complaints,
won't comment on them further since you mentioned 100 chars being the new
standard. But would complain about the alignment here and elsewhere in 
changes to section_defs even if checkpatch didn't exist :)

>  	BPF_EAPROG_SEC("sk_reuseport/migrate",	BPF_PROG_TYPE_SK_REUSEPORT,
>  						BPF_SK_REUSEPORT_SELECT_OR_MIGRATE),
>  	BPF_EAPROG_SEC("sk_reuseport",		BPF_PROG_TYPE_SK_REUSEPORT,
>  						BPF_SK_REUSEPORT_SELECT),

[...]

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 bpf-next 7/9] libbpf: complete SEC() table unification for BPF_APROG_SEC/BPF_EAPROG_SEC
  2021-09-20 23:43 ` [PATCH v2 bpf-next 7/9] libbpf: complete SEC() table unification for BPF_APROG_SEC/BPF_EAPROG_SEC Andrii Nakryiko
@ 2021-09-22  1:42   ` Dave Marchevsky
  2021-09-22 21:55     ` Andrii Nakryiko
  0 siblings, 1 reply; 27+ messages in thread
From: Dave Marchevsky @ 2021-09-22  1:42 UTC (permalink / raw)
  To: Andrii Nakryiko, bpf, ast, daniel; +Cc: kernel-team

On 9/20/21 7:43 PM, Andrii Nakryiko wrote:   
> Complete SEC() table refactoring towards unified form by rewriting
> BPF_APROG_SEC and BPF_EAPROG_SEC definitions with
> SEC_DEF(SEC_ATTACHABLE_OPT) (for optional expected_attach_type) and
> SEC_DEF(SEC_ATTACHABLE) (mandatory expected_attach_type), respectively.
> Drop BPF_APROG_SEC, BPF_EAPROG_SEC, and BPF_PROG_SEC_IMPL macros after
> that, leaving SEC_DEF() macro as the only one used.
> 
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> ---
>  tools/lib/bpf/libbpf.c | 136 +++++++++++------------------------------
>  1 file changed, 35 insertions(+), 101 deletions(-)
> 
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 734be7dc52a0..56082865ceff 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -7959,32 +7959,6 @@ void bpf_program__set_expected_attach_type(struct bpf_program *prog,
>  	prog->expected_attach_type = type;
>  }
>  
> -#define BPF_PROG_SEC_IMPL(string, ptype, eatype, eatype_optional,	    \
> -			  attachable, attach_btf)			    \
> -	{								    \
> -		.sec = string,						    \
> -		.prog_type = ptype,					    \
> -		.expected_attach_type = eatype,				    \
> -		.cookie = (long) (					    \
> -			(eatype_optional ? SEC_EXP_ATTACH_OPT : 0) |   \
> -			(attachable ? SEC_ATTACHABLE : 0) |		    \
> -			(attach_btf ? SEC_ATTACH_BTF : 0)		    \
> -		),							    \
> -		.preload_fn = libbpf_preload_prog,			    \
> -	}
> -
> -/* Programs that can be attached. */
> -#define BPF_APROG_SEC(string, ptype, atype) \
> -	BPF_PROG_SEC_IMPL(string, ptype, atype, true, 1, 0)
> -
> -/* Programs that must specify expected attach type at load time. */
> -#define BPF_EAPROG_SEC(string, ptype, eatype) \
> -	BPF_PROG_SEC_IMPL(string, ptype, eatype, false, 1, 0)
> -
> -/* Programs that use BTF to identify attach point */
> -#define BPF_PROG_BTF(string, ptype, eatype) \
> -	BPF_PROG_SEC_IMPL(string, ptype, eatype, false, 0, 1)
> -

Similar thoughts about comment usefulness as patch 6.

>  #define SEC_DEF(sec_pfx, ptype, atype, flags, ...) {			    \
>  	.sec = sec_pfx,							    \
>  	.prog_type = BPF_PROG_TYPE_##ptype,				    \
> @@ -8003,10 +7977,8 @@ 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),
> -	BPF_EAPROG_SEC("sk_reuseport/migrate",	BPF_PROG_TYPE_SK_REUSEPORT,
> -						BPF_SK_REUSEPORT_SELECT_OR_MIGRATE),
> -	BPF_EAPROG_SEC("sk_reuseport",		BPF_PROG_TYPE_SK_REUSEPORT,
> -						BPF_SK_REUSEPORT_SELECT),
> +	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),

Ah, I see that after this patch the alignment issue from patch 6 is better.
Nevermind then.

>  	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("sk_skb/stream_parser",	SK_SKB, BPF_SK_SKB_STREAM_PARSER, SEC_ATTACHABLE_OPT),
> +	SEC_DEF("sk_skb/stream_verdict",SK_SKB, BPF_SK_SKB_STREAM_VERDICT, SEC_ATTACHABLE_OPT),

checkpatch really doesn't like the lack of space after comma here, I agree
with it.

>  	SEC_DEF("sk_skb",		SK_SKB, 0, SEC_NONE),
> -	BPF_APROG_SEC("sk_msg",			BPF_PROG_TYPE_SK_MSG,
> -						BPF_SK_MSG_VERDICT),
> -	BPF_APROG_SEC("lirc_mode2",		BPF_PROG_TYPE_LIRC_MODE2,

[...]


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 bpf-next 8/9] libbpf: add opt-in strict BPF program section name handling logic
  2021-09-20 23:43 ` [PATCH v2 bpf-next 8/9] libbpf: add opt-in strict BPF program section name handling logic Andrii Nakryiko
@ 2021-09-22  1:53   ` Dave Marchevsky
  2021-09-22 21:57     ` Andrii Nakryiko
  0 siblings, 1 reply; 27+ messages in thread
From: Dave Marchevsky @ 2021-09-22  1:53 UTC (permalink / raw)
  To: Andrii Nakryiko, bpf, ast, daniel; +Cc: kernel-team

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

[...]

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 bpf-next 9/9] selftests/bpf: switch sk_lookup selftests to strict SEC("sk_lookup") use
  2021-09-20 23:43 ` [PATCH v2 bpf-next 9/9] selftests/bpf: switch sk_lookup selftests to strict SEC("sk_lookup") use Andrii Nakryiko
@ 2021-09-22  2:37   ` Dave Marchevsky
  0 siblings, 0 replies; 27+ messages in thread
From: Dave Marchevsky @ 2021-09-22  2:37 UTC (permalink / raw)
  To: Andrii Nakryiko, bpf, ast, daniel; +Cc: kernel-team

On 9/20/21 7:43 PM, Andrii Nakryiko wrote:   
> Update "sk_lookup/" definition to be a stand-alone type specifier,
> with backwards-compatible prefix match logic in non-libbpf-1.0 mode.
> 
> Currently in selftests all the "sk_lookup/<whatever>" uses just use
> <whatever> for duplicated unique name encoding, which is redundant as
> BPF program's name (C function name) uniquely and descriptively
> identifies the intended use for such BPF programs.
> 
> With libbpf's SEC_DEF("sk_lookup") definition updated, switch existing
> sk_lookup programs to use "unqualified" SEC("sk_lookup") section names,
> with no random text after it.
> 
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> ---
Acked-by: Dave Marchevsky <davemarchevsky@fb.com>

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 bpf-next 6/9] libbpf: refactor ELF section handler definitions
  2021-09-22  1:34   ` Dave Marchevsky
@ 2021-09-22 21:54     ` Andrii Nakryiko
  0 siblings, 0 replies; 27+ messages in thread
From: Andrii Nakryiko @ 2021-09-22 21:54 UTC (permalink / raw)
  To: Dave Marchevsky
  Cc: Andrii Nakryiko, bpf, Alexei Starovoitov, Daniel Borkmann, Kernel Team

On Tue, Sep 21, 2021 at 6:34 PM Dave Marchevsky <davemarchevsky@fb.com> wrote:
>
> On 9/20/21 7:43 PM, Andrii Nakryiko wrote:
> > Refactor ELF section handler definitions table to use a set of flags and
> > unified SEC_DEF() macro. This allows for more succinct and table-like
> > set of definitions, and allows to more easily extend the logic without
> > adding more verbosity (this is utilized in later patches in the series).
> >
> > This approach is also making libbpf-internal program pre-load callback
> > not rely on bpf_sec_def definition, which demonstrates that future
> > pluggable ELF section handlers will be able to achieve similar level of
> > integration without libbpf having to expose extra types and APIs.
> >
> > For starters, update SEC_DEF() definitions and make them more succinct.
> > Also convert BPF_PROG_SEC() and BPF_APROG_COMPAT() definitions to
> > a common SEC_DEF() use.
> >
> > Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> > ---
> >  tools/lib/bpf/libbpf.c | 183 ++++++++++++++++-------------------------
> >  1 file changed, 73 insertions(+), 110 deletions(-)
>
> To summarize VC convo we had about this patch, you don't expect custom sec_def
> writers to necessarily follow your sec_def_flags approach, but it's a good
> demonstration that a long's worth of flags is plenty for enabling custom
> functionality. And custom sec_def writers can treat the cookie as a ptr to a
> config struct if they need something more complicated, without imposing the
> struct format on all other sec_defs.

Right.

>
> [...]
>
> > @@ -7955,15 +7965,14 @@ void bpf_program__set_expected_attach_type(struct bpf_program *prog,
> >               .sec = string,                                              \
> >               .prog_type = ptype,                                         \
> >               .expected_attach_type = eatype,                             \
> > -             .is_exp_attach_type_optional = eatype_optional,             \
> > -             .is_attachable = attachable,                                \
> > -             .is_attach_btf = attach_btf,                                \
> > +             .cookie = (long) (                                          \
> > +                     (eatype_optional ? SEC_EXP_ATTACH_OPT : 0) |   \
> > +                     (attachable ? SEC_ATTACHABLE : 0) |                 \
> > +                     (attach_btf ? SEC_ATTACH_BTF : 0)                   \
> > +             ),                                                          \
> >               .preload_fn = libbpf_preload_prog,                          \
> >       }
> >
> > -/* Programs that can NOT be attached. */
>
> I found this comment and APROG_COMPAT comment useful. Not as clear to me what
> SEC_NONE implies without some comment explaining or giving example. The other
> flags are more obvious to me but might be worth being explicit there as well.

I actually find that particular comment misleading and harmful, tbh.
All BPF programs are attachable to some BPF hook, it's the whole BPF
workflow. In this case it meant that these programs can't be attached
with BPF_PROG_ATTACH command of bpf() syscall. Which is technically in
a legacy mode and it is nowadays recommended to use bpf_link-based
approach instead. So this comment isn't that useful or precise
anymore.

>
> > -#define BPF_PROG_SEC(string, ptype) BPF_PROG_SEC_IMPL(string, ptype, 0, 0, 0, 0)
> > -
> >  /* Programs that can be attached. */
> >  #define BPF_APROG_SEC(string, ptype, atype) \
> >       BPF_PROG_SEC_IMPL(string, ptype, atype, true, 1, 0)
> > @@ -7976,14 +7985,11 @@ void bpf_program__set_expected_attach_type(struct bpf_program *prog,
> >  #define BPF_PROG_BTF(string, ptype, eatype) \
> >       BPF_PROG_SEC_IMPL(string, ptype, eatype, false, 0, 1)
> >
> > -/* Programs that can be attached but attach type can't be identified by section
> > - * name. Kept for backward compatibility.
> > - */
> > -#define BPF_APROG_COMPAT(string, ptype) BPF_PROG_SEC(string, ptype)
> > -
> > -#define SEC_DEF(sec_pfx, ptype, ...) {                                           \
> > +#define SEC_DEF(sec_pfx, ptype, atype, flags, ...) {                     \
> >       .sec = sec_pfx,                                                     \
> >       .prog_type = BPF_PROG_TYPE_##ptype,                                 \
> > +     .expected_attach_type = atype,                                      \
> > +     .cookie = (long)(flags),                                            \
> >       .preload_fn = libbpf_preload_prog,                                  \
> >       __VA_ARGS__                                                         \
> >  }
> > @@ -7996,92 +8002,49 @@ 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[] = {
> > -     BPF_PROG_SEC("socket",                  BPF_PROG_TYPE_SOCKET_FILTER),
> > +     SEC_DEF("socket",               SOCKET_FILTER, 0, SEC_NONE),
>
> Didn't know how strictly you felt about checkpatch line-length complaints,
> won't comment on them further since you mentioned 100 chars being the new
> standard. But would complain about the alignment here and elsewhere in
> changes to section_defs even if checkpatch didn't exist :)

the goal here was to have one entry per line for quick and easy
overview of all supported section definitions

>
> >       BPF_EAPROG_SEC("sk_reuseport/migrate",  BPF_PROG_TYPE_SK_REUSEPORT,
> >                                               BPF_SK_REUSEPORT_SELECT_OR_MIGRATE),
> >       BPF_EAPROG_SEC("sk_reuseport",          BPF_PROG_TYPE_SK_REUSEPORT,
> >                                               BPF_SK_REUSEPORT_SELECT),
>
> [...]

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 bpf-next 7/9] libbpf: complete SEC() table unification for BPF_APROG_SEC/BPF_EAPROG_SEC
  2021-09-22  1:42   ` Dave Marchevsky
@ 2021-09-22 21:55     ` Andrii Nakryiko
  2021-09-22 22:12       ` Andrii Nakryiko
  0 siblings, 1 reply; 27+ messages in thread
From: Andrii Nakryiko @ 2021-09-22 21:55 UTC (permalink / raw)
  To: Dave Marchevsky
  Cc: Andrii Nakryiko, bpf, Alexei Starovoitov, Daniel Borkmann, Kernel Team

On Tue, Sep 21, 2021 at 6:42 PM Dave Marchevsky <davemarchevsky@fb.com> wrote:
>
> On 9/20/21 7:43 PM, Andrii Nakryiko wrote:
> > Complete SEC() table refactoring towards unified form by rewriting
> > BPF_APROG_SEC and BPF_EAPROG_SEC definitions with
> > SEC_DEF(SEC_ATTACHABLE_OPT) (for optional expected_attach_type) and
> > SEC_DEF(SEC_ATTACHABLE) (mandatory expected_attach_type), respectively.
> > Drop BPF_APROG_SEC, BPF_EAPROG_SEC, and BPF_PROG_SEC_IMPL macros after
> > that, leaving SEC_DEF() macro as the only one used.
> >
> > Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> > ---
> >  tools/lib/bpf/libbpf.c | 136 +++++++++++------------------------------
> >  1 file changed, 35 insertions(+), 101 deletions(-)
> >
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index 734be7dc52a0..56082865ceff 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -7959,32 +7959,6 @@ void bpf_program__set_expected_attach_type(struct bpf_program *prog,
> >       prog->expected_attach_type = type;
> >  }
> >
> > -#define BPF_PROG_SEC_IMPL(string, ptype, eatype, eatype_optional,        \
> > -                       attachable, attach_btf)                           \
> > -     {                                                                   \
> > -             .sec = string,                                              \
> > -             .prog_type = ptype,                                         \
> > -             .expected_attach_type = eatype,                             \
> > -             .cookie = (long) (                                          \
> > -                     (eatype_optional ? SEC_EXP_ATTACH_OPT : 0) |   \
> > -                     (attachable ? SEC_ATTACHABLE : 0) |                 \
> > -                     (attach_btf ? SEC_ATTACH_BTF : 0)                   \
> > -             ),                                                          \
> > -             .preload_fn = libbpf_preload_prog,                          \
> > -     }
> > -
> > -/* Programs that can be attached. */
> > -#define BPF_APROG_SEC(string, ptype, atype) \
> > -     BPF_PROG_SEC_IMPL(string, ptype, atype, true, 1, 0)
> > -
> > -/* Programs that must specify expected attach type at load time. */
> > -#define BPF_EAPROG_SEC(string, ptype, eatype) \
> > -     BPF_PROG_SEC_IMPL(string, ptype, eatype, false, 1, 0)
> > -
> > -/* Programs that use BTF to identify attach point */
> > -#define BPF_PROG_BTF(string, ptype, eatype) \
> > -     BPF_PROG_SEC_IMPL(string, ptype, eatype, false, 0, 1)
> > -
>
> Similar thoughts about comment usefulness as patch 6.
>

I'll add succinct comments to each SEC_xxx flag, trying to emphasize
what it is about

> >  #define SEC_DEF(sec_pfx, ptype, atype, flags, ...) {                     \
> >       .sec = sec_pfx,                                                     \
> >       .prog_type = BPF_PROG_TYPE_##ptype,                                 \
> > @@ -8003,10 +7977,8 @@ 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),
> > -     BPF_EAPROG_SEC("sk_reuseport/migrate",  BPF_PROG_TYPE_SK_REUSEPORT,
> > -                                             BPF_SK_REUSEPORT_SELECT_OR_MIGRATE),
> > -     BPF_EAPROG_SEC("sk_reuseport",          BPF_PROG_TYPE_SK_REUSEPORT,
> > -                                             BPF_SK_REUSEPORT_SELECT),
> > +     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),
>
> Ah, I see that after this patch the alignment issue from patch 6 is better.
> Nevermind then.
>
> >       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("sk_skb/stream_parser", SK_SKB, BPF_SK_SKB_STREAM_PARSER, SEC_ATTACHABLE_OPT),
> > +     SEC_DEF("sk_skb/stream_verdict",SK_SKB, BPF_SK_SKB_STREAM_VERDICT, SEC_ATTACHABLE_OPT),
>
> checkpatch really doesn't like the lack of space after comma here, I agree
> with it.

it was either this, or one extra tab for all other entries making
every line longer still. But I guess I'll just do s/SEC_DEF/SECDEF/
and get that space back :)

But keep in mind, checkpatch.pl is a guide, not a law.

>
> >       SEC_DEF("sk_skb",               SK_SKB, 0, SEC_NONE),
> > -     BPF_APROG_SEC("sk_msg",                 BPF_PROG_TYPE_SK_MSG,
> > -                                             BPF_SK_MSG_VERDICT),
> > -     BPF_APROG_SEC("lirc_mode2",             BPF_PROG_TYPE_LIRC_MODE2,
>
> [...]
>

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 bpf-next 8/9] libbpf: add opt-in strict BPF program section name handling logic
  2021-09-22  1:53   ` Dave Marchevsky
@ 2021-09-22 21:57     ` Andrii Nakryiko
  0 siblings, 0 replies; 27+ messages in thread
From: Andrii Nakryiko @ 2021-09-22 21:57 UTC (permalink / raw)
  To: Dave Marchevsky
  Cc: Andrii Nakryiko, bpf, Alexei Starovoitov, Daniel Borkmann, Kernel Team

On Tue, Sep 21, 2021 at 6:53 PM Dave Marchevsky <davemarchevsky@fb.com> 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@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.

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),
>
> [...]

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 bpf-next 4/9] libbpf: refactor internal sec_def handling to enable pluggability
  2021-09-22  0:42   ` Dave Marchevsky
@ 2021-09-22 22:06     ` Andrii Nakryiko
  0 siblings, 0 replies; 27+ messages in thread
From: Andrii Nakryiko @ 2021-09-22 22:06 UTC (permalink / raw)
  To: Dave Marchevsky
  Cc: Andrii Nakryiko, bpf, Alexei Starovoitov, Daniel Borkmann, Kernel Team

On Tue, Sep 21, 2021 at 5:42 PM Dave Marchevsky <davemarchevsky@fb.com> wrote:
>
> On 9/20/21 7:43 PM, Andrii Nakryiko wrote:
> > Refactor internals of libbpf to allow adding custom SEC() handling logic
> > easily from outside of libbpf. To that effect, each SEC()-handling
> > registration sets mandatory program type/expected attach type for
> > a given prefix and can provide three callbacks called at different
> > points of BPF program lifetime:
> >
> >   - init callback for right after bpf_program is initialized and
> >   prog_type/expected_attach_type is set. This happens during
> >   bpf_object__open() step, close to the very end of constructing
> >   bpf_object, so all the libbpf APIs for querying and updating
> >   bpf_program properties should be available;
>
> Do you have a usecase in mind that would set this? USDT?

Don't have specific use case in mind, but this callback gives the
fully constructed `struct bpf_program` access to those custom
callbacks, so if there is any additional book keeping/feature
checking/whatever that needs to be done, this will be the earliest
point where some library/framework will discover the program. Felt
like an important addition, even if libbpf internally has no need for
it (because libbpf can always access struct bpf_program through its
own means).

>
> >   - pre-load callback is called right before BPF_PROG_LOAD command is
> >   called in the kernel. This callbacks has ability to set both
> >   bpf_program properties, as well as program load attributes, overriding
> >   and augmenting the standard libbpf handling of them;
>
> [...]
>
> > @@ -6094,6 +6100,44 @@ static int bpf_object__sanitize_prog(struct bpf_object *obj, struct bpf_program
> >       return 0;
> >  }
> >
> > +static int libbpf_find_attach_btf_id(struct bpf_program *prog, int *btf_obj_fd, int *btf_type_id);
> > +
> > +/* this is called as prog->sec_def->preload_fn for libbpf-supported sec_defs */
> > +static int libbpf_preload_prog(struct bpf_program *prog,
> > +                            struct bpf_prog_load_params *attr, long cookie)
> > +{
> > +     /* old kernels might not support specifying expected_attach_type */
> > +     if (prog->sec_def->is_exp_attach_type_optional &&
> > +         !kernel_supports(prog->obj, FEAT_EXP_ATTACH_TYPE))
> > +             attr->expected_attach_type = 0;
> > +
> > +     if (prog->sec_def->is_sleepable)
> > +             attr->prog_flags |= BPF_F_SLEEPABLE;
> > +
> > +     if ((prog->type == BPF_PROG_TYPE_TRACING ||
> > +          prog->type == BPF_PROG_TYPE_LSM ||
> > +          prog->type == BPF_PROG_TYPE_EXT) && !prog->attach_btf_id) {
> > +             int btf_obj_fd = 0, btf_type_id = 0, err;
> > +
> > +             err = libbpf_find_attach_btf_id(prog, &btf_obj_fd, &btf_type_id);
> > +             if (err)
> > +                     return err;
> > +
> > +             /* cache resolved BTF FD and BTF type ID in the prog */
> > +             prog->attach_btf_obj_fd = btf_obj_fd;
> > +             prog->attach_btf_id = btf_type_id;
> > +
> > +             /* but by now libbpf common logic is not utilizing
> > +              * prog->atach_btf_obj_fd/prog->attach_btf_id anymore because
> > +              * this callback is called after attrs were populated by
> > +              * libbpf, so this callback has to update attr explicitly here
> > +              */
> > +             attr->attach_btf_obj_fd = btf_obj_fd;
> > +             attr->attach_btf_id = btf_type_id;
> > +     }
> > +     return 0;
> > +}
> > +
> We talked on VC about some general approach questions I had here, will
> summarize. Discussion touched on changes in patches 5 and 6 as well. I thought
> the pulling of these chunks into libbpf_preload_prog made sense, but wondered
> whether some of this prog-type specific functionality would also be useful to
> "average" custom sec_def writer even if it's not considered 'standard libbpf
> handling', e.g. custom sec_def writer whose SEC produces a PROG_TYPE_TRACING
> is likely to want the find_attach_btf_id niceness as well. So perhaps something
> like the ability to chain the callbacks so that sec_def writer can use libbpf's
> would be useful.
>
> Your response was that you explicitly wanted to avoid doing this because this
> would result in libbpf's callbacks becoming part of the API and stability
> requirements following from that. Furthermore, you don't anticipate libbpf's
> preload callback becoming very complicated and expect that the average
> custom sec_def writer will be familiar enough with libbpf to be able to pull
> out whatever they need.
>
> Response made sense to me, LGTM
>
> Acked-by: Dave Marchevsky <davemarchevsky@fb.com>

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 bpf-next 7/9] libbpf: complete SEC() table unification for BPF_APROG_SEC/BPF_EAPROG_SEC
  2021-09-22 21:55     ` Andrii Nakryiko
@ 2021-09-22 22:12       ` Andrii Nakryiko
  0 siblings, 0 replies; 27+ messages in thread
From: Andrii Nakryiko @ 2021-09-22 22:12 UTC (permalink / raw)
  To: Dave Marchevsky
  Cc: Andrii Nakryiko, bpf, Alexei Starovoitov, Daniel Borkmann, Kernel Team

On Wed, Sep 22, 2021 at 2:55 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Tue, Sep 21, 2021 at 6:42 PM Dave Marchevsky <davemarchevsky@fb.com> wrote:
> >
> > On 9/20/21 7:43 PM, Andrii Nakryiko wrote:
> > > Complete SEC() table refactoring towards unified form by rewriting
> > > BPF_APROG_SEC and BPF_EAPROG_SEC definitions with
> > > SEC_DEF(SEC_ATTACHABLE_OPT) (for optional expected_attach_type) and
> > > SEC_DEF(SEC_ATTACHABLE) (mandatory expected_attach_type), respectively.
> > > Drop BPF_APROG_SEC, BPF_EAPROG_SEC, and BPF_PROG_SEC_IMPL macros after
> > > that, leaving SEC_DEF() macro as the only one used.
> > >
> > > Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> > > ---
> > >  tools/lib/bpf/libbpf.c | 136 +++++++++++------------------------------
> > >  1 file changed, 35 insertions(+), 101 deletions(-)
> > >
> > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > > index 734be7dc52a0..56082865ceff 100644
> > > --- a/tools/lib/bpf/libbpf.c
> > > +++ b/tools/lib/bpf/libbpf.c
> > > @@ -7959,32 +7959,6 @@ void bpf_program__set_expected_attach_type(struct bpf_program *prog,
> > >       prog->expected_attach_type = type;
> > >  }
> > >
> > > -#define BPF_PROG_SEC_IMPL(string, ptype, eatype, eatype_optional,        \
> > > -                       attachable, attach_btf)                           \
> > > -     {                                                                   \
> > > -             .sec = string,                                              \
> > > -             .prog_type = ptype,                                         \
> > > -             .expected_attach_type = eatype,                             \
> > > -             .cookie = (long) (                                          \
> > > -                     (eatype_optional ? SEC_EXP_ATTACH_OPT : 0) |   \
> > > -                     (attachable ? SEC_ATTACHABLE : 0) |                 \
> > > -                     (attach_btf ? SEC_ATTACH_BTF : 0)                   \
> > > -             ),                                                          \
> > > -             .preload_fn = libbpf_preload_prog,                          \
> > > -     }
> > > -
> > > -/* Programs that can be attached. */
> > > -#define BPF_APROG_SEC(string, ptype, atype) \
> > > -     BPF_PROG_SEC_IMPL(string, ptype, atype, true, 1, 0)
> > > -
> > > -/* Programs that must specify expected attach type at load time. */
> > > -#define BPF_EAPROG_SEC(string, ptype, eatype) \
> > > -     BPF_PROG_SEC_IMPL(string, ptype, eatype, false, 1, 0)
> > > -
> > > -/* Programs that use BTF to identify attach point */
> > > -#define BPF_PROG_BTF(string, ptype, eatype) \
> > > -     BPF_PROG_SEC_IMPL(string, ptype, eatype, false, 0, 1)
> > > -
> >
> > Similar thoughts about comment usefulness as patch 6.
> >
>
> I'll add succinct comments to each SEC_xxx flag, trying to emphasize
> what it is about
>
> > >  #define SEC_DEF(sec_pfx, ptype, atype, flags, ...) {                     \
> > >       .sec = sec_pfx,                                                     \
> > >       .prog_type = BPF_PROG_TYPE_##ptype,                                 \
> > > @@ -8003,10 +7977,8 @@ 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),
> > > -     BPF_EAPROG_SEC("sk_reuseport/migrate",  BPF_PROG_TYPE_SK_REUSEPORT,
> > > -                                             BPF_SK_REUSEPORT_SELECT_OR_MIGRATE),
> > > -     BPF_EAPROG_SEC("sk_reuseport",          BPF_PROG_TYPE_SK_REUSEPORT,
> > > -                                             BPF_SK_REUSEPORT_SELECT),
> > > +     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),
> >
> > Ah, I see that after this patch the alignment issue from patch 6 is better.
> > Nevermind then.
> >
> > >       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("sk_skb/stream_parser", SK_SKB, BPF_SK_SKB_STREAM_PARSER, SEC_ATTACHABLE_OPT),
> > > +     SEC_DEF("sk_skb/stream_verdict",SK_SKB, BPF_SK_SKB_STREAM_VERDICT, SEC_ATTACHABLE_OPT),
> >
> > checkpatch really doesn't like the lack of space after comma here, I agree
> > with it.
>
> it was either this, or one extra tab for all other entries making
> every line longer still. But I guess I'll just do s/SEC_DEF/SECDEF/
> and get that space back :)
>

Nah, it looks too ugly, like a pile of letters. I'll leave it as is,
checkpatch.pl will have to deal with this ;)

> But keep in mind, checkpatch.pl is a guide, not a law.
>
> >
> > >       SEC_DEF("sk_skb",               SK_SKB, 0, SEC_NONE),
> > > -     BPF_APROG_SEC("sk_msg",                 BPF_PROG_TYPE_SK_MSG,
> > > -                                             BPF_SK_MSG_VERDICT),
> > > -     BPF_APROG_SEC("lirc_mode2",             BPF_PROG_TYPE_LIRC_MODE2,
> >
> > [...]
> >

^ permalink raw reply	[flat|nested] 27+ messages in thread

end of thread, other threads:[~2021-09-22 22:12 UTC | newest]

Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-20 23:43 [PATCH v2 bpf-next 0/9] libbpf: stricter BPF program section name handling Andrii Nakryiko
2021-09-20 23:43 ` [PATCH v2 bpf-next 1/9] selftests/bpf: normalize XDP section names in selftests Andrii Nakryiko
2021-09-21  4:55   ` Dave Marchevsky
2021-09-21 23:08     ` Andrii Nakryiko
2021-09-20 23:43 ` [PATCH v2 bpf-next 2/9] selftests/bpf: normalize SEC("classifier") usage Andrii Nakryiko
2021-09-21  5:20   ` Dave Marchevsky
2021-09-21 23:10     ` Andrii Nakryiko
2021-09-20 23:43 ` [PATCH v2 bpf-next 3/9] selftests/bpf: normalize all the rest SEC() uses Andrii Nakryiko
2021-09-21  5:41   ` Dave Marchevsky
2021-09-21 23:12     ` Andrii Nakryiko
2021-09-20 23:43 ` [PATCH v2 bpf-next 4/9] libbpf: refactor internal sec_def handling to enable pluggability Andrii Nakryiko
2021-09-22  0:42   ` Dave Marchevsky
2021-09-22 22:06     ` Andrii Nakryiko
2021-09-20 23:43 ` [PATCH v2 bpf-next 5/9] libbpf: reduce reliance of attach_fns on sec_def internals Andrii Nakryiko
2021-09-22  1:00   ` Dave Marchevsky
2021-09-20 23:43 ` [PATCH v2 bpf-next 6/9] libbpf: refactor ELF section handler definitions Andrii Nakryiko
2021-09-22  1:34   ` Dave Marchevsky
2021-09-22 21:54     ` Andrii Nakryiko
2021-09-20 23:43 ` [PATCH v2 bpf-next 7/9] libbpf: complete SEC() table unification for BPF_APROG_SEC/BPF_EAPROG_SEC Andrii Nakryiko
2021-09-22  1:42   ` Dave Marchevsky
2021-09-22 21:55     ` Andrii Nakryiko
2021-09-22 22:12       ` Andrii Nakryiko
2021-09-20 23:43 ` [PATCH v2 bpf-next 8/9] libbpf: add opt-in strict BPF program section name handling logic Andrii Nakryiko
2021-09-22  1:53   ` Dave Marchevsky
2021-09-22 21:57     ` Andrii Nakryiko
2021-09-20 23:43 ` [PATCH v2 bpf-next 9/9] selftests/bpf: switch sk_lookup selftests to strict SEC("sk_lookup") use Andrii Nakryiko
2021-09-22  2:37   ` Dave Marchevsky

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.