Subject: Re: [PATCH bpf v3 3/4] selftest/bpf: Parametrize AF_UNIX redir functions to accept send() flags

From: Jakub Sitnicki <[email-masked]>
Date: Tue, 09 Jul 2024 11:59:47 +0200

On Sun, Jul 07, 2024 at 11:28 PM +02, Michal Luczaj wrote:
> Extend pairs_redir_to_connected() and unix_inet_redir_to_connected() with a
> send_flags parameter. Replace write() with send() allowing packets to be
> sent as MSG_OOB.
>
> Signed-off-by: Michal Luczaj <[email-masked]>
> ---
>  .../selftests/bpf/prog_tests/sockmap_listen.c | 40 +++++++++++++------
>  1 file changed, 27 insertions(+), 13 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c b/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
> index c075d376fcab..59e16f8f2090 100644
> --- a/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
> +++ b/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
> @@ -1374,9 +1374,10 @@ static void test_redir(struct test_sockmap_listen *skel, struct bpf_map *map,
>  	}
>  }
>  
> -static void pairs_redir_to_connected(int cli0, int peer0, int cli1, int peer1,
> -				     int sock_mapfd, int nop_mapfd,
> -				     int verd_mapfd, enum redir_mode mode)
> +static void __pairs_redir_to_connected(int cli0, int peer0, int cli1, int peer1,
> +				       int sock_mapfd, int nop_mapfd,
> +				       int verd_mapfd, enum redir_mode mode,
> +				       int send_flags)
>  {
>  	const char *log_prefix = redir_mode_str(mode);
>  	unsigned int pass;
> @@ -1396,11 +1397,9 @@ static void pairs_redir_to_connected(int cli0, int peer0, int cli1, int peer1,
>  			return;
>  	}
>  
> -	n = write(cli1, "a", 1);
> -	if (n < 0)
> -		FAIL_ERRNO("%s: write", log_prefix);
> +	n = xsend(cli1, "a", 1, send_flags);
>  	if (n == 0)
> -		FAIL("%s: incomplete write", log_prefix);
> +		FAIL("%s: incomplete send", log_prefix);
>  	if (n < 1)
>  		return;
>  
> @@ -1418,6 +1417,14 @@ static void pairs_redir_to_connected(int cli0, int peer0, int cli1, int peer1,
>  		FAIL("%s: incomplete recv", log_prefix);
>  }
>  
> +static void pairs_redir_to_connected(int cli0, int peer0, int cli1, int peer1,
> +				     int sock_mapfd, int nop_mapfd,
> +				     int verd_mapfd, enum redir_mode mode)
> +{
> +	__pairs_redir_to_connected(cli0, peer0, cli1, peer1, sock_mapfd,
> +				   nop_mapfd, verd_mapfd, mode, 0);
> +}
> +
>  static void unix_redir_to_connected(int sotype, int sock_mapfd,
>  			       int verd_mapfd, enum redir_mode mode)
>  {
> @@ -1815,10 +1822,9 @@ static void inet_unix_skb_redir_to_connected(struct test_sockmap_listen *skel,
>  	xbpf_prog_detach2(verdict, sock_map, BPF_SK_SKB_VERDICT);
>  }
>  
> -static void unix_inet_redir_to_connected(int family, int type,
> -					int sock_mapfd, int nop_mapfd,
> -					int verd_mapfd,
> -					enum redir_mode mode)
> +static void __unix_inet_redir_to_connected(int family, int type, int sock_mapfd,
> +					   int nop_mapfd, int verd_mapfd,
> +					   enum redir_mode mode, int send_flags)
>  {
>  	int c0, c1, p0, p1;
>  	int sfd[2];
> @@ -1832,8 +1838,8 @@ static void unix_inet_redir_to_connected(int family, int type,
>  		goto close_cli0;
>  	c1 = sfd[0], p1 = sfd[1];
>  
> -	pairs_redir_to_connected(c0, p0, c1, p1,
> -				 sock_mapfd, nop_mapfd, verd_mapfd, mode);
> +	__pairs_redir_to_connected(c0, p0, c1, p1, sock_mapfd, nop_mapfd,
> +				   verd_mapfd, mode, send_flags);
>  
>  	xclose(c1);
>  	xclose(p1);
> @@ -1842,6 +1848,14 @@ static void unix_inet_redir_to_connected(int family, int type,
>  	xclose(p0);
>  }
>  
> +static void unix_inet_redir_to_connected(int family, int type, int sock_mapfd,
> +					 int nop_mapfd, int verd_mapfd,
> +					 enum redir_mode mode)
> +{
> +	__unix_inet_redir_to_connected(family, type, sock_mapfd, nop_mapfd,
> +				       verd_mapfd, mode, 0);
> +}
> +
>  static void unix_inet_skb_redir_to_connected(struct test_sockmap_listen *skel,
>  					    struct bpf_map *inner_map, int family)
>  {

I've got some cosmetic suggestions.

Instead of having two helper variants - with and without send_flags - we
could stick to just one and always pass send_flags. For readability I'd
use a constant for "no flags".

This way we keep the path open to convert
unix_inet_skb_redir_to_connected() to to a loop over a parameter
combination matrix, instead of open-coding multiple calls to
unix_inet_redir_to_connected() for each argument combination.

It seems doing it the current way, it is way too easy to miss
typos. Pretty sure we have another typo at [1], looks like should be
s/SOCK_DGRAM/SOCK_STREAM/.

[1]
https://elixir.bootlin.com/linux/v6.10-rc7/source/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c#L1863

See below for what I have in mind.

--8<--
diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c b/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
index 59e16f8f2090..3ddffaead2cd 100644
--- a/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
+++ b/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
@@ -29,6 +29,8 @@
 
 #include "sockmap_helpers.h"
 
+#define NO_FLAGS 0
+
 static void test_insert_invalid(struct test_sockmap_listen *skel __always_unused,
 				int family, int sotype, int mapfd)
 {
@@ -1374,10 +1376,10 @@ static void test_redir(struct test_sockmap_listen *skel, struct bpf_map *map,
 	}
 }
 
-static void __pairs_redir_to_connected(int cli0, int peer0, int cli1, int peer1,
-				       int sock_mapfd, int nop_mapfd,
-				       int verd_mapfd, enum redir_mode mode,
-				       int send_flags)
+static void pairs_redir_to_connected(int cli0, int peer0, int cli1, int peer1,
+				     int sock_mapfd, int nop_mapfd,
+				     int verd_mapfd, enum redir_mode mode,
+				     int send_flags)
 {
 	const char *log_prefix = redir_mode_str(mode);
 	unsigned int pass;
@@ -1417,14 +1419,6 @@ static void __pairs_redir_to_connected(int cli0, int peer0, int cli1, int peer1,
 		FAIL("%s: incomplete recv", log_prefix);
 }
 
-static void pairs_redir_to_connected(int cli0, int peer0, int cli1, int peer1,
-				     int sock_mapfd, int nop_mapfd,
-				     int verd_mapfd, enum redir_mode mode)
-{
-	__pairs_redir_to_connected(cli0, peer0, cli1, peer1, sock_mapfd,
-				   nop_mapfd, verd_mapfd, mode, 0);
-}
-
 static void unix_redir_to_connected(int sotype, int sock_mapfd,
 			       int verd_mapfd, enum redir_mode mode)
 {
@@ -1439,7 +1433,7 @@ static void unix_redir_to_connected(int sotype, int sock_mapfd,
 		goto close0;
 	c1 = sfd[0], p1 = sfd[1];
 
-	pairs_redir_to_connected(c0, p0, c1, p1, sock_mapfd, -1, verd_mapfd, mode);
+	pairs_redir_to_connected(c0, p0, c1, p1, sock_mapfd, -1, verd_mapfd, mode, NO_FLAGS);
 
 	xclose(c1);
 	xclose(p1);
@@ -1729,7 +1723,7 @@ static void udp_redir_to_connected(int family, int sock_mapfd, int verd_mapfd,
 	if (err)
 		goto close_cli0;
 
-	pairs_redir_to_connected(c0, p0, c1, p1, sock_mapfd, -1, verd_mapfd, mode);
+	pairs_redir_to_connected(c0, p0, c1, p1, sock_mapfd, -1, verd_mapfd, mode, NO_FLAGS);
 
 	xclose(c1);
 	xclose(p1);
@@ -1787,7 +1781,7 @@ static void inet_unix_redir_to_connected(int family, int type, int sock_mapfd,
 	if (err)
 		goto close;
 
-	pairs_redir_to_connected(c0, p0, c1, p1, sock_mapfd, -1, verd_mapfd, mode);
+	pairs_redir_to_connected(c0, p0, c1, p1, sock_mapfd, -1, verd_mapfd, mode, NO_FLAGS);
 
 	xclose(c1);
 	xclose(p1);
@@ -1822,9 +1816,9 @@ static void inet_unix_skb_redir_to_connected(struct test_sockmap_listen *skel,
 	xbpf_prog_detach2(verdict, sock_map, BPF_SK_SKB_VERDICT);
 }
 
-static void __unix_inet_redir_to_connected(int family, int type, int sock_mapfd,
-					   int nop_mapfd, int verd_mapfd,
-					   enum redir_mode mode, int send_flags)
+static void unix_inet_redir_to_connected(int family, int type, int sock_mapfd,
+					 int nop_mapfd, int verd_mapfd,
+					 enum redir_mode mode, int send_flags)
 {
 	int c0, c1, p0, p1;
 	int sfd[2];
@@ -1838,8 +1832,8 @@ static void __unix_inet_redir_to_connected(int family, int type, int sock_mapfd,
 		goto close_cli0;
 	c1 = sfd[0], p1 = sfd[1];
 
-	__pairs_redir_to_connected(c0, p0, c1, p1, sock_mapfd, nop_mapfd,
-				   verd_mapfd, mode, send_flags);
+	pairs_redir_to_connected(c0, p0, c1, p1, sock_mapfd, nop_mapfd,
+				 verd_mapfd, mode, send_flags);
 
 	xclose(c1);
 	xclose(p1);
@@ -1848,14 +1842,6 @@ static void __unix_inet_redir_to_connected(int family, int type, int sock_mapfd,
 	xclose(p0);
 }
 
-static void unix_inet_redir_to_connected(int family, int type, int sock_mapfd,
-					 int nop_mapfd, int verd_mapfd,
-					 enum redir_mode mode)
-{
-	__unix_inet_redir_to_connected(family, type, sock_mapfd, nop_mapfd,
-				       verd_mapfd, mode, 0);
-}
-
 static void unix_inet_skb_redir_to_connected(struct test_sockmap_listen *skel,
 					    struct bpf_map *inner_map, int family)
 {
@@ -1872,31 +1858,31 @@ static void unix_inet_skb_redir_to_connected(struct test_sockmap_listen *skel,
 	skel->bss->test_ingress = false;
 	unix_inet_redir_to_connected(family, SOCK_DGRAM,
 				     sock_map, -1, verdict_map,
-				     REDIR_EGRESS);
+				     REDIR_EGRESS, NO_FLAGS);
 	unix_inet_redir_to_connected(family, SOCK_DGRAM,
 				     sock_map, -1, verdict_map,
-				     REDIR_EGRESS);
+				     REDIR_EGRESS, NO_FLAGS);
 
 	unix_inet_redir_to_connected(family, SOCK_DGRAM,
 				     sock_map, nop_map, verdict_map,
-				     REDIR_EGRESS);
+				     REDIR_EGRESS, NO_FLAGS);
 	unix_inet_redir_to_connected(family, SOCK_STREAM,
 				     sock_map, nop_map, verdict_map,
-				     REDIR_EGRESS);
+				     REDIR_EGRESS, NO_FLAGS);
 	skel->bss->test_ingress = true;
 	unix_inet_redir_to_connected(family, SOCK_DGRAM,
 				     sock_map, -1, verdict_map,
-				     REDIR_INGRESS);
+				     REDIR_INGRESS, NO_FLAGS);
 	unix_inet_redir_to_connected(family, SOCK_STREAM,
 				     sock_map, -1, verdict_map,
-				     REDIR_INGRESS);
+				     REDIR_INGRESS, NO_FLAGS);
 
 	unix_inet_redir_to_connected(family, SOCK_DGRAM,
 				     sock_map, nop_map, verdict_map,
-				     REDIR_INGRESS);
+				     REDIR_INGRESS, NO_FLAGS);
 	unix_inet_redir_to_connected(family, SOCK_STREAM,
 				     sock_map, nop_map, verdict_map,
-				     REDIR_INGRESS);
+				     REDIR_INGRESS, NO_FLAGS);
 
 	xbpf_prog_detach2(verdict, sock_map, BPF_SK_SKB_VERDICT);
 }