Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Critical: 0 · High: 0 · Medium: 1 · Low: 0 (1) - [Medium] Undocumented change of connect()'s return value / sk_err consumption for an already-established socket. Pre-existing issues: - [Low] Reported gap (validated as resolved later in this series, therefore discarded): at this commit vsock_connect() keys its "do not… -- vsock: Do not reset a TCP_CLOSING socket This teaches the vsock_connect() wait loop about the TCP_ESTABLISHED -> TCP_CLOSING transition triggered by VIRTIO_VSOCK_OP_RST, so a socket that is still linked in vsock_connected_table is not reset behind the receive path's back. It also rewrites the tail of vsock_connect() and drops a stale comment. Link: https://lore.kernel.org/netdev/anzT1fREOSyHT99k@v4bel/ > diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c > index 499e902becfad..adf3f018347ea 100644 > --- a/net/vmw_vsock/af_vsock.c > +++ b/net/vmw_vsock/af_vsock.c [ ... ] > @@ -1834,23 +1833,22 @@ 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 > - * release it, that's not connect()'s concern. No need to go > + /* Connection was 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 = 0; > + goto out_wait; > + } [Severity: Medium] The old code did a plain break here, so control fell through to the err = sock_error(sk) tail. Now it jumps straight to out_wait with err = 0. Combined with the tail change below, does this mean a blocking connect() that wakes up in TCP_ESTABLISHED with sk_err already set now returns 0 instead of that error, leaving sk_err queued for the next send()/recv()? That window looks reachable via virtio_transport_recv_connected(), which runs only for TCP_ESTABLISHED sockets: net/vmw_vsock/virtio_transport_common.c:virtio_transport_recv_connected() { case VIRTIO_VSOCK_OP_RW: if (!virtio_transport_recv_enqueue(vsk, skb)) { ... (void)virtio_transport_reset(vsk, skb); virtio_transport_do_close(vsk, true); sk->sk_err = ENOBUFS; ... } and virtio_transport_do_close() only downgrades the state when the receive queue is empty: net/vmw_vsock/virtio_transport_common.c:virtio_transport_do_close() { WRITE_ONCE(vsk->peer_shutdown, SHUTDOWN_MASK); if (vsock_stream_has_data(vsk) <= 0) sk->sk_state = TCP_CLOSING; ... } In the queue-full case vsock_stream_has_data(vsk) > 0, so sk_state stays TCP_ESTABLISHED while sk_err == ENOBUFS. The changelog mentions only the TCP_CLOSING transition plus "Drop the redundant err=0 and the inaccurate comment above signal_pending()". Could the changelog also spell out that connect() no longer reports or consumes a pending sk_err for an established socket? The patch carries a Fixes: d021c344051a tag, so it is a backport candidate and the user-visible change of the connect() return value seems worth documenting. > > /* If connection was _not_ established and a signal/timeout came > * to be, we want the socket's state reset. User space may want > * to retry. > - * > - * sk_state != TCP_ESTABLISHED implies that socket is not on > - * vsock_connected_table. We keep the binding and the transport > - * assigned. > */ > if (signal_pending(current) || timeout == 0) { > err = timeout == 0 ? -ETIMEDOUT : > sock_intr_errno(timeout); [ ... ] > @@ -1874,8 +1872,8 @@ static int vsock_connect(struct socket *sock, struct > sockaddr_unsized *addr, > prepare_to_wait(sk_sleep(sk), &wait, TASK_INTERRUPTIBLE); > } > > - err = sock_error(sk); > - if (err) { > + if (sk->sk_state != TCP_ESTABLISHED && sk->sk_state != TCP_CLOSING) { > + err = sock_error(sk); > sk->sk_state = TCP_CLOSE; > sock->state = SS_UNCONNECTED; > } [Severity: Low] This is a pre-existing issue and not introduced by this patch, but the new whitelist covers only TCP_ESTABLISHED and TCP_CLOSING. A socket that is still linked in vsock_connected_table but was moved to TCP_CLOSE by a lockless event handler still gets reset to SS_UNCONNECTED both here and in the signal_pending() branch above. Two writers do that without calling vsock_remove_connected(): net/vmw_vsock/virtio_transport.c:virtio_vsock_reset_sock() { vsk->peer_shutdown = SHUTDOWN_MASK; sk->sk_state = TCP_CLOSE; sk->sk_err = ECONNRESET; sk_error_report(sk); } net/vmw_vsock/vmci_transport.c:vmci_transport_handle_detach() { WRITE_ONCE(vsk->peer_shutdown, SHUTDOWN_MASK); ... sk->sk_state = TCP_CLOSE; } With sock->state == SS_UNCONNECTED, a retry connect() takes the default: branch, vsock_assign_transport() returns early because the transport is unchanged, transport->connect() is re-issued, and the next RESPONSE reaches __vsock_insert_connected(): net/vmw_vsock/af_vsock.c:__vsock_insert_connected() { sock_hold(&vsk->sk); list_add(&vsk->connected_table, list); } which is the same list_add double add BUG quoted in the changelog, plus a leaked struct sock reference. For the record, this appears to be closed by the next patch in the series, "vsock: Handle sudden TCP_CLOSE during connect", which adds /* Virtio/PM events are serviced locklessly. */ if (READ_ONCE(vsk->peer_shutdown)) { err = -ECONNRESET; goto out; } to the same default: branch, and every writer above sets peer_shutdown = SHUTDOWN_MASK before moving the socket to TCP_CLOSE, so no action seems needed on the series as a whole. Would it be worth noting in this patch's changelog that the TCP_CLOSE variant of the same race is handled by the following patch? -- Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260915-vsock-connect-reset-closing-v2-0-a1d9abb472f7%40rbox.co
