Linux Virtualization - Dec 2011 - [PATCH RFC] virtio_blk: fix config handler race

Fix a theoretical race related to config work
handler: a config interrupt might happen
after we flush config work but before we
reset the device. It will then cause the
config work to run during or after reset.

Two problems with this:
- if this runs after device is gone we will get use after free
- access of config while reset is in progress is racy
(as layout is changing).

As a solution
1. flush after reset when we know there will be no more interrupts
2. add a flag to disable config access before reset

Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
---

RFC only as it's untested.
Bugfix so 3.2 material? Comments?

 drivers/block/virtio_blk.c |   22 +++++++++++++++++++++-
 1 files changed, 21 insertions(+), 1 deletions(-)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 4d0b70a..34633f3 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -4,6 +4,7 @@
 #include <linux/blkdev.h>
 #include <linux/hdreg.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
 #include <linux/virtio.h>
 #include <linux/virtio_blk.h>
 #include <linux/scatterlist.h>
@@ -36,6 +37,12 @@ struct virtio_blk
 	/* Process context for config space updates */
 	struct work_struct config_work;
 
+	/* Lock for config space updates */
+	struct mutex config_lock;
+
+	/* enable config space updates */
+	bool config_enable;
+
 	/* What host tells us, plus 2 for header & tailer. */
 	unsigned int sg_elems;
 
@@ -318,6 +325,10 @@ static void virtblk_config_changed_work(struct work_struct
*work)
 	char cap_str_2[10], cap_str_10[10];
 	u64 capacity, size;
 
+	mutex_lock(&vblk->config_lock);
+	if (!vblk->config_enable)
+		goto done;
+
 	/* Host must always specify the capacity. */
 	vdev->config->get(vdev, offsetof(struct virtio_blk_config, capacity),
 			  &capacity, sizeof(capacity));
@@ -340,6 +351,8 @@ static void virtblk_config_changed_work(struct work_struct
*work)
 		  cap_str_10, cap_str_2);
 
 	set_capacity(vblk->disk, capacity);
+done:
+	mutex_lock(&vblk->config_lock);
 }
 
 static void virtblk_config_changed(struct virtio_device *vdev)
@@ -388,7 +401,9 @@ static int __devinit virtblk_probe(struct virtio_device
*vdev)
 	vblk->vdev = vdev;
 	vblk->sg_elems = sg_elems;
 	sg_init_table(vblk->sg, vblk->sg_elems);
+	mutex_init(&vblk->config_lock);
 	INIT_WORK(&vblk->config_work, virtblk_config_changed_work);
+	vblk->config_enable = true;
 
 	/* We expect one virtqueue, for output. */
 	vblk->vq = virtio_find_single_vq(vdev, blk_done, "requests");
@@ -542,7 +557,10 @@ static void __devexit virtblk_remove(struct virtio_device
*vdev)
 	struct virtio_blk *vblk = vdev->priv;
 	int index = vblk->index;
 
-	flush_work(&vblk->config_work);
+	/* Prevent config work handler from accessing the device. */
+	mutex_lock(&vblk->config_lock);
+	vblk->config_enable = false;
+	mutex_unlock(&vblk->config_lock);
 
 	/* Nothing should be pending. */
 	BUG_ON(!list_empty(&vblk->reqs));
@@ -550,6 +568,8 @@ static void __devexit virtblk_remove(struct virtio_device
*vdev)
 	/* Stop all the virtqueues. */
 	vdev->config->reset(vdev);
 
+	flush_work(&vblk->config_work);
+
 	del_gendisk(vblk->disk);
 	blk_cleanup_queue(vblk->disk->queue);
 	put_disk(vblk->disk);
-- 
1.7.5.53.gc233e

