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
