[PATCH] p54usb: fix usb_kill_urb hang with slub_debug=P

[PATCH] p54usb: fix usb_kill_urb hang with slub_debug=P

[Christian Lamparter]

This patch fixes a problem identified by Johannes Berg.

It looks like this is a classic case of "use-after-freed". 
A module which should reproduce the problem on
any other USB device can be found right here:
http://kerneltrap.org/mailarchive/linux-usb/2008/12/4/4317064

Tested-by: Johannes Berg <johannes@sipsolutions.net>
Signed-off-by: Christian Lamparter <chunkeey@web.de>
CC: stable@vger.kernel.org
---
diff --git a/drivers/net/wireless/p54/p54usb.c b/drivers/net/wireless/p54/p54usb.c
index 3444f52..0b9a922 100644
--- a/drivers/net/wireless/p54/p54usb.c
+++ b/drivers/net/wireless/p54/p54usb.c
@@ -209,7 +209,14 @@ static void p54u_free_urbs(struct ieee80211_hw *dev)
 		if (!info->urb)
 			continue;
 
+		/*
+		 * usb_get_urb and usb_free_urb are part of a temporary
+		 * workaround. Otherwise we get a pretty bad freeze,
+		 * if SLUB's poisoning debug option is enabled.
+		 */
+		usb_get_urb(info->urb);
 		usb_kill_urb(info->urb);
+		usb_free_urb(info->urb);
 		kfree_skb(skb);
 	}
 }

Re: [PATCH] p54usb: fix usb_kill_urb hang with slub_debug=P

[Larry Finger]

Christian Lamparter wrote:
> This patch fixes a problem identified by Johannes Berg.
> 
> It looks like this is a classic case of "use-after-freed". 
> A module which should reproduce the problem on
> any other USB device can be found right here:
> http://kerneltrap.org/mailarchive/linux-usb/2008/12/4/4317064

I didn't find either the code nor a link to it in the above link. What did I miss?

Larry

Re: [PATCH] p54usb: fix usb_kill_urb hang with slub_debug=P

[Christian Lamparter]

On Friday 05 December 2008 16:14:09 Larry Finger wrote:
> Christian Lamparter wrote:
> > This patch fixes a problem identified by Johannes Berg.
> > 
> > It looks like this is a classic case of "use-after-freed". 
> > A module which should reproduce the problem on
> > any other USB device can be found right here:
> > http://kerneltrap.org/mailarchive/linux-usb/2008/12/4/4317064
 
> I didn't find either the code nor a link to it in the above link. What did I miss?
> 
Sorry... kerneltrap doesn't like attachments.

http://marc.info/?l=linux-usb&m=122843263217096&w=2
has it.

Regards, 
	Chr

Re: [PATCH] p54usb: fix usb_kill_urb hang with slub_debug=P

[Greg KH]

On Fri, Dec 05, 2008 at 03:47:45PM +0100, Christian Lamparter wrote:
> This patch fixes a problem identified by Johannes Berg.

No, it only papers over the real problem here, let's work on a correct
patch please.

thanks,

greg k-h

Re: [PATCH] p54usb: fix usb_kill_urb hang with slub_debug=P

[Larry Finger]

Greg KH wrote:
> On Fri, Dec 05, 2008 at 03:47:45PM +0100, Christian Lamparter wrote:
>> This patch fixes a problem identified by Johannes Berg.
> 
> No, it only papers over the real problem here, let's work on a correct
> patch please.

I can contribute a little info. If SLUB debugging is enabled, and the boot
command includes 'slub_debug=P', I get a GPF in kref_get(), which is called from
kobject_get() with the following code:

        if (kobj)
                kref_get(&kobj->kref);

>From the dump, &kobj->kref is 0x6b6b6b6b6b6b6dbb, a poisoned value.

Somewhere, the "struct urb" has been freed, but kobj has not been set to NULL.

As everything I've found is a symptom, I'm still looking for the real cause.

Larry

Re: [PATCH] p54usb: fix usb_kill_urb hang with slub_debug=P

[Greg KH]

On Fri, Dec 05, 2008 at 05:18:07PM -0600, Larry Finger wrote:
> Greg KH wrote:
> > On Fri, Dec 05, 2008 at 03:47:45PM +0100, Christian Lamparter wrote:
> >> This patch fixes a problem identified by Johannes Berg.
> > 
> > No, it only papers over the real problem here, let's work on a correct
> > patch please.
> 
> I can contribute a little info. If SLUB debugging is enabled, and the boot
> command includes 'slub_debug=P', I get a GPF in kref_get(), which is called from
> kobject_get() with the following code:
> 
>         if (kobj)
>                 kref_get(&kobj->kref);
> 
> >From the dump, &kobj->kref is 0x6b6b6b6b6b6b6dbb, a poisoned value.
> 
> Somewhere, the "struct urb" has been freed, but kobj has not been set to NULL.
> 
> As everything I've found is a symptom, I'm still looking for the real cause.

See alan's post on linux-usb, it shows the real cause.

thanks,

