Re: [PATCH net-next v4 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 8627fb28e7c64d1d9b02b254ab9dca7140468f14
Author: Bobby Eshleman <bobbyeshleman@meta.com>

vsock/virtio: support guest device network namespace

This commit enables network namespace assignment for the virtio-vsock guest
device, updating reachability checks in the virtio transport and re-reading
the assigned namespace per packet in virtio_transport_rx_work(). It also
uses vsock_maybe_set_connected() when transitioning sockets to the connected
state so sockets that lose device reachability during namespace reassignment
are reset.

> 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.

[ ... ]

> diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
> index 1e762a480df49..0c864db2bc65b 100644
> --- a/net/vmw_vsock/virtio_transport_common.c
> +++ b/net/vmw_vsock/virtio_transport_common.c
> @@ -1478,9 +1478,13 @@ 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)) {
> +			skerr = ECONNRESET;
> +			err = -ENETUNREACH;
> +			goto destroy;
> +		}
> +
>  		sk->sk_socket->state = SS_CONNECTED;
> -		vsock_insert_connected(vsk);
>  		sk->sk_state_change(sk);
>  		break;
>  	case VIRTIO_VSOCK_OP_INVALID:

[ ... ]

> @@ -1758,7 +1760,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, sock_net(sk));
> +		sock_put(child);
> +		return -ENETUNREACH;
> +	}
> +
>  	vsock_enqueue_accept(sk, child);
>  	virtio_transport_send_response(vchild, skb);

[Severity: Medium]
Will virtio_transport_send_pkt() drop the VIRTIO_VSOCK_OP_RST packets sent
when a connection handshake races with a device namespace change?

In net/vmw_vsock/af_vsock.c, vsock_maybe_set_connected() checks reachability
via vsock_g2h_reachable_sk():

    spin_lock_bh(&vsock_table_lock);
    reachable = vsock_g2h_reachable_sk(vsk);
    if (reachable) {
            sk_vsock(vsk)->sk_state = TCP_ESTABLISHED;
            __vsock_insert_connected(list, vsk);
    }
    spin_unlock_bh(&vsock_table_lock);

For virtio_transport, vsock_g2h_reachable_sk(vsk) returns false when
vsock_g2h_net_reachable(sock_net(sk_vsock(vsk))) is false.

When vsock_maybe_set_connected() returns false in
virtio_transport_recv_connecting(), jumping to destroy calls:

    virtio_transport_reset(vsk, skb)
      -> virtio_transport_send_pkt_info(vsk, &info) /* info.net = sock_net(sk) */
        -> virtio_transport_send_pkt(skb, info.net)

Similarly, in virtio_transport_recv_listen(), when vsock_maybe_set_connected()
or vsock_assign_transport() fails after a namespace change, it calls:

    virtio_transport_reset_no_sock(t, skb, sock_net(sk))
      -> virtio_transport_send_pkt(reply, sock_net(sk))

The same also happens in virtio_transport_recv_pkt() if an established socket
is swept to TCP_CLOSE by vsock_g2h_reset_unreachable() before lock_sock(sk)
acquires the socket lock, hitting the default case and calling
virtio_transport_reset_no_sock(t, skb, net).

Because virtio_transport_send_pkt() in net/vmw_vsock/virtio_transport.c now
checks !vsock_g2h_net_reachable(net) and frees the skb with -ENETUNREACH,
won't all of these outgoing VIRTIO_VSOCK_OP_RST replies be dropped before
reaching the host peer, leaving a guest-initiated connection half-open on the
host or causing a host-initiated connect() to hang until timeout?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261020-vsock-guest-ns-v4-0-047527cfccfc@meta.com?part=5