block/qcow2-cluster.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)
From: Maxim Levitsky <mlevitsk@redhat.com>
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 <mlevitsk@redhat.com>
[ kwolf: Fixed to restore the correct original order from before
205fa50750; added comments like in discard_in_l2_slice(). ]
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
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);
--
2.28.0
On Tue 24 Nov 2020 10:28:15 AM CET, Kevin Wolf wrote: > From: Maxim Levitsky <mlevitsk@redhat.com> > > 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 <mlevitsk@redhat.com> > [ kwolf: Fixed to restore the correct original order from before > 205fa50750; added comments like in discard_in_l2_slice(). ] Reviewed-by: Alberto Garcia <berto@igalia.com> Berto
© 2016 - 2024 Red Hat, Inc.