[Qemu-devel] [PATCH 0/5] qcow2 fixes

[Qemu-devel] [PATCH 0/5] qcow2 fixes

[Kevin Wolf]

Another series of fixes for bugs that were found using blkdebug. The last two
of them are presumably relevant even outside error paths and can cause
corruption. They should be applied to stable-0.12, too.

Kevin Wolf (5):
  qcow2: Allow qcow2_get_cluster_offset to return errors
  qcow2: Change l2_load to return 0/-errno
  qcow2: Return right error code in write_refcount_block_entries
  qcow2: Fix corruption after refblock allocation
  qcow2: Fix corruption after error in update_refcount

 block/qcow2-cluster.c  |   70 ++++++++++++++++++++++++++++-------------------
 block/qcow2-refcount.c |   42 +++++++++++++++++++---------
 block/qcow2.c          |   16 +++++++++--
 block/qcow2.h          |    4 +-
 4 files changed, 85 insertions(+), 47 deletions(-)

[Qemu-devel] [PATCH 1/5] qcow2: Allow qcow2_get_cluster_offset to return errors

[Kevin Wolf]

qcow2_get_cluster_offset() looks up a given virtual disk offset and returns the
offset of the corresponding cluster in the image file. Errors (e.g. L2 table
can't be read) are currenctly indicated by a return value of 0, which is
unfortuately the same as for any unallocated cluster. So in effect we can't
check for errors.

This makes the old return value a by-reference parameter and returns the usual
0/-errno error code.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow2-cluster.c |   36 ++++++++++++++++++++++--------------
 block/qcow2.c         |   16 +++++++++++++---
 block/qcow2.h         |    4 ++--
 3 files changed, 37 insertions(+), 19 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 244b4a7..ea98afc 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -345,7 +345,13 @@ static int qcow_read(BlockDriverState *bs, int64_t sector_num,
 
     while (nb_sectors > 0) {
         n = nb_sectors;
-        cluster_offset = qcow2_get_cluster_offset(bs, sector_num << 9, &n);
+
+        ret = qcow2_get_cluster_offset(bs, sector_num << 9, &n,
+            &cluster_offset);
+        if (ret < 0) {
+            return ret;
+        }
+
         index_in_cluster = sector_num & (s->cluster_sectors - 1);
         if (!cluster_offset) {
             if (bs->backing_hd) {
@@ -412,25 +418,25 @@ static int copy_sectors(BlockDriverState *bs, uint64_t start_sect,
 /*
  * get_cluster_offset
  *
- * For a given offset of the disk image, return cluster offset in
- * qcow2 file.
+ * For a given offset of the disk image, find the cluster offset in
+ * qcow2 file. The offset is stored in *cluster_offset.
  *
  * on entry, *num is the number of contiguous clusters we'd like to
  * access following offset.
  *
  * on exit, *num is the number of contiguous clusters we can read.
  *
- * Return 1, if the offset is found
- * Return 0, otherwise.
+ * Return 0, if the offset is found
+ * Return -errno, otherwise.
  *
  */
 
-uint64_t qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset,
-    int *num)
+int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset,
+    int *num, uint64_t *cluster_offset)
 {
     BDRVQcowState *s = bs->opaque;
     unsigned int l1_index, l2_index;
-    uint64_t l2_offset, *l2_table, cluster_offset;
+    uint64_t l2_offset, *l2_table;
     int l1_bits, c;
     unsigned int index_in_cluster, nb_clusters;
     uint64_t nb_available, nb_needed;
@@ -454,7 +460,7 @@ uint64_t qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset,
         nb_needed = nb_available;
     }
 
-    cluster_offset = 0;
+    *cluster_offset = 0;
 
     /* seek the the l2 offset in the l1 table */
 
@@ -473,16 +479,17 @@ uint64_t qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset,
 
     l2_offset &= ~QCOW_OFLAG_COPIED;
     l2_table = l2_load(bs, l2_offset);
-    if (l2_table == NULL)
-        return 0;
+    if (l2_table == NULL) {
+        return -EIO;
+    }
 
     /* find the cluster offset for the given disk offset */
 
     l2_index = (offset >> s->cluster_bits) & (s->l2_size - 1);
-    cluster_offset = be64_to_cpu(l2_table[l2_index]);
+    *cluster_offset = be64_to_cpu(l2_table[l2_index]);
     nb_clusters = size_to_clusters(s, nb_needed << 9);
 
-    if (!cluster_offset) {
+    if (!*cluster_offset) {
         /* how many empty clusters ? */
         c = count_contiguous_free_clusters(nb_clusters, &l2_table[l2_index]);
     } else {
@@ -498,7 +505,8 @@ out:
 
     *num = nb_available - index_in_cluster;
 
-    return cluster_offset & ~QCOW_OFLAG_COPIED;
+    *cluster_offset &=~QCOW_OFLAG_COPIED;
+    return 0;
 }
 
 /*
diff --git a/block/qcow2.c b/block/qcow2.c
index 5b72758..33fa9a9 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -297,9 +297,15 @@ static int qcow_is_allocated(BlockDriverState *bs, int64_t sector_num,
                              int nb_sectors, int *pnum)
 {
     uint64_t cluster_offset;
+    int ret;
 
     *pnum = nb_sectors;
-    cluster_offset = qcow2_get_cluster_offset(bs, sector_num << 9, pnum);
+    /* FIXME We can get errors here, but the bdrv_is_allocated interface can't
+     * pass them on today */
+    ret = qcow2_get_cluster_offset(bs, sector_num << 9, pnum, &cluster_offset);
+    if (ret < 0) {
+        *pnum = 0;
+    }
 
     return (cluster_offset != 0);
 }
@@ -409,8 +415,12 @@ static void qcow_aio_read_cb(void *opaque, int ret)
 
     /* prepare next AIO request */
     acb->cur_nr_sectors = acb->remaining_sectors;
-    acb->cluster_offset = qcow2_get_cluster_offset(bs, acb->sector_num << 9,
-                                                   &acb->cur_nr_sectors);
+    ret = qcow2_get_cluster_offset(bs, acb->sector_num << 9,
+        &acb->cur_nr_sectors, &acb->cluster_offset);
+    if (ret < 0) {
+        goto done;
+    }
+
     index_in_cluster = acb->sector_num & (s->cluster_sectors - 1);
 
     if (!acb->cluster_offset) {
diff --git a/block/qcow2.h b/block/qcow2.h
index 01053b7..c59b827 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -196,8 +196,8 @@ void qcow2_encrypt_sectors(BDRVQcowState *s, int64_t sector_num,
                      int nb_sectors, int enc,
                      const AES_KEY *key);
 
-uint64_t qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset,
-    int *num);
+int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset,
+    int *num, uint64_t *cluster_offset);
 int qcow2_alloc_cluster_offset(BlockDriverState *bs, uint64_t offset,
     int n_start, int n_end, int *num, QCowL2Meta *m);
 uint64_t qcow2_alloc_compressed_cluster_offset(BlockDriverState *bs,
-- 
1.6.6.1

[Qemu-devel] [PATCH 2/5] qcow2: Change l2_load to return 0/-errno

[Kevin Wolf]

Provide the error code to the caller instead of just indicating success/error.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow2-cluster.c |   38 ++++++++++++++++++++++----------------
 1 files changed, 22 insertions(+), 16 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index ea98afc..03a9f25 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -157,31 +157,36 @@ static uint64_t *seek_l2_table(BDRVQcowState *s, uint64_t l2_offset)
  * the image file failed.
  */
 
-static uint64_t *l2_load(BlockDriverState *bs, uint64_t l2_offset)
+static int l2_load(BlockDriverState *bs, uint64_t l2_offset,
+    uint64_t **l2_table)
 {
     BDRVQcowState *s = bs->opaque;
     int min_index;
-    uint64_t *l2_table;
+    int ret;
 
     /* seek if the table for the given offset is in the cache */
 
-    l2_table = seek_l2_table(s, l2_offset);
-    if (l2_table != NULL)
-        return l2_table;
+    *l2_table = seek_l2_table(s, l2_offset);
+    if (*l2_table != NULL) {
+        return 0;
+    }
 
     /* not found: load a new entry in the least used one */
 
     min_index = l2_cache_new_entry(bs);
-    l2_table = s->l2_cache + (min_index << s->l2_bits);
+    *l2_table = s->l2_cache + (min_index << s->l2_bits);
 
     BLKDBG_EVENT(bs->file, BLKDBG_L2_LOAD);
-    if (bdrv_pread(bs->file, l2_offset, l2_table, s->l2_size * sizeof(uint64_t)) !=
-        s->l2_size * sizeof(uint64_t))
-        return NULL;
+    ret = bdrv_pread(bs->file, l2_offset, *l2_table,
+        s->l2_size * sizeof(uint64_t));
+    if (ret < 0) {
+        return ret;
+    }
+
     s->l2_cache_offsets[min_index] = l2_offset;
     s->l2_cache_counts[min_index] = 1;
 
-    return l2_table;
+    return 0;
 }
 
 /*
@@ -440,6 +445,7 @@ int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset,
     int l1_bits, c;
     unsigned int index_in_cluster, nb_clusters;
     uint64_t nb_available, nb_needed;
+    int ret;
 
     index_in_cluster = (offset >> 9) & (s->cluster_sectors - 1);
     nb_needed = *num + index_in_cluster;
@@ -478,9 +484,9 @@ int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset,
     /* load the l2 table in memory */
 
     l2_offset &= ~QCOW_OFLAG_COPIED;
-    l2_table = l2_load(bs, l2_offset);
-    if (l2_table == NULL) {
-        return -EIO;
+    ret = l2_load(bs, l2_offset, &l2_table);
+    if (ret < 0) {
+        return ret;
     }
 
     /* find the cluster offset for the given disk offset */
@@ -547,9 +553,9 @@ static int get_cluster_table(BlockDriverState *bs, uint64_t offset,
     if (l2_offset & QCOW_OFLAG_COPIED) {
         /* load the l2 table in memory */
         l2_offset &= ~QCOW_OFLAG_COPIED;
-        l2_table = l2_load(bs, l2_offset);
-        if (l2_table == NULL) {
-            return -EIO;
+        ret = l2_load(bs, l2_offset, &l2_table);
+        if (ret < 0) {
+            return ret;
         }
     } else {
         if (l2_offset)
-- 
1.6.6.1

[Qemu-devel] [PATCH 3/5] qcow2: Return right error code in write_refcount_block_entries

[Kevin Wolf]

write_refcount_block_entries used to return -EIO for any errors. Change this to
return the real error code.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow2-refcount.c |   27 +++++++++++++++------------
 1 files changed, 15 insertions(+), 12 deletions(-)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 744107c..a7f295d 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -404,6 +404,7 @@ static int write_refcount_block_entries(BlockDriverState *bs,
 {
     BDRVQcowState *s = bs->opaque;
     size_t size;
+    int ret;
 
     if (cache_refcount_updates) {
         return 0;
@@ -414,12 +415,13 @@ static int write_refcount_block_entries(BlockDriverState *bs,
         & ~(REFCOUNTS_PER_SECTOR - 1);
 
     size = (last_index - first_index) << REFCOUNT_SHIFT;
+
     BLKDBG_EVENT(bs->file, BLKDBG_REFBLOCK_UPDATE_PART);
-    if (bdrv_pwrite(bs->file,
+    ret = bdrv_pwrite(bs->file,
         refcount_block_offset + (first_index << REFCOUNT_SHIFT),
-        &s->refcount_block_cache[first_index], size) != size)
-    {
-        return -EIO;
+        &s->refcount_block_cache[first_index], size);
+    if (ret < 0) {
+        return ret;
     }
 
     return 0;
@@ -460,10 +462,10 @@ static int QEMU_WARN_UNUSED_RESULT update_refcount(BlockDriverState *bs,
         table_index = cluster_index >> (s->cluster_bits - REFCOUNT_SHIFT);
         if ((old_table_index >= 0) && (table_index != old_table_index)) {
 
-            if (write_refcount_block_entries(bs, refcount_block_offset,
-                first_index, last_index) < 0)
-            {
-                return -EIO;
+            ret = write_refcount_block_entries(bs, refcount_block_offset,
+                first_index, last_index);
+            if (ret < 0) {
+                return ret;
             }
 
             first_index = -1;
@@ -505,10 +507,11 @@ fail:
 
     /* Write last changed block to disk */
     if (refcount_block_offset != 0) {
-        if (write_refcount_block_entries(bs, refcount_block_offset,
-            first_index, last_index) < 0)
-        {
-            return ret < 0 ? ret : -EIO;
+        int wret;
+        wret = write_refcount_block_entries(bs, refcount_block_offset,
+            first_index, last_index);
+        if (wret < 0) {
+            return ret < 0 ? ret : wret;
         }
     }
 
-- 
1.6.6.1

[Qemu-devel] [PATCH 4/5] qcow2: Fix corruption after refblock allocation

[Kevin Wolf]

Refblock allocation code needs to take into consideration that update_refcount
will load a different refcount block into the cache, so it must initialize the
cache for a new refcount block only afterwards. Not doing this means that not
only the refcount in the wrong block is updated, but also that the caller will
work on the wrong block.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow2-refcount.c |   11 +++++++++--
 1 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index a7f295d..5b7cda4 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -221,8 +221,6 @@ static int64_t alloc_refcount_block(BlockDriverState *bs, int64_t cluster_index)
 
     /* Allocate the refcount block itself and mark it as used */
     uint64_t new_block = alloc_clusters_noref(bs, s->cluster_size);
-    memset(s->refcount_block_cache, 0, s->cluster_size);
-    s->refcount_block_cache_offset = new_block;
 
 #ifdef DEBUG_ALLOC2
     fprintf(stderr, "qcow2: Allocate refcount block %d for %" PRIx64
@@ -231,6 +229,10 @@ static int64_t alloc_refcount_block(BlockDriverState *bs, int64_t cluster_index)
 #endif
 
     if (in_same_refcount_block(s, new_block, cluster_index << s->cluster_bits)) {
+        /* Zero the new refcount block before updating it */
+        memset(s->refcount_block_cache, 0, s->cluster_size);
+        s->refcount_block_cache_offset = new_block;
+
         /* The block describes itself, need to update the cache */
         int block_index = (new_block >> s->cluster_bits) &
             ((1 << (s->cluster_bits - REFCOUNT_SHIFT)) - 1);
@@ -242,6 +244,11 @@ static int64_t alloc_refcount_block(BlockDriverState *bs, int64_t cluster_index)
         if (ret < 0) {
             goto fail_block;
         }
+
+        /* Initialize the new refcount block only after updating its refcount,
+         * update_refcount uses the refcount cache itself */
+        memset(s->refcount_block_cache, 0, s->cluster_size);
+        s->refcount_block_cache_offset = new_block;
     }
 
     /* Now the new refcount block needs to be written to disk */
-- 
1.6.6.1

[Qemu-devel] [PATCH 5/5] qcow2: Fix corruption after error in update_refcount

[Kevin Wolf]

After it is done with updating refcounts in the cache, update_refcount writes
all changed entries to disk. If a refcount block allocation fails, however,
there was no change yet and therefore first_index = last_index = -1. Don't
treat -1 as a normal sector index (resulting in a 512 byte write!) but return
without updating anything in this case.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow2-refcount.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 5b7cda4..22b0b45 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -417,6 +417,10 @@ static int write_refcount_block_entries(BlockDriverState *bs,
         return 0;
     }
 
+    if (first_index < 0) {
+        return 0;
+    }
+
     first_index &= ~(REFCOUNTS_PER_SECTOR - 1);
     last_index = (last_index + REFCOUNTS_PER_SECTOR)
         & ~(REFCOUNTS_PER_SECTOR - 1);
-- 
1.6.6.1

[Qemu-devel] Re: [PATCH 1/5] qcow2: Allow qcow2_get_cluster_offset to return errors

[Stefan Hajnoczi]

Reviewed-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>

[Qemu-devel] Re: [PATCH 2/5] qcow2: Change l2_load to return 0/-errno

[Stefan Hajnoczi]

Reviewed-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>

[Qemu-devel] Re: [PATCH 3/5] qcow2: Return right error code in write_refcount_block_entries

[Stefan Hajnoczi]

Reviewed-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>

[Qemu-devel] Re: [PATCH 5/5] qcow2: Fix corruption after error in update_refcount

[Stefan Hajnoczi]

Reviewed-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>

[Qemu-devel] Re: [PATCH 4/5] qcow2: Fix corruption after refblock allocation

[Stefan Hajnoczi]

I haven't figured out this function yet so I can't review this one sorry.

Stefan