Re: [Qemu-devel] [Qemu-block] [PATCH] virtio-blk: check for NULL BlockDriverState

[Kevin Wolf <kwolf@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2500 bytes --]

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 <mark.kanda@oracle.com>
> > Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com>
> > Reviewed-by: Ameya More <ameya.more@oracle.com>
> > ---
> >  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.

We do need an in-flight counter even for those requests so that
blk_drain() works correctly, so just making the calls condition wouldn't
be right. However, this needs to become a separate counter in
BlockBackend, and the drain functions must be changed to make use of it.

I did post rough patches back then, but they weren't quite ready, and
since then they have fallen through the cracks.

Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]