| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20251205131332.16672-1-abbotti@mev.co.uk>
Date: Fri, 5 Dec 2025 13:13:32 +0000
From: Ian Abbott <abbotti@....co.uk>
To: linux-kernel@...r.kernel.org
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Ian Abbott <abbotti@....co.uk>,
H Hartley Sweeten <hsweeten@...ionengravers.com>
Subject: [PATCH] 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@....co.uk>
---
drivers/comedi/comedi_fops.c | 81 ++++++++++++++++++++++++++++++------
1 file changed, 68 insertions(+), 13 deletions(-)
diff --git a/drivers/comedi/comedi_fops.c b/drivers/comedi/comedi_fops.c
index 657c98cd723e..3fea34e69052 100644
--- 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;
--
2.51.0
Powered by blists - more mailing lists