[PATCH bpf v3 0/4] bug fixes for sockmap

[PATCH bpf v3 0/4] bug fixes for sockmap

[Xu Kuohai]

From: Xu Kuohai <xukuohai@huawei.com>

bug fixes and a new test case for sockmap.

v3:
fix bpf ci failure

v2: https://lore.kernel.org/bpf/20230803064838.108784-1-xukuohai@huaweicloud.com
add a test case

v1:
https://lore.kernel.org/bpf/20230728105649.3978774-1-xukuohai@huaweicloud.com
https://lore.kernel.org/bpf/20230728105717.3978849-1-xukuohai@huaweicloud.com

Xu Kuohai (4):
  bpf, sockmap: Fix map type error in sock_map_del_link
  bpf, sockmap: Fix bug that strp_done cannot be called
  selftests/bpf: fix a CI failure caused by vsock sockmap test
  selftests/bpf: Add sockmap test for redirecting partial skb data

 include/linux/skmsg.h                         |  1 +
 net/core/skmsg.c                              | 10 ++-
 net/core/sock_map.c                           | 10 +--
 .../selftests/bpf/prog_tests/sockmap_listen.c | 74 ++++++++++++++++++-
 .../selftests/bpf/progs/test_sockmap_listen.c | 14 ++++
 5 files changed, 101 insertions(+), 8 deletions(-)

-- 
2.30.2

[PATCH bpf v3 1/4] bpf, sockmap: Fix map type error in sock_map_del_link

[Xu Kuohai]

From: Xu Kuohai <xukuohai@huawei.com>

sock_map_del_link() operates on both SOCKMAP and SOCKHASH, although
both types have member named "progs", the offset of "progs" member in
these two types is different, so "progs" should be accessed with the
real map type.

Fixes: 604326b41a6f ("bpf, sockmap: convert to generic sk_msg interface")
Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
Reviewed-by: John Fastabend <john.fastabend@gmail.com>
---
 net/core/sock_map.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/net/core/sock_map.c b/net/core/sock_map.c
