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