vsock: do not reset a socket in connect() once it has connected

PATCH In review Hyunwoo Kim 3 participants 8 messages Aug 12 – Sep 9

Subsystems (4)

Networking Network drivers virtio core VM sockets (AF_VSOCK)

lore

Share & tools

lore

Hyunwoo Kim

author

2026-08-12 20:13 UTC #0 lore raw

PATCH 1/1 vsock: do not reset a socket in connect() once it has connected

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.

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.

The socket can now be connected again while it is still on the table. Reconnecting to an address served by a different transport makes vsock_assign_transport() drop the transport from a live socket and free vsk->trans, even if skbs it already sent are still in flight. Reconnecting to the same address inserts a node that is already on the table, provided shutdown() has cleared SOCK_DONE in between.

sock->state cannot be used for the check either. shutdown() overwrites SS_CONNECTED with SS_DISCONNECTING.

Record on the socket that the connection completed, and check that instead. The sk_err path after the loop does the same reset, so guard it as well.

Fixes d021c344051a ("VSOCK: Introduce VM Sockets")

Cc [email protected]

Signed-off-by Hyunwoo Kim <[email protected]>

---
 include/net/af_vsock.h   |  2 ++
 net/vmw_vsock/af_vsock.c | 12 ++++++++----
 2 files changed, 10 insertions(+), 4 deletions(-)

include/net/af_vsock.h +2 -0

