Re: [PATCH v3 0/5] qcow2: silent corruption when a dirty image becomes writable

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/[email protected]/
> v2:
> https://lore.kernel.org/qemu-devel/[email protected]/
> 
> 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 <[email protected]>
> CC: Kevin Wolf <[email protected]>
> CC: Hanna Reitz <[email protected]>
> CC: Andrey Drobyshev <[email protected]>
> 
> 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 <[email protected]>

Btw this respin lost 'Cc: [email protected]' compared to v2 - but
it's probably needed since we're fixing genuine crash.

Andrey