On Thu, Nov 13, 2025 at 10:26:04AM -0800, Bobby Eshleman wrote:
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 <[email protected]>
> > >
> > > 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 <[email protected]>
[...]
> > > @@ -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.
Right!
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");
+
This doesn't seem too bad IMO, but at this point, why we can't do that
in virtio_transport_reset_no_sock() for any kind of transport?
I mean, in any case the RST packet should be handled by the same net of
the "sender", no?
At this point, can we just put the `vsk` of the sender in the `info` and
virtio_transport_alloc_skb() will already do that.
WDYT?
Am I missing something?
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.
Yep, but I'll leave this out for now :-)
Thanks,
Stefano
