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

Stefano Garzarella <[email protected]> Thu, 23 Jul 2026 10:24:05 +0200
Newsgroups dev.linux.lists.virtualization,org.kernel.vger.linux-kernel,org.kernel.vger.netdev
Message-ID <amHN9mOGj7oMak4-@sgarzare-redhat>
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