Messages in this thread Patch in this message | | | From | Oliver Neukum <> | Subject | Re: [linux-usb-devel] USB: FIx locks and urb->status in adutux | Date | Tue, 23 Oct 2007 11:38:37 +0200 |
| |
Am Dienstag 23 Oktober 2007 schrieb Pete Zaitcev: > Two main issues fixed here are: > - An improper use of in-struct lock to protect an open count > - Use of urb status for -EINPROGRESS
> + /* XXX Anchor these instead */ > + spin_lock_irqsave(&dev->buflock, flags); > + if (!dev->read_urb_finished) { > + spin_unlock_irqrestore(&dev->buflock, flags); > + usb_kill_urb(dev->interrupt_in_urb); > + } else > + spin_unlock_irqrestore(&dev->buflock, flags); > + > + spin_lock_irqsave(&dev->buflock, flags); > + if (!dev->out_urb_finished) { > + spin_unlock_irqrestore(&dev->buflock, flags); > + usb_kill_urb(dev->interrupt_out_urb); > + } else > + spin_unlock_irqrestore(&dev->buflock, flags);
Why bother? Simply call usb_kill_urb() unconditionally.
> exit: > + mutex_unlock(&dev->mtx); > dbg(2," %s : leave", __FUNCTION__); > } > > @@ -162,8 +182,6 @@ static void adu_delete(struct adu_device *dev) > { > dbg(2, "%s enter", __FUNCTION__); > > - adu_abort_transfers(dev); > - > /* free data structures */ > usb_free_urb(dev->interrupt_in_urb); > usb_free_urb(dev->interrupt_out_urb); > @@ -239,7 +257,10 @@ static void adu_interrupt_out_callback(struct urb *urb) > goto exit; > } > > + spin_lock(&dev->buflock); > + dev->out_urb_finished = 1; > wake_up_interruptible(&dev->write_wait); > + spin_unlock(&dev->buflock);
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.
> exit: > > adu_debug_data(5, __FUNCTION__, urb->actual_length, > @@ -272,13 +293,11 @@ static int adu_open(struct inode *inode, struct file *file) > goto exit_no_device; > } > > - /* lock this device */ > - if ((retval = mutex_lock_interruptible(&dev->mtx))) { > + if ((retval = mutex_lock_interruptible(&adutux_mutex))) { > dbg(2, "%s : mutex lock failed", __FUNCTION__); > goto exit_no_device; > }
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.
> > - /* increment our usage count for the device */ > ++dev->open_count; > dbg(2,"%s : open count %d", __FUNCTION__, dev->open_count); > > @@ -297,13 +316,14 @@ static int adu_open(struct inode *inode, struct file *file) > le16_to_cpu(dev->interrupt_in_endpoint->wMaxPacketSize), > adu_interrupt_in_callback, dev, > dev->interrupt_in_endpoint->bInterval); > - /* dev->interrupt_in_urb->transfer_flags |= URB_ASYNC_UNLINK; */ > dev->read_urb_finished = 0; > retval = usb_submit_urb(dev->interrupt_in_urb, GFP_KERNEL); > - if (retval) > + if (retval) { > + dev->read_urb_finished = 1; > --dev->open_count; > + }
You should set file->private_data to NULL in the error case.
[..] > @@ -486,10 +495,14 @@ static ssize_t adu_read(struct file *file, __user char *buffer, size_t count, > /* we wait for I/O to complete */ > set_current_state(TASK_INTERRUPTIBLE); > add_wait_queue(&dev->read_wait, &wait); > - if (!dev->read_urb_finished) > + spin_lock_irqsave(&dev->buflock, flags); > + if (!dev->read_urb_finished) { > + spin_unlock_irqrestore(&dev->buflock, flags); > timeout = schedule_timeout(COMMAND_TIMEOUT);
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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |