Re: [PATCH 3/3] qcow2: Fix corruption bug in qcow2_detect_metadata_preallocation()

[Vladimir Sementsov-Ogievskiy]

23.10.2019 18:26, Kevin Wolf wrote:
> qcow2_detect_metadata_preallocation() calls qcow2_get_refcount() which
> requires s->lock to be taken to protect its accesses to the refcount
> table and refcount blocks. However, nothing in this code path actually
> took the lock. This could cause the same cache entry to be used by two
> requests at the same time, for different tables at different offsets,
> resulting in image corruption.
> 
> As it would be preferable to base the detection on consistent data (even
> though it's just heuristics), let's take the lock not only around the
> qcow2_get_refcount() calls, but around the whole function.
> 
> This patch takes the lock in qcow2_co_block_status() earlier and asserts
> in qcow2_detect_metadata_preallocation() that we hold the lock.
> 
> Fixes: 69f47505ee66afaa513305de0c1895a224e52c45
> Cc: address@hidden
> Reported-by: Michael Weiser <address@hidden>
> Signed-off-by: Kevin Wolf <address@hidden>

Oh, I'm very sorry. I was going to backport this patch, and found that it's
fixed in our downstream long ago, even before last upstream version patch sent 
:(

Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>

> ---
>   block/qcow2-refcount.c | 2 ++
>   block/qcow2.c          | 3 ++-
>   2 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
> index ef965d7895..0d64bf5a5e 100644
> --- a/block/qcow2-refcount.c
> +++ b/block/qcow2-refcount.c
> @@ -3455,6 +3455,8 @@ int 
> qcow2_detect_metadata_preallocation(BlockDriverState *bs)
>       int64_t i, end_cluster, cluster_count = 0, threshold;
>       int64_t file_length, real_allocation, real_clusters;
>   
> +    qemu_co_mutex_assert_locked(&s->lock);
> +
>       file_length = bdrv_getlength(bs->file->bs);
>       if (file_length < 0) {
>           return file_length;
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 8b05933565..0bc69e6996 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1916,6 +1916,8 @@ static int coroutine_fn 
> qcow2_co_block_status(BlockDriverState *bs,
>       unsigned int bytes;
>       int status = 0;
>   
> +    qemu_co_mutex_lock(&s->lock);
> +
>       if (!s->metadata_preallocation_checked) {
>           ret = qcow2_detect_metadata_preallocation(bs);
>           s->metadata_preallocation = (ret == 1);
> @@ -1923,7 +1925,6 @@ static int coroutine_fn 
> qcow2_co_block_status(BlockDriverState *bs,
>       }
>   
>       bytes = MIN(INT_MAX, count);
> -    qemu_co_mutex_lock(&s->lock);
>       ret = qcow2_get_cluster_offset(bs, offset, &bytes, &cluster_offset);
>       qemu_co_mutex_unlock(&s->lock);
>       if (ret < 0) {
> 


-- 
Best regards,
Vladimir