On Wed, Dec 07, 2011 at 05:39:02PM +0200, Michael S. Tsirkin
wrote:
> Fix a theoretical race related to config work
> handler: a config interrupt might happen
> after we flush config work but before we
> reset the device. It will then cause the
> config work to run during or after reset.
> 
> Two problems with this:
> - if this runs after device is gone we will get use after free
> - access of config while reset is in progress is racy
> (as layout is changing).
> 
> As a solution
> 1. flush after reset when we know there will be no more interrupts
> 2. add a flag to disable config access before reset
> 
> Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
> ---
> 
> RFC only as it's untested.
> Bugfix so 3.2 material? Comments?
Works fine for me. Comments?
> 
>  drivers/block/virtio_blk.c |   22 +++++++++++++++++++++-
>  1 files changed, 21 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index 4d0b70a..34633f3 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -4,6 +4,7 @@
>  #include <linux/blkdev.h>
>  #include <linux/hdreg.h>
>  #include <linux/module.h>
> +#include <linux/mutex.h>
>  #include <linux/virtio.h>
>  #include <linux/virtio_blk.h>
>  #include <linux/scatterlist.h>
> @@ -36,6 +37,12 @@ struct virtio_blk
>  	/* Process context for config space updates */
>  	struct work_struct config_work;
>  
> +	/* Lock for config space updates */
> +	struct mutex config_lock;
> +
> +	/* enable config space updates */
> +	bool config_enable;
> +
>  	/* What host tells us, plus 2 for header & tailer. */
>  	unsigned int sg_elems;
>  
> @@ -318,6 +325,10 @@ static void virtblk_config_changed_work(struct
work_struct *work)
>  	char cap_str_2[10], cap_str_10[10];
>  	u64 capacity, size;
>  
> +	mutex_lock(&vblk->config_lock);
> +	if (!vblk->config_enable)
> +		goto done;
> +
>  	/* Host must always specify the capacity. */
>  	vdev->config->get(vdev, offsetof(struct virtio_blk_config,
capacity),
>  			  &capacity, sizeof(capacity));
> @@ -340,6 +351,8 @@ static void virtblk_config_changed_work(struct
work_struct *work)
>  		  cap_str_10, cap_str_2);
>  
>  	set_capacity(vblk->disk, capacity);
> +done:
> +	mutex_lock(&vblk->config_lock);
>  }
>  
>  static void virtblk_config_changed(struct virtio_device *vdev)
> @@ -388,7 +401,9 @@ static int __devinit virtblk_probe(struct virtio_device
*vdev)
>  	vblk->vdev = vdev;
>  	vblk->sg_elems = sg_elems;
>  	sg_init_table(vblk->sg, vblk->sg_elems);
> +	mutex_init(&vblk->config_lock);
>  	INIT_WORK(&vblk->config_work, virtblk_config_changed_work);
> +	vblk->config_enable = true;
>  
>  	/* We expect one virtqueue, for output. */
>  	vblk->vq = virtio_find_single_vq(vdev, blk_done,
"requests");
> @@ -542,7 +557,10 @@ static void __devexit virtblk_remove(struct
virtio_device *vdev)
>  	struct virtio_blk *vblk = vdev->priv;
>  	int index = vblk->index;
>  
> -	flush_work(&vblk->config_work);
> +	/* Prevent config work handler from accessing the device. */
> +	mutex_lock(&vblk->config_lock);
> +	vblk->config_enable = false;
> +	mutex_unlock(&vblk->config_lock);
>  
>  	/* Nothing should be pending. */
>  	BUG_ON(!list_empty(&vblk->reqs));
> @@ -550,6 +568,8 @@ static void __devexit virtblk_remove(struct
virtio_device *vdev)
>  	/* Stop all the virtqueues. */
>  	vdev->config->reset(vdev);
>  
> +	flush_work(&vblk->config_work);
> +
>  	del_gendisk(vblk->disk);
>  	blk_cleanup_queue(vblk->disk->queue);
>  	put_disk(vblk->disk);
> -- 
> 1.7.5.53.gc233e

On Wed, 7 Dec 2011 17:39:02 +0200, "Michael S. Tsirkin" <mst at
redhat.com> wrote:
> Fix a theoretical race related to config work
> handler: a config interrupt might happen
> after we flush config work but before we
> reset the device. It will then cause the
> config work to run during or after reset.
Thanks for the reminder.  Apologies that this slipped through :(
> As a solution
> 1. flush after reset when we know there will be no more interrupts
> 2. add a flag to disable config access before reset
It's crude, but it works.  Applied.

If this happens again, do we want a "__virtio_device_disable" which
stops any interrupts from being delivered?  That would make this neater,
though the implementation might be a bit nasty.

Thanks,
Rusty.