virtio-blk: check for NULL BlockDriverState

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

Posted by Mark Kanda 6 years, 3 months ago

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

Posted by Stefan Hajnoczi 6 years, 3 months ago

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
>