* [PATCH v4 00/10] qcow2 check: check some reserved bits and subcluster bitmaps
@ 2021-09-14 12:24 Vladimir Sementsov-Ogievskiy
2021-09-14 12:24 ` [PATCH v4 01/10] qcow2-refcount: improve style of check_refcounts_l2() Vladimir Sementsov-Ogievskiy
` (10 more replies)
0 siblings, 11 replies; 18+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-09-14 12:24 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, hreitz, kwolf, den, ktkhai, eblake, berto, vsementsov
Hi all!
Here are some good refactorings and new (qemu-img check) checks for
qcow2.
06 qcow2-refcount: check_refcounts_l2(): check l2_bitmap
don't fix unallocated cluster with allocated subclusters (no strong opinion how to do it correctly)
drop Eric's r-b
keep Kirill's t-b (I believe it still applies, my original task didn't include error fixing)
define l2_bitmap near l2_entry
add separate assertion and "break;" for QCOW2_CLUSTER_ZERO_PLAIN
others: add r-b: Hanna
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 | 324 ++++++++++++++++++++++++++---------------
block/qcow2.c | 13 +-
4 files changed, 236 insertions(+), 128 deletions(-)
--
2.29.2
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v4 01/10] qcow2-refcount: improve style of check_refcounts_l2()
2021-09-14 12:24 [PATCH v4 00/10] qcow2 check: check some reserved bits and subcluster bitmaps Vladimir Sementsov-Ogievskiy
@ 2021-09-14 12:24 ` Vladimir Sementsov-Ogievskiy
2021-09-14 12:24 ` [PATCH v4 02/10] qcow2: compressed read: simplify cluster descriptor passing Vladimir Sementsov-Ogievskiy
` (9 subsequent siblings)
10 siblings, 0 replies; 18+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-09-14 12:24 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, hreitz, kwolf, den, ktkhai, eblake, berto, vsementsov
- 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>
Reviewed-by: Hanna Reitz <hreitz@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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v4 02/10] qcow2: compressed read: simplify cluster descriptor passing
2021-09-14 12:24 [PATCH v4 00/10] qcow2 check: check some reserved bits and subcluster bitmaps Vladimir Sementsov-Ogievskiy
2021-09-14 12:24 ` [PATCH v4 01/10] qcow2-refcount: improve style of check_refcounts_l2() Vladimir Sementsov-Ogievskiy
@ 2021-09-14 12:24 ` Vladimir Sementsov-Ogievskiy
2021-09-14 12:24 ` [PATCH v4 03/10] qcow2: introduce qcow2_parse_compressed_l2_entry() helper Vladimir Sementsov-Ogievskiy
` (8 subsequent siblings)
10 siblings, 0 replies; 18+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-09-14 12:24 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, hreitz, kwolf, den, ktkhai, eblake, berto, vsementsov
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>
Reviewed-by: Hanna Reitz <hreitz@redhat.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 9f1b6461c8..e5d8ab679e 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,
@@ -2205,7 +2205,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;
@@ -4693,7 +4693,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,
@@ -4705,8 +4705,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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v4 03/10] qcow2: introduce qcow2_parse_compressed_l2_entry() helper
2021-09-14 12:24 [PATCH v4 00/10] qcow2 check: check some reserved bits and subcluster bitmaps Vladimir Sementsov-Ogievskiy
2021-09-14 12:24 ` [PATCH v4 01/10] qcow2-refcount: improve style of check_refcounts_l2() Vladimir Sementsov-Ogievskiy
2021-09-14 12:24 ` [PATCH v4 02/10] qcow2: compressed read: simplify cluster descriptor passing Vladimir Sementsov-Ogievskiy
@ 2021-09-14 12:24 ` Vladimir Sementsov-Ogievskiy
2021-09-14 12:24 ` [PATCH v4 04/10] qcow2-refcount: introduce fix_l2_entry_by_zero() Vladimir Sementsov-Ogievskiy
` (7 subsequent siblings)
10 siblings, 0 replies; 18+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-09-14 12:24 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, hreitz, kwolf, den, ktkhai, eblake, berto, vsementsov
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>
Reviewed-by: Hanna Reitz <hreitz@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 e5d8ab679e..02f9f3e636 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -4700,17 +4700,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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v4 04/10] qcow2-refcount: introduce fix_l2_entry_by_zero()
2021-09-14 12:24 [PATCH v4 00/10] qcow2 check: check some reserved bits and subcluster bitmaps Vladimir Sementsov-Ogievskiy
` (2 preceding siblings ...)
2021-09-14 12:24 ` [PATCH v4 03/10] qcow2: introduce qcow2_parse_compressed_l2_entry() helper Vladimir Sementsov-Ogievskiy
@ 2021-09-14 12:24 ` Vladimir Sementsov-Ogievskiy
2021-09-14 12:24 ` [PATCH v4 05/10] qcow2-refcount: fix_l2_entry_by_zero(): also zero L2 entry bitmap Vladimir Sementsov-Ogievskiy
` (6 subsequent siblings)
10 siblings, 0 replies; 18+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-09-14 12:24 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, hreitz, kwolf, den, ktkhai, eblake, berto, vsementsov
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>
Reviewed-by: Hanna Reitz <hreitz@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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v4 05/10] qcow2-refcount: fix_l2_entry_by_zero(): also zero L2 entry bitmap
2021-09-14 12:24 [PATCH v4 00/10] qcow2 check: check some reserved bits and subcluster bitmaps Vladimir Sementsov-Ogievskiy
` (3 preceding siblings ...)
2021-09-14 12:24 ` [PATCH v4 04/10] qcow2-refcount: introduce fix_l2_entry_by_zero() Vladimir Sementsov-Ogievskiy
@ 2021-09-14 12:24 ` Vladimir Sementsov-Ogievskiy
2021-09-14 12:24 ` [PATCH v4 06/10] qcow2-refcount: check_refcounts_l2(): check l2_bitmap Vladimir Sementsov-Ogievskiy
` (5 subsequent siblings)
10 siblings, 0 replies; 18+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-09-14 12:24 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, hreitz, kwolf, den, ktkhai, eblake, berto, vsementsov
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>
Reviewed-by: Hanna Reitz <hreitz@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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v4 06/10] qcow2-refcount: check_refcounts_l2(): check l2_bitmap
2021-09-14 12:24 [PATCH v4 00/10] qcow2 check: check some reserved bits and subcluster bitmaps Vladimir Sementsov-Ogievskiy
` (4 preceding siblings ...)
2021-09-14 12:24 ` [PATCH v4 05/10] qcow2-refcount: fix_l2_entry_by_zero(): also zero L2 entry bitmap Vladimir Sementsov-Ogievskiy
@ 2021-09-14 12:24 ` Vladimir Sementsov-Ogievskiy
2021-09-14 14:25 ` Eric Blake
2021-09-14 17:12 ` Hanna Reitz
2021-09-14 12:24 ` [PATCH v4 07/10] qcow2-refcount: check_refcounts_l2(): check reserved bits Vladimir Sementsov-Ogievskiy
` (4 subsequent siblings)
10 siblings, 2 replies; 18+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-09-14 12:24 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, hreitz, kwolf, den, ktkhai, eblake, berto, vsementsov
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)
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Tested-by: Kirill Tkhai <ktkhai@virtuozzo.com>
---
block/qcow2-refcount.c | 28 ++++++++++++++++++++++++++--
1 file changed, 26 insertions(+), 2 deletions(-)
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index f48c5e1b5d..9a5ae3cac4 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -1661,7 +1661,7 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
int flags, BdrvCheckMode fix, bool active)
{
BDRVQcow2State *s = bs->opaque;
- uint64_t l2_entry;
+ uint64_t l2_entry, l2_bitmap;
uint64_t next_contiguous_offset = 0;
int i, ret;
size_t l2_size_bytes = s->l2_size * l2_entry_size(s);
@@ -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);
+ 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);
@@ -1799,7 +1814,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:
--
2.29.2
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v4 07/10] qcow2-refcount: check_refcounts_l2(): check reserved bits
2021-09-14 12:24 [PATCH v4 00/10] qcow2 check: check some reserved bits and subcluster bitmaps Vladimir Sementsov-Ogievskiy
` (5 preceding siblings ...)
2021-09-14 12:24 ` [PATCH v4 06/10] qcow2-refcount: check_refcounts_l2(): check l2_bitmap Vladimir Sementsov-Ogievskiy
@ 2021-09-14 12:24 ` Vladimir Sementsov-Ogievskiy
2021-09-14 17:15 ` Hanna Reitz
2021-09-14 12:24 ` [PATCH v4 08/10] qcow2-refcount: improve style of check_refcounts_l1() Vladimir Sementsov-Ogievskiy
` (3 subsequent siblings)
10 siblings, 1 reply; 18+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-09-14 12:24 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, hreitz, kwolf, den, ktkhai, eblake, berto, vsementsov
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Tested-by: Kirill Tkhai <ktkhai@virtuozzo.com>
Reviewed-by: Hanna Reitz <hreitz@redhat.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 9a5ae3cac4..5d57e677bc 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);
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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v4 08/10] qcow2-refcount: improve style of check_refcounts_l1()
2021-09-14 12:24 [PATCH v4 00/10] qcow2 check: check some reserved bits and subcluster bitmaps Vladimir Sementsov-Ogievskiy
` (6 preceding siblings ...)
2021-09-14 12:24 ` [PATCH v4 07/10] qcow2-refcount: check_refcounts_l2(): check reserved bits Vladimir Sementsov-Ogievskiy
@ 2021-09-14 12:24 ` Vladimir Sementsov-Ogievskiy
2021-09-14 12:24 ` [PATCH v4 09/10] qcow2-refcount: check_refcounts_l1(): check reserved bits Vladimir Sementsov-Ogievskiy
` (2 subsequent siblings)
10 siblings, 0 replies; 18+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-09-14 12:24 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, hreitz, kwolf, den, ktkhai, eblake, berto, vsementsov
- 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>
Reviewed-by: Hanna Reitz <hreitz@redhat.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 5d57e677bc..153e5ca087 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -1860,71 +1860,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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v4 09/10] qcow2-refcount: check_refcounts_l1(): check reserved bits
2021-09-14 12:24 [PATCH v4 00/10] qcow2 check: check some reserved bits and subcluster bitmaps Vladimir Sementsov-Ogievskiy
` (7 preceding siblings ...)
2021-09-14 12:24 ` [PATCH v4 08/10] qcow2-refcount: improve style of check_refcounts_l1() Vladimir Sementsov-Ogievskiy
@ 2021-09-14 12:24 ` Vladimir Sementsov-Ogievskiy
2021-09-14 12:24 ` [PATCH v4 10/10] qcow2-refcount: check_refblocks(): add separate message for reserved Vladimir Sementsov-Ogievskiy
2021-09-15 10:26 ` [PATCH v4 00/10] qcow2 check: check some reserved bits and subcluster bitmaps Hanna Reitz
10 siblings, 0 replies; 18+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-09-14 12:24 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, hreitz, kwolf, den, ktkhai, eblake, berto, vsementsov
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Tested-by: Kirill Tkhai <ktkhai@virtuozzo.com>
Reviewed-by: Hanna Reitz <hreitz@redhat.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 153e5ca087..75751a0181 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -1900,6 +1900,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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v4 10/10] qcow2-refcount: check_refblocks(): add separate message for reserved
2021-09-14 12:24 [PATCH v4 00/10] qcow2 check: check some reserved bits and subcluster bitmaps Vladimir Sementsov-Ogievskiy
` (8 preceding siblings ...)
2021-09-14 12:24 ` [PATCH v4 09/10] qcow2-refcount: check_refcounts_l1(): check reserved bits Vladimir Sementsov-Ogievskiy
@ 2021-09-14 12:24 ` Vladimir Sementsov-Ogievskiy
2021-09-15 10:26 ` [PATCH v4 00/10] qcow2 check: check some reserved bits and subcluster bitmaps Hanna Reitz
10 siblings, 0 replies; 18+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-09-14 12:24 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, hreitz, kwolf, den, ktkhai, eblake, berto, vsementsov
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>
Reviewed-by: Hanna Reitz <hreitz@redhat.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 75751a0181..d70667916b 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -2087,9 +2087,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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v4 06/10] qcow2-refcount: check_refcounts_l2(): check l2_bitmap
2021-09-14 12:24 ` [PATCH v4 06/10] qcow2-refcount: check_refcounts_l2(): check l2_bitmap Vladimir Sementsov-Ogievskiy
@ 2021-09-14 14:25 ` Eric Blake
2021-09-14 14:26 ` Vladimir Sementsov-Ogievskiy
2021-09-14 17:12 ` Hanna Reitz
1 sibling, 1 reply; 18+ messages in thread
From: Eric Blake @ 2021-09-14 14:25 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: kwolf, berto, qemu-block, qemu-devel, hreitz, ktkhai, den
On Tue, Sep 14, 2021 at 03:24:50PM +0300, 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)
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Tested-by: Kirill Tkhai <ktkhai@virtuozzo.com>
> ---
> block/qcow2-refcount.c | 28 ++++++++++++++++++++++++++--
> 1 file changed, 26 insertions(+), 2 deletions(-)
>
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4 06/10] qcow2-refcount: check_refcounts_l2(): check l2_bitmap
2021-09-14 14:25 ` Eric Blake
@ 2021-09-14 14:26 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 18+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-09-14 14:26 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-block, qemu-devel, hreitz, kwolf, den, ktkhai, berto
14.09.2021 17:25, Eric Blake wrote:
> On Tue, Sep 14, 2021 at 03:24:50PM +0300, 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)
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> Tested-by: Kirill Tkhai <ktkhai@virtuozzo.com>
>> ---
>> block/qcow2-refcount.c | 28 ++++++++++++++++++++++++++--
>> 1 file changed, 26 insertions(+), 2 deletions(-)
>>
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
Thanks!
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4 06/10] qcow2-refcount: check_refcounts_l2(): check l2_bitmap
2021-09-14 12:24 ` [PATCH v4 06/10] qcow2-refcount: check_refcounts_l2(): check l2_bitmap Vladimir Sementsov-Ogievskiy
2021-09-14 14:25 ` Eric Blake
@ 2021-09-14 17:12 ` Hanna Reitz
1 sibling, 0 replies; 18+ messages in thread
From: Hanna Reitz @ 2021-09-14 17:12 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: kwolf, berto, qemu-devel, ktkhai, den, eblake
On 14.09.21 14:24, 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)
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Tested-by: Kirill Tkhai <ktkhai@virtuozzo.com>
> ---
> block/qcow2-refcount.c | 28 ++++++++++++++++++++++++++--
> 1 file changed, 26 insertions(+), 2 deletions(-)
Reviewed-by: Hanna Reitz <hreitz@redhat.com>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4 07/10] qcow2-refcount: check_refcounts_l2(): check reserved bits
2021-09-14 12:24 ` [PATCH v4 07/10] qcow2-refcount: check_refcounts_l2(): check reserved bits Vladimir Sementsov-Ogievskiy
@ 2021-09-14 17:15 ` Hanna Reitz
2021-09-15 6:59 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 18+ messages in thread
From: Hanna Reitz @ 2021-09-14 17:15 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: kwolf, berto, qemu-devel, ktkhai, den, eblake
On 14.09.21 14:24, 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>
> Reviewed-by: Hanna Reitz <hreitz@redhat.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 9a5ae3cac4..5d57e677bc 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);
> l2_bitmap = get_l2_bitmap(s, l2_table, i);
> + QCow2ClusterType type = qcow2_get_cluster_type(bs, l2_entry);
Hm, with l2_bitmap being declared next to l2_entry, this is now the
patch that adds a declaration after a statement here.
(The possible resolutions seem to be the same, either move the
declaration up to the function’s root block, or move l2_entry and
l2_bitmap’s declarations here...)
(I don’t think we need a v5 for this, it should be fine if you tell me
which way you prefer.)
Hanna
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4 07/10] qcow2-refcount: check_refcounts_l2(): check reserved bits
2021-09-14 17:15 ` Hanna Reitz
@ 2021-09-15 6:59 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 18+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-09-15 6:59 UTC (permalink / raw)
To: Hanna Reitz, qemu-block; +Cc: qemu-devel, kwolf, den, ktkhai, eblake, berto
14.09.2021 20:15, Hanna Reitz wrote:
> On 14.09.21 14:24, 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>
>> Reviewed-by: Hanna Reitz <hreitz@redhat.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 9a5ae3cac4..5d57e677bc 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);
>> l2_bitmap = get_l2_bitmap(s, l2_table, i);
>> + QCow2ClusterType type = qcow2_get_cluster_type(bs, l2_entry);
Oh :(
>
> Hm, with l2_bitmap being declared next to l2_entry, this is now the patch that adds a declaration after a statement here.
>
> (The possible resolutions seem to be the same, either move the declaration up to the function’s root block, or move l2_entry and l2_bitmap’s declarations here...)
>
> (I don’t think we need a v5 for this, it should be fine if you tell me which way you prefer.)
>
I'd keep type here:
QCow2ClusterType type;
l2_entry = ...
l2_bitmap = ...
type = qcow2_get_cluster_type(bs, l2_entry);
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4 00/10] qcow2 check: check some reserved bits and subcluster bitmaps
2021-09-14 12:24 [PATCH v4 00/10] qcow2 check: check some reserved bits and subcluster bitmaps Vladimir Sementsov-Ogievskiy
` (9 preceding siblings ...)
2021-09-14 12:24 ` [PATCH v4 10/10] qcow2-refcount: check_refblocks(): add separate message for reserved Vladimir Sementsov-Ogievskiy
@ 2021-09-15 10:26 ` Hanna Reitz
2021-09-15 11:36 ` Vladimir Sementsov-Ogievskiy
10 siblings, 1 reply; 18+ messages in thread
From: Hanna Reitz @ 2021-09-15 10:26 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: kwolf, berto, qemu-devel, ktkhai, den, eblake
On 14.09.21 14:24, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
>
> Here are some good refactorings and new (qemu-img check) checks for
> qcow2.
Thanks, applied to my block branch:
https://gitlab.com/hreitz/qemu/-/commits/block/
(Patch 6 is here:
https://gitlab.com/hreitz/qemu/-/commit/93c40e7dab205047245028e97f7470d89c3a7ef3
– just to confirm the resolution fits what you had in mind)
Hanna
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4 00/10] qcow2 check: check some reserved bits and subcluster bitmaps
2021-09-15 10:26 ` [PATCH v4 00/10] qcow2 check: check some reserved bits and subcluster bitmaps Hanna Reitz
@ 2021-09-15 11:36 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 18+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-09-15 11:36 UTC (permalink / raw)
To: Hanna Reitz, qemu-block; +Cc: qemu-devel, kwolf, den, ktkhai, eblake, berto
15.09.2021 13:26, Hanna Reitz wrote:
> On 14.09.21 14:24, Vladimir Sementsov-Ogievskiy wrote:
>> Hi all!
>>
>> Here are some good refactorings and new (qemu-img check) checks for
>> qcow2.
>
> Thanks, applied to my block branch:
>
> https://gitlab.com/hreitz/qemu/-/commits/block/
>
> (Patch 6 is here: https://gitlab.com/hreitz/qemu/-/commit/93c40e7dab205047245028e97f7470d89c3a7ef3 – just to confirm the resolution fits what you had in mind)
>
It's OK, thanks!
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2021-09-15 11:38 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-14 12:24 [PATCH v4 00/10] qcow2 check: check some reserved bits and subcluster bitmaps Vladimir Sementsov-Ogievskiy
2021-09-14 12:24 ` [PATCH v4 01/10] qcow2-refcount: improve style of check_refcounts_l2() Vladimir Sementsov-Ogievskiy
2021-09-14 12:24 ` [PATCH v4 02/10] qcow2: compressed read: simplify cluster descriptor passing Vladimir Sementsov-Ogievskiy
2021-09-14 12:24 ` [PATCH v4 03/10] qcow2: introduce qcow2_parse_compressed_l2_entry() helper Vladimir Sementsov-Ogievskiy
2021-09-14 12:24 ` [PATCH v4 04/10] qcow2-refcount: introduce fix_l2_entry_by_zero() Vladimir Sementsov-Ogievskiy
2021-09-14 12:24 ` [PATCH v4 05/10] qcow2-refcount: fix_l2_entry_by_zero(): also zero L2 entry bitmap Vladimir Sementsov-Ogievskiy
2021-09-14 12:24 ` [PATCH v4 06/10] qcow2-refcount: check_refcounts_l2(): check l2_bitmap Vladimir Sementsov-Ogievskiy
2021-09-14 14:25 ` Eric Blake
2021-09-14 14:26 ` Vladimir Sementsov-Ogievskiy
2021-09-14 17:12 ` Hanna Reitz
2021-09-14 12:24 ` [PATCH v4 07/10] qcow2-refcount: check_refcounts_l2(): check reserved bits Vladimir Sementsov-Ogievskiy
2021-09-14 17:15 ` Hanna Reitz
2021-09-15 6:59 ` Vladimir Sementsov-Ogievskiy
2021-09-14 12:24 ` [PATCH v4 08/10] qcow2-refcount: improve style of check_refcounts_l1() Vladimir Sementsov-Ogievskiy
2021-09-14 12:24 ` [PATCH v4 09/10] qcow2-refcount: check_refcounts_l1(): check reserved bits Vladimir Sementsov-Ogievskiy
2021-09-14 12:24 ` [PATCH v4 10/10] qcow2-refcount: check_refblocks(): add separate message for reserved Vladimir Sementsov-Ogievskiy
2021-09-15 10:26 ` [PATCH v4 00/10] qcow2 check: check some reserved bits and subcluster bitmaps Hanna Reitz
2021-09-15 11:36 ` Vladimir Sementsov-Ogievskiy
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.