[Qemu-devel] [RFC PATCH 0/2] Allow checking and repairing corrupted internal snapshots

[Qemu-devel] [RFC PATCH 0/2] Allow checking and repairing corrupted internal snapshots

[Alberto Garcia]

Here's a small series that prevents the user from opening corrupted
internal snapshots and allows 'qemu-img check' to detect and repair
them.

This is an RFC because I tried a few different approaches to deal with
this but I'm not 100% convinced with any of them.

What this one does is simply delete the corrupted snapshots when
'qemu-img check' is run with '-r all'.

Other possible alternatives:

- Do the same but with a different option ('-r snapshots' or whatever)
- Zero the L1 size field of the corrupted snapshots (so they're empty
  in practice) but leave them in the snapshot table, so the user would
  have to delete them manually afterwards.
- Ask interactively "Do you want to delete this snapshot?" for each
  one of them. This was mentioned in the mailing list, but is this
  really useful?

Berto

Alberto Garcia (2):
  qcow2: Allow checking and repairing corrupted internal snapshots
  qcow2: Check the L1 table parameters from all internal snapshots

 block/qcow2-snapshot.c     | 71 ++++++++++++++++++++++++++++++++++++++++++----
 block/qcow2.c              |  9 ++++--
 block/qcow2.h              |  4 ++-
 tests/qemu-iotests/080     | 16 ++++++++++-
 tests/qemu-iotests/080.out | 42 +++++++++++++++++++++++++--
 5 files changed, 131 insertions(+), 11 deletions(-)

-- 
2.11.0

[Qemu-devel] [RFC PATCH 1/2] qcow2: Allow checking and repairing corrupted internal snapshots

[Alberto Garcia]

The L1 table parameters of internal snapshots are generally not
checked by QEMU. This patch allows 'qemu-img check' to detect broken
snapshots and to skip them when doing the refcount consistency check.

Since without an L1 table we don't have a reliable way to recover the
data from the snapshot, when 'qemu-img check' runs in repair mode this
patch simply removes the corrupted snapshots.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 block/qcow2-snapshot.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++
 block/qcow2.c          |  7 ++++++-
 block/qcow2.h          |  2 ++
 3 files changed, 61 insertions(+), 1 deletion(-)

diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index cee25f582b..7a36073e3e 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -736,3 +736,56 @@ int qcow2_snapshot_load_tmp(BlockDriverState *bs,
 
     return 0;
 }
