Removing a drive with drive_del while it is being used to run an I/O
intensive workload can cause QEMU to crash.

An AIO flush can yield at some point:

blk_aio_flush_entry()
 blk_co_flush(blk)
  bdrv_co_flush(blk->root->bs)
   ...
    qemu_coroutine_yield()

and let the HMP command to run, free blk->root and give control
back to the AIO flush:

    hmp_drive_del()
     blk_remove_bs()
      bdrv_root_unref_child(blk->root)
       child_bs = blk->root->bs
       bdrv_detach_child(blk->root)
        bdrv_replace_child(blk->root, NULL)
         blk->root->bs = NULL
        g_free(blk->root) <============== blk->root becomes stale
       bdrv_unref(child_bs)
        bdrv_delete(child_bs)
         bdrv_close()
          bdrv_drained_begin()
           bdrv_do_drained_begin()
            bdrv_drain_recurse()
             aio_poll()
              ...
              qemu_coroutine_switch()

and the AIO flush completion ends up dereferencing blk->root:

  blk_aio_complete()
   scsi_aio_complete()
    blk_get_aio_context(blk)
     bs = blk_bs(blk)
 ie, bs = blk->root ? blk->root->bs : NULL
            ^^^^^
            stale

The problem is that we should avoid making block driver graph
changes while we have in-flight requests. This patch hence adds
a drained section to bdrv_detach_child(), so that we're sure
all requests have been drained when blk->root is freed.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
v2: - drain I/O requests when detaching the BDS (Stefan, Paolo)
---
 block.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/block.c b/block.c
index 676e57f5623a..fc9379439883 100644
--- a/block.c
+++ b/block.c
@@ -2127,12 +2127,16 @@ BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs,
 
 static void bdrv_detach_child(BdrvChild *child)
 {
+    BlockDriverState *child_bs = child->bs;
+
     if (child->next.le_prev) {
         QLIST_REMOVE(child, next);
         child->next.le_prev = NULL;
     }
 
+    bdrv_drained_begin(child_bs);
     bdrv_replace_child(child, NULL);
+    bdrv_drained_end(child_bs);
 
     g_free(child->name);
     g_free(child);

On Thu, May 24, 2018 at 11:09:47AM +0200, Greg Kurz wrote:
> Removing a drive with drive_del while it is being used to run an I/O
> intensive workload can cause QEMU to crash.
> 
> An AIO flush can yield at some point:
> 
> blk_aio_flush_entry()
>  blk_co_flush(blk)
>   bdrv_co_flush(blk->root->bs)
>    ...
>     qemu_coroutine_yield()
> 
> and let the HMP command to run, free blk->root and give control
> back to the AIO flush:
> 
>     hmp_drive_del()
>      blk_remove_bs()
>       bdrv_root_unref_child(blk->root)
>        child_bs = blk->root->bs
>        bdrv_detach_child(blk->root)
>         bdrv_replace_child(blk->root, NULL)
>          blk->root->bs = NULL
>         g_free(blk->root) <============== blk->root becomes stale
>        bdrv_unref(child_bs)
>         bdrv_delete(child_bs)
>          bdrv_close()
>           bdrv_drained_begin()
>            bdrv_do_drained_begin()
>             bdrv_drain_recurse()
>              aio_poll()
>               ...
>               qemu_coroutine_switch()
> 
> and the AIO flush completion ends up dereferencing blk->root:
> 
>   blk_aio_complete()
>    scsi_aio_complete()
>     blk_get_aio_context(blk)
>      bs = blk_bs(blk)
>  ie, bs = blk->root ? blk->root->bs : NULL
>             ^^^^^
>             stale
> 
> The problem is that we should avoid making block driver graph
> changes while we have in-flight requests. This patch hence adds
> a drained section to bdrv_detach_child(), so that we're sure
> all requests have been drained when blk->root is freed.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
> v2: - drain I/O requests when detaching the BDS (Stefan, Paolo)
> ---
>  block.c |    4 ++++
>  1 file changed, 4 insertions(+)

<formletter>

This is not the correct way to submit patches for inclusion in the
stable kernel tree.  Please read:
    https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
for how to do this properly.

</formletter>

On Thu, May 24, 2018 at 11:09:47AM +0200, Greg Kurz wrote:
> diff --git a/block.c b/block.c
> index 676e57f5623a..fc9379439883 100644
> --- a/block.c
> +++ b/block.c
> @@ -2127,12 +2127,16 @@ BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs,
>  
>  static void bdrv_detach_child(BdrvChild *child)
>  {
> +    BlockDriverState *child_bs = child->bs;
> +
>      if (child->next.le_prev) {
>          QLIST_REMOVE(child, next);
>          child->next.le_prev = NULL;
>      }

This child->next modification makes me nervous.  Please start the
drained region before modifying the graph.

On Thu, 24 May 2018 17:52:56 +0100
Stefan Hajnoczi <stefanha@redhat.com> wrote:

> On Thu, May 24, 2018 at 11:09:47AM +0200, Greg Kurz wrote:
> > diff --git a/block.c b/block.c
> > index 676e57f5623a..fc9379439883 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -2127,12 +2127,16 @@ BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs,
> >  
> >  static void bdrv_detach_child(BdrvChild *child)
> >  {
> > +    BlockDriverState *child_bs = child->bs;
> > +
> >      if (child->next.le_prev) {
> >          QLIST_REMOVE(child, next);
> >          child->next.le_prev = NULL;
> >      }  
> 
> This child->next modification makes me nervous.  Please start the
> drained region before modifying the graph.

Ok, I'll send a v3, and Cc qemu-stable@nongnu.org instead of
stable@vger.kernel.org this time :P