[linux-usb-devel] USB: FIx locks and urb->status in adutux

[linux-usb-devel] USB: FIx locks and urb->status in adutux

Post by Oliver Neu » Wed, 24 Oct 2007 18:40:09


m Dienstag 23 Oktober 2007 schrieb Pete Zaitcev:


Why bother? Simply call usb_kill_urb() unconditionally.


This leaves a race here:

while (count > 0) {
spin_lock_irqsave(&dev->buflock, flags);
if (!dev->out_urb_finished) {
spin_unlock_irqrestore(&dev->buflock, flags);

timeout = COMMAND_TIMEOUT;
while (timeout > 0) {
if (signal_pending(current)) {
dbg(1," %s : interrupted", __FUNCTION__);
retval = -EINTR;
goto exit;
}
mutex_unlock(&dev->mtx);
timeout = interruptible_sleep_on_timeout(&dev->write_wait, timeout);

You can detect that the urb has not finished but the notification happens before
you go to sleep.


This is racy:
dev = usb_get_intfdata(interface);
if (!dev) {
retval = -ENODEV;
goto exit_no_device;
}

if ((retval = mutex_lock_interruptible(&adutux_mutex))) {
dbg(2, "%s : mutex lock failed", __FUNCTION__);
goto exit_no_device;
}

You need to manipulate intfdata under lock, or disconnect will
happily free the datastructures.


You should set file->private_data to NULL in the error case.

[..]

I find it a bit silly to bother with _irqsave if you call schedule_timeout() in the
next line.

Regards
Oliver
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to XXXX@XXXXX.COM
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
 
 
 

1. [linux-usb-devel] [PATCH 01/10] usb-serial: URB write locking macros.

2. [linux-usb-devel] [PATCH 7/9] USB usbfs: destroy submitted urbs only on the disconnected interface


Quite apart from the stylistic questions about sanity tests and so on,
this code contains a bug. It wasn't introduced by your patch; it was
there from before and I should have caught it earlier, along with a few
others.

The real problem is that the code in devio.c doesn't make a clear visual
distinction between interface number (i.e., desc.bInterfaceNumber) and
interface index (i.e., dev->actconfig->interface[index]). The two values
do not have to agree.

The claimintf(), releaseintf(), and checkintf() routines take an index as
argument, and the ifclaimed bitvector uses the same index. findintfif()
takes a number and returns the corresponding index, duplicating much of
the functionality of usb_ifnum_to_if(). Likewise, findintfep() returns an
index.

The code here in driver_disconnect() uses a number where it needs to use
an index.

Similarly, there's a typo in proc_releaseinterface(); the second argument
it passes to releaseintf() should be ret, not intf.

And in proc_submiturb(), the value stored in as->intf is an index when it
should be an interface number. Or possibly it could remain an index, but
then the value passed to destroy_async_on_interface() by
proc_releaseinterface() should be the index and not the number.

Alan Stern

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to XXXX@XXXXX.COM
More majordomo info at http://www.yqcomputer.com/
Please read the FAQ at http://www.yqcomputer.com/

3. [linux-usb-devel] [PATCH 7/9] USB usbfs: destroy submitted urbs only on the disconnected interface

4. [linux-usb-devel] [PATCH 2/2] usbatm: Detect usb device shutdown and ignore failed urbs.

5. [linux-usb-devel] [PATCH 2/2] usbatm: Detect usb device shutdown and ignore failed urbs.

6. [linux-usb-devel] status of the USB w9968cf.c driver in kernel 2.6?

7. [linux-usb-devel] status of the USB w9968cf.c driver in kernel 2.6?

8. [linux-usb-devel] status of the USB ov511.c driver in kernel 2.6?

9. [linux-usb-devel] [PATCH 00/10] usb-serial: Switches from spin lock to atomic_t.

10. [linux-usb-devel] [PATCH 00/10] usb-serial: Switches from spin lock to atomic_t.

11. [linux-usb-devel] [PATCH 00/10] usb-serial: Switches from spin lock to atomic_t.

12. [linux-usb-devel] [GIT PATCH] USB fixes for 2.6.20-rc5

13. [linux-usb-devel] [PATCH] USB: Fix masking bug initialization of Freescale EHCI controller

14. [linux-usb-devel] [PATCH] usb: microtek possible memleak fix [second try]

15. [linux-usb-devel] [PATCH] usb: microtek possible memleak fix