Re: [PATCH net v2 3/5] vsock: Enforce no-transport invariant for TCP_LISTEN sockets

[Stefano Garzarella]

On Tue, Sep 22, 2026 at 03:18:13PM +0200, Michal Luczaj wrote:
> On 9/16/26 14:30, Stefano Garzarella wrote:
> > On Tue, Sep 15, 2026 at 03:15:14PM +0200, Michal Luczaj wrote:
> > > A non-blocking connect() running in parallel with a blocking connect(),
> > > combined with a racy listen() that hits right after a connect timeout:
> > > TCP_SYN_SENT -> TCP_CLOSE -> TCP_LISTEN, while the connect() loop is still
> > > in progress.
> > >
> > > Enforce the invariant. Prevent a socket from becoming a listener after
> > > acquiring a transport.
> >
> > We should improve this comment; it's not entirely clear to me, TBH.
>
> The race I was thinking about:
>
> sk is CLOSE UNCONNECTED
> non-blocking connect():
>  sk := SYN_SENT CONNECTING
>  enqueue vsock_connect_timeout()
>                                   blocking connect():
>                                     release_sock()
>                                     schedule_timeout()
> vsock_connect_timeout():
>  sk := CLOSE UNCONNECTED
> listen():
>  sk := LISTEN UNCONNECTED
>                                     lock_sock()
>                                     sk is TCP_LISTEN UNCONNECTED
>
> It's not really critical (blocking connect() just timeouts), but I thought
> the invariant should be enforced once and for all.

I see, would it better to do this change in net-next?

> > > @@ -1973,13 +1973,13 @@ static int vsock_listen(struct socket *sock, int backlog)
> > > 		goto out;
> > > 	}
> > >
> > > -	if (sock->state != SS_UNCONNECTED) {
> > > +	vsk = vsock_sk(sk);
> > > +
> > > +	if (sock->state != SS_UNCONNECTED || vsk->transport) {
> >
> > Are we changing the behavior when an error occurs?
> >
> > If we call `connect()` on a socket (with no others running in parallel),
> > it fails, and then when we call `listen()`, it now fails, whereas before
> > it didn't. Can this happen? Is that what we want?
>
> Ah, true, I didn't consider that. So yeah, we'd changing the behaviour.
>
> > If so, we should mention it at least in the commit description; if not,
> > perhaps we should unassign the transport in the `connect` call.
>
> Do you mean immediately un-assign on every transition from SYN_SENT to
> CLOSE (failure, timeout, signal)? Then we could also drop the re-assign
> logic. I think that's a nice idea.

yeah, that!

> ---
>
> I've addressed all your other comments for v2 and went through Ashiko's
> reports (side effects of lockless peer_shutdown write, imperfect
> no-transport TCP_LISTENER enforcement). I've decided to try the
> eager-unassign approach. I think/hope this way we sidestep the lockless
> writes and enforce the invariant without breaking the API, while fixing the
> bugs.
>
> This should probably be RFC, but I'm posting as v3[1] so netdev's LLM can
> have a go (too). Hope I'm not breaking any workflow. Let me know what you
> think.

I think you can add RFC also on a v3 patch, just to make it clear you 
are not sure it's ready to be merged.

That said, thanks for that :-) I'll take a look today or next week 
because I'm off tomorrow.

I'm just worried it's becoming too big for net. Anyway, I'll comment 
there.

Thanks,
Stefano