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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ec9b6e588ab9c25e0c4f9d1d8822d91896e87b35.camel@kernel.org>
Date: Wed, 22 May 2024 18:12:31 +0800
From: Geliang Tang <geliang@...nel.org>
To: Jakub Sitnicki <jakub@...udflare.com>, bpf@...r.kernel.org
Cc: netdev@...r.kernel.org, Alexei Starovoitov <ast@...nel.org>, Daniel
 Borkmann <daniel@...earbox.net>, Andrii Nakryiko <andrii@...nel.org>, John
 Fastabend <john.fastabend@...il.com>
Subject: Re: [PATCH bpf-next] selftests/bpf: test_sockmap, use section names
 understood by libbpf

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@...udflare.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@...nel.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) {