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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] vsock/virtio: virtio_transport_send_pkt() drops RST replies on netns change
--

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

vsock/virtio: support guest device network namespace

This commit adds network namespace assignment support to the guest
virtio-vsock driver, restricting reachability to the assigned namespace and
atomically checking reachability when inserting sockets into the connected
table.

> 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;
> +	}
> +
>  	/* If send_pkt_queue is empty, we can safely bypass this queue
>  	 * because packet order is maintained and (try) to put the packet
>  	 * on the virtqueue using virtio_transport_send_skb_fast_path.

[ ... ]

> @@ -682,10 +697,15 @@ 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.
> +			/* 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
> @@ -1468,8 +1468,8 @@ void virtio_transport_release(struct vsock_sock *vsk)
>  EXPORT_SYMBOL_GPL(virtio_transport_release);
>  
>  static int
> -virtio_transport_recv_connecting(struct sock *sk,
> -				 struct sk_buff *skb)
> +virtio_transport_recv_connecting(struct sock *sk, struct sk_buff *skb,
> +				 struct virtio_transport *t, struct net *net)
>  {
>  	struct virtio_vsock_hdr *hdr = virtio_vsock_hdr(skb);
>  	struct vsock_sock *vsk = vsock_sk(sk);
> @@ -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);
> +			skerr = ECONNRESET;
> +			err = -ENETUNREACH;
> +			goto close;
> +		}
> +
>  		sk->sk_socket->state = SS_CONNECTED;
> -		vsock_insert_connected(vsk);
>  		sk->sk_state_change(sk);
>  		break;

[ ... ]

> @@ -1750,7 +1754,7 @@ virtio_transport_recv_listen(struct sock *sk, struct sk_buff *skb,
>  	 */
>  	if (ret || vchild->transport != &t->transport) {
>  		release_sock(child);
> -		virtio_transport_reset_no_sock(t, skb, sock_net(sk));
> +		virtio_transport_reset_no_sock(t, skb, net);
>  		sock_put(child);
>  		return ret;
>  	}
> @@ -1758,7 +1762,13 @@ 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);
> +	if (!vsock_maybe_set_connected(vchild)) {
> +		release_sock(child);
> +		virtio_transport_reset_no_sock(t, skb, net);
> +		sock_put(child);
> +		return -ENETUNREACH;
> +	}

[Severity: Medium]
As reported by Stefano Garzarella on v4 [1], will the VIRTIO_VSOCK_OP_RST
replies sent by virtio_transport_reset_no_sock(t, skb, net) still get dropped
by virtio_transport_send_pkt() when a namespace assignment races with an
incoming packet?

In virtio_transport_rx_work(), net is snapshotted via vsock_g2h_net_get()
before calling virtio_transport_recv_pkt(t, skb, net). Inside
virtio_transport_recv_pkt(), the socket sk is looked up with
vsock_find_connected_socket_net() or vsock_find_bound_socket_net(), both of
which require vsock_net_check_mode(sock_net(sk), net) to be true.

Because a namespace's vsock mode is fixed after initialization, sock_net(sk)
(as well as sock_net(child) in virtio_transport_recv_listen()) and the
snapshot net passed to virtio_transport_recv_pkt() always have the same
reachability with respect to vsock_g2h_net.

If a concurrent vsock_g2h_net_assign() changes vsock_g2h_net to an
unreachable namespace after the socket lookup in virtio_transport_recv_pkt()
(for instance while acquiring lock_sock(sk) or setting up the child socket):

1. In virtio_transport_recv_connecting(), vsock_maybe_set_connected(vsk)
   returns false because sock_net(sk) is no longer reachable from the new
   vsock_g2h_net.
2. In virtio_transport_recv_listen(), vsock_assign_transport(vchild, vsk) or
   vsock_maybe_set_connected(vchild) fails for the same reason.
3. In virtio_transport_recv_pkt(), if vsock_g2h_reset_unreachable() swept sk
   before lock_sock(sk) completed, sock_flag(sk, SOCK_DONE) is set.

All of these paths call:

  virtio_transport_reset_no_sock(t, skb, net)
    -> t->send_pkt(reply, net)
      -> virtio_transport_send_pkt(reply, net)

However, net is still the pre-assignment namespace snapshot from
virtio_transport_rx_work(). When virtio_transport_send_pkt() checks
vsock_g2h_net_reachable(net) against the newly assigned vsock_g2h_net, it
evaluates to false, freeing the RST skb and returning -ENETUNREACH without
sending the reset to the peer.

Should virtio_transport_send_pkt() allow outgoing VIRTIO_VSOCK_OP_RST packets
(or should virtio_transport_reset_no_sock() bypass the net reachability
check) so the peer receives the reset when a handshake or connection races
with namespace assignment?

[1] https://lore.kernel.org/r/atjvzn-zPYJjLAhy@sgarzare-redhat

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103-vsock-guest-ns-v5-0-48179b9b66b8@meta.com?part=5