Patchwork qcow2: Switch L1 table in a single sequence

login
register
mail settings
Submitter Max Reitz
Date Sept. 30, 2013, 3:57 p.m.
Message ID <1380556641-26256-1-git-send-email-mreitz@redhat.com>
Download mbox | patch
Permalink /patch/279180/
State New
Headers show

Comments

Max Reitz - Sept. 30, 2013, 3:57 p.m.
Switching the L1 table in memory should be an atomic operation, as far
as possible. Calling qcow2_free_clusters on the old L1 table on disk is
not a good idea when the old L1 table is no longer valid and the address
to the new one hasn't yet been written into the corresponding
BDRVQcowState field. To be more specific, this can lead to segfaults due
to qcow2_check_metadata_overlap trying to access the L1 table during the
free operation.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/qcow2-cluster.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
Eric Blake - Sept. 30, 2013, 5:48 p.m.
On 09/30/2013 09:57 AM, Max Reitz wrote:
> Switching the L1 table in memory should be an atomic operation, as far
> as possible. Calling qcow2_free_clusters on the old L1 table on disk is
> not a good idea when the old L1 table is no longer valid and the address
> to the new one hasn't yet been written into the corresponding
> BDRVQcowState field. To be more specific, this can lead to segfaults due
> to qcow2_check_metadata_overlap trying to access the L1 table during the
> free operation.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/qcow2-cluster.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>
Kevin Wolf - Oct. 1, 2013, 9:12 a.m.
Am 30.09.2013 um 17:57 hat Max Reitz geschrieben:
> Switching the L1 table in memory should be an atomic operation, as far
> as possible. Calling qcow2_free_clusters on the old L1 table on disk is
> not a good idea when the old L1 table is no longer valid and the address
> to the new one hasn't yet been written into the corresponding
> BDRVQcowState field. To be more specific, this can lead to segfaults due
> to qcow2_check_metadata_overlap trying to access the L1 table during the
> free operation.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Stefan Hajnoczi - Oct. 2, 2013, 1:38 p.m.
On Mon, Sep 30, 2013 at 05:57:21PM +0200, Max Reitz wrote:
> Switching the L1 table in memory should be an atomic operation, as far
> as possible. Calling qcow2_free_clusters on the old L1 table on disk is
> not a good idea when the old L1 table is no longer valid and the address
> to the new one hasn't yet been written into the corresponding
> BDRVQcowState field. To be more specific, this can lead to segfaults due
> to qcow2_check_metadata_overlap trying to access the L1 table during the
> free operation.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/qcow2-cluster.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)

Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan

Patch

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 72cb573..0fd26bb 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -35,6 +35,7 @@  int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size,
     BDRVQcowState *s = bs->opaque;
     int new_l1_size2, ret, i;
     uint64_t *new_l1_table;
+    int64_t old_l1_table_offset, old_l1_size;
     int64_t new_l1_table_offset, new_l1_size;
     uint8_t data[12];
 
@@ -106,11 +107,13 @@  int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size,
         goto fail;
     }
     g_free(s->l1_table);
-    qcow2_free_clusters(bs, s->l1_table_offset, s->l1_size * sizeof(uint64_t),
-                        QCOW2_DISCARD_OTHER);
+    old_l1_table_offset = s->l1_table_offset;
     s->l1_table_offset = new_l1_table_offset;
     s->l1_table = new_l1_table;
+    old_l1_size = s->l1_size;
     s->l1_size = new_l1_size;
+    qcow2_free_clusters(bs, old_l1_table_offset, old_l1_size * sizeof(uint64_t),
+                        QCOW2_DISCARD_OTHER);
     return 0;
  fail:
     g_free(new_l1_table);