LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Oliver Neukum <oliver@neukum.org>
To: linux-usb-devel@lists.sourceforge.net
Cc: Pete Zaitcev <zaitcev@redhat.com>,
	greg@kroah.com, linux-kernel@vger.kernel.org,
	vitalivanov@gmail.com, netwiz@crc.id.au
Subject: Re: [linux-usb-devel] USB: FIx locks and urb->status in adutux
Date: Tue, 23 Oct 2007 11:38:37 +0200	[thread overview]
Message-ID: <200710231138.38266.oliver@neukum.org> (raw)
In-Reply-To: <20071022203447.db6d7950.zaitcev@redhat.com>

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

  reply	other threads:[~2007-10-23  9:38 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2007-10-23  3:34 Pete Zaitcev
2007-10-23  9:38 ` Oliver Neukum [this message]
2007-10-23 21:38   ` Pete Zaitcev
2007-10-24 14:04     ` Oliver Neukum
2007-10-24  1:53   ` Pete Zaitcev
2007-10-24 14:09     ` Vitaliy Ivanov
2007-10-25  3:20       ` Pete Zaitcev
2007-10-25  3:25       ` Pete Zaitcev
     [not found]         ` <200710251403.48688.oliver@neukum.org>
2007-10-25 16:38           ` Pete Zaitcev
2007-10-29 18:04         ` Vitaliy Ivanov
2007-10-30  4:24           ` Pete Zaitcev
2007-10-30 13:09             ` Vitaliy Ivanov
2007-10-30 21:54               ` Pete Zaitcev
2007-10-31 11:54                 ` Vitaliy Ivanov
2007-10-31 22:01                   ` Pete Zaitcev
2007-11-01  9:06                     ` Vitaliy Ivanov
2007-11-01 17:28                       ` Pete Zaitcev
2007-10-24 14:49     ` Oliver Neukum
2007-10-24 21:25       ` Greg KH
2007-10-26  9:57         ` Vitaliy Ivanov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=200710231138.38266.oliver@neukum.org \
    --to=oliver@neukum.org \
    --cc=greg@kroah.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-usb-devel@lists.sourceforge.net \
    --cc=netwiz@crc.id.au \
    --cc=vitalivanov@gmail.com \
    --cc=zaitcev@redhat.com \
    --subject='Re: [linux-usb-devel] USB: FIx locks and urb->status in adutux' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).