[PATCH net] vsock: Do not reset a TCP_CLOSING socket

[PATCH net] vsock: Do not reset a TCP_CLOSING socket

[Michal Luczaj]

Ensure connect() resets the socket only if it has never been established.
Handle the previously overlooked TCP_ESTABLISHED -> TCP_CLOSING
transition (on VIRTIO_VSOCK_OP_RST), which could race with the connect
loop.

Resetting a socket that is still present in connected_table can lead to
memory corruption. The reporter noted lost transports for in-flight skbs,
and I have reproduced crashes caused by re-insertion into connected_table.

  list_add double add: new=, prev=, next=.
  kernel BUG at lib/list_debug.c:35!
  Oops: invalid opcode: 0000 [#1] SMP KASAN NOPTI
  Workqueue: vsock-loopback vsock_loopback_work
  RIP: 0010:__list_add_valid_or_report+0x11f/0x130
  Call Trace:
   vsock_insert_connected.cold+0xe/0x13
   virtio_transport_recv_pkt+0x10e9/0x1460
   vsock_loopback_work+0x305/0x480
   process_one_work+0xe4c/0x1560
   worker_thread+0x4f1/0xd60
   kthread+0x36e/0x470
   ret_from_fork+0x47b/0x6b0
   ret_from_fork_asm+0x1a/0x30

Drop the inaccurate comment above signal_pending(). This fix is
supplementary to commit 002541ef650b ("vsock: Ignore signal/timeout on
connect() if already established"). Details at Link.

Fixes: d021c344051a ("VSOCK: Introduce VM Sockets")
Reported-by: Hyunwoo Kim <imv4bel@gmail.com>
Link: https://lore.kernel.org/netdev/anzT1fREOSyHT99k@v4bel/
Signed-off-by: Michal Luczaj <mhal@rbox.co>
---
Note that this is not a complete fix. connect()'s schedule_timeout() can
still race with two other functions that set sk_state = TCP_CLOSE while
keeping the socket in connected_table:
1. vmci_transport_handle_detach(): no way for me to test,
2. virtio_vsock_reset_sock(): tested by unbinding the driver
   (/sys/bus/virtio/drivers/virtio_transport/unbind).
The latter appears easy to fix by adding __vsock_remove_connected() and
switching to a _safe iterator in vsock_for_each_connected_socket().
---
 net/vmw_vsock/af_vsock.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
index f840498b58af..eec5dd6daebb 100644
--- a/net/vmw_vsock/af_vsock.c
+++ b/net/vmw_vsock/af_vsock.c
@@ -1834,23 +1834,20 @@ 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)
+		if (sk->sk_state == TCP_ESTABLISHED ||
+		    sk->sk_state == TCP_CLOSING)
 			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
-		 * vsock_connected_table. We keep the binding and the transport
-		 * assigned.
 		 */
 		if (signal_pending(current) || timeout == 0) {
 			err = timeout == 0 ? -ETIMEDOUT : sock_intr_errno(timeout);
@@ -1875,7 +1872,8 @@ static int vsock_connect(struct socket *sock, struct sockaddr_unsized *addr,
 	}
 
 	err = sock_error(sk);
-	if (err) {
+	if (err &&
+	    sk->sk_state != TCP_ESTABLISHED && sk->sk_state != TCP_CLOSING) {
 		sk->sk_state = TCP_CLOSE;
 		sock->state = SS_UNCONNECTED;
 	}

---
base-commit: 9a1599eeb8d18a2113e4cabcbd3bec5a8377dbed
change-id: 20260820-vsock-connect-reset-closing-98dd28769d7f

Best regards,
--  
Michal Luczaj <mhal@rbox.co>

Re: [PATCH net] vsock: Do not reset a TCP_CLOSING socket

[Bobby Eshleman]

On Wed, Sep 09, 2026 at 11:58:26PM +0200, Michal Luczaj wrote:
> Ensure connect() resets the socket only if it has never been established.
> Handle the previously overlooked TCP_ESTABLISHED -> TCP_CLOSING
> transition (on VIRTIO_VSOCK_OP_RST), which could race with the connect
> loop.
> 
> Resetting a socket that is still present in connected_table can lead to
> memory corruption. The reporter noted lost transports for in-flight skbs,
> and I have reproduced crashes caused by re-insertion into connected_table.
> 
>   list_add double add: new=, prev=, next=.
>   kernel BUG at lib/list_debug.c:35!
>   Oops: invalid opcode: 0000 [#1] SMP KASAN NOPTI
>   Workqueue: vsock-loopback vsock_loopback_work
>   RIP: 0010:__list_add_valid_or_report+0x11f/0x130
>   Call Trace:
>    vsock_insert_connected.cold+0xe/0x13
>    virtio_transport_recv_pkt+0x10e9/0x1460
>    vsock_loopback_work+0x305/0x480
>    process_one_work+0xe4c/0x1560
>    worker_thread+0x4f1/0xd60
>    kthread+0x36e/0x470
>    ret_from_fork+0x47b/0x6b0
>    ret_from_fork_asm+0x1a/0x30
> 
> Drop the inaccurate comment above signal_pending(). This fix is
> supplementary to commit 002541ef650b ("vsock: Ignore signal/timeout on
> connect() if already established"). Details at Link.
> 
> Fixes: d021c344051a ("VSOCK: Introduce VM Sockets")
> Reported-by: Hyunwoo Kim <imv4bel@gmail.com>
> Link: https://lore.kernel.org/netdev/anzT1fREOSyHT99k@v4bel/
> Signed-off-by: Michal Luczaj <mhal@rbox.co>
> ---
> Note that this is not a complete fix. connect()'s schedule_timeout() can
> still race with two other functions that set sk_state = TCP_CLOSE while
> keeping the socket in connected_table:
> 1. vmci_transport_handle_detach(): no way for me to test,
> 2. virtio_vsock_reset_sock(): tested by unbinding the driver
>    (/sys/bus/virtio/drivers/virtio_transport/unbind).
> The latter appears easy to fix by adding __vsock_remove_connected() and
> switching to a _safe iterator in vsock_for_each_connected_socket().
> ---
>  net/vmw_vsock/af_vsock.c | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
> index f840498b58af..eec5dd6daebb 100644
> --- a/net/vmw_vsock/af_vsock.c
> +++ b/net/vmw_vsock/af_vsock.c
> @@ -1834,23 +1834,20 @@ 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)
> +		if (sk->sk_state == TCP_ESTABLISHED ||
> +		    sk->sk_state == TCP_CLOSING)
>  			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
> -		 * vsock_connected_table. We keep the binding and the transport
> -		 * assigned.
>  		 */
>  		if (signal_pending(current) || timeout == 0) {
>  			err = timeout == 0 ? -ETIMEDOUT : sock_intr_errno(timeout);
> @@ -1875,7 +1872,8 @@ static int vsock_connect(struct socket *sock, struct sockaddr_unsized *addr,
>  	}
>  
>  	err = sock_error(sk);
> -	if (err) {
> +	if (err &&
> +	    sk->sk_state != TCP_ESTABLISHED && sk->sk_state != TCP_CLOSING) {

If the OP_RESPONSE + a blast of OP_RW that pushes past the buffer limit
arrives while we were scheduled out, we end up with sk_err = ENOBUFS
here. Then I guess connect() returns an error, but sk_state/sock->state
is still TCP_ESTABLISHED and SS_CONNECTED. If the user sees the error
and tries connect() again, they just get -EISCONN back. Maybe the
sock_error() needs to be moved within the conditional here, and then let
subsequent calls return the error to the user (it looks sendmsg() at
least will report it faithfully, but not sure about recvmsg() or the
others).

Best,
Bobby

Re: [PATCH net] vsock: Do not reset a TCP_CLOSING socket

[Michal Luczaj]

On 9/10/26 00:44, Bobby Eshleman wrote:
> On Wed, Sep 09, 2026 at 11:58:26PM +0200, Michal Luczaj wrote:
>> Ensure connect() resets the socket only if it has never been established.
>> Handle the previously overlooked TCP_ESTABLISHED -> TCP_CLOSING
>> transition (on VIRTIO_VSOCK_OP_RST), which could race with the connect
>> loop.
>>
>> Resetting a socket that is still present in connected_table can lead to
>> memory corruption. The reporter noted lost transports for in-flight skbs,
>> and I have reproduced crashes caused by re-insertion into connected_table.
>>
>>   list_add double add: new=, prev=, next=.
>>   kernel BUG at lib/list_debug.c:35!
>>   Oops: invalid opcode: 0000 [#1] SMP KASAN NOPTI
>>   Workqueue: vsock-loopback vsock_loopback_work
>>   RIP: 0010:__list_add_valid_or_report+0x11f/0x130
>>   Call Trace:
>>    vsock_insert_connected.cold+0xe/0x13
>>    virtio_transport_recv_pkt+0x10e9/0x1460
>>    vsock_loopback_work+0x305/0x480
>>    process_one_work+0xe4c/0x1560
>>    worker_thread+0x4f1/0xd60
>>    kthread+0x36e/0x470
>>    ret_from_fork+0x47b/0x6b0
>>    ret_from_fork_asm+0x1a/0x30
>>
>> Drop the inaccurate comment above signal_pending(). This fix is
>> supplementary to commit 002541ef650b ("vsock: Ignore signal/timeout on
>> connect() if already established"). Details at Link.
>>
>> Fixes: d021c344051a ("VSOCK: Introduce VM Sockets")
>> Reported-by: Hyunwoo Kim <imv4bel@gmail.com>
>> Link: https://lore.kernel.org/netdev/anzT1fREOSyHT99k@v4bel/
>> Signed-off-by: Michal Luczaj <mhal@rbox.co>
>> ---
>> Note that this is not a complete fix. connect()'s schedule_timeout() can
>> still race with two other functions that set sk_state = TCP_CLOSE while
>> keeping the socket in connected_table:
>> 1. vmci_transport_handle_detach(): no way for me to test,
>> 2. virtio_vsock_reset_sock(): tested by unbinding the driver
>>    (/sys/bus/virtio/drivers/virtio_transport/unbind).
>> The latter appears easy to fix by adding __vsock_remove_connected() and
>> switching to a _safe iterator in vsock_for_each_connected_socket().
>> ---
>>  net/vmw_vsock/af_vsock.c | 14 ++++++--------
>>  1 file changed, 6 insertions(+), 8 deletions(-)
>>
>> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
>> index f840498b58af..eec5dd6daebb 100644
>> --- a/net/vmw_vsock/af_vsock.c
>> +++ b/net/vmw_vsock/af_vsock.c
>> @@ -1834,23 +1834,20 @@ 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)
>> +		if (sk->sk_state == TCP_ESTABLISHED ||
>> +		    sk->sk_state == TCP_CLOSING)
>>  			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
>> -		 * vsock_connected_table. We keep the binding and the transport
>> -		 * assigned.
>>  		 */
>>  		if (signal_pending(current) || timeout == 0) {
>>  			err = timeout == 0 ? -ETIMEDOUT : sock_intr_errno(timeout);
>> @@ -1875,7 +1872,8 @@ static int vsock_connect(struct socket *sock, struct sockaddr_unsized *addr,
>>  	}
>>  
>>  	err = sock_error(sk);
>> -	if (err) {
>> +	if (err &&
>> +	    sk->sk_state != TCP_ESTABLISHED && sk->sk_state != TCP_CLOSING) {
> 
> If the OP_RESPONSE + a blast of OP_RW that pushes past the buffer limit
> arrives while we were scheduled out, we end up with sk_err = ENOBUFS
> here. Then I guess connect() returns an error, but sk_state/sock->state
> is still TCP_ESTABLISHED and SS_CONNECTED. If the user sees the error
> and tries connect() again, they just get -EISCONN back. Maybe the
> sock_error() needs to be moved within the conditional here, and then let
> subsequent calls return the error to the user (it looks sendmsg() at
> least will report it faithfully, but not sure about recvmsg() or the
> others).

Right, I share the concern about ENOBUFS. I'll move sock_error() in v2.

thanks,
Michal