Message-ID: <g6bxp6hketbjrddzni2ln37gsezqvxbu2orheorzh7fs66roll@hhcrgsos3ui3>
Date: Wed, 12 Nov 2025 15:19:47 +0100
From: Stefano Garzarella <sgarzare@...hat.com>
To: Bobby Eshleman <bobbyeshleman@...il.com>
Cc: Shuah Khan <shuah@...nel.org>, "David S. Miller" <davem@...emloft.net>, 
	Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, 
	Paolo Abeni <pabeni@...hat.com>, Simon Horman <horms@...nel.org>, 
	Stefan Hajnoczi <stefanha@...hat.com>, "Michael S. Tsirkin" <mst@...hat.com>, 
	Jason Wang <jasowang@...hat.com>, Xuan Zhuo <xuanzhuo@...ux.alibaba.com>, 
	Eugenio Pérez <eperezma@...hat.com>, "K. Y. Srinivasan" <kys@...rosoft.com>, 
	Haiyang Zhang <haiyangz@...rosoft.com>, Wei Liu <wei.liu@...nel.org>, Dexuan Cui <decui@...rosoft.com>, 
	Bryan Tan <bryan-bt.tan@...adcom.com>, Vishnu Dasa <vishnu.dasa@...adcom.com>, 
	Broadcom internal kernel review list <bcm-kernel-feedback-list@...adcom.com>, virtualization@...ts.linux.dev, netdev@...r.kernel.org, 
	linux-kselftest@...r.kernel.org, linux-kernel@...r.kernel.org, kvm@...r.kernel.org, 
	linux-hyperv@...r.kernel.org, Sargun Dhillon <sargun@...gun.me>, berrange@...hat.com, 
	Bobby Eshleman <bobbyeshleman@...a.com>
Subject: Re: [PATCH net-next v9 06/14] vsock/loopback: add netns support

On Tue, Nov 11, 2025 at 10:54:48PM -0800, Bobby Eshleman wrote:
>From: Bobby Eshleman <bobbyeshleman@...a.com>
>
>Add NS support to vsock loopback. Sockets in a global mode netns
>communicate with each other, regardless of namespace. Sockets in a local
>mode netns may only communicate with other sockets within the same
>namespace.
>
>Signed-off-by: Bobby Eshleman <bobbyeshleman@...a.com>
>---
>Changes in v9:
>- remove per-netns vsock_loopback and workqueues, just re-using
>  the net and net_mode in skb->cb achieved the same result in a simpler
>  way. Also removed need for pernet_subsys.
>- properly track net references
>
>Changes in v7:
>- drop for_each_net() init/exit, drop net_rwsem, the pernet registration
>  handles this automatically and race-free
>- flush workqueue before destruction, purge pkt list
>- remember net_mode instead of current net mode
>- keep space after INIT_WORK()
>- change vsock_loopback in netns_vsock to ->priv void ptr
>- rename `orig_net_mode` to `net_mode`
>- remove useless comment
>- protect `register_pernet_subsys()` with `net_rwsem`
>- do cleanup before releasing `net_rwsem` when failure happens
>- call `unregister_pernet_subsys()` in `vsock_loopback_exit()`
>- call `vsock_loopback_deinit_vsock()` in `vsock_loopback_exit()`
>
>Changes in v6:
>- init pernet ops for vsock_loopback module
>- vsock_loopback: add space in struct to clarify lock protection
>- do proper cleanup/unregister on vsock_loopback_exit()
>- vsock_loopback: use virtio_vsock_skb_net()
>
>Changes in v5:
>- add callbacks code to avoid reverse dependency
>- add logic for handling vsock_loopback setup for already existing
>  namespaces
>---
> net/vmw_vsock/vsock_loopback.c | 41 ++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 40 insertions(+), 1 deletion(-)
>
>diff --git a/net/vmw_vsock/vsock_loopback.c b/net/vmw_vsock/vsock_loopback.c
>index d3ac056663ea..e62f6c516992 100644
>--- a/net/vmw_vsock/vsock_loopback.c
>+++ b/net/vmw_vsock/vsock_loopback.c
>@@ -32,6 +32,9 @@ static int vsock_loopback_send_pkt(struct sk_buff *skb, struct net *net,
> 	struct vsock_loopback *vsock = &the_vsock_loopback;
> 	int len = skb->len;
>
>+	virtio_vsock_skb_set_net(skb, net);
>+	virtio_vsock_skb_set_net_mode(skb, net_mode);
>+
> 	virtio_vsock_skb_queue_tail(&vsock->pkt_queue, skb);
> 	queue_work(vsock->workqueue, &vsock->pkt_work);
>
>@@ -116,8 +119,10 @@ static void vsock_loopback_work(struct work_struct *work)
> {
> 	struct vsock_loopback *vsock =
> 		container_of(work, struct vsock_loopback, pkt_work);
>+	enum vsock_net_mode net_mode;
> 	struct sk_buff_head pkts;
> 	struct sk_buff *skb;
>+	struct net *net;
>
> 	skb_queue_head_init(&pkts);
>
>@@ -131,7 +136,41 @@ static void vsock_loopback_work(struct work_struct *work)
> 		 */
> 		virtio_transport_consume_skb_sent(skb, false);
> 		virtio_transport_deliver_tap_pkt(skb);
>-		virtio_transport_recv_pkt(&loopback_transport, skb, NULL, 0);
>+
>+		/* In the case of virtio_transport_reset_no_sock(), the skb
>+		 * does not hold a reference on the socket, and so does not
>+		 * transitively hold a reference on the net.
>+		 *
>+		 * There is an ABA race condition in this sequence:
>+		 * 1. the sender sends a packet
>+		 * 2. worker calls virtio_transport_recv_pkt(), using the
>+		 *    sender's net
>+		 * 3. virtio_transport_recv_pkt() uses t->send_pkt() passing the
>+		 *    sender's net
>+		 * 4. virtio_transport_recv_pkt() free's the skb, dropping the
>+		 *    reference to the socket
>+		 * 5. the socket closes, frees its reference to the net
>+		 * 6. Finally, the worker for the second t->send_pkt() call
>+		 *    processes the skb, and uses the now stale net pointer for
>+		 *    socket lookups.
>+		 *
>+		 * To prevent this, we acquire a net reference in vsock_loopback_send_pkt()
>+		 * and hold it until virtio_transport_recv_pkt() completes.
>+		 *
>+		 * Additionally, we must grab a reference on the skb before
>+		 * calling virtio_transport_recv_pkt() to prevent it from
>+		 * freeing the skb before we have a chance to release the net.
>+		 */
>+		net_mode = virtio_vsock_skb_net_mode(skb);
>+		net = virtio_vsock_skb_net(skb);

Wait, we are adding those just for loopback (in theory used only for
testing/debugging)? And only to support virtio_transport_reset_no_sock() 
use case?

Honestly I don't like this, do we have any alternative?

I'll also try to think something else.

Stefano

>+
>+		skb_get(skb);
>+
>+		virtio_transport_recv_pkt(&loopback_transport, skb, net,
>+					  net_mode);
>+
>+		virtio_vsock_skb_clear_net(skb);
>+		kfree_skb(skb);
> 	}
> }
>
>
>-- 
>2.47.3
>

Powered by blists - more mailing lists