[PATCH v2] qcow2: Fix data loss on zero write with detect-zeroes=unmap

[PATCH v2] qcow2: Fix data loss on zero write with detect-zeroes=unmap

[Thomas Lamprecht]

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: qemu-stable@nongnu.org
Tested-by: Fiona Ebner <f.ebner@proxmox.com>
Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
---

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;
+        /*
+         * 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 &&
diff --git a/block/qcow2.h b/block/qcow2.h
index 192a45d596..ce517040c4 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -966,6 +966,10 @@ int coroutine_fn GRAPH_RDLOCK
 qcow2_subcluster_zeroize(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
                          int flags);
 
+void coroutine_mixed_fn
+qcow2_wait_for_dependencies(BlockDriverState *bs, uint64_t guest_offset,
+                            uint64_t bytes);
+
 int GRAPH_RDLOCK
 qcow2_expand_zero_clusters(BlockDriverState *bs,
                            BlockDriverAmendStatusCB *status_cb,
diff --git a/tests/qemu-iotests/046 b/tests/qemu-iotests/046
index e03dd40147..0d84b5c1c7 100755
--- a/tests/qemu-iotests/046
+++ b/tests/qemu-iotests/046
@@ -226,6 +226,26 @@ aio_write -z 0x140000 0x10000
 resume A
 aio_flush
 EOF
+
+# Start an allocating write to a previously unallocated cluster and, before
+# its L2 update is linked, issue a concurrent sub-cluster zero write with
+# MAY_UNMAP that targets a disjoint range within the same cluster. The zero
+# write's head/tail are zero (cluster is unallocated), so qcow2_co_pwrite_zeroes
+# would expand it to the full subcluster. Without waiting for dependencies
+# before the zero write's "unallocated" type check, that check passes,
+# qcow2_subcluster_zeroize then yields in wait_for_dependencies, the allocating
+# write links its L2 entry, and the resumed zeroize unmaps the cluster -
+# silently discarding the just-written data. Waiting first makes the zero write
+# fall back to a bounce-buffered real write, which only touches its own
+# subrange.
+cat <<EOF
+break write_aio A
+aio_write -P 180 0x200000 0x4000
+wait_break A
+aio_write -z -u 0x204000 0x4000
+resume A
+aio_flush
+EOF
 }
 
 overlay_io | $QEMU_IO blkdebug::"$TEST_IMG" | _filter_qemu_io |\
@@ -310,6 +330,9 @@ verify_io()
     echo read -P 0   0x120000 0x10000
     echo read -P 0   0x130000 0x10000
     echo read -P 0   0x140000 0x10000
+
+    echo read -P 180 0x200000 0x4000
+    echo read -P 0   0x204000 0xc000
 }
 
 verify_io | $QEMU_IO "$TEST_IMG" | _filter_qemu_io
diff --git a/tests/qemu-iotests/046.out b/tests/qemu-iotests/046.out
index 6341df335c..137cf527f1 100644
--- a/tests/qemu-iotests/046.out
+++ b/tests/qemu-iotests/046.out
@@ -169,6 +169,12 @@ wrote XXX/XXX bytes at offset XXX
 XXX KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 wrote XXX/XXX bytes at offset XXX
 XXX KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+blkdebug: Suspended request 'A'
+blkdebug: Resuming request 'A'
+wrote XXX/XXX bytes at offset XXX
+XXX KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote XXX/XXX bytes at offset XXX
+XXX KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 
 == Verify image content ==
 read 65536/65536 bytes at offset 0
@@ -275,5 +281,9 @@ read 65536/65536 bytes at offset 1245184
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 read 65536/65536 bytes at offset 1310720
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 16384/16384 bytes at offset 2097152
+16 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 49152/49152 bytes at offset 2113536
+48 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 No errors were found on the image.
 *** done
-- 
2.47.3

Re: [PATCH v2] qcow2: Fix data loss on zero write with detect-zeroes=unmap

[Kevin Wolf]

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: qemu-stable@nongnu.org
> Tested-by: Fiona Ebner <f.ebner@proxmox.com>
> Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>
> Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
> ---
> 
> 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