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

[Oliver Neukum <oliver@neukum.org>]

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