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

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.