When SLUB debugging is enabled in the kernel, and the boot command includes
the option "slub_debug=P", rtl8187 encounters a GPF due to a read-after-free
of a urb.
Following the example of changes in p54usb to fix the same problem, the code
has been modified to use the usb_anchor_urb() method, which avoids the problem.
Signed-off-by: Larry Finger <[email protected]>
---
Index: wireless-testing/drivers/net/wireless/rtl818x/rtl8187.h
===================================================================
--- wireless-testing.orig/drivers/net/wireless/rtl818x/rtl8187.h
+++ wireless-testing/drivers/net/wireless/rtl818x/rtl8187.h
@@ -99,6 +99,7 @@ struct rtl8187_priv {
struct ieee80211_supported_band band;
struct usb_device *udev;
u32 rx_conf;
+ struct usb_anchor anchored;
u16 txpwr_base;
u8 asic_rev;
u8 is_rtl8187b;
@@ -115,7 +116,6 @@ struct rtl8187_priv {
u8 aifsn[4];
struct {
__le64 buf;
- struct urb *urb;
struct sk_buff_head queue;
} b_tx_status;
};
Index: wireless-testing/drivers/net/wireless/rtl818x/rtl8187_dev.c
===================================================================
--- wireless-testing.orig/drivers/net/wireless/rtl818x/rtl8187_dev.c
+++ wireless-testing/drivers/net/wireless/rtl818x/rtl8187_dev.c
@@ -99,7 +99,6 @@ static const struct ieee80211_channel rt
static void rtl8187_iowrite_async_cb(struct urb *urb)
{
kfree(urb->context);
- usb_free_urb(urb);
}
static void rtl8187_iowrite_async(struct rtl8187_priv *priv, __le16 addr,
@@ -136,11 +135,13 @@ static void rtl8187_iowrite_async(struct
usb_fill_control_urb(urb, priv->udev, usb_sndctrlpipe(priv->udev, 0),
(unsigned char *)dr, buf, len,
rtl8187_iowrite_async_cb, buf);
+ usb_anchor_urb(urb, &priv->anchored);
rc = usb_submit_urb(urb, GFP_ATOMIC);
if (rc < 0) {
kfree(buf);
- usb_free_urb(urb);
+ usb_unanchor_urb(urb);
}
+ usb_free_urb(urb);
}
static inline void rtl818x_iowrite32_async(struct rtl8187_priv *priv,
@@ -172,7 +173,6 @@ static void rtl8187_tx_cb(struct urb *ur
struct ieee80211_hw *hw = info->rate_driver_data[0];
struct rtl8187_priv *priv = hw->priv;
- usb_free_urb(info->rate_driver_data[1]);
skb_pull(skb, priv->is_rtl8187b ? sizeof(struct rtl8187b_tx_hdr) :
sizeof(struct rtl8187_tx_hdr));
ieee80211_tx_info_clear_status(info);
@@ -273,11 +273,13 @@ static int rtl8187_tx(struct ieee80211_h
usb_fill_bulk_urb(urb, priv->udev, usb_sndbulkpipe(priv->udev, ep),
buf, skb->len, rtl8187_tx_cb, skb);
+ usb_anchor_urb(urb, &priv->anchored);
rc = usb_submit_urb(urb, GFP_ATOMIC);
if (rc < 0) {
- usb_free_urb(urb);
+ usb_unanchor_urb(urb);
kfree_skb(skb);
}
+ usb_free_urb(urb);
return 0;
}
@@ -301,14 +303,13 @@ static void rtl8187_rx_cb(struct urb *ur
return;
}
spin_unlock(&priv->rx_queue.lock);
+ skb_put(skb, urb->actual_length);
if (unlikely(urb->status)) {
- usb_free_urb(urb);
dev_kfree_skb_irq(skb);
return;
}
- skb_put(skb, urb->actual_length);
if (!priv->is_rtl8187b) {
struct rtl8187_rx_hdr *hdr =
(typeof(hdr))(skb_tail_pointer(skb) - sizeof(*hdr));
@@ -361,7 +362,6 @@ static void rtl8187_rx_cb(struct urb *ur
skb = dev_alloc_skb(RTL8187_MAX_RX);
if (unlikely(!skb)) {
- usb_free_urb(urb);
/* TODO check rx queue length and refill *somewhere* */
return;
}
@@ -373,24 +373,30 @@ static void rtl8187_rx_cb(struct urb *ur
urb->context = skb;
skb_queue_tail(&priv->rx_queue, skb);
- usb_submit_urb(urb, GFP_ATOMIC);
+ usb_anchor_urb(urb, &priv->anchored);
+ if (usb_submit_urb(urb, GFP_ATOMIC))
+ usb_unanchor_urb(urb);
}
static int rtl8187_init_urbs(struct ieee80211_hw *dev)
{
struct rtl8187_priv *priv = dev->priv;
- struct urb *entry;
+ struct urb *entry = NULL;
struct sk_buff *skb;
struct rtl8187_rx_info *info;
+ int ret = 0;
while (skb_queue_len(&priv->rx_queue) < 8) {
skb = __dev_alloc_skb(RTL8187_MAX_RX, 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,
@@ -401,10 +407,22 @@ static int rtl8187_init_urbs(struct ieee
info->urb = entry;
info->dev = dev;
skb_queue_tail(&priv->rx_queue, skb);
- usb_submit_urb(entry, GFP_KERNEL);
+ usb_anchor_urb(entry, &priv->anchored);
+ 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);
}
+ return ret;
- return 0;
+err:
+ usb_free_urb(entry);
+ kfree_skb(skb);
+ usb_kill_anchored_urbs(&priv->anchored);
+ return ret;
}
static void rtl8187b_status_cb(struct urb *urb)
@@ -415,7 +433,6 @@ static void rtl8187b_status_cb(struct ur
unsigned int cmd_type;
if (unlikely(urb->status)) {
- usb_free_urb(urb);
return;
}
@@ -488,26 +505,32 @@ static void rtl8187b_status_cb(struct ur
spin_unlock_irqrestore(&priv->b_tx_status.queue.lock, flags);
}
- usb_submit_urb(urb, GFP_ATOMIC);
+ usb_anchor_urb(urb, &priv->anchored);
+ if (usb_submit_urb(urb, GFP_ATOMIC))
+ usb_unanchor_urb(urb);
}
static int rtl8187b_init_status_urb(struct ieee80211_hw *dev)
{
struct rtl8187_priv *priv = dev->priv;
struct urb *entry;
+ int ret = 0;
entry = usb_alloc_urb(0, GFP_KERNEL);
if (!entry)
return -ENOMEM;
- priv->b_tx_status.urb = entry;
usb_fill_bulk_urb(entry, priv->udev, usb_rcvbulkpipe(priv->udev, 9),
&priv->b_tx_status.buf, sizeof(priv->b_tx_status.buf),
rtl8187b_status_cb, dev);
- usb_submit_urb(entry, GFP_KERNEL);
+ usb_anchor_urb(entry, &priv->anchored);
+ ret = usb_submit_urb(entry, GFP_KERNEL);
+ if (ret)
+ usb_unanchor_urb(entry);
+ usb_free_urb(entry);
- return 0;
+ return ret;
}
static int rtl8187_cmd_reset(struct ieee80211_hw *dev)
@@ -841,6 +864,9 @@ static int rtl8187_start(struct ieee8021
return ret;
mutex_lock(&priv->conf_mutex);
+
+ init_usb_anchor(&priv->anchored);
+
if (priv->is_rtl8187b) {
reg = RTL818X_RX_CONF_MGMT |
RTL818X_RX_CONF_DATA |
@@ -936,12 +962,12 @@ static void rtl8187_stop(struct ieee8021
while ((skb = skb_dequeue(&priv->rx_queue))) {
info = (struct rtl8187_rx_info *)skb->cb;
- usb_kill_urb(info->urb);
kfree_skb(skb);
}
while ((skb = skb_dequeue(&priv->b_tx_status.queue)))
dev_kfree_skb_any(skb);
- usb_kill_urb(priv->b_tx_status.urb);
+
+ usb_kill_anchored_urbs(&priv->anchored);
mutex_unlock(&priv->conf_mutex);
}
Hin-Tak Leung wrote:
>
> Just built it and using it right now.
Does this patch have any effect on bugzilla entry
11887(http://bugzilla.kernel.org/show_bug.cgi?id=11887)?
Larry
Larry Finger wrote:
> Hin-Tak Leung wrote:
>> Just built it and using it right now.
>
> Does this patch have any effect on bugzilla entry
> 11887(http://bugzilla.kernel.org/show_bug.cgi?id=11887)?
No, still requires manually unloading. (i.e. I removed my current
SUSPEND_MODULES pm defines, and it hangs on suspend). Need to look elsewhere
or fill in some suspend()/resume() routines, I guess.
Hin-Tak
On Wed, 2008-12-10 at 00:19 +0000, Hin-Tak Leung wrote:
> Larry Finger wrote:
> > Hin-Tak Leung wrote:
> >> Just built it and using it right now.
> >
> > Does this patch have any effect on bugzilla entry
> > 11887(http://bugzilla.kernel.org/show_bug.cgi?id=11887)?
>
> No, still requires manually unloading. (i.e. I removed my current
> SUSPEND_MODULES pm defines, and it hangs on suspend). Need to look elsewhere
> or fill in some suspend()/resume() routines, I guess.
The same happens for ar9170 right now, I was putting the blame on the
firmware loading but I'm not entirely sure. If you find anything here
I'd be interested, no time right now try finding the bug myself
unfortunately.
johannes
Larry Finger wrote:
> When SLUB debugging is enabled in the kernel, and the boot command includes
> the option "slub_debug=P", rtl8187 encounters a GPF due to a read-after-free
> of a urb.
>
> Following the example of changes in p54usb to fix the same problem, the code
> has been modified to use the usb_anchor_urb() method, which avoids the problem.
>
> Signed-off-by: Larry Finger <[email protected]>
Tested-by: Hin-Tak Leung <[email protected]>
Just built it and using it right now.
Hin-Tak