[Qemu-devel] [PATCH v3 0/8] qcow2 check improvements

[Qemu-devel] [PATCH v3 0/8] qcow2 check improvements

[Vladimir Sementsov-Ogievskiy]

Hi all!

v3:

01: s/l2_fixed_entries/l2_dirty/
02: add comment, s/>/>=
03: s/BDRV_SECTOR_SIZE/512
04: add Max's r-b
05: new
06-08: mostly rewritten

v2:
02, 06: check bdrv_getlength error return code

v1:

We've faced the following problem: after host fs corruption, vm images
becomes invalid. And which is interesting, starting qemu-img check on
them led to allocating of the whole RAM and then killing qemu-img by
OOM Killer.

This was due to corrupted l2 entries, which referenced clusters far-far
beyond the end of the qcow2 file.
02 is a generic fix for the bug, 01 is unrelated improvement, 03-07 are
additional info and fixing for such corrupted table entries.

Vladimir Sementsov-Ogievskiy (8):
  block/qcow2-refcount: fix check_oflag_copied
  block/qcow2-refcount: avoid eating RAM
  block/qcow2-refcount: check_refcounts_l2: refactor compressed case
  block/qcow2-refcount: check_refcounts_l2: reduce ignored overlaps
  block/qcow2-refcount: check_refcounts_l2: don't count corrupted
    entries
  block/qcow2-refcount: refactor fixing L2 entry
  block/qcow2-refcount: fix out-of-file L1 entries to be zero
  block/qcow2-refcount: fix out-of-file L2 entries

 block/qcow2-refcount.c | 352 ++++++++++++++++++++++++++++++-----------
 1 file changed, 264 insertions(+), 88 deletions(-)

-- 
2.18.0

[Qemu-devel] [PATCH v3 1/8] block/qcow2-refcount: fix check_oflag_copied

[Vladimir Sementsov-Ogievskiy]

