Re: [PATCH net v2 4/5] vsock: Do not reset a TCP_CLOSING socket

[Michal Luczaj <mhal@rbox.co>]

On 9/16/26 14:30, Stefano Garzarella wrote:
>> @@ -1834,23 +1833,22 @@ static int vsock_connect(struct socket *sock, struct sockaddr_unsized *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
>> +		/* Connection was 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 (sk->sk_state == TCP_ESTABLISHED ||
>> +		    sk->sk_state == TCP_CLOSING) {
>> +			err = 0;
>> +			goto out_wait;
>> +		}
>>
>> 		/* 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.
> 
> Should we keep "We keep the binding and the transport assigned." or 
> maybe add on the other phrase, "User space may want to retry, so we keep 
> ..."

Right, I'll extend the first paragraph.

>> 		 */
>> 		if (signal_pending(current) || timeout == 0) {
>> 			err = timeout == 0 ? -ETIMEDOUT : sock_intr_errno(timeout);
>> @@ -1874,8 +1872,8 @@ static int vsock_connect(struct socket *sock, struct sockaddr_unsized *addr,
>> 		prepare_to_wait(sk_sleep(sk), &wait, TASK_INTERRUPTIBLE);
>> 	}
>>
>> -	err = sock_error(sk);
>> -	if (err) {
> 
> Should we add a comment here explaining why we are doing this?

OK, I'll expand on this. With `if (sk->sk_state == TCP_ESTABLISHED) break`,
there's another bug here.

>> +	if (sk->sk_state != TCP_ESTABLISHED && sk->sk_state != TCP_CLOSING) {
>> +		err = sock_error(sk);
>> 		sk->sk_state = TCP_CLOSE;
>> 		sock->state = SS_UNCONNECTED;
>> 	}

We might exit the loop and get here on:
1. vsock_connect_timeout() timing out; setting sk_err, TCP_CLOSE,
   SS_UNCONNECTED
2. virtio_transport_recv_connecting() failing to establish connection;
   setting sk_err, TCP_CLOSE, and leaving SS_CONNECTING
And to be precise:
3. established connection getting torn by a transport event, locklessly
   setting sk_err, TCP_CLOSE, and leaving SS_CONNECTED
I sure hope I'm not missing something.

So the code above is taking a bit of a defensive approach, but I suppose it
could be simplified to an if()-less

  err = sock_error(sk);
  sock->state = SS_UNCONNECTED;

thanks,
Michal