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 <[email protected]> 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.
