diff mbox series

[for-4.2,08/13] qcow2: Fix broken snapshot table entries

Message ID 20190730172508.19911-9-mreitz@redhat.com (mailing list archive)
State New, archived
Headers show
Series qcow2: Let check -r all repair some snapshot bits | expand

Commit Message

Max Reitz July 30, 2019, 5:25 p.m. UTC
The only case where we currently reject snapshot table entries is when
they have too much extra data.  Fix them with qemu-img check -r all by
counting it as a corruption, reducing their extra_data_size, and then
letting qcow2_check_fix_snapshot_table() do the rest.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/qcow2-snapshot.c | 69 ++++++++++++++++++++++++++++++++++--------
 1 file changed, 56 insertions(+), 13 deletions(-)

Comments

Eric Blake July 30, 2019, 7:02 p.m. UTC | #1
On 7/30/19 12:25 PM, Max Reitz wrote:
> The only case where we currently reject snapshot table entries is when
> they have too much extra data.  Fix them with qemu-img check -r all by
> counting it as a corruption, reducing their extra_data_size, and then
> letting qcow2_check_fix_snapshot_table() do the rest.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/qcow2-snapshot.c | 69 ++++++++++++++++++++++++++++++++++--------
>  1 file changed, 56 insertions(+), 13 deletions(-)
> 

