Re: [Qemu-devel] [PATCH v6] qcow2: Buffer L1 table in snapshot refcount update

[Stefan Hajnoczi]

On Tue, Nov 11, 2014 at 04:27:51PM +0100, Max Reitz wrote:
> From: Zhang Haoyu <address@hidden>
> 
> Buffer the active L1 table in qcow2_update_snapshot_refcount() in order
> to prevent in-place conversion of the L1 table buffer in the
> BDRVQcowState to big endian and back, which would lead to data
> corruption if that buffer was accessed concurrently. This should not
> happen but better being safe than sorry.
> 
> Signed-off-by: Zhang Haoyu <address@hidden>
> Signed-off-by: Max Reitz <address@hidden>
> ---
> v6 for "snapshot: use local variable to bdrv_pwrite_sync L1 table" (I
> changed the commit message wording to make it more clear what this patch
> does and why we want it).
> 
> Changes in v6:
> - Only copy the local buffer back into s->l1_table if we are indeed
>   accessing the local L1 table
> - Use qemu_vfree() instead of g_free()
> ---
>  block/qcow2-refcount.c | 30 ++++++++++++++----------------
>  1 file changed, 14 insertions(+), 16 deletions(-)

If there is a code path where the L1 table is accessed while
qcow2_update_snapshot_refcount() is blocked, this patch does not fix the
bug.

It trades an L1 table entry corruption (due to endianness mismatch on
little-endian hosts) for a race condition where a stale L1 table is
accessed or L1 changes are overwritten when
qcow2_update_snapshot_refcount() memcpys back to s->l1_table.

Please identify the root cause and fix that.

> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
> index 9afdb40..c0c4a50 100644
> --- a/block/qcow2-refcount.c
> +++ b/block/qcow2-refcount.c
> @@ -877,14 +877,18 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs,
>  {
>      BDRVQcowState *s = bs->opaque;
>      uint64_t *l1_table, *l2_table, l2_offset, offset, l1_size2;
> -    bool l1_allocated = false;
> +    bool active_l1 = false;
>      int64_t old_offset, old_l2_offset;
>      int i, j, l1_modified = 0, nb_csectors, refcount;
>      int ret;
>  
>      l2_table = NULL;
> -    l1_table = NULL;
>      l1_size2 = l1_size * sizeof(uint64_t);
> +    l1_table = qemu_try_blockalign(bs->file, l1_size2);
> +    if (l1_table == NULL) {
> +        ret = -ENOMEM;
> +        goto fail;
> +    }
>  
>      s->cache_discards = true;
>  
> @@ -892,13 +896,6 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs,
>       * l1_table_offset when it is the current s->l1_table_offset! Be careful
>       * when changing this! */
>      if (l1_table_offset != s->l1_table_offset) {
> -        l1_table = g_try_malloc0(align_offset(l1_size2, 512));
> -        if (l1_size2 && l1_table == NULL) {
> -            ret = -ENOMEM;
> -            goto fail;
> -        }
> -        l1_allocated = true;
> -
>          ret = bdrv_pread(bs->file, l1_table_offset, l1_table, l1_size2);
>          if (ret < 0) {
>              goto fail;
> @@ -908,8 +905,8 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs,
>              be64_to_cpus(&l1_table[i]);
>      } else {
>          assert(l1_size == s->l1_size);
> -        l1_table = s->l1_table;
> -        l1_allocated = false;
> +        memcpy(l1_table, s->l1_table, l1_size2);
> +        active_l1 = true;
>      }
>  
>      for(i = 0; i < l1_size; i++) {
> @@ -1051,13 +1048,14 @@ fail:
>          }
>  
>          ret = bdrv_pwrite_sync(bs->file, l1_table_offset, l1_table, 
> l1_size2);
> -
> -        for (i = 0; i < l1_size; i++) {
> -            be64_to_cpus(&l1_table[i]);
> +        if (active_l1 && ret == 0) {
> +            for (i = 0; i < l1_size; i++) {
> +                be64_to_cpus(&l1_table[i]);
> +            }
> +            memcpy(s->l1_table, l1_table, l1_size2);
>          }
>      }
> -    if (l1_allocated)
> -        g_free(l1_table);
> +    qemu_vfree(l1_table);
>      return ret;
>  }
>  
> -- 
> 1.9.3
> 
>

pgpv7plTLuXoq.pgp
Description: PGP signature