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

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

[Max Reitz]

From: Zhang Haoyu <zhanghy@sangfor.com>

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 <zhanghy@sangfor.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
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(-)

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

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

[Max Reitz]

On 2014-11-11 at 16:27, Max Reitz wrote:
> From: Zhang Haoyu <zhanghy@sangfor.com>
>
> 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 <zhanghy@sangfor.com>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> 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(-)

Ping

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

[Max Reitz]

On 2014-11-20 at 15:32, Max Reitz wrote:
> On 2014-11-11 at 16:27, Max Reitz wrote:
>> From: Zhang Haoyu <zhanghy@sangfor.com>
>>
>> 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 <zhanghy@sangfor.com>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>> 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(-)
>
> Ping

-(Png)²

(got it? because i² = -1, ha-ha-haa)

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

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 3894 bytes --]

On Tue, Nov 11, 2014 at 04:27:51PM +0100, Max Reitz wrote:
> From: Zhang Haoyu <zhanghy@sangfor.com>
> 
> 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 <zhanghy@sangfor.com>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> 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
> 
> 

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]