+
+/* Check the snapshot table and optionally delete the corrupted entries */
+int qcow2_snapshot_table_check(BlockDriverState *bs, BdrvCheckResult *result,
+                               BdrvCheckMode fix)
+{
+    BDRVQcow2State *s = bs->opaque;
+    bool keep_checking;
+    int ret, i;
+
+    do {
+        keep_checking = false;
+
+        for (i = 0; i < s->nb_snapshots; i++) {
+            QCowSnapshot *sn = s->snapshots + i;
+            bool found_corruption = false;
+
+            if (offset_into_cluster(s, sn->l1_table_offset)) {
+                fprintf(stderr, "%s snapshot %s (%s) l1_offset=%#" PRIx64 ": "
+                        "L1 table is not cluster aligned; snapshot table entry "
+                        "corrupted\n",
+                        (fix & BDRV_FIX_ERRORS) ? "Deleting" : "ERROR",
+                        sn->id_str, sn->name, sn->l1_table_offset);
+                found_corruption = true;
+            } else if (sn->l1_size > QCOW_MAX_L1_SIZE / sizeof(uint64_t)) {
+                fprintf(stderr, "%s snapshot %s (%s) l1_size=%#" PRIx32 ": "
+                        "L1 table is too large; snapshot table entry corrupted\n",
+                        (fix & BDRV_FIX_ERRORS) ? "Deleting" : "ERROR",
+                        sn->id_str, sn->name, sn->l1_size);
+                found_corruption = true;
+            }
+
+            if (found_corruption) {
+                result->corruptions++;
+                sn->l1_size = 0; /* Prevent this L1 table from being used */
+                if (fix & BDRV_FIX_ERRORS) {
+                    ret = qcow2_snapshot_delete(bs, sn->id_str, sn->name, NULL);
+                    if (ret < 0) {
+                        return ret;
+                    }
+                    result->corruptions_fixed++;
+                    /* If we modified the snapshot table we can't keep
+                     * iterating. We have to start again from the
+                     * beginning instead. */
+                    keep_checking = true;
+                    break;
+                }
+            }
+        }
+
+    } while (keep_checking);
+
+    return 0;
+}
diff --git a/block/qcow2.c b/block/qcow2.c
index 2c6c33b67c..20e16ea602 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -546,7 +546,12 @@ int qcow2_mark_consistent(BlockDriverState *bs)
 static int qcow2_check(BlockDriverState *bs, BdrvCheckResult *result,
                        BdrvCheckMode fix)
 {
-    int ret = qcow2_check_refcounts(bs, result, fix);
+    int ret = qcow2_snapshot_table_check(bs, result, fix);
+    if (ret < 0) {
+        return ret;
+    }
+
+    ret = qcow2_check_refcounts(bs, result, fix);
     if (ret < 0) {
         return ret;
     }
diff --git a/block/qcow2.h b/block/qcow2.h
index 1a84cc77b0..19f14bfc1e 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -637,6 +637,8 @@ int qcow2_snapshot_load_tmp(BlockDriverState *bs,
                             const char *snapshot_id,
                             const char *name,
                             Error **errp);
+int qcow2_snapshot_table_check(BlockDriverState *bs, BdrvCheckResult *result,
+                               BdrvCheckMode fix);
 
 void qcow2_free_snapshots(BlockDriverState *bs);
 int qcow2_read_snapshots(BlockDriverState *bs);
-- 
2.11.0

[Qemu-devel] [RFC PATCH 2/2] qcow2: Check the L1 table parameters from all internal snapshots

[Alberto Garcia]

The code that reads the qcow2 snapshot table takes the offset and size
of all snapshots' L1 table without doing any kind of checks.

Although qcow2_snapshot_load_tmp() does verify that the table size is
valid, the table offset is not checked at all. On top of that there
are several other code paths that deal with internal snapshots that
don't use that function or do any other check.

This patch verifies that all L1 tables are correctly aligned and the
size does not exceed the maximum allowed valued.

The check from qcow2_snapshot_load_tmp() is removed since it's now
useless (opening an image will fail before that).

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 block/qcow2-snapshot.c     | 18 +++++++++++++-----
 block/qcow2.c              |  2 +-
 block/qcow2.h              |  2 +-
 tests/qemu-iotests/080     | 16 +++++++++++++++-
 tests/qemu-iotests/080.out | 42 ++++++++++++++++++++++++++++++++++++++++--
 5 files changed, 70 insertions(+), 10 deletions(-)

diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 7a36073e3e..4424b7f1dc 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -44,7 +44,7 @@ void qcow2_free_snapshots(BlockDriverState *bs)
     s->nb_snapshots = 0;
 }
 
