[PATCH] dm: 'wait for event' race

From: Joe Thornber <thornber@sistina.com>

There was a race associated with the 'wait for a significant event'
functionality.

Basically userland could read the status table, then wait for another
event, but the event it was waiting for could have occurred in the gap
between reading and waiting.

To solve this we assign identifiers to events, in order to successfully
wait for an event both userland and the kernel driver must be in agreement
about what the last event identifier was.  If they don't agree the wait
call will return immediately, allowing userland to re-read the status and
see what it missed.

The new ioctl interface will use this properly.

drivers/md/dm-ioctl.c

@@ -705,7 +705,6 @@ static int get_status(struct dm_ioctl *param, struct dm_ioctl *user)
 static int wait_device_event(struct dm_ioctl *param, struct dm_ioctl *user)
 {
 	struct mapped_device *md;
-	struct dm_table *table;
 	DECLARE_WAITQUEUE(wq, current);
 
 	md = find_device(param);
@@ -724,12 +723,12 @@ static int wait_device_event(struct dm_ioctl *param, struct dm_ioctl *user)
 	 * Wait for a notification event
 	 */
 	set_current_state(TASK_INTERRUPTIBLE);
-	table = dm_get_table(md);
-	dm_table_add_wait_queue(table, &wq);
-	dm_table_put(table);
-	dm_put(md);
-
-	schedule();
+ 	if (!dm_add_wait_queue(md, &wq, dm_get_event_nr(md))) {
+ 		schedule();
+ 		dm_remove_wait_queue(md, &wq);
+ 	}
+  	set_current_state(TASK_RUNNING);
+ 	dm_put(md);
 
       out:
 	return results_to_user(user, param, NULL, 0);

drivers/md/dm-table.c

@@ -48,11 +48,9 @@ struct dm_table {
 	 */
 	struct io_restrictions limits;
 
-	/*
-	 * A waitqueue for processes waiting for something
-	 * interesting to happen to this table.
-	 */
-	wait_queue_head_t eventq;
+	/* events get handed up using this callback */
+	void (*event_fn)(void *);
+	void *event_context;
 };
 
 /*
@@ -222,7 +220,6 @@ int dm_table_create(struct dm_table **result, int mode)
 		return -ENOMEM;
 	}
 
-	init_waitqueue_head(&t->eventq);
 	t->mode = mode;
 	*result = t;
 	return 0;
@@ -243,9 +240,6 @@ void table_destroy(struct dm_table *t)
 {
 	unsigned int i;
 
-	/* destroying the table counts as an event */
-	dm_table_event(t);
-
 	/* free the indexes (see dm_table_complete) */
 	if (t->depth >= 2)
 		vfree(t->index[t->depth - 2]);
@@ -694,9 +688,22 @@ int dm_table_complete(struct dm_table *t)
 	return r;
 }
 
+static spinlock_t _event_lock = SPIN_LOCK_UNLOCKED;
+void dm_table_event_callback(struct dm_table *t,
+			     void (*fn)(void *), void *context)
+{
+	spin_lock_irq(&_event_lock);
+	t->event_fn = fn;
+	t->event_context = context;
+	spin_unlock_irq(&_event_lock);
+}
+
 void dm_table_event(struct dm_table *t)
 {
-	wake_up_interruptible(&t->eventq);
+	spin_lock(&_event_lock);
+	if (t->event_fn)
+		t->event_fn(t->event_context);
+	spin_unlock(&_event_lock);
 }
 
 sector_t dm_table_get_size(struct dm_table *t)
@@ -761,11 +768,6 @@ int dm_table_get_mode(struct dm_table *t)
 	return t->mode;
 }
 
-void dm_table_add_wait_queue(struct dm_table *t, wait_queue_t *wq)
-{
-	add_wait_queue(&t->eventq, wq);
-}
-
 void dm_table_suspend_targets(struct dm_table *t)
 {
 	int i;

drivers/md/dm.c

@@ -62,6 +62,12 @@ struct mapped_device {
 	 * io objects are allocated from here.
 	 */
 	mempool_t *io_pool;
+
+	/*
+	 * Event handling.
+	 */
+	uint32_t event_nr;
+	wait_queue_head_t eventq;
 };
 
 #define MIN_IOS 256
@@ -618,6 +624,8 @@ static struct mapped_device *alloc_dev(unsigned int minor, int persistent)
 
 	atomic_set(&md->pending, 0);
 	init_waitqueue_head(&md->wait);
+	init_waitqueue_head(&md->eventq);
+
 	return md;
 }
 
