[PATCH v3 00/10] qcow2 check: check some reserved bits and subcluster bitmaps

[PATCH v3 00/10] qcow2 check: check some reserved bits and subcluster bitmaps

[Vladimir Sementsov-Ogievskiy]

Hi all!

Here are some good refactorings and new (qemu-img check) checks for
qcow2.

v3: add r-b mark by Alberto and t-b marks by Kirill
 07, 09: add missed "\n"

Vladimir Sementsov-Ogievskiy (10):
  qcow2-refcount: improve style of check_refcounts_l2()
  qcow2: compressed read: simplify cluster descriptor passing
  qcow2: introduce qcow2_parse_compressed_l2_entry() helper
  qcow2-refcount: introduce fix_l2_entry_by_zero()
  qcow2-refcount: fix_l2_entry_by_zero(): also zero L2 entry bitmap
  qcow2-refcount: check_refcounts_l2(): check l2_bitmap
  qcow2-refcount: check_refcounts_l2(): check reserved bits
  qcow2-refcount: improve style of check_refcounts_l1()
  qcow2-refcount: check_refcounts_l1(): check reserved bits
  qcow2-refcount: check_refblocks(): add separate message for reserved

 block/qcow2.h          |   7 +-
 block/qcow2-cluster.c  |  20 ++-
 block/qcow2-refcount.c | 328 ++++++++++++++++++++++++++---------------
 block/qcow2.c          |  13 +-
 4 files changed, 240 insertions(+), 128 deletions(-)

-- 
2.29.2

[PATCH v3 01/10] qcow2-refcount: improve style of check_refcounts_l2()

[Vladimir Sementsov-Ogievskiy]

 - don't use same name for size in bytes and in entries
 - use g_autofree for l2_table
 - add whitespace
 - fix block comment style

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block/qcow2-refcount.c | 47 +++++++++++++++++++++---------------------
 1 file changed, 24 insertions(+), 23 deletions(-)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 8e649b008e..2734338625 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -1601,23 +1601,22 @@ 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_entry;
     uint64_t next_contiguous_offset = 0;
-    int i, l2_size, nb_csectors, ret;
+    int i, nb_csectors, ret;
+    size_t l2_size_bytes = s->l2_size * l2_entry_size(s);
+    g_autofree uint64_t *l2_table = g_malloc(l2_size_bytes);
 
     /* Read L2 table from disk */
-    l2_size = s->l2_size * l2_entry_size(s);
-    l2_table = g_malloc(l2_size);
-
-    ret = bdrv_pread(bs->file, l2_offset, l2_table, l2_size);
+    ret = bdrv_pread(bs->file, l2_offset, l2_table, l2_size_bytes);
     if (ret < 0) {
         fprintf(stderr, "ERROR: I/O error in check_refcounts_l2\n");
         res->check_errors++;
-        goto fail;
+        return ret;
     }
 
     /* Do the actual checks */
-    for(i = 0; i < s->l2_size; i++) {
+    for (i = 0; i < s->l2_size; i++) {
         l2_entry = get_l2_entry(s, l2_table, i);
 
         switch (qcow2_get_cluster_type(bs, l2_entry)) {
@@ -1647,14 +1646,15 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
                 l2_entry & QCOW2_COMPRESSED_SECTOR_MASK,
                 nb_csectors * QCOW2_COMPRESSED_SECTOR_SIZE);
             if (ret < 0) {
-                goto fail;
+                return ret;
             }
 
             if (flags & CHECK_FRAG_INFO) {
                 res->bfi.allocated_clusters++;
                 res->bfi.compressed_clusters++;
 
-                /* Compressed clusters are fragmented by nature.  Since they
+                /*
+                 * 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.
@@ -1700,9 +1700,11 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
                         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;
+                            /*
+                             * Something is seriously wrong, so abort checking
+                             * this L2 table.
+                             */
+                            return ret;
                         }
 
                         ret = bdrv_pwrite_sync(bs->file, l2e_offset,
@@ -1712,13 +1714,17 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
                             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 */
+                            /*
+                             * Do not abort, continue checking the rest of this
+                             * L2 table's entries.
+                             */
                         } else {
                             res->corruptions--;
                             res->corruptions_fixed++;
-                            /* Skip marking the cluster as used
-                             * (it is unused now) */
+                            /*
+                             * Skip marking the cluster as used
+                             * (it is unused now).
+                             */
                             continue;
                         }
                     }
@@ -1743,7 +1749,7 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
                                                refcount_table_size,
                                                offset, s->cluster_size);
                 if (ret < 0) {
-                    goto fail;
+                    return ret;
                 }
             }
             break;
@@ -1758,12 +1764,7 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
         }
     }
 
-    g_free(l2_table);
     return 0;
-
-fail:
-    g_free(l2_table);
-    return ret;
 }
 
 /*
-- 
2.29.2

[PATCH v3 02/10] qcow2: compressed read: simplify cluster descriptor passing

[Vladimir Sementsov-Ogievskiy]

Let's pass the whole L2 entry and not bother with
L2E_COMPRESSED_OFFSET_SIZE_MASK.

It also helps further refactoring that adds generic
qcow2_parse_compressed_l2_entry() helper.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
 block/qcow2.h         |  1 -
 block/qcow2-cluster.c |  5 ++---
 block/qcow2.c         | 12 +++++++-----
 3 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/block/qcow2.h b/block/qcow2.h
index 0fe5f74ed3..42a0058ab7 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -588,7 +588,6 @@ typedef enum QCow2MetadataOverlap {
 
 #define L1E_OFFSET_MASK 0x00fffffffffffe00ULL
 #define L2E_OFFSET_MASK 0x00fffffffffffe00ULL
-#define L2E_COMPRESSED_OFFSET_SIZE_MASK 0x3fffffffffffffffULL
 
 #define REFT_OFFSET_MASK 0xfffffffffffffe00ULL
 
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index bd0597842f..04735ee439 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -556,8 +556,7 @@ static int coroutine_fn do_perform_cow_write(BlockDriverState *bs,
  * offset needs to be aligned to a cluster boundary.
  *
  * If the cluster is unallocated then *host_offset will be 0.
- * If the cluster is compressed then *host_offset will contain the
- * complete compressed cluster descriptor.
+ * If the cluster is compressed then *host_offset will contain the l2 entry.
  *
  * On entry, *bytes is the maximum number of contiguous bytes starting at
  * offset that we are interested in.
@@ -660,7 +659,7 @@ int qcow2_get_host_offset(BlockDriverState *bs, uint64_t offset,
             ret = -EIO;
             goto fail;
         }
-        *host_offset = l2_entry & L2E_COMPRESSED_OFFSET_SIZE_MASK;
+        *host_offset = l2_entry;
         break;
     case QCOW2_SUBCLUSTER_ZERO_PLAIN:
     case QCOW2_SUBCLUSTER_UNALLOCATED_PLAIN:
diff --git a/block/qcow2.c b/block/qcow2.c
index 39b91ef940..b3648f0ba5 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -74,7 +74,7 @@ typedef struct {
 
 static int coroutine_fn
 qcow2_co_preadv_compressed(BlockDriverState *bs,
-                           uint64_t cluster_descriptor,
+                           uint64_t l2_entry,
                            uint64_t offset,
                            uint64_t bytes,
                            QEMUIOVector *qiov,
@@ -2177,7 +2177,7 @@ typedef struct Qcow2AioTask {
 
     BlockDriverState *bs;
     QCow2SubclusterType subcluster_type; /* only for read */
-    uint64_t host_offset; /* or full descriptor in compressed clusters */
+    uint64_t host_offset; /* or l2_entry for compressed read */
     uint64_t offset;
     uint64_t bytes;
     QEMUIOVector *qiov;
