Re: [PATCH] vsock: use sock_error() to consume sk_err after connect timeout

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 <[email protected]>
>>>> Reported-by: [email protected]
>>>
>>> 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