Re: [PATCH net-next v5 5/7] vsock/virtio: support guest device network namespace

[Paolo Abeni <pabeni@redhat.com>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/
---
--- Patch [5]: [PATCH net-next v5 5/7] vsock/virtio: support guest device network namespace ---
I found three issues in this patch: one functional regression, one commit message problem, and one existing tap leak that the patch now reaches.

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

vsock/virtio: support guest device network namespace

This patch makes the virtio-vsock guest device assignable to a network
namespace. virtio_transport_rx_work() now re-reads the assigned netns for
each packet and passes it to virtio_transport_recv_pkt(). The listen and
connecting receive paths now call vsock_maybe_set_connected() instead of
vsock_insert_connected(), and virtio_transport_send_pkt() gains a
reachability check.

> Make the virtio-vsock device assignable to a namespace and initialize it
> to init_net. Because virtio-vsock and init_net are both hardcoded to
> global mode, nothing changes until the assign command is issued.

[ ... ]

> it wins the race to the table and does NOT pass the reachability tests,
> then it will be reset. This is the purpose of the new helper
> 'vsock_maybe_set_connected()'.

[Severity: Low]
[Finding: main-1-1]
[Sources: opus-5-5]
This isn't a bug, but does this description match what this patch does?

The previous commit in the series ("vsock: add a netlink command to
assign the g2h device to a netns") already defines and exports
vsock_maybe_set_connected(). The same commit adds netns_assign_allow to
struct vsock_transport and initializes the g2h netns to init_net:

net/vmw_vsock/af_vsock.c:
    static struct net __rcu *vsock_g2h_net = RCU_INITIALIZER(&init_net);

This patch only calls the helper and sets .netns_assign_allow = true.

The reachability check added to virtio_transport_send_pkt() is only
described in the v4 changelog below the --- line, so it won't reach git
history. Could the commit message say that virtio_transport_send_pkt()
now drops packets from a netns that cannot reach the device and returns
-ENETUNREACH? That includes credit updates, shutdown packets and RST
replies.

> diff --git a/net/vmw_vsock/virtio_transport.c b/net/vmw_vsock/virtio_transport.c
> index eced6929405df..06df82b130cff 100644
> --- a/net/vmw_vsock/virtio_transport.c
> +++ b/net/vmw_vsock/virtio_transport.c
> @@ -256,6 +256,15 @@ virtio_transport_send_pkt(struct sk_buff *skb, struct net *net)
>  		goto out_rcu;
>  	}
>  
> +	/* A socket reset by an assign can still send credit updates from
> +	 * recvmsg() and setsockopt(SO_RCVLOWAT).
> +	 */
> +	if (!vsock_g2h_net_reachable(net)) {
> +		kfree_skb(skb);
> +		len = -ENETUNREACH;
> +		goto out_rcu;
> +	}
> +

[ ... ]

> @@ -682,10 +697,15 @@ static void virtio_transport_rx_work(struct work_struct *work)
>  
>  			virtio_transport_deliver_tap_pkt(skb);
>  

[Severity: High]
[Finding: main-6-6]
[Sources: opus-5-5]
This isn't a bug introduced by this patch, but this patch extends it to
the guest device, which can now be isolated. Can tap delivery leak the
assigned namespace's traffic to vsockmon devices in other namespaces?

virtio_transport_deliver_tap_pkt() runs before the netns-aware
virtio_transport_recv_pkt(). The call chain
vsock_deliver_tap()->__vsock_deliver_tap() then walks the global
vsock_tap_all list without any netns filter:

net/vmw_vsock/af_vsock_tap.c:__vsock_deliver_tap() {
    ...
	list_for_each_entry_rcu(tmp, &vsock_tap_all, list) {
		ret = __vsock_deliver_tap_skb(skb, tmp->dev);
    ...
}

This loop never compares dev_net(tmp->dev) with the packet's namespace.
vsockmon_link_ops has no netns restriction. RTM_NEWLINK only needs
CAP_NET_ADMIN in the user namespace that owns the netns.

So an unprivileged user can run unshare -Urn and create a vsockmon link.
The module autoloads through MODULE_ALIAS_RTNL_LINK("vsockmon"). The user
can then capture on that link with AF_PACKET.

Wouldn't that let any namespace see the traffic of the local-mode
namespace the guest device was assigned to? The tap paths in vhost-vsock
and vsock_loopback have the same problem.

> -			/* Force virtio-transport into global mode since it
> -			 * does not yet support local-mode namespacing.
> +			/* Only grab a reference if the net changes underneath
> +			 * us.
>  			 */
> -			virtio_transport_recv_pkt(&virtio_transport, skb, NULL);
> +			if (unlikely(!vsock_g2h_net_eq(net))) {
> +				put_net_track(net, &ns_tracker);
> +				net = vsock_g2h_net_get(&ns_tracker);
> +			}
> +
> +			virtio_transport_recv_pkt(t, 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 1e762a480df49..7efcbdc7e04fc 100644
> --- a/net/vmw_vsock/virtio_transport_common.c
> +++ b/net/vmw_vsock/virtio_transport_common.c

[ ... ]

> @@ -1478,9 +1478,14 @@ virtio_transport_recv_connecting(struct sock *sk,
>  
>  	switch (le16_to_cpu(hdr->op)) {
>  	case VIRTIO_VSOCK_OP_RESPONSE:
> -		sk->sk_state = TCP_ESTABLISHED;
> +		if (!vsock_maybe_set_connected(vsk)) {
> +			virtio_transport_reset_no_sock(t, skb, net);

[Severity: High]
[Finding: main-1-0]
[Sources: opus-5-5]
Can this RST ever reach the host?

virtio_transport_reset_no_sock() ends in t->send_pkt(reply, net). The
check this patch adds to virtio_transport_send_pkt() frees the reply when
net cannot reach the device:

	if (!vsock_g2h_net_reachable(net)) {
		kfree_skb(skb);
		len = -ENETUNREACH;
		goto out_rcu;
	}

The v5 changelog says the arrival netns is passed here instead of
sock_net(sk) so that the RST is not dropped. But vsock_net_check_mode()
looks like an equivalence relation:

	if (net_eq(ns0, ns1))
		return true;
	...
	return mode0 == VSOCK_NET_MODE_GLOBAL && mode0 == mode1;

A netns's mode is also fixed when the netns is created: vsock_net_init()
sets it, and later writes to the ns_mode sysctl return -EPERM.

virtio_transport_recv_pkt() finds the socket through
__vsock_find_{connected,bound}_socket_net(), which requires
vsock_net_check_mode(sock_net(sk), net). vsock_maybe_set_connected() only
fails when sock_net(sk) cannot reach the newly assigned g2h netns.

So the arrival netns cannot reach it either, and
vsock_g2h_net_reachable(net) is false whenever this branch runs.

An assign can land after the per-packet snapshot in
virtio_transport_rx_work() and before the check in
vsock_maybe_set_connected(). That is the race the commit message
describes.

In this OP_RESPONSE case the host already holds an ESTABLISHED socket. If
the RST is dropped, won't that connection stay half-open until the host
sends something? A host that only reads would wait forever.

The commit message says such a socket "will be reset", but only the
local socket is closed.

The same applies to the new failure path in
virtio_transport_recv_listen():

	if (!vsock_maybe_set_connected(vchild)) {
		release_sock(child);
		virtio_transport_reset_no_sock(t, skb, net);
		sock_put(child);
		return -ENETUNREACH;
	}

Here the host's connect() gets neither a RESPONSE nor an RST. It waits for
its connect timeout instead of failing with ECONNRESET.

By the same reasoning, the other virtio_transport_reset_no_sock() calls in
virtio_transport_recv_listen() that v5 moved from sock_net(sk) to net
behave the same either way.

Would getting the RST out need a different approach? For example, it
could be sent on the newly assigned g2h netns, or replies could be exempt
from the check in virtio_transport_send_pkt().

> +			skerr = ECONNRESET;
> +			err = -ENETUNREACH;
> +			goto close;
> +		}
> +
>  		sk->sk_socket->state = SS_CONNECTED;
> -		vsock_insert_connected(vsk);
>  		sk->sk_state_change(sk);
>  		break;

[ ... ]
-- 
This is an AI-generated review.