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

[John Snow]

On 01/30/2018 10:56 AM, Kevin Wolf wrote:
> Am 30.01.2018 um 13:38 hat Stefan Hajnoczi geschrieben:
>> On Mon, Jan 29, 2018 at 04:41:07PM +0100, 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.
>>>
>>> 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.
>>
>> Will you send a new version of that patch series?
> 
> I would like to continue my work on the drain functions (which this
> would be a part of) sooner or later, but the work to enable libvirt to
> use blockdev-add is at a higher priority at the moment.
> 
> So if you can wait, I'll get to it eventually. If not, feel free to pick
> up the patches.
> 
> Kevin
> 

It'd probably be nice for 2.12.

I'm not sure I understand the throttling well enough to do it /quickly/
and I have other priorities right now, but let's try to keep this one in
mind as something to fix before another release goes by.

--js