@@ -633,6 +641,16 @@ static void free_dev(struct mapped_device *md)
 /*
  * Bind a table to the device.
  */
+static void event_callback(void *context)
+{
+	struct mapped_device *md = (struct mapped_device *) context;
+
+	down_write(&md->lock);
+	md->event_nr++;
+	wake_up_interruptible(&md->eventq);
+	up_write(&md->lock);
+}
+
 static int __bind(struct mapped_device *md, struct dm_table *t)
 {
 	request_queue_t *q = &md->queue;
@@ -644,6 +662,8 @@ static int __bind(struct mapped_device *md, struct dm_table *t)
 	if (size == 0)
 		return 0;
 
+	dm_table_event_callback(md->map, event_callback, md);
+
 	dm_table_get(t);
 	dm_table_set_restrictions(t, q);
 	return 0;
@@ -651,6 +671,7 @@ static int __bind(struct mapped_device *md, struct dm_table *t)
 
 static void __unbind(struct mapped_device *md)
 {
+	dm_table_event_callback(md->map, NULL, NULL);
 	dm_table_put(md->map);
 	md->map = NULL;
 	set_capacity(md->disk, 0);
@@ -819,6 +840,42 @@ int dm_resume(struct mapped_device *md)
 	return 0;
 }
 
+/*-----------------------------------------------------------------
+ * Event notification.
+ *---------------------------------------------------------------*/
+uint32_t dm_get_event_nr(struct mapped_device *md)
+{
+	uint32_t r;
+
+	down_read(&md->lock);
+	r = md->event_nr;
+	up_read(&md->lock);
+
+	return r;
+}
+
+int dm_add_wait_queue(struct mapped_device *md, wait_queue_t *wq,
+		      uint32_t event_nr)
+{
+	down_write(&md->lock);
+	if (event_nr != md->event_nr) {
+		up_write(&md->lock);
+		return 1;
+	}
+
+	add_wait_queue(&md->eventq, wq);
+	up_write(&md->lock);
+
+	return 0;
+}
+
+void dm_remove_wait_queue(struct mapped_device *md, wait_queue_t *wq)
+{
+	down_write(&md->lock);
+	remove_wait_queue(&md->eventq, wq);
+	up_write(&md->lock);
+}
+
 /*
  * The gendisk is only valid as long as you have a reference
  * count on 'md'.

drivers/md/dm.h

@@ -78,6 +78,14 @@ int dm_swap_table(struct mapped_device *md, struct dm_table *t);
  */
 struct dm_table *dm_get_table(struct mapped_device *md);
 
+/*
+ * Event functions.
+ */
+uint32_t dm_get_event_nr(struct mapped_device *md);
+int dm_add_wait_queue(struct mapped_device *md, wait_queue_t *wq,
+		      uint32_t event_nr);
+void dm_remove_wait_queue(struct mapped_device *md, wait_queue_t *wq);
+
 /*
  * Info functions.
  */
@@ -96,6 +104,8 @@ void dm_table_put(struct dm_table *t);
 int dm_table_add_target(struct dm_table *t, const char *type,
 			sector_t start,	sector_t len, char *params);
 int dm_table_complete(struct dm_table *t);
+void dm_table_event_callback(struct dm_table *t,
+			     void (*fn)(void *), void *context);
 void dm_table_event(struct dm_table *t);
 sector_t dm_table_get_size(struct dm_table *t);
 struct dm_target *dm_table_get_target(struct dm_table *t, unsigned int index);
@@ -104,7 +114,6 @@ void dm_table_set_restrictions(struct dm_table *t, struct request_queue *q);
 unsigned int dm_table_get_num_targets(struct dm_table *t);
 struct list_head *dm_table_get_devices(struct dm_table *t);
 int dm_table_get_mode(struct dm_table *t);
-void dm_table_add_wait_queue(struct dm_table *t, wait_queue_t *wq);
 void dm_table_suspend_targets(struct dm_table *t);
 void dm_table_resume_targets(struct dm_table *t);