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

[Christian Schoenebeck <qemu_oss@crudebyte.com>]

On Friday, 24 July 2026 09:49:57 CEST Philippe Mathieu-Daudé wrote:
> On 23/7/26 14:43, 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.
> > 
> > Additionally, explicit calls of xen_9pfs_disconnect() in the error
> > paths of xen_9pfs_pdu_vmarshal() and xen_9pfs_pdu_vunmarshal() must
> > be deferred (via aio_bh_schedule_oneshot()), because
> > xen_9pfs_pdu_v(un)marshal() are running within a coroutine context
> > which makes them unsafe [1] for calling v9fs_reset() directly, as
> > 
> > the latter e.g. has a loop like:
> >      while (!QLIST_EMPTY(&s->active_list)) {
> >      
> >          aio_poll(qemu_get_aio_context(), true);
> >      
> >      }
> > 
> > which would a) never terminate (as the coroutine is on the
> > active_list) and b) aio_poll() is marked as no_coroutine_fn.
> > 
> > [1] https://lore.kernel.org/qemu-devel/3351181.5fSG56mABF@weasel/
> > 
> > And finally, add an idempotent guard to xen_9pfs_disconnect()
> > for the v9fs_reset(s) and v9fs_device_unrealize_common(s) calls
> > specifically [2], just to be sure.
> > 
> > [2]
> > https://lore.kernel.org/qemu-devel/alpine.DEB.2.22.394.2607221815520.5295
> > @ubuntu-linux-20-04-desktop/
> > 
> > Fixes: b37eeb0201 ("xen/9pfs: introduce Xen 9pfs backend")
> > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> > Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> > ---
> > 
> >   hw/9pfs/9p.c             |  1 +
> >   hw/9pfs/xen-9p-backend.c | 17 +++++++++++++++--
> >   2 files changed, 16 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> > index 3119f01117..fc01791830 100644
> > --- a/hw/9pfs/9p.c
> > +++ b/hw/9pfs/9p.c
> > @@ -4530,6 +4530,7 @@ void v9fs_device_unrealize_common(V9fsState *s)
> > 
> >       qp_table_destroy(&s->qpp_table);
> >       qp_table_destroy(&s->qpf_table);
> >       g_free(s->ctx.fs_root);
> > 
> > +    s->transport = NULL;
> > 
> >   }
> >   
> >   typedef struct VirtfsCoResetData {
> > 
> > diff --git a/hw/9pfs/xen-9p-backend.c b/hw/9pfs/xen-9p-backend.c
> > index 24c90d97ec..a2f118d2ca 100644
> > --- a/hw/9pfs/xen-9p-backend.c
> > +++ b/hw/9pfs/xen-9p-backend.c
> > @@ -68,6 +68,11 @@ typedef struct Xen9pfsDev {
> > 
> >   static void xen_9pfs_disconnect(struct XenLegacyDevice *xendev);
> > 
> > +static void xen_9pfs_disconnect_bh(void *opaque)
> > +{
> > +    xen_9pfs_disconnect(opaque);
> > +}
> > +
> > 
> >   static void xen_9pfs_in_sg(Xen9pfsRing *ring,
> >   
> >                              struct iovec *in_sg,
> >                              int *num,
> > 
> > @@ -150,7 +155,8 @@ static ssize_t xen_9pfs_pdu_vmarshal(V9fsPDU *pdu,
> > 
> >                         "Failed to encode VirtFS reply type %d\n",
> >                         pdu->id + 1);
> >           
> >           xen_be_set_state(&xen_9pfs->xendev, XenbusStateClosing);
> > 
> > -        xen_9pfs_disconnect(&xen_9pfs->xendev);
> > +        aio_bh_schedule_oneshot(qemu_get_aio_context(),
> > +                                xen_9pfs_disconnect_bh,
> > &xen_9pfs->xendev);> 
> >       }
> >       return ret;
> >   
> >   }
> > 
> > @@ -173,7 +179,8 @@ static ssize_t xen_9pfs_pdu_vunmarshal(V9fsPDU *pdu,
> > 
> >           xen_pv_printf(&xen_9pfs->xendev, 0,
> >           
> >                         "Failed to decode VirtFS request type %d\n",
> >                         pdu->id);
> >           
> >           xen_be_set_state(&xen_9pfs->xendev, XenbusStateClosing);
> > 
> > -        xen_9pfs_disconnect(&xen_9pfs->xendev);
> > +        aio_bh_schedule_oneshot(qemu_get_aio_context(),
> > +                                xen_9pfs_disconnect_bh,
> > &xen_9pfs->xendev);> 
> >       }
> >       return ret;
> >   
> >   }
> > 
> > @@ -368,10 +375,16 @@ 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);
> > 
> > +    if (s->transport) {
> > +        v9fs_reset(s);
> 
> Maybe v9fs_reset() is misnamed.
> 
> What about v9fs_cancel_in_flight()?

The name v9fs_cancel_in_flight() would be too narrow. v9fs_reset() does two 
things:

  1. draining all in-flight PDUs

  2. freeing all FIDs (and their associated hash table)

Therefore IMO v9fs_reset() more appropriately describes a reset of server 
state. v9fs_reset() already exists for almost 10 years:

  commit 0e44a0fd3f28cccb8963fdfc05c53c546b3f46b6
  Author: Greg Kurz <groug@kaod.org>
  Date:   Mon Oct 17 14:13:58 2016 +0200

      virtio-9p: add reset handler
    
      Virtio devices should implement the VirtIODevice->reset() function to
      perform necessary cleanup actions and to bring the device to a quiescent
      state.
    
      In the case of the virtio-9p device, this means:
      - emptying the list of active PDUs (i.e. draining all in-flight I/O)
      - freeing all fids (i.e. close open file descriptors and free memory)

      ...

And even though the aspect of draining in-flight PDUs is focused and relevant 
for this particular security patch to prevent the mentioned UAF, it is still 
correct and expected to do a full server reset here.

> 
> > +        v9fs_device_unrealize_common(s);
> 
> IIUC xen_9pfs_disconnect() is mixing XenDevOps::disconnect()
> and XenDevOps::free()?

While you are correct with your observation that it does mix the two things, I 
fear this is required right now. Separating them would be cleaner and 
possible, but it would require significant changes. And if you review my 
discussions with Stefano in the previous versions of this series, you might 
realize that this code is somewhat tricky and not trivial.

And finally:

1. These patches are addressing real security issues.

2. Your concerns are pure code organizational issues, no behaviour issues.

3. These structural issues exist for many (9?) years.

4. Changing this struture might introduce new issues.

5. We are already very close to release.

6. This series (v1) was posted already 3 weeks ago.

7. I have a bunch of other important fixes in my queue that I should send a PR

So we can probably agree that this should not be within the scope of these 
security patches?
 
> > +    }
> > +
> > 
> >       for (i = 0; i < xen_9pdev->num_rings; i++) {
> >       
> >           if (xen_9pdev->rings[i].evtchndev != NULL) {
> >           
> >               qemu_set_fd_handler(qemu_xen_evtchn_fd(xen_9pdev->rings[i].e
> >               vtchndev),