Re: [PATCH net-next v4 4/7] vsock: add a netlink command to assign the g2h device to a netns

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] vsock: incomplete socket state reset in vsock_g2h_reset_unreachable()
--

commit fe6dcf2ff7ad3f974f6de58396eb4f397d8574af
Author: Bobby Eshleman <bobbyeshleman@meta.com>

vsock: add a netlink command to assign the g2h device to a netns

This commit introduces a vsock generic netlink family with
VSOCK_CMD_DEV_NETNS_SET and VSOCK_CMD_DEV_NETNS_GET to assign the guest-to-host
vsock device to a network namespace and query its current assignment. When the
device moves to a namespace that existing connected sockets cannot reach, those
sockets are swept and closed with ECONNRESET.

Link: https://lore.kernel.org/all/20200427142518.uwssa6dtasrp3bfc@steredhat/

> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
> index c9b28e34b1b3c..e16e72471b4a3 100644
> --- a/net/vmw_vsock/af_vsock.c
> +++ b/net/vmw_vsock/af_vsock.c

[ ... ]

> @@ -742,8 +823,303 @@ bool vsock_find_cid(unsigned int cid)

[ ... ]

> +/* Close every connected socket of @t that can no longer reach the g2h device.
> + */
> +static void vsock_g2h_reset_unreachable(const struct vsock_transport *t)
> +{
> +	struct vsock_sock *vsk;
> +	unsigned int i = 0;
> +	struct sock *sk;
> +	bool reset;
> +
> +	lockdep_assert_held(&vsock_g2h_assign_mutex);
> +	lockdep_assert_not_held(&vsock_register_mutex);
> +
> +	while ((vsk = vsock_g2h_next_unreachable(t, &i))) {
> +		sk = sk_vsock(vsk);
> +
> +		lock_sock(sk);
> +
> +		reset = false;
> +		spin_lock_bh(&vsock_table_lock);
> +		if (__vsock_in_connected_table(vsk) &&
> +		    vsock_g2h_should_reset(vsk, t)) {
> +			__vsock_remove_connected(vsk);
> +			reset = true;
> +		}
> +		spin_unlock_bh(&vsock_table_lock);
> +
> +		if (reset) {
> +			sk->sk_state = TCP_CLOSE;
> +			sk->sk_err = ECONNRESET;
> +			sk_error_report(sk);
> +		}
> +
> +		release_sock(sk);
> +		sock_put(sk);
> +	}
> +}

[Severity: Medium]
When vsock_g2h_reset_unreachable() resets an established socket, it sets
sk->sk_state = TCP_CLOSE and sk->sk_err = ECONNRESET, but leaves SOCK_DONE
unset, vsk->peer_shutdown unchanged, and sk->sk_socket->state as
SS_CONNECTED.

On a subsequent read, __vsock_connectible_recvmsg() checks sk->sk_state
before reaching the stream or seqpacket receive paths that check
sock_error(sk):

net/vmw_vsock/af_vsock.c:__vsock_connectible_recvmsg()
	if (!transport || sk->sk_state != TCP_ESTABLISHED) {
		if (sock_flag(sk, SOCK_DONE))
			err = 0;
		else
			err = -ENOTCONN;

		goto out;
	}

And on a subsequent write, vsock_connectible_sendmsg() skips the -EPIPE
branch because vsk->peer_shutdown does not have RCV_SHUTDOWN set, and
instead takes the -ENOTCONN branch:

net/vmw_vsock/af_vsock.c:vsock_connectible_sendmsg()
	if (sk->sk_shutdown & SEND_SHUTDOWN ||
	    vsk->peer_shutdown & RCV_SHUTDOWN) {
		err = -EPIPE;
		goto out;
	}

	if (!transport || sk->sk_state != TCP_ESTABLISHED ||
	    !vsock_addr_bound(&vsk->local_addr)) {
		err = -ENOTCONN;
		goto out;
	}
...
out:
	if (sk->sk_type == SOCK_STREAM)
		err = sk_stream_error(sk, msg->msg_flags, err);

Because sk_stream_error() only replaces err with sock_error(sk) when
err == -EPIPE (and is not called for SOCK_SEQPACKET), both recvmsg() and
sendmsg() return -ENOTCONN instead of -ECONNRESET without clearing
sk->sk_err, leaving vsock_poll() permanently reporting EPOLLERR | EPOLLOUT.

In addition, because sk->sk_socket->state remains SS_CONNECTED, a retried
vsock_connect() fails with -EISCONN:

net/vmw_vsock/af_vsock.c:vsock_connect()
	switch (sock->state) {
	case SS_CONNECTED:
		err = -EISCONN;
		goto out;

While vsock_shutdown() sees sock->state != SS_UNCONNECTED and still calls
vsock_send_shutdown() on the disallowed transport:

net/vmw_vsock/af_vsock.c:vsock_shutdown()
	if (sock->state == SS_UNCONNECTED) {
		err = -ENOTCONN;
		if (sock_type_connectible(sk->sk_type))
			goto out;
	} else {
		sock->state = SS_DISCONNECTING;
		err = 0;
	}

	/* Receive and send shutdowns are treated alike. */
	mode = mode & (RCV_SHUTDOWN | SEND_SHUTDOWN);
	if (mode) {
		sk->sk_shutdown |= mode;
		sk->sk_state_change(sk);

		if (sock_type_connectible(sk->sk_type)) {
			sock_reset_flag(sk, SOCK_DONE);
			vsock_send_shutdown(sk, mode);
		}
	}

Should vsock_g2h_reset_unreachable() also update vsk->peer_shutdown,
SOCK_DONE, and sk->sk_socket->state (when sk->sk_socket is present), or
ensure recvmsg() and sendmsg() consume sk->sk_err so callers observe
ECONNRESET and can reconnect as intended?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261020-vsock-guest-ns-v4-0-047527cfccfc@meta.com?part=4