staging: comedi: protect against detach during write operation

The 'write' file operation for comedi devices does not use the main
mutex in the `struct comedi_device` to avoid contention with some ioctls
that may take a while to complete.  Use the `attach_lock` semaphore to
protect against detachment while the 'write' operation is in progress.
This is a `struct rw_semaphore` and we read-lock it to protect against
device detachment.

Note that `comedi_device_cancel_all()` is called during device
detachment, which cancels any ongoing asynchronous commands.  This will
wake up any blocked writers which will then release the `attach_lock`
semaphore and complete the 'write' operation early.

The only time the 'write' file operation does use the main mutex is at
the end of the command when it has to call `do_become_nonbusy()` to mark
the subdevice as no longer busy handling an asynchronous command.  To
avoid deadlock, it has to remove the task from the wait queue and
release the `attach_lock` semaphore before acquiring the main mutex.  It
then needs to confirm that the device is still attached.  Unfortunately,
we do not yet protect against a dynamically allocated `struct
comedi_device` being deleted during the operation.  This will be
addressed by a later patch.

Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

1 files changed, 52 insertions, 9 deletions

diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c
index eafa18efdeea..1ae2bdf08646 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -2023,38 +2023,77 @@ static ssize_t comedi_write(struct file *file, const char __user *buf,
 	DECLARE_WAITQUEUE(wait, current);
 	const unsigned minor = iminor(file_inode(file));
 	struct comedi_device *dev = comedi_dev_from_minor(minor);
+	bool on_wait_queue = false;
+	bool attach_locked;
+	unsigned int old_detach_count;
 
 	if (!dev)
 		return -ENODEV;
 
+	/* Protect against device detachment during operation. */
+	down_read(&dev->attach_lock);
+	attach_locked = true;
+	old_detach_count = dev->detach_count;
+
 	if (!dev->attached) {
 		DPRINTK("no driver configured on comedi%i\n", dev->minor);
-		return -ENODEV;
+		retval = -ENODEV;
+		goto out;
 	}
 
 	s = comedi_write_subdevice(dev, minor);
-	if (!s || !s->async)
-		return -EIO;
+	if (!s || !s->async) {
+		retval = -EIO;
+		goto out;
+	}
 
 	async = s->async;
 
 	if (!s->busy || !nbytes)
-		return 0;
-	if (s->busy != file)
-		return -EACCES;
+		goto out;
+	if (s->busy != file) {
+		retval = -EACCES;
+		goto out;
+	}
 
 	add_wait_queue(&async->wait_head, &wait);
+	on_wait_queue = true;
 	while (nbytes > 0 && !retval) {
 		set_current_state(TASK_INTERRUPTIBLE);
 
 		if (!comedi_is_subdevice_running(s)) {
 			if (count == 0) {
-				mutex_lock(&dev->mutex);
+				struct comedi_subdevice *new_s;
+
 				if (comedi_is_subdevice_in_error(s))
 					retval = -EPIPE;
 				else
 					retval = 0;
-				do_become_nonbusy(dev, s);
+				/*
+				 * To avoid deadlock, cannot acquire dev->mutex
+				 * while dev->attach_lock is held.  Need to
+				 * remove task from the async wait queue before
+				 * releasing dev->attach_lock, as it might not
+				 * be valid afterwards.
+				 */
+				remove_wait_queue(&async->wait_head, &wait);
+				on_wait_queue = false;
+				up_read(&dev->attach_lock);
+				attach_locked = false;
+				mutex_lock(&dev->mutex);
+				/*
+				 * Become non-busy unless things have changed
+				 * behind our back.  Checking dev->detach_count
+				 * is unchanged ought to be sufficient (unless
+				 * there have been 2**32 detaches in the
+				 * meantime!), but check the subdevice pointer
+				 * as well just in case.
+				 */
+				new_s = comedi_write_subdevice(dev, minor);
+				if (dev->attached &&
+				    old_detach_count == dev->detach_count &&
+				    s == new_s && new_s->async == async)
+					do_become_nonbusy(dev, s);
 				mutex_unlock(&dev->mutex);
 			}
 			break;
@@ -2104,8 +2143,12 @@ static ssize_t comedi_write(struct file *file, const char __user *buf,
 		buf += n;
 		break;		/* makes device work like a pipe */
 	}
+out:
+	if (on_wait_queue)
+		remove_wait_queue(&async->wait_head, &wait);
 	set_current_state(TASK_RUNNING);
-	remove_wait_queue(&async->wait_head, &wait);
+	if (attach_locked)
+		up_read(&dev->attach_lock);
 
 	return count ? count : retval;
 }