| Message ID | 20230901031037.3314007-1-xukuohai@huaweicloud.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | b0193c731e438ac01ae90133962ef8b45368771f |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next,v2] selftests/bpf: fix a CI failure caused by vsock write | expand |
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!
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
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 > > .
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.
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;