Re: [PATCH net v2 5/5] vsock: Handle sudden TCP_CLOSE during connect

[netdev-bot+sashiko@kernel.org]

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