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().
v1:
https://lore.kernel.org/qemu-devel/20260731220039.1765584-1-den@openvz.org/
v2:
https://lore.kernel.org/qemu-devel/20260811173857.396571-1-den@openvz.org/

Notes for review
----------------

 - The repair still runs under bdrv_drain_all(), so a block-commit onto
   a large dirty base stalls guest I/O for the length of a full metadata
   scan. Rejecting the reopen instead would be cheap, but block-commit
   depends on it succeeding, so repairing in place is the only option.

 - .bdrv_reopen_commit_post() runs after the transaction is committed
   and cannot roll anything back, so a negative return value reports an
   unusable node rather than rejecting the reopen. That is unusual, and
   it is the only channel available: there is no failable hook once the
   node holds BLK_PERM_WRITE.

Changes in v3
-------------

 - patch 1: the same bug aborts QEMU, not only fails a reopen, when the
   file node below is writable; the message says so and iotests 039
   covers the read-write to read-only direction as well. (Andrey)
 - patch 2: new, a reopen of a node whose driver is gone segfaults in
   bdrv_reopen_queue_child() and asserts in bdrv_reopen_prepare(), which
   is the crash a failed repair would reach through commit_clean();
   bdrv_reopen_prepare() reports it instead, iotests 060 covers it.
   (Andrey)
 - patch 3: bdrv_reopen_multiple() documents that a failure means either
   nothing changed or the reopen went through and left an unusable tree;
   no caller changes. (Andrey)
 - patch 3: an implementation which fails must set an error, asserted,
   and the loop skips a node whose driver is already gone.
 - patch 4: the pre-reopen flags are recorded as int old_flags rather
   than a bool, with a bdrv_reopen_was_writable() accessor for drivers.
   (Andrey)
 - patch 5: the failure names the node and keeps the errno of the check.
   (Andrey)
 - patch 5: a failed repair drops the driver instead of calling
   qcow2_signal_corruption(), which would write the corrupt bit into the
   header for what may be a transient ENOSPC.
 - patch 5: blockdev-reopen and block-commit document the failure and the
   state it leaves. (Andrey)
 - patch 5: iotests 040 covers a commit onto a dirty base through both
   commit_start() and commit_active_start(), and the failed repair.
   (Andrey)


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

Denis V. Lunev (5):
  qcow2: do not clear the dirty bit when reopening a read-only node
  block: reject a reopen of an unusable node instead of crashing
  block: let bdrv_reopen_commit_post() report a failure
  block: remember the flags a reopen starts from
  qcow2: repair a dirty image when it becomes writable

 block.c                          |  55 ++++++++++++--
 block/qcow2.c                    |  33 ++++++++-
 include/block/block-common.h     |   1 +
 include/block/block_int-common.h |  11 ++-
 qapi/block-core.json             |  16 ++++
 tests/qemu-iotests/039           | 110 ++++++++++++++++++++++++++++
 tests/qemu-iotests/039.out       |  56 ++++++++++++++
 tests/qemu-iotests/040           | 122 +++++++++++++++++++++++++++++++
 tests/qemu-iotests/040.out       |   4 +-
 tests/qemu-iotests/060           |  30 ++++++++
 tests/qemu-iotests/060.out       |  13 ++++
 11 files changed, 436 insertions(+), 15 deletions(-)


base-commit: fa19879df1658f96ac07365fca8835b7decd6995
-- 
2.53.0

On 8/19/26 3:05 PM, Denis V. Lunev wrote:
> 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().
> v1:
> https://lore.kernel.org/qemu-devel/20260731220039.1765584-1-den@openvz.org/
> v2:
> https://lore.kernel.org/qemu-devel/20260811173857.396571-1-den@openvz.org/
> 
> Notes for review
> ----------------
> 
>  - The repair still runs under bdrv_drain_all(), so a block-commit onto
>    a large dirty base stalls guest I/O for the length of a full metadata
>    scan. Rejecting the reopen instead would be cheap, but block-commit
>    depends on it succeeding, so repairing in place is the only option.
> 
>  - .bdrv_reopen_commit_post() runs after the transaction is committed
>    and cannot roll anything back, so a negative return value reports an
>    unusable node rather than rejecting the reopen. That is unusual, and
>    it is the only channel available: there is no failable hook once the
>    node holds BLK_PERM_WRITE.
> 
> Changes in v3
> -------------
> 
>  - patch 1: the same bug aborts QEMU, not only fails a reopen, when the
>    file node below is writable; the message says so and iotests 039
>    covers the read-write to read-only direction as well. (Andrey)
>  - patch 2: new, a reopen of a node whose driver is gone segfaults in
>    bdrv_reopen_queue_child() and asserts in bdrv_reopen_prepare(), which
>    is the crash a failed repair would reach through commit_clean();
>    bdrv_reopen_prepare() reports it instead, iotests 060 covers it.
>    (Andrey)
>  - patch 3: bdrv_reopen_multiple() documents that a failure means either
>    nothing changed or the reopen went through and left an unusable tree;
>    no caller changes. (Andrey)
>  - patch 3: an implementation which fails must set an error, asserted,
>    and the loop skips a node whose driver is already gone.
>  - patch 4: the pre-reopen flags are recorded as int old_flags rather
>    than a bool, with a bdrv_reopen_was_writable() accessor for drivers.
>    (Andrey)
>  - patch 5: the failure names the node and keeps the errno of the check.
>    (Andrey)
>  - patch 5: a failed repair drops the driver instead of calling
>    qcow2_signal_corruption(), which would write the corrupt bit into the
>    header for what may be a transient ENOSPC.
>  - patch 5: blockdev-reopen and block-commit document the failure and the
>    state it leaves. (Andrey)
>  - patch 5: iotests 040 covers a commit onto a dirty base through both
>    commit_start() and commit_active_start(), and the failed repair.
>    (Andrey)
> 
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Hanna Reitz <hreitz@redhat.com>
> CC: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
> 
> Denis V. Lunev (5):
>   qcow2: do not clear the dirty bit when reopening a read-only node
>   block: reject a reopen of an unusable node instead of crashing
>   block: let bdrv_reopen_commit_post() report a failure
>   block: remember the flags a reopen starts from
>   qcow2: repair a dirty image when it becomes writable
> 
>  block.c                          |  55 ++++++++++++--
>  block/qcow2.c                    |  33 ++++++++-
>  include/block/block-common.h     |   1 +
>  include/block/block_int-common.h |  11 ++-
>  qapi/block-core.json             |  16 ++++
>  tests/qemu-iotests/039           | 110 ++++++++++++++++++++++++++++
>  tests/qemu-iotests/039.out       |  56 ++++++++++++++
>  tests/qemu-iotests/040           | 122 +++++++++++++++++++++++++++++++
>  tests/qemu-iotests/040.out       |   4 +-
>  tests/qemu-iotests/060           |  30 ++++++++
>  tests/qemu-iotests/060.out       |  13 ++++
>  11 files changed, 436 insertions(+), 15 deletions(-)
> 
> 
> base-commit: fa19879df1658f96ac07365fca8835b7decd6995

Overall LGTM, with a couple of wording nits in the last commit.  For the
series:

Reviewed-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>

Btw this respin lost 'Cc: qemu-stable@nongnu.org' compared to v2 - but
it's probably needed since we're fixing genuine crash.

Andrey