Re: [PATCH net] vsock/bpf: Handle EINTR connect() racing against sockmap update

[John Fastabend <john.fastabend@gmail.com>]

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@redhat.com/
> > 
> > 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;
> }
> 
>