Message ID | 20120220160148.GA7910@linutronix.de |
---|---|
State | Changes Requested, archived |
Delegated to: | David Miller |
Headers | show |
Am Montag, 20. Februar 2012, 17:01:48 schrieb Sebastian Andrzej Siewior: > |kernel BUG at kernel/rtmutex.c:724! > |[<c029599c>] (rt_spin_lock_slowlock+0x108/0x2bc) from [<c01c2330>] (defer_bh+0x1c/0xb4) > |[<c01c2330>] (defer_bh+0x1c/0xb4) from [<c01c3afc>] (rx_complete+0x14c/0x194) > |[<c01c3afc>] (rx_complete+0x14c/0x194) from [<c01cac88>] (usb_hcd_giveback_urb+0xa0/0xf0) > |[<c01cac88>] (usb_hcd_giveback_urb+0xa0/0xf0) from [<c01e1ff4>] (musb_giveback+0x34/0x40) > |[<c01e1ff4>] (musb_giveback+0x34/0x40) from [<c01e2b1c>] (musb_advance_schedule+0xb4/0x1c0) > |[<c01e2b1c>] (musb_advance_schedule+0xb4/0x1c0) from [<c01e2ca8>] (musb_cleanup_urb.isra.9+0x80/0x8c) > |[<c01e2ca8>] (musb_cleanup_urb.isra.9+0x80/0x8c) from [<c01e2ed0>] (musb_urb_dequeue+0xec/0x108) > |[<c01e2ed0>] (musb_urb_dequeue+0xec/0x108) from [<c01cbb90>] (unlink1+0xbc/0xcc) > |[<c01cbb90>] (unlink1+0xbc/0xcc) from [<c01cc2ec>] (usb_hcd_unlink_urb+0x54/0xa8) > |[<c01cc2ec>] (usb_hcd_unlink_urb+0x54/0xa8) from [<c01c2a84>] (unlink_urbs.isra.17+0x2c/0x58) > |[<c01c2a84>] (unlink_urbs.isra.17+0x2c/0x58) from [<c01c2b44>] (usbnet_terminate_urbs+0x94/0x10c) > |[<c01c2b44>] (usbnet_terminate_urbs+0x94/0x10c) from [<c01c2d68>] (usbnet_stop+0x100/0x15c) > |[<c01c2d68>] (usbnet_stop+0x100/0x15c) from [<c020f718>] (__dev_close_many+0x94/0xc8) > > defer_bh() takes the lok which is hold during unlink_urbs(). The safe > walk suggest that the skb will be removed from the list and this is done > by defer_bh() so it seems to be okay to drop the lock here. I am afraid there's something wrong in the hcd driver. Async unlink must be possible with a lock held. I cannot approve this patch. Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/20/2012 05:21 PM, Oliver Neukum wrote: >> defer_bh() takes the lok which is hold during unlink_urbs(). The safe >> walk suggest that the skb will be removed from the list and this is done >> by defer_bh() so it seems to be okay to drop the lock here. > > I am afraid there's something wrong in the hcd driver. Async unlink must > be possible with a lock held. I cannot approve this patch. Hmmm. The comment above unlink() says that. Looking through other hcds it seems that musb is not the only one doing it wrong. Oh well... > > Regards > Oliver Sebastian -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 20 Feb 2012, Sebastian Andrzej Siewior wrote: > On 02/20/2012 05:21 PM, Oliver Neukum wrote: > >> defer_bh() takes the lok which is hold during unlink_urbs(). The safe > >> walk suggest that the skb will be removed from the list and this is done > >> by defer_bh() so it seems to be okay to drop the lock here. > > > > I am afraid there's something wrong in the hcd driver. Async unlink must > > be possible with a lock held. I cannot approve this patch. > > Hmmm. The comment above unlink() says that. Looking through other hcds > it seems that musb is not the only one doing it wrong. Oh well... What's the issue here? If a driver calls usb_unlink_urb() while holding a lock, and the completion routine tries to acquire the same lock, then deadlock is possible. The fact that usb_unlink_urb() is asynchronous is not a guarantee of anything; the HCD is allowed to call the completion handler from within usb_unlink_urb(). It's true that the kerneldoc for usb_unlink_urb() says "This request is always asynchronous". It might be a good idea to remove the word "always", because it seems to give people the wrong idea. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/20/2012 09:31 PM, Alan Stern wrote: > On Mon, 20 Feb 2012, Sebastian Andrzej Siewior wrote: > >> On 02/20/2012 05:21 PM, Oliver Neukum wrote: >>>> defer_bh() takes the lok which is hold during unlink_urbs(). The safe >>>> walk suggest that the skb will be removed from the list and this is done >>>> by defer_bh() so it seems to be okay to drop the lock here. >>> >>> I am afraid there's something wrong in the hcd driver. Async unlink must >>> be possible with a lock held. I cannot approve this patch. >> >> Hmmm. The comment above unlink() says that. Looking through other hcds >> it seems that musb is not the only one doing it wrong. Oh well... > > What's the issue here? > > If a driver calls usb_unlink_urb() while holding a lock, and the > completion routine tries to acquire the same lock, then deadlock is > possible. The fact that usb_unlink_urb() is asynchronous is not a > guarantee of anything; the HCD is allowed to call the completion > handler from within usb_unlink_urb(). > > It's true that the kerneldoc for usb_unlink_urb() says "This request is > always asynchronous". It might be a good idea to remove the word > "always", because it seems to give people the wrong idea. I see. So you approve that patch and suggest to remove the "always" wording plus adding something like "the hcd might call complete routine during unlink" ? > > Alan Stern > Sebastian -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 20 Feb 2012, Sebastian Andrzej Siewior wrote: > > What's the issue here? > > > > If a driver calls usb_unlink_urb() while holding a lock, and the > > completion routine tries to acquire the same lock, then deadlock is > > possible. The fact that usb_unlink_urb() is asynchronous is not a > > guarantee of anything; the HCD is allowed to call the completion > > handler from within usb_unlink_urb(). > > > > It's true that the kerneldoc for usb_unlink_urb() says "This request is > > always asynchronous". It might be a good idea to remove the word > > "always", because it seems to give people the wrong idea. > > I see. So you approve that patch and suggest to remove the "always" > wording plus adding something like "the hcd might call complete routine > during unlink" ? Well, I haven't read the patch and don't really understand what issue it tries to solve. But if that issue is the one I talked about above then yes, it makes sense. And changing the documentation as you suggest would be a good thing to do in any case. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Oliver Neukum | 2012-02-20 17:21:38 [+0100]: >Am Montag, 20. Februar 2012, 17:01:48 schrieb Sebastian Andrzej Siewior: >> |kernel BUG at kernel/rtmutex.c:724! >> |[<c029599c>] (rt_spin_lock_slowlock+0x108/0x2bc) from [<c01c2330>] (defer_bh+0x1c/0xb4) >> |[<c01c2330>] (defer_bh+0x1c/0xb4) from [<c01c3afc>] (rx_complete+0x14c/0x194) >> |[<c01c3afc>] (rx_complete+0x14c/0x194) from [<c01cac88>] (usb_hcd_giveback_urb+0xa0/0xf0) >> |[<c01cac88>] (usb_hcd_giveback_urb+0xa0/0xf0) from [<c01e1ff4>] (musb_giveback+0x34/0x40) >> |[<c01e1ff4>] (musb_giveback+0x34/0x40) from [<c01e2b1c>] (musb_advance_schedule+0xb4/0x1c0) >> |[<c01e2b1c>] (musb_advance_schedule+0xb4/0x1c0) from [<c01e2ca8>] (musb_cleanup_urb.isra.9+0x80/0x8c) >> |[<c01e2ca8>] (musb_cleanup_urb.isra.9+0x80/0x8c) from [<c01e2ed0>] (musb_urb_dequeue+0xec/0x108) >> |[<c01e2ed0>] (musb_urb_dequeue+0xec/0x108) from [<c01cbb90>] (unlink1+0xbc/0xcc) >> |[<c01cbb90>] (unlink1+0xbc/0xcc) from [<c01cc2ec>] (usb_hcd_unlink_urb+0x54/0xa8) >> |[<c01cc2ec>] (usb_hcd_unlink_urb+0x54/0xa8) from [<c01c2a84>] (unlink_urbs.isra.17+0x2c/0x58) >> |[<c01c2a84>] (unlink_urbs.isra.17+0x2c/0x58) from [<c01c2b44>] (usbnet_terminate_urbs+0x94/0x10c) >> |[<c01c2b44>] (usbnet_terminate_urbs+0x94/0x10c) from [<c01c2d68>] (usbnet_stop+0x100/0x15c) >> |[<c01c2d68>] (usbnet_stop+0x100/0x15c) from [<c020f718>] (__dev_close_many+0x94/0xc8) >> >> defer_bh() takes the lok which is hold during unlink_urbs(). The safe >> walk suggest that the skb will be removed from the list and this is done >> by defer_bh() so it seems to be okay to drop the lock here. > >I am afraid there's something wrong in the hcd driver. Async unlink must >be possible with a lock held. I cannot approve this patch. This argument is no longer valid, the documentation has been updated [0]. Is it okay for the patch to be merged? [0] http://git.kernel.org/?p=linux/kernel/git/gregkh/usb.git;a=commitdiff;h=371f3b49f2cb1a8b6ac09b6b108841ca92349eb1;hp=2a5be8783e0016d15e7907ddd212b2c312e196eb > Regards > Oliver Sebastian -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Date: Mon, 5 Mar 2012 22:25:00 +0100 > Is it okay for the patch to be merged? Just simply make a fresh submission of it. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c index fae0fbd..81b96e3 100644 --- a/drivers/net/usb/usbnet.c +++ b/drivers/net/usb/usbnet.c @@ -589,6 +589,7 @@ static int unlink_urbs (struct usbnet *dev, struct sk_buff_head *q) entry = (struct skb_data *) skb->cb; urb = entry->urb; + spin_unlock_irqrestore(&q->lock, flags); // during some PM-driven resume scenarios, // these (async) unlinks complete immediately retval = usb_unlink_urb (urb); @@ -596,6 +597,7 @@ static int unlink_urbs (struct usbnet *dev, struct sk_buff_head *q) netdev_dbg(dev->net, "unlink urb err, %d\n", retval); else count++; + spin_lock_irqsave(&q->lock, flags); } spin_unlock_irqrestore (&q->lock, flags); return count;
|kernel BUG at kernel/rtmutex.c:724! |[<c029599c>] (rt_spin_lock_slowlock+0x108/0x2bc) from [<c01c2330>] (defer_bh+0x1c/0xb4) |[<c01c2330>] (defer_bh+0x1c/0xb4) from [<c01c3afc>] (rx_complete+0x14c/0x194) |[<c01c3afc>] (rx_complete+0x14c/0x194) from [<c01cac88>] (usb_hcd_giveback_urb+0xa0/0xf0) |[<c01cac88>] (usb_hcd_giveback_urb+0xa0/0xf0) from [<c01e1ff4>] (musb_giveback+0x34/0x40) |[<c01e1ff4>] (musb_giveback+0x34/0x40) from [<c01e2b1c>] (musb_advance_schedule+0xb4/0x1c0) |[<c01e2b1c>] (musb_advance_schedule+0xb4/0x1c0) from [<c01e2ca8>] (musb_cleanup_urb.isra.9+0x80/0x8c) |[<c01e2ca8>] (musb_cleanup_urb.isra.9+0x80/0x8c) from [<c01e2ed0>] (musb_urb_dequeue+0xec/0x108) |[<c01e2ed0>] (musb_urb_dequeue+0xec/0x108) from [<c01cbb90>] (unlink1+0xbc/0xcc) |[<c01cbb90>] (unlink1+0xbc/0xcc) from [<c01cc2ec>] (usb_hcd_unlink_urb+0x54/0xa8) |[<c01cc2ec>] (usb_hcd_unlink_urb+0x54/0xa8) from [<c01c2a84>] (unlink_urbs.isra.17+0x2c/0x58) |[<c01c2a84>] (unlink_urbs.isra.17+0x2c/0x58) from [<c01c2b44>] (usbnet_terminate_urbs+0x94/0x10c) |[<c01c2b44>] (usbnet_terminate_urbs+0x94/0x10c) from [<c01c2d68>] (usbnet_stop+0x100/0x15c) |[<c01c2d68>] (usbnet_stop+0x100/0x15c) from [<c020f718>] (__dev_close_many+0x94/0xc8) defer_bh() takes the lok which is hold during unlink_urbs(). The safe walk suggest that the skb will be removed from the list and this is done by defer_bh() so it seems to be okay to drop the lock here. Cc: stable@kernel.org Reported-by: AnÃbal Almeida Pinto <anibal.pinto@efacec.com> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> --- drivers/net/usb/usbnet.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)