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: <rptu2o7jpqw5u5g4xt76ntyupsak3dcs7rzfhzeidn6vcwf6ku@dcd47yt6ebhu>
Date: Wed, 19 Nov 2025 11:36:15 +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 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.

Thanks,
Stefano