[PATCH bpf-next] selftests/bpf: test_sockmap, use section names understood by libbpf

[PATCH bpf-next] selftests/bpf: test_sockmap, use section names understood by libbpf

[Jakub Sitnicki]

libbpf can deduce program type and attach type from the ELF section name.
We don't need to pass it out-of-band if we switch to libbpf convention [1].

[1] https://docs.kernel.org/bpf/libbpf/program_types.html

Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
---
 .../selftests/bpf/progs/test_sockmap_kern.h   | 17 +++++-----
 tools/testing/selftests/bpf/test_sockmap.c    | 31 -------------------
 2 files changed, 9 insertions(+), 39 deletions(-)

diff --git a/tools/testing/selftests/bpf/progs/test_sockmap_kern.h b/tools/testing/selftests/bpf/progs/test_sockmap_kern.h
index 99d2ea9fb658..3dff0813730b 100644
--- a/tools/testing/selftests/bpf/progs/test_sockmap_kern.h
+++ b/tools/testing/selftests/bpf/progs/test_sockmap_kern.h
@@ -92,7 +92,7 @@ struct {
 	__uint(value_size, sizeof(int));
 } tls_sock_map SEC(".maps");
 
-SEC("sk_skb1")
+SEC("sk_skb/stream_parser")
 int bpf_prog1(struct __sk_buff *skb)
 {
 	int *f, two = 2;
@@ -104,7 +104,7 @@ int bpf_prog1(struct __sk_buff *skb)
 	return skb->len;
 }
 
-SEC("sk_skb2")
+SEC("sk_skb/stream_verdict")
 int bpf_prog2(struct __sk_buff *skb)
 {
 	__u32 lport = skb->local_port;
@@ -151,7 +151,7 @@ static inline void bpf_write_pass(struct __sk_buff *skb, int offset)
 		memcpy(c + offset, "PASS", 4);
 }
 
-SEC("sk_skb3")
+SEC("sk_skb/stream_verdict")
 int bpf_prog3(struct __sk_buff *skb)
 {
 	int err, *f, ret = SK_PASS;
@@ -233,7 +233,7 @@ int bpf_sockmap(struct bpf_sock_ops *skops)
 	return 0;
 }
 
-SEC("sk_msg1")
+SEC("sk_msg")
 int bpf_prog4(struct sk_msg_md *msg)
 {
 	int *bytes, zero = 0, one = 1, two = 2, three = 3, four = 4, five = 5;
@@ -263,7 +263,7 @@ int bpf_prog4(struct sk_msg_md *msg)
 	return SK_PASS;
 }
 
-SEC("sk_msg2")
+SEC("sk_msg")
 int bpf_prog6(struct sk_msg_md *msg)
 {
 	int zero = 0, one = 1, two = 2, three = 3, four = 4, five = 5, key = 0;
@@ -308,7 +308,7 @@ int bpf_prog6(struct sk_msg_md *msg)
 #endif
 }
 
-SEC("sk_msg3")
+SEC("sk_msg")
 int bpf_prog8(struct sk_msg_md *msg)
 {
 	void *data_end = (void *)(long) msg->data_end;
@@ -329,7 +329,8 @@ int bpf_prog8(struct sk_msg_md *msg)
 
 	return SK_PASS;
 }
-SEC("sk_msg4")
+
+SEC("sk_msg")
 int bpf_prog9(struct sk_msg_md *msg)
 {
 	void *data_end = (void *)(long) msg->data_end;
@@ -347,7 +348,7 @@ int bpf_prog9(struct sk_msg_md *msg)
 	return SK_PASS;
 }
 
