[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v5 1/6] qcow2: Fix dangling pointer after reopen for 'file'
From: |
Vladimir Sementsov-Ogievskiy |
Subject: |
Re: [PATCH v5 1/6] qcow2: Fix dangling pointer after reopen for 'file' |
Date: |
Tue, 6 Jul 2021 16:12:47 +0300 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 |
06.07.2021 14:23, Kevin Wolf wrote:
Without an external data file, s->data_file is a second pointer with the
same value as bs->file. When changing bs->file to a different BdrvChild
and freeing the old BdrvChild, s->data_file must also be updated,
otherwise it points to freed memory and causes crashes.
This problem was caught by iotests case 245.
Fixes: df2b7086f169239ebad5d150efa29c9bb6d4f820
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Still, some ideas below:
---
block/qcow2.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/block/qcow2.c b/block/qcow2.c
index ee4530cdbd..cb459ef6a6 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -962,6 +962,7 @@ static bool read_cache_sizes(BlockDriverState *bs, QemuOpts
*opts,
}
typedef struct Qcow2ReopenState {
+ bool had_data_file;
Qcow2Cache *l2_table_cache;
Qcow2Cache *refcount_block_cache;
int l2_slice_size; /* Number of entries in a slice of the L2 table */
@@ -1932,6 +1933,8 @@ static int qcow2_reopen_prepare(BDRVReopenState *state,
r = g_new0(Qcow2ReopenState, 1);
state->opaque = r;
+ r->had_data_file = has_data_file(state->bs);
+
So, during reopen, at some moment s->data_file become invalid. So, we shouldn't
rely on it..
Maybe we need
s->data_file = NULL;
here..
ret = qcow2_update_options_prepare(state->bs, r, state->options,
state->flags, errp);
if (ret < 0) {
@@ -1966,7 +1969,18 @@ fail:
static void qcow2_reopen_commit(BDRVReopenState *state)
{
+ BDRVQcow2State *s = state->bs->opaque;
+ Qcow2ReopenState *r = state->opaque;
+
qcow2_update_options_commit(state->bs, state->opaque);
Worth doing
assert(r->had_data_file == has_data_file(state->bs));
here, to be double sure?
+ if (!r->had_data_file && s->data_file != state->bs->file) {
+ /*
+ * If s->data_file is just a second pointer to bs->file (which is the
+ * case without an external data file), it may need to be updated.
+ */
+ s->data_file = state->bs->file;
+ assert(!has_data_file(state->bs));
+ }
g_free(state->opaque);
}
--
Best regards,
Vladimir
- [PATCH v5 0/6] Make blockdev-reopen stable, Kevin Wolf, 2021/07/06
- [PATCH v5 3/6] block: Acquire AioContexts during bdrv_reopen_multiple(), Kevin Wolf, 2021/07/06
- [PATCH v5 1/6] qcow2: Fix dangling pointer after reopen for 'file', Kevin Wolf, 2021/07/06
- Re: [PATCH v5 1/6] qcow2: Fix dangling pointer after reopen for 'file',
Vladimir Sementsov-Ogievskiy <=
- [PATCH v5 5/6] iotests: Test reopening multiple devices at the same time, Kevin Wolf, 2021/07/06
- [PATCH v5 2/6] block: Add bdrv_reopen_queue_free(), Kevin Wolf, 2021/07/06
- [PATCH v5 4/6] block: Support multiple reopening with x-blockdev-reopen, Kevin Wolf, 2021/07/06
- [PATCH v5 6/6] block: Make blockdev-reopen stable API, Kevin Wolf, 2021/07/06