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

[Michal Luczaj <mhal@rbox.co>]

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