| Message ID | 20190912135632.13925-2-mreitz@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | mirror: Do not dereference invalid pointers | expand |
On 9/12/19 9:56 AM, Max Reitz wrote: > mirror_exit_common() may be called twice (if it is called from > mirror_prepare() and fails, it will be called from mirror_abort() > again). > > In such a case, many of the pointers in the MirrorBlockJob object will > already be freed. This can be seen most reliably for s->target, which > is set to NULL (and then dereferenced by blk_bs()). > > Cc: qemu-stable@nongnu.org > Fixes: 737efc1eda23b904fbe0e66b37715fb0e5c3e58b > Signed-off-by: Max Reitz <mreitz@redhat.com> Sorry that I left the mirror callbacks such a mess. I think I got the design for the completion callbacks completely wrong, because of how hard it is to disentangle mirror into something reasonable here. The original idea was that it calls .prepare, then either .commit or .abort, then .clean. Ideally, there would be no exit_common for mirror; everything would be sorted into the proper little callback silos. The problem with the existing design is that if it has already failed by .prepare time, we jump straight to .abort, so it has this uneven, unbalanced design. The code in abort and cleanup therefore has to work doubly-hard to figure out what exactly it needs to do. It's a mess. I wonder if it can be improved by always calling prepare, even when we've already failed. We could just posit that it now means "prepare to commit" or "prepare to abort" but we can do all of the work that can still fail there in one shot. Maybe that will make the work that needs to happen in abort/commit easier to digest. > --- > block/mirror.c | 13 +++++++++---- > 1 file changed, 9 insertions(+), 4 deletions(-) > > diff --git a/block/mirror.c b/block/mirror.c > index fe984efb90..706d80fced 100644 > --- a/block/mirror.c > +++ b/block/mirror.c > @@ -620,11 +620,11 @@ static int mirror_exit_common(Job *job) > { > MirrorBlockJob *s = container_of(job, MirrorBlockJob, common.job); > BlockJob *bjob = &s->common; > - MirrorBDSOpaque *bs_opaque = s->mirror_top_bs->opaque; > + MirrorBDSOpaque *bs_opaque; > AioContext *replace_aio_context = NULL; > - BlockDriverState *src = s->mirror_top_bs->backing->bs; > - BlockDriverState *target_bs = blk_bs(s->target); > - BlockDriverState *mirror_top_bs = s->mirror_top_bs; > + BlockDriverState *src; > + BlockDriverState *target_bs; > + BlockDriverState *mirror_top_bs; > Error *local_err = NULL; > bool abort = job->ret < 0; > int ret = 0; > @@ -634,6 +634,11 @@ static int mirror_exit_common(Job *job) > } > s->prepared = true; > > + mirror_top_bs = s->mirror_top_bs; > + bs_opaque = mirror_top_bs->opaque; > + src = mirror_top_bs->backing->bs; > + target_bs = blk_bs(s->target); > + > if (bdrv_chain_contains(src, target_bs)) { > bdrv_unfreeze_backing_chain(mirror_top_bs, target_bs); > } > Reviewed-by: John Snow <jsnow@redhat.com>
12.09.2019 16:56, Max Reitz wrote: > mirror_exit_common() may be called twice (if it is called from > mirror_prepare() and fails, it will be called from mirror_abort() > again). > > In such a case, many of the pointers in the MirrorBlockJob object will > already be freed. This can be seen most reliably for s->target, which > is set to NULL (and then dereferenced by blk_bs()). > > Cc: qemu-stable@nongnu.org > Fixes: 737efc1eda23b904fbe0e66b37715fb0e5c3e58b > Signed-off-by: Max Reitz <mreitz@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > block/mirror.c | 13 +++++++++---- > 1 file changed, 9 insertions(+), 4 deletions(-) > > diff --git a/block/mirror.c b/block/mirror.c > index fe984efb90..706d80fced 100644 > --- a/block/mirror.c > +++ b/block/mirror.c > @@ -620,11 +620,11 @@ static int mirror_exit_common(Job *job) > { > MirrorBlockJob *s = container_of(job, MirrorBlockJob, common.job); > BlockJob *bjob = &s->common; > - MirrorBDSOpaque *bs_opaque = s->mirror_top_bs->opaque; > + MirrorBDSOpaque *bs_opaque; > AioContext *replace_aio_context = NULL; > - BlockDriverState *src = s->mirror_top_bs->backing->bs; > - BlockDriverState *target_bs = blk_bs(s->target); > - BlockDriverState *mirror_top_bs = s->mirror_top_bs; > + BlockDriverState *src; > + BlockDriverState *target_bs; > + BlockDriverState *mirror_top_bs; > Error *local_err = NULL; > bool abort = job->ret < 0; > int ret = 0; > @@ -634,6 +634,11 @@ static int mirror_exit_common(Job *job) > } > s->prepared = true; > > + mirror_top_bs = s->mirror_top_bs; > + bs_opaque = mirror_top_bs->opaque; > + src = mirror_top_bs->backing->bs; > + target_bs = blk_bs(s->target); > + > if (bdrv_chain_contains(src, target_bs)) { > bdrv_unfreeze_backing_chain(mirror_top_bs, target_bs); > } >
diff --git a/block/mirror.c b/block/mirror.c index fe984efb90..706d80fced 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -620,11 +620,11 @@ static int mirror_exit_common(Job *job) { MirrorBlockJob *s = container_of(job, MirrorBlockJob, common.job); BlockJob *bjob = &s->common; - MirrorBDSOpaque *bs_opaque = s->mirror_top_bs->opaque; + MirrorBDSOpaque *bs_opaque; AioContext *replace_aio_context = NULL; - BlockDriverState *src = s->mirror_top_bs->backing->bs; - BlockDriverState *target_bs = blk_bs(s->target); - BlockDriverState *mirror_top_bs = s->mirror_top_bs; + BlockDriverState *src; + BlockDriverState *target_bs; + BlockDriverState *mirror_top_bs; Error *local_err = NULL; bool abort = job->ret < 0; int ret = 0; @@ -634,6 +634,11 @@ static int mirror_exit_common(Job *job) } s->prepared = true; + mirror_top_bs = s->mirror_top_bs; + bs_opaque = mirror_top_bs->opaque; + src = mirror_top_bs->backing->bs; + target_bs = blk_bs(s->target); + if (bdrv_chain_contains(src, target_bs)) { bdrv_unfreeze_backing_chain(mirror_top_bs, target_bs); }
mirror_exit_common() may be called twice (if it is called from mirror_prepare() and fails, it will be called from mirror_abort() again). In such a case, many of the pointers in the MirrorBlockJob object will already be freed. This can be seen most reliably for s->target, which is set to NULL (and then dereferenced by blk_bs()). Cc: qemu-stable@nongnu.org Fixes: 737efc1eda23b904fbe0e66b37715fb0e5c3e58b Signed-off-by: Max Reitz <mreitz@redhat.com> --- block/mirror.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)