[PATCH bpf-next 0/8] fixes for test_sockmap

[PATCH bpf-next 0/8] fixes for test_sockmap

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

This patchset contains some fixes and improvements for test_sockmap.

3-5: switching attachments to bpf_link as Jakub suggested in [1].
1-2, 6-8: Small fixes.

[1]
https://lore.kernel.org/bpf/87zfsiw3a3.fsf@cloudflare.com/

Geliang Tang (8):
  selftests/bpf: Fix tx_prog_fd values in test_sockmap
  selftests/bpf: Drop duplicate definition of i in test_sockmap
  selftests/bpf: Use bpf_link attachments in test_sockmap
  selftests/bpf: Replace tx_prog_fd with tx_prog in test_sockmap
  selftests/bpf: Drop prog_fd array in test_sockmap
  selftests/bpf: Fix size of map_fd in test_sockmap
  selftests/bpf: Check length of recv in test_sockmap
  selftests/bpf: Drop duplicate bpf_map_lookup_elem in test_sockmap

 .../selftests/bpf/progs/test_sockmap_kern.h   |   3 -
 tools/testing/selftests/bpf/test_sockmap.c    | 101 +++++++++---------
 2 files changed, 51 insertions(+), 53 deletions(-)

-- 
2.43.0

[PATCH bpf-next 1/8] selftests/bpf: Fix tx_prog_fd values in test_sockmap

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

The values of tx_prog_fd in run_options() should not be 0, so set it as -1
in else branch, and test it using "if (tx_prog_fd > 0)" condition, not
"if (tx_prog_fd)" or "if (tx_prog_fd >= 0)".

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 tools/testing/selftests/bpf/test_sockmap.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/bpf/test_sockmap.c b/tools/testing/selftests/bpf/test_sockmap.c
index 4499b3cfc3a6..fde0dd741e69 100644
--- a/tools/testing/selftests/bpf/test_sockmap.c
+++ b/tools/testing/selftests/bpf/test_sockmap.c
@@ -1027,9 +1027,9 @@ static int run_options(struct sockmap_options *options, int cg_fd,  int test)
 	else if (txmsg_drop)
 		tx_prog_fd = prog_fd[8];
 	else
-		tx_prog_fd = 0;
+		tx_prog_fd = -1;
 
