Re: [PATCH net] vsock: do not reset a socket in connect() once it has connected

[Michal Luczaj <mhal@rbox.co>]

On 8/14/26 03:45, Hyunwoo Kim wrote:
> On Thu, Aug 13, 2026 at 11:42:17AM +0200, Michal Luczaj wrote:
>> On 8/12/26 22:13, Hyunwoo Kim wrote:
>>> commit 002541ef650b ("vsock: Ignore signal/timeout on connect() if
>>> already established") stopped connect() from resetting an established
>>> socket. The check only looks at whether sk_state is TCP_ESTABLISHED at
>>> that moment, and the state can change while connect() sleeps.
>>
>> I guess this makes my fix incomplete. "Fixes: 002541ef650b"?
> 
> It is incomplete, yes. But this has been triggerable since d021c344051a, so
> I'd keep Fixes: d021c344051a.

OK, I get it.

>>> A peer RST moves the socket to TCP_CLOSING, and it is not removed from
>>> vsock_connected_table on that path. connect() then wakes up, fails the
>>> check, and resets a socket that had actually connected to TCP_CLOSE and
>>> SS_UNCONNECTED.
>>
>> Thanks for the details. Do I get it right: connect() misses the fact that
>> socket might have already transitioned TCP_ESTABLISHED -> TCP_CLOSING
>> during schedule_timeout()?
> 
> Yes, that's it.
> 
>>
>> How about:
>>
>> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
>> index 622dbd046799..39c42ef016c3 100644
>> --- a/net/vmw_vsock/af_vsock.c
>> +++ b/net/vmw_vsock/af_vsock.c
>> @@ -1807,15 +1807,18 @@ 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
>> +		/* Connection (has been) 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 = -sk->sk_err;
>> +			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
>>
>> ?
> 
> Yes, I like it better than mine. I confirmed it fixes the issue.

Great, thanks.

> If you don't mind, would you take the patch from here?

Sure, no problem.

Stefano, does this look good to you? And should any sk_err be consumed
here, too? (`err = sock_error(sk)` instead of `err = -sk->sk_err`)

thanks,
Michal