On Thu, Nov 13, 2025 at 04:24:44PM +0100, Stefano Garzarella wrote:
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
[...]
@@ -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?
I think the issue with this is that at the time vsock_loopback calls virtio_transport_recv_pkt() the reply skb hasn't yet been allocated by virtio_transport_reset_no_sock() and we can't wait for it to return because the original skb may be freed by then.
We might be able to keep it all in vsock_loopback if we removed the need to use the original skb or sk by just using the net. But to do that we would need to add a netns_tracker per net somewhere. I guess that would end up in a list or hashmap in struct vsock_loopback.
Another option that does simplify a little, but unfortunately still doesn't keep everything in loopback:
@@ -1205,7 +1205,7 @@ static int virtio_transport_reset_no_sock(const struct virtio_transport *t, if (!reply) return -ENOMEM;
- return t->send_pkt(reply, net, net_mode); + return t->send_pkt(reply, net, net_mode, skb->sk); }
@@ -27,11 +27,16 @@ static u32 vsock_loopback_get_local_cid(void) }
static int vsock_loopback_send_pkt(struct sk_buff *skb, struct net *net, - enum vsock_net_mode net_mode) + enum vsock_net_mode net_mode, + struct sock *rst_owner) { struct vsock_loopback *vsock = &the_vsock_loopback; int len = skb->len;
+ if (!skb->sk && rst_owner) + WARN_ONCE(!skb_set_owner_sk_safe(skb, rst_owner), + "loopback socket has sk_refcnt == 0\n"); + virtio_vsock_skb_queue_tail(&vsock->pkt_queue, skb); queue_work(vsock->workqueue, &vsock->pkt_work);
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?
I agree, it is called a little prematurely.
Thanks, Bobby