All of lore.kernel.org
 help / color / mirror / Atom feed
From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To: linux-kernel@vger.kernel.org
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	stable@vger.kernel.org, Oliver Neukum <oneukum@suse.de>,
	Grant Grundler <grundler@google.com>,
	"David S. Miller" <davem@davemloft.net>
Subject: [PATCH 3.10 25/41] usbnet: include wait queue head in device structure
Date: Fri, 11 Apr 2014 09:09:56 -0700	[thread overview]
Message-ID: <20140411160934.645221901@linuxfoundation.org> (raw)
In-Reply-To: <20140411160932.865173041@linuxfoundation.org>

3.10-stable review patch.  If anyone has any objections, please let me know.

------------------

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
@@ -727,14 +727,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);
 
@@ -748,15 +746,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);
@@ -766,6 +763,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) {
@@ -792,6 +791,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);
@@ -1360,11 +1362,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) &&
@@ -1502,6 +1505,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);
@@ -1694,9 +1698,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;



  parent reply	other threads:[~2014-04-11 16:08 UTC|newest]

Thread overview: 45+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-04-11 16:09 [PATCH 3.10 00/41] 3.10.37-stable review Greg Kroah-Hartman
2014-04-11 16:09 ` [PATCH 3.10 01/41] selinux: correctly label /proc inodes in use before the policy is loaded Greg Kroah-Hartman
2014-04-11 16:09 ` [PATCH 3.10 02/41] powernow-k6: disable cache when changing frequency Greg Kroah-Hartman
2014-04-11 16:09 ` [PATCH 3.10 03/41] powernow-k6: correctly initialize default parameters Greg Kroah-Hartman
2014-04-11 16:09 ` [PATCH 3.10 04/41] powernow-k6: reorder frequencies Greg Kroah-Hartman
2014-04-11 16:09 ` [PATCH 3.10 05/41] kbuild: fix make headers_install when path is too long Greg Kroah-Hartman
2014-04-11 16:09 ` [PATCH 3.10 06/41] cpuidle: Check the result of cpuidle_get_driver() against NULL Greg Kroah-Hartman
2014-04-11 16:09 ` [PATCH 3.10 07/41] net: fix for a race condition in the inet frag code Greg Kroah-Hartman
2014-04-11 16:09 ` [PATCH 3.10 08/41] net: sctp: fix skb leakage in COOKIE ECHO path of chunk->auth_chunk Greg Kroah-Hartman
2014-04-11 16:09 ` [PATCH 3.10 09/41] bridge: multicast: add sanity check for query source addresses Greg Kroah-Hartman
2014-04-11 16:09 ` [PATCH 3.10 10/41] inet: frag: make sure forced eviction removes all frags Greg Kroah-Hartman
2014-04-11 16:09 ` [PATCH 3.10 11/41] net: unix: non blocking recvmsg() should not return -EINTR Greg Kroah-Hartman
2014-04-11 16:21   ` Rainer Weikusat
2014-04-11 16:09 ` [PATCH 3.10 12/41] ipv6: Fix exthdrs offload registration Greg Kroah-Hartman
2014-04-11 16:09 ` [PATCH 3.10 13/41] ipv6: dont set DST_NOCOUNT for remotely added routes Greg Kroah-Hartman
2014-04-11 16:09 ` [PATCH 3.10 14/41] vlan: Set correct source MAC address with TX VLAN offload enabled Greg Kroah-Hartman
2014-04-11 16:09 ` [PATCH 3.10 15/41] tcp: tcp_release_cb() should release socket ownership Greg Kroah-Hartman
2014-04-11 16:09 ` [PATCH 3.10 16/41] net: socket: error on a negative msg_namelen Greg Kroah-Hartman
2014-04-11 16:09 ` [PATCH 3.10 17/41] ipv6: Avoid unnecessary temporary addresses being generated Greg Kroah-Hartman
2014-04-11 16:09 ` [PATCH 3.10 18/41] ipv6: ip6_append_data_mtu do not handle the mtu of the second fragment properly Greg Kroah-Hartman
2014-04-11 16:09 ` [PATCH 3.10 19/41] vxlan: fix potential NULL dereference in arp_reduce() Greg Kroah-Hartman
2014-04-11 16:09 ` [PATCH 3.10 20/41] rtnetlink: fix fdb notification flags Greg Kroah-Hartman
2014-04-11 16:09 ` [PATCH 3.10 21/41] ipmr: fix mfc " Greg Kroah-Hartman
2014-04-11 16:09 ` [PATCH 3.10 22/41] ip6mr: " Greg Kroah-Hartman
2014-04-11 16:09 ` [PATCH 3.10 23/41] netpoll: fix the skb check in pkt_is_ns Greg Kroah-Hartman
2014-04-11 16:09 ` [PATCH 3.10 24/41] tg3: Do not include vlan acceleration features in vlan_features Greg Kroah-Hartman
2014-04-11 16:09 ` Greg Kroah-Hartman [this message]
2014-04-11 16:09 ` [PATCH 3.10 26/41] vlan: Set hard_header_len according to available acceleration Greg Kroah-Hartman
2014-04-11 16:09 ` [PATCH 3.10 27/41] vhost: fix total length when packets are too short Greg Kroah-Hartman
2014-04-11 16:09 ` [PATCH 3.10 28/41] vhost: validate vhost_get_vq_desc return value Greg Kroah-Hartman
2014-04-11 16:10 ` [PATCH 3.10 29/41] xen-netback: remove pointless clause from if statement Greg Kroah-Hartman
2014-04-11 16:10 ` [PATCH 3.10 30/41] ipv6: some ipv6 statistic counters failed to disable bh Greg Kroah-Hartman
2014-04-11 16:10 ` [PATCH 3.10 31/41] netlink: dont compare the nul-termination in nla_strcmp Greg Kroah-Hartman
2014-04-11 16:10 ` [PATCH 3.10 32/41] isdnloop: Validate NUL-terminated strings from user Greg Kroah-Hartman
2014-04-11 16:10 ` [PATCH 3.10 33/41] isdnloop: several buffer overflows Greg Kroah-Hartman
2014-04-11 16:10 ` [PATCH 3.10 34/41] rds: prevent dereference of a NULL device in rds_iw_laddr_check Greg Kroah-Hartman
2014-04-11 16:10 ` [PATCH 3.10 35/41] ARC: [nsimosci] Change .dts to use generic 8250 UART Greg Kroah-Hartman
2014-04-11 16:10 ` [PATCH 3.10 36/41] ARC: [nsimosci] Unbork console Greg Kroah-Hartman
2014-04-11 16:10 ` [PATCH 3.10 37/41] futex: Allow architectures to skip futex_atomic_cmpxchg_inatomic() test Greg Kroah-Hartman
2014-04-11 16:10 ` [PATCH 3.10 38/41] m68k: Skip " Greg Kroah-Hartman
2014-04-11 16:10 ` [PATCH 3.10 39/41] crypto: ghash-clmulni-intel - use C implementation for setkey() Greg Kroah-Hartman
2014-04-11 16:10 ` [PATCH 3.10 40/41] cpufreq: Fix governor start/stop race condition Greg Kroah-Hartman
2014-04-11 16:10 ` [PATCH 3.10 41/41] cpufreq: Fix timer/workqueue corruption due to double queueing Greg Kroah-Hartman
2014-04-11 21:44 ` [PATCH 3.10 00/41] 3.10.37-stable review Guenter Roeck
2014-04-11 23:45 ` Shuah Khan

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20140411160934.645221901@linuxfoundation.org \
    --to=gregkh@linuxfoundation.org \
    --cc=davem@davemloft.net \
    --cc=grundler@google.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=oneukum@suse.de \
    --cc=stable@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.