greg k-h

[RFC][PATCH] p54usb: rewriting rx/tx routines to make use of usb_anchor's facilities

[Christian Lamparter]

Alan Stern found several flaws in p54usb's implementation and annotated: 
"usb_kill_urb() and similar routines do not expect an URB's completion
routine to deallocate it.  This is almost obvious -- if the URB is deallocated
before the completion routine returns then there's no way for usb_kill_urb
to detect when the URB actually is complete."

I really hope this patch addresses all of Alan's comments and fixes the 
"use-after-freed" hang, when SLUB debug's poisoning option is enabled.
---
patch is from wireless-testing. 
As far as I can see the usb_anchor code works now rather well
and the skb leak should be gone as well.

Alan, I've got a question about:
"Create and initialize a usb_anchor structure, and each time you create
a new URB, call usb_anchor_urb().  Then you can free the URB as soon as
it is submitted; the anchor will keep it pinned until it completes, and
it is automatically removed from the anchor when the completion routine
is called."

Do we have to call usb_free_urb again, if we're resubmitting the urb in the
complete callback? (the code what I'm referring to is p54u_rx_cb in p54usb.c)

Regards,
	Chr
---
diff -Nurp a/drivers/net/wireless/p54/p54usb.c b/drivers/net/wireless/p54/p54usb.c
--- a/drivers/net/wireless/p54/p54usb.c	2008-12-06 20:06:23.000000000 +0100
+++ b/drivers/net/wireless/p54/p54usb.c	2008-12-06 20:32:58.000000000 +0100
@@ -86,13 +86,13 @@ static void p54u_rx_cb(struct urb *urb)
 	struct ieee80211_hw *dev = info->dev;
 	struct p54u_priv *priv = dev->priv;
 
+	skb_unlink(skb, &priv->rx_queue);
+
 	if (unlikely(urb->status)) {
-		info->urb = NULL;
-		usb_free_urb(urb);
+		dev_kfree_skb_irq(skb);
 		return;
 	}
 
-	skb_unlink(skb, &priv->rx_queue);
 	skb_put(skb, urb->actual_length);
 
 	if (priv->hw_type == P54U_NET2280)
@@ -105,7 +105,6 @@ static void p54u_rx_cb(struct urb *urb)
 	if (p54_rx(dev, skb)) {
 		skb = dev_alloc_skb(priv->common.rx_mtu + 32);
 		if (unlikely(!skb)) {
-			usb_free_urb(urb);
 			/* TODO check rx queue length and refill *somewhere* */
 			return;
 		}
@@ -134,7 +133,11 @@ static void p54u_rx_cb(struct urb *urb)
 		skb_queue_tail(&priv->rx_queue, skb);
 	}
 
-	usb_submit_urb(urb, GFP_ATOMIC);
+	if (usb_submit_urb(urb, GFP_ATOMIC)) {
+		skb_unlink(skb, &priv->rx_queue);
+		usb_unanchor_urb(urb);
+		dev_kfree_skb_irq(skb);
+	}
 }
 
 static void p54u_tx_reuse_skb_cb(struct urb *urb)
@@ -144,18 +147,6 @@ static void p54u_tx_reuse_skb_cb(struct 
 		usb_get_intfdata(usb_ifnum_to_if(urb->dev, 0)))->priv;
 
 	skb_pull(skb, priv->common.tx_hdr_len);
-	usb_free_urb(urb);
-}
-
-static void p54u_tx_cb(struct urb *urb)
-{
-	usb_free_urb(urb);
-}
-
-static void p54u_tx_free_cb(struct urb *urb)
-{
-	kfree(urb->transfer_buffer);
-	usb_free_urb(urb);
 }
 
 static void p54u_tx_free_skb_cb(struct urb *urb)
