Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:
Critical: 0 · High: 0 · Medium: 1 · Low: 1
- [Medium] The new consumer in vsock_connect() reads the poison flag with
READ_ONCE(vsk->peer_shutdown) (net/vmw_vsock/af_vsock.c:1747), but the…
- [Low] Undeclared intra-series dependency: the new guard in
vsock_connect() (net/vmw_vsock/af_vsock.c:1747) only *reads*…
--
vsock: Handle sudden TCP_CLOSE during connect
This adds a check in vsock_connect() that refuses a fresh connect attempt
with -ECONNRESET when vsk->peer_shutdown is already set, so that a socket
poisoned by a lockless virtio/PM/vhost/VMCI reset event cannot be handed a
new transport or pushed back into TCP_SYN_SENT.
> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
> index adf3f018347ea..972952d04a81a 100644
> --- a/net/vmw_vsock/af_vsock.c
> +++ b/net/vmw_vsock/af_vsock.c
> @@ -1743,6 +1743,12 @@ static int vsock_connect(struct socket *sock, struct
> sockaddr_unsized *addr,
> goto out;
> }
>
> + /* Virtio/PM events are serviced locklessly. */
> + if (READ_ONCE(vsk->peer_shutdown)) {
> + err = -ECONNRESET;
> + goto out;
> + }
> +
[Severity: Low]
The comment mentions virtio/PM events, but this hunk only reads
vsk->peer_shutdown. For the virtio/PM path the flag is published by
virtio_vsock_reset_sock() in net/vmw_vsock/virtio_transport.c:
vsk->peer_shutdown = SHUTDOWN_MASK;
sk->sk_state = TCP_CLOSE;
sk->sk_err = ECONNRESET;
That store comes from the immediately preceding patch in this series
("vsock/virtio: Streamline socket reset on transport/PM event"); at the
parent commit virtio_vsock_reset_sock() only sets sk_state and sk_err.
Given the Fixes: d021c344051a tag reaching back to 2013, would it be worth
spelling out the dependency so that a stable/AUTOSEL pick does not take this
commit alone? On its own the new check would be a no-op for exactly the
virtio/PM scenario the changelog leads with, and only vhost's
vhost_vsock_reset_orphans() and VMCI's vmci_transport_handle_detach() (which
already set the flag) would be covered.
[Severity: Medium]
Should the writers of this flag be marked to match the new READ_ONCE()
reader? The two handlers named in the changelog store it plainly:
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);
}
drivers/vhost/vsock.c:vhost_vsock_reset_orphans() {
sock_set_flag(sk, SOCK_DONE);
vsk->peer_shutdown = SHUTDOWN_MASK;
sk->sk_state = TCP_CLOSE;
sk->sk_err = ECONNRESET;
}
Both run from vsock_for_each_connected_socket() under
spin_lock_bh(&vsock_table_lock) only, never under lock_sock(), so they share
no lock with vsock_connect(). Every other lockless writer of the field uses
WRITE_ONCE(), for example vmci_transport_handle_detach(),
virtio_transport_recv_pkt() in virtio_transport_common.c and
hvs_close_connection() in hyperv_transport.c. Is the plain store racing the
new marked read something KCSAN would flag here?
There is also no barrier pairing the poison store with the sk_state/sk_err
stores that connect() actually leaves its wait loop on. Can the following
still happen on a weakly ordered machine?
connect() wait loop exits on timeout/signal:
sk->sk_state = TCP_CLOSE;
sock->state = SS_UNCONNECTED;
vsock_transport_cancel_pkt(vsk);
userspace retries connect(), reads peer_shutdown == 0,
falls through to vsock_assign_transport() and
sk->sk_state = TCP_SYN_SENT;
while the socket is still hashed in connected_table. A later OP_RESPONSE
then reaches virtio_transport_recv_connecting(), which calls
vsock_insert_connected() unconditionally, and __vsock_insert_connected() does
a bare list_add() on an already-linked node. That is the list_add double add
splat described in the sibling commit 6f06a44e.
Would a WRITE_ONCE() for both stores, plus explicit ordering between the
peer_shutdown store and the sk_state/sk_err stores, close that window?
--
Sashiko AI review ·
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260915-vsock-connect-reset-closing-v2-0-a1d9abb472f7%40rbox.co