@@ -4665,7 +4665,7 @@ qcow2_co_pwritev_compressed_part(BlockDriverState *bs,
 
 static int coroutine_fn
 qcow2_co_preadv_compressed(BlockDriverState *bs,
-                           uint64_t cluster_descriptor,
+                           uint64_t l2_entry,
                            uint64_t offset,
                            uint64_t bytes,
                            QEMUIOVector *qiov,
@@ -4677,8 +4677,10 @@ qcow2_co_preadv_compressed(BlockDriverState *bs,
     uint8_t *buf, *out_buf;
     int offset_in_cluster = offset_into_cluster(s, offset);
 
-    coffset = cluster_descriptor & s->cluster_offset_mask;
-    nb_csectors = ((cluster_descriptor >> s->csize_shift) & s->csize_mask) + 1;
+    assert(qcow2_get_cluster_type(bs, l2_entry) == QCOW2_CLUSTER_COMPRESSED);
+
+    coffset = l2_entry & s->cluster_offset_mask;
+    nb_csectors = ((l2_entry >> s->csize_shift) & s->csize_mask) + 1;
     csize = nb_csectors * QCOW2_COMPRESSED_SECTOR_SIZE -
         (coffset & ~QCOW2_COMPRESSED_SECTOR_MASK);
 
-- 
2.29.2

[PATCH v3 03/10] qcow2: introduce qcow2_parse_compressed_l2_entry() helper

[Vladimir Sementsov-Ogievskiy]

Add helper to parse compressed l2_entry and use it everywhere instead
of open-coding.

Note, that in most places we move to precise coffset/csize instead of
sector-aligned. Still it should work good enough for updating
refcounts.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block/qcow2.h          |  3 ++-
 block/qcow2-cluster.c  | 15 +++++++++++++++
 block/qcow2-refcount.c | 36 +++++++++++++++++-------------------
 block/qcow2.c          |  9 ++-------
 4 files changed, 36 insertions(+), 27 deletions(-)

diff --git a/block/qcow2.h b/block/qcow2.h
index 42a0058ab7..c0e1e83796 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -110,7 +110,6 @@
 
 /* Defined in the qcow2 spec (compressed cluster descriptor) */
 #define QCOW2_COMPRESSED_SECTOR_SIZE 512U
-#define QCOW2_COMPRESSED_SECTOR_MASK (~(QCOW2_COMPRESSED_SECTOR_SIZE - 1ULL))
 
 /* Must be at least 2 to cover COW */
 #define MIN_L2_CACHE_SIZE 2 /* cache entries */
@@ -913,6 +912,8 @@ int qcow2_alloc_compressed_cluster_offset(BlockDriverState *bs,
                                           uint64_t offset,
                                           int compressed_size,
                                           uint64_t *host_offset);
+void qcow2_parse_compressed_l2_entry(BlockDriverState *bs, uint64_t l2_entry,
+                                     uint64_t *coffset, int *csize);
 
 int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, QCowL2Meta *m);
 void qcow2_alloc_cluster_abort(BlockDriverState *bs, QCowL2Meta *m);
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 04735ee439..70d0570a33 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -2462,3 +2462,18 @@ fail:
     g_free(l1_table);
     return ret;
 }
+
+void qcow2_parse_compressed_l2_entry(BlockDriverState *bs, uint64_t l2_entry,
+                                     uint64_t *coffset, int *csize)
+{
+    BDRVQcow2State *s = bs->opaque;
+    int nb_csectors;
+
+    assert(qcow2_get_cluster_type(bs, l2_entry) == QCOW2_CLUSTER_COMPRESSED);
+
+    *coffset = l2_entry & s->cluster_offset_mask;
+
+    nb_csectors = ((l2_entry >> s->csize_shift) & s->csize_mask) + 1;
+    *csize = nb_csectors * QCOW2_COMPRESSED_SECTOR_SIZE -
+        (*coffset & (QCOW2_COMPRESSED_SECTOR_SIZE - 1));
+}
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 2734338625..66cbb94ef9 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -1177,11 +1177,11 @@ void qcow2_free_any_cluster(BlockDriverState *bs, uint64_t l2_entry,
     switch (ctype) {
     case QCOW2_CLUSTER_COMPRESSED:
         {
-            int64_t offset = (l2_entry & s->cluster_offset_mask)
-                & QCOW2_COMPRESSED_SECTOR_MASK;
-            int size = QCOW2_COMPRESSED_SECTOR_SIZE *
-                (((l2_entry >> s->csize_shift) & s->csize_mask) + 1);
-            qcow2_free_clusters(bs, offset, size, type);
+            uint64_t coffset;
+            int csize;
+
+            qcow2_parse_compressed_l2_entry(bs, l2_entry, &coffset, &csize);
+            qcow2_free_clusters(bs, coffset, csize, type);
         }
         break;
     case QCOW2_CLUSTER_NORMAL:
@@ -1247,7 +1247,7 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs,
     bool l1_allocated = false;
     int64_t old_entry, old_l2_offset;
     unsigned slice, slice_size2, n_slices;
-    int i, j, l1_modified = 0, nb_csectors;
+    int i, j, l1_modified = 0;
     int ret;
 
     assert(addend >= -1 && addend <= 1);
@@ -1318,14 +1318,14 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs,
 
                     switch (qcow2_get_cluster_type(bs, entry)) {
                     case QCOW2_CLUSTER_COMPRESSED:
-                        nb_csectors = ((entry >> s->csize_shift) &
-                                       s->csize_mask) + 1;
                         if (addend != 0) {
-                            uint64_t coffset = (entry & s->cluster_offset_mask)
-                                & QCOW2_COMPRESSED_SECTOR_MASK;
+                            uint64_t coffset;
+                            int csize;
+
+                            qcow2_parse_compressed_l2_entry(bs, entry,
+                                                            &coffset, &csize);
                             ret = update_refcount(
-                                bs, coffset,
-                                nb_csectors * QCOW2_COMPRESSED_SECTOR_SIZE,
+                                bs, coffset, csize,
                                 abs(addend), addend < 0,
                                 QCOW2_DISCARD_SNAPSHOT);
                             if (ret < 0) {
@@ -1603,7 +1603,7 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
     BDRVQcow2State *s = bs->opaque;
     uint64_t l2_entry;
     uint64_t next_contiguous_offset = 0;
-    int i, nb_csectors, ret;
+    int i, ret;
     size_t l2_size_bytes = s->l2_size * l2_entry_size(s);
     g_autofree uint64_t *l2_table = g_malloc(l2_size_bytes);
 
@@ -1617,6 +1617,8 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
 
     /* Do the actual checks */
     for (i = 0; i < s->l2_size; i++) {
+        uint64_t coffset;
+        int csize;
         l2_entry = get_l2_entry(s, l2_table, i);
 
         switch (qcow2_get_cluster_type(bs, l2_entry)) {
@@ -1638,13 +1640,9 @@ 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;
+            qcow2_parse_compressed_l2_entry(bs, l2_entry, &coffset, &csize);
             ret = qcow2_inc_refcounts_imrt(
-                bs, res, refcount_table, refcount_table_size,
-                l2_entry & QCOW2_COMPRESSED_SECTOR_MASK,
-                nb_csectors * QCOW2_COMPRESSED_SECTOR_SIZE);
+                bs, res, refcount_table, refcount_table_size, coffset, csize);
             if (ret < 0) {
                 return ret;
             }
diff --git a/block/qcow2.c b/block/qcow2.c
index b3648f0ba5..56a69b26c1 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -4672,17 +4672,12 @@ qcow2_co_preadv_compressed(BlockDriverState *bs,
                            size_t qiov_offset)
 {
     BDRVQcow2State *s = bs->opaque;
-    int ret = 0, csize, nb_csectors;
+    int ret = 0, csize;
     uint64_t coffset;
     uint8_t *buf, *out_buf;
     int offset_in_cluster = offset_into_cluster(s, offset);
 
-    assert(qcow2_get_cluster_type(bs, l2_entry) == QCOW2_CLUSTER_COMPRESSED);
-
-    coffset = l2_entry & s->cluster_offset_mask;
-    nb_csectors = ((l2_entry >> s->csize_shift) & s->csize_mask) + 1;
-    csize = nb_csectors * QCOW2_COMPRESSED_SECTOR_SIZE -
-        (coffset & ~QCOW2_COMPRESSED_SECTOR_MASK);
+    qcow2_parse_compressed_l2_entry(bs, l2_entry, &coffset, &csize);
 
     buf = g_try_malloc(csize);
     if (!buf) {
-- 
2.29.2

[PATCH v3 04/10] qcow2-refcount: introduce fix_l2_entry_by_zero()

[Vladimir Sementsov-Ogievskiy]

Split fix_l2_entry_by_zero() out of check_refcounts_l2() to be
reused in further patch.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block/qcow2-refcount.c | 87 +++++++++++++++++++++++++++++-------------
 1 file changed, 60 insertions(+), 27 deletions(-)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 66cbb94ef9..184b96ad63 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -1587,6 +1587,54 @@ enum {
     CHECK_FRAG_INFO = 0x2,      /* update BlockFragInfo counters */
 };
 
+/*
+ * Fix L2 entry by making it QCOW2_CLUSTER_ZERO_PLAIN.
+ *
+ * This function decrements res->corruptions on success, so the caller is
+ * responsible to increment res->corruptions prior to the call.
+ *
+ * On failure in-memory @l2_table may be modified.
+ */
+static int fix_l2_entry_by_zero(BlockDriverState *bs, BdrvCheckResult *res,
+                                uint64_t l2_offset,
+                                uint64_t *l2_table, int l2_index, bool active,
+                                bool *metadata_overlap)
+{
+    BDRVQcow2State *s = bs->opaque;
+    int ret;
+    int idx = l2_index * (l2_entry_size(s) / sizeof(uint64_t));
+    uint64_t l2e_offset = l2_offset + (uint64_t)l2_index * l2_entry_size(s);
+    int ign = active ? QCOW2_OL_ACTIVE_L2 : QCOW2_OL_INACTIVE_L2;
+    uint64_t l2_entry = has_subclusters(s) ? 0 : QCOW_OFLAG_ZERO;
+
+    set_l2_entry(s, l2_table, l2_index, l2_entry);
+    ret = qcow2_pre_write_overlap_check(bs, ign, l2e_offset, l2_entry_size(s),
+                                        false);
+    if (metadata_overlap) {
+        *metadata_overlap = ret < 0;
+    }
+    if (ret < 0) {
+        fprintf(stderr, "ERROR: Overlap check failed\n");
+        goto fail;
+    }
+
+    ret = bdrv_pwrite_sync(bs->file, l2e_offset, &l2_table[idx],
+                           l2_entry_size(s));
+    if (ret < 0) {
+        fprintf(stderr, "ERROR: Failed to overwrite L2 "
+                "table entry: %s\n", strerror(-ret));
+        goto fail;
+    }
+
+    res->corruptions--;
+    res->corruptions_fixed++;
+    return 0;
+
+fail:
+    res->check_errors++;
+    return ret;
+}
+
 /*
  * 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
@@ -1606,6 +1654,7 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
     int i, ret;
     size_t l2_size_bytes = s->l2_size * l2_entry_size(s);
     g_autofree uint64_t *l2_table = g_malloc(l2_size_bytes);
+    bool metadata_overlap;
 
     /* Read L2 table from disk */
     ret = bdrv_pread(bs->file, l2_offset, l2_table, l2_size_bytes);
@@ -1685,19 +1734,10 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
                             fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR",
                             offset);
                     if (fix & BDRV_FIX_ERRORS) {
-                        int idx = i * (l2_entry_size(s) / sizeof(uint64_t));
-                        uint64_t l2e_offset =
-                            l2_offset + (uint64_t)i * l2_entry_size(s);
-                        int ign = active ? QCOW2_OL_ACTIVE_L2 :
-                                           QCOW2_OL_INACTIVE_L2;
-
-                        l2_entry = has_subclusters(s) ? 0 : QCOW_OFLAG_ZERO;
-                        set_l2_entry(s, l2_table, i, l2_entry);
-                        ret = qcow2_pre_write_overlap_check(bs, ign,
-                                l2e_offset, l2_entry_size(s), false);
-                        if (ret < 0) {
-                            fprintf(stderr, "ERROR: Overlap check failed\n");
-                            res->check_errors++;
+                        ret = fix_l2_entry_by_zero(bs, res, l2_offset,
+                                                   l2_table, i, active,
+                                                   &metadata_overlap);
+                        if (metadata_overlap) {
                             /*
                              * Something is seriously wrong, so abort checking
                              * this L2 table.
@@ -1705,26 +1745,19 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
                             return ret;
                         }
 
-                        ret = bdrv_pwrite_sync(bs->file, l2e_offset,
-                                               &l2_table[idx],
-                                               l2_entry_size(s));
-                        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--;
-                            res->corruptions_fixed++;
+                        if (ret == 0) {
                             /*
                              * Skip marking the cluster as used
                              * (it is unused now).
                              */
                             continue;
                         }
+
+                        /*
+                         * Failed to fix.
+                         * Do not abort, continue checking the rest of this
+                         * L2 table's entries.
+                         */
                     }
                 } else {
                     fprintf(stderr, "ERROR offset=%" PRIx64 ": Data cluster is "
-- 
2.29.2

[PATCH v3 05/10] qcow2-refcount: fix_l2_entry_by_zero(): also zero L2 entry bitmap

[Vladimir Sementsov-Ogievskiy]

We'll reuse the function to fix wrong L2 entry bitmap. Support it now.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block/qcow2-refcount.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 184b96ad63..f48c5e1b5d 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -1588,7 +1588,8 @@ enum {
 };
 
 /*
- * Fix L2 entry by making it QCOW2_CLUSTER_ZERO_PLAIN.
+ * Fix L2 entry by making it QCOW2_CLUSTER_ZERO_PLAIN (or making all its present
+ * subclusters QCOW2_SUBCLUSTER_ZERO_PLAIN).
  *
  * This function decrements res->corruptions on success, so the caller is
  * responsible to increment res->corruptions prior to the call.
@@ -1605,9 +1606,20 @@ static int fix_l2_entry_by_zero(BlockDriverState *bs, BdrvCheckResult *res,
     int idx = l2_index * (l2_entry_size(s) / sizeof(uint64_t));
     uint64_t l2e_offset = l2_offset + (uint64_t)l2_index * l2_entry_size(s);
     int ign = active ? QCOW2_OL_ACTIVE_L2 : QCOW2_OL_INACTIVE_L2;
-    uint64_t l2_entry = has_subclusters(s) ? 0 : QCOW_OFLAG_ZERO;
 
-    set_l2_entry(s, l2_table, l2_index, l2_entry);
+    if (has_subclusters(s)) {
+        uint64_t l2_bitmap = get_l2_bitmap(s, l2_table, l2_index);
+
+        /* Allocated subclusters become zero */
+        l2_bitmap |= l2_bitmap << 32;
+        l2_bitmap &= QCOW_L2_BITMAP_ALL_ZEROES;
+
+        set_l2_bitmap(s, l2_table, l2_index, l2_bitmap);
+        set_l2_entry(s, l2_table, l2_index, 0);
+    } else {
+        set_l2_entry(s, l2_table, l2_index, QCOW_OFLAG_ZERO);
+    }
+
     ret = qcow2_pre_write_overlap_check(bs, ign, l2e_offset, l2_entry_size(s),
                                         false);
     if (metadata_overlap) {
-- 
2.29.2

[PATCH v3 06/10] qcow2-refcount: check_refcounts_l2(): check l2_bitmap

[Vladimir Sementsov-Ogievskiy]

Check subcluster bitmap of the l2 entry for different types of
clusters:

 - for compressed it must be zero
 - for allocated check consistency of two parts of the bitmap
 - for unallocated all subclusters should be unallocated
   (or zero-plain)

For unallocated clusters we can safely fix the entry by making it
zero-plain.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Tested-by: Kirill Tkhai <ktkhai@virtuozzo.com>
---
 block/qcow2-refcount.c | 30 +++++++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index f48c5e1b5d..062ec48a15 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -1681,6 +1681,7 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
         uint64_t coffset;
         int csize;
         l2_entry = get_l2_entry(s, l2_table, i);
+        uint64_t l2_bitmap = get_l2_bitmap(s, l2_table, i);
 
         switch (qcow2_get_cluster_type(bs, l2_entry)) {
         case QCOW2_CLUSTER_COMPRESSED:
@@ -1700,6 +1701,14 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
                 break;
             }
 
+            if (l2_bitmap) {
+                fprintf(stderr, "ERROR compressed cluster %d with non-zero "
+                        "subcluster allocation bitmap, entry=0x%" PRIx64 "\n",
+                        i, l2_entry);
+                res->corruptions++;
+                break;
+            }
+
             /* Mark cluster as used */
             qcow2_parse_compressed_l2_entry(bs, l2_entry, &coffset, &csize);
             ret = qcow2_inc_refcounts_imrt(
@@ -1727,13 +1736,19 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
         {
             uint64_t offset = l2_entry & L2E_OFFSET_MASK;
 
+            if ((l2_bitmap >> 32) & l2_bitmap) {
+                res->corruptions++;
+                fprintf(stderr, "ERROR offset=%" PRIx64 ": Allocated "
+                        "cluster has corrupted subcluster allocation bitmap\n",
+                        offset);
+            }
+
             /* Correct offsets are cluster aligned */
             if (offset_into_cluster(s, offset)) {
                 bool contains_data;
                 res->corruptions++;
 
                 if (has_subclusters(s)) {
-                    uint64_t l2_bitmap = get_l2_bitmap(s, l2_table, i);
                     contains_data = (l2_bitmap & QCOW_L2_BITMAP_ALL_ALLOC);
                 } else {
                     contains_data = !(l2_entry & QCOW_OFLAG_ZERO);
@@ -1800,6 +1815,19 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
 
         case QCOW2_CLUSTER_ZERO_PLAIN:
         case QCOW2_CLUSTER_UNALLOCATED:
+            if (l2_bitmap & QCOW_L2_BITMAP_ALL_ALLOC) {
+                res->corruptions++;
+                fprintf(stderr, "%s: Unallocated "
+                        "cluster has non-zero subcluster allocation map\n",
+                        fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR");
+                if (fix & BDRV_FIX_ERRORS) {
+                    ret = fix_l2_entry_by_zero(bs, res, l2_offset, l2_table, i,
+                                               active, &metadata_overlap);
+                    if (metadata_overlap) {
+                        return ret;
+                    }
+                }
+            }
             break;
 
         default:
-- 
2.29.2

[PATCH v3 07/10] qcow2-refcount: check_refcounts_l2(): check reserved bits

[Vladimir Sementsov-Ogievskiy]

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Tested-by: Kirill Tkhai <ktkhai@virtuozzo.com>
---
 block/qcow2.h          |  1 +
 block/qcow2-refcount.c | 12 +++++++++++-
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/block/qcow2.h b/block/qcow2.h
index c0e1e83796..b8b1093b61 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -587,6 +587,7 @@ typedef enum QCow2MetadataOverlap {
 
 #define L1E_OFFSET_MASK 0x00fffffffffffe00ULL
 #define L2E_OFFSET_MASK 0x00fffffffffffe00ULL
+#define L2E_STD_RESERVED_MASK 0x3f000000000001feULL
 
 #define REFT_OFFSET_MASK 0xfffffffffffffe00ULL
 
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 062ec48a15..b8fd6337d5 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -1682,8 +1682,18 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
         int csize;
         l2_entry = get_l2_entry(s, l2_table, i);
         uint64_t l2_bitmap = get_l2_bitmap(s, l2_table, i);
+        QCow2ClusterType type = qcow2_get_cluster_type(bs, l2_entry);
 
-        switch (qcow2_get_cluster_type(bs, l2_entry)) {
+        if (type != QCOW2_CLUSTER_COMPRESSED) {
+            /* Check reserved bits of Standard Cluster Descriptor */
+            if (l2_entry & L2E_STD_RESERVED_MASK) {
+                fprintf(stderr, "ERROR found l2 entry with reserved bits set: "
+                        "%" PRIx64 "\n", l2_entry);
+                res->corruptions++;
+            }
+        }
+
+        switch (type) {
         case QCOW2_CLUSTER_COMPRESSED:
             /* Compressed clusters don't have QCOW_OFLAG_COPIED */
             if (l2_entry & QCOW_OFLAG_COPIED) {
-- 
2.29.2

[PATCH v3 08/10] qcow2-refcount: improve style of check_refcounts_l1()

[Vladimir Sementsov-Ogievskiy]

 - use g_autofree for l1_table
 - better name for size in bytes variable
 - reduce code blocks nesting
 - whitespaces, braces, newlines

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

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index b8fd6337d5..adebb15598 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -1864,71 +1864,73 @@ static int check_refcounts_l1(BlockDriverState *bs,
                               int flags, BdrvCheckMode fix, bool active)
 {
     BDRVQcow2State *s = bs->opaque;
-    uint64_t *l1_table = NULL, l2_offset, l1_size2;
+    size_t l1_size_bytes = l1_size * L1E_SIZE;
+    g_autofree uint64_t *l1_table = NULL;
+    uint64_t l2_offset;
     int i, ret;
 
-    l1_size2 = l1_size * L1E_SIZE;
+    if (!l1_size) {
+        return 0;
+    }
 
     /* Mark L1 table as used */
     ret = qcow2_inc_refcounts_imrt(bs, res, refcount_table, refcount_table_size,
-                                   l1_table_offset, l1_size2);
+                                   l1_table_offset, l1_size_bytes);
     if (ret < 0) {
-        goto fail;
+        return ret;
+    }
+
+    l1_table = g_try_malloc(l1_size_bytes);
+    if (l1_table == NULL) {
+        res->check_errors++;
+        return -ENOMEM;
     }
 
     /* Read L1 table entries from disk */
-    if (l1_size2 > 0) {
-        l1_table = g_try_malloc(l1_size2);
-        if (l1_table == NULL) {
-            ret = -ENOMEM;
-            res->check_errors++;
-            goto fail;
-        }
-        ret = bdrv_pread(bs->file, l1_table_offset, l1_table, l1_size2);
-        if (ret < 0) {
-            fprintf(stderr, "ERROR: I/O error in check_refcounts_l1\n");
-            res->check_errors++;
-            goto fail;
-        }
-        for(i = 0;i < l1_size; i++)
-            be64_to_cpus(&l1_table[i]);
+    ret = bdrv_pread(bs->file, l1_table_offset, l1_table, l1_size_bytes);
+    if (ret < 0) {
+        fprintf(stderr, "ERROR: I/O error in check_refcounts_l1\n");
+        res->check_errors++;
+        return ret;
+    }
+
+    for (i = 0; i < l1_size; i++) {
+        be64_to_cpus(&l1_table[i]);
     }
 
     /* Do the actual checks */
-    for(i = 0; i < l1_size; i++) {
-        l2_offset = l1_table[i];
-        if (l2_offset) {
-            /* Mark L2 table as used */
-            l2_offset &= L1E_OFFSET_MASK;
-            ret = qcow2_inc_refcounts_imrt(bs, res,
-                                           refcount_table, refcount_table_size,
-                                           l2_offset, s->cluster_size);
-            if (ret < 0) {
-                goto fail;
-            }
+    for (i = 0; i < l1_size; i++) {
+        if (!l1_table[i]) {
+            continue;
+        }
 
-            /* L2 tables are cluster aligned */
-            if (offset_into_cluster(s, l2_offset)) {
-                fprintf(stderr, "ERROR l2_offset=%" PRIx64 ": Table is not "
-                    "cluster aligned; L1 entry corrupted\n", l2_offset);
-                res->corruptions++;
-            }
+        l2_offset = l1_table[i] & L1E_OFFSET_MASK;
 
-            /* Process and check L2 entries */
-            ret = check_refcounts_l2(bs, res, refcount_table,
-                                     refcount_table_size, l2_offset, flags,
-                                     fix, active);
-            if (ret < 0) {
-                goto fail;
-            }
+        /* Mark L2 table as used */
+        ret = qcow2_inc_refcounts_imrt(bs, res,
+                                       refcount_table, refcount_table_size,
+                                       l2_offset, s->cluster_size);
+        if (ret < 0) {
+            return ret;
+        }
+
+        /* L2 tables are cluster aligned */
+        if (offset_into_cluster(s, l2_offset)) {
+            fprintf(stderr, "ERROR l2_offset=%" PRIx64 ": Table is not "
+                "cluster aligned; L1 entry corrupted\n", l2_offset);
+            res->corruptions++;
+        }
+
+        /* Process and check L2 entries */
+        ret = check_refcounts_l2(bs, res, refcount_table,
+                                 refcount_table_size, l2_offset, flags,
+                                 fix, active);
+        if (ret < 0) {
+            return ret;
         }
     }
-    g_free(l1_table);
-    return 0;
 
-fail:
-    g_free(l1_table);
-    return ret;
+    return 0;
 }
 
 /*
-- 
2.29.2

[PATCH v3 09/10] qcow2-refcount: check_refcounts_l1(): check reserved bits

[Vladimir Sementsov-Ogievskiy]

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Tested-by: Kirill Tkhai <ktkhai@virtuozzo.com>
---
 block/qcow2.h          | 1 +
 block/qcow2-refcount.c | 6 ++++++
 2 files changed, 7 insertions(+)

diff --git a/block/qcow2.h b/block/qcow2.h
index b8b1093b61..58fd7f1678 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -586,6 +586,7 @@ typedef enum QCow2MetadataOverlap {
     (QCOW2_OL_CACHED | QCOW2_OL_INACTIVE_L2)
 
 #define L1E_OFFSET_MASK 0x00fffffffffffe00ULL
+#define L1E_RESERVED_MASK 0x7f000000000001ffULL
 #define L2E_OFFSET_MASK 0x00fffffffffffe00ULL
 #define L2E_STD_RESERVED_MASK 0x3f000000000001feULL
 
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index adebb15598..5903e058b9 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -1904,6 +1904,12 @@ static int check_refcounts_l1(BlockDriverState *bs,
             continue;
         }
 
+        if (l1_table[i] & L1E_RESERVED_MASK) {
+            fprintf(stderr, "ERROR found L1 entry with reserved bits set: "
+                    "%" PRIx64 "\n", l1_table[i]);
+            res->corruptions++;
+        }
+
         l2_offset = l1_table[i] & L1E_OFFSET_MASK;
 
         /* Mark L2 table as used */
-- 
2.29.2

[PATCH v3 10/10] qcow2-refcount: check_refblocks(): add separate message for reserved

[Vladimir Sementsov-Ogievskiy]

Split checking for reserved bits out of aligned offset check.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Tested-by: Kirill Tkhai <ktkhai@virtuozzo.com>
---
 block/qcow2.h          |  1 +
 block/qcow2-refcount.c | 10 +++++++++-
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/block/qcow2.h b/block/qcow2.h
index 58fd7f1678..fd48a89d45 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -591,6 +591,7 @@ typedef enum QCow2MetadataOverlap {
 #define L2E_STD_RESERVED_MASK 0x3f000000000001feULL
 
 #define REFT_OFFSET_MASK 0xfffffffffffffe00ULL
+#define REFT_RESERVED_MASK 0x1ffULL
 
 #define INV_OFFSET (-1ULL)
 
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 5903e058b9..57311c5610 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -2091,9 +2091,17 @@ static int check_refblocks(BlockDriverState *bs, BdrvCheckResult *res,
 
     for(i = 0; i < s->refcount_table_size; i++) {
         uint64_t offset, cluster;
-        offset = s->refcount_table[i];
+        offset = s->refcount_table[i] & REFT_OFFSET_MASK;
         cluster = offset >> s->cluster_bits;
 
+        if (s->refcount_table[i] & REFT_RESERVED_MASK) {
+            fprintf(stderr, "ERROR refcount table entry %" PRId64 " has "
+                    "reserved bits set\n", i);
+            res->corruptions++;
+            *rebuild = true;
+            continue;
+        }
+
         /* Refcount blocks are cluster aligned */
         if (offset_into_cluster(s, offset)) {
             fprintf(stderr, "ERROR refcount block %" PRId64 " is not "
-- 
2.29.2

Re: [PATCH v3 00/10] qcow2 check: check some reserved bits and subcluster bitmaps

[Vladimir Sementsov-Ogievskiy]

Ping :)

This still applies to master with no conflicts. All patches reviewed except for 08.


24.05.2021 17:20, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
> 
> Here are some good refactorings and new (qemu-img check) checks for
> qcow2.
> 
> v3: add r-b mark by Alberto and t-b marks by Kirill
>   07, 09: add missed "\n"
> 
> Vladimir Sementsov-Ogievskiy (10):
>    qcow2-refcount: improve style of check_refcounts_l2()
>    qcow2: compressed read: simplify cluster descriptor passing
>    qcow2: introduce qcow2_parse_compressed_l2_entry() helper
>    qcow2-refcount: introduce fix_l2_entry_by_zero()
>    qcow2-refcount: fix_l2_entry_by_zero(): also zero L2 entry bitmap
>    qcow2-refcount: check_refcounts_l2(): check l2_bitmap
>    qcow2-refcount: check_refcounts_l2(): check reserved bits
>    qcow2-refcount: improve style of check_refcounts_l1()
>    qcow2-refcount: check_refcounts_l1(): check reserved bits
>    qcow2-refcount: check_refblocks(): add separate message for reserved
> 
>   block/qcow2.h          |   7 +-
>   block/qcow2-cluster.c  |  20 ++-
>   block/qcow2-refcount.c | 328 ++++++++++++++++++++++++++---------------
>   block/qcow2.c          |  13 +-
>   4 files changed, 240 insertions(+), 128 deletions(-)
> 


-- 
Best regards,
Vladimir

Re: [PATCH v3 00/10] qcow2 check: check some reserved bits and subcluster bitmaps

[Vladimir Sementsov-Ogievskiy]

Ping again:)

Nothing changed: patches applies to master, 08 doesn't have r-b.

03.07.2021 14:17, Vladimir Sementsov-Ogievskiy wrote:
> Ping :)
> 
> This still applies to master with no conflicts. All patches reviewed except for 08.
> 
> 
> 24.05.2021 17:20, Vladimir Sementsov-Ogievskiy wrote:
>> Hi all!
>>
>> Here are some good refactorings and new (qemu-img check) checks for
>> qcow2.
>>
>> v3: add r-b mark by Alberto and t-b marks by Kirill
>>   07, 09: add missed "\n"
>>
>> Vladimir Sementsov-Ogievskiy (10):
>>    qcow2-refcount: improve style of check_refcounts_l2()
>>    qcow2: compressed read: simplify cluster descriptor passing
>>    qcow2: introduce qcow2_parse_compressed_l2_entry() helper
>>    qcow2-refcount: introduce fix_l2_entry_by_zero()
>>    qcow2-refcount: fix_l2_entry_by_zero(): also zero L2 entry bitmap
>>    qcow2-refcount: check_refcounts_l2(): check l2_bitmap
>>    qcow2-refcount: check_refcounts_l2(): check reserved bits
>>    qcow2-refcount: improve style of check_refcounts_l1()
>>    qcow2-refcount: check_refcounts_l1(): check reserved bits
>>    qcow2-refcount: check_refblocks(): add separate message for reserved
>>
>>   block/qcow2.h          |   7 +-
>>   block/qcow2-cluster.c  |  20 ++-
>>   block/qcow2-refcount.c | 328 ++++++++++++++++++++++++++---------------
>>   block/qcow2.c          |  13 +-
>>   4 files changed, 240 insertions(+), 128 deletions(-)
>>
> 
> 


-- 
Best regards,
Vladimir

Re: [PATCH v3 01/10] qcow2-refcount: improve style of check_refcounts_l2()

[Hanna Reitz]

On 24.05.21 16:20, Vladimir Sementsov-Ogievskiy wrote:
>   - don't use same name for size in bytes and in entries
>   - use g_autofree for l2_table
>   - add whitespace
>   - fix block comment style
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>   block/qcow2-refcount.c | 47 +++++++++++++++++++++---------------------
>   1 file changed, 24 insertions(+), 23 deletions(-)

Reviewed-by: Hanna Reitz <hreitz@redhat.com>

Re: [PATCH v3 02/10] qcow2: compressed read: simplify cluster descriptor passing

[Hanna Reitz]

On 24.05.21 16:20, Vladimir Sementsov-Ogievskiy wrote:
> Let's pass the whole L2 entry and not bother with
> L2E_COMPRESSED_OFFSET_SIZE_MASK.
>
> It also helps further refactoring that adds generic
> qcow2_parse_compressed_l2_entry() helper.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Alberto Garcia <berto@igalia.com>
> ---
>   block/qcow2.h         |  1 -
>   block/qcow2-cluster.c |  5 ++---
>   block/qcow2.c         | 12 +++++++-----
>   3 files changed, 9 insertions(+), 9 deletions(-)

Reviewed-by: Hanna Reitz <hreitz@redhat.com>

Re: [PATCH v3 03/10] qcow2: introduce qcow2_parse_compressed_l2_entry() helper

[Hanna Reitz]

On 24.05.21 16:20, Vladimir Sementsov-Ogievskiy wrote:
> Add helper to parse compressed l2_entry and use it everywhere instead
> of open-coding.
>
> Note, that in most places we move to precise coffset/csize instead of
> sector-aligned. Still it should work good enough for updating
> refcounts.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>   block/qcow2.h          |  3 ++-
>   block/qcow2-cluster.c  | 15 +++++++++++++++
>   block/qcow2-refcount.c | 36 +++++++++++++++++-------------------
>   block/qcow2.c          |  9 ++-------
>   4 files changed, 36 insertions(+), 27 deletions(-)

Reviewed-by: Hanna Reitz <hreitz@redhat.com>

Re: [PATCH v3 04/10] qcow2-refcount: introduce fix_l2_entry_by_zero()

[Hanna Reitz]

On 24.05.21 16:20, Vladimir Sementsov-Ogievskiy wrote:
> Split fix_l2_entry_by_zero() out of check_refcounts_l2() to be
> reused in further patch.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>   block/qcow2-refcount.c | 87 +++++++++++++++++++++++++++++-------------
>   1 file changed, 60 insertions(+), 27 deletions(-)

Reviewed-by: Hanna Reitz <hreitz@redhat.com>

Re: [PATCH v3 05/10] qcow2-refcount: fix_l2_entry_by_zero(): also zero L2 entry bitmap

[Hanna Reitz]

On 24.05.21 16:20, Vladimir Sementsov-Ogievskiy wrote:
> We'll reuse the function to fix wrong L2 entry bitmap. Support it now.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>   block/qcow2-refcount.c | 18 +++++++++++++++---
>   1 file changed, 15 insertions(+), 3 deletions(-)

Reviewed-by: Hanna Reitz <hreitz@redhat.com>

Re: [PATCH v3 06/10] qcow2-refcount: check_refcounts_l2(): check l2_bitmap

[Hanna Reitz]

On 24.05.21 16:20, Vladimir Sementsov-Ogievskiy wrote:
> Check subcluster bitmap of the l2 entry for different types of
> clusters:
>
>   - for compressed it must be zero
>   - for allocated check consistency of two parts of the bitmap
>   - for unallocated all subclusters should be unallocated
>     (or zero-plain)
>
> For unallocated clusters we can safely fix the entry by making it
> zero-plain.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Tested-by: Kirill Tkhai <ktkhai@virtuozzo.com>
> ---
>   block/qcow2-refcount.c | 30 +++++++++++++++++++++++++++++-
>   1 file changed, 29 insertions(+), 1 deletion(-)
>
> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
> index f48c5e1b5d..062ec48a15 100644
> --- a/block/qcow2-refcount.c
> +++ b/block/qcow2-refcount.c
> @@ -1681,6 +1681,7 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
>           uint64_t coffset;
>           int csize;
>           l2_entry = get_l2_entry(s, l2_table, i);
> +        uint64_t l2_bitmap = get_l2_bitmap(s, l2_table, i);

This is a declaration after a statement.  (Easily fixable by moving the 
l2_entry declaration here, though.  Or by putting the l2_bitmap 
declaration where l2_entry is declared.)

[...]

> @@ -1800,6 +1815,19 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
>   
>           case QCOW2_CLUSTER_ZERO_PLAIN:
>           case QCOW2_CLUSTER_UNALLOCATED:
> +            if (l2_bitmap & QCOW_L2_BITMAP_ALL_ALLOC) {
> +                res->corruptions++;
> +                fprintf(stderr, "%s: Unallocated "
> +                        "cluster has non-zero subcluster allocation map\n",
> +                        fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR");
> +                if (fix & BDRV_FIX_ERRORS) {
> +                    ret = fix_l2_entry_by_zero(bs, res, l2_offset, l2_table, i,
> +                                               active, &metadata_overlap);

I believe this is indeed the correct repair method for 
QCOW2_CLUSTER_ZERO_PLAIN, but I’m not so sure for 
QCOW2_CLUSTER_UNALLOCATED.  As far as I can tell, 
qcow2_get_subcluster_type() will return QCOW2_SUBCLUSTER_INVALID for 
this case, and so trying to read from this clusters will produce I/O 
errors.  But still, shouldn’t we rather make such a cluster unallocated 
rather than zero then?

And as for QCOW2_CLUSTER_ZERO_PLAIN, I believe qcow2_get_cluster_type() 
will never return it when subclusters are enabled.  So this repair path 
will never happen with a cluster type of ZERO_PLAIN, but only for 
UNALLOCATED.

Hanna

> +                    if (metadata_overlap) {
> +                        return ret;
> +                    }
> +                }
> +            }
>               break;
>   
>           default:

Re: [PATCH v3 07/10] qcow2-refcount: check_refcounts_l2(): check reserved bits

[Hanna Reitz]

On 24.05.21 16:20, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Tested-by: Kirill Tkhai <ktkhai@virtuozzo.com>
> ---
>   block/qcow2.h          |  1 +
>   block/qcow2-refcount.c | 12 +++++++++++-
>   2 files changed, 12 insertions(+), 1 deletion(-)

Reviewed-by: Hanna Reitz <hreitz@redhat.com>

Re: [PATCH v3 08/10] qcow2-refcount: improve style of check_refcounts_l1()

[Hanna Reitz]

On 24.05.21 16:20, Vladimir Sementsov-Ogievskiy wrote:
>   - use g_autofree for l1_table
>   - better name for size in bytes variable
>   - reduce code blocks nesting
>   - whitespaces, braces, newlines
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   block/qcow2-refcount.c | 98 +++++++++++++++++++++---------------------
>   1 file changed, 50 insertions(+), 48 deletions(-)

Reviewed-by: Hanna Reitz <hreitz@redhat.com>

Re: [PATCH v3 09/10] qcow2-refcount: check_refcounts_l1(): check reserved bits

[Hanna Reitz]

On 24.05.21 16:20, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Tested-by: Kirill Tkhai <ktkhai@virtuozzo.com>
> ---
>   block/qcow2.h          | 1 +
>   block/qcow2-refcount.c | 6 ++++++
>   2 files changed, 7 insertions(+)

Reviewed-by: Hanna Reitz <hreitz@redhat.com>

Re: [PATCH v3 10/10] qcow2-refcount: check_refblocks(): add separate message for reserved

[Hanna Reitz]

On 24.05.21 16:20, Vladimir Sementsov-Ogievskiy wrote:
> Split checking for reserved bits out of aligned offset check.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Tested-by: Kirill Tkhai <ktkhai@virtuozzo.com>
> ---
>   block/qcow2.h          |  1 +
>   block/qcow2-refcount.c | 10 +++++++++-
>   2 files changed, 10 insertions(+), 1 deletion(-)

Reviewed-by: Hanna Reitz <hreitz@redhat.com>

Re: [PATCH v3 06/10] qcow2-refcount: check_refcounts_l2(): check l2_bitmap

[Vladimir Sementsov-Ogievskiy]

14.09.2021 11:54, Hanna Reitz wrote:
> On 24.05.21 16:20, Vladimir Sementsov-Ogievskiy wrote:
>> Check subcluster bitmap of the l2 entry for different types of
>> clusters:
>>
>>   - for compressed it must be zero
>>   - for allocated check consistency of two parts of the bitmap
>>   - for unallocated all subclusters should be unallocated
>>     (or zero-plain)
>>
>> For unallocated clusters we can safely fix the entry by making it
>> zero-plain.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> Tested-by: Kirill Tkhai <ktkhai@virtuozzo.com>
>> ---
>>   block/qcow2-refcount.c | 30 +++++++++++++++++++++++++++++-
>>   1 file changed, 29 insertions(+), 1 deletion(-)
>>
>> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
>> index f48c5e1b5d..062ec48a15 100644
>> --- a/block/qcow2-refcount.c
>> +++ b/block/qcow2-refcount.c
>> @@ -1681,6 +1681,7 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
>>           uint64_t coffset;
>>           int csize;
>>           l2_entry = get_l2_entry(s, l2_table, i);
>> +        uint64_t l2_bitmap = get_l2_bitmap(s, l2_table, i);
> 
> This is a declaration after a statement.  (Easily fixable by moving the l2_entry declaration here, though.  Or by putting the l2_bitmap declaration where l2_entry is declared.)

The latter seems nicer.

> 
> [...]
> 
>> @@ -1800,6 +1815,19 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
>>           case QCOW2_CLUSTER_ZERO_PLAIN:
>>           case QCOW2_CLUSTER_UNALLOCATED:
>> +            if (l2_bitmap & QCOW_L2_BITMAP_ALL_ALLOC) {
>> +                res->corruptions++;
>> +                fprintf(stderr, "%s: Unallocated "
>> +                        "cluster has non-zero subcluster allocation map\n",
>> +                        fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR");
>> +                if (fix & BDRV_FIX_ERRORS) {
>> +                    ret = fix_l2_entry_by_zero(bs, res, l2_offset, l2_table, i,
>> +                                               active, &metadata_overlap);
> 
> I believe this is indeed the correct repair method for QCOW2_CLUSTER_ZERO_PLAIN, but I’m not so sure for QCOW2_CLUSTER_UNALLOCATED.  As far as I can tell, qcow2_get_subcluster_type() will return QCOW2_SUBCLUSTER_INVALID for this case, and so trying to read from this clusters will produce I/O errors.  But still, shouldn’t we rather make such a cluster unallocated rather than zero then?
> 
> And as for QCOW2_CLUSTER_ZERO_PLAIN, I believe qcow2_get_cluster_type() will never return it when subclusters are enabled.  So this repair path will never happen with a cluster type of ZERO_PLAIN, but only for UNALLOCATED.
> 


Agree about ZERO_PLAIN, that it's impossible here.

But for UNALLOCATED, I'm not sure. If we make all wrongly "allocated" subclusters to be unallocted, underlying backing layer will become available. Could it be considered as security violation?

On the other hand, when user have to fix format corruptions, nothing is guaranteed and the aim is to make data available as far as it's possible. So, may be making wrong subclusters "unallocated" is correct thing..

> 
>> +                    if (metadata_overlap) {
>> +                        return ret;
>> +                    }
>> +                }
>> +            }
>>               break;
>>           default:
> 


-- 
Best regards,
Vladimir

Re: [PATCH v3 06/10] qcow2-refcount: check_refcounts_l2(): check l2_bitmap

[Hanna Reitz]

On 14.09.21 13:22, Vladimir Sementsov-Ogievskiy wrote:
> 14.09.2021 11:54, Hanna Reitz wrote:
>> On 24.05.21 16:20, Vladimir Sementsov-Ogievskiy wrote:
>>> Check subcluster bitmap of the l2 entry for different types of
>>> clusters:
>>>
>>>   - for compressed it must be zero
>>>   - for allocated check consistency of two parts of the bitmap
>>>   - for unallocated all subclusters should be unallocated
>>>     (or zero-plain)
>>>
>>> For unallocated clusters we can safely fix the entry by making it
>>> zero-plain.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>> Tested-by: Kirill Tkhai <ktkhai@virtuozzo.com>
>>> ---
>>>   block/qcow2-refcount.c | 30 +++++++++++++++++++++++++++++-
>>>   1 file changed, 29 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
>>> index f48c5e1b5d..062ec48a15 100644
>>> --- a/block/qcow2-refcount.c
>>> +++ b/block/qcow2-refcount.c
>>> @@ -1681,6 +1681,7 @@ static int check_refcounts_l2(BlockDriverState 
>>> *bs, BdrvCheckResult *res,
>>>           uint64_t coffset;
>>>           int csize;
>>>           l2_entry = get_l2_entry(s, l2_table, i);
>>> +        uint64_t l2_bitmap = get_l2_bitmap(s, l2_table, i);
>>
>> This is a declaration after a statement.  (Easily fixable by moving 
>> the l2_entry declaration here, though.  Or by putting the l2_bitmap 
>> declaration where l2_entry is declared.)
>
> The latter seems nicer.
>
>>
>> [...]
>>
>>> @@ -1800,6 +1815,19 @@ static int 
>>> check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
>>>           case QCOW2_CLUSTER_ZERO_PLAIN:
>>>           case QCOW2_CLUSTER_UNALLOCATED:
>>> +            if (l2_bitmap & QCOW_L2_BITMAP_ALL_ALLOC) {
>>> +                res->corruptions++;
>>> +                fprintf(stderr, "%s: Unallocated "
>>> +                        "cluster has non-zero subcluster allocation 
>>> map\n",
>>> +                        fix & BDRV_FIX_ERRORS ? "Repairing" : 
>>> "ERROR");
>>> +                if (fix & BDRV_FIX_ERRORS) {
>>> +                    ret = fix_l2_entry_by_zero(bs, res, l2_offset, 
>>> l2_table, i,
>>> +                                               active, 
>>> &metadata_overlap);
>>
>> I believe this is indeed the correct repair method for 
>> QCOW2_CLUSTER_ZERO_PLAIN, but I’m not so sure for 
>> QCOW2_CLUSTER_UNALLOCATED.  As far as I can tell, 
>> qcow2_get_subcluster_type() will return QCOW2_SUBCLUSTER_INVALID for 
>> this case, and so trying to read from this clusters will produce I/O 
>> errors.  But still, shouldn’t we rather make such a cluster 
>> unallocated rather than zero then?
>>
>> And as for QCOW2_CLUSTER_ZERO_PLAIN, I believe 
>> qcow2_get_cluster_type() will never return it when subclusters are 
>> enabled.  So this repair path will never happen with a cluster type 
>> of ZERO_PLAIN, but only for UNALLOCATED.
>>
>
>
> Agree about ZERO_PLAIN, that it's impossible here.
>
> But for UNALLOCATED, I'm not sure. If we make all wrongly "allocated" 
> subclusters to be unallocted, underlying backing layer will become 
> available. Could it be considered as security violation?

I don’t think so, because the image has to be corrupted first, which I 
hope guests cannot trigger.

> On the other hand, when user have to fix format corruptions, nothing 
> is guaranteed and the aim is to make data available as far as it's 
> possible. So, may be making wrong subclusters "unallocated" is correct 
> thing..

We could also consider refusing to repair this case for images that have 
backing files.

In any case, I don’t think we should force ourselves to make some 
cluster zero just because there’s no better choice.  For example, we 
also don’t make unallocated data clusters zero, because it would just be 
wrong.

(Though technically there is no right or wrong here, because we just 
refuse to read from such clusters.  Doing anything to the cluster would 
kind of be an improvement, whether it is making it zero or making it 
really unallocated...  If there was any important data here, it’s lost 
anyway.)

Perhaps we should have a truly destructive repair mode where all 
unreadable data is made 0.  But OTOH, if users have an image that’s so 
broken, then it’s probably not wrong to tell them it’s unrepairable and 
they need to convert it to a fresh image (with --salvage).

Hanna

Re: [PATCH v3 06/10] qcow2-refcount: check_refcounts_l2(): check l2_bitmap

[Vladimir Sementsov-Ogievskiy]

14.09.2021 14:46, Hanna Reitz wrote:
> On 14.09.21 13:22, Vladimir Sementsov-Ogievskiy wrote:
>> 14.09.2021 11:54, Hanna Reitz wrote:
>>> On 24.05.21 16:20, Vladimir Sementsov-Ogievskiy wrote:
>>>> Check subcluster bitmap of the l2 entry for different types of
>>>> clusters:
>>>>
>>>>   - for compressed it must be zero
>>>>   - for allocated check consistency of two parts of the bitmap
>>>>   - for unallocated all subclusters should be unallocated
>>>>     (or zero-plain)
>>>>
>>>> For unallocated clusters we can safely fix the entry by making it
>>>> zero-plain.
>>>>
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>>> Tested-by: Kirill Tkhai <ktkhai@virtuozzo.com>
>>>> ---
>>>>   block/qcow2-refcount.c | 30 +++++++++++++++++++++++++++++-
>>>>   1 file changed, 29 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
>>>> index f48c5e1b5d..062ec48a15 100644
>>>> --- a/block/qcow2-refcount.c
>>>> +++ b/block/qcow2-refcount.c
>>>> @@ -1681,6 +1681,7 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
>>>>           uint64_t coffset;
>>>>           int csize;
>>>>           l2_entry = get_l2_entry(s, l2_table, i);
>>>> +        uint64_t l2_bitmap = get_l2_bitmap(s, l2_table, i);
>>>
>>> This is a declaration after a statement.  (Easily fixable by moving the l2_entry declaration here, though.  Or by putting the l2_bitmap declaration where l2_entry is declared.)
>>
>> The latter seems nicer.
>>
>>>
>>> [...]
>>>
>>>> @@ -1800,6 +1815,19 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
>>>>           case QCOW2_CLUSTER_ZERO_PLAIN:
>>>>           case QCOW2_CLUSTER_UNALLOCATED:
>>>> +            if (l2_bitmap & QCOW_L2_BITMAP_ALL_ALLOC) {
>>>> +                res->corruptions++;
>>>> +                fprintf(stderr, "%s: Unallocated "
>>>> +                        "cluster has non-zero subcluster allocation map\n",
>>>> +                        fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR");
>>>> +                if (fix & BDRV_FIX_ERRORS) {
>>>> +                    ret = fix_l2_entry_by_zero(bs, res, l2_offset, l2_table, i,
>>>> +                                               active, &metadata_overlap);
>>>
>>> I believe this is indeed the correct repair method for QCOW2_CLUSTER_ZERO_PLAIN, but I’m not so sure for QCOW2_CLUSTER_UNALLOCATED.  As far as I can tell, qcow2_get_subcluster_type() will return QCOW2_SUBCLUSTER_INVALID for this case, and so trying to read from this clusters will produce I/O errors.  But still, shouldn’t we rather make such a cluster unallocated rather than zero then?
>>>
>>> And as for QCOW2_CLUSTER_ZERO_PLAIN, I believe qcow2_get_cluster_type() will never return it when subclusters are enabled.  So this repair path will never happen with a cluster type of ZERO_PLAIN, but only for UNALLOCATED.
>>>
>>
>>
>> Agree about ZERO_PLAIN, that it's impossible here.
>>
>> But for UNALLOCATED, I'm not sure. If we make all wrongly "allocated" subclusters to be unallocted, underlying backing layer will become available. Could it be considered as security violation?
> 
> I don’t think so, because the image has to be corrupted first, which I hope guests cannot trigger.
> 
>> On the other hand, when user have to fix format corruptions, nothing is guaranteed and the aim is to make data available as far as it's possible. So, may be making wrong subclusters "unallocated" is correct thing..
> 
> We could also consider refusing to repair this case for images that have backing files.
> 
> In any case, I don’t think we should force ourselves to make some cluster zero just because there’s no better choice.  For example, we also don’t make unallocated data clusters zero, because it would just be wrong.
> 
> (Though technically there is no right or wrong here, because we just refuse to read from such clusters.  Doing anything to the cluster would kind of be an improvement, whether it is making it zero or making it really unallocated...  If there was any important data here, it’s lost anyway.)
> 
> Perhaps we should have a truly destructive repair mode where all unreadable data is made 0.  But OTOH, if users have an image that’s so broken, then it’s probably not wrong to tell them it’s unrepairable and they need to convert it to a fresh image (with --salvage).
> 
> Hanna
> 

Agree. For simplicity, let's just drop thin last hunk for now. I'll resend now.


-- 
Best regards,
Vladimir

Re: [PATCH v3 06/10] qcow2-refcount: check_refcounts_l2(): check l2_bitmap

[Vladimir Sementsov-Ogievskiy]

14.09.2021 15:00, Vladimir Sementsov-Ogievskiy wrote:
> 14.09.2021 14:46, Hanna Reitz wrote:
>> On 14.09.21 13:22, Vladimir Sementsov-Ogievskiy wrote:
>>> 14.09.2021 11:54, Hanna Reitz wrote:
>>>> On 24.05.21 16:20, Vladimir Sementsov-Ogievskiy wrote:
>>>>> Check subcluster bitmap of the l2 entry for different types of
>>>>> clusters:
>>>>>
>>>>>   - for compressed it must be zero
>>>>>   - for allocated check consistency of two parts of the bitmap
>>>>>   - for unallocated all subclusters should be unallocated
>>>>>     (or zero-plain)
>>>>>
>>>>> For unallocated clusters we can safely fix the entry by making it
>>>>> zero-plain.
>>>>>
>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>>>> Tested-by: Kirill Tkhai <ktkhai@virtuozzo.com>
>>>>> ---
>>>>>   block/qcow2-refcount.c | 30 +++++++++++++++++++++++++++++-
>>>>>   1 file changed, 29 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
>>>>> index f48c5e1b5d..062ec48a15 100644
>>>>> --- a/block/qcow2-refcount.c
>>>>> +++ b/block/qcow2-refcount.c
>>>>> @@ -1681,6 +1681,7 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
>>>>>           uint64_t coffset;
>>>>>           int csize;
>>>>>           l2_entry = get_l2_entry(s, l2_table, i);
>>>>> +        uint64_t l2_bitmap = get_l2_bitmap(s, l2_table, i);
>>>>
>>>> This is a declaration after a statement.  (Easily fixable by moving the l2_entry declaration here, though.  Or by putting the l2_bitmap declaration where l2_entry is declared.)
>>>
>>> The latter seems nicer.
>>>
>>>>
>>>> [...]
>>>>
>>>>> @@ -1800,6 +1815,19 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
>>>>>           case QCOW2_CLUSTER_ZERO_PLAIN:
>>>>>           case QCOW2_CLUSTER_UNALLOCATED:
>>>>> +            if (l2_bitmap & QCOW_L2_BITMAP_ALL_ALLOC) {
>>>>> +                res->corruptions++;
>>>>> +                fprintf(stderr, "%s: Unallocated "
>>>>> +                        "cluster has non-zero subcluster allocation map\n",
>>>>> +                        fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR");
>>>>> +                if (fix & BDRV_FIX_ERRORS) {
>>>>> +                    ret = fix_l2_entry_by_zero(bs, res, l2_offset, l2_table, i,
>>>>> +                                               active, &metadata_overlap);
>>>>
>>>> I believe this is indeed the correct repair method for QCOW2_CLUSTER_ZERO_PLAIN, but I’m not so sure for QCOW2_CLUSTER_UNALLOCATED.  As far as I can tell, qcow2_get_subcluster_type() will return QCOW2_SUBCLUSTER_INVALID for this case, and so trying to read from this clusters will produce I/O errors.  But still, shouldn’t we rather make such a cluster unallocated rather than zero then?
>>>>
>>>> And as for QCOW2_CLUSTER_ZERO_PLAIN, I believe qcow2_get_cluster_type() will never return it when subclusters are enabled.  So this repair path will never happen with a cluster type of ZERO_PLAIN, but only for UNALLOCATED.
>>>>
>>>
>>>
>>> Agree about ZERO_PLAIN, that it's impossible here.
>>>
>>> But for UNALLOCATED, I'm not sure. If we make all wrongly "allocated" subclusters to be unallocted, underlying backing layer will become available. Could it be considered as security violation?
>>
>> I don’t think so, because the image has to be corrupted first, which I hope guests cannot trigger.
>>
>>> On the other hand, when user have to fix format corruptions, nothing is guaranteed and the aim is to make data available as far as it's possible. So, may be making wrong subclusters "unallocated" is correct thing..
>>
>> We could also consider refusing to repair this case for images that have backing files.
>>
>> In any case, I don’t think we should force ourselves to make some cluster zero just because there’s no better choice.  For example, we also don’t make unallocated data clusters zero, because it would just be wrong.
>>
>> (Though technically there is no right or wrong here, because we just refuse to read from such clusters.  Doing anything to the cluster would kind of be an improvement, whether it is making it zero or making it really unallocated...  If there was any important data here, it’s lost anyway.)
>>
>> Perhaps we should have a truly destructive repair mode where all unreadable data is made 0.  But OTOH, if users have an image that’s so broken, then it’s probably not wrong to tell them it’s unrepairable and they need to convert it to a fresh image (with --salvage).
>>
>> Hanna
>>
> 
> Agree. For simplicity, let's just drop thin last hunk for now. I'll resend now.
> 
> 

Not the whole hunk, only fixing part of course. To look like this:

@@ -1799,7 +1819,16 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
          }
  
          case QCOW2_CLUSTER_ZERO_PLAIN:
+            /* Impossible when image has subclusters */
+            assert(!l2_bitmap);
+            break;
+
          case QCOW2_CLUSTER_UNALLOCATED:
+            if (l2_bitmap & QCOW_L2_BITMAP_ALL_ALLOC) {
+                res->corruptions++;
+                fprintf(stderr, "ERROR: Unallocated "
+                        "cluster has non-zero subcluster allocation map\n");
+            }
              break;
  
          default:


-- 
Best regards,
Vladimir