[PATCH 0/2] qcow2: silent corruption when a dirty image becomes writable

[PATCH 0/2] qcow2: silent corruption when a dirty image becomes writable

[Denis V. Lunev]

Today I have faced real data corrupt from our customer with the
situation very close to the one addressed in the patch
"qcow2: do not try to clear the dirty bit on a read-only node" and
that is interesting. I was really unsure that design is correct
but with today case I can say that correct thing was done.

The problem
-----------

qcow2_do_open() repairs an image carrying QCOW2_INCOMPAT_DIRTY, but only
for a node that is writable from the start:

    if (!(flags & BDRV_O_CHECK) && bdrv_is_writable(bs) &&
        (s->incompatible_features & QCOW2_INCOMPAT_DIRTY)) {

A node opened read-only skips it, correctly, since it resolves nothing
and writes nothing. Nothing revisits the question when that same node
later becomes writable, and bdrv_reopen() is not an exotic way to get
there: commit_active_start() and commit_start() both reopen the base
read-write for the duration of the job, so an ordinary block-commit onto
a read-only backing file is enough.

With lazy refcounts the refcount blocks are not written again once the
dirty bit is set, so an image left behind by a killed QEMU has an
on-disk refcount block that accounts for the metadata clusters and
nothing else. Every data cluster reads as free. s->free_cluster_index
starts at 0, so the first allocation after such a reopen starts at the
front of the image and hands out clusters that L2 entries still point
at.

The result is aliasing: two guest offsets mapped onto one host cluster,
so the guest reads back data belonging to some other offset. Nothing
about this fails. No I/O error is reported, the corrupt bit stays clear,
and a clean close clears the dirty bit as well, so no later open will
repair the image either. Afterwards qemu-img check reports

    ERROR cluster N refcount=1 reference=2
    ERROR cluster N refcount=0 reference=1
    ERROR OFLAG_COPIED data cluster: l2_entry=<host>|COPIED refcount=0

and the only runtime witness, if something eventually frees one of those
clusters, is a bare

    qcow2_free_clusters failed: Invalid argument

on stderr, with the guest none the wiser.

How it looked in production
---------------------------

A VM was killed while its storage was unavailable, leaving a 100 GiB
volume dirty. It came back with a snapshot-revert overlay on top, so the
volume itself was now the read-only backing file, and the overlay was
committed into it two and a half hours later. 8082 host clusters ended
up referenced by two L2 entries each, roughly 8 GiB of guest data
cross-mapped. The guest filesystem began failing metadata verification
on buffers holding fragments of unrelated files.

Two properties of the damage are worth recording, because they are what
told us this was not a race:

 - aliasing is exactly two-way, never three or more, which is a single
   monotonic sweep of the allocator rather than a window hit repeatedly;

 - it stops dead at the host cluster that was the image end at the
   moment the volume was made writable. Everything allocated after that
   point is intact.

qemu-img check -r all makes the metadata self-consistent again, and then
honestly reports no errors, but it cannot un-alias anything. The guest
data stays wrong.

Reproducer
----------

Under a second, no guest and no block job required:

  qemu-img create -f qcow2 -o compat=1.1,lazy_refcounts=on base.qcow2 1G
  qemu-io -f qcow2 -c "write -P 0xaa 0 100M" -c flush \
          -c "sigraise 9" base.qcow2

  qemu-io -r -f qcow2 base.qcow2 \
      <<< $'reopen -w\nwrite -P 0xbb 900M 100M\nquit'

  qemu-io -r -f qcow2 -c "read -v 0 16" base.qcow2
  # 00000000:  bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb

The flush is load-bearing: it writes out the L2 cache but not the
refcount blocks, which is exactly the asymmetry the bug needs. Guest
fsyncs supply it in production, so a long-running VM is the ideal
victim. qemu-img commit of an overlay reaches the same state through
commit_active_start().

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Hanna Reitz <hreitz@redhat.com>

Denis V. Lunev (2):
  qcow2: do not clear the dirty bit when reopening a read-only node
  qcow2: repair a dirty image when it becomes writable

 block/qcow2.c              | 25 ++++++++++++++++++++++---
 tests/qemu-iotests/039     | 35 +++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/039.out | 22 ++++++++++++++++++++++
 3 files changed, 79 insertions(+), 3 deletions(-)

-- 
2.53.0

[PATCH 1/2] qcow2: do not clear the dirty bit when reopening a read-only node

[Denis V. Lunev]

qcow2_reopen_prepare() clears the dirty bit whenever the node is
reopened read-only, with an unguarded header write. A read-only node
can still be dirty, inherited from an earlier writable session, and it
holds no BLK_PERM_WRITE to resolve that. A read-only to read-only
reopen of a dirty image therefore fails outright:

  $ qemu-io -r -f qcow2 dirty.qcow2 <<< $'reopen -r\nquit'
  qemu-io: failed while preparing to reopen image 'dirty.qcow2'

Clear it only for a node that is writable now, the predicate
qcow2_do_open() already uses for the repair. bdrv_is_writable() also
excludes an inactive node, whose header must not be touched either.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Hanna Reitz <hreitz@redhat.com>
---
 block/qcow2.c              |  8 +++++---
 tests/qemu-iotests/039     | 11 +++++++++++
 tests/qemu-iotests/039.out |  5 +++++
 3 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 7292dd036c..1543255eba 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2102,9 +2102,11 @@ qcow2_reopen_prepare(BDRVReopenState *state,BlockReopenQueue *queue,
             goto fail;
         }
 
-        ret = qcow2_mark_clean(state->bs);
-        if (ret < 0) {
-            goto fail;
+        if (bdrv_is_writable(state->bs)) {
+            ret = qcow2_mark_clean(state->bs);
+            if (ret < 0) {
+                goto fail;
+            }
         }
     }
 
diff --git a/tests/qemu-iotests/039 b/tests/qemu-iotests/039
index 94a8bfe754..a5be81bc4a 100755
--- a/tests/qemu-iotests/039
+++ b/tests/qemu-iotests/039
@@ -95,6 +95,17 @@ $QEMU_IMG info --image-opts \
 # The dirty bit must still be set: this open never wrote any guest data
 _qcow2_dump_header | grep incompatible_features
 
+echo
+echo "== Read-only reopen must not clear the dirty bit =="
+
+# Reopening a read-only node must not try to write the QCOW2 header either,
+# and must leave the dirty bit for whoever ends up repairing the image.
+$QEMU_IO -r -c "reopen -r" -c "read -P 0x5a 0 512" "$TEST_IMG" \
+    | _filter_qemu_io
+
+# The dirty bit must still be set
+_qcow2_dump_header | grep incompatible_features
+
 echo
 echo "== Repairing the image file must succeed =="
 
diff --git a/tests/qemu-iotests/039.out b/tests/qemu-iotests/039.out
index c66361128f..3c71e5a3dd 100644
--- a/tests/qemu-iotests/039.out
+++ b/tests/qemu-iotests/039.out
@@ -27,6 +27,11 @@ incompatible_features     [0]
 == Read-only open must not crash on close ==
 incompatible_features     [0]
 
+== Read-only reopen must not clear the dirty bit ==
+read 512/512 bytes at offset 0
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+incompatible_features     [0]
+
 == Repairing the image file must succeed ==
 ERROR cluster 5 refcount=0 reference=1
 Rebuilding refcount structure
-- 
2.53.0

[PATCH 2/2] qcow2: repair a dirty image when it becomes writable

[Denis V. Lunev]

A dirty image must be repaired before anything allocates a cluster in
it. qcow2_do_open() does that, but only for a node that is writable
from the start. A node opened read-only skips it, and nothing revisits
the question once that node becomes writable, which block-commit does
routinely: commit_active_start() and commit_start() reopen the base
read-write for the duration of the job.

With lazy refcounts the on-disk refcount block then still accounts for
the metadata clusters only, so the allocator restarts at the front of
the image and hands out clusters that L2 entries point at. Two guest
offsets end up sharing one host cluster. Nothing fails, the corrupt bit
stays clear, and a clean close clears the dirty bit, so no later open
repairs the image either.

Do the repair in qcow2_reopen_commit_post(). bdrv_reopen_prepare() runs
before bdrv_list_refresh_perms(), so it holds no BLK_PERM_WRITE and,
with auto-read-only, bs->file may still have an O_RDONLY descriptor.
commit_post cannot reject the reopen, so signal corruption if the
repair fails rather than let writes alias live clusters. An inactive
node is skipped: bdrv_activate() calls qcow2_do_open() again through
qcow2_co_invalidate_cache().

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Hanna Reitz <hreitz@redhat.com>
---
 block/qcow2.c              | 14 ++++++++++++++
 tests/qemu-iotests/039     | 24 ++++++++++++++++++++++++
 tests/qemu-iotests/039.out | 17 +++++++++++++++++
 3 files changed, 55 insertions(+)

diff --git a/block/qcow2.c b/block/qcow2.c
index 1543255eba..e660655a0d 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2147,8 +2147,22 @@ static void qcow2_reopen_commit(BDRVReopenState *state)
 
 static void qcow2_reopen_commit_post(BDRVReopenState *state)
 {
+    BDRVQcow2State *s = state->bs->opaque;
+
     GRAPH_RDLOCK_GUARD_MAINLOOP();
 
+    if (bdrv_is_writable(state->bs) &&
+        (s->incompatible_features & QCOW2_INCOMPAT_DIRTY)) {
+        BdrvCheckResult result = {0};
+        int ret;
+
+        ret = bdrv_check(state->bs, &result, BDRV_FIX_ERRORS | BDRV_FIX_LEAKS);
+        if (ret < 0 || result.check_errors) {
+            qcow2_signal_corruption(state->bs, true, -1, -1,
+                                    "Could not repair dirty image");
+        }
+    }
+
     if (state->flags & BDRV_O_RDWR) {
         Error *local_err = NULL;
 
diff --git a/tests/qemu-iotests/039 b/tests/qemu-iotests/039
index a5be81bc4a..255dc2e7e5 100755
--- a/tests/qemu-iotests/039
+++ b/tests/qemu-iotests/039
@@ -137,6 +137,30 @@ $QEMU_IO -c "write 0 512" "$TEST_IMG" | _filter_qemu_io
 # The dirty bit must not be set
 _qcow2_dump_header | grep incompatible_features
 
+echo
+echo "== Reopening a dirty image read/write should repair it =="
+
+_make_test_img -o "compat=1.1,lazy_refcounts=on" $size
+
+_NO_VALGRIND \
+$QEMU_IO -c "write -P 0x5a 0 512" \
+         -c "sigraise $(kill -l KILL)" "$TEST_IMG" 2>&1 \
+    | _filter_qemu_io
+
+# The dirty bit must be set
+_qcow2_dump_header | grep incompatible_features
+
+# The refcounts are only resolved when the node becomes writable. Without
+# that, this write is allocated on top of the cluster at offset 0 and reading
+# it back returns the data written here.
+$QEMU_IO -r -c "reopen -w" \
+            -c "write -P 0xb1 1M 512" \
+            -c "read -P 0x5a 0 512" "$TEST_IMG" | _filter_qemu_io
+
+# The dirty bit must not be set
+_qcow2_dump_header | grep incompatible_features
+_check_test_img
+
 echo
 echo "== Creating an image file with lazy_refcounts=off =="
 
diff --git a/tests/qemu-iotests/039.out b/tests/qemu-iotests/039.out
index 3c71e5a3dd..62073916a9 100644
--- a/tests/qemu-iotests/039.out
+++ b/tests/qemu-iotests/039.out
@@ -64,6 +64,23 @@ wrote 512/512 bytes at offset 0
 512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 incompatible_features     []
 
+== Reopening a dirty image read/write should repair it ==
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
+wrote 512/512 bytes at offset 0
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+./common.rc: Killed ( VALGRIND_QEMU="${VALGRIND_QEMU_IO}" _qemu_proc_exec "${VALGRIND_LOGFILE}" "$QEMU_IO_PROG" $QEMU_IO_ARGS "$@" )
+incompatible_features     [0]
+ERROR cluster 5 refcount=0 reference=1
+Rebuilding refcount structure
+Repairing cluster 1 refcount=1 reference=0
+Repairing cluster 2 refcount=1 reference=0
+wrote 512/512 bytes at offset 1048576
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 512/512 bytes at offset 0
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+incompatible_features     []
+No errors were found on the image.
+
 == Creating an image file with lazy_refcounts=off ==
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
 wrote 512/512 bytes at offset 0
-- 
2.53.0

Re: [PATCH 2/2] qcow2: repair a dirty image when it becomes writable

[Hanna Czenczek]

On 01.08.26 00:00, Denis V. Lunev wrote:
> A dirty image must be repaired before anything allocates a cluster in
> it. qcow2_do_open() does that, but only for a node that is writable
> from the start. A node opened read-only skips it, and nothing revisits
> the question once that node becomes writable, which block-commit does
> routinely: commit_active_start() and commit_start() reopen the base
> read-write for the duration of the job.
>
> With lazy refcounts the on-disk refcount block then still accounts for
> the metadata clusters only, so the allocator restarts at the front of
> the image and hands out clusters that L2 entries point at. Two guest
> offsets end up sharing one host cluster. Nothing fails, the corrupt bit
> stays clear, and a clean close clears the dirty bit, so no later open
> repairs the image either.
>
> Do the repair in qcow2_reopen_commit_post(). bdrv_reopen_prepare() runs
> before bdrv_list_refresh_perms(), so it holds no BLK_PERM_WRITE and,
> with auto-read-only, bs->file may still have an O_RDONLY descriptor.
> commit_post cannot reject the reopen, so signal corruption if the
> repair fails rather than let writes alias live clusters. An inactive
> node is skipped: bdrv_activate() calls qcow2_do_open() again through
> qcow2_co_invalidate_cache().
>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Hanna Reitz <hreitz@redhat.com>
> ---
>   block/qcow2.c              | 14 ++++++++++++++
>   tests/qemu-iotests/039     | 24 ++++++++++++++++++++++++
>   tests/qemu-iotests/039.out | 17 +++++++++++++++++
>   3 files changed, 55 insertions(+)
>
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 1543255eba..e660655a0d 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -2147,8 +2147,22 @@ static void qcow2_reopen_commit(BDRVReopenState *state)
>   
>   static void qcow2_reopen_commit_post(BDRVReopenState *state)
>   {
> +    BDRVQcow2State *s = state->bs->opaque;
> +
>       GRAPH_RDLOCK_GUARD_MAINLOOP();
>   
> +    if (bdrv_is_writable(state->bs) &&
> +        (s->incompatible_features & QCOW2_INCOMPAT_DIRTY)) {
> +        BdrvCheckResult result = {0};
> +        int ret;
> +
> +        ret = bdrv_check(state->bs, &result, BDRV_FIX_ERRORS | BDRV_FIX_LEAKS);
> +        if (ret < 0 || result.check_errors) {
> +            qcow2_signal_corruption(state->bs, true, -1, -1,
> +                                    "Could not repair dirty image");
> +        }

Should we return here? I don’t think we should go on into the 
`qcow2_reopen_bitmaps_rw()` path.

Hanna

> +    }
> +
>       if (state->flags & BDRV_O_RDWR) {
>           Error *local_err = NULL;
>   

Re: [PATCH 2/2] qcow2: repair a dirty image when it becomes writable

[Andrey Drobyshev]

On 8/1/26 1:00 AM, Denis V. Lunev wrote:
> A dirty image must be repaired before anything allocates a cluster in
> it. qcow2_do_open() does that, but only for a node that is writable
> from the start. A node opened read-only skips it, and nothing revisits
> the question once that node becomes writable, which block-commit does
> routinely: commit_active_start() and commit_start() reopen the base
> read-write for the duration of the job.
> 
> With lazy refcounts the on-disk refcount block then still accounts for
> the metadata clusters only, so the allocator restarts at the front of
> the image and hands out clusters that L2 entries point at. Two guest
> offsets end up sharing one host cluster. Nothing fails, the corrupt bit
> stays clear, and a clean close clears the dirty bit, so no later open
> repairs the image either.
> 
> Do the repair in qcow2_reopen_commit_post(). bdrv_reopen_prepare() runs
> before bdrv_list_refresh_perms(), so it holds no BLK_PERM_WRITE and,
> with auto-read-only, bs->file may still have an O_RDONLY descriptor.
> commit_post cannot reject the reopen, so signal corruption if the
> repair fails rather than let writes alias live clusters. An inactive
> node is skipped: bdrv_activate() calls qcow2_do_open() again through
> qcow2_co_invalidate_cache().
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Hanna Reitz <hreitz@redhat.com>
> ---
>  block/qcow2.c              | 14 ++++++++++++++
>  tests/qemu-iotests/039     | 24 ++++++++++++++++++++++++
>  tests/qemu-iotests/039.out | 17 +++++++++++++++++
>  3 files changed, 55 insertions(+)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 1543255eba..e660655a0d 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -2147,8 +2147,22 @@ static void qcow2_reopen_commit(BDRVReopenState *state)
>  
>  static void qcow2_reopen_commit_post(BDRVReopenState *state)
>  {
> +    BDRVQcow2State *s = state->bs->opaque;
> +
>      GRAPH_RDLOCK_GUARD_MAINLOOP();
>  
> +    if (bdrv_is_writable(state->bs) &&
> +        (s->incompatible_features & QCOW2_INCOMPAT_DIRTY)) {
> +        BdrvCheckResult result = {0};
> +        int ret;
> +
> +        ret = bdrv_check(state->bs, &result, BDRV_FIX_ERRORS | BDRV_FIX_LEAKS);
> +        if (ret < 0 || result.check_errors) {
> +            qcow2_signal_corruption(state->bs, true, -1, -1,
> +                                    "Could not repair dirty image");
> +        }
> +    }
> +

This looks wrong.  What if we blockdev_reopen() a healthy image which is
already RW?  Say, we want to tweak L2 cache size.  Then we reopen
RW->RW, and yet with your patch we do full scan of metadata.  IIUC we're
being drained at this point -> we get guest IO stalled because of this
scan.

The subject says "... when it BECOMES writable".  But essentially we're
doing full check anytime it IS writable.  That's a completely different
thing.

Andrey

>      if (state->flags & BDRV_O_RDWR) {
>          Error *local_err = NULL;
>  
> diff --git a/tests/qemu-iotests/039 b/tests/qemu-iotests/039
> index a5be81bc4a..255dc2e7e5 100755
> --- a/tests/qemu-iotests/039
> +++ b/tests/qemu-iotests/039
> @@ -137,6 +137,30 @@ $QEMU_IO -c "write 0 512" "$TEST_IMG" | _filter_qemu_io
>  # The dirty bit must not be set
>  _qcow2_dump_header | grep incompatible_features
>  
> +echo
> +echo "== Reopening a dirty image read/write should repair it =="
> +
> +_make_test_img -o "compat=1.1,lazy_refcounts=on" $size
> +
> +_NO_VALGRIND \
> +$QEMU_IO -c "write -P 0x5a 0 512" \
> +         -c "sigraise $(kill -l KILL)" "$TEST_IMG" 2>&1 \
> +    | _filter_qemu_io
> +
> +# The dirty bit must be set
> +_qcow2_dump_header | grep incompatible_features
> +
> +# The refcounts are only resolved when the node becomes writable. Without
> +# that, this write is allocated on top of the cluster at offset 0 and reading
> +# it back returns the data written here.
> +$QEMU_IO -r -c "reopen -w" \
> +            -c "write -P 0xb1 1M 512" \
> +            -c "read -P 0x5a 0 512" "$TEST_IMG" | _filter_qemu_io
> +
> +# The dirty bit must not be set
> +_qcow2_dump_header | grep incompatible_features
> +_check_test_img
> +
>  echo
>  echo "== Creating an image file with lazy_refcounts=off =="
>  
> diff --git a/tests/qemu-iotests/039.out b/tests/qemu-iotests/039.out
> index 3c71e5a3dd..62073916a9 100644
> --- a/tests/qemu-iotests/039.out
> +++ b/tests/qemu-iotests/039.out
> @@ -64,6 +64,23 @@ wrote 512/512 bytes at offset 0
>  512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>  incompatible_features     []
>  
> +== Reopening a dirty image read/write should repair it ==
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
> +wrote 512/512 bytes at offset 0
> +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +./common.rc: Killed ( VALGRIND_QEMU="${VALGRIND_QEMU_IO}" _qemu_proc_exec "${VALGRIND_LOGFILE}" "$QEMU_IO_PROG" $QEMU_IO_ARGS "$@" )
> +incompatible_features     [0]
> +ERROR cluster 5 refcount=0 reference=1
> +Rebuilding refcount structure
> +Repairing cluster 1 refcount=1 reference=0
> +Repairing cluster 2 refcount=1 reference=0
> +wrote 512/512 bytes at offset 1048576
> +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +read 512/512 bytes at offset 0
> +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +incompatible_features     []
> +No errors were found on the image.
> +
>  == Creating an image file with lazy_refcounts=off ==
>  Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
>  wrote 512/512 bytes at offset 0