* [Qemu-devel] [PATCH 1/7] block/qcow2-refcount: fix check_oflag_copied
2018-06-19 18:34 [Qemu-devel] [PATCH 0/7] qcow2 check improvements Vladimir Sementsov-Ogievskiy
@ 2018-06-19 18:34 ` Vladimir Sementsov-Ogievskiy
2018-06-19 18:34 ` [Qemu-devel] [PATCH 2/7] block/qcow2-refcount: avoid eating RAM Vladimir Sementsov-Ogievskiy
` (5 subsequent siblings)
6 siblings, 0 replies; 15+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2018-06-19 18:34 UTC (permalink / raw)
To: qemu-block, qemu-devel; +Cc: kwolf, mreitz, vsementsov, den
Increase corruptions_fixed only after successful fix.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
block/qcow2-refcount.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 18c729aa27..f9d095aa2d 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -1816,7 +1816,7 @@ static int check_oflag_copied(BlockDriverState *bs, BdrvCheckResult *res,
for (i = 0; i < s->l1_size; i++) {
uint64_t l1_entry = s->l1_table[i];
uint64_t l2_offset = l1_entry & L1E_OFFSET_MASK;
- bool l2_dirty = false;
+ int l2_fixed_entries = 0;
if (!l2_offset) {
continue;
@@ -1878,8 +1878,7 @@ static int check_oflag_copied(BlockDriverState *bs, BdrvCheckResult *res,
l2_table[j] = cpu_to_be64(refcount == 1
? l2_entry | QCOW_OFLAG_COPIED
: l2_entry & ~QCOW_OFLAG_COPIED);
- l2_dirty = true;
- res->corruptions_fixed++;
+ l2_fixed_entries++;
} else {
res->corruptions++;
}
@@ -1887,7 +1886,7 @@ static int check_oflag_copied(BlockDriverState *bs, BdrvCheckResult *res,
}
}
- if (l2_dirty) {
+ if (l2_fixed_entries > 0) {
ret = qcow2_pre_write_overlap_check(bs, QCOW2_OL_ACTIVE_L2,
l2_offset, s->cluster_size);
if (ret < 0) {
@@ -1905,6 +1904,7 @@ static int check_oflag_copied(BlockDriverState *bs, BdrvCheckResult *res,
res->check_errors++;
goto fail;
}
+ res->corruptions_fixed += l2_fixed_entries;
}
}
--
2.11.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [Qemu-devel] [PATCH 2/7] block/qcow2-refcount: avoid eating RAM
2018-06-19 18:34 [Qemu-devel] [PATCH 0/7] qcow2 check improvements Vladimir Sementsov-Ogievskiy
2018-06-19 18:34 ` [Qemu-devel] [PATCH 1/7] block/qcow2-refcount: fix check_oflag_copied Vladimir Sementsov-Ogievskiy
@ 2018-06-19 18:34 ` Vladimir Sementsov-Ogievskiy
2018-06-19 18:47 ` Eric Blake
2018-06-19 18:34 ` [Qemu-devel] [PATCH 3/7] block/qcow2-refcount: check_refcounts_l2: refactor compressed case Vladimir Sementsov-Ogievskiy
` (4 subsequent siblings)
6 siblings, 1 reply; 15+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2018-06-19 18:34 UTC (permalink / raw)
To: qemu-block, qemu-devel; +Cc: kwolf, mreitz, vsementsov, den
qcow2_inc_refcounts_imrt() (through realloc_refcount_array()) can eat
unpredicted amount of memory on corrupted table entries, which are
referencing regions far beyond the end of file.
Prevent this, by skipping such regions from further processing.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
block/qcow2-refcount.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index f9d095aa2d..28d21bedc3 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -1505,6 +1505,14 @@ int qcow2_inc_refcounts_imrt(BlockDriverState *bs, BdrvCheckResult *res,
return 0;
}
+ if (offset + size - bdrv_getlength(bs->file->bs) > s->cluster_size) {
+ fprintf(stderr, "ERROR: counting reference for region exceeding the "
+ "end of the file by more than one cluster: offset 0x%" PRIx64
+ " size 0x%" PRIx64 "\n", offset, size);
+ res->corruptions++;
+ return 0;
+ }
+
start = start_of_cluster(s, offset);
last = start_of_cluster(s, offset + size - 1);
for(cluster_offset = start; cluster_offset <= last;
--
2.11.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH 2/7] block/qcow2-refcount: avoid eating RAM
2018-06-19 18:34 ` [Qemu-devel] [PATCH 2/7] block/qcow2-refcount: avoid eating RAM Vladimir Sementsov-Ogievskiy
@ 2018-06-19 18:47 ` Eric Blake
0 siblings, 0 replies; 15+ messages in thread
From: Eric Blake @ 2018-06-19 18:47 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block, qemu-devel; +Cc: kwolf, den, mreitz
On 06/19/2018 01:34 PM, Vladimir Sementsov-Ogievskiy wrote:
> qcow2_inc_refcounts_imrt() (through realloc_refcount_array()) can eat
> unpredicted amount of memory on corrupted table entries, which are
s/unpredicted/an unpredictable/
> referencing regions far beyond the end of file.
>
> Prevent this, by skipping such regions from further processing.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> block/qcow2-refcount.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
> index f9d095aa2d..28d21bedc3 100644
> --- a/block/qcow2-refcount.c
> +++ b/block/qcow2-refcount.c
> @@ -1505,6 +1505,14 @@ int qcow2_inc_refcounts_imrt(BlockDriverState *bs, BdrvCheckResult *res,
> return 0;
> }
>
> + if (offset + size - bdrv_getlength(bs->file->bs) > s->cluster_size) {
bdrv_getlength() can fail (returning a negative value); this needs to be
refactored so that you aren't performing arithmetic comparisons after
such a failure (even if that failure is unlikely).
> + fprintf(stderr, "ERROR: counting reference for region exceeding the "
> + "end of the file by more than one cluster: offset 0x%" PRIx64
> + " size 0x%" PRIx64 "\n", offset, size);
Why is this dumping directly to stderr?
/me reads the file
Oh. We probably ought to fix the code to pass an Error **errp parameter
through the callstack, but that's a bigger audit (and not the fault of
your patch for copying existing usage).
> + res->corruptions++;
> + return 0;
> + }
> +
> start = start_of_cluster(s, offset);
> last = start_of_cluster(s, offset + size - 1);
> for(cluster_offset = start; cluster_offset <= last;
>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 15+ messages in thread
* [Qemu-devel] [PATCH 3/7] block/qcow2-refcount: check_refcounts_l2: refactor compressed case
2018-06-19 18:34 [Qemu-devel] [PATCH 0/7] qcow2 check improvements Vladimir Sementsov-Ogievskiy
2018-06-19 18:34 ` [Qemu-devel] [PATCH 1/7] block/qcow2-refcount: fix check_oflag_copied Vladimir Sementsov-Ogievskiy
2018-06-19 18:34 ` [Qemu-devel] [PATCH 2/7] block/qcow2-refcount: avoid eating RAM Vladimir Sementsov-Ogievskiy
@ 2018-06-19 18:34 ` Vladimir Sementsov-Ogievskiy
2018-06-19 18:50 ` Eric Blake
2018-06-19 18:34 ` [Qemu-devel] [PATCH 4/7] block/qcow2-refcount: check_refcounts_l2: reduce ignored overlaps Vladimir Sementsov-Ogievskiy
` (3 subsequent siblings)
6 siblings, 1 reply; 15+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2018-06-19 18:34 UTC (permalink / raw)
To: qemu-block, qemu-devel; +Cc: kwolf, mreitz, vsementsov, den
Separate offset and size of compressed cluster.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
block/qcow2-refcount.c | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 28d21bedc3..42167b7040 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -1564,7 +1564,7 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
BDRVQcow2State *s = bs->opaque;
uint64_t *l2_table, l2_entry;
uint64_t next_contiguous_offset = 0;
- int i, l2_size, nb_csectors, ret;
+ int i, l2_size, ret;
/* Read L2 table from disk */
l2_size = s->l2_size * sizeof(uint64_t);
@@ -1583,6 +1583,9 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
switch (qcow2_get_cluster_type(l2_entry)) {
case QCOW2_CLUSTER_COMPRESSED:
+ {
+ int64_t csize, coffset;
+
/* Compressed clusters don't have QCOW_OFLAG_COPIED */
if (l2_entry & QCOW_OFLAG_COPIED) {
fprintf(stderr, "ERROR: coffset=0x%" PRIx64 ": "
@@ -1593,12 +1596,13 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
}
/* Mark cluster as used */
- nb_csectors = ((l2_entry >> s->csize_shift) &
- s->csize_mask) + 1;
- l2_entry &= s->cluster_offset_mask;
+ csize = (((l2_entry >> s->csize_shift) & s->csize_mask) + 1) *
+ BDRV_SECTOR_SIZE;
+ coffset = l2_entry & s->cluster_offset_mask &
+ ~(BDRV_SECTOR_SIZE - 1);
ret = qcow2_inc_refcounts_imrt(bs, res,
refcount_table, refcount_table_size,
- l2_entry & ~511, nb_csectors * 512);
+ coffset, csize);
if (ret < 0) {
goto fail;
}
@@ -1615,6 +1619,7 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
res->bfi.fragmented_clusters++;
}
break;
+ }
case QCOW2_CLUSTER_ZERO_ALLOC:
case QCOW2_CLUSTER_NORMAL:
--
2.11.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH 3/7] block/qcow2-refcount: check_refcounts_l2: refactor compressed case
2018-06-19 18:34 ` [Qemu-devel] [PATCH 3/7] block/qcow2-refcount: check_refcounts_l2: refactor compressed case Vladimir Sementsov-Ogievskiy
@ 2018-06-19 18:50 ` Eric Blake
2018-06-20 9:37 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 15+ messages in thread
From: Eric Blake @ 2018-06-19 18:50 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block, qemu-devel; +Cc: kwolf, den, mreitz
On 06/19/2018 01:34 PM, Vladimir Sementsov-Ogievskiy wrote:
> Separate offset and size of compressed cluster.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> block/qcow2-refcount.c | 15 ++++++++++-----
> 1 file changed, 10 insertions(+), 5 deletions(-)
Hmm, I wonder if this duplicates my pending patch:
https://lists.gnu.org/archive/html/qemu-devel/2018-04/msg04542.html
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH 3/7] block/qcow2-refcount: check_refcounts_l2: refactor compressed case
2018-06-19 18:50 ` Eric Blake
@ 2018-06-20 9:37 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 15+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2018-06-20 9:37 UTC (permalink / raw)
To: Eric Blake, qemu-block, qemu-devel; +Cc: kwolf, den, mreitz
19.06.2018 21:50, Eric Blake wrote:
> On 06/19/2018 01:34 PM, Vladimir Sementsov-Ogievskiy wrote:
>> Separate offset and size of compressed cluster.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>> block/qcow2-refcount.c | 15 ++++++++++-----
>> 1 file changed, 10 insertions(+), 5 deletions(-)
>
> Hmm, I wonder if this duplicates my pending patch:
>
> https://lists.gnu.org/archive/html/qemu-devel/2018-04/msg04542.html
>
hm which one? don't see.
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 15+ messages in thread
* [Qemu-devel] [PATCH 4/7] block/qcow2-refcount: check_refcounts_l2: reduce ignored overlaps
2018-06-19 18:34 [Qemu-devel] [PATCH 0/7] qcow2 check improvements Vladimir Sementsov-Ogievskiy
` (2 preceding siblings ...)
2018-06-19 18:34 ` [Qemu-devel] [PATCH 3/7] block/qcow2-refcount: check_refcounts_l2: refactor compressed case Vladimir Sementsov-Ogievskiy
@ 2018-06-19 18:34 ` Vladimir Sementsov-Ogievskiy
2018-06-19 18:34 ` [Qemu-devel] [PATCH 5/7] block/qcow2-refcount: check_refcounts_l2: split fix_l2_entry_to_zero Vladimir Sementsov-Ogievskiy
` (2 subsequent siblings)
6 siblings, 0 replies; 15+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2018-06-19 18:34 UTC (permalink / raw)
To: qemu-block, qemu-devel; +Cc: kwolf, mreitz, vsementsov, den
Reduce number of structures ignored in overlap check: when checking
active table ignore active tables, when checking inactive table ignore
inactive ones.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
block/qcow2-refcount.c | 16 +++++++++-------
1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 42167b7040..02583f260b 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -1559,7 +1559,7 @@ enum {
static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
void **refcount_table,
int64_t *refcount_table_size, int64_t l2_offset,
- int flags, BdrvCheckMode fix)
+ int flags, BdrvCheckMode fix, bool active)
{
BDRVQcow2State *s = bs->opaque;
uint64_t *l2_table, l2_entry;
@@ -1648,11 +1648,12 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
if (fix & BDRV_FIX_ERRORS) {
uint64_t l2e_offset =
l2_offset + (uint64_t)i * sizeof(uint64_t);
+ int ign = active ? QCOW2_OL_ACTIVE_L2 :
+ QCOW2_OL_INACTIVE_L2;
l2_entry = QCOW_OFLAG_ZERO;
l2_table[i] = cpu_to_be64(l2_entry);
- ret = qcow2_pre_write_overlap_check(bs,
- QCOW2_OL_ACTIVE_L2 | QCOW2_OL_INACTIVE_L2,
+ ret = qcow2_pre_write_overlap_check(bs, ign,
l2e_offset, sizeof(uint64_t));
if (ret < 0) {
fprintf(stderr, "ERROR: Overlap check failed\n");
@@ -1726,7 +1727,7 @@ static int check_refcounts_l1(BlockDriverState *bs,
void **refcount_table,
int64_t *refcount_table_size,
int64_t l1_table_offset, int l1_size,
- int flags, BdrvCheckMode fix)
+ int flags, BdrvCheckMode fix, bool active)
{
BDRVQcow2State *s = bs->opaque;
uint64_t *l1_table = NULL, l2_offset, l1_size2;
@@ -1782,7 +1783,7 @@ static int check_refcounts_l1(BlockDriverState *bs,
/* Process and check L2 entries */
ret = check_refcounts_l2(bs, res, refcount_table,
refcount_table_size, l2_offset, flags,
- fix);
+ fix, active);
if (ret < 0) {
goto fail;
}
@@ -2068,7 +2069,7 @@ static int calculate_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
/* current L1 table */
ret = check_refcounts_l1(bs, res, refcount_table, nb_clusters,
s->l1_table_offset, s->l1_size, CHECK_FRAG_INFO,
- fix);
+ fix, true);
if (ret < 0) {
return ret;
}
@@ -2091,7 +2092,8 @@ static int calculate_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
continue;
}
ret = check_refcounts_l1(bs, res, refcount_table, nb_clusters,
- sn->l1_table_offset, sn->l1_size, 0, fix);
+ sn->l1_table_offset, sn->l1_size, 0, fix,
+ false);
if (ret < 0) {
return ret;
}
--
2.11.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [Qemu-devel] [PATCH 5/7] block/qcow2-refcount: check_refcounts_l2: split fix_l2_entry_to_zero
2018-06-19 18:34 [Qemu-devel] [PATCH 0/7] qcow2 check improvements Vladimir Sementsov-Ogievskiy
` (3 preceding siblings ...)
2018-06-19 18:34 ` [Qemu-devel] [PATCH 4/7] block/qcow2-refcount: check_refcounts_l2: reduce ignored overlaps Vladimir Sementsov-Ogievskiy
@ 2018-06-19 18:34 ` Vladimir Sementsov-Ogievskiy
2018-06-19 18:34 ` [Qemu-devel] [PATCH 6/7] block/qcow2-refcount: fix out-of-file L1 entries to be zero Vladimir Sementsov-Ogievskiy
2018-06-19 18:34 ` [Qemu-devel] [PATCH 7/7] block/qcow2-refcount: fix out-of-file L2 entries to be read-as-zero Vladimir Sementsov-Ogievskiy
6 siblings, 0 replies; 15+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2018-06-19 18:34 UTC (permalink / raw)
To: qemu-block, qemu-devel; +Cc: kwolf, mreitz, vsementsov, den
Split entry repairing to separate function, to be reused later.
Note: entry in in-memory l2 table (local variable in
check_refcounts_l2) is not updated after this patch.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
block/qcow2-refcount.c | 147 ++++++++++++++++++++++++++++++++++++-------------
1 file changed, 109 insertions(+), 38 deletions(-)
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 02583f260b..d993252fb6 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -1548,6 +1548,99 @@ enum {
CHECK_FRAG_INFO = 0x2, /* update BlockFragInfo counters */
};
+/* Update entry in L1 or L2 table
+ *
+ * Returns: -errno if overlap check failed
+ * 0 if write failed
+ * 1 on success
+ */
+static int write_table_entry(BlockDriverState *bs, const char *table_name,
+ uint64_t table_offset, int entry_index,
+ uint64_t new_val, int ign)
+{
+ int ret;
+ uint64_t entry_offset =
+ table_offset + (uint64_t)entry_index * sizeof(new_val);
+
+ cpu_to_be64s(&new_val);
+ ret = qcow2_pre_write_overlap_check(bs, ign, entry_offset, sizeof(new_val));
+ if (ret < 0) {
+ fprintf(stderr,
+ "ERROR: Can't write %s table entry: overlap check failed: %s\n",
+ table_name, strerror(-ret));
+ return ret;
+ }
+
+ ret = bdrv_pwrite_sync(bs->file, entry_offset, &new_val, sizeof(new_val));
+ if (ret < 0) {
+ fprintf(stderr, "ERROR: Failed to overwrite %s table entry: %s\n",
+ table_name, strerror(-ret));
+ return 0;
+ }
+
+ return 1;
+}
+
+/* Try to fix (if allowed) entry in L1 or L2 table. Update @res correspondingly.
+ *
+ * Returns: -errno if overlap check failed
+ * 0 if entry was not updated for other reason
+ * (fixing disabled or write failed)
+ * 1 on success
+ */
+static int fix_table_entry(BlockDriverState *bs, BdrvCheckResult *res,
+ BdrvCheckMode fix, const char *table_name,
+ uint64_t table_offset, int entry_index,
+ uint64_t new_val, int ign,
+ const char *fmt, va_list args)
+{
+ int ret;
+
+ fprintf(stderr, fix & BDRV_FIX_ERRORS ? "Repairing: " : "ERROR: ");
+ vfprintf(stderr, fmt, args);
+ fprintf(stderr, "\n");
+
+ if (!(fix & BDRV_FIX_ERRORS)) {
+ res->corruptions++;
+ return 0;
+ }
+
+ ret = write_table_entry(bs, table_name, table_offset, entry_index, new_val,
+ ign);
+
+ if (ret == 1) {
+ res->corruptions_fixed++;
+ } else {
+ res->check_errors++;
+ }
+
+ return ret;
+}
+
+/* Make L2 entry to be QCOW2_CLUSTER_ZERO_PLAIN
+ *
+ * Returns: -errno if overlap check failed
+ * 0 if write failed
+ * 1 on success
+ */
+static int fix_l2_entry_to_zero(BlockDriverState *bs, BdrvCheckResult *res,
+ BdrvCheckMode fix, int64_t l2_offset,
+ int l2_index, bool active,
+ const char *fmt, ...)
+{
+ int ret;
+ int ign = active ? QCOW2_OL_ACTIVE_L2 : QCOW2_OL_INACTIVE_L2;
+ uint64_t l2_entry = QCOW_OFLAG_ZERO;
+ va_list args;
+
+ va_start(args, fmt);
+ ret = fix_table_entry(bs, res, fix, "L2", l2_offset, l2_index, l2_entry,
+ ign, fmt, args);
+ va_end(args);
+
+ 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
@@ -1640,46 +1733,24 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
if (qcow2_get_cluster_type(l2_entry) ==
QCOW2_CLUSTER_ZERO_ALLOC)
{
- fprintf(stderr, "%s offset=%" PRIx64 ": Preallocated zero "
- "cluster is not properly aligned; L2 entry "
- "corrupted.\n",
- fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR",
+ ret = fix_l2_entry_to_zero(
+ bs, res, fix, l2_offset, i, active,
+ "offset=%" PRIx64 ": Preallocated zero cluster is "
+ "not properly aligned; L2 entry corrupted.",
offset);
- if (fix & BDRV_FIX_ERRORS) {
- uint64_t l2e_offset =
- l2_offset + (uint64_t)i * sizeof(uint64_t);
- int ign = active ? QCOW2_OL_ACTIVE_L2 :
- QCOW2_OL_INACTIVE_L2;
-
- l2_entry = QCOW_OFLAG_ZERO;
- l2_table[i] = cpu_to_be64(l2_entry);
- ret = qcow2_pre_write_overlap_check(bs, ign,
- l2e_offset, sizeof(uint64_t));
- if (ret < 0) {
- fprintf(stderr, "ERROR: Overlap check failed\n");
- res->check_errors++;
- /* Something is seriously wrong, so abort checking
- * this L2 table */
- goto fail;
- }
-
- ret = bdrv_pwrite_sync(bs->file, l2e_offset,
- &l2_table[i], sizeof(uint64_t));
- if (ret < 0) {
- fprintf(stderr, "ERROR: Failed to overwrite L2 "
- "table entry: %s\n", strerror(-ret));
- res->check_errors++;
- /* Do not abort, continue checking the rest of this
- * L2 table's entries */
- } else {
- res->corruptions_fixed++;
- /* Skip marking the cluster as used
- * (it is unused now) */
- continue;
- }
- } else {
- res->corruptions++;
+ if (ret < 0) {
+ /* Something is seriously wrong, so abort checking
+ * this L2 table */
+ goto fail;
+ }
+ if (ret == 1) {
+ /* Skip marking the cluster as used
+ * (it is unused now) */
+ continue;
}
+ /* Entry was not updated, but do not abort, mark cluster
+ * as used and continue checking the rest of this L2
+ * table's entries */
} else {
fprintf(stderr, "ERROR offset=%" PRIx64 ": Data cluster is "
"not properly aligned; L2 entry corrupted.\n", offset);
--
2.11.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [Qemu-devel] [PATCH 6/7] block/qcow2-refcount: fix out-of-file L1 entries to be zero
2018-06-19 18:34 [Qemu-devel] [PATCH 0/7] qcow2 check improvements Vladimir Sementsov-Ogievskiy
` (4 preceding siblings ...)
2018-06-19 18:34 ` [Qemu-devel] [PATCH 5/7] block/qcow2-refcount: check_refcounts_l2: split fix_l2_entry_to_zero Vladimir Sementsov-Ogievskiy
@ 2018-06-19 18:34 ` Vladimir Sementsov-Ogievskiy
2018-06-19 18:54 ` Eric Blake
2018-06-19 18:34 ` [Qemu-devel] [PATCH 7/7] block/qcow2-refcount: fix out-of-file L2 entries to be read-as-zero Vladimir Sementsov-Ogievskiy
6 siblings, 1 reply; 15+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2018-06-19 18:34 UTC (permalink / raw)
To: qemu-block, qemu-devel; +Cc: kwolf, mreitz, vsementsov, den
Zero out corrupted L1 table entry, which reference L2 table out of
underlying file.
Zero L1 table entry means that "the L2 table and all clusters described
by this L2 table are unallocated."
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
block/qcow2-refcount.c | 37 +++++++++++++++++++++++++++++++++++++
1 file changed, 37 insertions(+)
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index d993252fb6..3c9e2da39e 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -1641,6 +1641,29 @@ static int fix_l2_entry_to_zero(BlockDriverState *bs, BdrvCheckResult *res,
return ret;
}
+/* Zero out L1 entry
+ *
+ * Returns: -errno if overlap check failed
+ * 0 if write failed
+ * 1 on success
+ */
+static int fix_l1_entry_to_zero(BlockDriverState *bs, BdrvCheckResult *res,
+ BdrvCheckMode fix, int64_t l1_offset,
+ int l1_index, bool active,
+ const char *fmt, ...)
+{
+ int ret;
+ int ign = active ? QCOW2_OL_ACTIVE_L2 : QCOW2_OL_INACTIVE_L2;
+ va_list args;
+
+ va_start(args, fmt);
+ ret = fix_table_entry(bs, res, fix, "L1", l1_offset, l1_index, 0, ign,
+ fmt, args);
+ va_end(args);
+
+ 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
@@ -1837,6 +1860,20 @@ static int check_refcounts_l1(BlockDriverState *bs,
if (l2_offset) {
/* Mark L2 table as used */
l2_offset &= L1E_OFFSET_MASK;
+ if (l2_offset >= bdrv_getlength(bs->file->bs)) {
+ ret = fix_l1_entry_to_zero(
+ bs, res, fix, l1_table_offset, i, active,
+ "l2 table offset out of file: offset 0x%" PRIx64,
+ l2_offset);
+ if (ret < 0) {
+ /* Something is seriously wrong, so abort checking
+ * this L1 table */
+ goto fail;
+ }
+
+ continue;
+ }
+
ret = qcow2_inc_refcounts_imrt(bs, res,
refcount_table, refcount_table_size,
l2_offset, s->cluster_size);
--
2.11.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH 6/7] block/qcow2-refcount: fix out-of-file L1 entries to be zero
2018-06-19 18:34 ` [Qemu-devel] [PATCH 6/7] block/qcow2-refcount: fix out-of-file L1 entries to be zero Vladimir Sementsov-Ogievskiy
@ 2018-06-19 18:54 ` Eric Blake
2018-06-20 9:34 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 15+ messages in thread
From: Eric Blake @ 2018-06-19 18:54 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block, qemu-devel; +Cc: kwolf, den, mreitz
On 06/19/2018 01:34 PM, Vladimir Sementsov-Ogievskiy wrote:
> Zero out corrupted L1 table entry, which reference L2 table out of
> underlying file.
> Zero L1 table entry means that "the L2 table and all clusters described
> by this L2 table are unallocated."
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> block/qcow2-refcount.c | 37 +++++++++++++++++++++++++++++++++++++
> 1 file changed, 37 insertions(+)
>
> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
> index d993252fb6..3c9e2da39e 100644
> --- a/block/qcow2-refcount.c
> +++ b/block/qcow2-refcount.c
> @@ -1641,6 +1641,29 @@ static int fix_l2_entry_to_zero(BlockDriverState *bs, BdrvCheckResult *res,
> return ret;
> }
>
> +/* Zero out L1 entry
> + *
> + * Returns: -errno if overlap check failed
> + * 0 if write failed
If the write failed, wouldn't there be an errno value worth returning?
> + * 1 on success
> + */
> +static int fix_l1_entry_to_zero(BlockDriverState *bs, BdrvCheckResult *res,
> + BdrvCheckMode fix, int64_t l1_offset,
> + int l1_index, bool active,
> + const char *fmt, ...)
> +{
> + int ret;
> + int ign = active ? QCOW2_OL_ACTIVE_L2 : QCOW2_OL_INACTIVE_L2;
> + va_list args;
> +
> + va_start(args, fmt);
> + ret = fix_table_entry(bs, res, fix, "L1", l1_offset, l1_index, 0, ign,
> + fmt, args);
> + va_end(args);
> +
> + 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
> @@ -1837,6 +1860,20 @@ static int check_refcounts_l1(BlockDriverState *bs,
> if (l2_offset) {
> /* Mark L2 table as used */
> l2_offset &= L1E_OFFSET_MASK;
> + if (l2_offset >= bdrv_getlength(bs->file->bs)) {
Again, bdrv_getlength() can fail; you want to make sure that you check
for failures before using it in comparisons.
> + ret = fix_l1_entry_to_zero(
> + bs, res, fix, l1_table_offset, i, active,
> + "l2 table offset out of file: offset 0x%" PRIx64,
> + l2_offset);
> + if (ret < 0) {
> + /* Something is seriously wrong, so abort checking
> + * this L1 table */
> + goto fail;
> + }
> +
> + continue;
> + }
> +
> ret = qcow2_inc_refcounts_imrt(bs, res,
> refcount_table, refcount_table_size,
> l2_offset, s->cluster_size);
>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH 6/7] block/qcow2-refcount: fix out-of-file L1 entries to be zero
2018-06-19 18:54 ` Eric Blake
@ 2018-06-20 9:34 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 15+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2018-06-20 9:34 UTC (permalink / raw)
To: Eric Blake, qemu-block, qemu-devel; +Cc: kwolf, den, mreitz
19.06.2018 21:54, Eric Blake wrote:
> On 06/19/2018 01:34 PM, Vladimir Sementsov-Ogievskiy wrote:
>> Zero out corrupted L1 table entry, which reference L2 table out of
>> underlying file.
>> Zero L1 table entry means that "the L2 table and all clusters described
>> by this L2 table are unallocated."
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>> block/qcow2-refcount.c | 37 +++++++++++++++++++++++++++++++++++++
>> 1 file changed, 37 insertions(+)
>>
>> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
>> index d993252fb6..3c9e2da39e 100644
>> --- a/block/qcow2-refcount.c
>> +++ b/block/qcow2-refcount.c
>> @@ -1641,6 +1641,29 @@ static int
>> fix_l2_entry_to_zero(BlockDriverState *bs, BdrvCheckResult *res,
>> return ret;
>> }
>> +/* Zero out L1 entry
>> + *
>> + * Returns: -errno if overlap check failed
>> + * 0 if write failed
>
> If the write failed, wouldn't there be an errno value worth returning?
it's done to mimic existing behavior in check_refcounts_l2, when on
rewriting error, overlap error is fatal and write error is not.
>
>> + * 1 on success
>> + */
>> +static int fix_l1_entry_to_zero(BlockDriverState *bs,
>> BdrvCheckResult *res,
>> + BdrvCheckMode fix, int64_t l1_offset,
>> + int l1_index, bool active,
>> + const char *fmt, ...)
>> +{
>> + int ret;
>> + int ign = active ? QCOW2_OL_ACTIVE_L2 : QCOW2_OL_INACTIVE_L2;
>> + va_list args;
>> +
>> + va_start(args, fmt);
>> + ret = fix_table_entry(bs, res, fix, "L1", l1_offset, l1_index,
>> 0, ign,
>> + fmt, args);
>> + va_end(args);
>> +
>> + 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
>> @@ -1837,6 +1860,20 @@ static int check_refcounts_l1(BlockDriverState
>> *bs,
>> if (l2_offset) {
>> /* Mark L2 table as used */
>> l2_offset &= L1E_OFFSET_MASK;
>> + if (l2_offset >= bdrv_getlength(bs->file->bs)) {
>
> Again, bdrv_getlength() can fail; you want to make sure that you check
> for failures before using it in comparisons.
>
>> + ret = fix_l1_entry_to_zero(
>> + bs, res, fix, l1_table_offset, i, active,
>> + "l2 table offset out of file: offset 0x%"
>> PRIx64,
>> + l2_offset);
>> + if (ret < 0) {
>> + /* Something is seriously wrong, so abort checking
>> + * this L1 table */
>> + goto fail;
>> + }
>> +
>> + continue;
>> + }
>> +
>> ret = qcow2_inc_refcounts_imrt(bs, res,
>> refcount_table,
>> refcount_table_size,
>> l2_offset,
>> s->cluster_size);
>>
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 15+ messages in thread
* [Qemu-devel] [PATCH 7/7] block/qcow2-refcount: fix out-of-file L2 entries to be read-as-zero
2018-06-19 18:34 [Qemu-devel] [PATCH 0/7] qcow2 check improvements Vladimir Sementsov-Ogievskiy
` (5 preceding siblings ...)
2018-06-19 18:34 ` [Qemu-devel] [PATCH 6/7] block/qcow2-refcount: fix out-of-file L1 entries to be zero Vladimir Sementsov-Ogievskiy
@ 2018-06-19 18:34 ` Vladimir Sementsov-Ogievskiy
6 siblings, 0 replies; 15+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2018-06-19 18:34 UTC (permalink / raw)
To: qemu-block, qemu-devel; +Cc: kwolf, mreitz, vsementsov, den
Rewrite corrupted L2 table entry, which reference space out of
underlying file.
Make this L2 table entry read-as-all-zeros without any allocation.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
block/qcow2-refcount.c | 32 ++++++++++++++++++++++++++++++++
1 file changed, 32 insertions(+)
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 3c9e2da39e..cbad8355f3 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -1714,8 +1714,30 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
/* Mark cluster as used */
csize = (((l2_entry >> s->csize_shift) & s->csize_mask) + 1) *
BDRV_SECTOR_SIZE;
+ if (csize > s->cluster_size) {
+ ret = fix_l2_entry_to_zero(
+ bs, res, fix, l2_offset, i, active,
+ "compressed cluster larger than cluster: size 0x%"
+ PRIx64, csize);
+ if (ret < 0) {
+ goto fail;
+ }
+ continue;
+ }
+
coffset = l2_entry & s->cluster_offset_mask &
~(BDRV_SECTOR_SIZE - 1);
+ if (coffset >= bdrv_getlength(bs->file->bs)) {
+ ret = fix_l2_entry_to_zero(
+ bs, res, fix, l2_offset, i, active,
+ "compressed cluster out of file: offset 0x%" PRIx64,
+ coffset);
+ if (ret < 0) {
+ goto fail;
+ }
+ continue;
+ }
+
ret = qcow2_inc_refcounts_imrt(bs, res,
refcount_table, refcount_table_size,
coffset, csize);
@@ -1742,6 +1764,16 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
{
uint64_t offset = l2_entry & L2E_OFFSET_MASK;
+ if (offset >= bdrv_getlength(bs->file->bs)) {
+ ret = fix_l2_entry_to_zero(
+ bs, res, fix, l2_offset, i, active,
+ "cluster out of file: offset 0x%" PRIx64, offset);
+ if (ret < 0) {
+ goto fail;
+ }
+ continue;
+ }
+
if (flags & CHECK_FRAG_INFO) {
res->bfi.allocated_clusters++;
if (next_contiguous_offset &&
--
2.11.1
^ permalink raw reply related [flat|nested] 15+ messages in thread