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

On Mon, Aug 17, 2026 at 12:29:11AM +0200, Michal Luczaj wrote:
>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?

Yep, thanks for helping here!

My only doubt is if it makes sense to leave the `break` there, and add a 
similar check before resetting the socket, I mean something like this:

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

Just to be a bit more defensive, but I don't have a strong opinion, your 
version is also fine.

>And should any sk_err be consumed
>here, too? (`err = sock_error(sk)` instead of `err = -sk->sk_err`)

I'd stay with sock_error() to consume the error if it makes sense also 
for you.

Thanks,
Stefano