-SEC("sk_msg5")
+SEC("sk_msg")
 int bpf_prog10(struct sk_msg_md *msg)
 {
 	int *bytes, *start, *end, *start_push, *end_push, *start_pop, *pop;
diff --git a/tools/testing/selftests/bpf/test_sockmap.c b/tools/testing/selftests/bpf/test_sockmap.c
index 4499b3cfc3a6..ddc6a9cef36f 100644
--- a/tools/testing/selftests/bpf/test_sockmap.c
+++ b/tools/testing/selftests/bpf/test_sockmap.c
@@ -1783,30 +1783,6 @@ char *map_names[] = {
 	"tls_sock_map",
 };
 
-int prog_attach_type[] = {
-	BPF_SK_SKB_STREAM_PARSER,
-	BPF_SK_SKB_STREAM_VERDICT,
-	BPF_SK_SKB_STREAM_VERDICT,
-	BPF_CGROUP_SOCK_OPS,
-	BPF_SK_MSG_VERDICT,
-	BPF_SK_MSG_VERDICT,
-	BPF_SK_MSG_VERDICT,
-	BPF_SK_MSG_VERDICT,
-	BPF_SK_MSG_VERDICT,
-};
-
-int prog_type[] = {
-	BPF_PROG_TYPE_SK_SKB,
-	BPF_PROG_TYPE_SK_SKB,
-	BPF_PROG_TYPE_SK_SKB,
-	BPF_PROG_TYPE_SOCK_OPS,
-	BPF_PROG_TYPE_SK_MSG,
-	BPF_PROG_TYPE_SK_MSG,
-	BPF_PROG_TYPE_SK_MSG,
-	BPF_PROG_TYPE_SK_MSG,
-	BPF_PROG_TYPE_SK_MSG,
-};
-
 static int populate_progs(char *bpf_file)
 {
 	struct bpf_program *prog;
@@ -1825,13 +1801,6 @@ static int populate_progs(char *bpf_file)
 		return -1;
 	}
 
-	bpf_object__for_each_program(prog, obj) {
-		bpf_program__set_type(prog, prog_type[i]);
-		bpf_program__set_expected_attach_type(prog,
-						      prog_attach_type[i]);
-		i++;
-	}
-
 	i = bpf_object__load(obj);
 	i = 0;
 	bpf_object__for_each_program(prog, obj) {
-- 
2.40.1

Re: [PATCH bpf-next] selftests/bpf: test_sockmap, use section names understood by libbpf

[Geliang Tang]

Hi Jakub,

On Wed, 2024-05-22 at 10:09 +0200, Jakub Sitnicki wrote:
> libbpf can deduce program type and attach type from the ELF section
> name.
> We don't need to pass it out-of-band if we switch to libbpf
> convention [1].
> 
> [1] https://docs.kernel.org/bpf/libbpf/program_types.html
> 
> Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>

Thanks for this patch, it works well. But ...

> ---
>  .../selftests/bpf/progs/test_sockmap_kern.h   | 17 +++++-----
>  tools/testing/selftests/bpf/test_sockmap.c    | 31 -----------------
> --
>  2 files changed, 9 insertions(+), 39 deletions(-)
> 
> diff --git a/tools/testing/selftests/bpf/progs/test_sockmap_kern.h
> b/tools/testing/selftests/bpf/progs/test_sockmap_kern.h
> index 99d2ea9fb658..3dff0813730b 100644
> --- a/tools/testing/selftests/bpf/progs/test_sockmap_kern.h
> +++ b/tools/testing/selftests/bpf/progs/test_sockmap_kern.h
> @@ -92,7 +92,7 @@ struct {
>  	__uint(value_size, sizeof(int));
>  } tls_sock_map SEC(".maps");
>  
> -SEC("sk_skb1")
> +SEC("sk_skb/stream_parser")
>  int bpf_prog1(struct __sk_buff *skb)
>  {
>  	int *f, two = 2;
> @@ -104,7 +104,7 @@ int bpf_prog1(struct __sk_buff *skb)
>  	return skb->len;
>  }
>  
> -SEC("sk_skb2")
> +SEC("sk_skb/stream_verdict")
>  int bpf_prog2(struct __sk_buff *skb)
>  {
>  	__u32 lport = skb->local_port;
> @@ -151,7 +151,7 @@ static inline void bpf_write_pass(struct
> __sk_buff *skb, int offset)
>  		memcpy(c + offset, "PASS", 4);
>  }
>  
> -SEC("sk_skb3")
> +SEC("sk_skb/stream_verdict")
>  int bpf_prog3(struct __sk_buff *skb)
>  {
>  	int err, *f, ret = SK_PASS;
> @@ -233,7 +233,7 @@ int bpf_sockmap(struct bpf_sock_ops *skops)
>  	return 0;
>  }
>  
> -SEC("sk_msg1")
> +SEC("sk_msg")
>  int bpf_prog4(struct sk_msg_md *msg)
>  {
>  	int *bytes, zero = 0, one = 1, two = 2, three = 3, four = 4,
> five = 5;
> @@ -263,7 +263,7 @@ int bpf_prog4(struct sk_msg_md *msg)
>  	return SK_PASS;
>  }
>  
> -SEC("sk_msg2")
> +SEC("sk_msg")
>  int bpf_prog6(struct sk_msg_md *msg)
>  {
>  	int zero = 0, one = 1, two = 2, three = 3, four = 4, five =
> 5, key = 0;
> @@ -308,7 +308,7 @@ int bpf_prog6(struct sk_msg_md *msg)
>  #endif
>  }
>  
> -SEC("sk_msg3")
> +SEC("sk_msg")
>  int bpf_prog8(struct sk_msg_md *msg)
>  {
>  	void *data_end = (void *)(long) msg->data_end;
> @@ -329,7 +329,8 @@ int bpf_prog8(struct sk_msg_md *msg)
>  
>  	return SK_PASS;
>  }
> -SEC("sk_msg4")
> +
> +SEC("sk_msg")
>  int bpf_prog9(struct sk_msg_md *msg)
>  {
>  	void *data_end = (void *)(long) msg->data_end;
> @@ -347,7 +348,7 @@ int bpf_prog9(struct sk_msg_md *msg)
>  	return SK_PASS;
>  }
>  
> -SEC("sk_msg5")
> +SEC("sk_msg")
>  int bpf_prog10(struct sk_msg_md *msg)
>  {
>  	int *bytes, *start, *end, *start_push, *end_push,
> *start_pop, *pop;
> diff --git a/tools/testing/selftests/bpf/test_sockmap.c
> b/tools/testing/selftests/bpf/test_sockmap.c
> index 4499b3cfc3a6..ddc6a9cef36f 100644
> --- a/tools/testing/selftests/bpf/test_sockmap.c
> +++ b/tools/testing/selftests/bpf/test_sockmap.c
> @@ -1783,30 +1783,6 @@ char *map_names[] = {
>  	"tls_sock_map",
>  };
>  
> -int prog_attach_type[] = {
> -	BPF_SK_SKB_STREAM_PARSER,
> -	BPF_SK_SKB_STREAM_VERDICT,
> -	BPF_SK_SKB_STREAM_VERDICT,
> -	BPF_CGROUP_SOCK_OPS,
> -	BPF_SK_MSG_VERDICT,
> -	BPF_SK_MSG_VERDICT,
> -	BPF_SK_MSG_VERDICT,
> -	BPF_SK_MSG_VERDICT,
> -	BPF_SK_MSG_VERDICT,
> -};

prog_attach_type is still used by my commit:

https://lore.kernel.org/bpf/e27d7d0c1e0e79b0acd22ac6ad5d8f9f00225303.1716372485.git.tanggeliang@kylinos.cn/T/#u

Please review it for me.

If my commit is acceptable, this patch will conflict with it. It's a
bit strange to delete this prog_attach_type in your patch and then add
it back in my commit. So could you please rebase this patch on my
commit in that case. Sorry for the trouble.

Anyway please add my tag:

Acked-and-tested-by: Geliang Tang <geliang@kernel.org> 

Thanks,
-Geliang

> -
> -int prog_type[] = {
> -	BPF_PROG_TYPE_SK_SKB,
> -	BPF_PROG_TYPE_SK_SKB,
> -	BPF_PROG_TYPE_SK_SKB,
> -	BPF_PROG_TYPE_SOCK_OPS,
> -	BPF_PROG_TYPE_SK_MSG,
> -	BPF_PROG_TYPE_SK_MSG,
> -	BPF_PROG_TYPE_SK_MSG,
> -	BPF_PROG_TYPE_SK_MSG,
> -	BPF_PROG_TYPE_SK_MSG,
> -};
> -
>  static int populate_progs(char *bpf_file)
>  {
>  	struct bpf_program *prog;
> @@ -1825,13 +1801,6 @@ static int populate_progs(char *bpf_file)
>  		return -1;
>  	}
>  
> -	bpf_object__for_each_program(prog, obj) {
> -		bpf_program__set_type(prog, prog_type[i]);
> -		bpf_program__set_expected_attach_type(prog,
> -						     
> prog_attach_type[i]);
> -		i++;
> -	}
> -
>  	i = bpf_object__load(obj);
>  	i = 0;
>  	bpf_object__for_each_program(prog, obj) {

Re: [PATCH bpf-next] selftests/bpf: test_sockmap, use section names understood by libbpf

[Jakub Sitnicki]

On Wed, May 22, 2024 at 06:12 PM +08, Geliang Tang wrote:
> On Wed, 2024-05-22 at 10:09 +0200, Jakub Sitnicki wrote:

[...]

> prog_attach_type is still used by my commit:
>
> https://lore.kernel.org/bpf/e27d7d0c1e0e79b0acd22ac6ad5d8f9f00225303.1716372485.git.tanggeliang@kylinos.cn/T/#u
>
> Please review it for me.
>
> If my commit is acceptable, this patch will conflict with it. It's a
> bit strange to delete this prog_attach_type in your patch and then add
> it back in my commit. So could you please rebase this patch on my
> commit in that case. Sorry for the trouble.

If you want to help improve and modernize this test code, I suggest
switching attachments to bpf_link instead, they are now available for
sockmap:

https://lore.kernel.org/bpf/20240408152451.4162024-1-yonghong.song@linux.dev/

Re: [PATCH bpf-next] selftests/bpf: test_sockmap, use section names understood by libbpf

[Geliang Tang]

Hi Jakub,

On Wed, 2024-05-22 at 12:26 +0200, Jakub Sitnicki wrote:
> On Wed, May 22, 2024 at 06:12 PM +08, Geliang Tang wrote:
> > On Wed, 2024-05-22 at 10:09 +0200, Jakub Sitnicki wrote:
> 
> [...]
> 
> > prog_attach_type is still used by my commit:
> > 
> > https://lore.kernel.org/bpf/e27d7d0c1e0e79b0acd22ac6ad5d8f9f00225303.1716372485.git.tanggeliang@kylinos.cn/T/#u

I changes my commit above as "Rejected". So this patch is ready to
apply now.

> > Please review it for me.
> > 
> > If my commit is acceptable, this patch will conflict with it. It's
> > a
> > bit strange to delete this prog_attach_type in your patch and then
> > add
> > it back in my commit. So could you please rebase this patch on my
> > commit in that case. Sorry for the trouble.
> 
> If you want to help improve and modernize this test code, I suggest
> switching attachments to bpf_link instead, they are now available for
> sockmap:

Here's the patches for switching attachments to bpf_link, together with
some small fixes and cleanups:

https://patchwork.kernel.org/project/netdevbpf/cover/cover.1716446893.git.tanggeliang@kylinos.cn/

Please review them for me.

Thanks,
-Geliang

> 
> https://lore.kernel.org/bpf/20240408152451.4162024-1-yonghong.song@linux.dev/
> 

Re: [PATCH bpf-next] selftests/bpf: test_sockmap, use section names understood by libbpf

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to bpf/bpf-next.git (master)
by Andrii Nakryiko <andrii@kernel.org>:

On Wed, 22 May 2024 10:09:36 +0200 you wrote:
> libbpf can deduce program type and attach type from the ELF section name.
> We don't need to pass it out-of-band if we switch to libbpf convention [1].
> 
> [1] https://docs.kernel.org/bpf/libbpf/program_types.html
> 
> Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
> 
> [...]

Here is the summary with links:
  - [bpf-next] selftests/bpf: test_sockmap, use section names understood by libbpf
    https://git.kernel.org/bpf/bpf-next/c/46253c4ae961

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH bpf-next] selftests/bpf: test_sockmap, use section names understood by libbpf

[Tony Ambardar]

Hi Jakub, Daniel,

On Wed, May 22, 2024 at 10:09:36AM +0200, Jakub Sitnicki wrote:
> libbpf can deduce program type and attach type from the ELF section name.
> We don't need to pass it out-of-band if we switch to libbpf convention [1].
> 
> [1] https://docs.kernel.org/bpf/libbpf/program_types.html
> 
> Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
> ---
>  .../selftests/bpf/progs/test_sockmap_kern.h   | 17 +++++-----
>  tools/testing/selftests/bpf/test_sockmap.c    | 31 -------------------
>  2 files changed, 9 insertions(+), 39 deletions(-)
> 
[ snip ]

I recently hit some CI errors on kernel-patches/bpf related to veristat
and the test_sockmap BPF object files:

https://github.com/kernel-patches/bpf/actions/runs/9775040227/job/26985293772?pr=7279

I hadn't seen your submission, and after some investigation my own fix
ended up reproducing your patch: <sigh...>

https://github.com/kernel-patches/bpf/pull/7279/commits/9e5bc23d4a3643e6a4733aac431ee425310e03d6

Given it fixes a CI issue for bpf/master, could we backport/apply your
patch there? 

Thanks,
Tony