Message-ID: <e78254c5-8f2f-4dc5-bf81-401caefabdd1@rbox.co>
Date: Tue, 24 Sep 2024 12:25:33 +0200
From: Michal Luczaj <mhal@...x.co>
To: Jakub Sitnicki <jakub@...udflare.com>
Cc: Andrii Nakryiko <andrii@...nel.org>, Eduard Zingerman
 <eddyz87@...il.com>, Mykola Lysenko <mykolal@...com>,
 Alexei Starovoitov <ast@...nel.org>, Daniel Borkmann <daniel@...earbox.net>,
 Martin KaFai Lau <martin.lau@...ux.dev>, Song Liu <song@...nel.org>,
 Yonghong Song <yonghong.song@...ux.dev>,
 John Fastabend <john.fastabend@...il.com>, KP Singh <kpsingh@...nel.org>,
 Stanislav Fomichev <sdf@...ichev.me>, Hao Luo <haoluo@...gle.com>,
 Jiri Olsa <jolsa@...nel.org>, Shuah Khan <shuah@...nel.org>,
 bpf@...r.kernel.org, linux-kselftest@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH bpf-next v2 0/6] selftests/bpf: Various sockmap-related
 fixes

On 8/19/24 22:05, Jakub Sitnicki wrote:
> On Wed, Aug 14, 2024 at 06:14 PM +02, Michal Luczaj wrote:
>> On 8/6/24 19:45, Jakub Sitnicki wrote:
>>> On Tue, Aug 06, 2024 at 07:18 PM +02, Michal Luczaj wrote:
>>>> Great, thanks for the review. With this completed, I guess we can unwind
>>>> the (mail) stack to [1]. Is that ingress-to-local et al. something you
>>>> wanted to take care of yourself or can I give it a try?
>>>> [1] https://lore.kernel.org/netdev/87msmqn9ws.fsf@cloudflare.com/
>>>
>>> I haven't stated any work on. You're welcome to tackle that.
>>>
>>> All I have is a toy test that I've used to generate the redirect matrix.
>>> Perhaps it can serve as inspiration:
>>>
>>> https://github.com/jsitnicki/sockmap-redir-matrix
>>
>> All right, please let me know if this is more or less what you meant and
>> I'll post the whole series for a review (+patch to purge sockmap_listen of
>> redir tests, fix misnomers). [...]
> 
> Gave it a look as promised. It makes sense to me as well to put these
> tests in a new module. There will be some overlap with sockmap_listen,
> which has diverged from its inital scope, but we can dedup that later.
> 
> One thought that I had is that it could make sense to test the not
> supported redirect combos (and expect an error). Sometimes folks make
> changes and enable some parts of the API by accient.

All right, so I did what sockmap_listen does: check
test_sockmap_listen.c:verdict_map[SK_PASS] to see if the redirect took
place for a given combo. And that works well... except for skb/msg to
ingress af_vsock. Even though this is unsupported and no redirect
actually happens, verdict appears to be SK_PASS. Is this correct?

Maybe I'm missing something, so below is a crude testcase I've cobbled
together.

And sorry for the delay, I was away from keyboard.
Michal

All error logs:
./test_progs:unix_vsock_redir_fail:1600: want pass=0 / drop=1, have 1 / 0
unix_vsock_redir_fail:FAIL:1600

diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c b/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
index 4ee1148d22be..e59e1654f110 100644
--- a/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
+++ b/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
@@ -1561,6 +1561,45 @@ static void vsock_unix_redir_connectible(int sock_mapfd, int verd_mapfd,
 	close(u1);
 }
 
+static void unix_vsock_redir_fail(int sock_mapfd, int verd_mapfd)
+{
+	int v0, v1, u[2], pass, drop;
+	char a = 'a';
+
+	bpf_map_delete_elem(sock_mapfd, &(int){0});
+	bpf_map_delete_elem(sock_mapfd, &(int){1});
+	zero_verdict_count(verd_mapfd);
+
+	if (socketpair(AF_UNIX, SOCK_STREAM, 0, u)) {
+		FAIL_ERRNO("socketpair(af_unix)");
+		return;
+	}
+
+	if (create_pair(AF_VSOCK, SOCK_STREAM, &v0, &v1))
+		return;
+
+	if (add_to_sockmap(sock_mapfd, v0, u[0]))
+		return;
+
+	if (write(u[1], &a, sizeof(a)) != 1) {
+		FAIL_ERRNO("write()");
+		return;
+	}
+
+	errno = 0;
+	if (recv_timeout(v0, &a, sizeof(a), 0, 1) >= 0 ||
+	    recv_timeout(v1, &a, sizeof(a), 0, 1) >= 0 ||
+	    recv_timeout(u[0], &a, sizeof(a), 0, 1) >= 0 ||
+	    recv_timeout(u[1], &a, sizeof(a), 0, 1) >= 0)
+		FAIL("recv() returned >=0, errno=%d", errno);
+
+	if (xbpf_map_lookup_elem(verd_mapfd, &(int){SK_PASS}, &pass) ||
+	    xbpf_map_lookup_elem(verd_mapfd, &(int){SK_DROP}, &drop))
+		return;
+	if (pass != 0 || drop != 1)
+		FAIL("want pass=0 / drop=1, have %d / %d", pass, drop);
+}
+
 static void vsock_unix_skb_redir_connectible(struct test_sockmap_listen *skel,
 					     struct bpf_map *inner_map,
 					     int sotype)
@@ -1582,6 +1621,23 @@ static void vsock_unix_skb_redir_connectible(struct test_sockmap_listen *skel,
 	xbpf_prog_detach2(verdict, sock_map, BPF_SK_SKB_VERDICT);
 }
 
+static void unix_vsock_redir(struct test_sockmap_listen *skel, struct bpf_map *inner_map)
+{
+	int verdict = bpf_program__fd(skel->progs.prog_skb_verdict);
+	int verdict_map = bpf_map__fd(skel->maps.verdict_map);
+	int sock_map = bpf_map__fd(inner_map);
+	int err;
+
+	err = xbpf_prog_attach(verdict, sock_map, BPF_SK_SKB_VERDICT, 0);
+	if (err)
+		return;
+
+	skel->bss->test_ingress = true;
+	unix_vsock_redir_fail(sock_map, verdict_map);
+
+	xbpf_prog_detach2(verdict, sock_map, BPF_SK_SKB_VERDICT);
+}
+
 static void test_vsock_redir(struct test_sockmap_listen *skel, struct bpf_map *map)
 {
 	const char *family_name, *map_name;
@@ -1883,6 +1939,7 @@ void serial_test_sockmap_listen(void)
 	test_unix_redir(skel, skel->maps.sock_map, SOCK_DGRAM);
 	test_unix_redir(skel, skel->maps.sock_map, SOCK_STREAM);
 	test_vsock_redir(skel, skel->maps.sock_map);
+	unix_vsock_redir(skel, skel->maps.sock_map);
 
 	skel->bss->test_sockmap = false;
 	run_tests(skel, skel->maps.sock_hash, AF_INET);

Powered by blists - more mailing lists