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

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

[Mark Kanda]

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)) {
+        virtio_error(vdev, "virtio-blk BlockDriverState is NULL");
+        return -1;
+    }
+
     /* We always touch the last byte, so just see how big in_iov is.  */
     req->in_len = iov_size(in_iov, in_num);
     req->in = (void *)in_iov[in_num - 1].iov_base
-- 
1.8.3.1

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

[Stefan Hajnoczi]

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

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.

> +        virtio_error(vdev, "virtio-blk BlockDriverState is NULL");
> +        return -1;
> +    }
> +
>      /* We always touch the last byte, so just see how big in_iov is.  */
>      req->in_len = iov_size(in_iov, in_num);
>      req->in = (void *)in_iov[in_num - 1].iov_base
> -- 
> 1.8.3.1
> 

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

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

[Kevin Wolf]

[-- 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 --]

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

[Mark Kanda]

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 <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.

That's correct. The segfault I encountered was the bdrv_inc_in_flight() 
call in blk_aio_prwv().

Thanks,

-Mark

> 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
> 

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

[Stefan Hajnoczi]

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

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 <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.

Will you send a new version of that patch series?

Stefan

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

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

[Kevin Wolf]

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

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 <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.
> 
> 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

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

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 <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.
>>
>> 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

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

[Stefan Hajnoczi]

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

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 <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.
> 
> 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

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

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

[Stefan Hajnoczi]

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

On Tue, Jan 30, 2018 at 04:56:12PM +0100, 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 <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.
> > 
> > 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.

Okay, I'll let you know early next week whether I am able to work on it.

Stefan

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