index 19538d628714..936c5cabe9f3 100644
--- a/net/core/sock_map.c
+++ b/net/core/sock_map.c
@@ -148,13 +148,13 @@ static void sock_map_del_link(struct sock *sk,
 	list_for_each_entry_safe(link, tmp, &psock->link, list) {
 		if (link->link_raw == link_raw) {
 			struct bpf_map *map = link->map;
-			struct bpf_stab *stab = container_of(map, struct bpf_stab,
-							     map);
-			if (psock->saved_data_ready && stab->progs.stream_parser)
+			struct sk_psock_progs *progs = sock_map_progs(map);
+
+			if (psock->saved_data_ready && progs->stream_parser)
 				strp_stop = true;
-			if (psock->saved_data_ready && stab->progs.stream_verdict)
+			if (psock->saved_data_ready && progs->stream_verdict)
 				verdict_stop = true;
-			if (psock->saved_data_ready && stab->progs.skb_verdict)
+			if (psock->saved_data_ready && progs->skb_verdict)
 				verdict_stop = true;
 			list_del(&link->list);
 			sk_psock_free_link(link);
-- 
2.30.2

[PATCH bpf v3 2/4] bpf, sockmap: Fix bug that strp_done cannot be called

[Xu Kuohai]

From: Xu Kuohai <xukuohai@huawei.com>

strp_done is only called when psock->progs.stream_parser is not NULL,
but stream_parser was set to NULL by sk_psock_stop_strp(), called
by sk_psock_drop() earlier. So, strp_done can never be called.

Introduce SK_PSOCK_RX_ENABLED to mark whether there is strp on psock.
Change the condition for calling strp_done from judging whether
stream_parser is set to judging whether this flag is set. This flag is
only set once when strp_init() succeeds, and will never be cleared later.

Fixes: c0d95d3380ee ("bpf, sockmap: Re-evaluate proto ops when psock is removed from sockmap")
Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
Reviewed-by: John Fastabend <john.fastabend@gmail.com>
---
 include/linux/skmsg.h |  1 +
 net/core/skmsg.c      | 10 ++++++++--
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/include/linux/skmsg.h b/include/linux/skmsg.h
index 054d7911bfc9..c1637515a8a4 100644
--- a/include/linux/skmsg.h
+++ b/include/linux/skmsg.h
@@ -62,6 +62,7 @@ struct sk_psock_progs {
 
 enum sk_psock_state_bits {
 	SK_PSOCK_TX_ENABLED,
+	SK_PSOCK_RX_STRP_ENABLED,
 };
 
 struct sk_psock_link {
diff --git a/net/core/skmsg.c b/net/core/skmsg.c
index a29508e1ff35..ef1a2eb6520b 100644
--- a/net/core/skmsg.c
+++ b/net/core/skmsg.c
@@ -1120,13 +1120,19 @@ static void sk_psock_strp_data_ready(struct sock *sk)
 
 int sk_psock_init_strp(struct sock *sk, struct sk_psock *psock)
 {
+	int ret;
+
 	static const struct strp_callbacks cb = {
 		.rcv_msg	= sk_psock_strp_read,
 		.read_sock_done	= sk_psock_strp_read_done,
 		.parse_msg	= sk_psock_strp_parse,
 	};
 
-	return strp_init(&psock->strp, sk, &cb);
+	ret = strp_init(&psock->strp, sk, &cb);
+	if (!ret)
+		sk_psock_set_state(psock, SK_PSOCK_RX_STRP_ENABLED);
+
+	return ret;
 }
 
 void sk_psock_start_strp(struct sock *sk, struct sk_psock *psock)
@@ -1154,7 +1160,7 @@ void sk_psock_stop_strp(struct sock *sk, struct sk_psock *psock)
 static void sk_psock_done_strp(struct sk_psock *psock)
 {
 	/* Parser has been stopped */
-	if (psock->progs.stream_parser)
+	if (sk_psock_test_state(psock, SK_PSOCK_RX_STRP_ENABLED))
 		strp_done(&psock->strp);
 }
 #else
-- 
2.30.2

[PATCH bpf v3 3/4] selftests/bpf: fix a CI failure caused by vsock sockmap test

[Xu Kuohai]

From: Xu Kuohai <xukuohai@huawei.com>

BPF CI has reported the following failure:

Error: #200/79 sockmap_listen/sockmap VSOCK test_vsock_redir
  Error: #200/79 sockmap_listen/sockmap VSOCK test_vsock_redir
  ./test_progs:vsock_unix_redir_connectible:1506: egress: write: Transport endpoint is not connected
  vsock_unix_redir_connectible:FAIL:1506
  ./test_progs:vsock_unix_redir_connectible:1506: ingress: write: Transport endpoint is not connected
  vsock_unix_redir_connectible:FAIL:1506
  ./test_progs:vsock_unix_redir_connectible:1506: egress: write: Transport endpoint is not connected
  vsock_unix_redir_connectible:FAIL:1506
  ./test_progs:vsock_unix_redir_connectible:1514: ingress: recv() err, errno=11
  vsock_unix_redir_connectible:FAIL:1514
  ./test_progs:vsock_unix_redir_connectible:1518: ingress: vsock socket map failed, a != b
  vsock_unix_redir_connectible:FAIL:1518
  ./test_progs:vsock_unix_redir_connectible:1525: ingress: want pass count 1, have 0

It’s because the recv(... MSG_DONTWAIT) syscall in the test case is
called before the queued work sk_psock_backlog() in the kernel finishes
executing. So the data to be read is still queued in psock->ingress_skb
and cannot be read by the user program. Therefore, the non-blocking
recv() reads nothing and reports an EAGAIN error.

So replace recv(... MSG_DONTWAIT) with xrecv_nonblock(), which calls
select() to wait for data to be readable or timeout before calls recv().

Fixes: d61bd8c1fd02 ("selftests/bpf: add a test case for vsock sockmap")
Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
---
 tools/testing/selftests/bpf/prog_tests/sockmap_listen.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c b/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
index b4f6f3a50ae5..ba35bcc66e7e 100644
--- a/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
+++ b/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
@@ -1432,7 +1432,7 @@ static void vsock_unix_redir_connectible(int sock_mapfd, int verd_mapfd,
 	if (n < 1)
 		goto out;
 
-	n = recv(mode == REDIR_INGRESS ? u0 : u1, &b, sizeof(b), MSG_DONTWAIT);
+	n = xrecv_nonblock(mode == REDIR_INGRESS ? u0 : u1, &b, sizeof(b), 0);
 	if (n < 0)
 		FAIL("%s: recv() err, errno=%d", log_prefix, errno);
 	if (n == 0)
-- 
2.30.2

[PATCH bpf v3 4/4] selftests/bpf: Add sockmap test for redirecting partial skb data

[Xu Kuohai]

From: Xu Kuohai <xukuohai@huawei.com>

Add a test case to check whether sockmap redirection works correctly
when data length returned by stream_parser is less than skb->len.

In addition, this test checks whether strp_done is called correctly.
The reason is that we returns skb->len - 1 from the stream_parser, so
the last byte in the skb will be held by strp->skb_head. Therefore,
if strp_done is not called to free strp->skb_head, we'll get a memleak
warning.

Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
---
 .../selftests/bpf/prog_tests/sockmap_listen.c | 72 +++++++++++++++++++
 .../selftests/bpf/progs/test_sockmap_listen.c | 14 ++++
 2 files changed, 86 insertions(+)

diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c b/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
index ba35bcc66e7e..5674a9d0cacf 100644
--- a/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
+++ b/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
@@ -869,6 +869,77 @@ static void test_msg_redir_to_listening(struct test_sockmap_listen *skel,
 	xbpf_prog_detach2(verdict, sock_map, BPF_SK_MSG_VERDICT);
 }
 
+static void redir_partial(int family, int sotype, int sock_map, int parser_map)
+{
+	int s, c0, c1, p0, p1;
+	int err, n, key, value;
+	char buf[] = "abc";
+
+	key = 0;
+	value = sizeof(buf) - 1;
+	err = xbpf_map_update_elem(parser_map, &key, &value, 0);
+	if (err)
+		return;
+
+	s = socket_loopback(family, sotype | SOCK_NONBLOCK);
+	if (s < 0)
+		goto clean_parser_map;
+
+	err = create_socket_pairs(s, family, sotype, &c0, &c1, &p0, &p1);
+	if (err)
+		goto close_srv;
+
+	err = add_to_sockmap(sock_map, p0, p1);
+	if (err)
+		goto close;
+
+	n = xsend(c1, buf, sizeof(buf), 0);
+	if (n < sizeof(buf))
+		FAIL("incomplete write");
+
+	n = xrecv_nonblock(c0, buf, sizeof(buf), 0);
+	if (n != sizeof(buf) - 1)
+		FAIL("expect %zu, received %d", sizeof(buf) - 1, n);
+
+close:
+	xclose(c0);
+	xclose(p0);
+	xclose(c1);
+	xclose(p1);
+close_srv:
+	xclose(s);
+
+clean_parser_map:
+	key = 0;
+	value = 0;
+	xbpf_map_update_elem(parser_map, &key, &value, 0);
+}
+
+static void test_skb_redir_partial(struct test_sockmap_listen *skel,
+				   struct bpf_map *inner_map, int family,
+				   int sotype)
+{
+	int verdict = bpf_program__fd(skel->progs.prog_stream_verdict);
+	int parser = bpf_program__fd(skel->progs.prog_stream_parser);
+	int parser_map = bpf_map__fd(skel->maps.parser_map);
+	int sock_map = bpf_map__fd(inner_map);
+	int err;
+
+	err = xbpf_prog_attach(parser, sock_map, BPF_SK_SKB_STREAM_PARSER, 0);
+	if (err)
+		return;
+
+	err = xbpf_prog_attach(verdict, sock_map, BPF_SK_SKB_STREAM_VERDICT, 0);
+	if (err)
+		goto detach;
+
+	redir_partial(family, sotype, sock_map, parser_map);
+
+	xbpf_prog_detach2(verdict, sock_map, BPF_SK_SKB_STREAM_VERDICT);
+detach:
+	xbpf_prog_detach2(parser, sock_map, BPF_SK_SKB_STREAM_PARSER);
+}
+
 static void test_reuseport_select_listening(int family, int sotype,
 					    int sock_map, int verd_map,
 					    int reuseport_prog)
@@ -1243,6 +1314,7 @@ static void test_redir(struct test_sockmap_listen *skel, struct bpf_map *map,
 	} tests[] = {
 		TEST(test_skb_redir_to_connected),
 		TEST(test_skb_redir_to_listening),
+		TEST(test_skb_redir_partial),
 		TEST(test_msg_redir_to_connected),
 		TEST(test_msg_redir_to_listening),
 	};
diff --git a/tools/testing/selftests/bpf/progs/test_sockmap_listen.c b/tools/testing/selftests/bpf/progs/test_sockmap_listen.c
index 325c9f193432..464d35bd57c7 100644
--- a/tools/testing/selftests/bpf/progs/test_sockmap_listen.c
+++ b/tools/testing/selftests/bpf/progs/test_sockmap_listen.c
@@ -28,12 +28,26 @@ struct {
 	__type(value, unsigned int);
 } verdict_map SEC(".maps");
 
+struct {
+	__uint(type, BPF_MAP_TYPE_ARRAY);
+	__uint(max_entries, 1);
+	__type(key, int);
+	__type(value, int);
+} parser_map SEC(".maps");
+
 bool test_sockmap = false; /* toggled by user-space */
 bool test_ingress = false; /* toggled by user-space */
 
 SEC("sk_skb/stream_parser")
 int prog_stream_parser(struct __sk_buff *skb)
 {
+	int *value;
+	__u32 key = 0;
+
+	value = bpf_map_lookup_elem(&parser_map, &key);
+	if (value && *value)
+		return *value;
+
 	return skb->len;
 }
 
-- 
2.30.2

Re: [PATCH bpf v3 0/4] bug fixes for sockmap

[patchwork-bot+netdevbpf]

Hello:

This series was applied to bpf/bpf.git (master)
by Martin KaFai Lau <martin.lau@kernel.org>:

On Fri,  4 Aug 2023 03:37:36 -0400 you wrote:
> From: Xu Kuohai <xukuohai@huawei.com>
> 
> bug fixes and a new test case for sockmap.
> 
> v3:
> fix bpf ci failure
> 
> [...]

Here is the summary with links:
  - [bpf,v3,1/4] bpf, sockmap: Fix map type error in sock_map_del_link
    https://git.kernel.org/bpf/bpf/c/7e96ec0e6605
  - [bpf,v3,2/4] bpf, sockmap: Fix bug that strp_done cannot be called
    https://git.kernel.org/bpf/bpf/c/809e4dc71a0f
  - [bpf,v3,3/4] selftests/bpf: fix a CI failure caused by vsock sockmap test
    https://git.kernel.org/bpf/bpf/c/90f0074cd9f9
  - [bpf,v3,4/4] selftests/bpf: Add sockmap test for redirecting partial skb data
    https://git.kernel.org/bpf/bpf/c/a4b7193d8efd

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

Re: [PATCH bpf v3 3/4] selftests/bpf: fix a CI failure caused by vsock sockmap test

[Daniel Borkmann]

Hi Xu,

On 8/4/23 9:37 AM, Xu Kuohai wrote:
> From: Xu Kuohai <xukuohai@huawei.com>
> 
> BPF CI has reported the following failure:
> 
> Error: #200/79 sockmap_listen/sockmap VSOCK test_vsock_redir
>    Error: #200/79 sockmap_listen/sockmap VSOCK test_vsock_redir
>    ./test_progs:vsock_unix_redir_connectible:1506: egress: write: Transport endpoint is not connected
>    vsock_unix_redir_connectible:FAIL:1506
>    ./test_progs:vsock_unix_redir_connectible:1506: ingress: write: Transport endpoint is not connected
>    vsock_unix_redir_connectible:FAIL:1506
>    ./test_progs:vsock_unix_redir_connectible:1506: egress: write: Transport endpoint is not connected
>    vsock_unix_redir_connectible:FAIL:1506
>    ./test_progs:vsock_unix_redir_connectible:1514: ingress: recv() err, errno=11
>    vsock_unix_redir_connectible:FAIL:1514
>    ./test_progs:vsock_unix_redir_connectible:1518: ingress: vsock socket map failed, a != b
>    vsock_unix_redir_connectible:FAIL:1518
>    ./test_progs:vsock_unix_redir_connectible:1525: ingress: want pass count 1, have 0
> 
> It’s because the recv(... MSG_DONTWAIT) syscall in the test case is
> called before the queued work sk_psock_backlog() in the kernel finishes
> executing. So the data to be read is still queued in psock->ingress_skb
> and cannot be read by the user program. Therefore, the non-blocking
> recv() reads nothing and reports an EAGAIN error.
> 
> So replace recv(... MSG_DONTWAIT) with xrecv_nonblock(), which calls
> select() to wait for data to be readable or timeout before calls recv().
> 
> Fixes: d61bd8c1fd02 ("selftests/bpf: add a test case for vsock sockmap")
> Signed-off-by: Xu Kuohai <xukuohai@huawei.com>

This is unfortunately still flaky and showing up from time to time in BPF CI, e.g. a
very recent one can be found here:

https://github.com/kernel-patches/bpf/actions/runs/6021475685/job/16335248421

[...]
Error: #211 sockmap_listen
Error: #211/79 sockmap_listen/sockmap VSOCK test_vsock_redir
   Error: #211/79 sockmap_listen/sockmap VSOCK test_vsock_redir
   ./test_progs:vsock_unix_redir_connectible:1501: egress: write: Transport endpoint is not connected
   vsock_unix_redir_connectible:FAIL:1501
   ./test_progs:vsock_unix_redir_connectible:1501: ingress: write: Transport endpoint is not connected
   vsock_unix_redir_connectible:FAIL:1501
   ./test_progs:vsock_unix_redir_connectible:1501: egress: write: Transport endpoint is not connected
   vsock_unix_redir_connectible:FAIL:1501

Could you continue to look into it to make the test more robust?

Thanks a lot,
Daniel

Re: [PATCH bpf v3 3/4] selftests/bpf: fix a CI failure caused by vsock sockmap test

[Xu Kuohai]

On 8/30/2023 4:10 PM, Daniel Borkmann wrote:
> Hi Xu,
> 
> On 8/4/23 9:37 AM, Xu Kuohai wrote:
>> From: Xu Kuohai <xukuohai@huawei.com>
>>
>> BPF CI has reported the following failure:
>>
>> Error: #200/79 sockmap_listen/sockmap VSOCK test_vsock_redir
>>    Error: #200/79 sockmap_listen/sockmap VSOCK test_vsock_redir
>>    ./test_progs:vsock_unix_redir_connectible:1506: egress: write: Transport endpoint is not connected
>>    vsock_unix_redir_connectible:FAIL:1506
>>    ./test_progs:vsock_unix_redir_connectible:1506: ingress: write: Transport endpoint is not connected
>>    vsock_unix_redir_connectible:FAIL:1506
>>    ./test_progs:vsock_unix_redir_connectible:1506: egress: write: Transport endpoint is not connected
>>    vsock_unix_redir_connectible:FAIL:1506
>>    ./test_progs:vsock_unix_redir_connectible:1514: ingress: recv() err, errno=11
>>    vsock_unix_redir_connectible:FAIL:1514
>>    ./test_progs:vsock_unix_redir_connectible:1518: ingress: vsock socket map failed, a != b
>>    vsock_unix_redir_connectible:FAIL:1518
>>    ./test_progs:vsock_unix_redir_connectible:1525: ingress: want pass count 1, have 0
>>
>> It’s because the recv(... MSG_DONTWAIT) syscall in the test case is
>> called before the queued work sk_psock_backlog() in the kernel finishes
>> executing. So the data to be read is still queued in psock->ingress_skb
>> and cannot be read by the user program. Therefore, the non-blocking
>> recv() reads nothing and reports an EAGAIN error.
>>
>> So replace recv(... MSG_DONTWAIT) with xrecv_nonblock(), which calls
>> select() to wait for data to be readable or timeout before calls recv().
>>
>> Fixes: d61bd8c1fd02 ("selftests/bpf: add a test case for vsock sockmap")
>> Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
> 
> This is unfortunately still flaky and showing up from time to time in BPF CI, e.g. a
> very recent one can be found here:
> 
> https://github.com/kernel-patches/bpf/actions/runs/6021475685/job/16335248421
> 
> [...]
> Error: #211 sockmap_listen
> Error: #211/79 sockmap_listen/sockmap VSOCK test_vsock_redir
>    Error: #211/79 sockmap_listen/sockmap VSOCK test_vsock_redir
>    ./test_progs:vsock_unix_redir_connectible:1501: egress: write: Transport endpoint is not connected
>    vsock_unix_redir_connectible:FAIL:1501
>    ./test_progs:vsock_unix_redir_connectible:1501: ingress: write: Transport endpoint is not connected
>    vsock_unix_redir_connectible:FAIL:1501
>    ./test_progs:vsock_unix_redir_connectible:1501: egress: write: Transport endpoint is not connected
>    vsock_unix_redir_connectible:FAIL:1501
> 
> Could you continue to look into it to make the test more robust?
> 

OK, it looks like I only noticed the recv failure and ignored the
write failure. I'll take it a look.

> Thanks a lot,
> Daniel