linux-kernel - Re: [PATCH net] vsock: Ignore signal/timeout on connect() if already established

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: <663yvkk2sh5lesfvdeerlca567xb64qbwih52bxjftob3umsah@eamuykmarrfr>
Date: Wed, 19 Nov 2025 17:31:01 +0100
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>, virtualization@...ts.linux.dev, 
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net] vsock: Ignore signal/timeout on connect() if already
 established

On Wed, Nov 19, 2025 at 03:09:25PM +0100, Michal Luczaj wrote:
>On 11/19/25 11:36, Stefano Garzarella wrote:
>> On Tue, Nov 18, 2025 at 11:02:17PM +0100, Michal Luczaj wrote:
>>> On 11/18/25 10:51, Stefano Garzarella wrote:
>>>> On Mon, Nov 17, 2025 at 09:57:25PM +0100, Michal Luczaj wrote:
>>>>> ...
>>>>> +static void vsock_reset_interrupted(struct sock *sk)
>>>>> +{
>>>>> +	struct vsock_sock *vsk = vsock_sk(sk);
>>>>> +
>>>>> +	/* Try to cancel VIRTIO_VSOCK_OP_REQUEST skb sent out by
>>>>> +	 * transport->connect().
>>>>> +	 */
>>>>> +	vsock_transport_cancel_pkt(vsk);
>>>>> +
>>>>> +	/* Listener might have already responded with VIRTIO_VSOCK_OP_RESPONSE.
>>>>> +	 * Its handling expects our sk_state == TCP_SYN_SENT, which hereby we
>>>>> +	 * break. In such case VIRTIO_VSOCK_OP_RST will follow.
>>>>> +	 */
>>>>> +	sk->sk_state = TCP_CLOSE;
>>>>> +	sk->sk_socket->state = SS_UNCONNECTED;
>>>>> +}
>>>>> +
>>>>> static int vsock_connect(struct socket *sock, struct sockaddr *addr,
>>>>> 			 int addr_len, int flags)
>>>>> {
>>>>> @@ -1661,18 +1678,33 @@ static int vsock_connect(struct socket *sock, struct sockaddr *addr,
>>>>> 		timeout = schedule_timeout(timeout);
>>>>> 		lock_sock(sk);
>>>>>
>>>>> +		/* Connection established. Whatever happens to socket once we
>>>>> +		 * release it, that's not connect()'s concern. No need to go
>>>>> +		 * into signal and timeout handling. Call it a day.
>>>>> +		 *
>>>>> +		 * Note that allowing to "reset" an already established socket
>>>>> +		 * here is racy and insecure.
>>>>> +		 */
>>>>> +		if (sk->sk_state == TCP_ESTABLISHED)
>>>>> +			break;
>>>>> +
>>>>> +		/* If connection was _not_ established and a signal/timeout came
>>>>> +		 * to be, we want the socket's state reset. User space may want
>>>>> +		 * to retry.
>>>>> +		 *
>>>>> +		 * sk_state != TCP_ESTABLISHED implies that socket is not on
>>>>> +		 * vsock_connected_table. We keep the binding and the transport
>>>>> +		 * assigned.
>>>>> +		 */
>>>>> 		if (signal_pending(current)) {
>>>>> 			err = sock_intr_errno(timeout);
>>>>> -			sk->sk_state = sk->sk_state == TCP_ESTABLISHED ? TCP_CLOSING : TCP_CLOSE;
>>>>> -			sock->state = SS_UNCONNECTED;
>>>>> -			vsock_transport_cancel_pkt(vsk);
>>>>> -			vsock_remove_connected(vsk);
>>>>> +			vsock_reset_interrupted(sk);
>>>>> 			goto out_wait;
>>>>> -		} else if ((sk->sk_state != TCP_ESTABLISHED) && (timeout == 0)) {
>>>>> +		}
>>>>> +
>>>>> +		if (timeout == 0) {
>>>>> 			err = -ETIMEDOUT;
>>>>> -			sk->sk_state = TCP_CLOSE;
>>>>> -			sock->state = SS_UNCONNECTED;
>>>>> -			vsock_transport_cancel_pkt(vsk);
>>>>> +			vsock_reset_interrupted(sk);
>>>>> 			goto out_wait;
>>>>
>>>> I'm fine with the change, but now both code blocks are the same, so
>>>> can we unify them?
>>>> I mean something like this:
>>>> 		if (signal_pending(current) || timeout == 0 {
>>>> 			err = timeout == 0 ? -ETIMEDOUT : sock_intr_errno(timeout);
>>>> 			...
>>>> 		}
>>>>
>>>> Maybe at that point we can also remove the vsock_reset_interrupted()
>>>> function and put the code right there.
>>>>
>>>> BTW I don't have a strong opinion, what do you prefer?
>>>
>>> Sure, no problem.
>>>
>>> But I've realized invoking `sock_intr_errno(timeout)` is unnecessary.
>>> `timeout` can't be MAX_SCHEDULE_TIMEOUT, so the call always evaluates to
>>> -EINTR, right?
>>
>> IIUC currently schedule_timeout() can return MAX_SCHEDULE_TIMEOUT only
>> if it was called with that parameter, and I think we never call it in
>> that way, so I'd agree with you.
>>
>> My only concern is if it's true for all the stable branches we will
>> backport this patch.
>>
>> I would probably touch it as little as possible and continue using
>> sock_intr_errno() for now, but if you verify that it has always been
>> that way, then it's fine to change it.
>
>All right then, here's a v2 with minimum changes:
>https://lore.kernel.org/netdev/20251119-vsock-interrupted-connect-v2-1-70734cf1233f@rbox.co/
>

Thanks!

>Note a detail though: should signal and timeout happen at the same time,
>now it's the timeout errno returned.
>

Yeah, I thought about that, but I don't see any problems with that.
I mean, it's something that if it happens, it's still not deterministic,
so we're not really changing anything.

Thanks,
Stefano