Re: [PATCH net-next 2/6] vsock: add IOCTL_VM_SOCKETS_ASSIGN_G2H_NETNS

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 0 · Low: 3

- [Low] Claim: the new ioctl is inert because no in-tree transport
  implements `netns_assign_allow`, and `vsock_g2h_net_get()` /…
- [Low] Claim: TOCTOU between `vsock_g2h_net_assign()` (which only sweeps
  the connected hash table) and in-flight connections — a socket in…
- [Low] Claim: the reachability gate in `vsock_assign_transport()` is
  skipped by the `vsk->transport == new_transport` early return (so a…

--

vsock: add IOCTL_VM_SOCKETS_ASSIGN_G2H_NETNS

This adds an ioctl on /dev/vsock that moves the guest's vsock device into
the network namespace of the calling process, gated on CAP_NET_ADMIN in the
initial user namespace, plus a transport hook so transports can opt in.
Sockets that can no longer reach the device after a move are reset, and the
device falls back to init_net when the assigned namespace is destroyed.

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

No regressions to report on this one. A few areas were looked at closely
and appear fine; they are noted below only so the review record is
complete.

> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
> index 29cde17e08f3..ad11f0f56eb8 100644
> --- a/net/vmw_vsock/af_vsock.c
> +++ b/net/vmw_vsock/af_vsock.c

[ ... ]

> @@ -548,6 +571,17 @@ static void vsock_deassign_transport(struct vsock_sock *vsk)
>  	vsk->transport = NULL;
>  }
>  
> +/* Return true if the loaded g2h transport honours namespace assignment. One
> + * that does not keeps the reachability rules it had before the ioctl existed.
> + *
> + * Must be called with vsock_register_mutex held.
> + */
> +static bool vsock_g2h_netns_assignable(void)
> +{
> +	return transport_g2h && transport_g2h->netns_assign_allow &&
> +	       transport_g2h->netns_assign_allow();
> +}

At this commit no in-tree transport sets .netns_assign_allow, so
vsock_g2h_netns_assignable() is always false and vsock_g2h_net_assign()
returns -EOPNOTSUPP, while vsock_g2h_net_get(), vsock_g2h_reachable_sk()
and vsock_maybe_set_connected() have no callers yet.

The following patch in the series adds virtio_transport_netns_assign_allow()
and the calls into these helpers, so this is the usual core-first split
rather than dead code.

> @@ -622,6 +656,13 @@ int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock *psk)
>  		goto err;
>  	}
>  
> +	if (new_transport && new_transport == transport_g2h &&
> +	    vsock_g2h_netns_assignable() &&
> +	    !vsock_g2h_net_reachable(sock_net(sk))) {
> +		ret = -ENETUNREACH;
> +		goto err;
> +	}
> +

This gate is skipped by the preceding

	if (vsk->transport && vsk->transport == new_transport) {
		ret = 0;
		goto err;
	}

early return, but vsock_connect() still consults
transport->stream_allow() after vsock_assign_transport() returns, and the
follow-up patch makes virtio_transport_stream_allow() and
virtio_transport_seqpacket_allow() namespace aware, so a re-connect from an
unreachable namespace is still refused with -ENETUNREACH.

For SOCK_DGRAM, virtio registers only as VSOCK_TRANSPORT_F_G2H and
virtio_transport_dgram_allow() returns false unconditionally, so a datagram
socket never holds the assignable transport.

[ ... ]

> +bool vsock_maybe_set_connected(struct vsock_sock *vsk)
> +{
> +	struct list_head *list = vsock_connected_sockets(&vsk->remote_addr,
> +							 &vsk->local_addr);
> +	bool reachable;
> +
> +	spin_lock_bh(&vsock_table_lock);
> +	reachable = vsock_g2h_reachable_sk(vsk);
> +	if (reachable) {
> +		sk_vsock(vsk)->sk_state = TCP_ESTABLISHED;
> +		__vsock_insert_connected(list, vsk);
> +	}
> +	spin_unlock_bh(&vsock_table_lock);
> +
> +	return reachable;
> +}

[ ... ]

> +static int vsock_g2h_net_assign(struct net *net)
> +{
[ ... ]
> +		rcu_assign_pointer(vsock_g2h_net, net);
> +		vsock_for_each_connected_socket(transport_g2h,
> +						vsock_reset_unreachable_sock);
> +	}

Sockets still in TCP_SYN_SENT, and pending children, are not in the
connected table and so are not visible to this sweep. The ordering here
covers that case:

vsock_g2h_net_assign()
    rcu_assign_pointer(vsock_g2h_net, net);   /* store first */
    vsock_for_each_connected_socket()         /* under vsock_table_lock */

vsock_maybe_set_connected()
    spin_lock_bh(&vsock_table_lock);
    reachable = vsock_g2h_reachable_sk(vsk);  /* re-check under same lock */

Either the insert wins the lock and the later walk sees and resets the
socket, or the walk wins and the insert observes the new namespace and
refuses, so a socket cannot slip into TCP_ESTABLISHED in a namespace that
can no longer reach the device once the transport side lands.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260902-vsock-guest-ns-v1-0-9995383e9a8b%40meta.com