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 <[email protected]>
>> Link: https://lore.kernel.org/netdev/anzT1fREOSyHT99k@v4bel/
>> Signed-off-by: Michal Luczaj <[email protected]>
>> ---
>> 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
