[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: |
Kevin Wolf |
Subject: |
Re: [PATCH v5 1/6] qcow2: Fix dangling pointer after reopen for 'file' |
Date: |
Tue, 6 Jul 2021 15:43:03 +0200 |
Am 06.07.2021 um 15:12 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 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..
"need" is a strong word, but I guess we shouldn't access it between
prepare and commit, so I agree setting it to NULL would make bugs in
this area very visible.
In fact, we wouldn't even need r->had_data_file then because commit
could just set s->data_file = state->bs->file if it's NULL.
> > 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?
This would be wrong because at the time that this runs, state->bs->file
is already updated and this is what has_data_file() checks against. So
you can't use has_data_file() any more until it's synced again with the
code below.
In fact, this is why I even added r->had_data_file. At first I directly
used has_data_file(state->bs) here and watched it break.
> > + 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);
> > }
Kevin
- [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 4/6] block: Support multiple reopening with x-blockdev-reopen, Kevin Wolf, 2021/07/06
- [PATCH v5 1/6] qcow2: Fix dangling pointer after reopen for 'file', Kevin Wolf, 2021/07/06
- [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 6/6] block: Make blockdev-reopen stable API, Kevin Wolf, 2021/07/06