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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ee9ee99d-115a-f488-2de5-f402daa892a8@huawei.com>
Date: Fri, 1 Sep 2023 16:38:08 +0800
From: Xu Kuohai <xukuohai@...wei.com>
To: Daniel Borkmann <daniel@...earbox.net>, Xu Kuohai
	<xukuohai@...weicloud.com>, <bpf@...r.kernel.org>, <netdev@...r.kernel.org>
CC: Bobby Eshleman <bobby.eshleman@...edance.com>
Subject: Re: [PATCH bpf-next v2] selftests/bpf: fix a CI failure caused by
 vsock write

On 9/1/2023 4:22 PM, Daniel Borkmann wrote:
> On 9/1/23 5:10 AM, Xu Kuohai wrote:
>> From: Xu Kuohai <xukuohai@...wei.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@...wei.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
> 
> .