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

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 (1) · Medium: 1 · Low: 2

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] A socket closed by device reassignment can still send packets
  through the device it supposedly lost access to.
- [Low] The implementation-notes comment at the top of
  net/vmw_vsock/af_vsock.c still gives virtio-vsock as the example of a…
- [Low] The dev-netns-set uAPI doc in
  Documentation/netlink/specs/vsock.yaml (added by the earlier commit…

Pre-existing issues:
- [High] This problem was not introduced by this patch, but the patch's
  new isolation can be bypassed through vsock packet capture.

--

vsock/virtio: support guest device network namespace

This commit makes the virtio-vsock guest device assignable to a network
namespace. It re-reads the assigned netns for each packet in
virtio_transport_rx_work(). It also uses vsock_maybe_set_connected() so
that new connections are added to the connected table atomically with
respect to namespace assignment.

> diff --git a/net/vmw_vsock/virtio_transport.c b/net/vmw_vsock/virtio_transport.c
> index eced6929405df..4123924536dd6 100644
> --- a/net/vmw_vsock/virtio_transport.c
> +++ b/net/vmw_vsock/virtio_transport.c
> @@ -542,7 +542,7 @@ static bool virtio_transport_msgzerocopy_allow(void)
>  
>  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)));
>  }

[Severity: Medium]
virtio_transport_stream_allow() gates new connections. Now that the
device can be assigned, can a socket closed by
vsock_g2h_reset_unreachable() still put packets on the device?

The sweep only changes sk_state and sk_err, then removes the socket from
the connected table:

net/vmw_vsock/af_vsock.c:vsock_g2h_reset_unreachable() {
    ...
			sk->sk_state = TCP_CLOSE;
			sk->sk_err = ECONNRESET;
			sk_error_report(sk);

			__vsock_remove_connected(vsk);
    ...
}

It does not touch vsk->transport or sock->state, which stays
SS_CONNECTED. vsock_shutdown() only rejects SS_UNCONNECTED:

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;
    ...
			vsock_send_shutdown(sk, mode);
    ...
}

So calling shutdown() on the old fd goes through:

vsock_send_shutdown()
  virtio_transport_shutdown()
    virtio_transport_send_pkt_info()
      virtio_transport_send_pkt()

virtio_transport_send_pkt() never uses its net argument before it queues
the packet on the device. sock->state never goes back to SS_UNCONNECTED,
so this can be repeated.

The wire header carries only CIDs and ports. Suppose the newly assigned
local namespace reuses the same guest/host port tuple. Could these
SHUTDOWN packets then tear down a host connection that now belongs to
the new owner?

Even without a tuple collision, the namespace that lost the device can
still send traffic on it. That seems to contradict the documented rule
"assigned to a local mode namespace - only that namespace may use it".

[ ... ]

> @@ -587,6 +587,7 @@ static struct virtio_transport virtio_transport = {
>  		.seqpacket_has_data       = virtio_transport_seqpacket_has_data,
>  
>  		.msgzerocopy_allow        = virtio_transport_msgzerocopy_allow,
> +		.netns_assign_allow       = true,

[Severity: Low]
This isn't a bug, but the implementation notes at the top of
net/vmw_vsock/af_vsock.c still use virtio-vsock as the example of a
transport without local mode support:

 *       a different transport that *does* support local mode. For
 *       example, virtio-vsock may not support local mode, but the socket
 *       may still accept a connection from vhost-vsock which does.

This patch sets netns_assign_allow, and virtio_transport_rx_work() now
delivers into the assigned namespace. So virtio-vsock can serve a local
mode namespace once the device is assigned.

Should this example be updated? It also seems to conflict with the
sysctl doc, which says "Of the in-tree guest transports, only
virtio-vsock implements it."

[Severity: Low]
This is the first transport to set netns_assign_allow, so the
dev-netns-set behaviour becomes reachable with this patch. The uAPI doc
in Documentation/netlink/specs/vsock.yaml, added earlier in this series,
says:

        Movement to an unreachable local namespace results in sockets being
        closed with ECONNRESET. Peers are not guaranteed to be notified via
        an RST.

However, vsock_g2h_reset_unreachable() closes every socket that fails
vsock_g2h_reachable_sk() after any move:

			if (vsk->transport != t ||
			    sk->sk_state == TCP_CLOSE ||
			    vsock_g2h_reachable_sk(vsk))
				continue;

That includes moving the device out of a local namespace L and back to
init_net or another global namespace. vsock_net_check_mode(L, init_net)
is false, so every connected socket in L is closed with ECONNRESET.

Should the yaml text also cover moves out of a local namespace? The
sysctl doc already does ("from a namespace that can no longer reach the
device").

[ ... ]

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

[Severity: High]
This is a pre-existing issue and was not introduced by this patch. Can
the vsock tap path bypass the isolation that this patch adds?

virtio_transport_deliver_tap_pkt() runs for every received packet,
before any namespace is applied. vsock_deliver_tap() then clones the
packet, payload included, to every entry on the global vsock_tap_all
list:

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);
    ...
}

vsock_add_tap(), called from vsockmon_open(), only checks the device
type:

	if (unlikely(vt->dev->type != ARPHRD_VSOCKMON))
		return -EINVAL;

Nothing on the tap path filters with net_eq() or vsock_net_check_mode().
vsockmon has no netns restriction either. So CAP_NET_ADMIN over any
netns, including one owned by a user namespace, is enough to create a
vsockmon device, bring it up and capture on it with AF_PACKET.

Once the guest device is assigned to a local mode namespace, can any
other namespace still read all guest<->host vsock traffic this way?

The TX side in virtio_transport_send_pkt_work() appears to have the same
exposure, and vhost-vsock local mode already had it. By contrast, nlmon
taps are kept per netns.

>  
> -			/* 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);

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261012-vsock-guest-ns-v3-0-b32447c66b23%40meta.com