From c8bf9a9169db094aaed680cdba570758c0dc18b9 Mon Sep 17 00:00:00 2001 From: Maxim Levitsky Date: Tue, 24 Nov 2020 10:28:15 +0100 Subject: [PATCH] qcow2: Fix corruption on write_zeroes with MAY_UNMAP Commit 205fa50750 ("qcow2: Add subcluster support to zero_in_l2_slice()") introduced a subtle change to code in zero_in_l2_slice: It swapped the order of 1. qcow2_cache_entry_mark_dirty(s->l2_table_cache, l2_slice); 2. set_l2_entry(s, l2_slice, l2_index + i, QCOW_OFLAG_ZERO); 3. qcow2_free_any_clusters(bs, old_offset, 1, QCOW2_DISCARD_REQUEST); To 1. qcow2_cache_entry_mark_dirty(s->l2_table_cache, l2_slice); 2. qcow2_free_any_clusters(bs, old_offset, 1, QCOW2_DISCARD_REQUEST); 3. set_l2_entry(s, l2_slice, l2_index + i, QCOW_OFLAG_ZERO); It seems harmless, however the call to qcow2_free_any_clusters can trigger a cache flush which can mark the L2 table as clean, and assuming that this was the last write to it, a stale version of it will remain on the disk. Now we have a valid L2 entry pointing to a freed cluster. Oops. Fixes: 205fa50750 ("qcow2: Add subcluster support to zero_in_l2_slice()") Signed-off-by: Maxim Levitsky [ kwolf: Fixed to restore the correct original order from before 205fa50750; added comments like in discard_in_l2_slice(). ] Signed-off-by: Kevin Wolf Message-Id: <20201124092815.39056-1-kwolf@redhat.com> Reviewed-by: Alberto Garcia Signed-off-by: Kevin Wolf --- block/qcow2-cluster.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c index 485b4cb92e..bd0597842f 100644 --- a/block/qcow2-cluster.c +++ b/block/qcow2-cluster.c @@ -2010,14 +2010,17 @@ static int zero_in_l2_slice(BlockDriverState *bs, uint64_t offset, continue; } + /* First update L2 entries */ qcow2_cache_entry_mark_dirty(s->l2_table_cache, l2_slice); - if (unmap) { - qcow2_free_any_cluster(bs, old_l2_entry, QCOW2_DISCARD_REQUEST); - } set_l2_entry(s, l2_slice, l2_index + i, new_l2_entry); if (has_subclusters(s)) { set_l2_bitmap(s, l2_slice, l2_index + i, new_l2_bitmap); } + + /* Then decrease the refcount */ + if (unmap) { + qcow2_free_any_cluster(bs, old_l2_entry, QCOW2_DISCARD_REQUEST); + } } qcow2_cache_put(s->l2_table_cache, (void **) &l2_slice);