-int qcow2_read_snapshots(BlockDriverState *bs)
+int qcow2_read_snapshots(BlockDriverState *bs, int flags)
 {
     BDRVQcow2State *s = bs->opaque;
     QCowSnapshotHeader h;
@@ -121,6 +121,18 @@ int qcow2_read_snapshots(BlockDriverState *bs)
         offset += name_size;
         sn->name[name_size] = '\0';
 
+        if (!(flags & BDRV_O_CHECK)) {
+            if (offset_into_cluster(s, sn->l1_table_offset)) {
+                ret = -EINVAL;
+                goto fail;
+            }
+
+            if (sn->l1_size > QCOW_MAX_L1_SIZE / sizeof(uint64_t)) {
+                ret = -EFBIG;
+                goto fail;
+            }
+        }
+
         if (offset - s->snapshots_offset > QCOW_MAX_SNAPSHOTS_SIZE) {
             ret = -EFBIG;
             goto fail;
@@ -704,10 +716,6 @@ int qcow2_snapshot_load_tmp(BlockDriverState *bs,
     sn = &s->snapshots[snapshot_index];
 
     /* Allocate and read in the snapshot's L1 table */
-    if (sn->l1_size > QCOW_MAX_L1_SIZE / sizeof(uint64_t)) {
-        error_setg(errp, "Snapshot L1 table too large");
-        return -EFBIG;
-    }
     new_l1_bytes = sn->l1_size * sizeof(uint64_t);
     new_l1_table = qemu_try_blockalign(bs->file->bs,
                                        ROUND_UP(new_l1_bytes, 512));
diff --git a/block/qcow2.c b/block/qcow2.c
index 20e16ea602..f56947aebf 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1474,7 +1474,7 @@ static int qcow2_do_open(BlockDriverState *bs, QDict *options, int flags,
     s->snapshots_offset = header.snapshots_offset;
     s->nb_snapshots = header.nb_snapshots;
 
-    ret = qcow2_read_snapshots(bs);
+    ret = qcow2_read_snapshots(bs, flags);
     if (ret < 0) {
         error_setg_errno(errp, -ret, "Could not read snapshots");
         goto fail;
diff --git a/block/qcow2.h b/block/qcow2.h
index 19f14bfc1e..fcd336aec4 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -641,7 +641,7 @@ int qcow2_snapshot_table_check(BlockDriverState *bs, BdrvCheckResult *result,
                                BdrvCheckMode fix);
 
 void qcow2_free_snapshots(BlockDriverState *bs);
-int qcow2_read_snapshots(BlockDriverState *bs);
+int qcow2_read_snapshots(BlockDriverState *bs, int flags);
 
 /* qcow2-cache.c functions */
 Qcow2Cache *qcow2_cache_create(BlockDriverState *bs, int num_tables,
diff --git a/tests/qemu-iotests/080 b/tests/qemu-iotests/080
index 1c2bd85742..728a5f937a 100755
--- a/tests/qemu-iotests/080
+++ b/tests/qemu-iotests/080
@@ -171,13 +171,27 @@ poke_file "$TEST_IMG" "$offset_l2_table_0" "\x80\x00\x00\xff\xff\xff\x00\x00"
 { $QEMU_IMG snapshot -c test $TEST_IMG; } 2>&1 | _filter_qemu_io | _filter_testdir
 
 echo
-echo "== Invalid snapshot L1 table =="
+echo "== Invalid snapshot L1 table offset =="
+_make_test_img 64M
+{ $QEMU_IO -c "write 0 512" $TEST_IMG; } 2>&1 | _filter_qemu_io | _filter_testdir
+{ $QEMU_IMG snapshot -c test $TEST_IMG; } 2>&1 | _filter_qemu_io | _filter_testdir
+poke_file "$TEST_IMG" "$offset_snap1_l1_offset" "\x00\x00\x00\x00\x00\x40\x02\x00"
+{ $QEMU_IMG convert -s test $TEST_IMG $TEST_IMG.snap; } 2>&1 | _filter_testdir
+
+# Fix the image
+_check_test_img -r all
+
+echo
+echo "== Invalid snapshot L1 table size =="
 _make_test_img 64M
 { $QEMU_IO -c "write 0 512" $TEST_IMG; } 2>&1 | _filter_qemu_io | _filter_testdir
 { $QEMU_IMG snapshot -c test $TEST_IMG; } 2>&1 | _filter_testdir
 poke_file "$TEST_IMG" "$offset_snap1_l1_size" "\x10\x00\x00\x00"
 { $QEMU_IMG convert -s test $TEST_IMG $TEST_IMG.snap; } 2>&1 | _filter_testdir
 
+# Fix the image
+_check_test_img -r all
+
 # success, all done
 echo "*** done"
 rm -f $seq.full
diff --git a/tests/qemu-iotests/080.out b/tests/qemu-iotests/080.out
index 6a7fda1356..eb26496566 100644
--- a/tests/qemu-iotests/080.out
+++ b/tests/qemu-iotests/080.out
@@ -59,9 +59,47 @@ wrote 512/512 bytes at offset 0
 qemu-img: Could not create snapshot 'test': -27 (File too large)
 qemu-img: Could not create snapshot 'test': -11 (Resource temporarily unavailable)
 
-== Invalid snapshot L1 table ==
+== Invalid snapshot L1 table offset ==
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
 wrote 512/512 bytes at offset 0
 512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-qemu-img: Failed to load snapshot: Snapshot L1 table too large
+qemu-img: Could not open 'TEST_DIR/t.qcow2': Could not read snapshots: Invalid argument
+Deleting snapshot 1 (test) l1_offset=0x400200: L1 table is not cluster aligned; snapshot table entry corrupted
+Leaked cluster 4 refcount=2 reference=1
+Leaked cluster 5 refcount=2 reference=1
+Leaked cluster 6 refcount=1 reference=0
+Repairing cluster 4 refcount=2 reference=1
+Repairing cluster 5 refcount=2 reference=1
+Repairing cluster 6 refcount=1 reference=0
+Repairing OFLAG_COPIED L2 cluster: l1_index=0 l1_entry=40000 refcount=1
+Repairing OFLAG_COPIED data cluster: l2_entry=50000 refcount=1
+The following inconsistencies were found and repaired:
+
+    3 leaked clusters
+    3 corruptions
+
+Double checking the fixed image now...
+No errors were found on the image.
+
+== Invalid snapshot L1 table size ==
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+wrote 512/512 bytes at offset 0
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+qemu-img: Could not open 'TEST_DIR/t.qcow2': Could not read snapshots: File too large
+Deleting snapshot 1 (test) l1_size=0x10000000: L1 table is too large; snapshot table entry corrupted
+Leaked cluster 4 refcount=2 reference=1
+Leaked cluster 5 refcount=2 reference=1
+Leaked cluster 6 refcount=1 reference=0
+Repairing cluster 4 refcount=2 reference=1
+Repairing cluster 5 refcount=2 reference=1
+Repairing cluster 6 refcount=1 reference=0
+Repairing OFLAG_COPIED L2 cluster: l1_index=0 l1_entry=40000 refcount=1
+Repairing OFLAG_COPIED data cluster: l2_entry=50000 refcount=1
+The following inconsistencies were found and repaired:
+
+    3 leaked clusters
+    3 corruptions
+
+Double checking the fixed image now...
+No errors were found on the image.
 *** done
-- 
2.11.0

Re: [Qemu-devel] [RFC PATCH 1/2] qcow2: Allow checking and repairing corrupted internal snapshots

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 5939 bytes --]

On 2018-02-15 17:30, Alberto Garcia wrote:
> The L1 table parameters of internal snapshots are generally not
> checked by QEMU. This patch allows 'qemu-img check' to detect broken
> snapshots and to skip them when doing the refcount consistency check.
> 
> Since without an L1 table we don't have a reliable way to recover the
> data from the snapshot, when 'qemu-img check' runs in repair mode this
> patch simply removes the corrupted snapshots.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  block/qcow2-snapshot.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  block/qcow2.c          |  7 ++++++-
>  block/qcow2.h          |  2 ++
>  3 files changed, 61 insertions(+), 1 deletion(-)

I think shouldn't delete things in qemu-img check.  I think we do need a
new mode (-r lossy? -r destructive?), although I'd personally even
prefer indeed asking the user before every destructive change.  The only
reason I'm not strongly in favor of this is because we don't have an
infrastructure for that (yet).

> diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
> index cee25f582b..7a36073e3e 100644
> --- a/block/qcow2-snapshot.c
> +++ b/block/qcow2-snapshot.c
> @@ -736,3 +736,56 @@ int qcow2_snapshot_load_tmp(BlockDriverState *bs,
>  
>      return 0;
>  }
> +
> +/* Check the snapshot table and optionally delete the corrupted entries */
> +int qcow2_snapshot_table_check(BlockDriverState *bs, BdrvCheckResult *result,
> +                               BdrvCheckMode fix)
> +{
> +    BDRVQcow2State *s = bs->opaque;
> +    bool keep_checking;
> +    int ret, i;
> +
> +    do {
> +        keep_checking = false;
> +
> +        for (i = 0; i < s->nb_snapshots; i++) {> +            QCowSnapshot *sn = s->snapshots + i;
> +            bool found_corruption = false;
> +
> +            if (offset_into_cluster(s, sn->l1_table_offset)) {
> +                fprintf(stderr, "%s snapshot %s (%s) l1_offset=%#" PRIx64 ": "
> +                        "L1 table is not cluster aligned; snapshot table entry "
> +                        "corrupted\n",
> +                        (fix & BDRV_FIX_ERRORS) ? "Deleting" : "ERROR",
> +                        sn->id_str, sn->name, sn->l1_table_offset);
> +                found_corruption = true;
> +            } else if (sn->l1_size > QCOW_MAX_L1_SIZE / sizeof(uint64_t)) {
> +                fprintf(stderr, "%s snapshot %s (%s) l1_size=%#" PRIx32 ": "
> +                        "L1 table is too large; snapshot table entry corrupted\n",
> +                        (fix & BDRV_FIX_ERRORS) ? "Deleting" : "ERROR",
> +                        sn->id_str, sn->name, sn->l1_size);
> +                found_corruption = true;
> +            }

This code assumes the snapshot table itself has been valid.  Why should
it be when it contains garbage entries?

> +
> +            if (found_corruption) {
> +                result->corruptions++;
> +                sn->l1_size = 0; /* Prevent this L1 table from being used */
> +                if (fix & BDRV_FIX_ERRORS) {
> +                    ret = qcow2_snapshot_delete(bs, sn->id_str, sn->name, NULL);

So calling this is actually very dangerous.  It modifies the snapshot
table which I wouldn't trust is actually just a snapshot table.  It
could intersect any other structure in the qcow2 image.  Yes, we do an
overlap check, but that only protects metadata, and I don't really want
to see an overlap check corruption when repairing the image; especially
since this means you cannot fix the corruption.

I don't quite know myself what to do instead, but I guess my main point
would be:  Before any (potentially) destructive changes are made, the
user should have the chance of still opening the image read-only and
copying all the data off somewhere else.  Which of course again means we
shouldn't prevent the user from opening an image because a snapshot is
broken.

(This would at least allow the user to convert the image to raw, then
invoke -r destructive, and then compare the result to see whether
anything has visibly changed.)

Max

> +                    if (ret < 0) {
> +                        return ret;
> +                    }
> +                    result->corruptions_fixed++;
> +                    /* If we modified the snapshot table we can't keep
> +                     * iterating. We have to start again from the
> +                     * beginning instead. */
> +                    keep_checking = true;
> +                    break;
> +                }
> +            }
> +        }
> +
> +    } while (keep_checking);
> +
> +    return 0;
> +}
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 2c6c33b67c..20e16ea602 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -546,7 +546,12 @@ int qcow2_mark_consistent(BlockDriverState *bs)
>  static int qcow2_check(BlockDriverState *bs, BdrvCheckResult *result,
>                         BdrvCheckMode fix)
>  {
> -    int ret = qcow2_check_refcounts(bs, result, fix);
> +    int ret = qcow2_snapshot_table_check(bs, result, fix);
> +    if (ret < 0) {
> +        return ret;
> +    }
> +
> +    ret = qcow2_check_refcounts(bs, result, fix);
>      if (ret < 0) {
>          return ret;
>      }
> diff --git a/block/qcow2.h b/block/qcow2.h
> index 1a84cc77b0..19f14bfc1e 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -637,6 +637,8 @@ int qcow2_snapshot_load_tmp(BlockDriverState *bs,
>                              const char *snapshot_id,
>                              const char *name,
>                              Error **errp);
> +int qcow2_snapshot_table_check(BlockDriverState *bs, BdrvCheckResult *result,
> +                               BdrvCheckMode fix);
>  
>  void qcow2_free_snapshots(BlockDriverState *bs);
>  int qcow2_read_snapshots(BlockDriverState *bs);
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: [Qemu-devel] [RFC PATCH 1/2] qcow2: Allow checking and repairing corrupted internal snapshots

[Alberto Garcia]

On Mon 26 Feb 2018 02:40:08 PM CET, Max Reitz wrote:
>> +                        "L1 table is too large; snapshot table entry corrupted\n",
>> +                        (fix & BDRV_FIX_ERRORS) ? "Deleting" : "ERROR",
>> +                        sn->id_str, sn->name, sn->l1_size);
>> +                found_corruption = true;
>> +            }
>
> This code assumes the snapshot table itself has been valid.  Why should
> it be when it contains garbage entries?
>
>> +
>> +            if (found_corruption) {
>> +                result->corruptions++;
>> +                sn->l1_size = 0; /* Prevent this L1 table from being used */
>> +                if (fix & BDRV_FIX_ERRORS) {
>> +                    ret = qcow2_snapshot_delete(bs, sn->id_str, sn->name, NULL);
>
> So calling this is actually very dangerous.  It modifies the snapshot
> table which I wouldn't trust is actually just a snapshot table.  It
> could intersect any other structure in the qcow2 image.  Yes, we do an
> overlap check, but that only protects metadata, and I don't really
> want to see an overlap check corruption when repairing the image;
> especially since this means you cannot fix the corruption.

I think you're right. One thing that could be done is check that the
area where the snapshot table is supposed to be doesn't overlap with any
other data structures.

> I don't quite know myself what to do instead, but I guess my main
> point would be: Before any (potentially) destructive changes are made,
> the user should have the chance of still opening the image read-only
> and copying all the data off somewhere else.  Which of course again
> means we shouldn't prevent the user from opening an image because a
> snapshot is broken.

Ok, you've convinced me. I'll see what I can come up with. I guess
initially we can simply add checks for sn->l1_table_offset and
sn->l1_size in the places where they're used.

Berto