If @bs does not have any parents, the only reference to @mirror_top_bs
will be held by the BlockJob object after the bdrv_unref() following
block_job_create(). However, if block_job_create() fails, this reference
will not exist and @mirror_top_bs will have been deleted when we
goto fail.

The issue comes back at all later entries to the fail label: We delete
the BlockJob object before rolling back our changes to the node graph.
This means that we will delete @mirror_top_bs in the process.

All in all, whenever @bs does not have any parents and we go down the
fail path we will dereference @mirror_top_bs after it has been deleted.

Fix this by invoking bdrv_unref() only when block_job_create() was
successful and by bdrv_ref()'ing @mirror_top_bs in the fail path before
deleting the BlockJob object. Finally, bdrv_unref() it at the end of the
fail path after we actually no longer need it.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/mirror.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index 9e2fecc15e..46ecd38ef0 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1150,7 +1150,7 @@ static void mirror_start_job(const char *job_id, BlockDriverState *bs,
     mirror_top_bs->total_sectors = bs->total_sectors;
 
     /* bdrv_append takes ownership of the mirror_top_bs reference, need to keep
-     * it alive until block_job_create() even if bs has no parent. */
+     * it alive until block_job_create() succeeds even if bs has no parent. */
     bdrv_ref(mirror_top_bs);
     bdrv_drained_begin(bs);
     bdrv_append(mirror_top_bs, bs, &local_err);
@@ -1168,10 +1168,12 @@ static void mirror_start_job(const char *job_id, BlockDriverState *bs,
                          BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED |
                          BLK_PERM_WRITE | BLK_PERM_GRAPH_MOD, speed,
                          creation_flags, cb, opaque, errp);
-    bdrv_unref(mirror_top_bs);
     if (!s) {
         goto fail;
     }
+    /* The block job now has a reference to this node */
+    bdrv_unref(mirror_top_bs);
+
     s->source = bs;
     s->mirror_top_bs = mirror_top_bs;
 
@@ -1242,6 +1244,10 @@ static void mirror_start_job(const char *job_id, BlockDriverState *bs,
 
 fail:
     if (s) {
+        /* Make sure this BDS does not go away until we have completed the graph
+         * changes below */
+        bdrv_ref(mirror_top_bs);
+
         g_free(s->replaces);
         blk_unref(s->target);
         block_job_unref(&s->common);
@@ -1250,6 +1256,8 @@ fail:
     bdrv_child_try_set_perm(mirror_top_bs->backing, 0, BLK_PERM_ALL,
                             &error_abort);
     bdrv_replace_node(mirror_top_bs, backing_bs(mirror_top_bs), &error_abort);
+
+    bdrv_unref(mirror_top_bs);
 }
 
 void mirror_start(const char *job_id, BlockDriverState *bs,
-- 
2.12.1

nice analysis/catch

On 04/03/2017 02:51 PM, Max Reitz wrote:
> If @bs does not have any parents, the only reference to @mirror_top_bs
> will be held by the BlockJob object after the bdrv_unref() following
> block_job_create(). However, if block_job_create() fails, this reference
> will not exist and @mirror_top_bs will have been deleted when we
> goto fail.
>
> The issue comes back at all later entries to the fail label: We delete
> the BlockJob object before rolling back our changes to the node graph.
> This means that we will delete @mirror_top_bs in the process.
>
> All in all, whenever @bs does not have any parents and we go down the
> fail path we will dereference @mirror_top_bs after it has been deleted.
>
> Fix this by invoking bdrv_unref() only when block_job_create() was
> successful and by bdrv_ref()'ing @mirror_top_bs in the fail path before
> deleting the BlockJob object. Finally, bdrv_unref() it at the end of the
> fail path after we actually no longer need it.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  block/mirror.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/block/mirror.c b/block/mirror.c
> index 9e2fecc15e..46ecd38ef0 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -1150,7 +1150,7 @@ static void mirror_start_job(const char *job_id, BlockDriverState *bs,
>      mirror_top_bs->total_sectors = bs->total_sectors;
>
>      /* bdrv_append takes ownership of the mirror_top_bs reference, need to keep
> -     * it alive until block_job_create() even if bs has no parent. */
> +     * it alive until block_job_create() succeeds even if bs has no parent. */
>      bdrv_ref(mirror_top_bs);
>      bdrv_drained_begin(bs);
>      bdrv_append(mirror_top_bs, bs, &local_err);
> @@ -1168,10 +1168,12 @@ static void mirror_start_job(const char *job_id, BlockDriverState *bs,
>                           BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED |
>                           BLK_PERM_WRITE | BLK_PERM_GRAPH_MOD, speed,
>                           creation_flags, cb, opaque, errp);
> -    bdrv_unref(mirror_top_bs);
>      if (!s) {
>          goto fail;
>      }
> +    /* The block job now has a reference to this node */
> +    bdrv_unref(mirror_top_bs);
> +
>      s->source = bs;
>      s->mirror_top_bs = mirror_top_bs;
>
> @@ -1242,6 +1244,10 @@ static void mirror_start_job(const char *job_id, BlockDriverState *bs,
>
>  fail:
>      if (s) {
> +        /* Make sure this BDS does not go away until we have completed the graph
> +         * changes below */
> +        bdrv_ref(mirror_top_bs);
> +
>          g_free(s->replaces);
>          blk_unref(s->target);
>          block_job_unref(&s->common);
> @@ -1250,6 +1256,8 @@ fail:
>      bdrv_child_try_set_perm(mirror_top_bs->backing, 0, BLK_PERM_ALL,
>                              &error_abort);
>      bdrv_replace_node(mirror_top_bs, backing_bs(mirror_top_bs), &error_abort);
> +
> +    bdrv_unref(mirror_top_bs);
>  }
>
>  void mirror_start(const char *job_id, BlockDriverState *bs,
>