Re: [PATCH net-next v2 4/6] vsock/virtio: support guest device network namespace

[sashiko-bot@kernel.org]

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

commit 22f9613466fe3520fb04ad3d32779638f9afe30e
Author: Bobby Eshleman <bobbyeshleman@meta.com>
vsock: add a netlink command to assign the g2h device to a netns

This commit introduces the VSOCK_CMD_DEV_NETNS_SET netlink command to move
the guest-to-host vsock device to a different network namespace. It resets
established connections that are no longer able to reach the device from
their respective namespaces. The commit also updates CID discovery and
socket creation paths to honour the new namespace assignments.

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 95a435aef512b..9938dd5010192 100644
> --- a/net/vmw_vsock/af_vsock.c
> +++ b/net/vmw_vsock/af_vsock.c
[ ... ]
> @@ -742,6 +812,173 @@ bool vsock_find_cid(unsigned int cid)
[ ... ]
> +/* Reset every connected socket of @t that can no longer reach the g2h device,
> + * and let the transport tell each peer.
> + */
> +static void vsock_g2h_reset_unreachable(const struct vsock_transport *t)
> +{
> +	struct vsock_sock *vsk, *tmp;
> +	LIST_HEAD(reset_list);
> +	struct sock *sk;
> +	int i;
> +
> +	/* The calling context must hold vsock_register_mutex, which serializes
> +	 * concurrent netns assignments' use of vsk->pending_reset.
> +	 */
> +	lockdep_assert_held(&vsock_register_mutex);
> +
> +	spin_lock_bh(&vsock_table_lock);
> +
> +	for (i = 0; i < ARRAY_SIZE(vsock_connected_table); i++) {
> +		list_for_each_entry(vsk, &vsock_connected_table[i],
> +				    connected_table) {
> +			sk = sk_vsock(vsk);
> +
> +			if (vsk->transport != t ||
> +			    sk->sk_state == TCP_CLOSE ||
> +			    vsock_g2h_reachable_sk(vsk))
> +				continue;
> +
> +			sk->sk_state = TCP_CLOSE;
> +			sk->sk_err = ECONNRESET;
> +			sk_error_report(sk);

[Severity: High]
Does this code modify the socket state locklessly?

sk->sk_state and sk->sk_err are modified here while holding only the
vsock_table_lock spinlock, without acquiring the socket lock (lock_sock).

Core socket operations, such as vsock_connectible_sendmsg(), rely on
lock_sock() to protect state transitions and evaluate sk->sk_state to
determine if data can be enqueued. Modifying the state here without the socket
lock introduces a race condition. For example, a thread holding lock_sock()
might read an established state and proceed to enqueue data, while this
function concurrently sets the state to TCP_CLOSE.

Should the state modification be deferred to the second loop where
lock_sock() can be acquired?

> +
> +			sock_hold(sk);
> +			list_add_tail(&vsk->pending_reset, &reset_list);
> +		}
> +	}
> +
> +	spin_unlock_bh(&vsock_table_lock);
> +
> +	/* Reset outside of spinlock because the transport may sleep
> +	 * (e.g., GFP_KERNEL alloc).
> +	 */
> +	list_for_each_entry_safe(vsk, tmp, &reset_list, pending_reset) {
> +		list_del_init(&vsk->pending_reset);
> +		t->reset(vsk, NULL);
> +		sock_put(sk_vsock(vsk));
> +	}
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260921-vsock-guest-ns-v2-0-693bd78fde9e@meta.com?part=4