Increase corruptions_fixed only after successful fix.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/qcow2-refcount.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 3c539f02e5..b453d87a3f 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -1816,7 +1816,7 @@ static int check_oflag_copied(BlockDriverState *bs, BdrvCheckResult *res,
     for (i = 0; i < s->l1_size; i++) {
         uint64_t l1_entry = s->l1_table[i];
         uint64_t l2_offset = l1_entry & L1E_OFFSET_MASK;
-        bool l2_dirty = false;
+        int l2_dirty = 0;
 
         if (!l2_offset) {
             continue;
@@ -1878,8 +1878,7 @@ static int check_oflag_copied(BlockDriverState *bs, BdrvCheckResult *res,
                         l2_table[j] = cpu_to_be64(refcount == 1
                                     ? l2_entry |  QCOW_OFLAG_COPIED
                                     : l2_entry & ~QCOW_OFLAG_COPIED);
-                        l2_dirty = true;
-                        res->corruptions_fixed++;
+                        l2_dirty++;
                     } else {
                         res->corruptions++;
                     }
@@ -1887,7 +1886,7 @@ static int check_oflag_copied(BlockDriverState *bs, BdrvCheckResult *res,
             }
         }
 
-        if (l2_dirty) {
+        if (l2_dirty > 0) {
             ret = qcow2_pre_write_overlap_check(bs, QCOW2_OL_ACTIVE_L2,
                                                 l2_offset, s->cluster_size);
             if (ret < 0) {
@@ -1905,6 +1904,7 @@ static int check_oflag_copied(BlockDriverState *bs, BdrvCheckResult *res,
                 res->check_errors++;
                 goto fail;
             }
+            res->corruptions_fixed += l2_dirty;
         }
     }
 
-- 
2.18.0

[Qemu-devel] [PATCH v3 2/8] block/qcow2-refcount: avoid eating RAM

[Vladimir Sementsov-Ogievskiy]

qcow2_inc_refcounts_imrt() (through realloc_refcount_array()) can eat
an unpredictable amount of memory on corrupted table entries, which are
referencing regions far beyond the end of file.

Prevent this, by skipping such regions from further processing.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/qcow2-refcount.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index b453d87a3f..afaa1a1409 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -1499,12 +1499,30 @@ int qcow2_inc_refcounts_imrt(BlockDriverState *bs, BdrvCheckResult *res,
 {
     BDRVQcow2State *s = bs->opaque;
     uint64_t start, last, cluster_offset, k, refcount;
+    int64_t file_len;
     int ret;
 
     if (size <= 0) {
         return 0;
     }
 
+    file_len = bdrv_getlength(bs->file->bs);
+    if (file_len < 0) {
+        return file_len;
+    }
+
+    /* Last cluster of qcow2 image may be semi-allocated, so it's may be OK to
+     * reference some space after file end but it should be less than one
+     * cluster.
+     */
+    if (offset + size - file_len >= s->cluster_size) {
+        fprintf(stderr, "ERROR: counting reference for region exceeding the "
+                "end of the file by one cluster or more: offset 0x%" PRIx64
+                " size 0x%" PRIx64 "\n", offset, size);
+        res->corruptions++;
+        return 0;
+    }
+
     start = start_of_cluster(s, offset);
     last = start_of_cluster(s, offset + size - 1);
     for(cluster_offset = start; cluster_offset <= last;
-- 
2.18.0

[Qemu-devel] [PATCH v3 3/8] block/qcow2-refcount: check_refcounts_l2: refactor compressed case

[Vladimir Sementsov-Ogievskiy]

Separate offset and size of compressed cluster.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/qcow2-refcount.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index afaa1a1409..23b105b43b 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -1574,7 +1574,7 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
     BDRVQcow2State *s = bs->opaque;
     uint64_t *l2_table, l2_entry;
     uint64_t next_contiguous_offset = 0;
-    int i, l2_size, nb_csectors, ret;
+    int i, l2_size, ret;
 
     /* Read L2 table from disk */
     l2_size = s->l2_size * sizeof(uint64_t);
@@ -1593,6 +1593,9 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
 
         switch (qcow2_get_cluster_type(l2_entry)) {
         case QCOW2_CLUSTER_COMPRESSED:
+        {
+            int64_t csize, coffset;
+
             /* Compressed clusters don't have QCOW_OFLAG_COPIED */
             if (l2_entry & QCOW_OFLAG_COPIED) {
                 fprintf(stderr, "ERROR: coffset=0x%" PRIx64 ": "
@@ -1603,12 +1606,11 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
             }
 
             /* Mark cluster as used */
-            nb_csectors = ((l2_entry >> s->csize_shift) &
-                           s->csize_mask) + 1;
-            l2_entry &= s->cluster_offset_mask;
+            csize = (((l2_entry >> s->csize_shift) & s->csize_mask) + 1) * 512;
+            coffset = l2_entry & s->cluster_offset_mask & ~511;
             ret = qcow2_inc_refcounts_imrt(bs, res,
                                            refcount_table, refcount_table_size,
-                                           l2_entry & ~511, nb_csectors * 512);
+                                           coffset, csize);
             if (ret < 0) {
                 goto fail;
             }
@@ -1625,6 +1627,7 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
                 res->bfi.fragmented_clusters++;
             }
             break;
+        }
 
         case QCOW2_CLUSTER_ZERO_ALLOC:
         case QCOW2_CLUSTER_NORMAL:
-- 
2.18.0

[Qemu-devel] [PATCH v3 4/8] block/qcow2-refcount: check_refcounts_l2: reduce ignored overlaps

[Vladimir Sementsov-Ogievskiy]

Reduce number of structures ignored in overlap check: when checking
active table ignore active tables, when checking inactive table ignore
inactive ones.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 block/qcow2-refcount.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 23b105b43b..8fb9c9af39 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -1569,7 +1569,7 @@ enum {
 static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
                               void **refcount_table,
                               int64_t *refcount_table_size, int64_t l2_offset,
-                              int flags, BdrvCheckMode fix)
+                              int flags, BdrvCheckMode fix, bool active)
 {
     BDRVQcow2State *s = bs->opaque;
     uint64_t *l2_table, l2_entry;
@@ -1656,11 +1656,12 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
                     if (fix & BDRV_FIX_ERRORS) {
                         uint64_t l2e_offset =
                             l2_offset + (uint64_t)i * sizeof(uint64_t);
+                        int ign = active ? QCOW2_OL_ACTIVE_L2 :
+                                           QCOW2_OL_INACTIVE_L2;
 
                         l2_entry = QCOW_OFLAG_ZERO;
                         l2_table[i] = cpu_to_be64(l2_entry);
-                        ret = qcow2_pre_write_overlap_check(bs,
-                                QCOW2_OL_ACTIVE_L2 | QCOW2_OL_INACTIVE_L2,
+                        ret = qcow2_pre_write_overlap_check(bs, ign,
                                 l2e_offset, sizeof(uint64_t));
                         if (ret < 0) {
                             fprintf(stderr, "ERROR: Overlap check failed\n");
@@ -1734,7 +1735,7 @@ static int check_refcounts_l1(BlockDriverState *bs,
                               void **refcount_table,
                               int64_t *refcount_table_size,
                               int64_t l1_table_offset, int l1_size,
-                              int flags, BdrvCheckMode fix)
+                              int flags, BdrvCheckMode fix, bool active)
 {
     BDRVQcow2State *s = bs->opaque;
     uint64_t *l1_table = NULL, l2_offset, l1_size2;
@@ -1790,7 +1791,7 @@ static int check_refcounts_l1(BlockDriverState *bs,
             /* Process and check L2 entries */
             ret = check_refcounts_l2(bs, res, refcount_table,
                                      refcount_table_size, l2_offset, flags,
-                                     fix);
+                                     fix, active);
             if (ret < 0) {
                 goto fail;
             }
@@ -2076,7 +2077,7 @@ static int calculate_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
     /* current L1 table */
     ret = check_refcounts_l1(bs, res, refcount_table, nb_clusters,
                              s->l1_table_offset, s->l1_size, CHECK_FRAG_INFO,
-                             fix);
+                             fix, true);
     if (ret < 0) {
         return ret;
     }
@@ -2099,7 +2100,8 @@ static int calculate_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
             continue;
         }
         ret = check_refcounts_l1(bs, res, refcount_table, nb_clusters,
-                                 sn->l1_table_offset, sn->l1_size, 0, fix);
+                                 sn->l1_table_offset, sn->l1_size, 0, fix,
+                                 false);
         if (ret < 0) {
             return ret;
         }
-- 
2.18.0

[Qemu-devel] [PATCH v3 5/8] block/qcow2-refcount: check_refcounts_l2: don't count corrupted entries

[Vladimir Sementsov-Ogievskiy]

Misaligned entries will lead to fatal qcow2 driver corruption on read
or write to corresponding offset, so there is no sense to take them
into account.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/qcow2-refcount.c | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 8fb9c9af39..3b34681f16 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -1634,15 +1634,6 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
         {
             uint64_t offset = l2_entry & L2E_OFFSET_MASK;
 
-            if (flags & CHECK_FRAG_INFO) {
-                res->bfi.allocated_clusters++;
-                if (next_contiguous_offset &&
-                    offset != next_contiguous_offset) {
-                    res->bfi.fragmented_clusters++;
-                }
-                next_contiguous_offset = offset + s->cluster_size;
-            }
-
             /* Correct offsets are cluster aligned */
             if (offset_into_cluster(s, offset)) {
                 if (qcow2_get_cluster_type(l2_entry) ==
@@ -1681,9 +1672,6 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
                              * L2 table's entries */
                         } else {
                             res->corruptions_fixed++;
-                            /* Skip marking the cluster as used
-                             * (it is unused now) */
-                            continue;
                         }
                     } else {
                         res->corruptions++;
@@ -1693,6 +1681,19 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
                         "not properly aligned; L2 entry corrupted.\n", offset);
                     res->corruptions++;
                 }
+
+                /* Skip marking the cluster as used
+                 * (l2 entry is marked as zero or still fatally corrupted) */
+                continue;
+            }
+
+            if (flags & CHECK_FRAG_INFO) {
+                res->bfi.allocated_clusters++;
+                if (next_contiguous_offset &&
+                    offset != next_contiguous_offset) {
+                    res->bfi.fragmented_clusters++;
+                }
+                next_contiguous_offset = offset + s->cluster_size;
             }
 
             /* Mark cluster as used */
-- 
2.18.0

[Qemu-devel] [PATCH v3 6/8] block/qcow2-refcount: refactor fixing L2 entry

[Vladimir Sementsov-Ogievskiy]

Split entry repairing to separate function, to be reused later in
check_refcounts_l2, prepare the whole pipeline for adding more
corruption types which may be repaired.

Note: entry in in-memory l2 table (local variable in
check_refcounts_l2) is not updated after this patch.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/qcow2-refcount.c | 234 ++++++++++++++++++++++++++---------------
 1 file changed, 148 insertions(+), 86 deletions(-)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 3b34681f16..65e0fd2de3 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -1558,6 +1558,63 @@ enum {
     CHECK_FRAG_INFO = 0x2,      /* update BlockFragInfo counters */
 };
 
+typedef enum {
+    REP_ACTIVE_L1,
+    REP_INACTIVE_L1,
+    REP_ACTIVE_L2,
+    REP_INACTIVE_L2
+} RepTableType;
+
+/* repair_table_entry
+ *
+ * Rewrite entry in L1 or L2 table and set @res appropriately.
+ *
+ * Returns: -errno if overlap check failed
+ *          0 if write failed
+ *          1 on success
+ */
+static int repair_table_entry(BlockDriverState *bs, BdrvCheckResult *res,
+                              RepTableType type, uint64_t table_offset,
+                              int entry_index, uint64_t new_val)
+{
+    int ret;
+    uint64_t entry_offset =
+            table_offset + (uint64_t)entry_index * sizeof(new_val);
+
+    static const struct {
+        const char *name;
+        int ign;
+    } types[] = {
+        [REP_ACTIVE_L1] = { "L1", QCOW2_OL_ACTIVE_L1 },
+        [REP_INACTIVE_L1] = { "L1", QCOW2_OL_INACTIVE_L1 },
+        [REP_ACTIVE_L2] = { "L2", QCOW2_OL_ACTIVE_L1 },
+        [REP_INACTIVE_L2] = { "L2", QCOW2_OL_INACTIVE_L1 },
+    };
+
+    cpu_to_be64s(&new_val);
+    ret = qcow2_pre_write_overlap_check(bs, types[type].ign, entry_offset,
+                                        sizeof(new_val));
+    if (ret < 0) {
+        res->check_errors++;
+        fprintf(stderr,
+                "ERROR: Cannot write %s table entry: "
+                "overlap check failed: %s\n",
+                types[type].name, strerror(-ret));
+        return ret;
+    }
+
+    ret = bdrv_pwrite_sync(bs->file, entry_offset, &new_val, sizeof(new_val));
+    if (ret < 0) {
+        res->check_errors++;
+        fprintf(stderr, "ERROR: Failed to overwrite %s table entry: %s\n",
+                types[type].name, strerror(-ret));
+        return 0;
+    }
+
+    res->corruptions_fixed++;
+    return 1;
+}
+
 /*
  * Increases the refcount in the given refcount table for the all clusters
  * referenced in the L2 table. While doing so, performs some checks on L2
@@ -1572,9 +1629,11 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
                               int flags, BdrvCheckMode fix, bool active)
 {
     BDRVQcow2State *s = bs->opaque;
-    uint64_t *l2_table, l2_entry;
+    uint64_t *l2_table;
     uint64_t next_contiguous_offset = 0;
     int i, l2_size, ret;
+    bool fix_er = fix & BDRV_FIX_ERRORS;
+    RepTableType type = active ? REP_ACTIVE_L2 : REP_INACTIVE_L2;
 
     /* Read L2 table from disk */
     l2_size = s->l2_size * sizeof(uint64_t);
@@ -1589,13 +1648,17 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
 
     /* Do the actual checks */
     for(i = 0; i < s->l2_size; i++) {
-        l2_entry = be64_to_cpu(l2_table[i]);
-
-        switch (qcow2_get_cluster_type(l2_entry)) {
+        uint64_t l2_entry = be64_to_cpu(l2_table[i]);
+        uint64_t l2_entry_fix = l2_entry;
+        QCow2ClusterType entry_type = qcow2_get_cluster_type(l2_entry);
+        int64_t offset, size;
+        bool fatal = false; /* l2_entry is fatally corrupted, don't count it if
+                                fail to fix */
+
+        /* check for errors: either increase res->corruptions or set
+         * @l2_entry_fix to something other */
+        switch (entry_type) {
         case QCOW2_CLUSTER_COMPRESSED:
-        {
-            int64_t csize, coffset;
-
             /* Compressed clusters don't have QCOW_OFLAG_COPIED */
             if (l2_entry & QCOW_OFLAG_COPIED) {
                 fprintf(stderr, "ERROR: coffset=0x%" PRIx64 ": "
@@ -1605,113 +1668,112 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
                 res->corruptions++;
             }
 
-            /* Mark cluster as used */
-            csize = (((l2_entry >> s->csize_shift) & s->csize_mask) + 1) * 512;
-            coffset = l2_entry & s->cluster_offset_mask & ~511;
-            ret = qcow2_inc_refcounts_imrt(bs, res,
-                                           refcount_table, refcount_table_size,
-                                           coffset, csize);
-            if (ret < 0) {
-                goto fail;
-            }
-
-            if (flags & CHECK_FRAG_INFO) {
-                res->bfi.allocated_clusters++;
-                res->bfi.compressed_clusters++;
-
-                /* Compressed clusters are fragmented by nature.  Since they
-                 * take up sub-sector space but we only have sector granularity
-                 * I/O we need to re-read the same sectors even for adjacent
-                 * compressed clusters.
-                 */
-                res->bfi.fragmented_clusters++;
-            }
+            offset = l2_entry & s->cluster_offset_mask & ~511;
+            size = (((l2_entry >> s->csize_shift) & s->csize_mask) + 1) * 512;
             break;
-        }
 
         case QCOW2_CLUSTER_ZERO_ALLOC:
         case QCOW2_CLUSTER_NORMAL:
-        {
-            uint64_t offset = l2_entry & L2E_OFFSET_MASK;
+            offset = l2_entry & L2E_OFFSET_MASK;
+            size = s->cluster_size;
 
             /* Correct offsets are cluster aligned */
             if (offset_into_cluster(s, offset)) {
+                fatal = true; /* misaligned cluster leads to
+                               * qcow2_signal_corruption(fatal = true)
+                               */
                 if (qcow2_get_cluster_type(l2_entry) ==
                     QCOW2_CLUSTER_ZERO_ALLOC)
                 {
                     fprintf(stderr, "%s offset=%" PRIx64 ": Preallocated zero "
                             "cluster is not properly aligned; L2 entry "
                             "corrupted.\n",
-                            fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR",
-                            offset);
-                    if (fix & BDRV_FIX_ERRORS) {
-                        uint64_t l2e_offset =
-                            l2_offset + (uint64_t)i * sizeof(uint64_t);
-                        int ign = active ? QCOW2_OL_ACTIVE_L2 :
-                                           QCOW2_OL_INACTIVE_L2;
-
-                        l2_entry = QCOW_OFLAG_ZERO;
-                        l2_table[i] = cpu_to_be64(l2_entry);
-                        ret = qcow2_pre_write_overlap_check(bs, ign,
-                                l2e_offset, sizeof(uint64_t));
-                        if (ret < 0) {
-                            fprintf(stderr, "ERROR: Overlap check failed\n");
-                            res->check_errors++;
-                            /* Something is seriously wrong, so abort checking
-                             * this L2 table */
-                            goto fail;
-                        }
-
-                        ret = bdrv_pwrite_sync(bs->file, l2e_offset,
-                                               &l2_table[i], sizeof(uint64_t));
-                        if (ret < 0) {
-                            fprintf(stderr, "ERROR: Failed to overwrite L2 "
-                                    "table entry: %s\n", strerror(-ret));
-                            res->check_errors++;
-                            /* Do not abort, continue checking the rest of this
-                             * L2 table's entries */
-                        } else {
-                            res->corruptions_fixed++;
-                        }
-                    } else {
-                        res->corruptions++;
-                    }
+                            fix_er ? "Repairing" : "ERROR", offset);
+                    l2_entry_fix = QCOW_OFLAG_ZERO;
                 } else {
                     fprintf(stderr, "ERROR offset=%" PRIx64 ": Data cluster is "
                         "not properly aligned; L2 entry corrupted.\n", offset);
                     res->corruptions++;
                 }
+            }
 
-                /* Skip marking the cluster as used
-                 * (l2 entry is marked as zero or still fatally corrupted) */
-                continue;
+            break;
+
+        case QCOW2_CLUSTER_ZERO_PLAIN:
+        case QCOW2_CLUSTER_UNALLOCATED:
+            /* No allocation to be counted */
+            continue;
+
+        default:
+            abort();
+        }
+
+        /* handle found errors */
+        if (l2_entry_fix != l2_entry) {
+            if (fix_er) {
+                ret = repair_table_entry(bs, res, type,
+                                         l2_offset, i, l2_entry_fix);
+                if (ret < 0) {
+                    /* Something is seriously wrong, so abort checking
+                     * this L2 table */
+                    goto fail;
+                } else if (ret == 1) {
+                    /* successfully fixed */
+                    l2_entry = l2_entry_fix;
+                    entry_type = qcow2_get_cluster_type(l2_entry);
+                    fatal = false;
+                }
+            } else {
+                res->corruptions++;
             }
+        }
+        if (fatal || entry_type == QCOW2_CLUSTER_ZERO_PLAIN ||
+            entry_type == QCOW2_CLUSTER_ZERO_PLAIN)
+        {
+            /* entry is either:
+             *   fatally corrupted, and we did not fix it,
+             *   we can't count it like normal entry
+             * or:
+             *   successfully fixed to be zero/unallocated,
+             *   we should not count it.
+             */
+            continue;
+        }
+
+        ret = qcow2_inc_refcounts_imrt(bs, res,
+                                       refcount_table, refcount_table_size,
+                                       offset, size);
+        if (ret < 0) {
+            goto fail;
+        }
+
+        /* update counters */
+        if (flags & CHECK_FRAG_INFO) {
+            res->bfi.allocated_clusters++;
+            switch (entry_type) {
+            case QCOW2_CLUSTER_COMPRESSED:
+                res->bfi.compressed_clusters++;
 
-            if (flags & CHECK_FRAG_INFO) {
-                res->bfi.allocated_clusters++;
+                /* Compressed clusters are fragmented by nature.  Since they
+                 * take up sub-sector space but we only have sector granularity
+                 * I/O we need to re-read the same sectors even for adjacent
+                 * compressed clusters.
+                 */
+                res->bfi.fragmented_clusters++;
+                break;
+
+            case QCOW2_CLUSTER_ZERO_ALLOC:
+            case QCOW2_CLUSTER_NORMAL:
                 if (next_contiguous_offset &&
                     offset != next_contiguous_offset) {
                     res->bfi.fragmented_clusters++;
                 }
                 next_contiguous_offset = offset + s->cluster_size;
-            }
+                break;
 
-            /* Mark cluster as used */
-            ret = qcow2_inc_refcounts_imrt(bs, res,
-                                           refcount_table, refcount_table_size,
-                                           offset, s->cluster_size);
-            if (ret < 0) {
-                goto fail;
+            default:
+                abort();
             }
-            break;
-        }
-
-        case QCOW2_CLUSTER_ZERO_PLAIN:
-        case QCOW2_CLUSTER_UNALLOCATED:
-            break;
-
-        default:
-            abort();
         }
     }
 
-- 
2.18.0

[Qemu-devel] [PATCH v3 7/8] block/qcow2-refcount: fix out-of-file L1 entries to be zero

[Vladimir Sementsov-Ogievskiy]

Zero out corrupted L1 table entry, which reference L2 table out of
underlying file.
Zero L1 table entry means that "the L2 table and all clusters described
by this L2 table are unallocated."

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/qcow2-refcount.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 65e0fd2de3..5ebe292fc1 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -1803,6 +1803,13 @@ static int check_refcounts_l1(BlockDriverState *bs,
     BDRVQcow2State *s = bs->opaque;
     uint64_t *l1_table = NULL, l2_offset, l1_size2;
     int i, ret;
+    bool fix_er = fix & BDRV_FIX_ERRORS;
+    RepTableType type = active ? REP_ACTIVE_L1 : REP_INACTIVE_L1;
+    int64_t file_len = bdrv_getlength(bs->file->bs);
+
+    if (file_len < 0) {
+        return file_len;
+    }
 
     l1_size2 = l1_size * sizeof(uint64_t);
 
@@ -1837,6 +1844,25 @@ static int check_refcounts_l1(BlockDriverState *bs,
         if (l2_offset) {
             /* Mark L2 table as used */
             l2_offset &= L1E_OFFSET_MASK;
+            if (l2_offset >= file_len) {
+                fprintf(stderr,
+                        "%s: l2 table offset out of file: offset 0x%" PRIx64
+                        "\n", fix_er ? "Repairing" : "ERROR", l2_offset);
+                if (fix_er) {
+                    ret = repair_table_entry(bs, res, type,
+                                             l1_table_offset, i, 0);
+                    if (ret < 0) {
+                        /* Something is seriously wrong, so abort checking
+                         * this L1 table */
+                        goto fail;
+                    }
+                } else {
+                    res->corruptions++;
+                }
+
+                continue;
+            }
+
             ret = qcow2_inc_refcounts_imrt(bs, res,
                                            refcount_table, refcount_table_size,
                                            l2_offset, s->cluster_size);
-- 
2.18.0

[Qemu-devel] [PATCH v3 8/8] block/qcow2-refcount: fix out-of-file L2 entries

[Vladimir Sementsov-Ogievskiy]

Rewrite corrupted L2 table entry, which reference space out of
underlying file.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/qcow2-refcount.c | 66 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 65 insertions(+), 1 deletion(-)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 5ebe292fc1..2e2ba806ed 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -1629,11 +1629,37 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
                               int flags, BdrvCheckMode fix, bool active)
 {
     BDRVQcow2State *s = bs->opaque;
-    uint64_t *l2_table;
+    uint64_t *l2_table, zero_fix_entry;
     uint64_t next_contiguous_offset = 0;
     int i, l2_size, ret;
     bool fix_er = fix & BDRV_FIX_ERRORS;
     RepTableType type = active ? REP_ACTIVE_L2 : REP_INACTIVE_L2;
+    const char *zero_fix_desc = "";
+    int64_t file_len = bdrv_getlength(bs->file->bs);
+
+    if (file_len < 0) {
+        return file_len;
+    }
+
+    if (fix_er) {
+        /* prepare options for fixing corrupted l2 entries to be marked as zero
+         * or unallocated
+         */
+        if (s->qcow_version >= 3) {
+            zero_fix_entry = QCOW_OFLAG_ZERO;
+            zero_fix_desc = ": mark cluster as zero";
+        } else {
+            /* v2 doesn't support ZERO flag, so we just mark it unallocated.
+             * It's a bit unsafe, as backing file may become available through
+             * the hole. However, discard for v2 do the same, so we don't care
+             * too.
+             */
+            zero_fix_entry = 0;
+            zero_fix_desc = ": mark cluster as unallocated (warning: "
+                            "underlying backing file may become available "
+                            "through the hole)";
+        }
+    }
 
     /* Read L2 table from disk */
     l2_size = s->l2_size * sizeof(uint64_t);
@@ -1670,6 +1696,34 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
 
             offset = l2_entry & s->cluster_offset_mask & ~511;
             size = (((l2_entry >> s->csize_shift) & s->csize_mask) + 1) * 512;
+
+            if (offset + size > QEMU_ALIGN_UP(file_len, 512)) {
+                /* The compressed data does not necessarily occupy
+                 * all of the bytes in the final 512-bytes sector
+                 */
+                const char *fix_desc = "";
+                uint64_t orig_size = size;
+                fatal = true;
+
+                if (fix_er) {
+                    if (offset < file_len) {
+                        size = QEMU_ALIGN_UP(file_len, 512) - offset;
+                        l2_entry_fix &=
+                                ~((uint64_t)s->csize_mask << s->csize_shift);
+                        l2_entry_fix |= (size / 512 - 1) << s->csize_shift;
+                        fix_desc = ": try to fix cluster size";
+                    } else {
+                        l2_entry_fix = zero_fix_entry;
+                        fix_desc = zero_fix_desc;
+                    }
+                } else {
+                    res->corruptions++;
+                }
+                fprintf(stderr, "%s: compressed cluster out of file: "
+                        "offset 0x%" PRIx64 " size 0x%" PRIx64 "%s\n",
+                        fix_er ? "Repairing" : "ERROR",
+                        offset, orig_size, fix_desc);
+            }
             break;
 
         case QCOW2_CLUSTER_ZERO_ALLOC:
@@ -1677,6 +1731,16 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
             offset = l2_entry & L2E_OFFSET_MASK;
             size = s->cluster_size;
 
+            if (offset >= file_len) {
+                fatal = true; /* for sure, there is no allocation after EOF */
+                fprintf(stderr,
+                        "%s: cluster out of file: offset 0x%" PRIx64 "%s\n",
+                        fix_er ? "Repairing" : "ERROR", offset,
+                        fix_er ? zero_fix_desc : "");
+                l2_entry_fix = zero_fix_entry;
+                break;
+            }
+
             /* Correct offsets are cluster aligned */
             if (offset_into_cluster(s, offset)) {
                 fatal = true; /* misaligned cluster leads to
-- 
2.18.0