Skip to contentSkip to conversation

qcow2: fix image corruption after committing qcow2 image into base

PATCH Applied Daniel P. Berrange 3 participants 5 messages Nov 17, 2017 Block layer

lore
Share & tools
lore

Daniel P. Berrange

author
#0 lore raw

PATCH v2 1/1 qcow2: fix image corruption after committing qcow2 image into base

After committing the qcow2 image contents into the base image, qemu-img will call bdrv_make_empty to drop the payload in the layered image.

When this is done for qcow2 images, it blows away the LUKS encryption header, making the resulting image unusable. There are two codepaths for emptying a qcow2 image, and the second (slower) codepath leaves the LUKS header intact, so force use of that codepath.

Reviewed-by Eric Blake <[email protected]>
Signed-off-by Daniel P. Berrange <[email protected]>
---
 block/qcow2.c                    |   6 +-
 tests/qemu-iotests/198           | 104 ++++++++++++++++++++++++++++++++
 tests/qemu-iotests/198.out       | 126 +++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/common.filter |   4 +-
 tests/qemu-iotests/group         |   1 +
 5 files changed, 238 insertions(+), 3 deletions(-)
 create mode 100755 tests/qemu-iotests/198
 create mode 100644 tests/qemu-iotests/198.out
block/qcow2.c +4 -2
index 92e5d548e3..e9a86b7443 100644
@@ -3601,12 +3601,14 @@ static int qcow2_make_empty(BlockDriverState *bs)
     l1_clusters = DIV_ROUND_UP(s->l1_size, s->cluster_size / sizeof(uint64_t));
 
     if (s->qcow_version >= 3 && !s->snapshots &&
-        3 + l1_clusters <= s->refcount_block_size) {
+        3 + l1_clusters <= s->refcount_block_size &&
+        s->crypt_method_header != QCOW_CRYPT_LUKS) {
         /* The following function only works for qcow2 v3 images (it requires
          * the dirty flag) and only as long as there are no snapshots (because
          * it completely empties the image). Furthermore, the L1 table and three
          * additional clusters (image header, refcount table, one refcount
-         * block) have to fit inside one refcount block. */
+         * block) have to fit inside one refcount block. It cannot be used
+         * for LUKS (yet) as it throws away the LUKS header cluster(s) */
         return make_completely_empty(bs);
     }
tests/qemu-iotests/198 new file +104 -0

104 lines not on this page · raw message

tests/qemu-iotests/198.out new file +126 -0

126 lines not on this page · raw message

tests/qemu-iotests/common.filter +3 -1
index 873ca6b104..d9237799e9 100644
@@ -157,7 +157,9 @@ _filter_img_info()
         -e "/lazy_refcounts: \\(on\\|off\\)/d" \
         -e "/block_size: [0-9]\\+/d" \
         -e "/block_state_zero: \\(on\\|off\\)/d" \
-        -e "/log_size: [0-9]\\+/d"
+        -e "/log_size: [0-9]\\+/d" \
+        -e "s/iters: [0-9]\\+/iters: 1024/" \
+        -e "s/uuid: [-a-f0-9]\\+/uuid: 00000000-0000-0000-0000-000000000000/"
 }
 
 # filter out offsets and file names from qemu-img map; good for both
tests/qemu-iotests/group +1 -0
index 24e5ad1b79..83b839bbe3 100644
@@ -194,3 +194,4 @@
 194 rw auto migration quick
 195 rw auto quick
 197 rw auto quick
+198 rw auto
Signature · 1 line
2.14.3
Applied
#1 lore raw

Am 17.11.2017 um 12:29 hat Daniel P. Berrange geschrieben:

Quoted 10 lines

After committing the qcow2 image contents into the base image, qemu-img will call bdrv_make_empty to drop the payload in the layered image.

When this is done for qcow2 images, it blows away the LUKS encryption header, making the resulting image unusable. There are two codepaths for emptying a qcow2 image, and the second (slower) codepath leaves the LUKS header intact, so force use of that codepath.

