On Mon, Nov 29, 2004 at 03:51:55PM -0800, Pete Zaitcev wrote: [skip] > On Mon, 29 Nov 2004 20:23:45 +0200 (EET), Meelis Roos <[EMAIL PROTECTED]> > wrote: > > > This > > http://linux.bkbits.net:8080/linux-2.4/cset%40412c6c71gH2KqyBakAeQE-CJpIK3hA?nav=index.html|[EMAIL > > PROTECTED] > > patch in 2.4.28 breaks some usb devices that use concurrent access from > > different processes (like modem_run from speedtouch package but I have > > heard this is not the only one).
Also, this code has an oopsable bug. Consider a program which opens a
device through usbfs and calls USBDEVFS_REAPURB. usbdev_ioctl() will
grab ps->devsem for read, then it will grab ps->dev->exclusive_access
and call proc_reapurb(), which then will release ps->devsem and sleep.
If the device is disconnected while proc_reapurb() is sleeping,
ps->dev will be set to NULL; when the process will wake up,
up(&ps->dev->exclusive_access) in usbdev_ioctl() will oops.
However, USBDEVFS_REAPURB (and some other ioctls) do not really need
the ps->dev->exclusive_access lock, because they do not perform any
operation with the device. Therefore the oops above and the original
problem could be solved by handling the ioctls which do not need the
exclusive_access lock without taking this lock.
This patch (compile tested only) moves handling of USBDEVFS_GETDRIVER,
USBDEVFS_CONNECTINFO, USBDEVFS_REAPURB, USBDEVFS_REAPURBNDELAY,
USBDEVFS_DISCSIGNAL out of the ps->dev->exclusive_access lock.
=======================================================================
Summary: [USB] Fix usbfs exclusive_access locking problems
Signed-off-by: Sergey Vlasov <[EMAIL PROTECTED]>
Adding the exclusive_access semaphore introduced some more problems:
- Some userspace programs (like modem_run from speedtouch package)
use concurrent usbfs access from different processes, which no
longer works after this patch. The main problem is that while the
USBDEVFS_REAPURB handler sleeps waiting for an URB to complete, it
still holds the exclusive_access mutex for the device, locking out
all other processes.
- If the device is removed while proc_reapurb() was waiting,
usbdev_ioctl() will oops on up(&ps->dev->exclusive_access) in the
return path.
This patch solves the above problems by not taking the
exclusive_access mutex for ioctls which do not need it
(USBDEVFS_GETDRIVER, USBDEVFS_CONNECTINFO, USBDEVFS_REAPURB,
USBDEVFS_REAPURBNDELAY, USBDEVFS_DISCSIGNAL).
diff -urNp -x'*~' linux-2.4.29-pre1/drivers/usb/devio.c
linux-2.4.29-pre1-usb-2/drivers/usb/devio.c
--- linux-2.4.29-pre1/drivers/usb/devio.c 2004-11-17 14:54:21 +0300
+++ linux-2.4.29-pre1-usb-2/drivers/usb/devio.c 2004-11-30 14:58:26 +0300
@@ -1146,25 +1146,11 @@ static int proc_ioctl (struct dev_state
return retval;
}
-static int usbdev_ioctl(struct inode *inode, struct file *file, unsigned int
cmd, unsigned long arg)
+static int usbdev_ioctl_exclusive(struct dev_state *ps, struct inode *inode,
+ unsigned int cmd, unsigned long arg)
{
- struct dev_state *ps = (struct dev_state *)file->private_data;
int ret = -ENOIOCTLCMD;
- if (!(file->f_mode & FMODE_WRITE))
- return -EPERM;
- down_read(&ps->devsem);
- if (!ps->dev) {
- up_read(&ps->devsem);
- return -ENODEV;
- }
-
- /*
- * grab device's exclusive_access mutex to prevent its driver from
- * using this device while it is being accessed by us.
- */
- down(&ps->dev->exclusive_access);
-
switch (cmd) {
case USBDEVFS_CONTROL:
ret = proc_control(ps, (void *)arg);
@@ -1194,14 +1180,6 @@ static int usbdev_ioctl(struct inode *in
inode->i_mtime = CURRENT_TIME;
break;
- case USBDEVFS_GETDRIVER:
- ret = proc_getdriver(ps, (void *)arg);
- break;
-
- case USBDEVFS_CONNECTINFO:
- ret = proc_connectinfo(ps, (void *)arg);
- break;
-
case USBDEVFS_SETINTERFACE:
ret = proc_setintf(ps, (void *)arg);
break;
@@ -1220,6 +1198,50 @@ static int usbdev_ioctl(struct inode *in
ret = proc_unlinkurb(ps, (void *)arg);
break;
+ case USBDEVFS_CLAIMINTERFACE:
+ ret = proc_claiminterface(ps, (void *)arg);
+ break;
+
+ case USBDEVFS_RELEASEINTERFACE:
+ ret = proc_releaseinterface(ps, (void *)arg);
+ break;
+
+ case USBDEVFS_IOCTL:
+ ret = proc_ioctl(ps, (void *) arg);
+ break;
+ }
+ return ret;
+}
+
+static int usbdev_ioctl(struct inode *inode, struct file *file,
+ unsigned int cmd, unsigned long arg)
+{
+ struct dev_state *ps = file->private_data;
+ int ret;
+
+ if (!(file->f_mode & FMODE_WRITE))
+ return -EPERM;
+ down_read(&ps->devsem);
+ if (!ps->dev) {
+ up_read(&ps->devsem);
+ return -ENODEV;
+ }
+
+ /*
+ * Some ioctls don't touch the device and can be called without
+ * grabbing its exclusive_access mutex; they are handled in this
+ * switch. Other ioctls which need exclusive_access are handled in
+ * usbdev_ioctl_exclusive().
+ */
+ switch (cmd) {
+ case USBDEVFS_GETDRIVER:
+ ret = proc_getdriver(ps, (void *)arg);
+ break;
+
+ case USBDEVFS_CONNECTINFO:
+ ret = proc_connectinfo(ps, (void *)arg);
+ break;
+
case USBDEVFS_REAPURB:
ret = proc_reapurb(ps, (void *)arg);
break;
@@ -1232,19 +1254,12 @@ static int usbdev_ioctl(struct inode *in
ret = proc_disconnectsignal(ps, (void *)arg);
break;
- case USBDEVFS_CLAIMINTERFACE:
- ret = proc_claiminterface(ps, (void *)arg);
- break;
-
- case USBDEVFS_RELEASEINTERFACE:
- ret = proc_releaseinterface(ps, (void *)arg);
- break;
-
- case USBDEVFS_IOCTL:
- ret = proc_ioctl(ps, (void *) arg);
+ default:
+ down(&ps->dev->exclusive_access);
+ ret = usbdev_ioctl_exclusive(ps, inode, cmd, arg);
+ up(&ps->dev->exclusive_access);
break;
}
- up(&ps->dev->exclusive_access);
up_read(&ps->devsem);
if (ret >= 0)
inode->i_atime = CURRENT_TIME;
pgpBgakS4Zlvt.pgp
Description: PGP signature
