From 45c2bc557ca333a2115bb9f812b026b482cca5fa Mon Sep 17 00:00:00 2001 From: Ian Abbott Date: Fri, 8 Nov 2013 15:03:31 +0000 Subject: [PATCH] staging: comedi: protect against detach during read operation The 'read' 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 'read' 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 readers which will then release the `attach_lock` semaphore and complete the 'read' operation early. The only time the 'read' 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 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 Signed-off-by: Greg Kroah-Hartman --- drivers/staging/comedi/comedi_fops.c | 62 +++++++++++++++++++++------- 1 file changed, 48 insertions(+), 14 deletions(-) diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c index 1ae2bdf08646..fa8da20df753 100644 --- a/drivers/staging/comedi/comedi_fops.c +++ b/drivers/staging/comedi/comedi_fops.c @@ -2162,24 +2162,37 @@ static ssize_t comedi_read(struct file *file, char __user *buf, size_t nbytes, DECLARE_WAITQUEUE(wait, current); const unsigned minor = iminor(file_inode(file)); struct comedi_device *dev = comedi_dev_from_minor(minor); + unsigned int old_detach_count; + bool become_nonbusy = false; + bool attach_locked; 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_read_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); while (nbytes > 0 && !retval) { @@ -2197,13 +2210,11 @@ static ssize_t comedi_read(struct file *file, char __user *buf, size_t nbytes, if (n == 0) { if (!comedi_is_subdevice_running(s)) { - mutex_lock(&dev->mutex); - do_become_nonbusy(dev, s); if (comedi_is_subdevice_in_error(s)) retval = -EPIPE; else retval = 0; - mutex_unlock(&dev->mutex); + become_nonbusy = true; break; } if (file->f_flags & O_NONBLOCK) { @@ -2241,14 +2252,37 @@ static ssize_t comedi_read(struct file *file, char __user *buf, size_t nbytes, buf += n; break; /* makes device work like a pipe */ } - if (comedi_is_subdevice_idle(s)) { + remove_wait_queue(&async->wait_head, &wait); + set_current_state(TASK_RUNNING); + if (become_nonbusy || comedi_is_subdevice_idle(s)) { + struct comedi_subdevice *new_s; + + /* + * To avoid deadlock, cannot acquire dev->mutex + * while dev->attach_lock is held. + */ + up_read(&dev->attach_lock); + attach_locked = false; mutex_lock(&dev->mutex); - if (async->buf_read_count - async->buf_write_count == 0) - do_become_nonbusy(dev, s); + /* + * Check device hasn't become detached 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_read_subdevice(dev, minor); + if (dev->attached && old_detach_count == dev->detach_count && + s == new_s && new_s->async == async) { + if (become_nonbusy || + async->buf_read_count - async->buf_write_count == 0) + do_become_nonbusy(dev, s); + } mutex_unlock(&dev->mutex); } - set_current_state(TASK_RUNNING); - remove_wait_queue(&async->wait_head, &wait); +out: + if (attach_locked) + up_read(&dev->attach_lock); return count ? count : retval; } -- 2.30.2