-	if (tx_prog_fd) {
+	if (tx_prog_fd > 0) {
 		int redir_fd, i = 0;
 
 		err = bpf_prog_attach(tx_prog_fd,
@@ -1285,7 +1285,7 @@ static int run_options(struct sockmap_options *options, int cg_fd,  int test)
 	bpf_prog_detach2(prog_fd[0], map_fd[8], BPF_SK_SKB_STREAM_PARSER);
 	bpf_prog_detach2(prog_fd[2], map_fd[8], BPF_SK_SKB_STREAM_VERDICT);
 
-	if (tx_prog_fd >= 0)
+	if (tx_prog_fd > 0)
 		bpf_prog_detach2(tx_prog_fd, map_fd[1], BPF_SK_MSG_VERDICT);
 
 	for (i = 0; i < 8; i++) {
-- 
2.43.0

RE: [PATCH bpf-next 1/8] selftests/bpf: Fix tx_prog_fd values in test_sockmap

[John Fastabend]

Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> The values of tx_prog_fd in run_options() should not be 0, so set it as -1
> in else branch, and test it using "if (tx_prog_fd > 0)" condition, not
> "if (tx_prog_fd)" or "if (tx_prog_fd >= 0)".
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---

Acked-by: John Fastabend <john.fastabend@gmail.com>

[PATCH bpf-next 2/8] selftests/bpf: Drop duplicate definition of i in test_sockmap

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

There's already a definition of i in run_options() at the beginning, no
need to define a new one in "if (tx_prog_fd > 0)" block.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 tools/testing/selftests/bpf/test_sockmap.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/test_sockmap.c b/tools/testing/selftests/bpf/test_sockmap.c
index fde0dd741e69..e7dbf49a2ca6 100644
--- a/tools/testing/selftests/bpf/test_sockmap.c
+++ b/tools/testing/selftests/bpf/test_sockmap.c
@@ -1030,7 +1030,7 @@ static int run_options(struct sockmap_options *options, int cg_fd,  int test)
 		tx_prog_fd = -1;
 
 	if (tx_prog_fd > 0) {
-		int redir_fd, i = 0;
+		int redir_fd;
 
 		err = bpf_prog_attach(tx_prog_fd,
 				      map_fd[1], BPF_SK_MSG_VERDICT, 0);
@@ -1041,6 +1041,7 @@ static int run_options(struct sockmap_options *options, int cg_fd,  int test)
 			goto out;
 		}
 
+		i = 0;
 		err = bpf_map_update_elem(map_fd[1], &i, &c1, BPF_ANY);
 		if (err) {
 			fprintf(stderr,
-- 
2.43.0

RE: [PATCH bpf-next 2/8] selftests/bpf: Drop duplicate definition of i in test_sockmap

[John Fastabend]

Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> There's already a definition of i in run_options() at the beginning, no
> need to define a new one in "if (tx_prog_fd > 0)" block.
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---

Acked-by: John Fastabend <john.fastabend@gmail.com>

[PATCH bpf-next 3/8] selftests/bpf: Use bpf_link attachments in test_sockmap

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

Switch attachments to bpf_link using bpf_program__attach_sockmap() instead
of bpf_prog_attach().

This patch adds a new array progs[] to replace prog_fd[] array, set in
populate_progs() for each program in bpf object.

And another new array links[] to save the attached bpf_link. It is
initalized as NULL in populate_progs, set as the return valuses of
bpf_program__attach_sockmap(), and detached by bpf_link__detach().

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 tools/testing/selftests/bpf/test_sockmap.c | 59 ++++++++++++----------
 1 file changed, 31 insertions(+), 28 deletions(-)

diff --git a/tools/testing/selftests/bpf/test_sockmap.c b/tools/testing/selftests/bpf/test_sockmap.c
index e7dbf49a2ca6..d7581bbbc473 100644
--- a/tools/testing/selftests/bpf/test_sockmap.c
+++ b/tools/testing/selftests/bpf/test_sockmap.c
@@ -64,6 +64,8 @@ int failed;
 int map_fd[9];
 struct bpf_map *maps[9];
 int prog_fd[9];
+struct bpf_program *progs[9];
+struct bpf_link *links[9];
 
 int txmsg_pass;
 int txmsg_redir;
@@ -960,43 +962,39 @@ static int run_options(struct sockmap_options *options, int cg_fd,  int test)
 
 	/* Attach programs to sockmap */
 	if (!txmsg_omit_skb_parser) {
-		err = bpf_prog_attach(prog_fd[0], map_fd[0],
-				      BPF_SK_SKB_STREAM_PARSER, 0);
-		if (err) {
+		links[0] = bpf_program__attach_sockmap(progs[0], map_fd[0]);
+		if (!links[0]) {
 			fprintf(stderr,
-				"ERROR: bpf_prog_attach (sockmap %i->%i): %d (%s)\n",
-				prog_fd[0], map_fd[0], err, strerror(errno));
-			return err;
+				"ERROR: bpf_program__attach_sockmap (sockmap %i->%i): (%s)\n",
+				bpf_program__fd(progs[0]), map_fd[0], strerror(errno));
+			return -1;
 		}
 	}
 
-	err = bpf_prog_attach(prog_fd[1], map_fd[0],
-				BPF_SK_SKB_STREAM_VERDICT, 0);
-	if (err) {
-		fprintf(stderr, "ERROR: bpf_prog_attach (sockmap): %d (%s)\n",
-			err, strerror(errno));
-		return err;
+	links[1] = bpf_program__attach_sockmap(progs[1], map_fd[0]);
+	if (!links[1]) {
+		fprintf(stderr, "ERROR: bpf_program__attach_sockmap (sockmap): (%s)\n",
+			strerror(errno));
+		return -1;
 	}
 
 	/* Attach programs to TLS sockmap */
 	if (txmsg_ktls_skb) {
 		if (!txmsg_omit_skb_parser) {
-			err = bpf_prog_attach(prog_fd[0], map_fd[8],
-					      BPF_SK_SKB_STREAM_PARSER, 0);
-			if (err) {
+			links[2] = bpf_program__attach_sockmap(progs[0], map_fd[8]);
+			if (!links[2]) {
 				fprintf(stderr,
-					"ERROR: bpf_prog_attach (TLS sockmap %i->%i): %d (%s)\n",
-					prog_fd[0], map_fd[8], err, strerror(errno));
-				return err;
+					"ERROR: bpf_program__attach_sockmap (TLS sockmap %i->%i): (%s)\n",
+					bpf_program__fd(progs[0]), map_fd[8], strerror(errno));
+				return -1;
 			}
 		}
 
-		err = bpf_prog_attach(prog_fd[2], map_fd[8],
-				      BPF_SK_SKB_STREAM_VERDICT, 0);
-		if (err) {
-			fprintf(stderr, "ERROR: bpf_prog_attach (TLS sockmap): %d (%s)\n",
-				err, strerror(errno));
-			return err;
+		links[3] = bpf_program__attach_sockmap(progs[2], map_fd[8]);
+		if (!links[3]) {
+			fprintf(stderr, "ERROR: bpf_program__attach_sockmap (TLS sockmap): (%s)\n",
+				strerror(errno));
+			return -1;
 		}
 	}
 
@@ -1281,10 +1279,11 @@ static int run_options(struct sockmap_options *options, int cg_fd,  int test)
 out:
 	/* Detatch and zero all the maps */
 	bpf_prog_detach2(prog_fd[3], cg_fd, BPF_CGROUP_SOCK_OPS);
-	bpf_prog_detach2(prog_fd[0], map_fd[0], BPF_SK_SKB_STREAM_PARSER);
-	bpf_prog_detach2(prog_fd[1], map_fd[0], BPF_SK_SKB_STREAM_VERDICT);
-	bpf_prog_detach2(prog_fd[0], map_fd[8], BPF_SK_SKB_STREAM_PARSER);
-	bpf_prog_detach2(prog_fd[2], map_fd[8], BPF_SK_SKB_STREAM_VERDICT);
+
+	for (i = 0; i < ARRAY_SIZE(links); i++) {
+		if (links[i])
+			bpf_link__detach(links[i]);
+	}
 
 	if (tx_prog_fd > 0)
 		bpf_prog_detach2(tx_prog_fd, map_fd[1], BPF_SK_MSG_VERDICT);
@@ -1836,6 +1835,7 @@ static int populate_progs(char *bpf_file)
 	i = bpf_object__load(obj);
 	i = 0;
 	bpf_object__for_each_program(prog, obj) {
+		progs[i] = prog;
 		prog_fd[i] = bpf_program__fd(prog);
 		i++;
 	}
@@ -1850,6 +1850,9 @@ static int populate_progs(char *bpf_file)
 		}
 	}
 
+	for (i = 0; i < ARRAY_SIZE(links); i++)
+		links[i] = NULL;
+
 	return 0;
 }
 
-- 
2.43.0

RE: [PATCH bpf-next 3/8] selftests/bpf: Use bpf_link attachments in test_sockmap

[John Fastabend]

Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> Switch attachments to bpf_link using bpf_program__attach_sockmap() instead
> of bpf_prog_attach().

Sorry it took me a few days to get to this.

Is there a reason to push this to links vs just leave it as is? I had
a plan to port all the test_sockmap tests into prog_tests anyways. I'll
try to push some initial patch next week.

The one advantage of test_sockmap is we can have it run for longer
runs by pushing different options through so might be worth keeping
just for that.

If you really want links here I'm OK with that I guess just asking.

Thanks,
John

> 
> This patch adds a new array progs[] to replace prog_fd[] array, set in
> populate_progs() for each program in bpf object.
> 
> And another new array links[] to save the attached bpf_link. It is
> initalized as NULL in populate_progs, set as the return valuses of
> bpf_program__attach_sockmap(), and detached by bpf_link__detach().
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  tools/testing/selftests/bpf/test_sockmap.c | 59 ++++++++++++----------
>  1 file changed, 31 insertions(+), 28 deletions(-)
> 
> diff --git a/tools/testing/selftests/bpf/test_sockmap.c b/tools/testing/selftests/bpf/test_sockmap.c
> index e7dbf49a2ca6..d7581bbbc473 100644
> --- a/tools/testing/selftests/bpf/test_sockmap.c
> +++ b/tools/testing/selftests/bpf/test_sockmap.c
> @@ -64,6 +64,8 @@ int failed;
>  int map_fd[9];
>  struct bpf_map *maps[9];
>  int prog_fd[9];
> +struct bpf_program *progs[9];
> +struct bpf_link *links[9];
>  
>  int txmsg_pass;
>  int txmsg_redir;
> @@ -960,43 +962,39 @@ static int run_options(struct sockmap_options *options, int cg_fd,  int test)
>  
>  	/* Attach programs to sockmap */
>  	if (!txmsg_omit_skb_parser) {
> -		err = bpf_prog_attach(prog_fd[0], map_fd[0],
> -				      BPF_SK_SKB_STREAM_PARSER, 0);
> -		if (err) {
> +		links[0] = bpf_program__attach_sockmap(progs[0], map_fd[0]);
> +		if (!links[0]) {
>  			fprintf(stderr,
> -				"ERROR: bpf_prog_attach (sockmap %i->%i): %d (%s)\n",
> -				prog_fd[0], map_fd[0], err, strerror(errno));
> -			return err;
> +				"ERROR: bpf_program__attach_sockmap (sockmap %i->%i): (%s)\n",
> +				bpf_program__fd(progs[0]), map_fd[0], strerror(errno));
> +			return -1;
>  		}
>  	}
>  
> -	err = bpf_prog_attach(prog_fd[1], map_fd[0],
> -				BPF_SK_SKB_STREAM_VERDICT, 0);
> -	if (err) {
> -		fprintf(stderr, "ERROR: bpf_prog_attach (sockmap): %d (%s)\n",
> -			err, strerror(errno));
> -		return err;
> +	links[1] = bpf_program__attach_sockmap(progs[1], map_fd[0]);
> +	if (!links[1]) {
> +		fprintf(stderr, "ERROR: bpf_program__attach_sockmap (sockmap): (%s)\n",
> +			strerror(errno));
> +		return -1;
>  	}
>  
>  	/* Attach programs to TLS sockmap */
>  	if (txmsg_ktls_skb) {
>  		if (!txmsg_omit_skb_parser) {
> -			err = bpf_prog_attach(prog_fd[0], map_fd[8],
> -					      BPF_SK_SKB_STREAM_PARSER, 0);
> -			if (err) {
> +			links[2] = bpf_program__attach_sockmap(progs[0], map_fd[8]);
> +			if (!links[2]) {
>  				fprintf(stderr,
> -					"ERROR: bpf_prog_attach (TLS sockmap %i->%i): %d (%s)\n",
> -					prog_fd[0], map_fd[8], err, strerror(errno));
> -				return err;
> +					"ERROR: bpf_program__attach_sockmap (TLS sockmap %i->%i): (%s)\n",
> +					bpf_program__fd(progs[0]), map_fd[8], strerror(errno));
> +				return -1;
>  			}
>  		}
>  
> -		err = bpf_prog_attach(prog_fd[2], map_fd[8],
> -				      BPF_SK_SKB_STREAM_VERDICT, 0);
> -		if (err) {
> -			fprintf(stderr, "ERROR: bpf_prog_attach (TLS sockmap): %d (%s)\n",
> -				err, strerror(errno));
> -			return err;
> +		links[3] = bpf_program__attach_sockmap(progs[2], map_fd[8]);
> +		if (!links[3]) {
> +			fprintf(stderr, "ERROR: bpf_program__attach_sockmap (TLS sockmap): (%s)\n",
> +				strerror(errno));
> +			return -1;
>  		}
>  	}
>  
> @@ -1281,10 +1279,11 @@ static int run_options(struct sockmap_options *options, int cg_fd,  int test)
>  out:
>  	/* Detatch and zero all the maps */
>  	bpf_prog_detach2(prog_fd[3], cg_fd, BPF_CGROUP_SOCK_OPS);
> -	bpf_prog_detach2(prog_fd[0], map_fd[0], BPF_SK_SKB_STREAM_PARSER);
> -	bpf_prog_detach2(prog_fd[1], map_fd[0], BPF_SK_SKB_STREAM_VERDICT);
> -	bpf_prog_detach2(prog_fd[0], map_fd[8], BPF_SK_SKB_STREAM_PARSER);
> -	bpf_prog_detach2(prog_fd[2], map_fd[8], BPF_SK_SKB_STREAM_VERDICT);
> +
> +	for (i = 0; i < ARRAY_SIZE(links); i++) {
> +		if (links[i])
> +			bpf_link__detach(links[i]);
> +	}
>  
>  	if (tx_prog_fd > 0)
>  		bpf_prog_detach2(tx_prog_fd, map_fd[1], BPF_SK_MSG_VERDICT);
> @@ -1836,6 +1835,7 @@ static int populate_progs(char *bpf_file)
>  	i = bpf_object__load(obj);
>  	i = 0;
>  	bpf_object__for_each_program(prog, obj) {
> +		progs[i] = prog;
>  		prog_fd[i] = bpf_program__fd(prog);
>  		i++;
>  	}
> @@ -1850,6 +1850,9 @@ static int populate_progs(char *bpf_file)
>  		}
>  	}
>  
> +	for (i = 0; i < ARRAY_SIZE(links); i++)
> +		links[i] = NULL;
> +
>  	return 0;
>  }
>  
> -- 
> 2.43.0
> 

Re: [PATCH bpf-next 3/8] selftests/bpf: Use bpf_link attachments in test_sockmap

[Jakub Sitnicki]

On Mon, May 27, 2024 at 10:12 AM -07, John Fastabend wrote:
> Geliang Tang wrote:
>> From: Geliang Tang <tanggeliang@kylinos.cn>
>> 
>> Switch attachments to bpf_link using bpf_program__attach_sockmap() instead
>> of bpf_prog_attach().
>
> Sorry it took me a few days to get to this.
>
> Is there a reason to push this to links vs just leave it as is? I had
> a plan to port all the test_sockmap tests into prog_tests anyways. I'll
> try to push some initial patch next week.
>
> The one advantage of test_sockmap is we can have it run for longer
> runs by pushing different options through so might be worth keeping
> just for that.
>
> If you really want links here I'm OK with that I guess just asking.

It was me who suggested the switch to bpf_link in reaction to a series
of cleanups to prog_type and prog_attach_type submitted by Geliang.

Relevant threads:

https://lore.kernel.org/bpf/9c10d9f974f07fcb354a43a8eca67acb2fafc587.1715926605.git.tanggeliang@kylinos.cn
https://lore.kernel.org/bpf/20240522080936.2475833-1-jakub@cloudflare.com
https://lore.kernel.org/bpf/e27d7d0c1e0e79b0acd22ac6ad5d8f9f00225303.1716372485.git.tanggeliang@kylinos.cn

I thought bpf_links added more value than cleaning up "old style"
attachments.

Re: [PATCH bpf-next 3/8] selftests/bpf: Use bpf_link attachments in test_sockmap

[Geliang Tang]

On Mon, 2024-05-27 at 21:36 +0200, Jakub Sitnicki wrote:
> On Mon, May 27, 2024 at 10:12 AM -07, John Fastabend wrote:
> > Geliang Tang wrote:
> > > From: Geliang Tang <tanggeliang@kylinos.cn>
> > > 
> > > Switch attachments to bpf_link using
> > > bpf_program__attach_sockmap() instead
> > > of bpf_prog_attach().
> > 
> > Sorry it took me a few days to get to this.
> > 
> > Is there a reason to push this to links vs just leave it as is? I
> > had
> > a plan to port all the test_sockmap tests into prog_tests anyways.
> > I'll
> > try to push some initial patch next week.

Great, I strongly agree with porting them into prog_tests. I am also
willing to participate in implementing this plan together.

> > 
> > The one advantage of test_sockmap is we can have it run for longer
> > runs by pushing different options through so might be worth keeping
> > just for that.
> > 
> > If you really want links here I'm OK with that I guess just asking.
> 
> It was me who suggested the switch to bpf_link in reaction to a
> series
> of cleanups to prog_type and prog_attach_type submitted by Geliang.

Yes, patches 3-5 address Jakub's suggestion: switching attachments to
bpf_link.

> Relevant threads:
> 
> https://lore.kernel.org/bpf/9c10d9f974f07fcb354a43a8eca67acb2fafc587.1715926605.git.tanggeliang@kylinos.cn
> https://lore.kernel.org/bpf/20240522080936.2475833-1-jakub@cloudflare.com
> https://lore.kernel.org/bpf/e27d7d0c1e0e79b0acd22ac6ad5d8f9f00225303.1716372485.git.tanggeliang@kylinos.cn
> 
> I thought bpf_links added more value than cleaning up "old style"
> attachments.

Other patches 1-2, 6-8 are small fixes which I found while trying to
solve the NONBLOCK issue [1]. Yes, I haven't given up on solving this
issue yet. I think it must be solved, since there is a bug somewhere.
WDYT?

[1]
https://patchwork.kernel.org/project/netdevbpf/patch/b67101632c6858d281f105b5d4e1bc62dd6b7d27.1712133039.git.tanggeliang@kylinos.cn/

Thanks,
-Geliang

Re: [PATCH bpf-next 3/8] selftests/bpf: Use bpf_link attachments in test_sockmap

[John Fastabend]

Geliang Tang wrote:
> On Mon, 2024-05-27 at 21:36 +0200, Jakub Sitnicki wrote:
> > On Mon, May 27, 2024 at 10:12 AM -07, John Fastabend wrote:
> > > Geliang Tang wrote:
> > > > From: Geliang Tang <tanggeliang@kylinos.cn>
> > > > 
> > > > Switch attachments to bpf_link using
> > > > bpf_program__attach_sockmap() instead
> > > > of bpf_prog_attach().
> > > 
> > > Sorry it took me a few days to get to this.
> > > 
> > > Is there a reason to push this to links vs just leave it as is? I
> > > had
> > > a plan to port all the test_sockmap tests into prog_tests anyways.
> > > I'll
> > > try to push some initial patch next week.
> 
> Great, I strongly agree with porting them into prog_tests. I am also
> willing to participate in implementing this plan together.

I have a first patch that starts to move things I'll dig it up here.
Still a bit behind on everything as you see its Thr already.

> 
> > > 
> > > The one advantage of test_sockmap is we can have it run for longer
> > > runs by pushing different options through so might be worth keeping
> > > just for that.
> > > 
> > > If you really want links here I'm OK with that I guess just asking.
> > 
> > It was me who suggested the switch to bpf_link in reaction to a
> > series
> > of cleanups to prog_type and prog_attach_type submitted by Geliang.
> 
> Yes, patches 3-5 address Jakub's suggestion: switching attachments to
> bpf_link.

OK. Lets just take them the series lgtm. Jakub any other comments?

> 
> > Relevant threads:
> > 
> > https://lore.kernel.org/bpf/9c10d9f974f07fcb354a43a8eca67acb2fafc587.1715926605.git.tanggeliang@kylinos.cn
> > https://lore.kernel.org/bpf/20240522080936.2475833-1-jakub@cloudflare.com
> > https://lore.kernel.org/bpf/e27d7d0c1e0e79b0acd22ac6ad5d8f9f00225303.1716372485.git.tanggeliang@kylinos.cn
> > 
> > I thought bpf_links added more value than cleaning up "old style"
> > attachments.
> 
> Other patches 1-2, 6-8 are small fixes which I found while trying to
> solve the NONBLOCK issue [1]. Yes, I haven't given up on solving this
> issue yet. I think it must be solved, since there is a bug somewhere.
> WDYT?

Yes I think this is an actual issue with the stream parser waking up
sockets before the data is copied into the recv buffers.

Re: [PATCH bpf-next 3/8] selftests/bpf: Use bpf_link attachments in test_sockmap

[Jakub Sitnicki]

On Thu, May 30, 2024 at 04:45 PM -07, John Fastabend wrote:
> Geliang Tang wrote:
>> On Mon, 2024-05-27 at 21:36 +0200, Jakub Sitnicki wrote:
>> > On Mon, May 27, 2024 at 10:12 AM -07, John Fastabend wrote:
>> > > Geliang Tang wrote:

[...]

>> > > The one advantage of test_sockmap is we can have it run for longer
>> > > runs by pushing different options through so might be worth keeping
>> > > just for that.
>> > > 
>> > > If you really want links here I'm OK with that I guess just asking.
>> > 
>> > It was me who suggested the switch to bpf_link in reaction to a
>> > series
>> > of cleanups to prog_type and prog_attach_type submitted by Geliang.
>> 
>> Yes, patches 3-5 address Jakub's suggestion: switching attachments to
>> bpf_link.
>
> OK. Lets just take them the series lgtm. Jakub any other comments?

Gave it a run - all looks well. Thanks for the patches.

Geliang, is there some MPTCP+sockmap use-case you're working towards?

Re: [PATCH bpf-next 3/8] selftests/bpf: Use bpf_link attachments in test_sockmap

[Geliang Tang]

On Fri, May 31, 2024 at 01:13:39PM +0200, Jakub Sitnicki wrote:
> On Thu, May 30, 2024 at 04:45 PM -07, John Fastabend wrote:
> > Geliang Tang wrote:
> >> On Mon, 2024-05-27 at 21:36 +0200, Jakub Sitnicki wrote:
> >> > On Mon, May 27, 2024 at 10:12 AM -07, John Fastabend wrote:
> >> > > Geliang Tang wrote:
> 
> [...]
> 
> >> > > The one advantage of test_sockmap is we can have it run for longer
> >> > > runs by pushing different options through so might be worth keeping
> >> > > just for that.
> >> > > 
> >> > > If you really want links here I'm OK with that I guess just asking.
> >> > 
> >> > It was me who suggested the switch to bpf_link in reaction to a
> >> > series
> >> > of cleanups to prog_type and prog_attach_type submitted by Geliang.
> >> 
> >> Yes, patches 3-5 address Jakub's suggestion: switching attachments to
> >> bpf_link.
> >
> > OK. Lets just take them the series lgtm. Jakub any other comments?
> 
> Gave it a run - all looks well. Thanks for the patches.
> 
> Geliang, is there some MPTCP+sockmap use-case you're working towards?

Yes, indeed. I have been working on a task related to MPTCP+sockmap
recently, at least related to this test_sockmap.c selftest. We recently
received an issue with MPTCP [1], that is TLS cannot be set on MPTCP
sockets. The reason is that both MPTCP and TLS are implemented on TCP ULP.
And each socket only supports one type of TCP ULP.

I simply modified this test_sockmap.c selftest to support MPTCP, so that
it can be used as the first version of test for MPTCP+TLS. So I spent some
time reading and debugging this test.

The development of MPTCP+TLS is still ongoing, and currently only setsockopt
part has been successfully supported. The idea is simple, use an array of
tcp_ulp_ops in a socket, instead of a single one:

struct inet_connection_sock {
      ... ...
      const struct tcp_ulp_ops  *icsk_ulp_ops[ULP_INDEX_MAX];
      void __rcu                *icsk_ulp_data[ULP_INDEX_MAX];
}

The entire patch is in my commit "mptcp: tls support" [2]. It's not finish
yet, but I really want to hear your opinions, especially John's.

[1]
https://github.com/multipath-tcp/mptcp_net-next/issues/480
[2]
https://github.com/geliangtang/mptcp_net-next/commit/bba00a6cde75bab5a2c1c196d49812b4ed6addb0

Thanks,
-Geliang

[PATCH bpf-next 4/8] selftests/bpf: Replace tx_prog_fd with tx_prog in test_sockmap

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

bpf_program__attach_sockmap() needs to take a parameter of type bpf_program
instead of an fd, so tx_prog_fd becomes useless. This patch uses a pointer
tx_prog to point to an item in progs[] array.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 tools/testing/selftests/bpf/test_sockmap.c | 30 ++++++++++------------
 1 file changed, 14 insertions(+), 16 deletions(-)

diff --git a/tools/testing/selftests/bpf/test_sockmap.c b/tools/testing/selftests/bpf/test_sockmap.c
index d7581bbbc473..8e32d157bac0 100644
--- a/tools/testing/selftests/bpf/test_sockmap.c
+++ b/tools/testing/selftests/bpf/test_sockmap.c
@@ -954,7 +954,8 @@ enum {
 
 static int run_options(struct sockmap_options *options, int cg_fd,  int test)
 {
-	int i, key, next_key, err, tx_prog_fd = -1, zero = 0;
+	int i, key, next_key, err, zero = 0;
+	struct bpf_program *tx_prog;
 
 	/* If base test skip BPF setup */
 	if (test == BASE || test == BASE_SENDPAGE)
@@ -1015,27 +1016,27 @@ static int run_options(struct sockmap_options *options, int cg_fd,  int test)
 
 	/* Attach txmsg program to sockmap */
 	if (txmsg_pass)
-		tx_prog_fd = prog_fd[4];
+		tx_prog = progs[4];
 	else if (txmsg_redir)
-		tx_prog_fd = prog_fd[5];
+		tx_prog = progs[5];
 	else if (txmsg_apply)
-		tx_prog_fd = prog_fd[6];
+		tx_prog = progs[6];
 	else if (txmsg_cork)
-		tx_prog_fd = prog_fd[7];
+		tx_prog = progs[7];
 	else if (txmsg_drop)
-		tx_prog_fd = prog_fd[8];
+		tx_prog = progs[8];
 	else
-		tx_prog_fd = -1;
+		tx_prog = NULL;
 
-	if (tx_prog_fd > 0) {
+	if (tx_prog) {
 		int redir_fd;
 
-		err = bpf_prog_attach(tx_prog_fd,
-				      map_fd[1], BPF_SK_MSG_VERDICT, 0);
-		if (err) {
+		links[4] = bpf_program__attach_sockmap(tx_prog, map_fd[1]);
+		if (!links[4]) {
 			fprintf(stderr,
-				"ERROR: bpf_prog_attach (txmsg): %d (%s)\n",
-				err, strerror(errno));
+				"ERROR: bpf_program__attach_sockmap (txmsg): (%s)\n",
+				strerror(errno));
+			err = -1;
 			goto out;
 		}
 
@@ -1285,9 +1286,6 @@ static int run_options(struct sockmap_options *options, int cg_fd,  int test)
 			bpf_link__detach(links[i]);
 	}
 
-	if (tx_prog_fd > 0)
-		bpf_prog_detach2(tx_prog_fd, map_fd[1], BPF_SK_MSG_VERDICT);
-
 	for (i = 0; i < 8; i++) {
 		key = next_key = 0;
 		bpf_map_update_elem(map_fd[i], &key, &zero, BPF_ANY);
-- 
2.43.0

[PATCH bpf-next 5/8] selftests/bpf: Drop prog_fd array in test_sockmap

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

The program fds can be got by using bpf_program__fd(progs[]), then
prog_fd becomes useless. This patch drops it.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 tools/testing/selftests/bpf/test_sockmap.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/bpf/test_sockmap.c b/tools/testing/selftests/bpf/test_sockmap.c
index 8e32d157bac0..e83ca0005721 100644
--- a/tools/testing/selftests/bpf/test_sockmap.c
+++ b/tools/testing/selftests/bpf/test_sockmap.c
@@ -63,7 +63,6 @@ int passed;
 int failed;
 int map_fd[9];
 struct bpf_map *maps[9];
-int prog_fd[9];
 struct bpf_program *progs[9];
 struct bpf_link *links[9];
 
@@ -1000,7 +999,7 @@ static int run_options(struct sockmap_options *options, int cg_fd,  int test)
 	}
 
 	/* Attach to cgroups */
-	err = bpf_prog_attach(prog_fd[3], cg_fd, BPF_CGROUP_SOCK_OPS, 0);
+	err = bpf_prog_attach(bpf_program__fd(progs[3]), cg_fd, BPF_CGROUP_SOCK_OPS, 0);
 	if (err) {
 		fprintf(stderr, "ERROR: bpf_prog_attach (groups): %d (%s)\n",
 			err, strerror(errno));
@@ -1279,7 +1278,7 @@ static int run_options(struct sockmap_options *options, int cg_fd,  int test)
 		fprintf(stderr, "unknown test\n");
 out:
 	/* Detatch and zero all the maps */
-	bpf_prog_detach2(prog_fd[3], cg_fd, BPF_CGROUP_SOCK_OPS);
+	bpf_prog_detach2(bpf_program__fd(progs[3]), cg_fd, BPF_CGROUP_SOCK_OPS);
 
 	for (i = 0; i < ARRAY_SIZE(links); i++) {
 		if (links[i])
@@ -1834,7 +1833,6 @@ static int populate_progs(char *bpf_file)
 	i = 0;
 	bpf_object__for_each_program(prog, obj) {
 		progs[i] = prog;
-		prog_fd[i] = bpf_program__fd(prog);
 		i++;
 	}
 
-- 
2.43.0

[PATCH bpf-next 6/8] selftests/bpf: Fix size of map_fd in test_sockmap

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

The array size of map_fd[] is 9, not 8. This patch changes it as a more
general form: ARRAY_SIZE(map_fd).

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 tools/testing/selftests/bpf/test_sockmap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/test_sockmap.c b/tools/testing/selftests/bpf/test_sockmap.c
index e83ca0005721..3654babfac59 100644
--- a/tools/testing/selftests/bpf/test_sockmap.c
+++ b/tools/testing/selftests/bpf/test_sockmap.c
@@ -1285,7 +1285,7 @@ static int run_options(struct sockmap_options *options, int cg_fd,  int test)
 			bpf_link__detach(links[i]);
 	}
 
-	for (i = 0; i < 8; i++) {
+	for (i = 0; i < ARRAY_SIZE(map_fd); i++) {
 		key = next_key = 0;
 		bpf_map_update_elem(map_fd[i], &key, &zero, BPF_ANY);
 		while (bpf_map_get_next_key(map_fd[i], &key, &next_key) == 0) {
-- 
2.43.0

RE: [PATCH bpf-next 6/8] selftests/bpf: Fix size of map_fd in test_sockmap

[John Fastabend]

Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> The array size of map_fd[] is 9, not 8. This patch changes it as a more
> general form: ARRAY_SIZE(map_fd).
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  tools/testing/selftests/bpf/test_sockmap.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/bpf/test_sockmap.c b/tools/testing/selftests/bpf/test_sockmap.c
> index e83ca0005721..3654babfac59 100644
> --- a/tools/testing/selftests/bpf/test_sockmap.c
> +++ b/tools/testing/selftests/bpf/test_sockmap.c
> @@ -1285,7 +1285,7 @@ static int run_options(struct sockmap_options *options, int cg_fd,  int test)
>  			bpf_link__detach(links[i]);
>  	}
>  
> -	for (i = 0; i < 8; i++) {
> +	for (i = 0; i < ARRAY_SIZE(map_fd); i++) {
>  		key = next_key = 0;
>  		bpf_map_update_elem(map_fd[i], &key, &zero, BPF_ANY);
>  		while (bpf_map_get_next_key(map_fd[i], &key, &next_key) == 0) {
> -- 
> 2.43.0
> 

Acked-by: John Fastabend <john.fastabend@gmail.com>

[PATCH bpf-next 7/8] selftests/bpf: Check length of recv in test_sockmap

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

The value of recv in msg_loop may be negative, like EWOULDBLOCK, so it's
necessary to check if it is positive before accumulating it to bytes_recvd.

Fixes: 16962b2404ac ("bpf: sockmap, add selftests")
Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 tools/testing/selftests/bpf/test_sockmap.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/test_sockmap.c b/tools/testing/selftests/bpf/test_sockmap.c
index 3654babfac59..4c7cb206b31d 100644
--- a/tools/testing/selftests/bpf/test_sockmap.c
+++ b/tools/testing/selftests/bpf/test_sockmap.c
@@ -681,7 +681,8 @@ static int msg_loop(int fd, int iov_count, int iov_length, int cnt,
 				}
 			}
 
-			s->bytes_recvd += recv;
+			if (recv > 0)
+				s->bytes_recvd += recv;
 
 			if (opt->check_recved_len && s->bytes_recvd > total_bytes) {
 				errno = EMSGSIZE;
-- 
2.43.0

RE: [PATCH bpf-next 7/8] selftests/bpf: Check length of recv in test_sockmap

[John Fastabend]

Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> The value of recv in msg_loop may be negative, like EWOULDBLOCK, so it's
> necessary to check if it is positive before accumulating it to bytes_recvd.
> 
> Fixes: 16962b2404ac ("bpf: sockmap, add selftests")
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  tools/testing/selftests/bpf/test_sockmap.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/bpf/test_sockmap.c b/tools/testing/selftests/bpf/test_sockmap.c
> index 3654babfac59..4c7cb206b31d 100644
> --- a/tools/testing/selftests/bpf/test_sockmap.c
> +++ b/tools/testing/selftests/bpf/test_sockmap.c
> @@ -681,7 +681,8 @@ static int msg_loop(int fd, int iov_count, int iov_length, int cnt,
>  				}
>  			}
>  
> -			s->bytes_recvd += recv;
> +			if (recv > 0)
> +				s->bytes_recvd += recv;
>  
>  			if (opt->check_recved_len && s->bytes_recvd > total_bytes) {
>  				errno = EMSGSIZE;
> -- 
> 2.43.0
> 

Acked-by: John Fastabend <john.fastabend@gmail.com>

[PATCH bpf-next 8/8] selftests/bpf: Drop duplicate bpf_map_lookup_elem in test_sockmap

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

bpf_map_lookup_elem is invoked in bpf_prog3() already, no need to invoke
it again. This patch drops it.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 tools/testing/selftests/bpf/progs/test_sockmap_kern.h | 3 ---
 1 file changed, 3 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..2399958991e7 100644
--- a/tools/testing/selftests/bpf/progs/test_sockmap_kern.h
+++ b/tools/testing/selftests/bpf/progs/test_sockmap_kern.h
@@ -177,9 +177,6 @@ int bpf_prog3(struct __sk_buff *skb)
 		return bpf_sk_redirect_hash(skb, &tls_sock_map, &ret, flags);
 #endif
 	}
-	f = bpf_map_lookup_elem(&sock_skb_opts, &one);
-	if (f && *f)
-		ret = SK_DROP;
 	err = bpf_skb_adjust_room(skb, 4, 0, 0);
 	if (err)
 		return SK_DROP;
-- 
2.43.0

RE: [PATCH bpf-next 8/8] selftests/bpf: Drop duplicate bpf_map_lookup_elem in test_sockmap

[John Fastabend]

Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> bpf_map_lookup_elem is invoked in bpf_prog3() already, no need to invoke
> it again. This patch drops it.
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  tools/testing/selftests/bpf/progs/test_sockmap_kern.h | 3 ---
>  1 file changed, 3 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..2399958991e7 100644
> --- a/tools/testing/selftests/bpf/progs/test_sockmap_kern.h
> +++ b/tools/testing/selftests/bpf/progs/test_sockmap_kern.h
> @@ -177,9 +177,6 @@ int bpf_prog3(struct __sk_buff *skb)
>  		return bpf_sk_redirect_hash(skb, &tls_sock_map, &ret, flags);
>  #endif
>  	}
> -	f = bpf_map_lookup_elem(&sock_skb_opts, &one);
> -	if (f && *f)
> -		ret = SK_DROP;
>  	err = bpf_skb_adjust_room(skb, 4, 0, 0);
>  	if (err)
>  		return SK_DROP;
> -- 
> 2.43.0
> 

Acked-by: John Fastabend <john.fastabend@gmail.com>

Re: [PATCH bpf-next 0/8] fixes for test_sockmap

[Jakub Sitnicki]

On Thu, May 23, 2024 at 02:49 PM +08, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
>
> This patchset contains some fixes and improvements for test_sockmap.
>
> 3-5: switching attachments to bpf_link as Jakub suggested in [1].
> 1-2, 6-8: Small fixes.
>
> [1]
> https://lore.kernel.org/bpf/87zfsiw3a3.fsf@cloudflare.com/
>
> Geliang Tang (8):
>   selftests/bpf: Fix tx_prog_fd values in test_sockmap
>   selftests/bpf: Drop duplicate definition of i in test_sockmap
>   selftests/bpf: Use bpf_link attachments in test_sockmap
>   selftests/bpf: Replace tx_prog_fd with tx_prog in test_sockmap
>   selftests/bpf: Drop prog_fd array in test_sockmap
>   selftests/bpf: Fix size of map_fd in test_sockmap
>   selftests/bpf: Check length of recv in test_sockmap
>   selftests/bpf: Drop duplicate bpf_map_lookup_elem in test_sockmap
>
>  .../selftests/bpf/progs/test_sockmap_kern.h   |   3 -
>  tools/testing/selftests/bpf/test_sockmap.c    | 101 +++++++++---------
>  2 files changed, 51 insertions(+), 53 deletions(-)

Tested-by: Jakub Sitnicki <jakub@cloudflare.com>

Re: [PATCH bpf-next 0/8] fixes for test_sockmap

[patchwork-bot+netdevbpf]

Hello:

This series was applied to bpf/bpf-next.git (master)
by Daniel Borkmann <daniel@iogearbox.net>:

On Thu, 23 May 2024 14:49:56 +0800 you wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> This patchset contains some fixes and improvements for test_sockmap.
> 
> 3-5: switching attachments to bpf_link as Jakub suggested in [1].
> 1-2, 6-8: Small fixes.
> 
> [...]

Here is the summary with links:
  - [bpf-next,1/8] selftests/bpf: Fix tx_prog_fd values in test_sockmap
    https://git.kernel.org/bpf/bpf-next/c/d95ba15b9784
  - [bpf-next,2/8] selftests/bpf: Drop duplicate definition of i in test_sockmap
    https://git.kernel.org/bpf/bpf-next/c/a9f0ea175948
  - [bpf-next,3/8] selftests/bpf: Use bpf_link attachments in test_sockmap
    https://git.kernel.org/bpf/bpf-next/c/3f32a115f61d
  - [bpf-next,4/8] selftests/bpf: Replace tx_prog_fd with tx_prog in test_sockmap
    https://git.kernel.org/bpf/bpf-next/c/24bb90a42633
  - [bpf-next,5/8] selftests/bpf: Drop prog_fd array in test_sockmap
    https://git.kernel.org/bpf/bpf-next/c/467a0c79b551
  - [bpf-next,6/8] selftests/bpf: Fix size of map_fd in test_sockmap
    https://git.kernel.org/bpf/bpf-next/c/dcb681b659f2
  - [bpf-next,7/8] selftests/bpf: Check length of recv in test_sockmap
    https://git.kernel.org/bpf/bpf-next/c/de1b5ea789dc
  - [bpf-next,8/8] selftests/bpf: Drop duplicate bpf_map_lookup_elem in test_sockmap
    https://git.kernel.org/bpf/bpf-next/c/49784c797932

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