Re: [PATCH v4] vsock: use sock_error() to consume sk_err after a failed connect

On Wed, Aug 05, 2026 at 06:29:36PM +0800, Nguyen Dinh Phi [SG] wrote:
>On 5/8/26 16:34, Stefano Garzarella wrote:
>>On Tue, Aug 04, 2026 at 09:52:36PM +0800, [email protected] wrote:
>>>From: Nguyen Dinh Phi <[email protected]>
>>>
>>>Syzbot reported an issue which can be reproduced with these steps:
>>>
>>>  r0 = socket(AF_VSOCK, SOCK_STREAM, 0)
>>>  bind(r0, {VMADDR_CID_ANY, PORT})
>>>  connect(r0, {VMADDR_CID_LOCAL, PORT})   -> -1, EPROTO  (self-connect)
>>>  listen(r0, backlog)                     -> 0
>>>  r1 = socket(AF_VSOCK, SOCK_STREAM, 0)
>>>  connect(r1, {VMADDR_CID_LOCAL, PORT})   -> 0
>>>  accept(r0)                              -> -1, EPROTO  (stale sk_err)
>>>
>>>Basically, it creates a socket (r0) and triggers a self-connect after
>>>binding it. This self-connect fails with EPROTO because it loops back to
>>>r0 while the socket is still in the TCP_SYN_SENT state, causing it to be
>>>incorrectly dispatched to the connecting-client path. The unexpected
>>>packet type encountered there sets sk_err to EPROTO.
>>>
>>>After that, it invokes a listen() call on the same socket. This listen()
>>>call succeeds because the kernel's listening path never inspects or
>>>clears sk_err. Then, a new socket (r1) is created as a normal client and
>>>connects to r0. However, vsock_accept() rejects this incoming connection
>>>because the listener's sk_err still holds the EPROTO error from the
>>>earlier failed self-connect.
>>>
>>>This rejection causes the child socket created for r1's connection to
>>>never be freed on virtio or hyperv transports; only the VMCI transport
>>>implements pending_work to revisit and clean up a rejected socket.
>>>
>>>Fix the issue in blocking connect() by using sock_error() to read the
>>>sk_err to prevent the rejection branch from occurring in this scenario.
>>>
>>>sock_error() atomically reads and clears sk_err, ensuring the error is
>>>consumed when vsock_connect() returns and cannot affect subsequent
>>>operations on the same socket. This matches the established pattern
>>>used by other protocol connect() implementations in the network
>>>stack like __inet_stream_connect(), tipc_wait_for_connect()...
>>>
>>>For non-blocking connection, vsock_connect_timeout() may set
>>>sk->sk_err after vsock_connect() has returned. To handle it, we also
>>>remove the sk_err checks from vsock_accept(). Nothing in vsock sets
>>>sk_err on a listening socket, so accept() has no reason to inspect it
>>>at all.
>>>
>>>Reported-by: [email protected]
>>>Closes: https://syzkaller.appspot.com/bug?extid=1b2c9c4a0f8708082678
>>>Fixes: d021c344051af ("VSOCK: Introduce VM Sockets")
>>>Suggested-by: Michal Luczaj <[email protected]>
>>>Signed-off-by: Nguyen Dinh Phi <[email protected]>
>>>Tested-by: Wupeng Ma <[email protected]>
>>>---
>>>V2: Add reproducer steps to commit message.
>>>V3: Fix truncated title and add annotations to reproducer steps.
>>>V4: Remove sk_err checks from vsock_accept()
>>>
>>>net/vmw_vsock/af_vsock.c | 13 ++++---------
>>>1 file changed, 4 insertions(+), 9 deletions(-)
>>>
>>>diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
>>>index 622dbd046799..594fe27d2ebe 100644
>>>--- a/net/vmw_vsock/af_vsock.c
>>>+++ b/net/vmw_vsock/af_vsock.c
>>>@@ -1847,12 +1847,10 @@ static int vsock_connect(struct socket *sock, 
>>>struct sockaddr_unsized *addr,
>>>        prepare_to_wait(sk_sleep(sk), &wait, TASK_INTERRUPTIBLE);
>>>    }
>>>
>>>-    if (sk->sk_err) {
>>>-        err = -sk->sk_err;
>>>+    err = sock_error(sk);
>>>+    if (err) {
>>>        sk->sk_state = TCP_CLOSE;
>>>        sock->state = SS_UNCONNECTED;
>>>-    } else {
>>>-        err = 0;
>>>    }
>>>
>>>out_wait:
>>>@@ -1893,7 +1891,7 @@ static int vsock_accept(struct socket *sock, 
>>>struct socket *newsock,
>>>    timeout = sock_rcvtimeo(listener, arg->flags & O_NONBLOCK);
>>>
>>>    while ((connected = vsock_dequeue_accept(listener)) == NULL &&
>>>-           listener->sk_err == 0 && timeout != 0) {
>>>+           timeout != 0) {
>>>        prepare_to_wait(sk_sleep(listener), &wait, TASK_INTERRUPTIBLE);
>>>        release_sock(listener);
>>>        timeout = schedule_timeout(timeout);
>>>@@ -1906,11 +1904,8 @@ static int vsock_accept(struct socket 
>>>*sock, struct socket *newsock,
>>>        }
>>>    }
>>>
>>>-    if (listener->sk_err) {
>>>-        err = -listener->sk_err;
>>>-    } else if (!connected) {
>>>+    if (!connected)
>>>        err = -EAGAIN;
>>>-    }
>>>
>>>    if (connected) {
>>
>>Can this become an `} else {` ?
>>
>>Or just add a `goto out` when setting `err = -EAGAIN`.
>>
>
>I will update it.
>
>>>        sk_acceptq_removed(listener);
>>
>>         lock_sock_nested(connected, SINGLE_DEPTH_NESTING);
>>         vconnected = vsock_sk(connected);
>>
>>         /* If the listener socket has received an error, then we should
>>          * reject this socket and return.  Note that we simply mark the
>>          * socket rejected, drop our reference, and let the cleanup
>>          * function handle the cleanup; the fact that we found it in
>>          * the listener's accept queue guarantees that the cleanup
>>          * function hasn't run yet.
>>          */
>>         if (err) {
>>             vconnected->rejected = true;
>>         } else {
>>
>>
>>Should we update this comment too and maybe remove the `if (err)` at
>>all. With that change I guess `rejected` is never set at the end and
>>maybe we can remove it at all from `struct vsock_sock`.
>>
>>Looking at commit d021c344051a ("VSOCK: Introduce VM Sockets") where
>>`rejected` was introduced, I can't see any path where sk_err is set on
>>a listener socket, so I guess that path was dead since the beginning.
>>
>
>That seems true, let me verify it.
>
>>So now I'm thinking if it's better to split in 2 patches (both with the
>>same Fixes tag):
>>- Patch 1: "vsock: remove stale sk_err checks from vsock_accept()"
>>   Where we can also remove `rejected` since it's never set to true since
>>   the beginning
>>- Patch 2: "vsock: use sock_error() to consume sk_err after a failed
>>   connect"
>>
>>WDYT?
>>
>
>Yes, I felt the same when I was writing the commit message, but honestly I
>didn't know that I could split it into a series when sending the new 
>version.
>
>So, it may contain 3 patches, if the rejected flag can be removed from 
>vsock_sock.

Yeah, or including the rejected flag removal in the patch 1 I suggested.

Up to you.

Thanks,
Stefano