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

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

This patch addresses all known limitations in the old implementation an=
d fixes
khub's "use-after-freed" hang, when SLUB debug's poisoning option is en=
abled.

Signed-off-by: Christian Lamparter <chunkeey@web.de>
Cc: stable@kernel.org
---
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 v2] p54usb: rewriting rx/tx routines to make use of usb_anchor's facilities

[John W. Linville]

On Tue, Dec 09, 2008 at 03:14:37PM +0100, Christian Lamparter wrote:
> Alan Stern found several flaws in p54usb's implementation and annotat=
ed:=20
> "usb_kill_urb() and similar routines do not expect an URB's completio=
n
> routine to deallocate it. =A0This is almost obvious -- if the URB is =
deallocated
> before the completion routine returns then there's no way for usb_kil=
l_urb
> to detect when the URB actually is complete."
>=20
> This patch addresses all known limitations in the old implementation =
and fixes
> khub's "use-after-freed" hang, when SLUB debug's poisoning option is =
enabled.
>=20
> Signed-off-by: Christian Lamparter <chunkeey@web.de>
> Cc: stable@kernel.org

That's a big patch for so late in the cycle.  Is there a bugzilla open =
for it?

John
--=20
John W. Linville		Linux should be at the core
linville@tuxdriver.com			of your literate lifestyle.
--
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 v2] p54usb: rewriting rx/tx routines to make use of usb_anchor's facilities

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 1142 bytes --]

On Tue, 2008-12-09 at 09:35 -0500, John W. Linville wrote:
> On Tue, Dec 09, 2008 at 03:14:37PM +0100, Christian Lamparter wrote:
> > 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."
> > 
> > This patch addresses all known limitations in the old implementation and fixes
> > khub's "use-after-freed" hang, when SLUB debug's poisoning option is enabled.
> > 
> > Signed-off-by: Christian Lamparter <chunkeey@web.de>
> > Cc: stable@kernel.org
> 
> That's a big patch for so late in the cycle.  Is there a bugzilla open for it?

No, but we can open one ;) It's strange, but nobody but me apparently
found the bug so far, even though it has been in there forever. Might be
due to slub debugging being disabled by default even if you enable
SLUB_DEBUG in Kconfig (you also need to enable SLUB_DEBUG_ON)

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[John W. Linville]

On Tue, Dec 09, 2008 at 03:46:39PM +0100, Johannes Berg wrote:
> On Tue, 2008-12-09 at 09:35 -0500, John W. Linville wrote:
> > On Tue, Dec 09, 2008 at 03:14:37PM +0100, Christian Lamparter wrote:
> > > 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."
> > > 
> > > This patch addresses all known limitations in the old implementation and fixes
> > > khub's "use-after-freed" hang, when SLUB debug's poisoning option is enabled.
> > > 
> > > Signed-off-by: Christian Lamparter <chunkeey@web.de>
> > > Cc: stable@kernel.org
> > 
> > That's a big patch for so late in the cycle.  Is there a bugzilla open for it?
> 
> No, but we can open one ;) It's strange, but nobody but me apparently
> found the bug so far, even though it has been in there forever. Might be
> due to slub debugging being disabled by default even if you enable
> SLUB_DEBUG in Kconfig (you also need to enable SLUB_DEBUG_ON)

It might be helpful to have open a bug that includes a backtrace or something.

John
-- 
John W. Linville		Linux should be at the core
linville@tuxdriver.com			of your literate lifestyle.

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

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 1545 bytes --]

On Tue, 2008-12-09 at 09:54 -0500, John W. Linville wrote:
> On Tue, Dec 09, 2008 at 03:46:39PM +0100, Johannes Berg wrote:
> > On Tue, 2008-12-09 at 09:35 -0500, John W. Linville wrote:
> > > On Tue, Dec 09, 2008 at 03:14:37PM +0100, Christian Lamparter wrote:
> > > > 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."
> > > > 
> > > > This patch addresses all known limitations in the old implementation and fixes
> > > > khub's "use-after-freed" hang, when SLUB debug's poisoning option is enabled.
> > > > 
> > > > Signed-off-by: Christian Lamparter <chunkeey@web.de>
> > > > Cc: stable@kernel.org
> > > 
> > > That's a big patch for so late in the cycle.  Is there a bugzilla open for it?
> > 
> > No, but we can open one ;) It's strange, but nobody but me apparently
> > found the bug so far, even though it has been in there forever. Might be
> > due to slub debugging being disabled by default even if you enable
> > SLUB_DEBUG in Kconfig (you also need to enable SLUB_DEBUG_ON)
> 
> It might be helpful to have open a bug that includes a backtrace or something.

There's no backtrace in that sense, it just hangs. I dumped it with
sysrq-w but that wasn't too helpful...

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Larry Finger]

John W. Linville wrote:
> 
> That's a big patch for so late in the cycle.  Is there a bugzilla open for it?

There is a similar patch coming for rtl8187. I'll also open a bugzilla entry for
it as well.

Larry

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

[Larry Finger]

Johannes Berg wrote:
> 
> There's no backtrace in that sense, it just hangs. I dumped it with
> sysrq-w but that wasn't too helpful...
> 

On x86_64 with rtl8187, I get a GPF due to a pointer being "poisoned". Perhaps
my platform will get a similar backtrace with p54usb. I hadn't tried it.

Larry

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

[Larry Finger]

Christian Lamparter wrote:
> 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."
> 
> This patch addresses all known limitations in the old implementation and fixes
> khub's "use-after-freed" hang, when SLUB debug's poisoning option is enabled.
> 
> Signed-off-by: Christian Lamparter <chunkeey@web.de>
> Cc: stable@kernel.org
> ---

Tested-by: Larry Finger <Larry.Finger@lwfinger.net>
---

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

[Larry Finger]

Larry Finger wrote:
> Christian Lamparter wrote:
>> 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."
>>
>> This patch addresses all known limitations in the old implementation and fixes
>> khub's "use-after-freed" hang, when SLUB debug's poisoning option is enabled.
>>
>> Signed-off-by: Christian Lamparter <chunkeey@web.de>
>> Cc: stable@kernel.org
>> ---
> 
> Tested-by: Larry Finger <Larry.Finger@lwfinger.net>
> ---

For reference, the Bugzilla Entry for the p54usb problem is #12186. On my system
without the patch, 'modprobe -r p54usb' just hangs. There is no crash dump and
the process can be killed.

Larry