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

[Stefano Garzarella <sgarzare@redhat.com>]

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