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@redhat.com> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

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@redhat.com> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Michael S. Tsirkin · Rusty Russell
1 parent e93300b1af
Showing 1 changed file with 21 additions and 1 deletions Side-by-side Diff
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 @@
 	/* 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 @@
 	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 @@
 		  cap_str_10, cap_str_2);
  
 	set_capacity(vblk->disk, capacity);
+done:
+	mutex_unlock(&vblk->config_lock);
 }
  
 static void virtblk_config_changed(struct virtio_device *vdev)
  
@@ -388,7 +401,9 @@
 	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,13 +557,18 @@
 	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));
  
 	/* Stop all the virtqueues. */
 	vdev->config->reset(vdev);
+
+	flush_work(&vblk->config_work);
  
 	del_gendisk(vblk->disk);
 	blk_cleanup_queue(vblk->disk->queue);
...	...	@@ -4,6 +4,7 @@
4	4	#include <linux/blkdev.h>
5	5	#include <linux/hdreg.h>
6	6	#include <linux/module.h>
	7	+#include <linux/mutex.h>
7	8	#include <linux/virtio.h>
8	9	#include <linux/virtio_blk.h>
9	10	#include <linux/scatterlist.h>
...	...	@@ -36,6 +37,12 @@
36	37	/* Process context for config space updates */
37	38	struct work_struct config_work;
38	39
	40	+ /* Lock for config space updates */
	41	+ struct mutex config_lock;
	42	+
	43	+ /* enable config space updates */
	44	+ bool config_enable;
	45	+
39	46	/* What host tells us, plus 2 for header & tailer. */
40	47	unsigned int sg_elems;
41	48
...	...	@@ -318,6 +325,10 @@
318	325	char cap_str_2[10], cap_str_10[10];
319	326	u64 capacity, size;
320	327
	328	+ mutex_lock(&vblk->config_lock);
	329	+ if (!vblk->config_enable)
	330	+ goto done;
	331	+
321	332	/* Host must always specify the capacity. */
322	333	vdev->config->get(vdev, offsetof(struct virtio_blk_config, capacity),
323	334	&capacity, sizeof(capacity));
...	...	@@ -340,6 +351,8 @@
340	351	cap_str_10, cap_str_2);
341	352
342	353	set_capacity(vblk->disk, capacity);
	354	+done:
	355	+ mutex_unlock(&vblk->config_lock);
343	356	}
344	357
345	358	static void virtblk_config_changed(struct virtio_device *vdev)
346	359
...	...	@@ -388,7 +401,9 @@
388	401	vblk->vdev = vdev;
389	402	vblk->sg_elems = sg_elems;
390	403	sg_init_table(vblk->sg, vblk->sg_elems);
	404	+ mutex_init(&vblk->config_lock);
391	405	INIT_WORK(&vblk->config_work, virtblk_config_changed_work);
	406	+ vblk->config_enable = true;
392	407
393	408	/* We expect one virtqueue, for output. */
394	409	vblk->vq = virtio_find_single_vq(vdev, blk_done, "requests");
395	410
...	...	@@ -542,13 +557,18 @@
542	557	struct virtio_blk *vblk = vdev->priv;
543	558	int index = vblk->index;
544	559
545		- flush_work(&vblk->config_work);
	560	+ /* Prevent config work handler from accessing the device. */
	561	+ mutex_lock(&vblk->config_lock);
	562	+ vblk->config_enable = false;
	563	+ mutex_unlock(&vblk->config_lock);
546	564
547	565	/* Nothing should be pending. */
548	566	BUG_ON(!list_empty(&vblk->reqs));
549	567
550	568	/* Stop all the virtqueues. */
551	569	vdev->config->reset(vdev);
	570	+
	571	+ flush_work(&vblk->config_work);
552	572
553	573	del_gendisk(vblk->disk);
554	574	blk_cleanup_queue(vblk->disk->queue);