Reviewed-by Eric Blake <[email protected]>
Signed-off-by Daniel P. Berrange <[email protected]>

Thanks, applied to the block branch.

Kevin

#2 lore raw
In reply to Daniel P. Berrange
Quoted 14 lines · Daniel P. Berrange

After committing the qcow2 image contents into the base image, qemu-img will call bdrv_make_empty to drop the payload in the layered image.

When this is done for qcow2 images, it blows away the LUKS encryption header, making the resulting image unusable. There are two codepaths for emptying a qcow2 image, and the second (slower) codepath leaves the LUKS header intact, so force use of that codepath.

Reviewed-by Eric Blake <[email protected]>
Signed-off-by Daniel P. Berrange <[email protected]>

---

     if (s->qcow_version >= 3 && !s->snapshots &&

block/qcow2.c static int qcow2_make_empty(BlockDriverState *bs) · line 3605

Quoted 3 lines
block/qcow2.c +2 -1
-        3 + l1_clusters <= s->refcount_block_size) {
+        3 + l1_clusters <= s->refcount_block_size &&
+        s->crypt_method_header != QCOW_CRYPT_LUKS) {

Missing the check for s->nb_bitmaps == 0 that Vladimir mentioned on v1. Either we can add that in this patch, or I can post the obvious followup that also needs queuing.

Signature · 3 lines
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org
#3 lore raw

Am 17.11.2017 um 15:32 hat Eric Blake geschrieben:

Quoted 14 lines · Daniel P. Berrange
Quoted 13 lines

After committing the qcow2 image contents into the base image, qemu-img will call bdrv_make_empty to drop the payload in the layered image.

When this is done for qcow2 images, it blows away the LUKS encryption header, making the resulting image unusable. There are two codepaths for emptying a qcow2 image, and the second (slower) codepath leaves the LUKS header intact, so force use of that codepath.

Reviewed-by Eric Blake <[email protected]>
Signed-off-by Daniel P. Berrange <[email protected]>

---

Quoted 8 lines

block/qcow2.c static int qcow2_make_empty(BlockDriverState *bs) · line 3605

Quoted 4 lines
     if (s->qcow_version >= 3 && !s->snapshots &&
block/qcow2.c +2 -1
-        3 + l1_clusters <= s->refcount_block_size) {
+        3 + l1_clusters <= s->refcount_block_size &&
+        s->crypt_method_header != QCOW_CRYPT_LUKS) {

Missing the check for s->nb_bitmaps == 0 that Vladimir mentioned on v1. Either we can add that in this patch, or I can post the obvious followup that also needs queuing.

I've already queued this, so please just send a follow-up patch.

Kevin

author
#4 lore raw
In reply to Eric Blake
Quoted 14 lines · Eric Blake
Quoted 13 lines · Daniel P. Berrange

After committing the qcow2 image contents into the base image, qemu-img will call bdrv_make_empty to drop the payload in the layered image.

When this is done for qcow2 images, it blows away the LUKS encryption header, making the resulting image unusable. There are two codepaths for emptying a qcow2 image, and the second (slower) codepath leaves the LUKS header intact, so force use of that codepath.

Reviewed-by Eric Blake <[email protected]>
Signed-off-by Daniel P. Berrange <[email protected]>

---

Quoted 8 lines

block/qcow2.c static int qcow2_make_empty(BlockDriverState *bs) · line 3605

Quoted 4 lines
     if (s->qcow_version >= 3 && !s->snapshots &&
block/qcow2.c +2 -1
-        3 + l1_clusters <= s->refcount_block_size) {
+        3 + l1_clusters <= s->refcount_block_size &&
+        s->crypt_method_header != QCOW_CRYPT_LUKS) {

Missing the check for s->nb_bitmaps == 0 that Vladimir mentioned on v1. Either we can add that in this patch, or I can post the obvious followup that also needs queuing.

That's unrelated to LUKS though, so best as a separate patch, which adds a I/O test case that validates that feature.

Regards,
Daniel

Signature · 3 lines
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|