Re: USB ioctl problem in 2.4.28

[Pete Zaitcev]

On Wed, 1 Dec 2004 14:57:41 +0200 (EET), Kaupo Arulo <kaups@xxxxxxxx> wrote:

> We can use a macro, if it is really necessary

No, I prefer it explicit. See the attached patch. I changed Sergey's patch
just a little, so the invalid ioctls are detected outside the lock.

> IMHO it is correct to return -ENOIOCTLCMD right away, not waiting for 
> freeing a mutex.

Returning ENOIOCTLCMD is never correct, BTW, so I fixed that too.

Someone please test this patch with modem_run and let me know if it works.
I think it should be correct, but I would prefer a confirmation before
this goes to Marcelo.

Thank you in advance,
-- Pete

diff -urp -X dontdiff linux-2.4.29/drivers/usb/devio.c linux-2.4.29-usb/drivers/usb/devio.c
--- linux-2.4.29/drivers/usb/devio.c	2004-11-22 23:04:18.000000000 -0800
+++ linux-2.4.29-usb/drivers/usb/devio.c	2005-01-31 17:57:45.342576912 -0800
@@ -1132,6 +1132,8 @@ static int proc_ioctl (struct dev_state 
 			/* ifno might usefully be passed ... */
                        retval = driver->ioctl (ps->dev, ctrl.ioctl_code, buf);
 			/* size = min_t(int, size, retval)? */
+			if (retval == -ENOIOCTLCMD)
+				retval = -ENOTTY;
                }
 	}
 
@@ -1146,24 +1148,10 @@ 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);
+	int ret;
 
 	switch (cmd) {
 	case USBDEVFS_CONTROL:
@@ -1194,14 +1182,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 +1200,53 @@ 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;
+
+	default:
+		ret = -ENOTTY;
+	}
+	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 +1259,28 @@ static int usbdev_ioctl(struct inode *in
 		ret = proc_disconnectsignal(ps, (void *)arg);
 		break;
 
+	case USBDEVFS_CONTROL:
+	case USBDEVFS_BULK:
+	case USBDEVFS_RESETEP:
+	case USBDEVFS_RESET:
+	case USBDEVFS_CLEAR_HALT:
+	case USBDEVFS_SETINTERFACE:
+	case USBDEVFS_SETCONFIGURATION:
+	case USBDEVFS_SUBMITURB:
+	case USBDEVFS_DISCARDURB:
 	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);
+		ret = -ERESTARTSYS;
+		if (down_interruptible(&ps->dev->exclusive_access) == 0) {
+			ret = usbdev_ioctl_exclusive(ps, inode, cmd, arg);
+			up(&ps->dev->exclusive_access);
+		}
 		break;
+
+	default:
+		ret = -ENOTTY;
 	}
-	up(&ps->dev->exclusive_access);
 	up_read(&ps->devsem);
 	if (ret >= 0)
 		inode->i_atime = CURRENT_TIME;
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/