Re: [PATCH v2 RESEND] xen: Fix SEGV on domain disconnect

[Mark Syms <mark.syms@cloud.com>]

[-- Attachment #1: Type: text/plain, Size: 3804 bytes --]

Copying in Tim who did the final phase of the changes.

On Mon, 24 Apr 2023 at 11:32, Paul Durrant <xadimgnik@gmail.com> wrote:
>
> On 20/04/2023 12:02, mark.syms@citrix.com wrote:
> > From: Mark Syms <mark.syms@citrix.com>
> >
> > Ensure the PV ring is drained on disconnect. Also ensure all pending
> > AIO is complete, otherwise AIO tries to complete into a mapping of the
> > ring which has been torn down.
> >
> > Signed-off-by: Mark Syms <mark.syms@citrix.com>
> > ---
> > CC: Stefano Stabellini <sstabellini@kernel.org>
> > CC: Anthony Perard <anthony.perard@citrix.com>
> > CC: Paul Durrant <paul@xen.org>
> > CC: xen-devel@lists.xenproject.org
> >
> > v2:
> >   * Ensure all inflight requests are completed before teardown
> >   * RESEND to fix formatting
> > ---
> >   hw/block/dataplane/xen-block.c | 31 +++++++++++++++++++++++++------
> >   1 file changed, 25 insertions(+), 6 deletions(-)
> >
> > diff --git a/hw/block/dataplane/xen-block.c
b/hw/block/dataplane/xen-block.c
> > index 734da42ea7..d9da4090bf 100644
> > --- a/hw/block/dataplane/xen-block.c
> > +++ b/hw/block/dataplane/xen-block.c
> > @@ -523,6 +523,10 @@ static bool
xen_block_handle_requests(XenBlockDataPlane *dataplane)
> >
> >       dataplane->more_work = 0;
> >
> > +    if (dataplane->sring == 0) {
> > +        return done_something;
> > +    }
> > +
>
> I think you could just return false here... Nothing is ever going to be
> done if there's no ring :-)
>
> >       rc = dataplane->rings.common.req_cons;
> >       rp = dataplane->rings.common.sring->req_prod;
> >       xen_rmb(); /* Ensure we see queued requests up to 'rp'. */
> > @@ -666,14 +670,35 @@ void
xen_block_dataplane_destroy(XenBlockDataPlane *dataplane >   void
xen_block_dataplane_stop(XenBlockDataPlane *dataplane)
> >   {
> >       XenDevice *xendev;
> > +    XenBlockRequest *request, *next;
> >
> >       if (!dataplane) {
> >           return;
> >       }
> >
> > +    /* We're about to drain the ring. We can cancel the scheduling of
any
> > +     * bottom half now */
> > +    qemu_bh_cancel(dataplane->bh);
> > +
> > +    /* Ensure we have drained the ring */
> > +    aio_context_acquire(dataplane->ctx);
> > +    do {
> > +        xen_block_handle_requests(dataplane);
> > +    } while (dataplane->more_work);
> > +    aio_context_release(dataplane->ctx);
> > +
>
> I don't think we want to be taking new requests, do we?
>
> > +    /* Now ensure that all inflight requests are complete */
> > +    while (!QLIST_EMPTY(&dataplane->inflight)) {
> > +        QLIST_FOREACH_SAFE(request, &dataplane->inflight, list, next) {
> > +            blk_aio_flush(request->dataplane->blk,
xen_block_complete_aio,
> > +                        request);
> > +        }
> > +    }
> > +
>
> I think this could possibly be simplified by doing the drain after the
> call to blk_set_aio_context(), as long as we set dataplane->ctx to
> qemu_get_aio_context(). Alos, as long as more_work is not set then it
> should still be safe to cancel the bh before the drain AFAICT.
>
>    Paul
>
> >       xendev = dataplane->xendev;
> >
> >       aio_context_acquire(dataplane->ctx);
> > +
> >       if (dataplane->event_channel) {
> >           /* Only reason for failure is a NULL channel */
> >           xen_device_set_event_channel_context(xendev,
dataplane->event_channel,
> > @@ -684,12 +709,6 @@ void xen_block_dataplane_stop(XenBlockDataPlane
*dataplane)
> >       blk_set_aio_context(dataplane->blk, qemu_get_aio_context(),
&error_abort);
> >       aio_context_release(dataplane->ctx);
> >
> > -    /*
> > -     * Now that the context has been moved onto the main thread, cancel
> > -     * further processing.
> > -     */
> > -    qemu_bh_cancel(dataplane->bh);
> > -
> >       if (dataplane->event_channel) {
> >           Error *local_err = NULL;
> >
>

[-- Attachment #2: Type: text/html, Size: 5357 bytes --]