Re: [PATCH 3/3] hw/9pfs/xen: drain in-flight PDUs before xen-9p disconnect

[Christian Schoenebeck <qemu_oss@crudebyte.com>]

On Saturday, 18 July 2026 02:46:36 CEST Stefano Stabellini wrote:
> On Thu, 16 Jul 2026, Christian Schoenebeck wrote:
> > Hi Stefano,
> > 
> > do you might have a chance to look at this Xen patch?
> 
> Sorry for the delay

Thanks for looking at this, much appreciated!

> > On Thursday, 9 July 2026 15:50:36 CEST Christian Schoenebeck wrote:
> > > The xen-9p disconnect path has two issues:
> > > 
> > > 1. It frees the Xen9pfsRing structures while in-flight PDUs may still
> > > 
> > >    reference them via pdu->tag to index rings[]. This causes a UAF
> > >    in xen_9pfs_push_and_notify() when worker threads resume after
> > >    completing filesystem operations.
> > > 
> > > 2. It never calls v9fs_device_unrealize_common(), which means server
> > > 
> > >    state (struct LocalData, mountfd, FIDs) is never cleaned up on
> > >    disconnect, causing a resource leak on every guest-initiated
> > >    disconnect.
> > > 
> > > Fix both by draining in-flight PDUs via v9fs_reset() before tearing
> > > down rings, and calling v9fs_device_unrealize_common() to clean up
> > > server state.
> > > 
> > > Fixes: b37eeb0201 ("xen/9pfs: introduce Xen 9pfs backend")
> > > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> > > ---
> > > 
> > >  hw/9pfs/xen-9p-backend.c | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > > 
> > > diff --git a/hw/9pfs/xen-9p-backend.c b/hw/9pfs/xen-9p-backend.c
> > > index 24c90d97ec..3b7a68779a 100644
> > > --- a/hw/9pfs/xen-9p-backend.c
> > > +++ b/hw/9pfs/xen-9p-backend.c
> > > @@ -368,10 +368,14 @@ static void xen_9pfs_evtchn_event(void *opaque)
> > > 
> > >  static void xen_9pfs_disconnect(struct XenLegacyDevice *xendev)
> > >  {
> > >  
> > >      Xen9pfsDev *xen_9pdev = container_of(xendev, Xen9pfsDev, xendev);
> > > 
> > > +    V9fsState *s = &xen_9pdev->state;
> > > 
> > >      int i;
> > >      
> > >      trace_xen_9pfs_disconnect(xendev->name);
> > > 
> > > +    v9fs_reset(s);
> > > +    v9fs_device_unrealize_common(s);
> 
> While the patch addresses a real issue, xen_9pfs_disconnect() is also
> called from the error paths of xen_9pfs_pdu_vmarshal/vunmarshal, which
> run inside the PDU's own coroutine. v9fs_reset() is not safe to be
> called there.

Well, xen_9pfs_pdu_vmarshal() / xen_9pfs_pdu_vunmarshal() are both called on 
main thread, and xen_be_disconnect() has a guard that prevents the disconnect 
handler being called twice (more about that later), however you are right that 
these two functions run within coroutines. Which then leads to the following 
problem in v9fs_reset():

    while (!QLIST_EMPTY(&s->active_list)) {
        aio_poll(qemu_get_aio_context(), true);
    }

This loop would then run forever, as the coroutine calling v9fs_reset() is on 
the active_list, hence this list would never turn empty and the loop condition  
never turn false.

Furthermore aio_poll() is marked as no_coroutine_fn:

include/qemu/aio.h:bool no_coroutine_fn aio_poll(AioContext *ctx, bool 
blocking);

So aio_poll() should actually not be called in a coroutine context at all.
 
> We could remove the direct call to xen_9pfs_disconnect() from
> xen_9pfs_pdu_vmarshal/vunmarshal because they already change the state
> to XenbusStateClosing, which should result in the same

The problem with this approach is that xen_9pfs_pdu_vmarshal() / 
xen_9pfs_pdu_vunmarshal() would still call:

    xen_be_set_state(&xen_9pfs->xendev, XenbusStateClosing);

And the previously mentioned guard in xen_be_disconnect() for not calling the 
disconnect callback more than once, looks like this:

    if (xendev->be_state != XenbusStateClosing &&
        xendev->be_state != XenbusStateClosed  &&
        xendev->ops->disconnect) {
        xendev->ops->disconnect(xendev);
    }

Hence if this (v)unmarshal error path was triggered, the disconnect handler 
would then actually never be executed. And that's apparently the reason why 
these two functions do call xen_9pfs_disconnect() explicitly in their error 
path right now.

What I think might work though, was replacing the current direct 
xen_9pfs_disconnect() calls by deferred ones, something like:

    aio_bh_schedule_oneshot(... xen_9pfs_disconnect ...);

Because xen_9pfs_disconnect() would then run both on main thread and not run 
in a coroutine context. So it should turn it safe.

I'll prepare a v2 with the latter solution.

/Christian