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

[netdev-bot+sashiko]

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

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

- [Medium] After IOCTL_VM_SOCKETS_ASSIGN_G2H_NETNS becomes functional
  (this patch adds the only .netns_assign_allow implementation in the…
- [Medium] vsock_maybe_set_connected() makes only the table insertion
  atomic with respect to namespace assignment; the connection-setup…
- [Medium] virtio_transport_rx_work() samples the g2h namespace once (net
  = vsock_g2h_net_get(), before mutex_lock(&vsock->rx_lock)) and reuses…
- [Low] In the new `!net` branch of virtio_transport_rx_work()
  (net/vmw_vsock/virtio_transport.c:~694) the code passes `&init_net` to…

--

vsock/virtio: support guest device network namespace

This makes the virtio-vsock guest device assignable to a network namespace,
adds a .netns_assign_allow callback, and switches the RX path to look up
sockets in the device's namespace. It also exports
virtio_transport_reset_no_sock() so the RX path can reset packets that
arrive while the device's namespace is going away.

A few questions about the new namespace assignment behaviour below.

> diff --git a/net/vmw_vsock/virtio_transport.c b/net/vmw_vsock/virtio_transport.c
> index 4f9aa9c4c3aa..a453a4f828dc 100644
> --- a/net/vmw_vsock/virtio_transport.c
> +++ b/net/vmw_vsock/virtio_transport.c
> @@ -540,9 +540,14 @@ static bool virtio_transport_msgzerocopy_allow(void)
>  	return true;
>  }
>  
> +static bool virtio_transport_netns_assign_allow(void)
> +{
> +	return true;
> +}
> +

[Severity: Medium]
This is the only .netns_assign_allow implementation in the tree, so this is
the patch that turns vsock_g2h_net_assign() and its reset sweep from
-EOPNOTSUPP into live behaviour.

The commit message says "every already established vsock socket that is no
longer able to reach the device is forcibly reset".  Is the host peer ever
told about that reset?  vsock_g2h_net_assign() only runs:

net/vmw_vsock/af_vsock.c:vsock_reset_unreachable_sock() {
	if (vsock_g2h_net_reachable(sock_net(sk)))
		return;

	sk->sk_state = TCP_CLOSE;
	sk->sk_err = ECONNRESET;
	sk_error_report(sk);
}

There is no virtio_transport_reset() and no VIRTIO_VSOCK_OP_RST here, and
SOCK_DONE is not set nor is the socket removed from the connected table.

When the guest application later closes the socket, does anything get sent
either?  virtio_transport_close() returns early for any state other than
TCP_ESTABLISHED/TCP_CLOSING:

	if (!(sk->sk_state == TCP_ESTABLISHED ||
	      sk->sk_state == TCP_CLOSING))
		return true;

so a swept socket in TCP_CLOSE emits neither SHUTDOWN nor RST, and
virtio_transport_release() just sets SOCK_DONE and calls
virtio_transport_remove_sock().

Does this leave the host side of each connection in ESTABLISHED until it
happens to send something (at which point recv_pkt's lookup misses in the
new namespace and replies RST)?  A host peer blocked in recv() would then
wait for its own timeout, which is the same failure mode the commit message
rules out for the dropped-packet case.

