[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH] virtio-blk: check for NULL BlockDriverState
From: |
Stefan Hajnoczi |
Subject: |
Re: [Qemu-block] [PATCH] virtio-blk: check for NULL BlockDriverState |
Date: |
Thu, 1 Feb 2018 17:58:48 +0000 |
User-agent: |
Mutt/1.9.1 (2017-09-22) |
On Mon, Jan 29, 2018 at 10:13:02AM -0600, Mark Kanda wrote:
>
>
> On 1/29/2018 9:41 AM, Kevin Wolf wrote:
> > Am 24.01.2018 um 12:31 hat Stefan Hajnoczi geschrieben:
> > > On Mon, Jan 22, 2018 at 09:01:49AM -0600, Mark Kanda wrote:
> > > > Add a BlockDriverState NULL check to virtio_blk_handle_request()
> > > > to prevent a segfault if the drive is forcibly removed using HMP
> > > > 'drive_del' (without performing a hotplug 'device_del' first).
> > > >
> > > > Signed-off-by: Mark Kanda <address@hidden>
> > > > Reviewed-by: Karl Heubaum <address@hidden>
> > > > Reviewed-by: Ameya More <address@hidden>
> > > > ---
> > > > hw/block/virtio-blk.c | 7 +++++++
> > > > 1 file changed, 7 insertions(+)
> > > >
> > > > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> > > > index b1532e4..76ddbbf 100644
> > > > --- a/hw/block/virtio-blk.c
> > > > +++ b/hw/block/virtio-blk.c
> > > > @@ -507,6 +507,13 @@ static int
> > > > virtio_blk_handle_request(VirtIOBlockReq *req, MultiReqBuffer *mrb)
> > > > return -1;
> > > > }
> > > > + /* If the drive was forcibly removed (e.g. HMP 'drive_del'), the
> > > > block
> > > > + * driver state may be NULL and there is nothing left to do. */
> > > > + if (!blk_bs(req->dev->blk)) {
> > >
> > > Adding Markus Armbruster to check my understanding of drive_del:
> > >
> > > 1. If id is a node name (e.g. created via blockdev-add) then attempting
> > > to remove the root node produces the "Node %s is in use" error. In
> > > that case this patch isn't needed.
> > >
> > > 2. If id is a BlockBackend (e.g. created via -drive) then removing the
> > > root node is allowed. The BlockBackend stays in place but blk->root
> > > becomes NULL, hence this patch is needed.
> > >
> > > Markus: What are the valid use cases for #2? If blk->bs becomes NULL I
> > > would think a lot more code beyond virtio-blk can segfault.
> >
> > blk->root = NULL is completely normal, it is what happens with removable
> > media when the drive is empty.
> >
> > The problem, which was first reported during the 2.10 RC phase and was
> > worked around in IDE code then, is that Paolo's commit 99723548561 added
> > unconditional bdrv_inc/dec_in_flight() calls. I am pretty sure that any
> > segfaults that Mark is seeing have the same cause.
>
> That's correct. The segfault I encountered was the bdrv_inc_in_flight() call
> in blk_aio_prwv().
I think we should aim to fix the root cause for QEMU 2.12. blk_*() APIs
are supposed to return an error when blk->root is NULL but the
blk_aio_*() ones currently crash.
If that's not possible then this patch can be merged as a workaround for
2.12 close to the release date.
Stefan
signature.asc
Description: PGP signature
- Re: [Qemu-block] [PATCH] virtio-blk: check for NULL BlockDriverState,
Stefan Hajnoczi <=