Re: [PATCH] block/mirror: fix core when using iothreads

[John Snow]

On 8/26/20 9:19 AM, Peng Liang wrote:
> We found an issue when doing block-commit with iothreads, which tries to
> dereference a NULL pointer.

I'm clearing out my patch backlog. I am a bit out of the loop on block 
issues at the moment, did this issue get addressed in a later patch that 
I am not seeing, or does it still need attention?

--js

> <main thread>                      |<IO thread>
>                                     |
> mirror_start_job                   |
>   1. bdrv_ref(mirror_top_bs);       |
>      bdrv_drained_begin(bs);        |
>      bdrv_append(mirror_top_bs,     |
>                  bs, &local_err);   |
>      bdrv_drained_end(bs);          |
>                                     |
>   2. s = block_job_create(...);     |
>                                     |bdrv_mirror_top_pwritev
>                                     | MirrorBDSOpaque *s = bs->opaque;
>                                     | bool copy_to_target;
>                                     | copy_to_target = s->job->ret >= 0 &&
>                                     |     s->job->copy_mode ==
>                                     |     MIRROR_COPY_MODE_WRITE_BLOCKING;
>                                     | (s->job is not NULL until 3!)
>   3. bs_opaque->job = s;            |
>
> Just moving step 2 & 3 before 1 can avoid this.
>
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Peng Liang <liangpeng10@huawei.com>
> ---
>   block/mirror.c | 21 ++++++++++-----------
>   1 file changed, 10 insertions(+), 11 deletions(-)
>
> diff --git a/block/mirror.c b/block/mirror.c
> index e8e8844afc40..7c872be71149 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -1600,6 +1600,16 @@ static BlockJob *mirror_start_job(
>       mirror_top_bs->supported_zero_flags = BDRV_REQ_WRITE_UNCHANGED |
>                                             BDRV_REQ_NO_FALLBACK;
>       bs_opaque = g_new0(MirrorBDSOpaque, 1);
> +    /* Make sure that the source is not resized while the job is running */
> +    s = block_job_create(job_id, driver, NULL, bs,
> +                         BLK_PERM_CONSISTENT_READ,
> +                         BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED |
> +                         BLK_PERM_WRITE | BLK_PERM_GRAPH_MOD, speed,
> +                         creation_flags, cb, opaque, errp);
> +    if (!s) {
> +        goto fail;
> +    }
> +    bs_opaque->job = s;
>       mirror_top_bs->opaque = bs_opaque;
>   
>       /* bdrv_append takes ownership of the mirror_top_bs reference, need to keep
> @@ -1612,19 +1622,8 @@ static BlockJob *mirror_start_job(
>       if (local_err) {
>           bdrv_unref(mirror_top_bs);
>           error_propagate(errp, local_err);
> -        return NULL;
> -    }
> -
> -    /* Make sure that the source is not resized while the job is running */
> -    s = block_job_create(job_id, driver, NULL, mirror_top_bs,
> -                         BLK_PERM_CONSISTENT_READ,
> -                         BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED |
> -                         BLK_PERM_WRITE | BLK_PERM_GRAPH_MOD, speed,
> -                         creation_flags, cb, opaque, errp);
> -    if (!s) {
>           goto fail;
>       }
> -    bs_opaque->job = s;
>   
>       /* The block job now has a reference to this node */
>       bdrv_unref(mirror_top_bs);