On 2025-03-07 17:01:11, Michal Luczaj wrote: > On 3/7/25 15:35, Stefano Garzarella wrote: > > On Fri, Mar 07, 2025 at 10:58:55AM +0100, Michal Luczaj wrote: > >>> Signal delivered during connect() may result in a disconnect of an already > >>> TCP_ESTABLISHED socket. Problem is that such established socket might have > >>> been placed in a sockmap before the connection was closed. We end up with a > >>> SS_UNCONNECTED vsock in a sockmap. And this, combined with the ability to > >>> reassign (unconnected) vsock's transport to NULL, breaks the sockmap > >>> contract. As manifested by WARN_ON_ONCE. > >> > >> Note that Luigi is currently working on a (vsock test suit) test[1] for a > >> related bug, which could be neatly adapted to test this bug as well. > >> [1]: https://lore.kernel.org/netdev/20250306-test_vsock-v1-0-0320b5accf92@xxxxxxxxxx/ > > > > Can you work with Luigi to include the changes in that series? > > I was just going to wait for Luigi to finish his work (no rush, really) and > then try to parametrize it. > > That is unless BPF/sockmap maintainers decide this thread's thing is a > sockmap thing and should be in tools/testing/selftests/bpf. I think it makes sense to pull into selftests/bpf then it would get run from BPF CI so we can ensure BPF changes will keep this working correctly. I guess the trick would be to see how long to run racer to get the warning but also not hang the CI. If you run it for 5 seconds or so does it trip? Or are you running this for minutes? If it takes too long to run it could be put into test_sockmap which has longer running things already. We also have some longer TCP compliance tests that would be good to start running in public somehow so might think a bit more how the infra for testing is going in sockmap side. Thanks! > > Below is a repro. If I'm not mistaken, it's basically what Luigi wrote, > just sprinkled with map_update_elem() and recv(). > > #include <stdio.h> > #include <stdint.h> > #include <stdlib.h> > #include <unistd.h> > #include <errno.h> > #include <pthread.h> > #include <sys/wait.h> > #include <sys/socket.h> > #include <sys/syscall.h> > #include <linux/bpf.h> > #include <linux/vm_sockets.h> > > static void die(const char *msg) > { > perror(msg); > exit(-1); > } > > static int sockmap_create(void) > { > union bpf_attr attr = { > .map_type = BPF_MAP_TYPE_SOCKMAP, > .key_size = sizeof(int), > .value_size = sizeof(int), > .max_entries = 1 > }; > int map; > > map = syscall(SYS_bpf, BPF_MAP_CREATE, &attr, sizeof(attr)); > if (map < 0) > die("map_create"); > > return map; > } > > static void map_update_elem(int fd, int key, int value) > { > union bpf_attr attr = { > .map_fd = fd, > .key = (uint64_t)&key, > .value = (uint64_t)&value, > .flags = BPF_ANY > }; > > (void)syscall(SYS_bpf, BPF_MAP_UPDATE_ELEM, &attr, sizeof(attr)); > } > > static void sighandler(int sig) > { > /* nop */ > } > > static void *racer(void *c) > { > int map = sockmap_create(); > > for (;;) { > map_update_elem(map, 0, *(int *)c); > if (kill(0, SIGUSR1) < 0) > die("kill"); > } > } > > int main(void) > { > struct sockaddr_vm addr = { > .svm_family = AF_VSOCK, > .svm_cid = VMADDR_CID_LOCAL, > .svm_port = VMADDR_PORT_ANY > }; > socklen_t alen = sizeof(addr); > struct sockaddr_vm bad_addr; > pthread_t thread; > int s, c; > > s = socket(AF_VSOCK, SOCK_SEQPACKET, 0); > if (s < 0) > die("socket s"); This would likely be a good test for all protocol types to stress test the update/connect/close flow. If we can land it in selftests/bpf I would be happy to make it run for TCP and others. It might be worth looking over ./tools/testing/selftests/bpf/test_sockmap.c and see if any tests from there should run for AF_VSOCK as well. > > if (bind(s, (struct sockaddr *)&addr, alen)) > die("bind"); > > if (listen(s, -1)) > die("listen"); > > if (getsockname(s, (struct sockaddr *)&addr, &alen)) > die("getsockname"); > > bad_addr = addr; > bad_addr.svm_cid = 0x42424242; /* non-existing */ > > if (signal(SIGUSR1, sighandler) == SIG_ERR) > die("signal"); > > if (pthread_create(&thread, 0, racer, &c)) > die("pthread_create"); > > for (;;) { > c = socket(AF_VSOCK, SOCK_SEQPACKET, 0); > if (c < 0) > die("socket c"); > > if (!connect(c, (struct sockaddr *)&addr, alen) || > errno != EINTR) > goto outro; > > if (!connect(c, (struct sockaddr *)&bad_addr, alen) || > errno != ESOCKTNOSUPPORT) > goto outro; > > (void)recv(c, &(char){0}, 1, MSG_DONTWAIT); > outro: > close(accept(s, NULL, NULL)); > close(c); > } > > return 0; > } > >
