On Wed, Aug 19, 2026 at 10:54:38AM +0200, Stefano Garzarella wrote:
> On Mon, Aug 17, 2026 at 12:29:11AM +0200, Michal Luczaj wrote:
> > On 8/14/26 03:45, Hyunwoo Kim wrote:
> > > On Thu, Aug 13, 2026 at 11:42:17AM +0200, Michal Luczaj wrote:
> > > > On 8/12/26 22:13, Hyunwoo Kim wrote:
> > > > > commit 002541ef650b ("vsock: Ignore signal/timeout on connect() if
> > > > > already established") stopped connect() from resetting an established
> > > > > socket. The check only looks at whether sk_state is TCP_ESTABLISHED at
> > > > > that moment, and the state can change while connect() sleeps.
> > > >
> > > > I guess this makes my fix incomplete. "Fixes: 002541ef650b"?
> > >
> > > It is incomplete, yes. But this has been triggerable since d021c344051a,
> > > so
> > > I'd keep Fixes: d021c344051a.
> >
> > OK, I get it.
> >
> > > > > A peer RST moves the socket to TCP_CLOSING, and it is not removed from
> > > > > vsock_connected_table on that path. connect() then wakes up, fails the
> > > > > check, and resets a socket that had actually connected to TCP_CLOSE
> > > > > and
> > > > > SS_UNCONNECTED.
> > > >
> > > > Thanks for the details. Do I get it right: connect() misses the fact
> > > > that
> > > > socket might have already transitioned TCP_ESTABLISHED -> TCP_CLOSING
> > > > during schedule_timeout()?
> > >
> > > Yes, that's it.
> > >
> > > >
> > > > How about:
> > > >
> > > > diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
> > > > index 622dbd046799..39c42ef016c3 100644
> > > > --- a/net/vmw_vsock/af_vsock.c
> > > > +++ b/net/vmw_vsock/af_vsock.c
> > > > @@ -1807,15 +1807,18 @@ static int vsock_connect(struct socket *sock,
> > > > struct sockaddr_unsized *addr,
> > > > timeout = schedule_timeout(timeout);
> > > > lock_sock(sk);
> > > >
> > > > - /* Connection established. Whatever happens to socket
> > > > once we
> > > > + /* Connection (has been) established. Whatever happens
> > > > to socket once we
> > > > * release it, that's not connect()'s concern. No need
> > > > to go
> > > > * into signal and timeout handling. Call it a day.
> > > > *
> > > > * Note that allowing to "reset" an already established
> > > > socket
> > > > * here is racy and insecure.
> > > > */
> > > > - if (sk->sk_state == TCP_ESTABLISHED)
> > > > - break;
> > > > + if (sk->sk_state == TCP_ESTABLISHED ||
> > > > + sk->sk_state == TCP_CLOSING) {
> > > > + err = -sk->sk_err;
> > > > + goto out_wait;
> > > > + }
> > > >
> > > > /* If connection was _not_ established and a
> > > > signal/timeout came
> > > > * to be, we want the socket's state reset. User space
> > > > may want
> > > >
> > > > ?
> > >
> > > Yes, I like it better than mine. I confirmed it fixes the issue.
> >
> > Great, thanks.
> >
> > > If you don't mind, would you take the patch from here?
> >
> > Sure, no problem.
> >
> > Stefano, does this look good to you?
>
> Yep, thanks for helping here!
>
> My only doubt is if it makes sense to leave the `break` there, and add a
> similar check before resetting the socket, I mean something like this:
>
> err = sock_error(sk);
> if (err && sk->sk_state != TCP_ESTABLISHED &&
> sk->sk_state != TCP_CLOSING)) {
> sk->sk_state = TCP_CLOSE;
> sock->state = SS_UNCONNECTED;
> }
>
> Just to be a bit more defensive, but I don't have a strong opinion, your
> version is also fine.
>
> > And should any sk_err be consumed
> > here, too? (`err = sock_error(sk)` instead of `err = -sk->sk_err`)
>
> I'd stay with sock_error() to consume the error if it makes sense also for
> you.
>
> Thanks,
> Stefano
>
Gentle ping. I'd appreciate an update on where this stands.
Best regards,
Hyunwoo Kim
