Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:
Critical: 0 · High: 1 · Medium: 0 · Low: 0
- [High] Incomplete race fix: `vsk->transport != NULL` is not a reliable
sentinel for "a connect() waiter is still in flight", so the exact bug…
--
vsock: Enforce no-transport invariant for TCP_LISTEN sockets
The patch makes vsock_listen() reject sockets that already have a transport
assigned, so that a socket cannot become a listener while a connect() wait
loop is still running on it.
> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
> index f840498b58afb..499e902becfad 100644
> --- a/net/vmw_vsock/af_vsock.c
> +++ b/net/vmw_vsock/af_vsock.c
> @@ -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) {
> err = -EINVAL;
> goto out;
> }
[Severity: High]
Is vsk->transport a reliable indicator that no connect() waiter is in
flight?
vsock_assign_transport() drops the old transport before the new one is
validated and initialized:
net/vmw_vsock/af_vsock.c:vsock_assign_transport() {
...
if (vsk->transport) {
vsk->transport->release(vsk);
vsock_deassign_transport(vsk); /* vsk->transport = NULL */
...
sk->sk_state = TCP_CLOSE;
...
}
if (sk->sk_type == SOCK_SEQPACKET) {
if (!new_transport->seqpacket_allow ||
!new_transport->seqpacket_allow(vsk, remote_cid)) {
module_put(new_transport->module);
return -ESOCKTNOSUPPORT;
}
}
ret = new_transport->init(vsk, psk);
if (ret) {
module_put(new_transport->module);
return ret;
}
vsk->transport = new_transport;
...
}
Both of those failure returns happen after vsock_deassign_transport(), so
the socket is left with vsk->transport == NULL while it is still alive and
still bound (vsock_remove_sock() keeps the binding unless SOCK_DEAD is set).
The seqpacket path looks reachable without any allocation failure: only
virtio_transport.c and vsock_loopback.c define .seqpacket_allow, so a
SOCK_SEQPACKET reassignment that resolves to vmci returns -ESOCKTNOSUPPORT.
The init path is reachable via the kzalloc in
virtio_transport_do_socket_init().
Can the following interleaving still put a listener into TCP_CLOSE?
Thread A: blocking connect(), transport T1 assigned, TCP_SYN_SENT,
sock->state = SS_CONNECTING, enters the wait loop
Thread B: connect() on the same fd hits case SS_CONNECTING (-EALREADY)
and joins the same wait loop
Thread A: timeout fires and runs the signal/timeout branch of
vsock_connect(), leaving TCP_CLOSE / SS_UNCONNECTED and keeping
T1 ("We keep the binding and the transport assigned"), while
thread B still sleeps with the sock lock released
Thread C: connect() to a peer that resolves to a different transport, so
vsock_assign_transport() deassigns T1 and then fails at the
seqpacket_allow check or in init(); vsk->transport is now NULL
Thread D: listen() sees sock->state == SS_UNCONNECTED and
vsk->transport == NULL, passes the new gate and sets TCP_LISTEN
Thread B: wakes up; TCP_LISTEN is neither TCP_ESTABLISHED nor TCP_CLOSING
and sk_err == 0, so it keeps looping, and on the next
signal/timeout it runs:
net/vmw_vsock/af_vsock.c:vsock_connect() {
...
if (signal_pending(current) || timeout == 0) {
err = timeout == 0 ? -ETIMEDOUT :
sock_intr_errno(timeout);
...
sk->sk_state = TCP_CLOSE;
sock->state = SS_UNCONNECTED;
vsock_transport_cancel_pkt(vsk);
goto out_wait;
}
...
}
That stale waiter rewrites sk_state and sock->state without re-checking
whether the socket became a listener in the meantime, so accept() on it
would then return -EINVAL and incoming requests would no longer be queued,
while entries from the listener phase remain on pending_links/accept_queue.
The window is also user-controllable through SO_VM_SOCKETS_CONNECT_TIMEOUT,
which sets vsk->connect_timeout read at loop entry.
Reading the rest of the series, up to b93debf817d4 ("vsock: Handle sudden
TCP_CLOSE during connect"), vsock_assign_transport() is unchanged,
vsock_listen() still gates only on sock->state != SS_UNCONNECTED ||
vsk->transport, and the waiter still rewrites sk_state/sock->state, so the
gap does not appear to be closed later either.
Would it make more sense to express the invariant as "no connect() waiter
outstanding", or to make the reassignment failure paths not leave a live
socket transport-less?
--
Sashiko AI review ·
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260915-vsock-connect-reset-closing-v2-0-a1d9abb472f7%40rbox.co
