@samitouri / QOSamiQemu / commits / b8bfb1478d

qcow2: Fix corruption on discard during write with COW

Most code in qcow2 that accesses (and potentially modifies) L2 tables does so while holding s->lock. There is one exception, which is allocating writes. They hold the lock initially while allocating clusters, but drop it for writing the guest payload before taking the lock again for updating the L2 tables. This allows concurrent requests that touch other parts of the image file to continue in parallel and is an important performance optimisation. However, this means that other requests that run while the lock is dropped for writing guest data must synchronise with the list of allocating requests in s->cluster_allocs and wait if they would overlap. For writes, this is done in handle_dependencies(), but discard and write zeros operations neglect to synchronise with s->cluster_allocs. This means that discard can free a cluster whose L2 entry will already be modified in qcow2_alloc_cluster_link_l2() by a previously started write. In the case of a pre-allocated zero cluster that is in the process of being overwritten, this means that discard can lead to a situation where the cluster is still mapped (because the write will restore the L2 entry just without the zero flag), but its refcount has been decreased, resulting in a corrupted image. Add the missing synchronisation to qcow2_cluster_discard() and qcow2_subcluster_zeroize() to fix the problem. Cc: qemu-stable@nongnu.org Reported-by: Denis V. Lunev <den@openvz.org> Signed-off-by: Kevin Wolf <kwolf@redhat.com> Message-ID: <20260427170520.101242-4-kwolf@redhat.com> Reviewed-by: Denis V. Lunev <den@openvz.org> Tested-by: Denis V. Lunev <den@openvz.org> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

Kevin Wolf committed Apr 27, 2026 at 19:05 UTC b8bfb1478d61512f851badd0d912c6661a2efee7
1 file changed +49 -3
block/qcow2-cluster.c
+49 -3
@@ -1392,6 +1392,9 @@ count_single_write_clusters(BlockDriverState *bs, int nb_clusters,
1392 * the same cluster. In this case we need to wait until the previous
1393 * request has completed and updated the L2 table accordingly.
1394 *
1395 + * If allow_shortening == true, instead of waiting for a dependency, *cur_bytes
1396 + * can be shortened so that the cluster allocations don't overlap.
1397 + *
1398 * Returns:
1399 * 0 if there was no dependency. *cur_bytes indicates the number of
1400 * bytes from guest_offset that can be read before the next
@@ -1403,7 +1406,9 @@ count_single_write_clusters(BlockDriverState *bs, int nb_clusters,
1406 */
1407 static int coroutine_fn handle_dependencies(BlockDriverState *bs,
1408 uint64_t guest_offset,
1406 - uint64_t *cur_bytes, QCowL2Meta **m)
1409 + uint64_t *cur_bytes,
1410 + bool allow_shortening,
1411 + QCowL2Meta **m)
1412 {
1413 BDRVQcow2State *s = bs->opaque;
1414 QCowL2Meta *old_alloc;
@@ -1434,7 +1439,7 @@ static int coroutine_fn handle_dependencies(BlockDriverState *bs,
1439
1440 /* Conflict */
1441
1437 - if (start < old_start) {
1442 + if (start < old_start && allow_shortening) {
1443 /* Stop at the start of a running allocation */
1444 bytes = old_start - start;
1445 } else {
@@ -1469,6 +1474,29 @@ static int coroutine_fn handle_dependencies(BlockDriverState *bs,
1474 return 0;
1475 }
1476
1477 +static void coroutine_mixed_fn wait_for_dependencies(BlockDriverState *bs,
1478 + uint64_t guest_offset,
1479 + uint64_t bytes)
1480 +{
1481 + BDRVQcow2State *s = bs->opaque;
1482 + QCowL2Meta *m = NULL;
1483 + int ret;
1484 +
1485 + /*
1486 + * Discard has some non-coroutine callers (creating internal snapshots and
1487 + * make empty). They are calling from qemu-img or in a drained section, so
1488 + * we know that no writes can be in progress.
1489 + */
1490 + if (!qemu_in_coroutine()) {
1491 + assert(QLIST_EMPTY(&s->cluster_allocs));
1492 + return;
1493 + }
1494 +
1495 + do {
1496 + ret = handle_dependencies(bs, guest_offset, &bytes, false, &m);
1497 + } while (ret == -EAGAIN);
1498 +}
1499 +
1500 /*
1501 * Checks how many already allocated clusters that don't require a new
1502 * allocation there are at the given guest_offset (up to *bytes).
@@ -1840,7 +1868,7 @@ again:
1868 * the right synchronisation between the in-flight request and
1869 * the new one.
1870 */
1843 - ret = handle_dependencies(bs, start, &cur_bytes, m);
1871 + ret = handle_dependencies(bs, start, &cur_bytes, true, m);
1872 if (ret == -EAGAIN) {
1873 /* Currently handle_dependencies() doesn't yield if we already had
1874 * an allocation. If it did, we would have to clean up the L2Meta
@@ -2000,6 +2028,15 @@ int qcow2_cluster_discard(BlockDriverState *bs, uint64_t offset,
2028 int64_t cleared;
2029 int ret;
2030
2031 + /*
2032 + * If we're touching a cluster for which allocating writes are in flight,
2033 + * wait for them to complete to avoid conflicting metadata updates.
2034 + *
2035 + * We don't need to allocate a QCowL2Meta for the discard operation because
2036 + * s->lock is held for the duration of the whole operation.
2037 + */
2038 + wait_for_dependencies(bs, offset, bytes);
2039 +
2040 /* Caller must pass aligned values, except at image end */
2041 assert(QEMU_IS_ALIGNED(offset, s->cluster_size));
2042 assert(QEMU_IS_ALIGNED(end_offset, s->cluster_size) ||
@@ -2160,6 +2197,15 @@ int coroutine_fn qcow2_subcluster_zeroize(BlockDriverState *bs, uint64_t offset,
2197 int64_t cleared;
2198 int ret;
2199
2200 + /*
2201 + * If we're touching a cluster for which allocating writes are in flight,
2202 + * wait for them to complete to avoid conflicting metadata updates.
2203 + *
2204 + * We don't need to allocate a QCowL2Meta for the zeroize operation because
2205 + * s->lock is held for the duration of the whole operation.
2206 + */
2207 + wait_for_dependencies(bs, offset, bytes);
2208 +
2209 /* If we have to stay in sync with an external data file, zero out
2210 * s->data_file first. */
2211 if (data_file_is_raw(bs)) {