QEMU-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: John Snow <jsnow@redhat.com>
To: Max Reitz <mreitz@redhat.com>, qemu-block@nongnu.org
Cc: Kevin Wolf <kwolf@redhat.com>,
	qemu-devel@nongnu.org, Markus Armbruster <armbru@redhat.com>
Subject: Re: [Qemu-devel] [PATCH 1/4] mirror: Do not dereference invalid pointers
Date: Fri, 13 Sep 2019 18:43:41 -0400
Message-ID: <cf03d000-5c59-74e2-cebb-0a62414ce271@redhat.com> (raw)
In-Reply-To: <20190912135632.13925-2-mreitz@redhat.com>



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>


  reply index

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-09-12 13:56 [Qemu-devel] [PATCH 0/4] " Max Reitz
2019-09-12 13:56 ` [Qemu-devel] [PATCH 1/4] " Max Reitz
2019-09-13 22:43   ` John Snow [this message]
2019-09-18 15:16   ` Vladimir Sementsov-Ogievskiy
2019-09-12 13:56 ` [Qemu-devel] [PATCH 2/4] blkdebug: Allow taking/unsharing permissions Max Reitz
2019-09-18 16:01   ` Vladimir Sementsov-Ogievskiy
2019-09-19 16:49     ` Max Reitz
2019-09-12 13:56 ` [Qemu-devel] [PATCH 3/4] iotests: Add @error to wait_until_completed Max Reitz
2019-09-13 22:53   ` John Snow
2019-09-16  7:56     ` Max Reitz
2019-09-18 16:09   ` Vladimir Sementsov-Ogievskiy
2019-09-12 13:56 ` [Qemu-devel] [PATCH 4/4] iotests: Add test for failing mirror complete Max Reitz
2019-09-18 16:30   ` Vladimir Sementsov-Ogievskiy
2019-09-19 16:51     ` Max Reitz
2019-09-18 18:46   ` John Snow
2019-09-19 16:58     ` Max Reitz
2019-09-19 17:02       ` John Snow
2019-09-19 17:06         ` Max Reitz
2019-09-18 15:38 ` [Qemu-devel] [PATCH 0/4] mirror: Do not dereference invalid pointers Vladimir Sementsov-Ogievskiy
2019-09-19 16:45   ` Max Reitz
2019-09-19 16:50     ` Vladimir Sementsov-Ogievskiy

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=cf03d000-5c59-74e2-cebb-0a62414ce271@redhat.com \
    --to=jsnow@redhat.com \
    --cc=armbru@redhat.com \
    --cc=kwolf@redhat.com \
    --cc=mreitz@redhat.com \
    --cc=qemu-block@nongnu.org \
    --cc=qemu-devel@nongnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

QEMU-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/qemu-devel/0 qemu-devel/git/0.git
	git clone --mirror https://lore.kernel.org/qemu-devel/1 qemu-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 qemu-devel qemu-devel/ https://lore.kernel.org/qemu-devel \
		qemu-devel@nongnu.org qemu-devel@archiver.kernel.org
	public-inbox-index qemu-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.nongnu.qemu-devel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox