Message-ID: <g5dcyor4aryvtcnqxm5aekldbettetlmog3c7sj7sjx3yp2pgy@hcpxyubied2n>
Date: Thu, 13 Nov 2025 16:24:44 +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 Wed, Nov 12, 2025 at 10:27:18AM -0800, Bobby Eshleman wrote:
>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)

Interesting, but what about if we call skb_set_owner_sk_safe() in 
vsock_loopback.c just before calling virtio_transport_recv_pkt() for 
every skb?

Maybe we should refactor a bit virtio_transport_recv_pkt() e.g. moving 
`skb_set_owner_sk_safe()` to be sure it's called only when we are sure 
it's the right socket (e.g. after checking SOCK_DONE).

WDYT?

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

Yeah, let's keep this as plan B for now :-)

Thanks,
Stefano

Powered by blists - more mailing lists