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

On 8/14/26 03:45, Hyunwoo Kim wrote:

On Thu, Aug 13, 2026 at 11:42:17AM +0200, Michal Luczaj wrote:

quoted

On 8/12/26 22:13, Hyunwoo Kim wrote:

quoted

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.

quoted

quoted

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.

quoted

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