Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.
Critical: 0 · High: 2 · Medium: 1 · Low: 0
If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).
- [High] The WARN_ON_ONCE() assertions added in vsock_unconnected_reset()
(af_vsock.c:1695-1698) are reachable from ordinary peer behaviour, and…
- [High] vsock_unconnected_reset() infers 'never established' from
sk_state/peer_shutdown/SOCK_DONE only.
- [Medium] Incomplete lifecycle change vs. commit message: for a non-
blocking connect() the new de-assign path is never reached when the…
--
vsock: do not preserve transport assignment
This adds vsock_unconnected_reset() and reworks vsock_connect() so that a
failed connection attempt de-assigns the transport and reverts the socket
to its initial state, while an established socket keeps its transport until
close().
A few questions on the new helper and on the paths that reach it.
> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
> index 5c8e7e7d35b4f..20181ddde114f 100644
> --- a/net/vmw_vsock/af_vsock.c
> +++ b/net/vmw_vsock/af_vsock.c
> @@ -1679,6 +1679,42 @@ static int vsock_transport_cancel_pkt(struct
> vsock_sock *vsk)
> return transport->cancel_pkt(vsk);
> }
>
> +static void vsock_unconnected_reset(struct sock *sk)
> +{
> + struct vsock_sock *vsk = vsock_sk(sk);
> +
> + sock_owned_by_me(sk);
> +
> + /*
> + * Only connected socks may have peer_shutdown or SOCK_DONE set.
> + *
> + * Once established (TCP_ESTABLISHED, TCP_CLOSING), a socket can be
> + * de-assigned only on close(). But we can narrow the check down to
> + * states we actually expect (TCP_SYN_SENT, TCP_CLOSE).
> + */
> + if (WARN_ON_ONCE(vsk->peer_shutdown) ||
> + WARN_ON_ONCE(sock_flag(sk, SOCK_DONE)) ||
> + WARN_ON_ONCE(sk->sk_state != TCP_SYN_SENT &&
> + sk->sk_state != TCP_CLOSE))
> + return;
[Severity: High]
Is the "only connected socks may have peer_shutdown or SOCK_DONE set"
invariant true for VMCI?
vmci_transport_recv_connecting_client_negotiate() allocates the queue pair
and subscribes to VMCI_EVENT_QP_PEER_DETACH while the socket is still in
TCP_SYN_SENT. If the peer then detaches, vmci_transport_handle_detach()
runs on a never-established socket:
net/vmw_vsock/vmci_transport.c:vmci_transport_handle_detach() {
...
sock_set_flag(sk, SOCK_DONE);
...
WRITE_ONCE(vsk->peer_shutdown, SHUTDOWN_MASK);
...
if (sk->sk_state == TCP_SYN_SENT) {
...
sk->sk_state = TCP_CLOSE;
sk->sk_err = ECONNRESET;
sk_error_report(sk);
...
}
The blocking connect() waiter then wakes on sk_err, err = sock_error(sk)
becomes -ECONNRESET, and out_reset calls this helper with peer_shutdown ==
SHUTDOWN_MASK and SOCK_DONE set.
Since WARN_ON_ONCE() evaluates to the condition, the helper splats and
returns before vsock_transport_cancel_pkt(), vsock_deassign_transport() and
the TCP_CLOSE/SS_UNCONNECTED restore. Can that leave the socket stuck with
sk_socket->state == SS_CONNECTING and the transport (plus its module
reference) pinned, so a later connect() returns -EALREADY for O_NONBLOCK or
re-hits the same early return, and listen() returns -EINVAL?
Before this patch the same path unconditionally ran:
err = sock_error(sk);
if (err) {
sk->sk_state = TCP_CLOSE;
sock->state = SS_UNCONNECTED;
}
so the socket stayed retryable. Is the change in behaviour here intended?
On panic_on_warn=1 kernels this is also reachable from an unprivileged
connect() when the peer VM goes away mid-handshake.
> +
> + /*
> + * Try to cancel a VIRTIO_VSOCK_OP_REQUEST skb that may have been sent
> + * out by transport->connect().
> + */
> + vsock_transport_cancel_pkt(vsk);
> +
> + /*
> + * No need to invoke transport->release() for unconnected connectible
> + * sockets. Go straight for transport deassign.
> + */
> + vsock_deassign_transport(vsk);
[Severity: High]
Can this de-assign the transport of a socket that did reach
TCP_ESTABLISHED?
virtio_vsock_reset_sock() (and vhost_vsock_reset_orphans()) move an
established socket to TCP_CLOSE without lock_sock(), holding only
vsock_table_lock:
net/vmw_vsock/virtio_transport.c:virtio_vsock_reset_sock() {
...
sk->sk_state = TCP_CLOSE;
sk->sk_err = ECONNRESET;
sk_error_report(sk);
...
}
sk_socket->state stays SS_CONNECTED and the socket stays linked in
vsock_connected_table. A blocking connect() woken after the handshake
completed then reaches out_reset with err = -ECONNRESET, and all three
guards above pass because they only look at sk_state, peer_shutdown and
SOCK_DONE.
virtio_transport_destruct() frees the transport state without purging
vvs->rx_queue and without unhashing the socket:
net/vmw_vsock/virtio_transport_common.c:virtio_transport_destruct() {
virtio_transport_cancel_close_work(vsk, true);
kfree(vvs);
vsk->trans = NULL;
...
}
The rx_queue purge lives in virtio_transport_remove_sock(), which is only
reached via release(), deliberately skipped here. Does this leak the queued
rx skbs, and does a subsequent successful connect() perform a second
list_add() on the still-linked connected_table node in
__vsock_insert_connected()?
A later patch in this series, "vsock: handle socket bricking due to
transport event", adds an early return on sk_socket->state == SS_CONNECTED
or SS_DISCONNECTING at the top of this helper, which covers exactly this
case, so this only affects this commit taken on its own. Would it be
cleaner to fold that guard in here?
> +
> + /* Revert socket to initial state. Keep sk_err. */
> + WRITE_ONCE(sk->sk_state, TCP_CLOSE);
> + sk->sk_socket->state = SS_UNCONNECTED;
> +}
> +
> static void vsock_connect_timeout(struct work_struct *work)
> {
> struct sock *sk;
> @@ -1690,11 +1726,9 @@ static void vsock_connect_timeout(struct work_struct
> *work)
> lock_sock(sk);
> if (sk->sk_state == TCP_SYN_SENT &&
> (sk->sk_shutdown != SHUTDOWN_MASK)) {
> - sk->sk_state = TCP_CLOSE;
> - sk->sk_socket->state = SS_UNCONNECTED;
> sk->sk_err = ETIMEDOUT;
> sk_error_report(sk);
> - vsock_transport_cancel_pkt(vsk);
> + vsock_unconnected_reset(sk);
> }
> release_sock(sk);
>
[ ... ]
> @@ -1874,13 +1896,11 @@ static int vsock_connect(struct socket *sock, struct
> sockaddr_unsized *addr,
> }
>
> err = sock_error(sk);
> - if (err) {
> - sk->sk_state = TCP_CLOSE;
> - sock->state = SS_UNCONNECTED;
> - }
> -
> out_wait:
> finish_wait(sk_sleep(sk), &wait);
> +out_reset:
> + if (err)
> + vsock_unconnected_reset(sk);
> out:
> release_sock(sk);
> return err;
[Severity: Medium]
For a non-blocking connect() whose failure is detected asynchronously by
the transport, which path reaches out_reset?
connect(fd, O_NONBLOCK) assigns a transport, sets sk_state = TCP_SYN_SENT,
sock->state = SS_CONNECTING and returns -EINPROGRESS. If the peer answers
with VIRTIO_VSOCK_OP_RST, virtio_transport_recv_connecting() takes its
destroy label:
net/vmw_vsock/virtio_transport_common.c:virtio_transport_recv_connecting() {
...
destroy:
virtio_transport_reset(vsk, skb);
sk->sk_state = TCP_CLOSE;
sk->sk_err = skerr;
sk_error_report(sk);
...
}
sk_socket->state is left at SS_CONNECTING and the transport stays assigned.
vmci_transport_recv_connecting_client() has the same pattern.
vsock_connect_timeout() above is gated on sk->sk_state == TCP_SYN_SENT, so
it does not reset either once the state is TCP_CLOSE. And a retry with
O_NONBLOCK hits:
case SS_CONNECTING:
...
err = -EALREADY;
if (flags & O_NONBLOCK)
goto out;
which returns before out_reset.
Does the socket then keep the transport assignment and the transport module
reference until close()? The changelog says:
If connection fails (init went wrong, peer misbehaviour, time out,
signal), transport is de-assigned and socket state is re-initialized.
Should the asynchronous peer-misbehaviour case for a non-blocking connect()
be covered as well, or should the changelog be narrowed to the synchronous
paths?
--
Sashiko AI review ·
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260922-vsock-connect-reset-closing-v3-0-78907b8200d4%40rbox.co
