Skip to contentSkip to conversation

qcow2: fix image corruption after committing qcow2 image into base

PATCH Superseded Daniel P. Berrange 5 participants 8 messages Nov 3, 2017 – Nov 17, 2017 Block layer

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Daniel P. Berrange

author
#0 lore raw

PATCH 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) codepaths leaves the LUKS header intact, so force use of that codepath.

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

---

NB, ideally we would fix the faster codepath in make_completely_empty, but having looked at the code, I've really no idea how to even start on fixing that to not kill the LUKS header clusters.

 block/qcow2.c                    |   3 +-
 tests/qemu-iotests/198           | 104 +++++++++++++++++++++++++++++++
 tests/qemu-iotests/198.out       | 128 +++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/common.filter |   4 +-
 tests/qemu-iotests/group         |   1 +
 5 files changed, 238 insertions(+), 2 deletions(-)
 create mode 100755 tests/qemu-iotests/198
 create mode 100644 tests/qemu-iotests/198.out
block/qcow2.c +2 -1
index 8edf8ac3c7..618aab114a 100644
@@ -3594,7 +3594,8 @@ 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
tests/qemu-iotests/198 new file +104 -0

104 lines not on this page · raw message

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

128 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.13.6
Reviewed-by on v1 1/1
#1 lore raw
Quoted 6 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) codepaths leaves

s/codepaths/codepath/

Quoted 12 lines

the LUKS header intact, so force use of that codepath.

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

---

NB, ideally we would fix the faster codepath in make_completely_empty, but having looked at the code, I've really no idea how to even start on fixing that to not kill the LUKS header clusters.

+++ b/block/qcow2.c

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

Quoted 9 lines
block/qcow2.c +2 -1
@@ -3594,7 +3594,8 @@ 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

Worth updating the comment to explain why we can't use the fast path with LUKS encryption?

But that's minor enough that with or without a comment tweak, I'm fine with:

Reviewed-by Eric Blake <[email protected]>
Signature · 3 lines
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org
#2 lore raw
In reply to Daniel P. Berrange

Am 03.11.2017 um 15:41 hat Daniel P. Berrange geschrieben:

Quoted 15 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) codepaths leaves the LUKS header intact, so force use of that codepath.

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

---

NB, ideally we would fix the faster codepath in make_completely_empty, but having looked at the code, I've really no idea how to even start on fixing that to not kill the LUKS header clusters.

Quoted 8 lines
 block/qcow2.c                    |   3 +-
 tests/qemu-iotests/198           | 104 +++++++++++++++++++++++++++++++
 tests/qemu-iotests/198.out       | 128 +++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/common.filter |   4 +-
 tests/qemu-iotests/group         |   1 +
 5 files changed, 238 insertions(+), 2 deletions(-)
 create mode 100755 tests/qemu-iotests/198
 create mode 100644 tests/qemu-iotests/198.out

The test fails for me. Looks like we need some filtering.

Kevin

home/kwolf/source/qemu/tests/qemu-iotests/198.out.bad 2017-11-10 17:26:54.914932949 +0100 renamed from home/kwolf/source/qemu/tests/qemu-iotests/198.out 2017-11-10 17:23:54.484003430 +0100 +2 -2
@@ -35,7 +35,7 @@
 image: json:{"encrypt.key-secret": "sec0", "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.base"}}
 file format: IMGFMT
 virtual size: 16M (16777216 bytes)
-disk size: 17M
+disk size: 33M
 Format specific information:
     compat: 1.1
     lazy refcounts: false

@@ -82,7 +82,7 @@
 image: json:{"encrypt.key-secret": "sec1", "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT"}}
 file format: IMGFMT
 virtual size: 16M (16777216 bytes)
-disk size: 548K
+disk size: 17M
 backing file: TEST_DIR/t.IMGFMT.base
 Format specific information:
     compat: 1.1

Failures: 198
Failed 1 of 1 tests

#3 lore raw
In reply to Daniel P. Berrange
Quoted 8 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) codepaths leaves the LUKS header intact, so force use of that codepath.

Quoted 6 lines
Signed-off-by Daniel P. Berrange <[email protected]>

---

NB, ideally we would fix the faster codepath in make_completely_empty, but having looked at the code, I've really no idea how to even start on fixing that to not kill the LUKS header clusters.

Hmm - I wonder if persistent bitmaps are also corrupted in the fast path.

Signature · 3 lines
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org
#4 lore raw
Quoted 9 lines · Eric Blake
Quoted 8 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) codepaths leaves the LUKS header intact, so force use of that codepath.

Quoted 8 lines
Quoted 6 lines
Signed-off-by Daniel P. Berrange <[email protected]>

---

NB, ideally we would fix the faster codepath in make_completely_empty, but having looked at the code, I've really no idea how to even start on fixing that to not kill the LUKS header clusters.

Hmm - I wonder if persistent bitmaps are also corrupted in the fast path.

Oops.
If I understand correct fast path results in absolutely empty image, so bitmaps will be dropped of course. So, s->nb_bitmaps == 0 should be checked too. You can squash it or I can send separate patch. Don't think that special test is needed for bitmaps in this case.

Signature · 2 lines
Best regards,
Vladimir
author
#5 lore raw
In reply to Eric Blake
Quoted 9 lines · Eric Blake
Quoted 8 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) codepaths leaves the LUKS header intact, so force use of that codepath.

Quoted 8 lines
Quoted 6 lines
Signed-off-by Daniel P. Berrange <[email protected]>

---

NB, ideally we would fix the faster codepath in make_completely_empty, but having looked at the code, I've really no idea how to even start on fixing that to not kill the LUKS header clusters.

Hmm - I wonder if persistent bitmaps are also corrupted in the fast path.

I also wonder if there's anything better we can do to make us safer by default, so we default to the slow & safe path, unless we can provide we *only* have the subset of features that are safe for the fast path ?

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 :|
#6 lore raw
Quoted 17 lines · Daniel P. Berrange
Quoted 16 lines · Eric Blake
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) codepaths leaves the LUKS header intact, so force use of that codepath.

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

---

NB, ideally we would fix the faster codepath in make_completely_empty, but having looked at the code, I've really no idea how to even start on fixing that to not kill the LUKS header clusters.

Quoted 5 lines
Quoted 1 line

Hmm - I wonder if persistent bitmaps are also corrupted in the fast path.

I also wonder if there's anything better we can do to make us safer by default, so we default to the slow & safe path, unless we can provide we *only* have the subset of features that are safe for the fast path ?

I have wondered the same but I can't think of any. The only thing that comes close would be to check for which header extensions there are; but at the same time, we could just add a comment to qcow2_read_extensions() ("If you add a new feature to qcow2, note that you may want to adjust the qcow2_make_empty() fastpath conditions").

Max

#7 lore raw
Quoted 11 lines · Max Reitz
Quoted 5 lines
Quoted 1 line

Hmm - I wonder if persistent bitmaps are also corrupted in the fast path.

I also wonder if there's anything better we can do to make us safer by default, so we default to the slow & safe path, unless we can provide we *only* have the subset of features that are safe for the fast path ?

I have wondered the same but I can't think of any. The only thing that comes close would be to check for which header extensions there are; but at the same time, we could just add a comment to qcow2_read_extensions() ("If you add a new feature to qcow2, note that you may want to adjust the qcow2_make_empty() fastpath conditions").

Indeed, such a comment may be helpful.

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