net/vmw_vsock/af_vsock.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)
After vsock_connect() exits the wait loop due to sk->sk_err being
set, the error was read but not cleared. This left sk->sk_err set
for subsequent operations.
Switch to sock_error() which atomically reads and clears sk->sk_err,
so the error is consumed when returned.
Signed-off-by: Nguyen Dinh Phi <phind.uet@gmail.com>
Reported-by: syzbot+1b2c9c4a0f8708082678@syzkaller.appspotmail.com
---
net/vmw_vsock/af_vsock.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
index 622dbd046799..43eddc33ed12 100644
--- a/net/vmw_vsock/af_vsock.c
+++ b/net/vmw_vsock/af_vsock.c
@@ -1847,14 +1847,11 @@ 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:
finish_wait(sk_sleep(sk), &wait);
out:
--
2.53.0On Mon, Jul 20, 2026 at 05:57:47AM +0800, Nguyen Dinh Phi wrote:
>After vsock_connect() exits the wait loop due to sk->sk_err being
>set, the error was read but not cleared. This left sk->sk_err set
>for subsequent operations.
So, is this a fix? If yes, we should put a Fixes tag.
Also, can you describe how to trigger the issue?
Because I see this in vsock_connect(), so I thought it was in some way
already handled:
/* sk_err might have been set as a result of an earlier
* (failed) connect attempt.
*/
sk->sk_err = 0;
>Switch to sock_error() which atomically reads and clears sk->sk_err,
>so the error is consumed when returned.
>
>Signed-off-by: Nguyen Dinh Phi <phind.uet@gmail.com>
>Reported-by: syzbot+1b2c9c4a0f8708082678@syzkaller.appspotmail.com
Can you explain how this patch fixes that issue?
(this should be the first information to be put in the commit message
IMHO)
I'd like to understand better if this is a fix of real bug or just an
improvement to the code (which is fine by me).
Thanks,
Stefano
>---
> net/vmw_vsock/af_vsock.c | 7 ++-----
> 1 file changed, 2 insertions(+), 5 deletions(-)
>
>diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
>index 622dbd046799..43eddc33ed12 100644
>--- a/net/vmw_vsock/af_vsock.c
>+++ b/net/vmw_vsock/af_vsock.c
>@@ -1847,14 +1847,11 @@ 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:
> finish_wait(sk_sleep(sk), &wait);
> out:
>--
>2.53.0
>
On 7/20/2026 4:17 PM, Stefano Garzarella wrote:
> On Mon, Jul 20, 2026 at 05:57:47AM +0800, Nguyen Dinh Phi wrote:
>> After vsock_connect() exits the wait loop due to sk->sk_err being
>> set, the error was read but not cleared. This left sk->sk_err set
>> for subsequent operations.
>
> So, is this a fix? If yes, we should put a Fixes tag.
>
> Also, can you describe how to trigger the issue?
>
> Because I see this in vsock_connect(), so I thought it was in some way
> already handled:
>
> /* sk_err might have been set as a result of an earlier
> * (failed) connect attempt.
> */
> sk->sk_err = 0;
>
This only handles the case where the function following the failed
connect is another connect() call.
>> Switch to sock_error() which atomically reads and clears sk->sk_err,
>> so the error is consumed when returned.
>>
>> Signed-off-by: Nguyen Dinh Phi <phind.uet@gmail.com>
>> Reported-by: syzbot+1b2c9c4a0f8708082678@syzkaller.appspotmail.com
>
> Can you explain how this patch fixes that issue?
> (this should be the first information to be put in the commit message IMHO)
>
> I'd like to understand better if this is a fix of real bug or just an
> improvement to the code (which is fine by me).
>
> Thanks,
> Stefano
>
Here are the steps of the syzkaller reproducer:
r0 = socket(AF_VSOCK, SOCK_STREAM, 0)
bind(r0, {VMADDR_CID_ANY, PORT})
connect(r0, {VMADDR_CID_LOCAL, PORT})
listen(r0, backlog)
r1 = socket(AF_VSOCK, SOCK_STREAM, 0)
connect(r1, {VMADDR_CID_LOCAL, PORT})
connect(r0 -> self) -> -1, EPROTO
listen(r0) -> 0
connect(r1 -> r0) -> 0
accept(r0) -> -1, EPROTO
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
This patch will prevent the rejection branch to occur in this scenario.
I think we might schedule the cleanup worker to run in the rejection
path for these transports as well.
Thanks,
Phi
On Tue, Jul 21, 2026 at 01:34:03AM +0800, Phi Nguyen wrote:
>On 7/20/2026 4:17 PM, Stefano Garzarella wrote:
>>On Mon, Jul 20, 2026 at 05:57:47AM +0800, Nguyen Dinh Phi wrote:
>>>After vsock_connect() exits the wait loop due to sk->sk_err being
>>>set, the error was read but not cleared. This left sk->sk_err set
>>>for subsequent operations.
>>
>>So, is this a fix? If yes, we should put a Fixes tag.
>>
>>Also, can you describe how to trigger the issue?
>>
>>Because I see this in vsock_connect(), so I thought it was in some
>>way already handled:
>>
>> /* sk_err might have been set as a result of an earlier
>> * (failed) connect attempt.
>> */
>> sk->sk_err = 0;
>>
>This only handles the case where the function following the failed
>connect is another connect() call.
So, can we remove that with this patch, or better to leave as defensive
action?
>
>>>Switch to sock_error() which atomically reads and clears sk->sk_err,
>>>so the error is consumed when returned.
>>>
>>>Signed-off-by: Nguyen Dinh Phi <phind.uet@gmail.com>
>>>Reported-by: syzbot+1b2c9c4a0f8708082678@syzkaller.appspotmail.com
>>
>>Can you explain how this patch fixes that issue?
>>(this should be the first information to be put in the commit message IMHO)
>>
>>I'd like to understand better if this is a fix of real bug or just
>>an improvement to the code (which is fine by me).
>>
>>Thanks,
>>Stefano
>>
>Here are the steps of the syzkaller reproducer:
>
> r0 = socket(AF_VSOCK, SOCK_STREAM, 0)
>
> bind(r0, {VMADDR_CID_ANY, PORT})
>
> connect(r0, {VMADDR_CID_LOCAL, PORT})
>
> listen(r0, backlog)
>
> r1 = socket(AF_VSOCK, SOCK_STREAM, 0)
>
> connect(r1, {VMADDR_CID_LOCAL, PORT})
>
> connect(r0 -> self) -> -1, EPROTO
>
> listen(r0) -> 0
>
> connect(r1 -> r0) -> 0
>
> accept(r0) -> -1, EPROTO
>
>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
>This patch will prevent the rejection branch to occur in this scenario.
Okay, get it now, thanks! Please include a summary of this in the commit
description.
I understand that this resolves syzbot's specific test case, but it
would be best to handle rejected sockets more effectively in af_vsock.c
rather than in the transport layers (if possible). In any case, this can
be done in another patch.
>
>I think we might schedule the cleanup worker to run in the rejection
>path for these transports as well.
Yeah, we need to handle that part better, I think it's a leftover when
we generalized AF_VSOCK to support more transport than vmci.
Indeed this part is a bit confusing:
/* 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 {
Would be nice to handle everything in af_vsock.c in some way.
In conclusion, the patch LGTM, but please expand the commit description,
add the Fixes tag, and target the net tree in the v2.
Thanks,
Stefano
On 22/7/26 15:55, Stefano Garzarella wrote:
> On Tue, Jul 21, 2026 at 01:34:03AM +0800, Phi Nguyen wrote:
>> On 7/20/2026 4:17 PM, Stefano Garzarella wrote:
>>> On Mon, Jul 20, 2026 at 05:57:47AM +0800, Nguyen Dinh Phi wrote:
>>>> After vsock_connect() exits the wait loop due to sk->sk_err being
>>>> set, the error was read but not cleared. This left sk->sk_err set
>>>> for subsequent operations.
>>>
>>> So, is this a fix? If yes, we should put a Fixes tag.
>>>
>>> Also, can you describe how to trigger the issue?
>>>
>>> Because I see this in vsock_connect(), so I thought it was in some
>>> way already handled:
>>>
>>> /* sk_err might have been set as a result of an earlier
>>> * (failed) connect attempt.
>>> */
>>> sk->sk_err = 0;
>>>
>> This only handles the case where the function following the failed
>> connect is another connect() call.
>
> So, can we remove that with this patch, or better to leave as defensive
> action?
>
I prefer to keep it here as defensive action
>>
>>>> Switch to sock_error() which atomically reads and clears sk->sk_err,
>>>> so the error is consumed when returned.
>>>>
>>>> Signed-off-by: Nguyen Dinh Phi <phind.uet@gmail.com>
>>>> Reported-by: syzbot+1b2c9c4a0f8708082678@syzkaller.appspotmail.com
>>>
>>> Can you explain how this patch fixes that issue?
>>> (this should be the first information to be put in the commit message
>>> IMHO)
>>>
>>> I'd like to understand better if this is a fix of real bug or just an
>>> improvement to the code (which is fine by me).
>>>
>>> Thanks,
>>> Stefano
>>>
>> Here are the steps of the syzkaller reproducer:
>>
>> r0 = socket(AF_VSOCK, SOCK_STREAM, 0)
>>
>> bind(r0, {VMADDR_CID_ANY, PORT})
>>
>> connect(r0, {VMADDR_CID_LOCAL, PORT})
>>
>> listen(r0, backlog)
>>
>> r1 = socket(AF_VSOCK, SOCK_STREAM, 0)
>>
>> connect(r1, {VMADDR_CID_LOCAL, PORT})
>>
>> connect(r0 -> self) -> -1, EPROTO
>>
>> listen(r0) -> 0
>>
>> connect(r1 -> r0) -> 0
>>
>> accept(r0) -> -1, EPROTO
>>
>> 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
>> This patch will prevent the rejection branch to occur in this scenario.
>
> Okay, get it now, thanks! Please include a summary of this in the commit
> description.
>
> I understand that this resolves syzbot's specific test case, but it
> would be best to handle rejected sockets more effectively in af_vsock.c
> rather than in the transport layers (if possible). In any case, this can
> be done in another patch.
>
>>
>> I think we might schedule the cleanup worker to run in the rejection
>> path for these transports as well.
>
> Yeah, we need to handle that part better, I think it's a leftover when
> we generalized AF_VSOCK to support more transport than vmci.
>
> Indeed this part is a bit confusing:
>
> /* 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 {
>
>
> Would be nice to handle everything in af_vsock.c in some way.
>
> In conclusion, the patch LGTM, but please expand the commit description,
> add the Fixes tag, and target the net tree in the v2.
I'll send a v2 with an expanded commit description and the Fixes tag.
Regarding the rejected socket cleanup, I agree it would be cleaner to
handle it entirely in af_vsock.c. I'll look at that as a follow-up.
Thanks,
Phi
On 7/23/26 06:14, Nguyen Dinh Phi [SG] wrote: > On 22/7/26 15:55, Stefano Garzarella wrote: >> On Tue, Jul 21, 2026 at 01:34:03AM +0800, Phi Nguyen wrote: >>> On 7/20/2026 4:17 PM, Stefano Garzarella wrote: >>>> On Mon, Jul 20, 2026 at 05:57:47AM +0800, Nguyen Dinh Phi wrote: >>>>> After vsock_connect() exits the wait loop due to sk->sk_err being >>>>> set, the error was read but not cleared. This left sk->sk_err set >>>>> for subsequent operations. >>>> >>>> So, is this a fix? If yes, we should put a Fixes tag. >>>> >>>> Also, can you describe how to trigger the issue? >>>> >>>> Because I see this in vsock_connect(), so I thought it was in some >>>> way already handled: >>>> >>>> /* sk_err might have been set as a result of an earlier >>>> * (failed) connect attempt. >>>> */ >>>> sk->sk_err = 0; >>>> >>> This only handles the case where the function following the failed >>> connect is another connect() call. >> >> So, can we remove that with this patch, or better to leave as defensive >> action? >> > > I prefer to keep it here as defensive action Is changing how vsock_poll() behaves intended? I.e. if sk_err should be kept after a failed connect(), what about `sk->sk_err = 0;` in vsock_listen() instead? >> ... >> Yeah, we need to handle that part better, I think it's a leftover when >> we generalized AF_VSOCK to support more transport than vmci. Speaking of leftovers, I have trouble understanding where does vsock set sk_err on listener sockets anyway. If it doesn't, why vsock_accept() checks for it? thanks, Michal
On Thu, Jul 23, 2026 at 08:00:31AM +0200, Michal Luczaj wrote: >On 7/23/26 06:14, Nguyen Dinh Phi [SG] wrote: >> On 22/7/26 15:55, Stefano Garzarella wrote: >>> On Tue, Jul 21, 2026 at 01:34:03AM +0800, Phi Nguyen wrote: >>>> On 7/20/2026 4:17 PM, Stefano Garzarella wrote: >>>>> On Mon, Jul 20, 2026 at 05:57:47AM +0800, Nguyen Dinh Phi wrote: >>>>>> After vsock_connect() exits the wait loop due to sk->sk_err being >>>>>> set, the error was read but not cleared. This left sk->sk_err set >>>>>> for subsequent operations. >>>>> >>>>> So, is this a fix? If yes, we should put a Fixes tag. >>>>> >>>>> Also, can you describe how to trigger the issue? >>>>> >>>>> Because I see this in vsock_connect(), so I thought it was in some >>>>> way already handled: >>>>> >>>>> /* sk_err might have been set as a result of an earlier >>>>> * (failed) connect attempt. >>>>> */ >>>>> sk->sk_err = 0; >>>>> >>>> This only handles the case where the function following the failed >>>> connect is another connect() call. >>> >>> So, can we remove that with this patch, or better to leave as defensive >>> action? >>> >> >> I prefer to keep it here as defensive action > >Is changing how vsock_poll() behaves intended? Good point, but IIUC __inet_stream_connect() is also using consuming the error with sock_error(). > >I.e. if sk_err should be kept after a failed connect(), what about >`sk->sk_err = 0;` in vsock_listen() instead? Yeah, maybe this is a bit less invasive. > >>> ... >>> Yeah, we need to handle that part better, I think it's a leftover when >>> we generalized AF_VSOCK to support more transport than vmci. > >Speaking of leftovers, I have trouble understanding where does vsock set >sk_err on listener sockets anyway. If it doesn't, why vsock_accept() checks >for it? I can't also see where it can be set TBH. Should we remove it ? Thanks, Stefano
On 23/7/26 16:24, Stefano Garzarella wrote: > On Thu, Jul 23, 2026 at 08:00:31AM +0200, Michal Luczaj wrote: >> On 7/23/26 06:14, Nguyen Dinh Phi [SG] wrote: >>> On 22/7/26 15:55, Stefano Garzarella wrote: >>>> On Tue, Jul 21, 2026 at 01:34:03AM +0800, Phi Nguyen wrote: >>>>> On 7/20/2026 4:17 PM, Stefano Garzarella wrote: >>>>>> On Mon, Jul 20, 2026 at 05:57:47AM +0800, Nguyen Dinh Phi wrote: >>>>>>> After vsock_connect() exits the wait loop due to sk->sk_err being >>>>>>> set, the error was read but not cleared. This left sk->sk_err set >>>>>>> for subsequent operations. >>>>>> >>>>>> So, is this a fix? If yes, we should put a Fixes tag. >>>>>> >>>>>> Also, can you describe how to trigger the issue? >>>>>> >>>>>> Because I see this in vsock_connect(), so I thought it was in some >>>>>> way already handled: >>>>>> >>>>>> /* sk_err might have been set as a result of an earlier >>>>>> * (failed) connect attempt. >>>>>> */ >>>>>> sk->sk_err = 0; >>>>>> >>>>> This only handles the case where the function following the failed >>>>> connect is another connect() call. >>>> >>>> So, can we remove that with this patch, or better to leave as defensive >>>> action? >>>> >>> >>> I prefer to keep it here as defensive action >> >> Is changing how vsock_poll() behaves intended? > I think connect() already delivered the error directly to the caller when it returned -- there's no reason for poll() to keep reporting that same, already-handled error afterward. > Good point, but IIUC __inet_stream_connect() is also using consuming the > error with sock_error(). > >> >> I.e. if sk_err should be kept after a failed connect(), what about >> `sk->sk_err = 0;` in vsock_listen() instead? > > Yeah, maybe this is a bit less invasive. > yes, __inet_stream_connect() does it, and actually, other protocols like Bluetooth, TIPC, x25... are also consume sk_err with sock_error() after their wait loop. I think it is an established pattern, so I'd rather keep sock_error() in vsock_connect() than move it to vsock_listen(). vsock_listen() would only protect the listen() path; I see other places in af_vsock.c use this sk_err. >> >>>> ... >>>> Yeah, we need to handle that part better, I think it's a leftover when >>>> we generalized AF_VSOCK to support more transport than vmci. >> >> Speaking of leftovers, I have trouble understanding where does vsock set >> sk_err on listener sockets anyway. If it doesn't, why vsock_accept() >> checks >> for it? > > I can't also see where it can be set TBH. Should we remove it ? I couldn't find it for listener side too. Thanks, Phi.
On 7/23/26 12:26, Nguyen Dinh Phi [SG] wrote: >>>>> ... >>>>> Yeah, we need to handle that part better, I think it's a leftover when >>>>> we generalized AF_VSOCK to support more transport than vmci. >>> >>> Speaking of leftovers, I have trouble understanding where does vsock set >>> sk_err on listener sockets anyway. If it doesn't, why vsock_accept() >>> checks for it? >> >> I can't also see where it can be set TBH. Should we remove it ? > > I couldn't find it for listener side too. Removing sk_err handling from vsock_accept() solves the problem, right? thanks, Michal
On 24/7/26 05:43, Michal Luczaj wrote: > On 7/23/26 12:26, Nguyen Dinh Phi [SG] wrote: >>>>>> ... >>>>>> Yeah, we need to handle that part better, I think it's a leftover when >>>>>> we generalized AF_VSOCK to support more transport than vmci. >>>> >>>> Speaking of leftovers, I have trouble understanding where does vsock set >>>> sk_err on listener sockets anyway. If it doesn't, why vsock_accept() >>>> checks for it? >>> >>> I can't also see where it can be set TBH. Should we remove it ? >> >> I couldn't find it for listener side too. > > Removing sk_err handling from vsock_accept() solves the problem, right? > > thanks, > Michal Yes, confirmed, removing sk_err checks from vsock_accept() does solve the problem. Thanks, Phi
© 2016 - 2026 Red Hat, Inc.