[PATCH bpf-next v2] selftests/bpf: fix a CI failure caused by vsock write

[PATCH bpf-next v2] selftests/bpf: fix a CI failure caused by vsock write

[Xu Kuohai]

From: Xu Kuohai <xukuohai@huawei.com>

While commit 90f0074cd9f9 ("selftests/bpf: fix a CI failure caused by vsock sockmap test")
fixes a receive failure of vsock sockmap test, there is still a write failure:

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

The reason is that the vsock connection in the test is set to ESTABLISHED state
by function virtio_transport_recv_pkt, which is executed in a workqueue thread,
so when the user space test thread runs before the workqueue thread, this
problem occurs.

To fix it, before writing the connection, wait for it to be connected.

Fixes: d61bd8c1fd02 ("selftests/bpf: add a test case for vsock sockmap")
Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
---
v1->v2: initialize esize to sizeof(eval) to avoid getsockopt() reading
uninitialized value
---
 .../bpf/prog_tests/sockmap_helpers.h          | 29 +++++++++++++++++++
 .../selftests/bpf/prog_tests/sockmap_listen.c |  5 ++++
 2 files changed, 34 insertions(+)

diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_helpers.h b/tools/testing/selftests/bpf/prog_tests/sockmap_helpers.h
index d12665490a90..abd13d96d392 100644
--- a/tools/testing/selftests/bpf/prog_tests/sockmap_helpers.h
+++ b/tools/testing/selftests/bpf/prog_tests/sockmap_helpers.h
@@ -179,6 +179,35 @@
 		__ret;                                                         \
 	})
 
