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

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

[phind.uet]

From: Nguyen Dinh Phi <phind.uet@gmail.com>

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: syzbot+1b2c9c4a0f8708082678@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=1b2c9c4a0f8708082678
Fixes: d021c344051af ("VSOCK: Introduce VM Sockets")
Suggested-by: Michal Luczaj <mhal@rbox.co>
Signed-off-by: Nguyen Dinh Phi <phind.uet@gmail.com>
Tested-by: Wupeng Ma <mawupeng1@huawei.com>
---
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) {
 		sk_acceptq_removed(listener);
-- 
2.43.0

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

[Stefano Garzarella]

On Tue, Aug 04, 2026 at 09:52:36PM +0800, phind.uet@gmail.com wrote:
>From: Nguyen Dinh Phi <phind.uet@gmail.com>
>
>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: syzbot+1b2c9c4a0f8708082678@syzkaller.appspotmail.com
>Closes: https://syzkaller.appspot.com/bug?extid=1b2c9c4a0f8708082678
>Fixes: d021c344051af ("VSOCK: Introduce VM Sockets")
>Suggested-by: Michal Luczaj <mhal@rbox.co>
>Signed-off-by: Nguyen Dinh Phi <phind.uet@gmail.com>
>Tested-by: Wupeng Ma <mawupeng1@huawei.com>
>---
>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`.

> 		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.

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?

Thanks,
Stefano

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

[Nguyen Dinh Phi [SG]]

On 5/8/26 16:34, Stefano Garzarella wrote:
> On Tue, Aug 04, 2026 at 09:52:36PM +0800, phind.uet@gmail.com wrote:
>> From: Nguyen Dinh Phi <phind.uet@gmail.com>
>>
>> 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: syzbot+1b2c9c4a0f8708082678@syzkaller.appspotmail.com
>> Closes: https://syzkaller.appspot.com/bug?extid=1b2c9c4a0f8708082678
>> Fixes: d021c344051af ("VSOCK: Introduce VM Sockets")
>> Suggested-by: Michal Luczaj <mhal@rbox.co>
>> Signed-off-by: Nguyen Dinh Phi <phind.uet@gmail.com>
>> Tested-by: Wupeng Ma <mawupeng1@huawei.com>
>> ---
>> 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.

Thanks,
Phi