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

[Nguyen Dinh Phi [SG]]

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.