Message-ID: <aRTRhk/ok06YKTEu@devvm11784.nha0.facebook.com>
Date: Wed, 12 Nov 2025 10:27:18 -0800
From: Bobby Eshleman <bobbyeshleman@...il.com>
To: Stefano Garzarella <sgarzare@...hat.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 Wed, Nov 12, 2025 at 03:19:47PM +0100, Stefano Garzarella wrote:
> 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?

Yes, exactly, only loopback + reset_no_sock(). The issue doesn't exist
for vhost-vsock because vhost_vsock holds a net reference, and it
doesn't exist for non-reset_no_sock calls because after looking up the
socket we transfer skb ownership to it, which holds down the skb -> sk ->
net reference chain.

> 
> Honestly I don't like this, do we have any alternative?
> 
> I'll also try to think something else.
> 
> Stefano


I've been thinking about this all morning... maybe
we can do something like this:

```

virtio_transport_recv_pkt(...,  struct sock *reply_sk) {... }

virtio_transport_reset_no_sock(..., reply_sk)
{
	if (reply_sk)
		skb_set_owner_sk_safe(reply, reply_sk)

	t->send_pkt(reply);
}

vsock_loopback_work(...)
{
	virtio_transport_recv_pkt(..., skb, skb->sk);
}


for other transports:

	virtio_transport_recv_pkt(..., skb, NULL);

```

This way 'reply' keeps the sk and sk->net alive even after
virtio_transport_recv_pkt() frees 'skb'. The net won't be released until
after 'reply' is freed back on the other side, removing the race.

It makes semantic sense too... for loopback, we already know which sk
the reply is going back to. For other transports, we don't because
they're across the virt boundary.

WDYT?

I hate to suggest this, but another option might be to just do nothing?
In order for this race to have any real effect, a loopback socket must
send a pkt to a non-existent socket, immediately close(), then the
namespace deleted, a new namespace created with the same pointer
address, and finally a new socket with the same port created in that
namespace, all before the reply RST reaches recv_pkt()... at which point
the newly created socket would wrongfully receive the RST.

Best,
Bobby

Powered by blists - more mailing lists