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

[Michal Luczaj <mhal@rbox.co>]

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/

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