index 30046a3c20f735..d7ec976ed6e272 100644
@@ -63,6 +63,8 @@ struct vsock_sock {
 	struct delayed_work pending_work;
 	struct delayed_work close_work;
 	bool close_work_scheduled;
+	/* Set once the connection completed; never cleared. */
+	bool ever_connected;
 	u32 peer_shutdown;
 	bool sent_request;
 	bool ignore_connecting_rst;

net/vmw_vsock/af_vsock.c +8 -4

index 622dbd04679944..735c91bb762242 100644
@@ -351,6 +351,8 @@ void vsock_insert_connected(struct vsock_sock *vsk)
 	struct list_head *list = vsock_connected_sockets(
 		&vsk->remote_addr, &vsk->local_addr);
 
+	vsk->ever_connected = true;
+
 	spin_lock_bh(&vsock_table_lock);
 	__vsock_insert_connected(list, vsk);
 	spin_unlock_bh(&vsock_table_lock);

@@ -1814,14 +1816,14 @@ static int vsock_connect(struct socket *sock, struct sockaddr_unsized *addr,
 		 * Note that allowing to "reset" an already established socket
 		 * here is racy and insecure.
 		 */
-		if (sk->sk_state == TCP_ESTABLISHED)
+		if (vsk->ever_connected)
 			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
+		 * !ever_connected implies that socket is not on
 		 * vsock_connected_table. We keep the binding and the transport
 		 * assigned.
 		 */

@@ -1849,8 +1851,10 @@ static int vsock_connect(struct socket *sock, struct sockaddr_unsized *addr,
 
 	if (sk->sk_err) {
 		err = -sk->sk_err;
-		sk->sk_state = TCP_CLOSE;
-		sock->state = SS_UNCONNECTED;
+		if (!vsk->ever_connected) {
+			sk->sk_state = TCP_CLOSE;
+			sock->state = SS_UNCONNECTED;
+		}
 	} else {
 		err = 0;
 	}

Signature · 1 line

2.43.0

Michal Luczaj

2026-08-13 09:59 UTC #1 lore raw

Quoted 4 lines · Hyunwoo Kim

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"?

Quoted 4 lines

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()?

How about:

net/vmw_vsock/af_vsock.c +6 -3

index 622dbd046799..39c42ef016c3 100644
@@ -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

Quoted 13 lines

The socket can now be connected again while it is still on the table. Reconnecting to an address served by a different transport makes vsock_assign_transport() drop the transport from a live socket and free vsk->trans, even if skbs it already sent are still in flight. Reconnecting to the same address inserts a node that is already on the table, provided shutdown() has cleared SOCK_DONE in between.
sock->state cannot be used for the check either. shutdown() overwrites SS_CONNECTED with SS_DISCONNECTING.
Record on the socket that the connection completed, and check that instead. The sk_err path after the loop does the same reset, so guard it as well.

Hyunwoo Kim

author

2026-08-14 01:45 UTC #2 lore raw

Quoted 7 lines · Michal Luczaj

Quoted 4 lines · Hyunwoo Kim
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.

Quoted 9 lines

Quoted 5 lines
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 30 lines

How about:

net/vmw_vsock/af_vsock.c +6 -3

index 622dbd046799..39c42ef016c3 100644
@@ -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

Quoted 1 line

?

Yes, I like it better than mine. I confirmed it fixes the issue.

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

Best regards,
Hyunwoo Kim

Michal Luczaj

2026-08-16 22:30 UTC #3 lore raw

Quoted 11 lines · Hyunwoo Kim

Quoted 7 lines · Michal Luczaj
Quoted 4 lines · Hyunwoo Kim
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 38 lines

Quoted 8 lines
Quoted 4 lines
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 27 lines
How about:
net/vmw_vsock/af_vsock.c +6 -3
index 622dbd046799..39c42ef016c3 100644
@@ -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;
+		}
 

Quoted 6 lines

Quoted 4 lines
net/vmw_vsock/af_vsock.c +0 -0
 		/* 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.

Quoted 1 line

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

Stefano Garzarella

maintainer

2026-08-19 08:54 UTC #4 lore raw

Quoted 60 lines · Michal Luczaj

Quoted 11 lines · Hyunwoo Kim
Quoted 7 lines · Michal Luczaj
Quoted 4 lines · Hyunwoo Kim
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 38 lines
Quoted 8 lines
Quoted 4 lines
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 27 lines
How about:
net/vmw_vsock/af_vsock.c +6 -3
index 622dbd046799..39c42ef016c3 100644
@@ -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;
+		}
 
Quoted 6 lines
Quoted 4 lines
net/vmw_vsock/af_vsock.c +0 -0
 		/* 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.

Quoted 7 lines

Great, thanks.
Quoted 1 line
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.

Quoted 2 lines

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

Hyunwoo Kim

author

2026-09-07 18:35 UTC #5 lore raw

Quoted 84 lines · Stefano Garzarella

Quoted 60 lines · Michal Luczaj
Quoted 11 lines · Hyunwoo Kim
Quoted 7 lines · Michal Luczaj
Quoted 4 lines · Hyunwoo Kim
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 38 lines
Quoted 8 lines
Quoted 4 lines
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 27 lines
How about:
net/vmw_vsock/af_vsock.c +6 -3
index 622dbd046799..39c42ef016c3 100644
@@ -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;
+		}
 
Quoted 6 lines
Quoted 4 lines
net/vmw_vsock/af_vsock.c +0 -0
 		/* 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.
Quoted 7 lines
Great, thanks.
Quoted 1 line
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.

Quoted 9 lines

Quoted 2 lines
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

Gentle ping. I'd appreciate an update on where this stands.

Best regards,
Hyunwoo Kim

Michal Luczaj

2026-09-08 15:18 UTC #6 lore raw

Quoted 1 line · Hyunwoo Kim

Gentle ping. I'd appreciate an update on where this stands.

Thanks for poking me. I'll post a patch today or tomorrow.

Apologies for the delay,
Michal

Michal Luczaj

2026-09-09 22:06 UTC #7 lore raw

In reply to Stefano Garzarella

Quoted 51 lines · Stefano Garzarella

Quoted 31 lines

Quoted 24 lines

net/vmw_vsock/af_vsock.c +6 -3

index 622dbd046799..39c42ef016c3 100644
@@ -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;
+		}

Quoted 6 lines

Quoted 4 lines
net/vmw_vsock/af_vsock.c +0 -0
 		/* 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.

Quoted 7 lines

Great, thanks.
Quoted 1 line
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;
	}

Quoted 8 lines

Just to be a bit more defensive, but I don't have a strong opinion, your version is also fine.
Quoted 2 lines
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.

Sure, one note: perhaps it's better to always return 0 on TCP_ESTABLISHED/TCP_CLOSING? From connect()'s PoV these two states mean "we did good, we connected to something for some time". Currently, even if the connection was established, virtio_transport_recv_connected()'s VIRTIO_VSOCK_OP_RW error handling may set sk_err = ENOBUFS, and connect() would return that. Even though connect() itself was a success.

Anyway, here's a fix for the uaf:
https://lore.kernel.org/netdev/[email protected]/
It's incomplete, but I'm not sure how to follow up. Please see the below---comment.

thanks,
Michal