| From foo@baz Thu Apr 10 22:03:04 PDT 2014 |
| From: Oliver Neukum <oneukum@suse.de> |
| Date: Wed, 26 Mar 2014 14:32:51 +0100 |
| Subject: usbnet: include wait queue head in device structure |
| |
| From: Oliver Neukum <oneukum@suse.de> |
| |
| [ Upstream commit 14a0d635d18d0fb552dcc979d6d25106e6541f2e ] |
| |
| This fixes a race which happens by freeing an object on the stack. |
| Quoting Julius: |
| > The issue is |
| > that it calls usbnet_terminate_urbs() before that, which temporarily |
| > installs a waitqueue in dev->wait in order to be able to wait on the |
| > tasklet to run and finish up some queues. The waiting itself looks |
| > okay, but the access to 'dev->wait' is totally unprotected and can |
| > race arbitrarily. I think in this case usbnet_bh() managed to succeed |
| > it's dev->wait check just before usbnet_terminate_urbs() sets it back |
| > to NULL. The latter then finishes and the waitqueue_t structure on its |
| > stack gets overwritten by other functions halfway through the |
| > wake_up() call in usbnet_bh(). |
| |
| The fix is to just not allocate the data structure on the stack. |
| As dev->wait is abused as a flag it also takes a runtime PM change |
| to fix this bug. |
| |
| Signed-off-by: Oliver Neukum <oneukum@suse.de> |
| Reported-by: Grant Grundler <grundler@google.com> |
| Tested-by: Grant Grundler <grundler@google.com> |
| Signed-off-by: David S. Miller <davem@davemloft.net> |
| Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> |
| --- |
| drivers/net/usb/usbnet.c | 33 +++++++++++++++++++-------------- |
| include/linux/usb/usbnet.h | 2 +- |
| 2 files changed, 20 insertions(+), 15 deletions(-) |
| |
| --- a/drivers/net/usb/usbnet.c |
| +++ b/drivers/net/usb/usbnet.c |
| @@ -753,14 +753,12 @@ EXPORT_SYMBOL_GPL(usbnet_unlink_rx_urbs) |
| // precondition: never called in_interrupt |
| static void usbnet_terminate_urbs(struct usbnet *dev) |
| { |
| - DECLARE_WAIT_QUEUE_HEAD_ONSTACK(unlink_wakeup); |
| DECLARE_WAITQUEUE(wait, current); |
| int temp; |
| |
| /* ensure there are no more active urbs */ |
| - add_wait_queue(&unlink_wakeup, &wait); |
| + add_wait_queue(&dev->wait, &wait); |
| set_current_state(TASK_UNINTERRUPTIBLE); |
| - dev->wait = &unlink_wakeup; |
| temp = unlink_urbs(dev, &dev->txq) + |
| unlink_urbs(dev, &dev->rxq); |
| |
| @@ -774,15 +772,14 @@ static void usbnet_terminate_urbs(struct |
| "waited for %d urb completions\n", temp); |
| } |
| set_current_state(TASK_RUNNING); |
| - dev->wait = NULL; |
| - remove_wait_queue(&unlink_wakeup, &wait); |
| + remove_wait_queue(&dev->wait, &wait); |
| } |
| |
| int usbnet_stop (struct net_device *net) |
| { |
| struct usbnet *dev = netdev_priv(net); |
| struct driver_info *info = dev->driver_info; |
| - int retval; |
| + int retval, pm; |
| |
| clear_bit(EVENT_DEV_OPEN, &dev->flags); |
| netif_stop_queue (net); |
| @@ -792,6 +789,8 @@ int usbnet_stop (struct net_device *net) |
| net->stats.rx_packets, net->stats.tx_packets, |
| net->stats.rx_errors, net->stats.tx_errors); |
| |
| + /* to not race resume */ |
| + pm = usb_autopm_get_interface(dev->intf); |
| /* allow minidriver to stop correctly (wireless devices to turn off |
| * radio etc) */ |
| if (info->stop) { |
| @@ -818,6 +817,9 @@ int usbnet_stop (struct net_device *net) |
| dev->flags = 0; |
| del_timer_sync (&dev->delay); |
| tasklet_kill (&dev->bh); |
| + if (!pm) |
| + usb_autopm_put_interface(dev->intf); |
| + |
| if (info->manage_power && |
| !test_and_clear_bit(EVENT_NO_RUNTIME_PM, &dev->flags)) |
| info->manage_power(dev, 0); |
| @@ -1438,11 +1440,12 @@ static void usbnet_bh (unsigned long par |
| /* restart RX again after disabling due to high error rate */ |
| clear_bit(EVENT_RX_KILL, &dev->flags); |
| |
| - // waiting for all pending urbs to complete? |
| - if (dev->wait) { |
| - if ((dev->txq.qlen + dev->rxq.qlen + dev->done.qlen) == 0) { |
| - wake_up (dev->wait); |
| - } |
| + /* waiting for all pending urbs to complete? |
| + * only then can we forgo submitting anew |
| + */ |
| + if (waitqueue_active(&dev->wait)) { |
| + if (dev->txq.qlen + dev->rxq.qlen + dev->done.qlen == 0) |
| + wake_up_all(&dev->wait); |
| |
| // or are we maybe short a few urbs? |
| } else if (netif_running (dev->net) && |
| @@ -1581,6 +1584,7 @@ usbnet_probe (struct usb_interface *udev |
| dev->driver_name = name; |
| dev->msg_enable = netif_msg_init (msg_level, NETIF_MSG_DRV |
| | NETIF_MSG_PROBE | NETIF_MSG_LINK); |
| + init_waitqueue_head(&dev->wait); |
| skb_queue_head_init (&dev->rxq); |
| skb_queue_head_init (&dev->txq); |
| skb_queue_head_init (&dev->done); |
| @@ -1792,9 +1796,10 @@ int usbnet_resume (struct usb_interface |
| spin_unlock_irq(&dev->txq.lock); |
| |
| if (test_bit(EVENT_DEV_OPEN, &dev->flags)) { |
| - /* handle remote wakeup ASAP */ |
| - if (!dev->wait && |
| - netif_device_present(dev->net) && |
| + /* handle remote wakeup ASAP |
| + * we cannot race against stop |
| + */ |
| + if (netif_device_present(dev->net) && |
| !timer_pending(&dev->delay) && |
| !test_bit(EVENT_RX_HALT, &dev->flags)) |
| rx_alloc_submit(dev, GFP_NOIO); |
| --- a/include/linux/usb/usbnet.h |
| +++ b/include/linux/usb/usbnet.h |
| @@ -30,7 +30,7 @@ struct usbnet { |
| struct driver_info *driver_info; |
| const char *driver_name; |
| void *driver_priv; |
| - wait_queue_head_t *wait; |
| + wait_queue_head_t wait; |
| struct mutex phy_mutex; |
| unsigned char suspend_count; |
| unsigned char pkt_cnt, pkt_err; |