diff mbox series

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

Message ID 20240707222842.4119416-5-mhal@rbox.co (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series af_unix: MSG_OOB handling fix & selftest | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for bpf
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag present in non-next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit fail Errors and warnings before: 45 this patch: 45
netdev/build_tools success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers warning 15 maintainers not CCed: yonghong.song@linux.dev song@kernel.org andrii@kernel.org martin.lau@linux.dev mykolal@fb.com daniel@iogearbox.net sdf@fomichev.me linux-kselftest@vger.kernel.org ast@kernel.org jolsa@kernel.org geliang@kernel.org eddyz87@gmail.com shuah@kernel.org haoluo@google.com kpsingh@kernel.org
netdev/build_clang fail Errors and warnings before: 36 this patch: 36
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn fail Errors and warnings before: 45 this patch: 45
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 51 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-PR success PR summary
bpf/vmtest-bpf-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-VM_Test-3 success Logs for Validate matrix.py
bpf/vmtest-bpf-VM_Test-2 success Logs for Unittests
bpf/vmtest-bpf-VM_Test-5 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-VM_Test-4 success Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-VM_Test-6 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps on aarch64 with gcc
bpf/vmtest-bpf-VM_Test-9 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
bpf/vmtest-bpf-VM_Test-10 success Logs for aarch64-gcc / veristat
bpf/vmtest-bpf-VM_Test-11 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-VM_Test-12 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-VM_Test-13 success Logs for s390x-gcc / test (test_maps, false, 360) / test_maps on s390x with gcc
bpf/vmtest-bpf-VM_Test-14 success Logs for s390x-gcc / test (test_progs, false, 360) / test_progs on s390x with gcc
bpf/vmtest-bpf-VM_Test-16 success Logs for s390x-gcc / test (test_verifier, false, 360) / test_verifier on s390x with gcc
bpf/vmtest-bpf-VM_Test-17 success Logs for s390x-gcc / veristat
bpf/vmtest-bpf-VM_Test-18 success Logs for set-matrix
bpf/vmtest-bpf-VM_Test-19 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-VM_Test-20 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-VM_Test-27 success Logs for x86_64-gcc / veristat / veristat on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-28 success Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-VM_Test-30 success Logs for x86_64-llvm-17 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-17
bpf/vmtest-bpf-VM_Test-24 success Logs for x86_64-gcc / test (test_progs_no_alu32_parallel, true, 30) / test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-26 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-25 success Logs for x86_64-gcc / test (test_progs_parallel, true, 30) / test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-29 success Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17-O2
bpf/vmtest-bpf-VM_Test-31 success Logs for x86_64-llvm-17 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-17
bpf/vmtest-bpf-VM_Test-32 success Logs for x86_64-llvm-17 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-17
bpf/vmtest-bpf-VM_Test-33 success Logs for x86_64-llvm-17 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-17
bpf/vmtest-bpf-VM_Test-34 success Logs for x86_64-llvm-17 / veristat
bpf/vmtest-bpf-VM_Test-35 success Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-VM_Test-36 success Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18-O2
bpf/vmtest-bpf-VM_Test-37 success Logs for x86_64-llvm-18 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-18
bpf/vmtest-bpf-VM_Test-38 success Logs for x86_64-llvm-18 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-18
bpf/vmtest-bpf-VM_Test-39 success Logs for x86_64-llvm-18 / test (test_progs_cpuv4, false, 360) / test_progs_cpuv4 on x86_64 with llvm-18
bpf/vmtest-bpf-VM_Test-40 success Logs for x86_64-llvm-18 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-18
bpf/vmtest-bpf-VM_Test-41 success Logs for x86_64-llvm-18 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-18
bpf/vmtest-bpf-VM_Test-42 success Logs for x86_64-llvm-18 / veristat
bpf/vmtest-bpf-VM_Test-23 success Logs for x86_64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-21 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-22 success Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-7 success Logs for aarch64-gcc / test (test_progs, false, 360) / test_progs on aarch64 with gcc
bpf/vmtest-bpf-VM_Test-8 success Logs for aarch64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-VM_Test-15 success Logs for s390x-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on s390x with gcc

Commit Message

Michal Luczaj July 7, 2024, 9:28 p.m. UTC 
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()
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(-)

Comments

Jakub Sitnicki July 9, 2024, 10:08 a.m. UTC | #1 
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);
 }
Michal Luczaj July 11, 2024, 8:35 p.m. UTC | #2 
On 7/9/24 12:08, Jakub Sitnicki wrote:
> 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

Heh, I have it. Just not in the right repo :) Will fix.

> 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.

Yeah, all fair points. Thanks.
diff mbox series

Patch

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);
 }