Am 22.05.2026 um 17:13 hat Thomas Lamprecht geschrieben:
> Commit b8bfb1478d ("qcow2: Fix corruption on discard during write with
> COW") added a wait_for_dependencies() at the start of
> qcow2_subcluster_zeroize(). That fixes the inconsistency it set out to
> fix, but turns the lock-protected pre-check in the caller,
> qcow2_co_pwrite_zeroes(), into a stale one: the wait yields s->lock,
> so an in-flight allocating write whose QCowL2Meta is already on
> s->cluster_allocs (but whose L2 entry is not yet linked) gets to link
> its entry during the yield. When the zeroize wakes, the cluster is now
> NORMAL, and with BDRV_REQ_MAY_UNMAP the free path in zero_in_l2_slice()
> unmaps the just-written cluster, silently dropping the data write's
> payload.
>
> This is reachable with detect-zeroes=unmap (the default for VirtIO
> disks with discard on in Proxmox VE), under which the block layer
> auto-promotes all-zero buffers to BDRV_REQ_ZERO_WRITE |
> BDRV_REQ_MAY_UNMAP. A memory-constrained Debian guest running 'apt
> full-upgrade' on such a disk reproduces it as random SIGSEGVs:
> swapped-out code pages come back as zero.
>
> Wait for in-flight dependencies before the lock-protected check in
> qcow2_co_pwrite_zeroes(). If a write linked its L2 entry during the
> wait, the type check now fails and the block layer falls back to a
> bounce-buffered zero write that only touches the requested subrange,
> preserving the racing write's data. Promote wait_for_dependencies() to
> qcow2_wait_for_dependencies() so qcow2.c can call it.
>
> Fixes: b8bfb1478d ("qcow2: Fix corruption on discard during write with COW")
> Cc: [email protected]
> Tested-by: Fiona Ebner <[email protected]>
> Reviewed-by: Fiona Ebner <[email protected]>
> Signed-off-by: Thomas Lamprecht <[email protected]>
> ---
>
> Changes v1 -> v2:
> * improve comments (thx @Fiona)
> * add Fiona's R-b/T-b
>
> block/qcow2-cluster.c | 10 +++++-----
> block/qcow2.c | 10 ++++++++--
> block/qcow2.h | 4 ++++
> tests/qemu-iotests/046 | 23 +++++++++++++++++++++++
> tests/qemu-iotests/046.out | 10 ++++++++++
> 5 files changed, 50 insertions(+), 7 deletions(-)
>
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index 8b1e80bd0b..e02fae6a0c 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -1474,9 +1474,9 @@ static int coroutine_fn
> handle_dependencies(BlockDriverState *bs,
> return 0;
> }
>
> -static void coroutine_mixed_fn wait_for_dependencies(BlockDriverState *bs,
> - uint64_t guest_offset,
> - uint64_t bytes)
> +void coroutine_mixed_fn qcow2_wait_for_dependencies(BlockDriverState *bs,
> + uint64_t guest_offset,
> + uint64_t bytes)
> {
> BDRVQcow2State *s = bs->opaque;
> QCowL2Meta *m = NULL;
> @@ -2035,7 +2035,7 @@ int qcow2_cluster_discard(BlockDriverState *bs,
> uint64_t offset,
> * We don't need to allocate a QCowL2Meta for the discard operation
> because
> * s->lock is held for the duration of the whole operation.
> */
> - wait_for_dependencies(bs, offset, bytes);
> + qcow2_wait_for_dependencies(bs, offset, bytes);
>
> /* Caller must pass aligned values, except at image end */
> assert(QEMU_IS_ALIGNED(offset, s->cluster_size));
> @@ -2204,7 +2204,7 @@ int coroutine_fn
> qcow2_subcluster_zeroize(BlockDriverState *bs, uint64_t offset,
> * We don't need to allocate a QCowL2Meta for the zeroize operation
> because
> * s->lock is held for the duration of the whole operation.
> */
> - wait_for_dependencies(bs, offset, bytes);
> + qcow2_wait_for_dependencies(bs, offset, bytes);
>
> /* If we have to stay in sync with an external data file, zero out
> * s->data_file first. */
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 81fd299b4c..96efdd4503 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -4234,10 +4234,16 @@ qcow2_co_pwrite_zeroes(BlockDriverState *bs, int64_t
> offset, int64_t bytes,
> }
>
> qemu_co_mutex_lock(&s->lock);
> - /* We can have new write after previous check */
> offset -= head;
> bytes = s->subcluster_size;
> - nr = s->subcluster_size;
I'm not sure why you changed this line. Isn't the new version of it
doing exactly the same thing as before? This makes the diff a little
more confusing than it should be.
> + /*
> + * Wait for in-flight allocating writes first: otherwise the type
> + * check below could pass on UNALLOCATED while a yet-to-link_l2 write
> + * completes during qcow2_subcluster_zeroize()'s own wait, letting
> the
> + * resumed MAY_UNMAP discard the just-written data.
> + */
> + qcow2_wait_for_dependencies(bs, offset, bytes);
> + nr = bytes;
> ret = qcow2_get_host_offset(bs, offset, &nr, &off, &type);
> if (ret < 0 ||
> (type != QCOW2_SUBCLUSTER_UNALLOCATED_PLAIN &&
The rest looks good to me. Maybe not perfect because we're calling
qcow2_wait_for_dependencies() twice now and rely on the second instance
not doing anything any more, but I guess that's okay.
So if you agree, I'd just change the assignment of nr above back and
apply this.
Kevin
