usbnet: Recursive Locking bug ?

usbnet: Recursive Locking bug ?

[Neil Jones]

Hi,

I get a recursice lock warning from usbnet (asix adapter) with lockdep
checking on:

=============================================
[ INFO: possible recursive locking detected ]
2.6.37-rc4+ #1695
---------------------------------------------
ifconfig/536 is trying to acquire lock:
 (&(&list->lock)->rlock#2){-.-...}, at: [<401a63bc>] _defer_bh+0x24/0x124

but task is already holding lock:
 (&(&list->lock)->rlock#2){-.-...}, at: [<401a5f28>] _unlink_urbs+0x1c/0xa8

other info that might help us debug this:
2 locks held by ifconfig/536:
 #0:  (rtnl_mutex){+.+.+.}, at: [<4026b4f8>] _rtnl_lock+0x1c/0x2c
 #1:  (&(&list->lock)->rlock#2){-.-...}, at: [<401a5f28>] _unlink_urbs+0x1c/0xa8

this occurs on bring the interface down.

I think this is due to unlink_urbs():

static int unlink_urbs (struct usbnet *dev, struct sk_buff_head *q)
{
	unsigned long		flags;
	struct sk_buff		*skb, *skbnext;
	int			count = 0;

	spin_lock_irqsave (&q->lock, flags);
	skb_queue_walk_safe(q, skb, skbnext) {
		struct skb_data		*entry;
		struct urb		*urb;
		int			retval;

		entry = (struct skb_data *) skb->cb;
		urb = entry->urb;

		// during some PM-driven resume scenarios,
		// these (async) unlinks complete immediately
		retval = usb_unlink_urb (urb);
		if (retval != -EINPROGRESS && retval != 0)
			netdev_dbg(dev->net, "unlink urb err, %d\n", retval);
		else
			count++;
	}
	spin_unlock_irqrestore (&q->lock, flags);
	return count;
}

I dont think this should hold the list lock when calling usb_unlink_urb (urb).
The completion for the URBs takes the lock as required in its bottom half:

If people agree I will submit a patch.

Cheers

Neil
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: usbnet: Recursive Locking bug ?

[Oliver Neukum]

Am Dienstag, 7. Dezember 2010, 15:08:43 schrieb Neil Jones:
> I think this is due to unlink_urbs():

Definitely.

> static int unlink_urbs (struct usbnet *dev, struct sk_buff_head *q)
> {
>         unsigned long           flags;
>         struct sk_buff          *skb, *skbnext;
>         int                     count = 0;
> 
>         spin_lock_irqsave (&q->lock, flags);
>         skb_queue_walk_safe(q, skb, skbnext) {
>                 struct skb_data         *entry;
>                 struct urb              *urb;
>                 int                     retval;
> 
>                 entry = (struct skb_data *) skb->cb;
>                 urb = entry->urb;
> 
>                 // during some PM-driven resume scenarios,
>                 // these (async) unlinks complete immediately
>                 retval = usb_unlink_urb (urb);
>                 if (retval != -EINPROGRESS && retval != 0)
>                         netdev_dbg(dev->net, "unlink urb err, %d\n", retval);
>                 else
>                         count++;
>         }
>         spin_unlock_irqrestore (&q->lock, flags);
>         return count;
> }
> 
> I dont think this should hold the list lock when calling usb_unlink_urb (urb).
> The completion for the URBs takes the lock as required in its bottom half:

But what makes sure that the URB unlinked in unlink_urbs() stays a valid pointer?
The bottom half may run and free the URB.

	Regards
		Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Fwd: usbnet: Recursive Locking bug ?

[Neil Jones]

> But what makes sure that the URB unlinked in unlink_urbs() stays a valid pointer?
> The bottom half may run and free the URB.

I think it may be safe to remove the lock as we are walking a list of
SKBs not URBs,
the BH can remove SKB's from the list but we are doing a safe list walk.
+ the BH takse the list lock when it does the remove.

I could be wrong though?


On Tue, Dec 7, 2010 at 2:54 PM, Oliver Neukum <oneukum@suse.de> wrote:
> Am Dienstag, 7. Dezember 2010, 15:08:43 schrieb Neil Jones:
>> I think this is due to unlink_urbs():
>
> Definitely.
>
>> static int unlink_urbs (struct usbnet *dev, struct sk_buff_head *q)
>> {
>>         unsigned long           flags;
>>         struct sk_buff          *skb, *skbnext;
>>         int                     count = 0;
>>
>>         spin_lock_irqsave (&q->lock, flags);
>>         skb_queue_walk_safe(q, skb, skbnext) {
>>                 struct skb_data         *entry;
>>                 struct urb              *urb;
>>                 int                     retval;
>>
>>                 entry = (struct skb_data *) skb->cb;
>>                 urb = entry->urb;
>>
>>                 // during some PM-driven resume scenarios,
>>                 // these (async) unlinks complete immediately
>>                 retval = usb_unlink_urb (urb);
>>                 if (retval != -EINPROGRESS && retval != 0)
>>                         netdev_dbg(dev->net, "unlink urb err, %d\n", retval);
>>                 else
>>                         count++;
>>         }
>>         spin_unlock_irqrestore (&q->lock, flags);
>>         return count;
>> }
>>
>> I dont think this should hold the list lock when calling usb_unlink_urb (urb).
>> The completion for the URBs takes the lock as required in its bottom half:
>
> But what makes sure that the URB unlinked in unlink_urbs() stays a valid pointer?
> The bottom half may run and free the URB.
>
>        Regards
>                Oliver
>

Re: Fwd: usbnet: Recursive Locking bug ?

[Alan Stern]

On Tue, 7 Dec 2010, Neil Jones wrote:

> > But what makes sure that the URB unlinked in unlink_urbs() stays a valid pointer?
> > The bottom half may run and free the URB.
> 
> I think it may be safe to remove the lock as we are walking a list of
> SKBs not URBs,
> the BH can remove SKB's from the list but we are doing a safe list walk.
> + the BH takse the list lock when it does the remove.
> 
> I could be wrong though?

A simple answer to Oliver's question is to take a reference to the 
URB while still holding the lock, then release the lock before calling 
usb_unlink_urb(), then drop the reference to the URB.

Of course, this also requires you to restart the loop from the
beginning after each unlink, and it means you need to have a way to
recognize when an URB has already been unlinked.

Alan Stern

Re: Fwd: usbnet: Recursive Locking bug ?

[David Brownell]

I'll look at this some later, though I seem to
have missed a post with the diagnostic saying
what lock is being recursed.

That particular chunk of code has periodically
turned up problems, and isn't very pretty.  But
the most curious aspect of it is that it seemed
to shake out HCD-specific behaviors.  (We've
gotten rid of most such code by now, this is a
slight exception.

Specifically, HCDs that could unlink speedily
without certain locking patterns (ISTR OHCI and
EHCI, if not also UHCI) didn't trigger oddness.
But some other HCDs, with different approaches
to unlinking URBs, were less happy.  ( I was
likely working with MUSB at the time.)

I spent some time trying to rework that code in
"usbnet", but no clean-and-obvious solutions
became apparent when I did that (a few years
back).  Plus, ISTR being the only person to
find issues (back then), so I couldn't make
an argument to spend much more time on it.

Hope that helps anyone trying to fix this.

- Dave


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Fwd: usbnet: Recursive Locking bug ?

[Neil Jones]

>>though I seem to have missed a post with the diagnostic saying
>>what lock is being recursed.

[ INFO: possible recursive locking detected ]
2.6.37-rc4+ #1695
---------------------------------------------
ifconfig/536 is trying to acquire lock:
 (&(&list->lock)->rlock#2){-.-...}, at: [<401a63bc>] _defer_bh+0x24/0x124

but task is already holding lock:
 (&(&list->lock)->rlock#2){-.-...}, at: [<401a5f28>] _unlink_urbs+0x1c/0xa8

other info that might help us debug this:
2 locks held by ifconfig/536:
 #0:  (rtnl_mutex){+.+.+.}, at: [<4026b4f8>] _rtnl_lock+0x1c/0x2c
 #1:  (&(&list->lock)->rlock#2){-.-...}, at: [<401a5f28>] _unlink_urbs+0x1c/0xa8


We are using an OTG controller which isn't EHCI compliant and it has to
manage all the queues in software, thus unlinking URBs effectively
becomes synchronous. I cant see an easy solution to this short of modifying
the OTG-HCD driver to defer the unlinking to a workqueue, but even
then its still
possible to get this scenario.

Cheers

Neil


On Wed, Dec 8, 2010 at 4:55 AM, David Brownell <david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> wrote:
> I'll look at this some later, though I seem to
> have missed a post with the diagnostic saying
> what lock is being recursed.
>
> That particular chunk of code has periodically
> turned up problems, and isn't very pretty.  But
> the most curious aspect of it is that it seemed
> to shake out HCD-specific behaviors.  (We've
> gotten rid of most such code by now, this is a
> slight exception.
>
> Specifically, HCDs that could unlink speedily
> without certain locking patterns (ISTR OHCI and
> EHCI, if not also UHCI) didn't trigger oddness.
> But some other HCDs, with different approaches
> to unlinking URBs, were less happy.  ( I was
> likely working with MUSB at the time.)
>
> I spent some time trying to rework that code in
> "usbnet", but no clean-and-obvious solutions
> became apparent when I did that (a few years
> back).  Plus, ISTR being the only person to
> find issues (back then), so I couldn't make
> an argument to spend much more time on it.
>
> Hope that helps anyone trying to fix this.
>
> - Dave
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html