netdev - Re: vsock broken after connect() returns EINTR (was Re: [PATCH net 2/2] vsock/test: Add test for SO

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: <js3gdbpaupbglmtowcycniidowz46fp23camtvsohac44eybzd@w5w5mfpyjawd>
Date: Tue, 15 Apr 2025 15:07:16 +0200
From: Stefano Garzarella <sgarzare@...hat.com>
To: Michal Luczaj <mhal@...x.co>
Cc: "David S. Miller" <davem@...emloft.net>, 
	Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, 
	Paolo Abeni <pabeni@...hat.com>, Simon Horman <horms@...nel.org>, netdev@...r.kernel.org
Subject: Re: vsock broken after connect() returns EINTR (was Re: [PATCH net
 2/2] vsock/test: Add test for SO_LINGER null ptr deref)

On Fri, Apr 11, 2025 at 04:43:35PM +0200, Michal Luczaj wrote:
>On 4/11/25 15:21, Stefano Garzarella wrote:
>> On Fri, Apr 04, 2025 at 12:06:36AM +0200, Michal Luczaj wrote:
>>> On 4/1/25 12:32, Stefano Garzarella wrote:
>>>> On Tue, Mar 25, 2025 at 02:22:45PM +0100, Michal Luczaj wrote:
>>>>> ...
>>>>> Plus, it appears to be broken: when I hit this condition and I try to
>>>>> re-connect to the same listener, I get ETIMEDOUT for loopback and
>>>>> ECONNRESET for g2h virtio; see [2].
>>>>
>>>> Could this be related to the fix I sent some days ago?
>>>> https://lore.kernel.org/netdev/20250328141528.420719-1-sgarzare@redhat.com/
>>>
>>> I've tried that. I've also took a hint from your other mail and attempted
>>> flushing the listener queue, but to no avail. Crude code below. Is there
>>> something wrong with it?
>>
>> I can't see anything wrong, but I'm starting to get confused :-(
>> we're talking about too many things in the same thread.
>
>Uhm, that's true, sorry. I've split the thread, hope this helps.
>
>> What issues do you want to highlight?
>
>Once connect() fails with EINTR (e.g. due to a signal delivery), retrying
>connect() (to the same listener) fails. That is what the code below was
>trying to show.

mmm, something is going wrong in the vsock_connect().

IIUC if we fails with EINTR, we are kind of resetting the socket.
Should we do the same we do in vsock_assign_transport() when we found 
that we are changing transport?

I mean calling release(), vsock_deassign_transport(). etc.
I'm worried about having pending packets in flight.

BTW we need to investigate more, I agree.

Thanks,
Stefano

>
>> #include <stdio.h>
>> #include <errno.h>
>> #include <stdlib.h>
>> #include <unistd.h>
>> #include <signal.h>
>> #include <pthread.h>
>> #include <sys/socket.h>
>> #include <linux/vm_sockets.h>
>>
>> static void die(const char *msg)
>> {
>> 	perror(msg);
>> 	exit(-1);
>> }
>>
>> static void barrier(pthread_barrier_t *barr)
>> {
>> 	errno = pthread_barrier_wait(barr);
>> 	if (errno && errno != PTHREAD_BARRIER_SERIAL_THREAD)
>> 		die("pthread_barrier_wait");
>> }
>>
>> static void flush_accept(int s)
>> {
>> 	int p = accept(s, NULL, NULL);
>> 	if (p < 0) {
>> 		if (errno != EAGAIN)
>> 			perror("accept");
>> 		return;
>> 	}
>>
>> 	printf("accept: drained\n");
>> 	close(p);
>> }
>>
>> static void handler(int signum)
>> {
>> 	/* nop */
>> }
>>
>> void static set_accept_timeout(int s)
>> {
>> 	struct timeval tv = { .tv_sec = 1 };
>> 	if (setsockopt(s, SOL_SOCKET, SO_RCVTIMEO, &tv, sizeof(tv)))
>> 		die("setsockopt SO_RCVTIMEO");
>> }
>>
>> void static set_connect_timeout(int s)
>> {
>> 	struct timeval tv = { .tv_sec = 1 };
>> 	if (setsockopt(s, AF_VSOCK, SO_VM_SOCKETS_CONNECT_TIMEOUT, &tv,
>> 		       sizeof(tv)))
>> 		die("setsockopt SO_VM_SOCKETS_CONNECT_TIMEOUT");
>> }
>>
>> static void *killer(void *arg)
>> {
>> 	pthread_barrier_t *barr = (pthread_barrier_t *)arg;
>> 	pid_t pid = getpid();
>>
>> 	if ((errno = pthread_setcanceltype(PTHREAD_CANCEL_ASYNCHRONOUS,
>> 					   NULL)))
>> 		die("pthread_setcanceltype");
>>
>> 	for (;;) {
>> 		barrier(barr);
>> 		if (kill(pid, SIGUSR1))
>> 			die("kill");
>> 		barrier(barr);
>> 	}
>>
>> 	return NULL;
>> }
>>
>> int main(void)
>> {
>> 	struct sockaddr_vm addr = {
>> 		.svm_family = AF_VSOCK,
>> 		.svm_cid = VMADDR_CID_LOCAL,
>> 		.svm_port = 1234
>> 	};
>> 	socklen_t alen = sizeof(addr);
>> 	pthread_barrier_t barr;
>> 	pthread_t tid;
>> 	int s, c;
>>
>> 	if ((errno = pthread_barrier_init(&barr, NULL, 2)))
>> 		die("pthread_barrier_init");
>>
>> 	if (signal(SIGUSR1, handler) == SIG_ERR)
>> 		die("signal");
>>
>> 	s = socket(AF_VSOCK, SOCK_STREAM, 0);
>> 	if (s < 0)
>> 		die("socket s");
>> 	set_accept_timeout(s);
>>
>> 	if (bind(s, (struct sockaddr *)&addr, alen))
>> 		die("bind");
>>
>> 	if (listen(s, 64))
>> 		die("listen");
>>
>> 	if ((errno = pthread_create(&tid, NULL, killer, &barr)))
>> 		die("pthread_create");
>>
>> 	for (;;) {
>> 		c = socket(AF_VSOCK, SOCK_STREAM, 0);
>> 		if (c < 0)
>> 			die("socket c");
>>
>> 		barrier(&barr);
>> 		if (connect(c, (struct sockaddr *)&addr, sizeof(addr)) &&
>> 		    errno == EINTR) {
>> 		    	printf("connect: EINTR\n");
>> 			break;
>> 		}
>> 		barrier(&barr);
>>
>> 		close(c);
>> 		flush_accept(s);
>> 	}
>>
>> 	if ((errno = pthread_cancel(tid)))
>> 		die("pthread_cancel");
>>
>> 	if ((errno = pthread_join(tid, NULL)))
>> 		die("pthread_join");
>>
>> 	flush_accept(s);
>> 	set_connect_timeout(c);
>> 	if (connect(c, (struct sockaddr *)&addr, sizeof(addr)))
>> 		die("re-connect");
>>
>> 	printf("okay?\n");
>>
>> 	return 0;
>> }
>>
>
>