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