tjh.dev / kernel
Linux kernel mirror (for testing) git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
kernel os linux
1
fork

Configure Feed

Select the types of activity you want to include in your feed.

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>

authored by

Ian Abbott and committed by
Greg Kroah-Hartman d63cf1ee 45edeece

+68 -13
+68 -13
drivers/comedi/comedi_fops.c
··· 1169 1169 * COMEDI_BUFINFO ioctl 1170 1170 * buffer information 1171 1171 * 1172 + * Note that the comedi device's mutex has not been locked for this ioctl. 1173 + * 1172 1174 * arg: 1173 1175 * pointer to comedi_bufinfo structure 1174 1176 * ··· 1188 1186 struct comedi_async *async; 1189 1187 unsigned int runflags; 1190 1188 int retval = 0; 1189 + unsigned int old_detach_count; 1190 + unsigned int cmd_flags; 1191 1191 bool become_nonbusy = false; 1192 + bool attach_locked; 1192 1193 1193 - lockdep_assert_held(&dev->mutex); 1194 1194 if (copy_from_user(&bi, arg, sizeof(bi))) 1195 1195 return -EFAULT; 1196 1196 1197 - if (bi.subdevice >= dev->n_subdevices) 1198 - return -EINVAL; 1197 + /* Protect against device detachment during operation. */ 1198 + down_read(&dev->attach_lock); 1199 + attach_locked = true; 1200 + old_detach_count = dev->detach_count; 1201 + 1202 + if (!dev->attached) { 1203 + dev_dbg(dev->class_dev, "no driver attached\n"); 1204 + retval = -ENODEV; 1205 + goto out; 1206 + } 1207 + 1208 + if (bi.subdevice >= dev->n_subdevices) { 1209 + retval = -EINVAL; 1210 + goto out; 1211 + } 1199 1212 1200 1213 s = &dev->subdevices[bi.subdevice]; 1201 1214 1202 1215 async = s->async; 1203 1216 1204 - if (!async || s->busy != file) 1205 - return -EINVAL; 1217 + if (!async || s->busy != file) { 1218 + retval = -EINVAL; 1219 + goto out; 1220 + } 1206 1221 1207 1222 runflags = comedi_get_subdevice_runflags(s); 1208 - if (!(async->cmd.flags & CMDF_WRITE)) { 1223 + cmd_flags = async->cmd.flags; 1224 + if (!(cmd_flags & CMDF_WRITE)) { 1209 1225 /* command was set up in "read" direction */ 1210 1226 if (bi.bytes_read) { 1211 1227 _comedi_buf_read_alloc(s, bi.bytes_read); ··· 1263 1243 bi.buf_read_count = async->buf_read_count; 1264 1244 bi.buf_read_ptr = async->buf_read_ptr; 1265 1245 1266 - if (become_nonbusy) 1267 - do_become_nonbusy(dev, s); 1246 + if (become_nonbusy) { 1247 + struct comedi_subdevice *new_s = NULL; 1248 + 1249 + /* 1250 + * To avoid deadlock, cannot acquire dev->mutex 1251 + * while dev->attach_lock is held. 1252 + */ 1253 + up_read(&dev->attach_lock); 1254 + attach_locked = false; 1255 + mutex_lock(&dev->mutex); 1256 + /* 1257 + * Check device hasn't become detached behind our back. 1258 + * Checking dev->detach_count is unchanged ought to be 1259 + * sufficient, but check the subdevice pointer as well, 1260 + * and check the subdevice is still in a suitable state 1261 + * to become non-busy. It should still be "busy" after 1262 + * running an asynchronous commands, which should now have 1263 + * stopped, and for a command in the "read" direction, all 1264 + * available data should have been read. 1265 + */ 1266 + if (dev->attached && old_detach_count == dev->detach_count && 1267 + bi.subdevice < dev->n_subdevices) 1268 + new_s = &dev->subdevices[bi.subdevice]; 1269 + if (s == new_s && new_s->async == async && s->busy == file && 1270 + async->cmd.flags == cmd_flags && 1271 + !comedi_is_subdevice_running(s) && 1272 + ((cmd_flags & CMDF_WRITE) != 0 || 1273 + _comedi_buf_read_n_available(s) == 0)) 1274 + do_become_nonbusy(dev, s); 1275 + mutex_unlock(&dev->mutex); 1276 + } 1277 + 1278 + out: 1279 + if (attach_locked) 1280 + up_read(&dev->attach_lock); 1268 1281 1269 1282 if (retval) 1270 1283 return retval; ··· 2278 2225 struct comedi_device *dev = cfp->dev; 2279 2226 int rc; 2280 2227 2228 + /* Handle COMEDI_BUFINFO without locking the mutex first. */ 2229 + if (cmd == COMEDI_BUFINFO) { 2230 + return do_bufinfo_ioctl(dev, 2231 + (struct comedi_bufinfo __user *)arg, 2232 + file); 2233 + } 2234 + 2281 2235 mutex_lock(&dev->mutex); 2282 2236 2283 2237 /* ··· 2354 2294 rc = do_rangeinfo_ioctl(dev, &it); 2355 2295 break; 2356 2296 } 2357 - case COMEDI_BUFINFO: 2358 - rc = do_bufinfo_ioctl(dev, 2359 - (struct comedi_bufinfo __user *)arg, 2360 - file); 2361 - break; 2362 2297 case COMEDI_LOCK: 2363 2298 rc = do_lock_ioctl(dev, arg, file); 2364 2299 break;