p54usb: fix usb_kill_urb hang with slub_debug=P

PATCH In review Christian Lamparter 4 participants 11 messages Dec 5, 2008 – Dec 8, 2008 Networking drivers (wireless)

lore

Share & tools

lore

Christian Lamparter

author

2008-12-05 14:47 UTC #0 lore raw

PATCH 1/1 p54usb: fix usb_kill_urb hang with slub_debug=P

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 <[email protected]>

Signed-off-by Christian Lamparter <[email protected]>

CC [email protected]

---

drivers/net/wireless/p54/p54usb.c +7 -0

index 3444f52..0b9a922 100644
@@ -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);
 	}
 }

Larry Finger

2008-12-05 15:14 UTC #1 lore raw

Quoted 6 lines · 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

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

Larry

Christian Lamparter

author

2008-12-05 15:58 UTC #2 lore raw

Quoted 10 lines · On Friday 05 December 2008 16:14:09 Larry Finger wrote:

Quoted 6 lines · 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
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

Greg KH

2008-12-05 15:58 UTC #3 lore raw

In reply to Christian Lamparter

Quoted 1 line · Christian Lamparter

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

Larry Finger

2008-12-05 23:18 UTC #4 lore raw

Quoted 5 lines · Greg KH

Quoted 1 line · Christian Lamparter
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);

Quoted 1 line

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

Greg KH

2008-12-05 23:24 UTC #5 lore raw

Quoted 11 lines · Larry Finger

Quoted 5 lines · Greg KH
Quoted 1 line · Christian Lamparter
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:

Quoted 8 lines

        if (kobj)
                kref_get(&kobj->kref);
Quoted 1 line
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

Christian Lamparter

author

2008-12-06 20:09 UTC #6 lore raw

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

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
---

drivers/net/wireless/p54/p54usb.c +93 ~~-46~~

diff -Nurp a/drivers/net/wireless/p54/p54usb.c b/drivers/net/wireless/p54/p54usb.c
@@ -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);

drivers/net/wireless/p54/p54usb.h +1 -0

diff -Nurp a/drivers/net/wireless/p54/p54usb.h b/drivers/net/wireless/p54/p54usb.h
@@ -133,6 +133,7 @@ struct p54u_priv {
 
 	spinlock_t lock;
 	struct sk_buff_head rx_queue;
+	struct usb_anchor submitted;
 };
 
 #endif /* P54USB_H */

Larry Finger

2008-12-06 21:47 UTC #7 lore raw

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

Quoted 10 lines · Christian Lamparter

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

Alan Stern

2008-12-07 04:15 UTC #8 lore raw

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

Quoted 9 lines · Larry Finger

Quoted 8 lines · Christian Lamparter
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."

Quoted 7 lines

Quoted 2 lines
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

Christian Lamparter

author

2008-12-08 14:07 UTC #9 lore raw

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

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.
---

Quoted 1 line

I looked quickly at your new code. It seems basically right, but the=

Quoted 1 line

are few things that still need attention. For instance, you removed =

Quoted 1 line

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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Christian Lamparter

author

2008-12-05 23:28 UTC #10 lore raw

In reply to Larry Finger

Quoted 14 lines · On Saturday 06 December 2008 00:18:07 Larry Finger wrote:

Quoted 5 lines · Greg KH
Quoted 1 line · Christian Lamparter
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);

Quoted 8 lines

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 <[email protected]>
To: Christian Lamparter <[email protected]>

Quoted 29 lines · Christian Lamparter

Quoted 5 lines
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;
}

Quoted 7 lines

###
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