> @@ -112,16 +141,22 @@ int qcow2_read_snapshots(BlockDriverState *bs, Error **errp)
>          }
>  
>          if (sn->extra_data_size > sizeof(extra)) {
> -            /* Store unknown extra data */
>              size_t unknown_extra_data_size =
>                  sn->extra_data_size - sizeof(extra);
>  
> -            sn->unknown_extra_data = g_malloc(unknown_extra_data_size);
> -            ret = bdrv_pread(bs->file, offset, sn->unknown_extra_data,
> -                             unknown_extra_data_size);
> -            if (ret < 0) {
> -                error_setg_errno(errp, -ret, "Failed to read snapshot table");
> -                goto fail;
> +            if (discard_unknown_extra_data) {
> +                /* Discard unknown extra data */
> +                sn->extra_data_size = sizeof(extra);

This truncates it down to just the data we know. Should it instead
truncate down to the 1024 bytes of QCOW_MAX_SNAPSHOT_EXTRA_DATA defined
in 2/13?  (We can't keep all of the user's extra stuff, but we can at
least try to preserve as much as possible)

Otherwise, looks good.
Reviewed-by: Eric Blake <eblake@redhat.com>
Max Reitz July 31, 2019, 9:06 a.m. UTC | #2
On 30.07.19 21:02, Eric Blake wrote:
> On 7/30/19 12:25 PM, Max Reitz wrote:
>> The only case where we currently reject snapshot table entries is when
>> they have too much extra data.  Fix them with qemu-img check -r all by
>> counting it as a corruption, reducing their extra_data_size, and then
>> letting qcow2_check_fix_snapshot_table() do the rest.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  block/qcow2-snapshot.c | 69 ++++++++++++++++++++++++++++++++++--------
>>  1 file changed, 56 insertions(+), 13 deletions(-)
>>
> 
>> @@ -112,16 +141,22 @@ int qcow2_read_snapshots(BlockDriverState *bs, Error **errp)
>>          }
>>  
>>          if (sn->extra_data_size > sizeof(extra)) {
>> -            /* Store unknown extra data */
>>              size_t unknown_extra_data_size =
>>                  sn->extra_data_size - sizeof(extra);
>>  
>> -            sn->unknown_extra_data = g_malloc(unknown_extra_data_size);
>> -            ret = bdrv_pread(bs->file, offset, sn->unknown_extra_data,
>> -                             unknown_extra_data_size);
>> -            if (ret < 0) {
>> -                error_setg_errno(errp, -ret, "Failed to read snapshot table");
>> -                goto fail;
>> +            if (discard_unknown_extra_data) {
>> +                /* Discard unknown extra data */
>> +                sn->extra_data_size = sizeof(extra);
> 
> This truncates it down to just the data we know. Should it instead
> truncate down to the 1024 bytes of QCOW_MAX_SNAPSHOT_EXTRA_DATA defined
> in 2/13?  (We can't keep all of the user's extra stuff, but we can at
> least try to preserve as much as possible)

On one hand, potentially cutting unknown data in half sounds like not
such a good idea to me.

On the other, a field can only be considered present if it is fully
present.  So cutting any optional data in half shouldn’t have any
negative impact.

So, yes, truncating it down to 1024 bytes sounds good.

Max

> Otherwise, looks good.
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
diff mbox series

Patch

diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 58609b6ea4..9956c32964 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -44,7 +44,23 @@  void qcow2_free_snapshots(BlockDriverState *bs)
     s->nb_snapshots = 0;
 }
 
-int qcow2_read_snapshots(BlockDriverState *bs, Error **errp)
+/*
+ * If @repair is true, try to repair a broken snapshot table instead
+ * of just returning an error:
+ *
+ * - If there were snapshots with too much extra metadata, increment
+ *   *extra_data_dropped for each.
+ *   This requires the caller to eventually rewrite the whole snapshot
+ *   table, which requires cluster allocation.  Therefore, this should
+ *   be done only after qcow2_check_refcounts() made sure the refcount
+ *   structures are valid.
+ *   (In the meantime, the image is still valid because
+ *   qcow2_check_refcounts() does not do anything with snapshots'
+ *   extra data.)
+ */
+static int qcow2_do_read_snapshots(BlockDriverState *bs, bool repair,
+                                   int *extra_data_dropped,
+                                   Error **errp)
 {
     BDRVQcow2State *s = bs->opaque;
     QCowSnapshotHeader h;
@@ -64,6 +80,8 @@  int qcow2_read_snapshots(BlockDriverState *bs, Error **errp)
     s->snapshots = g_new0(QCowSnapshot, s->nb_snapshots);
 
     for(i = 0; i < s->nb_snapshots; i++) {
+        bool discard_unknown_extra_data = false;
+
         /* Read statically sized part of the snapshot header */
         offset = ROUND_UP(offset, 8);
         ret = bdrv_pread(bs->file, offset, &h, sizeof(h));
@@ -86,10 +104,21 @@  int qcow2_read_snapshots(BlockDriverState *bs, Error **errp)
         name_size = be16_to_cpu(h.name_size);
 
         if (sn->extra_data_size > QCOW_MAX_SNAPSHOT_EXTRA_DATA) {
-            ret = -EFBIG;
-            error_setg(errp, "Too much extra metadata in snapshot table "
-                       "entry %i", i);
-            goto fail;
+            if (!repair) {
+                ret = -EFBIG;
+                error_setg(errp, "Too much extra metadata in snapshot table "
+                           "entry %i", i);
+                error_append_hint(errp, "You can force-remove this extra "
+                                  "metadata with qemu-img check -r all\n");
+                goto fail;
+            }
+
+            fprintf(stderr, "Discarding too much extra metadata in snapshot "
+                    "table entry %i (%" PRIu32 " > %u)\n",
+                    i, sn->extra_data_size, QCOW_MAX_SNAPSHOT_EXTRA_DATA);
+
+            (*extra_data_dropped)++;
+            discard_unknown_extra_data = true;
         }
 
         /* Read known extra data */
@@ -112,16 +141,22 @@  int qcow2_read_snapshots(BlockDriverState *bs, Error **errp)
         }
 
         if (sn->extra_data_size > sizeof(extra)) {
-            /* Store unknown extra data */
             size_t unknown_extra_data_size =
                 sn->extra_data_size - sizeof(extra);
 
-            sn->unknown_extra_data = g_malloc(unknown_extra_data_size);
-            ret = bdrv_pread(bs->file, offset, sn->unknown_extra_data,
-                             unknown_extra_data_size);
-            if (ret < 0) {
-                error_setg_errno(errp, -ret, "Failed to read snapshot table");
-                goto fail;
+            if (discard_unknown_extra_data) {
+                /* Discard unknown extra data */
+                sn->extra_data_size = sizeof(extra);
+            } else {
+                /* Store unknown extra data */
+                sn->unknown_extra_data = g_malloc(unknown_extra_data_size);
+                ret = bdrv_pread(bs->file, offset, sn->unknown_extra_data,
+                                 unknown_extra_data_size);
+                if (ret < 0) {
+                    error_setg_errno(errp, -ret,
+                                     "Failed to read snapshot table");
+                    goto fail;
+                }
             }
             offset += unknown_extra_data_size;
         }
@@ -162,6 +197,11 @@  fail:
     return ret;
 }
 
+int qcow2_read_snapshots(BlockDriverState *bs, Error **errp)
+{
+    return qcow2_do_read_snapshots(bs, false, NULL, errp);
+}
+
 /* add at the end of the file a new list of snapshots */
 int qcow2_write_snapshots(BlockDriverState *bs)
 {
@@ -327,6 +367,7 @@  int coroutine_fn qcow2_check_read_snapshot_table(BlockDriverState *bs,
 {
     BDRVQcow2State *s = bs->opaque;
     Error *local_err = NULL;
+    int extra_data_dropped = 0;
     int ret;
     struct {
         uint32_t nb_snapshots;
@@ -362,7 +403,8 @@  int coroutine_fn qcow2_check_read_snapshot_table(BlockDriverState *bs,
     }
 
     qemu_co_mutex_unlock(&s->lock);
-    ret = qcow2_read_snapshots(bs, &local_err);
+    ret = qcow2_do_read_snapshots(bs, fix & BDRV_FIX_ERRORS,
+                                  &extra_data_dropped, &local_err);
     qemu_co_mutex_lock(&s->lock);
     if (ret < 0) {
         result->check_errors++;
@@ -375,6 +417,7 @@  int coroutine_fn qcow2_check_read_snapshot_table(BlockDriverState *bs,
 
         return ret;
     }
+    result->corruptions += extra_data_dropped;
 
     return 0;
 }