+static inline int poll_connect(int fd, unsigned int timeout_sec)
+{
+	struct timeval timeout = { .tv_sec = timeout_sec };
+	fd_set wfds;
+	int r;
+	int eval;
+	socklen_t esize = sizeof(eval);
+
+	FD_ZERO(&wfds);
+	FD_SET(fd, &wfds);
+
+	r = select(fd + 1, NULL, &wfds, NULL, &timeout);
+	if (r == 0)
+		errno = ETIME;
+
+	if (r != 1)
+		return -1;
+
+	if (getsockopt(fd, SOL_SOCKET, SO_ERROR, &eval, &esize) < 0)
+		return -1;
+
+	if (eval != 0) {
+		errno = eval;
+		return -1;
+	}
+
+	return 0;
+}
+
 static inline int poll_read(int fd, unsigned int timeout_sec)
 {
 	struct timeval timeout = { .tv_sec = timeout_sec };
diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c b/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
index 5674a9d0cacf..2d3bf38677b6 100644
--- a/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
+++ b/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
@@ -1452,6 +1452,11 @@ static int vsock_socketpair_connectible(int sotype, int *v0, int *v1)
 	if (p < 0)
 		goto close_cli;
 
+	if (poll_connect(c, IO_TIMEOUT_SEC) < 0) {
+		FAIL_ERRNO("poll_connect");
+		goto close_cli;
+	}
+
 	*v0 = p;
 	*v1 = c;
 
-- 
2.30.2

Re: [PATCH bpf-next v2] selftests/bpf: fix a CI failure caused by vsock write

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to bpf/bpf.git (master)
by Daniel Borkmann <daniel@iogearbox.net>:

On Fri,  1 Sep 2023 11:10:37 +0800 you wrote:
> From: Xu Kuohai <xukuohai@huawei.com>
> 
> While commit 90f0074cd9f9 ("selftests/bpf: fix a CI failure caused by vsock sockmap test")
> fixes a receive failure of vsock sockmap test, there is still a write failure:
> 
> 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
> 
> [...]

Here is the summary with links:
  - [bpf-next,v2] selftests/bpf: fix a CI failure caused by vsock write
    https://git.kernel.org/bpf/bpf/c/b0193c731e43

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

Re: [PATCH bpf-next v2] selftests/bpf: fix a CI failure caused by vsock write

[Daniel Borkmann]

On 9/1/23 5:10 AM, Xu Kuohai wrote:
> From: Xu Kuohai <xukuohai@huawei.com>
> 
> While commit 90f0074cd9f9 ("selftests/bpf: fix a CI failure caused by vsock sockmap test")
> fixes a receive failure of vsock sockmap test, there is still a write failure:
> 
> 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
> 
> The reason is that the vsock connection in the test is set to ESTABLISHED state
> by function virtio_transport_recv_pkt, which is executed in a workqueue thread,
> so when the user space test thread runs before the workqueue thread, this
> problem occurs.
> 
> To fix it, before writing the connection, wait for it to be connected.
> 
> Fixes: d61bd8c1fd02 ("selftests/bpf: add a test case for vsock sockmap")
> Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
> ---
> v1->v2: initialize esize to sizeof(eval) to avoid getsockopt() reading
> uninitialized value
> ---
>   .../bpf/prog_tests/sockmap_helpers.h          | 29 +++++++++++++++++++
>   .../selftests/bpf/prog_tests/sockmap_listen.c |  5 ++++
>   2 files changed, 34 insertions(+)
> 
> diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_helpers.h b/tools/testing/selftests/bpf/prog_tests/sockmap_helpers.h
> index d12665490a90..abd13d96d392 100644
> --- a/tools/testing/selftests/bpf/prog_tests/sockmap_helpers.h
> +++ b/tools/testing/selftests/bpf/prog_tests/sockmap_helpers.h
> @@ -179,6 +179,35 @@
>   		__ret;                                                         \
>   	})
>   
> +static inline int poll_connect(int fd, unsigned int timeout_sec)
> +{
> +	struct timeval timeout = { .tv_sec = timeout_sec };
> +	fd_set wfds;
> +	int r;
> +	int eval;
> +	socklen_t esize = sizeof(eval);
> +
> +	FD_ZERO(&wfds);
> +	FD_SET(fd, &wfds);
> +
> +	r = select(fd + 1, NULL, &wfds, NULL, &timeout);
> +	if (r == 0)
> +		errno = ETIME;
> +
> +	if (r != 1)
> +		return -1;
> +
> +	if (getsockopt(fd, SOL_SOCKET, SO_ERROR, &eval, &esize) < 0)
> +		return -1;
> +
> +	if (eval != 0) {
> +		errno = eval;
> +		return -1;
> +	}
> +
> +	return 0;
> +}
> +
>   static inline int poll_read(int fd, unsigned int timeout_sec)
>   {
>   	struct timeval timeout = { .tv_sec = timeout_sec };
> diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c b/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
> index 5674a9d0cacf..2d3bf38677b6 100644
> --- a/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
> +++ b/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
> @@ -1452,6 +1452,11 @@ static int vsock_socketpair_connectible(int sotype, int *v0, int *v1)
>   	if (p < 0)
>   		goto close_cli;
>   
> +	if (poll_connect(c, IO_TIMEOUT_SEC) < 0) {
> +		FAIL_ERRNO("poll_connect");
> +		goto close_cli;
> +	}
> +
>   	*v0 = p;
>   	*v1 = c;
>   
> 

Should the error path rather be ?

diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c b/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
index 2d3bf38677b6..8df8cbb447f1 100644
--- a/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
+++ b/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
@@ -1454,7 +1454,7 @@ static int vsock_socketpair_connectible(int sotype, int *v0, int *v1)

         if (poll_connect(c, IO_TIMEOUT_SEC) < 0) {
                 FAIL_ERRNO("poll_connect");
-               goto close_cli;
+               goto close_acc;
         }

         *v0 = p;
@@ -1462,6 +1462,8 @@ static int vsock_socketpair_connectible(int sotype, int *v0, int *v1)

         return 0;

+close_acc:
+       close(p);
  close_cli:
         close(c);
  close_srv:


Let me know and I'll squash this into the fix.

Anyway, BPF CI went through fine, only the ongoing panic left to be fixed after that.

Thanks,
Daniel

Re: [PATCH bpf-next v2] selftests/bpf: fix a CI failure caused by vsock write

[Xu Kuohai]

On 9/1/2023 4:22 PM, Daniel Borkmann wrote:
> On 9/1/23 5:10 AM, Xu Kuohai wrote:
>> From: Xu Kuohai <xukuohai@huawei.com>
>>
>> While commit 90f0074cd9f9 ("selftests/bpf: fix a CI failure caused by vsock sockmap test")
>> fixes a receive failure of vsock sockmap test, there is still a write failure:
>>
>> 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
>>
>> The reason is that the vsock connection in the test is set to ESTABLISHED state
>> by function virtio_transport_recv_pkt, which is executed in a workqueue thread,
>> so when the user space test thread runs before the workqueue thread, this
>> problem occurs.
>>
>> To fix it, before writing the connection, wait for it to be connected.
>>
>> Fixes: d61bd8c1fd02 ("selftests/bpf: add a test case for vsock sockmap")
>> Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
>> ---
>> v1->v2: initialize esize to sizeof(eval) to avoid getsockopt() reading
>> uninitialized value
>> ---
>>   .../bpf/prog_tests/sockmap_helpers.h          | 29 +++++++++++++++++++
>>   .../selftests/bpf/prog_tests/sockmap_listen.c |  5 ++++
>>   2 files changed, 34 insertions(+)
>>
>> diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_helpers.h b/tools/testing/selftests/bpf/prog_tests/sockmap_helpers.h
>> index d12665490a90..abd13d96d392 100644
>> --- a/tools/testing/selftests/bpf/prog_tests/sockmap_helpers.h
>> +++ b/tools/testing/selftests/bpf/prog_tests/sockmap_helpers.h
>> @@ -179,6 +179,35 @@
>>           __ret;                                                         \
>>       })
>> +static inline int poll_connect(int fd, unsigned int timeout_sec)
>> +{
>> +    struct timeval timeout = { .tv_sec = timeout_sec };
>> +    fd_set wfds;
>> +    int r;
>> +    int eval;
>> +    socklen_t esize = sizeof(eval);
>> +
>> +    FD_ZERO(&wfds);
>> +    FD_SET(fd, &wfds);
>> +
>> +    r = select(fd + 1, NULL, &wfds, NULL, &timeout);
>> +    if (r == 0)
>> +        errno = ETIME;
>> +
>> +    if (r != 1)
>> +        return -1;
>> +
>> +    if (getsockopt(fd, SOL_SOCKET, SO_ERROR, &eval, &esize) < 0)
>> +        return -1;
>> +
>> +    if (eval != 0) {
>> +        errno = eval;
>> +        return -1;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>>   static inline int poll_read(int fd, unsigned int timeout_sec)
>>   {
>>       struct timeval timeout = { .tv_sec = timeout_sec };
>> diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c b/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
>> index 5674a9d0cacf..2d3bf38677b6 100644
>> --- a/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
>> +++ b/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
>> @@ -1452,6 +1452,11 @@ static int vsock_socketpair_connectible(int sotype, int *v0, int *v1)
>>       if (p < 0)
>>           goto close_cli;
>> +    if (poll_connect(c, IO_TIMEOUT_SEC) < 0) {
>> +        FAIL_ERRNO("poll_connect");
>> +        goto close_cli;
>> +    }
>> +
>>       *v0 = p;
>>       *v1 = c;
>>
> 
> Should the error path rather be ?
> 
> diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c b/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
> index 2d3bf38677b6..8df8cbb447f1 100644
> --- a/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
> +++ b/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
> @@ -1454,7 +1454,7 @@ static int vsock_socketpair_connectible(int sotype, int *v0, int *v1)
> 
>          if (poll_connect(c, IO_TIMEOUT_SEC) < 0) {
>                  FAIL_ERRNO("poll_connect");
> -               goto close_cli;
> +               goto close_acc;
>          }
> 
>          *v0 = p;
> @@ -1462,6 +1462,8 @@ static int vsock_socketpair_connectible(int sotype, int *v0, int *v1)
> 
>          return 0;
> 
> +close_acc:
> +       close(p);
>   close_cli:
>          close(c);
>   close_srv:
> 
> 
> Let me know and I'll squash this into the fix.
>

Right, the accepted connection should be closed, thanks.

> Anyway, BPF CI went through fine, only the ongoing panic left to be fixed after that.
> 
> Thanks,
> Daniel
> 
> .

Re: [PATCH bpf-next v2] selftests/bpf: fix a CI failure caused by vsock write

[Daniel Borkmann]

On 9/1/23 10:38 AM, Xu Kuohai wrote:
> On 9/1/2023 4:22 PM, Daniel Borkmann wrote:
>> On 9/1/23 5:10 AM, Xu Kuohai wrote:
>>> From: Xu Kuohai <xukuohai@huawei.com>
[...]
>> Should the error path rather be ?
>>
>> diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c b/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
>> index 2d3bf38677b6..8df8cbb447f1 100644
>> --- a/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
>> +++ b/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
>> @@ -1454,7 +1454,7 @@ static int vsock_socketpair_connectible(int sotype, int *v0, int *v1)
>>
>>          if (poll_connect(c, IO_TIMEOUT_SEC) < 0) {
>>                  FAIL_ERRNO("poll_connect");
>> -               goto close_cli;
>> +               goto close_acc;
>>          }
>>
>>          *v0 = p;
>> @@ -1462,6 +1462,8 @@ static int vsock_socketpair_connectible(int sotype, int *v0, int *v1)
>>
>>          return 0;
>>
>> +close_acc:
>> +       close(p);
>>   close_cli:
>>          close(c);
>>   close_srv:
>>
>>
>> Let me know and I'll squash this into the fix.
>>
> 
> Right, the accepted connection should be closed, thanks.

Ok, done, pushed.