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

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>
Signed-off-by: John W. Linville <linville@tuxdriver.com>

2 files changed, 102 insertions, 50 deletions

diff --git a/drivers/net/wireless/p54/p54usb.c b/drivers/net/wireless/p54/p54usb.c
index 2dd3cd41d0fe..c2789e53b984 100644
--- 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;
                 }
@@ -115,7 +114,6 @@ static void p54u_rx_cb(struct urb *urb)
                 info->dev = dev;
                 urb->transfer_buffer = skb_tail_pointer(skb);
                 urb->context = skb;
-                skb_queue_tail(&priv->rx_queue, skb);
         } else {
                 if (priv->hw_type == 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 = 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);
+        }
 }
 
 static void p54u_tx_reuse_skb_cb(struct urb *urb)
@@ -144,18 +145,6 @@ static void p54u_tx_reuse_skb_cb(struct urb *urb)
                 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 +154,36 @@ static void p54u_tx_free_skb_cb(struct urb *urb)
                 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 +192,25 @@ static int p54u_init_urbs(struct ieee80211_hw *dev)
                 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 +218,7 @@ static void p54u_tx_3887(struct ieee80211_hw *dev, struct sk_buff *skb,
 {
         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 +233,31 @@ static void p54u_tx_3887(struct ieee80211_hw *dev, struct sk_buff *skb,
         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 +297,13 @@ static void p54u_tx_lm87(struct ieee80211_hw *dev, struct sk_buff *skb,
                           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 +313,7 @@ static void p54u_tx_net2280(struct ieee80211_hw *dev, struct sk_buff *skb,
         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)
@@ -320,15 +343,42 @@ static void p54u_tx_net2280(struct ieee80211_hw *dev, struct sk_buff *skb,
 
         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 |= URB_FREE_BUFFER;
 
         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,
@@ -885,6 +935,7 @@ static int __devinit p54u_probe(struct usb_interface *intf,
                 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 --git a/drivers/net/wireless/p54/p54usb.h b/drivers/net/wireless/p54/p54usb.h
index 5b8fe91379c3..54ee738bf2af 100644
--- 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 */