Re: Bug with USB proc_bulk in 2.4 kernel
From: Pete Zaitcev
Date: Sun Jul 30 2006 - 03:35:16 EST
On Thu, 27 Jul 2006 15:21:37 -0700, Benjamin Cherian <benjamin.cherian.kernel@xxxxxxxxx> wrote:
> On Monday 24 July 2006 23:07, Pete Zaitcev wrote:
> > Anyway, please test the attached patch. Does it do what you want?
>
> Sorry to say that it doesnt. When calling usb_get_string_simple in libusb,
> the program segfaults with the patched kernel. I believe that
> usb_get_string_simple eventually calls usbdev_ioctl.
Indeed, I should've tested better, sorry. How about now?
-- Pete
diff -urp -X dontdiff linux-2.4.32/drivers/usb/devices.c linux-2.4.32-wk/drivers/usb/devices.c
--- linux-2.4.32/drivers/usb/devices.c 2004-11-17 03:54:21.000000000 -0800
+++ linux-2.4.32-wk/drivers/usb/devices.c 2006-07-24 22:30:54.000000000 -0700
@@ -392,7 +392,7 @@ static char *usb_dump_desc(char *start,
* Grab device's exclusive_access mutex to prevent its driver or
* devio from using this device while we are accessing it.
*/
- down (&dev->exclusive_access);
+ usb_excl_lock(dev, 3, 0);
start = usb_dump_device_descriptor(start, end, &dev->descriptor);
@@ -411,7 +411,7 @@ static char *usb_dump_desc(char *start,
}
out:
- up (&dev->exclusive_access);
+ usb_excl_unlock(dev, 3);
return start;
}
diff -urp -X dontdiff linux-2.4.32/drivers/usb/devio.c linux-2.4.32-wk/drivers/usb/devio.c
--- linux-2.4.32/drivers/usb/devio.c 2006-04-13 19:02:30.000000000 -0700
+++ linux-2.4.32-wk/drivers/usb/devio.c 2006-07-30 00:27:13.000000000 -0700
@@ -623,7 +623,12 @@ static int proc_bulk(struct dev_state *p
free_page((unsigned long)tbuf);
return -EINVAL;
}
+ if (usb_excl_lock(dev, 1, 1) != 0) {
+ free_page((unsigned long)tbuf);
+ return -ERESTARTSYS;
+ }
i = usb_bulk_msg(dev, pipe, tbuf, len1, &len2, tmo);
+ usb_excl_unlock(dev, 1);
if (!i && len2) {
if (copy_to_user(bulk.data, tbuf, len2)) {
free_page((unsigned long)tbuf);
@@ -637,7 +642,12 @@ static int proc_bulk(struct dev_state *p
return -EFAULT;
}
}
+ if (usb_excl_lock(dev, 2, 1) != 0) {
+ free_page((unsigned long)tbuf);
+ return -ERESTARTSYS;
+ }
i = usb_bulk_msg(dev, pipe, tbuf, len1, &len2, tmo);
+ usb_excl_unlock(dev, 2);
}
free_page((unsigned long)tbuf);
if (i < 0) {
@@ -1160,12 +1170,6 @@ static int usbdev_ioctl_exclusive(struct
inode->i_mtime = CURRENT_TIME;
break;
- case USBDEVFS_BULK:
- ret = proc_bulk(ps, (void *)arg);
- if (ret >= 0)
- inode->i_mtime = CURRENT_TIME;
- break;
-
case USBDEVFS_RESETEP:
ret = proc_resetep(ps, (void *)arg);
if (ret >= 0)
@@ -1259,8 +1263,13 @@ static int usbdev_ioctl(struct inode *in
ret = proc_disconnectsignal(ps, (void *)arg);
break;
- case USBDEVFS_CONTROL:
case USBDEVFS_BULK:
+ ret = proc_bulk(ps, (void *)arg);
+ if (ret >= 0)
+ inode->i_mtime = CURRENT_TIME;
+ break;
+
+ case USBDEVFS_CONTROL:
case USBDEVFS_RESETEP:
case USBDEVFS_RESET:
case USBDEVFS_CLEAR_HALT:
@@ -1272,9 +1281,9 @@ static int usbdev_ioctl(struct inode *in
case USBDEVFS_RELEASEINTERFACE:
case USBDEVFS_IOCTL:
ret = -ERESTARTSYS;
- if (down_interruptible(&ps->dev->exclusive_access) == 0) {
+ if (usb_excl_lock(ps->dev, 3, 1) == 0) {
ret = usbdev_ioctl_exclusive(ps, inode, cmd, arg);
- up(&ps->dev->exclusive_access);
+ usb_excl_unlock(ps->dev, 3);
}
break;
diff -urp -X dontdiff linux-2.4.32/drivers/usb/storage/transport.c linux-2.4.32-wk/drivers/usb/storage/transport.c
--- linux-2.4.32/drivers/usb/storage/transport.c 2005-04-03 18:42:19.000000000 -0700
+++ linux-2.4.32-wk/drivers/usb/storage/transport.c 2006-07-24 22:58:58.000000000 -0700
@@ -628,16 +628,16 @@ void usb_stor_invoke_transport(Scsi_Cmnd
int result;
/*
- * Grab device's exclusive_access mutex to prevent libusb/usbfs from
+ * Grab device's exclusive access lock to prevent libusb/usbfs from
* sending out a command in the middle of ours (if libusb sends a
* get_descriptor or something on pipe 0 after our CBW and before
* our CSW, and then we get a stall, we have trouble).
*/
- down(&(us->pusb_dev->exclusive_access));
+ usb_excl_lock(us->pusb_dev, 3, 0);
/* send the command to the transport layer */
result = us->transport(srb, us);
- up(&(us->pusb_dev->exclusive_access));
+ usb_excl_unlock(us->pusb_dev, 3);
/* if the command gets aborted by the higher layers, we need to
* short-circuit all other processing
@@ -757,9 +757,9 @@ void usb_stor_invoke_transport(Scsi_Cmnd
srb->use_sg = 0;
/* issue the auto-sense command */
- down(&(us->pusb_dev->exclusive_access));
+ usb_excl_lock(us->pusb_dev, 3, 0);
temp_result = us->transport(us->srb, us);
- up(&(us->pusb_dev->exclusive_access));
+ usb_excl_unlock(us->pusb_dev, 3);
/* let's clean up right away */
srb->request_buffer = old_request_buffer;
diff -urp -X dontdiff linux-2.4.32/drivers/usb/usb.c linux-2.4.32-wk/drivers/usb/usb.c
--- linux-2.4.32/drivers/usb/usb.c 2004-11-17 03:54:21.000000000 -0800
+++ linux-2.4.32-wk/drivers/usb/usb.c 2006-07-24 22:41:19.000000000 -0700
@@ -989,7 +989,8 @@ struct usb_device *usb_alloc_dev(struct
INIT_LIST_HEAD(&dev->filelist);
init_MUTEX(&dev->serialize);
- init_MUTEX(&dev->exclusive_access);
+ spin_lock_init(&dev->excl_lock);
+ init_waitqueue_head(&dev->excl_wait);
dev->bus->op->allocate(dev);
@@ -2380,6 +2381,59 @@ struct list_head *usb_bus_get_list(void)
}
#endif
+int usb_excl_lock(struct usb_device *dev, unsigned int type, int interruptible)
+{
+ DECLARE_WAITQUEUE(waita, current);
+
+ add_wait_queue(&dev->excl_wait, &waita);
+ if (interruptible)
+ set_current_state(TASK_INTERRUPTIBLE);
+ else
+ set_current_state(TASK_UNINTERRUPTIBLE);
+
+ for (;;) {
+ spin_lock_irq(&dev->excl_lock);
+ switch (type) {
+ case 1: /* 1 - read */
+ case 2: /* 2 - write */
+ case 3: /* 3 - control: excludes both read and write */
+ if ((dev->excl_type & type) == 0) {
+ dev->excl_type |= type;
+ spin_unlock_irq(&dev->excl_lock);
+ set_current_state(TASK_RUNNING);
+ remove_wait_queue(&dev->excl_wait, &waita);
+ return 0;
+ }
+ break;
+ default:
+ spin_unlock_irq(&dev->excl_lock);
+ return -EINVAL;
+ }
+ spin_unlock_irq(&dev->excl_lock);
+
+ if (interruptible) {
+ schedule();
+ if (signal_pending(current)) {
+ remove_wait_queue(&dev->excl_wait, &waita);
+ return 1;
+ }
+ set_current_state(TASK_INTERRUPTIBLE);
+ } else {
+ schedule();
+ set_current_state(TASK_UNINTERRUPTIBLE);
+ }
+ }
+}
+
+void usb_excl_unlock(struct usb_device *dev, unsigned int type)
+{
+ unsigned long flags;
+
+ spin_lock_irqsave(&dev->excl_lock, flags);
+ dev->excl_type &= ~type;
+ wake_up(&dev->excl_wait);
+ spin_unlock_irqrestore(&dev->excl_lock, flags);
+}
/*
* Init
diff -urp -X dontdiff linux-2.4.32/include/linux/usb.h linux-2.4.32-wk/include/linux/usb.h
--- linux-2.4.32/include/linux/usb.h 2005-12-22 17:08:01.000000000 -0800
+++ linux-2.4.32-wk/include/linux/usb.h 2006-07-24 22:30:02.000000000 -0700
@@ -828,8 +828,19 @@ struct usb_device {
atomic_t refcnt; /* Reference count */
struct semaphore serialize;
- struct semaphore exclusive_access; /* prevent driver & proc accesses */
- /* from overlapping cmds at device */
+
+ /*
+ * This is our custom open-coded lock, similar to r/w locks in concept.
+ * It prevents drivers and /proc access from simultaneous access.
+ * Type:
+ * 0 - unlocked
+ * 1 - locked for reads
+ * 2 - locked for writes
+ * 3 - locked for everything
+ */
+ wait_queue_head_t excl_wait;
+ spinlock_t excl_lock;
+ unsigned excl_type;
unsigned int toggle[2]; /* one bit for each endpoint ([0] = IN, [1] = OUT) */
unsigned int halted[2]; /* endpoint halts; one bit per endpoint # & direction; */
@@ -904,6 +915,8 @@ extern void usb_destroy_configuration(st
int usb_get_current_frame_number (struct usb_device *usb_dev);
+int usb_excl_lock(struct usb_device *dev, unsigned int type, int interruptible);
+void usb_excl_unlock(struct usb_device *dev, unsigned int type);
/**
* usb_make_path - returns stable device path in the usb tree
-
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/