comedi: don't use mutex for COMEDI_BUFINFO ioctl

The main mutex in a comedi device can get held for quite a while when
processing comedi instructions, so for performance reasons, the "read",
"write", and "poll" file operations do not use it; they use the
`attach_lock` rwsemaphore to protect against the comedi device becoming
detached at an inopportune moment.  As an alternative to using the
"read" and "write" operations, user-space can mmap the data buffer and
use the `COMEDI_BUFINFO` ioctl to manage data transfer through the
buffer.  However, the "ioctl" file handler currently locks the main
mutex for all ioctl commands.  Make the handling of the `COMEDI_BUFINFO`
an exception, using the `attach_lock` rwsemaphore during normal
operation.  However, before it calls `do_become_nonbusy()` at the end of
acquisition, it does need to lock the main mutex, but it needs to unlock
the `attach_lock` rwsemaphore first to avoid deadlock.  After locking
the main mutex, it needs to check that it is still in a suitable state
to become non-busy, because things may have changed while unlocked.

Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
Link: https://patch.msgid.link/20251205131332.16672-1-abbotti@mev.co.uk
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

diff --git a/drivers/comedi/comedi_fops.c b/drivers/comedi/comedi_fops.c
index 657c98cd723e05169f0a34a28692650da0c0b91c..3fea34e6905209f124881f1d44eebdc079689f35 100644 (file)
--- a/drivers/comedi/comedi_fops.c
+++ b/drivers/comedi/comedi_fops.c
@@ -1169,6 +1169,8 @@ static int do_chaninfo_ioctl(struct comedi_device *dev,
  * COMEDI_BUFINFO ioctl
  * buffer information
  *
+ * Note that the comedi device's mutex has not been locked for this ioctl.
+ *
  * arg:
  *     pointer to comedi_bufinfo structure
  *
@@ -1186,24 +1188,42 @@ static int do_bufinfo_ioctl(struct comedi_device *dev,
        struct comedi_async *async;
        unsigned int runflags;
        int retval = 0;
+       unsigned int old_detach_count;
+       unsigned int cmd_flags;
        bool become_nonbusy = false;
+       bool attach_locked;
 
-       lockdep_assert_held(&dev->mutex);
        if (copy_from_user(&bi, arg, sizeof(bi)))
                return -EFAULT;
 
-       if (bi.subdevice >= dev->n_subdevices)
-               return -EINVAL;
+       /* Protect against device detachment during operation. */
+       down_read(&dev->attach_lock);
+       attach_locked = true;
+       old_detach_count = dev->detach_count;
+
+       if (!dev->attached) {
+               dev_dbg(dev->class_dev, "no driver attached\n");
+               retval = -ENODEV;
+               goto out;
+       }
+
+       if (bi.subdevice >= dev->n_subdevices) {
+               retval = -EINVAL;
+               goto out;
+       }
 
        s = &dev->subdevices[bi.subdevice];
 
        async = s->async;
 
-       if (!async || s->busy != file)
-               return -EINVAL;
+       if (!async || s->busy != file) {
+               retval = -EINVAL;
+               goto out;
+       }
 
        runflags = comedi_get_subdevice_runflags(s);
-       if (!(async->cmd.flags & CMDF_WRITE)) {
+       cmd_flags = async->cmd.flags;
+       if (!(cmd_flags & CMDF_WRITE)) {
                /* command was set up in "read" direction */
                if (bi.bytes_read) {
                        _comedi_buf_read_alloc(s, bi.bytes_read);
@@ -1243,8 +1263,41 @@ static int do_bufinfo_ioctl(struct comedi_device *dev,
        bi.buf_read_count = async->buf_read_count;
        bi.buf_read_ptr = async->buf_read_ptr;
 
-       if (become_nonbusy)
-               do_become_nonbusy(dev, s);
+       if (become_nonbusy) {
+               struct comedi_subdevice *new_s = NULL;
+
+               /*
+                * 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);
+               /*
+                * Check device hasn't become detached behind our back.
+                * Checking dev->detach_count is unchanged ought to be
+                * sufficient, but check the subdevice pointer as well,
+                * and check the subdevice is still in a suitable state
+                * to become non-busy.  It should still be "busy" after
+                * running an asynchronous commands, which should now have
+                * stopped, and for a command in the "read" direction, all
+                * available data should have been read.
+                */
+               if (dev->attached && old_detach_count == dev->detach_count &&
+                   bi.subdevice < dev->n_subdevices)
+                       new_s = &dev->subdevices[bi.subdevice];
+               if (s == new_s && new_s->async == async && s->busy == file &&
+                   async->cmd.flags == cmd_flags &&
+                   !comedi_is_subdevice_running(s) &&
+                   ((cmd_flags & CMDF_WRITE) != 0 ||
+                    _comedi_buf_read_n_available(s) == 0))
+                       do_become_nonbusy(dev, s);
+               mutex_unlock(&dev->mutex);
+       }
+
+out:
+       if (attach_locked)
+               up_read(&dev->attach_lock);
 
        if (retval)
                return retval;
@@ -2225,6 +2278,13 @@ static long comedi_unlocked_ioctl(struct file *file, unsigned int cmd,
        struct comedi_device *dev = cfp->dev;
        int rc;
 
+       /* Handle COMEDI_BUFINFO without locking the mutex first. */
+       if (cmd == COMEDI_BUFINFO) {
+               return do_bufinfo_ioctl(dev,
+                                       (struct comedi_bufinfo __user *)arg,
+                                       file);
+       }
+
        mutex_lock(&dev->mutex);
 
        /*
@@ -2294,11 +2354,6 @@ static long comedi_unlocked_ioctl(struct file *file, unsigned int cmd,
                        rc = do_rangeinfo_ioctl(dev, &it);
                break;
        }
-       case COMEDI_BUFINFO:
-               rc = do_bufinfo_ioctl(dev,
-                                     (struct comedi_bufinfo __user *)arg,
-                                     file);
-               break;
        case COMEDI_LOCK:
                rc = do_lock_ioctl(dev, arg, file);
                break;