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: qemu-sta...@nongnu.org
> Reported-by: Michael Weiser <mich...@weiser.dinsnail.net>
> Signed-off-by: Kevin Wolf <kw...@redhat.com>
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 <vsement...@virtuozzo.com>
> ---
> 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
