Re: [PATCH bpf v3 4/4] selftest/bpf: Test sockmap redirect for AF_UNIX MSG_OOB

[Jakub Sitnicki <jakub@cloudflare.com>]

On Sun, Jul 07, 2024 at 11:28 PM +02, Michal Luczaj wrote:
> Verify that out-of-band packets are silently dropped before they reach the
> redirection logic. Attempt to recv() stale data that might have been
> erroneously left reachable from the original socket.
>
> The idea is to test with a 2 byte long send(). Should a MSG_OOB flag be in
> use, only the last byte will be treated as out-of-band. Test fails if
> verd_mapfd indicates a wrong number of packets processed (e.g. if OOB data
> wasn't dropped at the source) or if it was still somehow possble to recv()

Nit: typo s/possble/possible

Something like below will catch these for you:

$ cat ~/src/linux/.git/hooks/post-commit
exec git show --format=email HEAD | ./scripts/checkpatch.pl --strict --codespell

> OOB from the mapped socket.
>
> Signed-off-by: Michal Luczaj <mhal@rbox.co>
> ---
>  .../selftests/bpf/prog_tests/sockmap_listen.c | 26 ++++++++++++++++---
>  1 file changed, 23 insertions(+), 3 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c b/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
> index 59e16f8f2090..878fcca36a55 100644
> --- a/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
> +++ b/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
> @@ -1397,10 +1397,10 @@ static void __pairs_redir_to_connected(int cli0, int peer0, int cli1, int peer1,
>  			return;
>  	}
>  
> -	n = xsend(cli1, "a", 1, send_flags);
> -	if (n == 0)
> +	n = xsend(cli1, "ab", 2, send_flags);
> +	if (n >= 0 && n < 2)
>  		FAIL("%s: incomplete send", log_prefix);
> -	if (n < 1)
> +	if (n < 2)
>  		return;
>  
>  	key = SK_PASS;
> @@ -1415,6 +1415,19 @@ static void __pairs_redir_to_connected(int cli0, int peer0, int cli1, int peer1,
>  		FAIL_ERRNO("%s: recv_timeout", log_prefix);
>  	if (n == 0)
>  		FAIL("%s: incomplete recv", log_prefix);
> +
> +	if (send_flags & MSG_OOB) {
> +		key = 0;
> +		xbpf_map_delete_elem(sock_mapfd, &key);
> +		key = 1;
> +		xbpf_map_delete_elem(sock_mapfd, &key);
> +
> +		n = recv(peer1, &b, 1, MSG_OOB | MSG_DONTWAIT);
> +		if (n > 0)
> +			FAIL("%s: recv(MSG_OOB) succeeded", log_prefix);
> +		if (n == 0)
> +			FAIL("%s: recv(MSG_OOB) returned 0", log_prefix);
> +	}
>  }
>  
>  static void pairs_redir_to_connected(int cli0, int peer0, int cli1, int peer1,
> @@ -1883,6 +1896,10 @@ static void unix_inet_skb_redir_to_connected(struct test_sockmap_listen *skel,
>  	unix_inet_redir_to_connected(family, SOCK_STREAM,
>  				     sock_map, nop_map, verdict_map,
>  				     REDIR_EGRESS);
> +	__unix_inet_redir_to_connected(family, SOCK_STREAM,
> +				       sock_map, nop_map, verdict_map,
> +				       REDIR_EGRESS, MSG_OOB);
> +
>  	skel->bss->test_ingress = true;
>  	unix_inet_redir_to_connected(family, SOCK_DGRAM,
>  				     sock_map, -1, verdict_map,
> @@ -1897,6 +1914,9 @@ static void unix_inet_skb_redir_to_connected(struct test_sockmap_listen *skel,
>  	unix_inet_redir_to_connected(family, SOCK_STREAM,
>  				     sock_map, nop_map, verdict_map,
>  				     REDIR_INGRESS);
> +	__unix_inet_redir_to_connected(family, SOCK_STREAM,
> +				       sock_map, nop_map, verdict_map,
> +				       REDIR_INGRESS, MSG_OOB);
>  
>  	xbpf_prog_detach2(verdict, sock_map, BPF_SK_SKB_VERDICT);
>  }

This AF_UNIX MSG_OOB use case is super exotic, IMO. TBH, I've just
learned about it. Hence, I think we could use some more comments for the
future readers.

Also, it seems like we only need to remove peer1 from sockmap to test
the behavior. If so, I'd stick to just what is needed to set up the
test. Extra stuff makes you wonder what was the authors intention.

I'd also be more direct about checking return value & error. These
selftests often serve as the only example / API documentation out there.

--8<--
diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c b/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
index 25938e66a3c1..1e30e6861805 100644
--- a/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
+++ b/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
@@ -1399,6 +1399,7 @@ static void pairs_redir_to_connected(int cli0, int peer0, int cli1, int peer1,
 			return;
 	}
 
+	/* Last byte is OOB data when send_flags has MSG_OOB bit set */
 	n = xsend(cli1, "ab", 2, send_flags);
 	if (n >= 0 && n < 2)
 		FAIL("%s: incomplete send", log_prefix);
@@ -1419,16 +1420,22 @@ static void pairs_redir_to_connected(int cli0, int peer0, int cli1, int peer1,
 		FAIL("%s: incomplete recv", log_prefix);
 
 	if (send_flags & MSG_OOB) {
-		key = 0;
-		xbpf_map_delete_elem(sock_mapfd, &key);
-		key = 1;
-		xbpf_map_delete_elem(sock_mapfd, &key);
+		/* Check that we can't read OOB while in sockmap */
+		errno = 0;
+		n = recv(peer1, &b, 1, MSG_OOB | MSG_DONTWAIT);
+		if (n != -1 || errno != EOPNOTSUPP)
+			FAIL("%s: recv(MSG_OOB): expected EOPNOTSUPP: retval=%d errno=%d",
+			     log_prefix, n, errno);
+
+		/* Remove peer1 from sockmap */
+		xbpf_map_delete_elem(sock_mapfd, &(int){ 1 });
 
+		/* Check that OOB was dropped on redirect */
+		errno = 0;
 		n = recv(peer1, &b, 1, MSG_OOB | MSG_DONTWAIT);
-		if (n > 0)
-			FAIL("%s: recv(MSG_OOB) succeeded", log_prefix);
-		if (n == 0)
-			FAIL("%s: recv(MSG_OOB) returned 0", log_prefix);
+		if (n != -1 || errno != EINVAL)
+			FAIL("%s: recv(MSG_OOB): expected EINVAL: retval=%d errno=%d",
+			     log_prefix, n, errno);
 	}
 }
 
@@ -1882,9 +1889,11 @@ static void unix_inet_skb_redir_to_connected(struct test_sockmap_listen *skel,
 	unix_inet_redir_to_connected(family, SOCK_STREAM,
 				     sock_map, nop_map, verdict_map,
 				     REDIR_EGRESS, NO_FLAGS);
-	__unix_inet_redir_to_connected(family, SOCK_STREAM,
-				       sock_map, nop_map, verdict_map,
-				       REDIR_EGRESS, MSG_OOB);
+
+	/* MSG_OOB not supported by AF_UNIX SOCK_DGRAM */
+	unix_inet_redir_to_connected(family, SOCK_STREAM,
+				     sock_map, nop_map, verdict_map,
+				     REDIR_EGRESS, MSG_OOB);
 
 	skel->bss->test_ingress = true;
 	unix_inet_redir_to_connected(family, SOCK_DGRAM,
@@ -1900,9 +1909,11 @@ static void unix_inet_skb_redir_to_connected(struct test_sockmap_listen *skel,
 	unix_inet_redir_to_connected(family, SOCK_STREAM,
 				     sock_map, nop_map, verdict_map,
 				     REDIR_INGRESS, NO_FLAGS);
-	__unix_inet_redir_to_connected(family, SOCK_STREAM,
-				       sock_map, nop_map, verdict_map,
-				       REDIR_INGRESS, MSG_OOB);
+
+	/* MSG_OOB not supported by AF_UNIX SOCK_DGRAM */
+	unix_inet_redir_to_connected(family, SOCK_STREAM,
+				     sock_map, nop_map, verdict_map,
+				     REDIR_INGRESS, MSG_OOB);
 
 	xbpf_prog_detach2(verdict, sock_map, BPF_SK_SKB_VERDICT);
 }