@@ -165,25 +156,36 @@ static void p54u_tx_free_skb_cb(struct u
 		usb_get_intfdata(usb_ifnum_to_if(urb->dev, 0));
 
 	p54_free_skb(dev, skb);
-	usb_free_urb(urb);
+}
+
+static void p54u_tx_dummy_cb(struct urb *urb) { }
+
+static void p54u_free_urbs(struct ieee80211_hw *dev)
+{
+	struct p54u_priv *priv = dev->priv;
+	usb_kill_anchored_urbs(&priv->submitted);
 }
 
 static int p54u_init_urbs(struct ieee80211_hw *dev)
 {
 	struct p54u_priv *priv = dev->priv;
-	struct urb *entry;
+	struct urb *entry = NULL;
 	struct sk_buff *skb;
 	struct p54u_rx_info *info;
+	int ret = 0;
 
 	while (skb_queue_len(&priv->rx_queue) < 32) {
 		skb = __dev_alloc_skb(priv->common.rx_mtu + 32, GFP_KERNEL);
-		if (!skb)
-			break;
+		if (!skb) {
+			ret = -ENOMEM;
+			goto err;
+		}
 		entry = usb_alloc_urb(0, GFP_KERNEL);
 		if (!entry) {
-			kfree_skb(skb);
-			break;
+			ret = -ENOMEM;
+			goto err;
 		}
+
 		usb_fill_bulk_urb(entry, priv->udev,
 				  usb_rcvbulkpipe(priv->udev, P54U_PIPE_DATA),
 				  skb_tail_pointer(skb),
@@ -192,26 +194,25 @@ static int p54u_init_urbs(struct ieee802
 		info->urb = entry;
 		info->dev = dev;
 		skb_queue_tail(&priv->rx_queue, skb);
-		usb_submit_urb(entry, GFP_KERNEL);
+
+		usb_anchor_urb(entry, &priv->submitted);
+		ret = usb_submit_urb(entry, GFP_KERNEL);
+		if (ret) {
+			skb_unlink(skb, &priv->rx_queue);
+			usb_unanchor_urb(entry);
+			goto err;
+		}
+		usb_free_urb(entry);
+		entry = NULL;
 	}
 
 	return 0;
-}
-
-static void p54u_free_urbs(struct ieee80211_hw *dev)
-{
-	struct p54u_priv *priv = dev->priv;
-	struct p54u_rx_info *info;
-	struct sk_buff *skb;
 
-	while ((skb = skb_dequeue(&priv->rx_queue))) {
-		info = (struct p54u_rx_info *) skb->cb;
-		if (!info->urb)
-			continue;
-
-		usb_kill_urb(info->urb);
-		kfree_skb(skb);
-	}
+ err:
+	usb_free_urb(entry);
+	kfree_skb(skb);
+	p54u_free_urbs(dev);
+	return ret;
 }
 
 static void p54u_tx_3887(struct ieee80211_hw *dev, struct sk_buff *skb,
@@ -219,6 +220,7 @@ static void p54u_tx_3887(struct ieee8021
 {
 	struct p54u_priv *priv = dev->priv;
 	struct urb *addr_urb, *data_urb;
+	int err = 0;
 
 	addr_urb = usb_alloc_urb(0, GFP_ATOMIC);
 	if (!addr_urb)
@@ -233,15 +235,31 @@ static void p54u_tx_3887(struct ieee8021
 	usb_fill_bulk_urb(addr_urb, priv->udev,
 			  usb_sndbulkpipe(priv->udev, P54U_PIPE_DATA),
 			  &((struct p54_hdr *)skb->data)->req_id, 4,
-			  p54u_tx_cb, dev);
+			  p54u_tx_dummy_cb, dev);
 	usb_fill_bulk_urb(data_urb, priv->udev,
 			  usb_sndbulkpipe(priv->udev, P54U_PIPE_DATA),
 			  skb->data, skb->len,
 			  free_on_tx ? p54u_tx_free_skb_cb :
 				       p54u_tx_reuse_skb_cb, skb);
 
-	usb_submit_urb(addr_urb, GFP_ATOMIC);
-	usb_submit_urb(data_urb, GFP_ATOMIC);
+	usb_anchor_urb(addr_urb, &priv->submitted);
+	err = usb_submit_urb(addr_urb, GFP_ATOMIC);
+	if (err) {
+		usb_unanchor_urb(addr_urb);
+		goto out;
+	}
+
+	usb_anchor_urb(addr_urb, &priv->submitted);
+	err = usb_submit_urb(data_urb, GFP_ATOMIC);
+	if (err)
+		usb_unanchor_urb(data_urb);
+
+ out:
+	usb_free_urb(addr_urb);
+	usb_free_urb(data_urb);
+
+	if (err)
+		p54_free_skb(dev, skb);
 }
 
 static __le32 p54u_lm87_chksum(const __le32 *data, size_t length)
@@ -281,7 +299,13 @@ static void p54u_tx_lm87(struct ieee8021
 			  free_on_tx ? p54u_tx_free_skb_cb :
 				       p54u_tx_reuse_skb_cb, skb);
 
-	usb_submit_urb(data_urb, GFP_ATOMIC);
+	usb_anchor_urb(data_urb, &priv->submitted);
+	if (usb_submit_urb(data_urb, GFP_ATOMIC)) {
+		usb_unanchor_urb(data_urb);
+		skb_pull(skb, sizeof(*hdr));
+		p54_free_skb(dev, skb);
+	}
+	usb_free_urb(data_urb);
 }
 
 static void p54u_tx_net2280(struct ieee80211_hw *dev, struct sk_buff *skb,
@@ -291,6 +315,7 @@ static void p54u_tx_net2280(struct ieee8
 	struct urb *int_urb, *data_urb;
 	struct net2280_tx_hdr *hdr;
 	struct net2280_reg_write *reg;
+	int err = 0;
 
 	reg = kmalloc(sizeof(*reg), GFP_ATOMIC);
 	if (!reg)
@@ -318,17 +343,38 @@ static void p54u_tx_net2280(struct ieee8
 	hdr->device_addr = ((struct p54_hdr *)skb->data)->req_id;
 	hdr->len = cpu_to_le16(skb->len + sizeof(struct p54_hdr));
 
+	int_urb->transfer_flags |= URB_FREE_BUFFER;
 	usb_fill_bulk_urb(int_urb, priv->udev,
 		usb_sndbulkpipe(priv->udev, P54U_PIPE_DEV), reg, sizeof(*reg),
-		p54u_tx_free_cb, dev);
-	usb_submit_urb(int_urb, GFP_ATOMIC);
+		p54u_tx_dummy_cb, dev);
 
 	usb_fill_bulk_urb(data_urb, priv->udev,
 			  usb_sndbulkpipe(priv->udev, P54U_PIPE_DATA),
 			  skb->data, skb->len,
 			  free_on_tx ? p54u_tx_free_skb_cb :
 				       p54u_tx_reuse_skb_cb, skb);
-	usb_submit_urb(data_urb, GFP_ATOMIC);
+
+	usb_anchor_urb(int_urb, &priv->submitted);
+	err = usb_submit_urb(int_urb, GFP_ATOMIC);
+	if (err) {
+		usb_unanchor_urb(int_urb);
+		goto out;
+	}
+
+	usb_anchor_urb(data_urb, &priv->submitted);
+	err = usb_submit_urb(data_urb, GFP_ATOMIC);
+	if (err) {
+		usb_unanchor_urb(data_urb);
+		goto out;
+	}
+ out:
+	usb_free_urb(int_urb);
+	usb_free_urb(data_urb);
+
+	if (err) {
+		skb_pull(skb, sizeof(*hdr));
+		p54_free_skb(dev, skb);
+	}
 }
 
 static int p54u_write(struct p54u_priv *priv,
@@ -886,6 +932,7 @@ static int __devinit p54u_probe(struct u
 		goto err_free_dev;
 
 	skb_queue_head_init(&priv->rx_queue);
+	init_usb_anchor(&priv->submitted);
 
 	p54u_open(dev);
 	err = p54_read_eeprom(dev);
diff -Nurp a/drivers/net/wireless/p54/p54usb.h b/drivers/net/wireless/p54/p54usb.h
--- a/drivers/net/wireless/p54/p54usb.h	2008-12-06 20:06:23.000000000 +0100
+++ b/drivers/net/wireless/p54/p54usb.h	2008-12-06 20:16:19.000000000 +0100
@@ -133,6 +133,7 @@ struct p54u_priv {
 
 	spinlock_t lock;
 	struct sk_buff_head rx_queue;
+	struct usb_anchor submitted;
 };
 
 #endif /* P54USB_H */

Re: [RFC][PATCH] p54usb: rewriting rx/tx routines to make use of usb_anchor's facilities

[Larry Finger]

Christian Lamparter wrote:
> 
> Alan, I've got a question about:
> "Create and initialize a usb_anchor structure, and each time you create
> a new URB, call usb_anchor_urb().  Then you can free the URB as soon as
> it is submitted; the anchor will keep it pinned until it completes, and
> it is automatically removed from the anchor when the completion routine
> is called."
> 
> Do we have to call usb_free_urb again, if we're resubmitting the urb in the
> complete callback? (the code what I'm referring to is p54u_rx_cb in p54usb.c)

I am very interested in the answer to this question. My interpretation is that
you cannot resubmit the urb after it has been posted to usb_anchor_usb() because
the USB core will have deleted it, and that the callback routine will have to
allocate a new urb, anchor it, and then free it as well.

Larry

Re: [RFC][PATCH] p54usb: rewriting rx/tx routines to make use of usb_anchor's facilities

[Alan Stern]

On Sat, 6 Dec 2008, Larry Finger wrote:

> Christian Lamparter wrote:
> > 
> > Alan, I've got a question about:
> > "Create and initialize a usb_anchor structure, and each time you create
> > a new URB, call usb_anchor_urb().  Then you can free the URB as soon as
> > it is submitted; the anchor will keep it pinned until it completes, and
> > it is automatically removed from the anchor when the completion routine
> > is called."
> > 
> > Do we have to call usb_free_urb again, if we're resubmitting the urb in the
> > complete callback? (the code what I'm referring to is p54u_rx_cb in p54usb.c)
> 
> I am very interested in the answer to this question. My interpretation is that
> you cannot resubmit the urb after it has been posted to usb_anchor_usb() because
> the USB core will have deleted it, and that the callback routine will have to
> allocate a new urb, anchor it, and then free it as well.

I looked quickly at your new code.  It seems basically right, but there
are few things that still need attention.  For instance, you removed a
line to free an URB's transfer buffer.  The USB core will automatically
call kfree(urb->transfer_buffer) for you when the URB is deallocated,
but only if you have set the URB_FREE_BUFFER bit in
urb->transfer_flags.  I didn't check to see whether the driver does 
this.

To answer your questions about anchors...  The main idea is simple
enough.  When you create an URB, its refcount is 1.  Each call to
usb_get_urb() increments the refcount and each call to usb_put_urb()  
or usb_free_urb() decrements it; when the refcount reaches 0 the URB is
deallocated.  When you successfully submit an URB, the core increments
its refcount; the refcount is then decremented when the completion
routine returns.

When you add an URB to an anchor, its refcount is incremented.  When it
is removed from the anchor, the refcount is decremented again.  An
anchored URB is automatically removed from its anchor just before the
completion routine is called.  So if your completion routine wants to
resubmit an URB, it has to add the URB back to the anchor.  And if
the submission fails then you have to decide what to do (usually you 
would unanchor the URB, which would cause it to be deallocated later).

As a sketch: (1) Initially:

	usb_alloc_urb			=> refcount = 1
	usb_anchor_urb			=> refcount = 2
	usb_submit_urb			=> refcount = 3
	usb_free_urb			=> refcount = 2

(2) Later on:

	URB completes
	URB is removed from anchor	=> refcount = 1
	Completion routine is called

(3a) If the completion routine doesn't resubmit:

	Completion routine returns	=> refcount = 0,
					URB is deallocated

(3b) If the completion routine does resubmit:

	usb_anchor_urb			=> refcount = 2
	usb_submit_urb			=> refcount = 3
	Completion routine returns	=> refcount = 2
	Go back to (2)...

Hopefully this clarifies things.

Alan Stern

[RFC][PATCH v2] p54usb: rewriting rx/tx routines to make use of usb_anchor's facilities

[Christian Lamparter]

Alan Stern found several flaws in p54usb's implementation and annotated=
:=20
"usb_kill_urb() and similar routines do not expect an URB's completion
routine to deallocate it. =A0This is almost obvious -- if the URB is de=
allocated
before the completion routine returns then there's no way for usb_kill_=
urb
to detect when the URB actually is complete."

I really hope this patch addresses all of Alan's comments and fixes the=
=20
"use-after-freed" hang, when SLUB debug's poisoning option is enabled.
---
> I looked quickly at your new code.  It seems basically right, but the=
re
> are few things that still need attention.  For instance, you removed =
a
> line to free an URB's transfer buffer.

yeah, it was there in the last patch, but probably in the wrong place.
Anyway, there's now a small comment so it's easier to spot it.

So here's the new "final" version, unless someone files a complain with=
in 24 hours.=20
---
diff -Nurp a/drivers/net/wireless/p54/p54usb.c b/drivers/net/wireless/p=
54/p54usb.c
--- a/drivers/net/wireless/p54/p54usb.c	2008-12-07 21:21:04.017774898 +=
0100
+++ b/drivers/net/wireless/p54/p54usb.c	2008-12-08 14:17:51.863521490 +=
0100
@@ -86,13 +86,13 @@ static void p54u_rx_cb(struct urb *urb)
 	struct ieee80211_hw *dev =3D info->dev;
 	struct p54u_priv *priv =3D dev->priv;
=20
+	skb_unlink(skb, &priv->rx_queue);
+
 	if (unlikely(urb->status)) {
-		info->urb =3D NULL;
-		usb_free_urb(urb);
+		dev_kfree_skb_irq(skb);
 		return;
 	}
=20
-	skb_unlink(skb, &priv->rx_queue);
 	skb_put(skb, urb->actual_length);
=20
 	if (priv->hw_type =3D=3D P54U_NET2280)
@@ -105,7 +105,6 @@ static void p54u_rx_cb(struct urb *urb)
 	if (p54_rx(dev, skb)) {
 		skb =3D dev_alloc_skb(priv->common.rx_mtu + 32);
 		if (unlikely(!skb)) {
-			usb_free_urb(urb);
 			/* TODO check rx queue length and refill *somewhere* */
 			return;
 		}
@@ -115,7 +114,6 @@ static void p54u_rx_cb(struct urb *urb)
 		info->dev =3D dev;
 		urb->transfer_buffer =3D skb_tail_pointer(skb);
 		urb->context =3D skb;
-		skb_queue_tail(&priv->rx_queue, skb);
 	} else {
 		if (priv->hw_type =3D=3D P54U_NET2280)
 			skb_push(skb, priv->common.tx_hdr_len);
@@ -130,11 +128,14 @@ static void p54u_rx_cb(struct urb *urb)
 			WARN_ON(1);
 			urb->transfer_buffer =3D skb_tail_pointer(skb);
 		}
-
-		skb_queue_tail(&priv->rx_queue, skb);
 	}
-
-	usb_submit_urb(urb, GFP_ATOMIC);
+	skb_queue_tail(&priv->rx_queue, skb);
+	usb_anchor_urb(urb, &priv->submitted);
+	if (usb_submit_urb(urb, GFP_ATOMIC)) {
+		skb_unlink(skb, &priv->rx_queue);
+		usb_unanchor_urb(urb);
+		dev_kfree_skb_irq(skb);
+	}
 }
=20
 static void p54u_tx_reuse_skb_cb(struct urb *urb)
@@ -144,18 +145,6 @@ static void p54u_tx_reuse_skb_cb(struct=20
 		usb_get_intfdata(usb_ifnum_to_if(urb->dev, 0)))->priv;
=20
 	skb_pull(skb, priv->common.tx_hdr_len);
-	usb_free_urb(urb);
-}
-
-static void p54u_tx_cb(struct urb *urb)
-{
-	usb_free_urb(urb);
-}
-
-static void p54u_tx_free_cb(struct urb *urb)
-{
-	kfree(urb->transfer_buffer);
-	usb_free_urb(urb);
 }
=20
 static void p54u_tx_free_skb_cb(struct urb *urb)
@@ -165,25 +154,36 @@ static void p54u_tx_free_skb_cb(struct u
 		usb_get_intfdata(usb_ifnum_to_if(urb->dev, 0));
=20
 	p54_free_skb(dev, skb);
-	usb_free_urb(urb);
+}
+
+static void p54u_tx_dummy_cb(struct urb *urb) { }
+
+static void p54u_free_urbs(struct ieee80211_hw *dev)
+{
+	struct p54u_priv *priv =3D dev->priv;
+	usb_kill_anchored_urbs(&priv->submitted);
 }
=20
 static int p54u_init_urbs(struct ieee80211_hw *dev)
 {
 	struct p54u_priv *priv =3D dev->priv;
-	struct urb *entry;
+	struct urb *entry =3D NULL;
 	struct sk_buff *skb;
 	struct p54u_rx_info *info;
+	int ret =3D 0;
=20
 	while (skb_queue_len(&priv->rx_queue) < 32) {
 		skb =3D __dev_alloc_skb(priv->common.rx_mtu + 32, GFP_KERNEL);
-		if (!skb)
-			break;
+		if (!skb) {
+			ret =3D -ENOMEM;
+			goto err;
+		}
 		entry =3D usb_alloc_urb(0, GFP_KERNEL);
 		if (!entry) {
-			kfree_skb(skb);
-			break;
+			ret =3D -ENOMEM;
+			goto err;
 		}
+
 		usb_fill_bulk_urb(entry, priv->udev,
 				  usb_rcvbulkpipe(priv->udev, P54U_PIPE_DATA),
 				  skb_tail_pointer(skb),
@@ -192,26 +192,25 @@ static int p54u_init_urbs(struct ieee802
 		info->urb =3D entry;
 		info->dev =3D dev;
 		skb_queue_tail(&priv->rx_queue, skb);
-		usb_submit_urb(entry, GFP_KERNEL);
+
+		usb_anchor_urb(entry, &priv->submitted);
+		ret =3D usb_submit_urb(entry, GFP_KERNEL);
+		if (ret) {
+			skb_unlink(skb, &priv->rx_queue);
+			usb_unanchor_urb(entry);
+			goto err;
+		}
+		usb_free_urb(entry);
+		entry =3D NULL;
 	}
=20
 	return 0;
-}
=20
-static void p54u_free_urbs(struct ieee80211_hw *dev)
-{
-	struct p54u_priv *priv =3D dev->priv;
-	struct p54u_rx_info *info;
-	struct sk_buff *skb;
-
-	while ((skb =3D skb_dequeue(&priv->rx_queue))) {
-		info =3D (struct p54u_rx_info *) skb->cb;
-		if (!info->urb)
-			continue;
-
-		usb_kill_urb(info->urb);
-		kfree_skb(skb);
-	}
+ err:
+	usb_free_urb(entry);
+	kfree_skb(skb);
+	p54u_free_urbs(dev);
+	return ret;
 }
=20
 static void p54u_tx_3887(struct ieee80211_hw *dev, struct sk_buff *skb=
,
@@ -219,6 +218,7 @@ static void p54u_tx_3887(struct ieee8021
 {
 	struct p54u_priv *priv =3D dev->priv;
 	struct urb *addr_urb, *data_urb;
+	int err =3D 0;
=20
 	addr_urb =3D usb_alloc_urb(0, GFP_ATOMIC);
 	if (!addr_urb)
@@ -233,15 +233,31 @@ static void p54u_tx_3887(struct ieee8021
 	usb_fill_bulk_urb(addr_urb, priv->udev,
 			  usb_sndbulkpipe(priv->udev, P54U_PIPE_DATA),
 			  &((struct p54_hdr *)skb->data)->req_id, 4,
-			  p54u_tx_cb, dev);
+			  p54u_tx_dummy_cb, dev);
 	usb_fill_bulk_urb(data_urb, priv->udev,
 			  usb_sndbulkpipe(priv->udev, P54U_PIPE_DATA),
 			  skb->data, skb->len,
 			  free_on_tx ? p54u_tx_free_skb_cb :
 				       p54u_tx_reuse_skb_cb, skb);
=20
-	usb_submit_urb(addr_urb, GFP_ATOMIC);
-	usb_submit_urb(data_urb, GFP_ATOMIC);
+	usb_anchor_urb(addr_urb, &priv->submitted);
+	err =3D usb_submit_urb(addr_urb, GFP_ATOMIC);
+	if (err) {
+		usb_unanchor_urb(addr_urb);
+		goto out;
+	}
+
+	usb_anchor_urb(addr_urb, &priv->submitted);
+	err =3D usb_submit_urb(data_urb, GFP_ATOMIC);
+	if (err)
+		usb_unanchor_urb(data_urb);
+
+ out:
+	usb_free_urb(addr_urb);
+	usb_free_urb(data_urb);
+
+	if (err)
+		p54_free_skb(dev, skb);
 }
=20
 static __le32 p54u_lm87_chksum(const __le32 *data, size_t length)
@@ -281,7 +297,13 @@ static void p54u_tx_lm87(struct ieee8021
 			  free_on_tx ? p54u_tx_free_skb_cb :
 				       p54u_tx_reuse_skb_cb, skb);
=20
-	usb_submit_urb(data_urb, GFP_ATOMIC);
+	usb_anchor_urb(data_urb, &priv->submitted);
+	if (usb_submit_urb(data_urb, GFP_ATOMIC)) {
+		usb_unanchor_urb(data_urb);
+		skb_pull(skb, sizeof(*hdr));
+		p54_free_skb(dev, skb);
+	}
+	usb_free_urb(data_urb);
 }
=20
 static void p54u_tx_net2280(struct ieee80211_hw *dev, struct sk_buff *=
skb,
@@ -291,6 +313,7 @@ static void p54u_tx_net2280(struct ieee8
 	struct urb *int_urb, *data_urb;
 	struct net2280_tx_hdr *hdr;
 	struct net2280_reg_write *reg;
+	int err =3D 0;
=20
 	reg =3D kmalloc(sizeof(*reg), GFP_ATOMIC);
 	if (!reg)
@@ -320,15 +343,42 @@ static void p54u_tx_net2280(struct ieee8
=20
 	usb_fill_bulk_urb(int_urb, priv->udev,
 		usb_sndbulkpipe(priv->udev, P54U_PIPE_DEV), reg, sizeof(*reg),
-		p54u_tx_free_cb, dev);
-	usb_submit_urb(int_urb, GFP_ATOMIC);
+		p54u_tx_dummy_cb, dev);
+
+	/*
+	 * This flag triggers a code path in the USB subsystem that will
+	 * free what's inside the transfer_buffer after the callback routine
+	 * has completed.
+	 */
+	int_urb->transfer_flags |=3D URB_FREE_BUFFER;
=20
 	usb_fill_bulk_urb(data_urb, priv->udev,
 			  usb_sndbulkpipe(priv->udev, P54U_PIPE_DATA),
 			  skb->data, skb->len,
 			  free_on_tx ? p54u_tx_free_skb_cb :
 				       p54u_tx_reuse_skb_cb, skb);
-	usb_submit_urb(data_urb, GFP_ATOMIC);
+
+	usb_anchor_urb(int_urb, &priv->submitted);
+	err =3D usb_submit_urb(int_urb, GFP_ATOMIC);
+	if (err) {
+		usb_unanchor_urb(int_urb);
+		goto out;
+	}
+
+	usb_anchor_urb(data_urb, &priv->submitted);
+	err =3D usb_submit_urb(data_urb, GFP_ATOMIC);
+	if (err) {
+		usb_unanchor_urb(data_urb);
+		goto out;
+	}
+ out:
+	usb_free_urb(int_urb);
+	usb_free_urb(data_urb);
+
+	if (err) {
+		skb_pull(skb, sizeof(*hdr));
+		p54_free_skb(dev, skb);
+	}
 }
=20
 static int p54u_write(struct p54u_priv *priv,
@@ -886,6 +936,7 @@ static int __devinit p54u_probe(struct u
 		goto err_free_dev;
=20
 	skb_queue_head_init(&priv->rx_queue);
+	init_usb_anchor(&priv->submitted);
=20
 	p54u_open(dev);
 	err =3D p54_read_eeprom(dev);
diff -Nurp a/drivers/net/wireless/p54/p54usb.h b/drivers/net/wireless/p=
54/p54usb.h
--- a/drivers/net/wireless/p54/p54usb.h	2008-12-07 21:21:04.017774898 +=
0100
+++ b/drivers/net/wireless/p54/p54usb.h	2008-12-07 22:04:45.956897502 +=
0100
@@ -133,6 +133,7 @@ struct p54u_priv {
=20
 	spinlock_t lock;
 	struct sk_buff_head rx_queue;
+	struct usb_anchor submitted;
 };
=20
 #endif /* P54USB_H */
--
To unsubscribe from this list: send the line "unsubscribe linux-wireles=
s" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] p54usb: fix usb_kill_urb hang with slub_debug=P

[Christian Lamparter]

On Saturday 06 December 2008 00:18:07 Larry Finger wrote:
> Greg KH wrote:
> > On Fri, Dec 05, 2008 at 03:47:45PM +0100, Christian Lamparter wrote:
> >> This patch fixes a problem identified by Johannes Berg.
> > 
> > No, it only papers over the real problem here, let's work on a correct
> > patch please.
> 
> I can contribute a little info. If SLUB debugging is enabled, and the boot
> command includes 'slub_debug=P', I get a GPF in kref_get(), which is called from
> kobject_get() with the following code:
> 
>         if (kobj)
>                 kref_get(&kobj->kref);
> 
> From the dump, &kobj->kref is 0x6b6b6b6b6b6b6dbb, a poisoned value.
> 
> Somewhere, the "struct urb" has been freed, but kobj has not been set to NULL.
> 
> As everything I've found is a symptom, I'm still looking for the real cause.
> 
> Larry
> 

This is from a different thread... unfortunately I forgot to add you to the CC.
It looks like rtl8187 is affected as well.

So let's make new patches and drop the "temporary workaround". (see comment)

Regards,
	Chr

----------  Forwarded Message  ----------

Subject: Re: usb_kill_urb hangs with slub_debug=P (Poision) aka usb-slXbdebug-crash-v2
Date: Friday 05 December 2008
From: Alan Stern <stern@rowland.harvard.edu>
To: Christian Lamparter <chunkeey@web.de>

On Fri, 5 Dec 2008, Christian Lamparter wrote:

> > The real problem is that this outline driver accesses skb's in multiple
> > places without any sort of mutual exlusion.  If the driver used
> > spinlocks properly then it wouldn't be possible for usbtest_stop() to
> > try killing an URB that dummy_cb() has just freed.
> >
> yes, yes can you please tell me exactly where?
> 
> as all skb_queue function are taking & releasing spin_lock_irqsave 
> (see net/skbuff.c...) e.g: 
> ###
> void skb_queue_tail(struct sk_buff_head *list, struct sk_buff *newsk)
> {
>         unsigned long flags;
> 
>         spin_lock_irqsave(&list->lock, flags);
>         __skb_queue_tail(list, newsk);
>         spin_unlock_irqrestore(&list->lock, flags);
> }
> [...]
> struct sk_buff *skb_dequeue(struct sk_buff_head *list)
> {
>         unsigned long flags;
>         struct sk_buff *result;
> 
>         spin_lock_irqsave(&list->lock, flags);
>         result = __skb_dequeue(list);
>         spin_unlock_irqrestore(&list->lock, flags);
>         return result;
> }
> ###
> The only exception is __skb_unlink(skb, &rx_queue);
> But this is "dead" code, since in our test case we don't plan to receive anything.
> Of course p54usb uses skb_unlink(skb, &rx_queue) and it freeze too...
> 
> So please please; tell me where the problems are and not that they exists...
> I know that already ;-) 

I have to apologize...  The problem isn't related to lack of spinlocks.

The problem is that usb_kill_urb() and similar routines do not expect
an URB's completion routine to deallocate it.  This is almost obvious 
-- if the URB is deallocated before the completion routine returns then 
there's no way for usb_kill_urb to detect when the URB actually is 
complete.

One solution is always to deallocate URBs in the stop method rather
than in the completion routine.  But your driver doesn't work that way,
so what you need to do is use an anchor.  Anchors were specifically 
designed to support a "fire and forget" approach, which is what you 
want.

Create and initialize a usb_anchor structure, and each time you create
a new URB, call usb_anchor_urb().  Then you can free the URB as soon as
it is submitted; the anchor will keep it pinned until it completes, and
it is automatically removed from the anchor when the completion routine
is called.

See the kerneldoc for usb_anchor_urb and related routines (such as
urb_kill_anchored_urb) in drivers/usb/core/urb.c, and the anchor
declarations in include/linux/usb.h.


This doesn't mean you should forget about using spinlocks.  Let's look
at the source code:

static void dummy_cb(struct urb *urb)
{
	struct sk_buff *skb = (struct sk_buff *) urb->context;
	struct rx_info *info = (struct rx_info *) skb->cb;

	dev_info(&udev->dev, "dummy cb urb:%p skb:%p status:%d\n",
		 urb, skb, urb->status);

	if (unlikely(urb->status)) {
		info->urb = NULL;
		usb_free_urb(urb);
		return;
	}

	__skb_unlink(skb, &rx_queue);
	dev_kfree_skb(skb);
	usb_free_urb(urb);
}

static void usbtest_stop(void)
{
	struct rx_info *info;
	struct sk_buff *skb;

	while ((skb = skb_dequeue(&rx_queue))) {
		info = (struct rx_info *) skb->cb;
		dev_info(&udev->dev, "Free entry urb:%p skb:%p queue_len:%d\n",
			info->urb, skb, skb_queue_len(&rx_queue));

		if (!info->urb)
			continue;

		usb_kill_urb(info->urb);
		kfree_skb(skb);
	}
}

So there's nothing to stop this sort of thing from happening:

	Thread 0			Thread 1
	--------			--------
	Call usbtest_stop()
	info->urb is not NULL
					Call dummy_cb()
					urb->status is nonzero
					info->urb = NULL;
					usb_free_urb(urb);
	usb_kill_urb(info->urb);
	But info->urb has just been set to NULL!

My original point was that two different routines,
dummy_cb() and usbtest_stop(), both accessed info->urb with no
protection.  Of course, if you use anchors then you won't have to worry 
about this scenario.

Alan Stern