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

[Michal Luczaj <mhal@rbox.co>]

On 11/19/25 17:31, Stefano Garzarella wrote:
> 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.

Mhm, I suppose.

To follow up, should I add a version of syzkaller's lockdep warning repro
to vsock test suite? In theory it could test this fix here as well, but in
practice the race window is small and hitting it (the brute way) takes
prohibitively long.