>  bool virtio_transport_stream_allow(struct vsock_sock *vsk, u32 cid, u32 port)
>  {
> -	return vsock_net_mode_global(vsk);
> +	return vsock_g2h_net_reachable(sock_net(sk_vsock(vsk)));
>  }
>  
>  static bool virtio_transport_seqpacket_allow(struct vsock_sock *vsk,
> @@ -587,6 +592,7 @@ static struct virtio_transport virtio_transport = {
>  		.seqpacket_has_data       = virtio_transport_seqpacket_has_data,
>  
>  		.msgzerocopy_allow        = virtio_transport_msgzerocopy_allow,
> +		.netns_assign_allow       = virtio_transport_netns_assign_allow,
>  

[ ... ]

> @@ -634,6 +640,9 @@ static void virtio_transport_rx_work(struct work_struct *work)
>  	struct virtio_vsock *vsock =
>  		container_of(work, struct virtio_vsock, rx_work);
>  	struct virtqueue *vq;
> +	struct net *net;
> +
> +	net = vsock_g2h_net_get();
>  
>  	mutex_lock(&vsock->rx_lock);

[Severity: Medium]
The namespace is sampled once here and then reused for the entire batch,
i.e. for the whole inner for(;;) drain and the outer
do{}while(!virtqueue_enable_cb(vq)) retry.  vsock_g2h_net is re-pointed by
IOCTL_VM_SOCKETS_ASSIGN_G2H_NETNS under vsock_register_mutex/RCU.

Can this snapshot go stale mid-batch, so that after the ioctl has already
returned success the remaining packets are still resolved against the old
namespace via vsock_find_connected_socket_net()/
vsock_find_bound_socket_net()?

For already established sockets the only fence is the assignment sweep, and
that sweep does not take the socket lock:

CPU0 (rx_work)                        CPU1 (assign ioctl)
virtio_transport_recv_pkt()
  lock_sock(sk)
  /* re-checks only SOCK_DONE and
     vsk->transport, not the ns */
                                      vsock_g2h_net_assign()
                                        rcu_assign_pointer(vsock_g2h_net, net)
                                        vsock_reset_unreachable_sock(sk)
                                          sk->sk_state = TCP_CLOSE
  switch (sk->sk_state)  /* read TCP_ESTABLISHED earlier */
    virtio_transport_recv_connected(sk, skb)

Does that let host payload be queued into a socket in the namespace the
device was just moved away from?  Would re-validating reachability after
lock_sock() (e.g. vsock_g2h_reachable_sk()) close this window?

> @@ -682,10 +691,14 @@ static void virtio_transport_rx_work(struct work_struct *work)
>  
>  			virtio_transport_deliver_tap_pkt(skb);
>  
> -			/* Force virtio-transport into global mode since it
> -			 * does not yet support local-mode namespacing.
> -			 */
> -			virtio_transport_recv_pkt(&virtio_transport, skb, NULL);
> +			if (unlikely(!net)) {
> +				virtio_transport_reset_no_sock(
> +					&virtio_transport, skb, &init_net);
> +				kfree_skb(skb);
> +				continue;
> +			}

[Severity: Low]
This isn't a functional bug today, but is &init_net the right value to pass
here?  vsock_g2h_net_get() returns NULL only after it has already rejected
init_net as a stand-in:

net/vmw_vsock/af_vsock.c:vsock_g2h_net_get() {
	net = maybe_get_net(assigned);
	...
	if (!net && vsock_net_check_mode(&init_net, assigned))
		net = get_net(&init_net);
}

So reaching this branch means vsock_net_check_mode(&init_net, assigned) was
false, yet &init_net is then handed to virtio_transport_reset_no_sock() as
"the namespace of whoever sent the invalid message".  The pre-patch code
passed NULL, and virtio_transport_recv_pkt() below is given the real device
namespace.

virtio_transport_send_pkt() ignores its net argument, so nothing breaks in
this transport, but the function is now exported and declared in
include/linux/virtio_vsock.h, where the vhost and loopback send_pkt()
implementations do use net.  Would NULL (as before) or a note that virtio
ignores net keep the two call sites consistent?

> +
> +			virtio_transport_recv_pkt(&virtio_transport, skb, net);
>  		}
>  	} while (!virtqueue_enable_cb(vq));
>  

[ ... ]

> diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
> index 88df82364f77..313ef263fd2d 100644
> --- a/net/vmw_vsock/virtio_transport_common.c
> +++ b/net/vmw_vsock/virtio_transport_common.c
> @@ -1315,8 +1315,8 @@ static int virtio_transport_reset(struct vsock_sock *vsk,
>   * loopback, this is the namespace of the socket. For vhost, this is the
>   * namespace of the VM (i.e., vhost_vsock).
>   */
> -static int virtio_transport_reset_no_sock(const struct virtio_transport *t,
> -					  struct sk_buff *skb, struct net *net)
> +int virtio_transport_reset_no_sock(const struct virtio_transport *t,
> +				   struct sk_buff *skb, struct net *net)
>  {

[ ... ]

> @@ -1478,9 +1479,14 @@ virtio_transport_recv_connecting(struct sock *sk,
>  
>  	switch (le16_to_cpu(hdr->op)) {
>  	case VIRTIO_VSOCK_OP_RESPONSE:
> -		sk->sk_state = TCP_ESTABLISHED;
> +		/* An assign cannot see a socket that is not connected yet. */
> +		if (!vsock_maybe_set_connected(vsk)) {
> +			skerr = ECONNRESET;
> +			err = -ENETUNREACH;
> +			goto destroy;
> +		}
> +
>  		sk->sk_socket->state = SS_CONNECTED;
> -		vsock_insert_connected(vsk);
>  		sk->sk_state_change(sk);
>  		break;

[ ... ]

> @@ -1758,7 +1762,17 @@ virtio_transport_recv_listen(struct sock *sk, struct sk_buff *skb,
>  	if (virtio_transport_space_update(child, skb))
>  		child->sk_write_space(child);
>  
> -	vsock_insert_connected(vchild);
> +	/* An assign cannot see a socket that is not connected yet, and the
> +	 * check in vsock_assign_transport() above has since dropped
> +	 * vsock_register_mutex.
> +	 */
> +	if (!vsock_maybe_set_connected(vchild)) {
> +		release_sock(child);
> +		virtio_transport_reset_no_sock(t, skb, sock_net(sk));
> +		sock_put(child);
> +		return -ENETUNREACH;
> +	}
> +
>  	vsock_enqueue_accept(sk, child);
>  	virtio_transport_send_response(vchild, skb);
>  

[Severity: Medium]
vsock_maybe_set_connected() makes only the table insertion atomic against
assignment, and it drops vsock_table_lock before returning.  Does anything
re-check the socket afterwards?

The commit message says "If it wins the race to the table and does NOT pass
the reachability tests, then the reset sweep will correctly catch it", but
the winner here keeps going past the insertion point:

CPU0 (virtio_transport_recv_listen)     CPU1 (assign ioctl)
lock_sock_nested(child)
vsock_maybe_set_connected(vchild)
  spin_lock_bh(&vsock_table_lock)
  sk_state = TCP_ESTABLISHED
  __vsock_insert_connected()
  spin_unlock_bh(&vsock_table_lock)
                                        vsock_g2h_net_assign()
                                          vsock_for_each_connected_socket()
                                            vsock_reset_unreachable_sock(child)
                                              sk_state = TCP_CLOSE
                                              sk_err = ECONNRESET
vsock_enqueue_accept(sk, child)
virtio_transport_send_response(vchild, skb)

The lock sets are disjoint (lock_sock(child) versus vsock_table_lock), so
does the guest end up sending VIRTIO_VSOCK_OP_RESPONSE for a child it has
already reset, and queueing that child on the accept queue?  vsock_accept()
inspects neither sk_err nor sk_state before doing:

	newsock->state = SS_CONNECTED;
	sock_graft(connected, newsock);

The same gap seems to exist in virtio_transport_recv_connecting() above,
where sk->sk_socket->state = SS_CONNECTED and sk_state_change() run after
vsock_maybe_set_connected() has dropped the table lock, even if the sweep
has already forced sk_state to TCP_CLOSE.

Since vsock_reset_unreachable_sock() neither sets SOCK_DONE nor removes the
socket from the table, and virtio_transport_close() short-circuits for
TCP_CLOSE, is any RST sent to the host in this case?

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