Several races in "usbnet" module (kernel 4.1.x)

Hi,

I have recently found several data races in "usbnet" module, checked on vanilla kernel 4.1.0 on x86_64. The races do actually happen, I have confirmed it by adding delays and using hardware breakpoints to detect the conflicting memory accesses (with RaceHound tool, https://github.com/winnukem/racehound).

I have not analyzed yet how harmful these races are (if they are), but it is better to report them anyway, I think.

Everything was checked using YOTA 4G LTE Modem that works via "usbnet" and "cdc_ether" kernel modules.
--------------------------

[Race #1]

Race on skb_queue ('next' pointer) between usbnet_stop() and rx_complete().

Reproduced that by unplugging the device while the system was downloading a large file from the Net.

Here is part of the call stack with the code where the changes to the queue happen:

#0 __skb_unlink (skbuff.h:1517)	
	prev->next = next;
#1 defer_bh (usbnet.c:430)
	spin_lock_irqsave(&list->lock, flags);
	old_state = entry->state;
	entry->state = state;
	__skb_unlink(skb, list);
	spin_unlock(&list->lock);
	spin_lock(&dev->done.lock);
	__skb_queue_tail(&dev->done, skb);
	if (dev->done.qlen == 1)
		tasklet_schedule(&dev->bh);
	spin_unlock_irqrestore(&dev->done.lock, flags);
#2 rx_complete (usbnet.c:640)
	state = defer_bh(dev, skb, &dev->rxq, state);

At the same time, the following code repeatedly checks if the queue is empty and reads the same values concurrently with the above changes:

#0  usbnet_terminate_urbs (usbnet.c:765)
	/* maybe wait for deletions to finish. */
	while (!skb_queue_empty(&dev->rxq)
		&& !skb_queue_empty(&dev->txq)
		&& !skb_queue_empty(&dev->done)) {
			schedule_timeout(msecs_to_jiffies(UNLINK_TIMEOUT_MS));
			set_current_state(TASK_UNINTERRUPTIBLE);
			netif_dbg(dev, ifdown, dev->net,
				  "waited for %d urb completions\n", temp);
	}
#1  usbnet_stop (usbnet.c:806)
	if (!(info->flags & FLAG_AVOID_UNLINK_URBS))
		usbnet_terminate_urbs(dev);

For example, it is possible that the skb is removed from dev->rxq by __skb_unlink() before the check "!skb_queue_empty(&dev->rxq)" in usbnet_terminate_urbs() is made. It is also possible in this case that the skb is added to dev->done queue after "!skb_queue_empty(&dev->done)" is checked. So usbnet_terminate_urbs() may stop waiting and return while dev->done queue still has an item.
--------------------------

Unrelated the that race, if the goal of that while loop in usbnet_terminate_urbs() is to wait till all three queues (dev->rxq, dev->txq, dev->done) become empty, perhaps the code should be changed as follows:

	while (!skb_queue_empty(&dev->rxq)
-		&& !skb_queue_empty(&dev->txq)
-		&& !skb_queue_empty(&dev->done)) {
+		|| !skb_queue_empty(&dev->txq)
+		|| !skb_queue_empty(&dev->done)) {
			schedule_timeout(...));
--------------------------

[Race #2]

Races on dev->rx_qlen. Reproduced these by repeatedly changing MTU (1500 <-> 1400) while downloading large files.

dev->rx_qlen is written to here:
#0  usbnet_update_max_qlen (usbnet.c:351)
	case USB_SPEED_HIGH:
		dev->rx_qlen = MAX_QUEUE_MEMORY / dev->rx_urb_size;
#1  __handle_link_change (usbnet.c:1049)
	/* hard_mtu or rx_urb_size may change during link change */
	usbnet_update_max_qlen(dev);
#2  usbnet_deferred_kevent (usbnet.c:1172)
	if (test_bit (EVENT_LINK_CHANGE, &dev->flags))
		__handle_link_change(dev);

Here are the conflicting reads from dev->rx_qlen (via RX_QLEN(dev)), 3 code locations:

* usbnet_bh (usbnet.c:1492)
	if (temp < RX_QLEN(dev)) { ...
* usbnet_bh (usbnet.c:1499)
	if (dev->rxq.qlen < RX_QLEN(dev)) ...
* rx_alloc_submit (usbnet.c:1431)
	for (i = 0; i < 10 && dev->rxq.qlen < RX_QLEN(dev); i++) { ...
--------------------------

[Race #3]

Similar to race #2 but on dev->tx_qlen. I reproduced it the same way.

dev->tx_qlen is written to here:
#0  usbnet_update_max_qlen (usbnet.c:352)
	case USB_SPEED_HIGH:
		dev->rx_qlen = MAX_QUEUE_MEMORY / dev->rx_urb_size;
		dev->tx_qlen = MAX_QUEUE_MEMORY / dev->hard_mtu;
#1  __handle_link_change (usbnet.c:1049)
	/* hard_mtu or rx_urb_size may change during link change */
	usbnet_update_max_qlen(dev);
#2  usbnet_deferred_kevent (usbnet.c:1172)
	if (test_bit (EVENT_LINK_CHANGE, &dev->flags))
		__handle_link_change(dev);

Here are the conflicting reads from dev->tx_qlen (via TX_QLEN(dev)), 2 code locations:

* usbnet_bh (usbnet.c:1502)
		if (dev->txq.qlen < TX_QLEN (dev))
			netif_wake_queue (dev->net);
* usbnet_start_xmit (usbnet.c:1398)
		if (dev->txq.qlen >= TX_QLEN (dev))
			netif_stop_queue (net);
--------------------------

[Race #4]

Race on dev->flags. It happened when I unplugged the device while a large file was being downloaded.

dev->flags is set to 0 here:

#0  usbnet_stop (usbnet.c:816)
	/* deferred work (task, timer, softirq) must also stop.
	 * can't flush_scheduled_work() until we drop rtnl (later),
	 * else workers could deadlock; so make workers a NOP.
	 */
	dev->flags = 0;
	del_timer_sync (&dev->delay);
	tasklet_kill (&dev->bh);

And here, the code clears EVENT_RX_KILL bit in dev->flags, which may execute concurrently with the above operation:
#0 clear_bit (bitops.h:113, inlined)
#1 usbnet_bh (usbnet.c:1475)
	/* restart RX again after disabling due to high error rate */
	clear_bit(EVENT_RX_KILL, &dev->flags);
	
If clear_bit() is atomic w.r.t. setting dev->flags to 0, this race is not a problem, I guess. Otherwise, it may be.
--------------------------

[Race #5]

Race on dev->rx_urb_size. I reproduced it a similar way as the races #2 and #3 (changing MTU while downloading files).

dev->rx_urb_size is written to here:
#0  usbnet_change_mtu (usbnet.c:392)
	dev->rx_urb_size = dev->hard_mtu;

Here is the conflicting read from dev->rx_urb_size:
* rx_submit (usbnet.c:467)
	size_t			size = dev->rx_urb_size;
--------------------------

Regards,
Eugene

-- 
Eugene Shatokhin, ROSA
www.rosalab.com

On Mon, 2015-07-20 at 21:13 +0300, Eugene Shatokhin wrote:

Hi,

I have recently found several data races in "usbnet" module, checked on 
vanilla kernel 4.1.0 on x86_64. The races do actually happen, I have 
confirmed it by adding delays and using hardware breakpoints to detect 
the conflicting memory accesses (with RaceHound tool, 
https://github.com/winnukem/racehound).

I have not analyzed yet how harmful these races are (if they are), but 
it is better to report them anyway, I think.

Everything was checked using YOTA 4G LTE Modem that works via "usbnet" 
and "cdc_ether" kernel modules.
--------------------------

[Race #1]

Race on skb_queue ('next' pointer) between usbnet_stop() and rx_complete().

Reproduced that by unplugging the device while the system was 
downloading a large file from the Net.

Here is part of the call stack with the code where the changes to the 
queue happen:

#0 __skb_unlink (skbuff.h:1517)	
	prev->next = next;
#1 defer_bh (usbnet.c:430)
	spin_lock_irqsave(&list->lock, flags);
	old_state = entry->state;
	entry->state = state;
	__skb_unlink(skb, list);
	spin_unlock(&list->lock);
	spin_lock(&dev->done.lock);
	__skb_queue_tail(&dev->done, skb);
	if (dev->done.qlen == 1)
		tasklet_schedule(&dev->bh);
	spin_unlock_irqrestore(&dev->done.lock, flags);
#2 rx_complete (usbnet.c:640)
	state = defer_bh(dev, skb, &dev->rxq, state);

At the same time, the following code repeatedly checks if the queue is 
empty and reads the same values concurrently with the above changes:

#0  usbnet_terminate_urbs (usbnet.c:765)
	/* maybe wait for deletions to finish. */
	while (!skb_queue_empty(&dev->rxq)
		&& !skb_queue_empty(&dev->txq)
		&& !skb_queue_empty(&dev->done)) {
			schedule_timeout(msecs_to_jiffies(UNLINK_TIMEOUT_MS));
			set_current_state(TASK_UNINTERRUPTIBLE);
			netif_dbg(dev, ifdown, dev->net,
				  "waited for %d urb completions\n", temp);
	}
#1  usbnet_stop (usbnet.c:806)
	if (!(info->flags & FLAG_AVOID_UNLINK_URBS))
		usbnet_terminate_urbs(dev);

For example, it is possible that the skb is removed from dev->rxq by 
__skb_unlink() before the check "!skb_queue_empty(&dev->rxq)" in 
usbnet_terminate_urbs() is made. It is also possible in this case that 
the skb is added to dev->done queue after "!skb_queue_empty(&dev->done)" 
is checked. So usbnet_terminate_urbs() may stop waiting and return while 
dev->done queue still has an item.

Hi,

your analysis is correct and it looks like in addition to your proposed
fix locking needs to be simplified and a common lock to be taken.
Suggestions?

	Regards
		Oliver

On Mon, 2015-07-20 at 21:13 +0300, Eugene Shatokhin wrote:

Races on dev->rx_qlen. Reproduced these by repeatedly changing MTU
(1500 
<-> 1400) while downloading large files.

Hi,

I don't see how it matters much. The number of buffers is just
an optimization. As long as it eventually is corrected I don't
see harm.

	Regards
		Oliver

On Mon, 2015-07-20 at 21:13 +0300, Eugene Shatokhin wrote:

And here, the code clears EVENT_RX_KILL bit in dev->flags, which may 
execute concurrently with the above operation:
#0 clear_bit (bitops.h:113, inlined)
#1 usbnet_bh (usbnet.c:1475)
        /* restart RX again after disabling due to high error rate */
        clear_bit(EVENT_RX_KILL, &dev->flags);
        
If clear_bit() is atomic w.r.t. setting dev->flags to 0, this race is 
not a problem, I guess. Otherwise, it may be.

clear_bit is atomic with respect to other atomic operations.
So how about this:

	Regards
		Oliver

From 1c4e685b3a9c183e04c46b661830e5c7ed35b513 Mon Sep 17 00:00:00 2001

From: Oliver Neukum <oneukum-IBi9RG/b67k@public.gmane.org>
Date: Tue, 21 Jul 2015 16:19:40 +0200
Subject: [PATCH] usbnet: fix race between usbnet_stop() and the BH

Does this do the job?

Signed-off-by: Oliver Neukum <oneukum-IBi9RG/b67k@public.gmane.org>
---
 drivers/net/usb/usbnet.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index 3c86b10..77a9a86 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c

@@ -778,7 +778,7 @@ int usbnet_stop (struct net_device *net)
 {
 	struct usbnet		*dev = netdev_priv(net);
 	struct driver_info	*info = dev->driver_info;
-	int			retval, pm;
+	int			retval, pm, mpn;
 
 	clear_bit(EVENT_DEV_OPEN, &dev->flags);
 	netif_stop_queue (net);

@@ -813,14 +813,17 @@ int usbnet_stop (struct net_device *net)
 	 * can't flush_scheduled_work() until we drop rtnl (later),
 	 * else workers could deadlock; so make workers a NOP.
 	 */
+	mpn = !test_and_clear_bit(EVENT_NO_RUNTIME_PM, &dev->flags);
 	dev->flags = 0;
 	del_timer_sync (&dev->delay);
 	tasklet_kill (&dev->bh);
+	mpn |= !test_and_clear_bit(EVENT_NO_RUNTIME_PM, &dev->flags);
+	/* in case the bh reset a flag */
+	dev->flags = 0;
 	if (!pm)
 		usb_autopm_put_interface(dev->intf);
 
-	if (info->manage_power &&
-	    !test_and_clear_bit(EVENT_NO_RUNTIME_PM, &dev->flags))
+	if (info->manage_power && mpn)
 		info->manage_power(dev, 0);
 	else
 		usb_autopm_put_interface(dev->intf);

-- 
2.1.4



--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

21.07.2015 17:22, Oliver Neukum пишет:

On Mon, 2015-07-20 at 21:13 +0300, Eugene Shatokhin wrote:

quoted

And here, the code clears EVENT_RX_KILL bit in dev->flags, which may
execute concurrently with the above operation:
#0 clear_bit (bitops.h:113, inlined)
#1 usbnet_bh (usbnet.c:1475)
         /* restart RX again after disabling due to high error rate */
         clear_bit(EVENT_RX_KILL, &dev->flags);

If clear_bit() is atomic w.r.t. setting dev->flags to 0, this race is
not a problem, I guess. Otherwise, it may be.

clear_bit is atomic with respect to other atomic operations.
So how about this:

	Regards
		Oliver

Thanks for the quick replies!
My comments are below.

+	mpn = !test_and_clear_bit(EVENT_NO_RUNTIME_PM, &dev->flags);

Right, I missed that. Indeed, if one needs EVENT_NO_RUNTIME_PM bit, one should get it before dev->flags is set to 0.

  	dev->flags = 0;

I suppose usbnet_bh() cannot be re-scheduled at this point. And if it is running now, tasklet_kill will wait till it finishes. So, I guess, it would be enough to zero dev->flags after "tasklet_kill (&dev->bh);" rather than before it, like it is now.

Anyway, if it is needed to clear any particular flags to prevent re-scheduling of usbnet_bh(), this can be done here with clear_bit(). Not sure if there are such flags, I am by no means an expert in usbnet.

  	del_timer_sync (&dev->delay);
  	tasklet_kill (&dev->bh);

The following part is not necessary, I think. usbnet_bh() does not touch EVENT_NO_RUNTIME_PM bit explicitly and these bit operations are atomic w.r.t. each other.

+	mpn |= !test_and_clear_bit(EVENT_NO_RUNTIME_PM, &dev->flags);
+	/* in case the bh reset a flag */

But zeroing dev->flags here is necessary, I agree.

+	dev->flags = 0;

Regards,
Eugene

On Wed, 2015-07-22 at 21:33 +0300, Eugene Shatokhin wrote:

The following part is not necessary, I think. usbnet_bh() does not
touch 
EVENT_NO_RUNTIME_PM bit explicitly and these bit operations are
atomic 
w.r.t. each other.

quoted

+     mpn |= !test_and_clear_bit(EVENT_NO_RUNTIME_PM, &dev->flags);
+     /* in case the bh reset a flag */

Yes, they are atomic w.r.t. each other. And that limitation worries me.

I am considering architectures which do atomic operations with
spinlocks. And this code mixes another operation into it. Can
this happen?

CPU A				CPU B

take lock
read old value
				set value to 0
clear bit
write back changed value
release lock

	Regards
		Oliver

On Mon, 2015-07-20 at 21:13 +0300, Eugene Shatokhin wrote:

[Race #5]

Race on dev->rx_urb_size. I reproduced it a similar way as the races
#2 
and #3 (changing MTU while downloading files).

dev->rx_urb_size is written to here:
#0  usbnet_change_mtu (usbnet.c:392)
        dev->rx_urb_size = dev->hard_mtu;

Here is the conflicting read from dev->rx_urb_size:
* rx_submit (usbnet.c:467)
        size_t                  size = dev->rx_urb_size;

Yes, but what is the actual bad race? I mean, if you change
the MTU in operation, there will be a race. That is just
unavoidable.
Do we generate errors?

	Regards
		Oliver


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

23.07.2015 12:43, Oliver Neukum пишет:

On Mon, 2015-07-20 at 21:13 +0300, Eugene Shatokhin wrote:

quoted

[Race #5]

Race on dev->rx_urb_size. I reproduced it a similar way as the races
#2
and #3 (changing MTU while downloading files).

dev->rx_urb_size is written to here:
#0  usbnet_change_mtu (usbnet.c:392)
         dev->rx_urb_size = dev->hard_mtu;

Here is the conflicting read from dev->rx_urb_size:
* rx_submit (usbnet.c:467)
         size_t                  size = dev->rx_urb_size;

Yes, but what is the actual bad race? I mean, if you change
the MTU in operation, there will be a race. That is just
unavoidable.
Do we generate errors?

No, I have observed no problems due to this race so far.

Regards,
Eugene

23.07.2015 12:15, Oliver Neukum пишет:

On Wed, 2015-07-22 at 21:33 +0300, Eugene Shatokhin wrote:

quoted

The following part is not necessary, I think. usbnet_bh() does not
touch
EVENT_NO_RUNTIME_PM bit explicitly and these bit operations are
atomic
w.r.t. each other.

quoted

+     mpn |= !test_and_clear_bit(EVENT_NO_RUNTIME_PM, &dev->flags);
+     /* in case the bh reset a flag */

Yes, they are atomic w.r.t. each other. And that limitation worries me.

I am considering architectures which do atomic operations with
spinlocks. And this code mixes another operation into it. Can
this happen?

CPU A				CPU B

take lock
read old value
				set value to 0
clear bit
write back changed value
release lock

From what I see now in Documentation/atomic_ops.txt, stores to the properly aligned memory locations are in fact atomic.

So, I think, the situation you described above cannot happen for dev->flags, which is good. No need to address that in the patch. The race might be harmless after all.

If I understand the code correctly now, dev->flags is set to 0 in usbnet_stop() so that the worker function (usbnet_deferred_kevent) would do nothing, should it start later. If so, how about adding memory barriers for all CPUs to see dev->flags is 0 before other things?

The patch could look like this then:

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

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index 3c86b10..d87b9c7 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c

@@ -778,7 +778,7 @@ int usbnet_stop (struct net_device *net)
 {
 	struct usbnet		*dev = netdev_priv(net);
 	struct driver_info	*info = dev->driver_info;
-	int			retval, pm;
+	int			retval, pm, mpn;

 	clear_bit(EVENT_DEV_OPEN, &dev->flags);
 	netif_stop_queue (net);

@@ -813,14 +813,17 @@ int usbnet_stop (struct net_device *net)
 	 * can't flush_scheduled_work() until we drop rtnl (later),
 	 * else workers could deadlock; so make workers a NOP.
 	 */
+	mpn = !test_and_clear_bit(EVENT_NO_RUNTIME_PM, &dev->flags);
 	dev->flags = 0;
+	smp_mb(); /* make sure the workers see that 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))
+	if (info->manage_power && mpn)
 		info->manage_power(dev, 0);
 	else
 		usb_autopm_put_interface(dev->intf);

@@ -1078,6 +1081,9 @@ usbnet_deferred_kevent (struct work_struct *work)
 		container_of(work, struct usbnet, kevent);
 	int			status;

+	/* See the changes in dev->flags from other CPUs. */
+	smp_mb();
+
 	/* usb_clear_halt() needs a thread context */
 	if (test_bit (EVENT_TX_HALT, &dev->flags)) {
 		unlink_urbs (dev, &dev->txq);
--------------------

What do you think?

Regards,
Eugene

21.07.2015 15:04, Oliver Neukum пишет:

On Mon, 2015-07-20 at 21:13 +0300, Eugene Shatokhin wrote:

quoted

Hi,

I have recently found several data races in "usbnet" module, checked on
vanilla kernel 4.1.0 on x86_64. The races do actually happen, I have
confirmed it by adding delays and using hardware breakpoints to detect
the conflicting memory accesses (with RaceHound tool,
https://github.com/winnukem/racehound).

I have not analyzed yet how harmful these races are (if they are), but
it is better to report them anyway, I think.

Everything was checked using YOTA 4G LTE Modem that works via "usbnet"
and "cdc_ether" kernel modules.
--------------------------

[Race #1]

Race on skb_queue ('next' pointer) between usbnet_stop() and rx_complete().

Reproduced that by unplugging the device while the system was
downloading a large file from the Net.

Here is part of the call stack with the code where the changes to the
queue happen:

#0 __skb_unlink (skbuff.h:1517)	
	prev->next = next;
#1 defer_bh (usbnet.c:430)
	spin_lock_irqsave(&list->lock, flags);
	old_state = entry->state;
	entry->state = state;
	__skb_unlink(skb, list);
	spin_unlock(&list->lock);
	spin_lock(&dev->done.lock);
	__skb_queue_tail(&dev->done, skb);
	if (dev->done.qlen == 1)
		tasklet_schedule(&dev->bh);
	spin_unlock_irqrestore(&dev->done.lock, flags);
#2 rx_complete (usbnet.c:640)
	state = defer_bh(dev, skb, &dev->rxq, state);

At the same time, the following code repeatedly checks if the queue is
empty and reads the same values concurrently with the above changes:

#0  usbnet_terminate_urbs (usbnet.c:765)
	/* maybe wait for deletions to finish. */
	while (!skb_queue_empty(&dev->rxq)
		&& !skb_queue_empty(&dev->txq)
		&& !skb_queue_empty(&dev->done)) {
			schedule_timeout(msecs_to_jiffies(UNLINK_TIMEOUT_MS));
			set_current_state(TASK_UNINTERRUPTIBLE);
			netif_dbg(dev, ifdown, dev->net,
				  "waited for %d urb completions\n", temp);
	}
#1  usbnet_stop (usbnet.c:806)
	if (!(info->flags & FLAG_AVOID_UNLINK_URBS))
		usbnet_terminate_urbs(dev);

For example, it is possible that the skb is removed from dev->rxq by
__skb_unlink() before the check "!skb_queue_empty(&dev->rxq)" in
usbnet_terminate_urbs() is made. It is also possible in this case that
the skb is added to dev->done queue after "!skb_queue_empty(&dev->done)"
is checked. So usbnet_terminate_urbs() may stop waiting and return while
dev->done queue still has an item.

Hi,

your analysis is correct and it looks like in addition to your proposed
fix locking needs to be simplified and a common lock to be taken.
Suggestions?

Just an idea, I haven't tested it.

How about moving the operations with dev->done under &list->lock in defer_bh, while keeping dev->done.lock too and changing usbnet_terminate_urbs() as described below?

Like this:

@@ -428,12 +428,12 @@ static enum skb_state defer_bh(struct usbnet *dev, struct sk_buff *skb,
 	old_state = entry->state;
 	entry->state = state;
 	__skb_unlink(skb, list);
-	spin_unlock(&list->lock);
 	spin_lock(&dev->done.lock);
 	__skb_queue_tail(&dev->done, skb);
 	if (dev->done.qlen == 1)
 		tasklet_schedule(&dev->bh);
-	spin_unlock_irqrestore(&dev->done.lock, flags);
+	spin_unlock(&dev->done.lock);
+	spin_unlock_irqrestore(&list->lock, flags);
 	return old_state;
 }
-------------------

usbnet_terminate_urbs() can then be changed as follows:

@@ -749,6 +749,20 @@ EXPORT_SYMBOL_GPL(usbnet_unlink_rx_urbs);

/*-------------------------------------------------------------------------*/

+static void wait_skb_queue_empty(struct sk_buff_head *q)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&q->lock, flags);
+	while (!skb_queue_empty(q)) {
+		spin_unlock_irqrestore(&q->lock, flags);
+		schedule_timeout(msecs_to_jiffies(UNLINK_TIMEOUT_MS));
+		set_current_state(TASK_UNINTERRUPTIBLE);
+		spin_lock_irqsave(&q->lock, flags);
+	}
+	spin_unlock_irqrestore(&q->lock, flags);
+}
+
 // precondition: never called in_interrupt
 static void usbnet_terminate_urbs(struct usbnet *dev)
 {

@@ -762,14 +776,11 @@ static void usbnet_terminate_urbs(struct usbnet *dev)
 		unlink_urbs(dev, &dev->rxq);

 	/* maybe wait for deletions to finish. */
-	while (!skb_queue_empty(&dev->rxq)
-		&& !skb_queue_empty(&dev->txq)
-		&& !skb_queue_empty(&dev->done)) {
-			schedule_timeout(msecs_to_jiffies(UNLINK_TIMEOUT_MS));
-			set_current_state(TASK_UNINTERRUPTIBLE);
-			netif_dbg(dev, ifdown, dev->net,
-				  "waited for %d urb completions\n", temp);
-	}
+	wait_skb_queue_empty(&dev->rxq);
+	wait_skb_queue_empty(&dev->txq);
+	wait_skb_queue_empty(&dev->done);
+	netif_dbg(dev, ifdown, dev->net,
+		  "waited for %d urb completions\n", temp);
 	set_current_state(TASK_RUNNING);
 	remove_wait_queue(&dev->wait, &wait);
 }
-------------------

This way, when usbnet_terminate_urbs() finds dev->rxq or dev->txq empty, the skbs from these queues, if there were any, have already been queued to dev->done.

At the first glance, moving the code under list->lock in defer_bh() should not produce deadlocks. Still, I suppose, it is better to use lockdep to be sure.

Regards,
Eugene

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Fri, 2015-07-24 at 17:41 +0300, Eugene Shatokhin wrote:

23.07.2015 12:15, Oliver Neukum пишет:

 From what I see now in Documentation/atomic_ops.txt, stores to the 
properly aligned memory locations are in fact atomic.

They are, but again only with respect to each other.

So, I think, the situation you described above cannot happen for 
dev->flags, which is good. No need to address that in the patch. The 
race might be harmless after all.

If I understand the code correctly now, dev->flags is set to 0 in 
usbnet_stop() so that the worker function (usbnet_deferred_kevent) would

Yes, particularly not reschedule itself.

do nothing, should it start later. If so, how about adding memory 
barriers for all CPUs to see dev->flags is 0 before other things?

Taking a lock, as del_timer_sync() does, implies a memory barrier,
as does a work.

	Regards
		Oliver

On Fri, 2015-07-24 at 20:38 +0300, Eugene Shatokhin wrote:

21.07.2015 15:04, Oliver Neukum пишет:

quoted

your analysis is correct and it looks like in addition to your proposed
fix locking needs to be simplified and a common lock to be taken.
Suggestions?

Just an idea, I haven't tested it.

How about moving the operations with dev->done under &list->lock in 
defer_bh, while keeping dev->done.lock too and changing 

Why keep dev->done.lock?
Does it make sense at all?

I suppose you want to invert those lines

Your changes make sense, but it locks to me as if a lock would
become totally redundant.

	Regards
		Oliver

27.07.2015 15:29, Oliver Neukum пишет:

On Fri, 2015-07-24 at 20:38 +0300, Eugene Shatokhin wrote:

quoted

21.07.2015 15:04, Oliver Neukum пишет:

quoted

quoted

your analysis is correct and it looks like in addition to your proposed
fix locking needs to be simplified and a common lock to be taken.
Suggestions?

Just an idea, I haven't tested it.

How about moving the operations with dev->done under &list->lock in
defer_bh, while keeping dev->done.lock too and changing

Why keep dev->done.lock?
Does it make sense at all?

I think it does.

Both skb_queue_tail(&dev->done, skb) called from rx_process() and skb_dequeue (&dev->done) called from usbnet_bh() take dev->done.lock internally. So, to synchronize accesses to dev->done, one needs that lock in defer_bh() too.

quoted

usbnet_terminate_urbs() as described below?

Like this:

@@ -428,12 +428,12 @@ static enum skb_state defer_bh(struct usbnet *dev,

struct sk_buff *skb,
   	old_state = entry->state;
   	entry->state = state;
   	__skb_unlink(skb, list);
-	spin_unlock(&list->lock);
   	spin_lock(&dev->done.lock);
   	__skb_queue_tail(&dev->done, skb);
   	if (dev->done.qlen == 1)
   		tasklet_schedule(&dev->bh);
-	spin_unlock_irqrestore(&dev->done.lock, flags);
+	spin_unlock(&dev->done.lock);
+	spin_unlock_irqrestore(&list->lock, flags);
   	return old_state;
   }
-------------------

usbnet_terminate_urbs() can then be changed as follows:

@@ -749,6 +749,20 @@ EXPORT_SYMBOL_GPL(usbnet_unlink_rx_urbs);

/*-------------------------------------------------------------------------*/

+static void wait_skb_queue_empty(struct sk_buff_head *q)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&q->lock, flags);
+	while (!skb_queue_empty(q)) {
+		spin_unlock_irqrestore(&q->lock, flags);
+		schedule_timeout(msecs_to_jiffies(UNLINK_TIMEOUT_MS));
+		set_current_state(TASK_UNINTERRUPTIBLE);

I suppose you want to invert those lines

Do you mean
+set_current_state(TASK_UNINTERRUPTIBLE);
+schedule_timeout(msecs_to_jiffies(UNLINK_TIMEOUT_MS));
?

quoted

+		spin_lock_irqsave(&q->lock, flags);
+	}
+	spin_unlock_irqrestore(&q->lock, flags);
+}
+

Your changes make sense, but it locks to me as if a lock would
become totally redundant.

Regards,

Eugene

27.07.2015 13:00, Oliver Neukum пишет:

On Fri, 2015-07-24 at 17:41 +0300, Eugene Shatokhin wrote:

quoted

23.07.2015 12:15, Oliver Neukum пишет:

quoted

  From what I see now in Documentation/atomic_ops.txt, stores to the
properly aligned memory locations are in fact atomic.

They are, but again only with respect to each other.

You are right. The architectures like "sparc" and may be others, indeed, use spinlocks to implement atomic operations, including bit manupulation.

Well then, I can only think about clearing each flag individually (with clear_bit()) instead of using "dev->flags = 0".

Something like this:

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

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index 3c86b10..826eefe 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c

@@ -779,6 +790,7 @@ int usbnet_stop (struct net_device *net)
        struct usbnet           *dev = netdev_priv(net);
        struct driver_info      *info = dev->driver_info;
        int                     retval, pm;
+       int                     e;

        clear_bit(EVENT_DEV_OPEN, &dev->flags);
        netif_stop_queue (net);

@@ -813,7 +825,8 @@ int usbnet_stop (struct net_device *net)
         * can't flush_scheduled_work() until we drop rtnl (later),
         * else workers could deadlock; so make workers a NOP.
         */
-       dev->flags = 0;
+       for (e = 0; e < EVENT_NUM_EVENTS; ++e)
+               clear_bit(e, &dev->flags)
        del_timer_sync (&dev->delay);
        tasklet_kill (&dev->bh);
        if (!pm)

diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h
index 6e0ce8c..7ad62da 100644
--- a/include/linux/usb/usbnet.h
+++ b/include/linux/usb/usbnet.h

@@ -79,6 +79,7 @@ struct usbnet {

# define EVENT_RX_KILL 10
# define EVENT_LINK_CHANGE 11
# define EVENT_SET_RX_MODE 12
+# define EVENT_NUM_EVENTS 13 /* Or may be keep all these in an enum? */
};

static inline struct usb_driver *driver_of(struct usb_interface *intf)
-------------------

clear_bit() is atomic w.r.t. itself and other bit ops.

quoted

So, I think, the situation you described above cannot happen for
dev->flags, which is good. No need to address that in the patch. The
race might be harmless after all.

If I understand the code correctly now, dev->flags is set to 0 in
usbnet_stop() so that the worker function (usbnet_deferred_kevent) would

Yes, particularly not reschedule itself.

quoted

do nothing, should it start later. If so, how about adding memory
barriers for all CPUs to see dev->flags is 0 before other things?

Taking a lock, as del_timer_sync() does, implies a memory barrier,
as does a work.

If so, then, yes, additional barriers are not needed.

Regards,
Eugene

Hi,

21.07.2015 17:22, Oliver Neukum пишет:

From what we have discussed here, I have combined a patch that fixes the race #1 in usbnet_stop() and makes #4 harmless by using atomics. I will send it shortly.

I had to make some adjustments (e.g. using spin_lock_nested in one place for lockdep to see it is OK to take dev->done.lock there).

I have tested the patch on the mainline kernel 4.2-rc6 built for x86-64, with the same USB modem. So far, lockdep, Kmemleak (just in case) and my tools have not detected problems in the relevant parts of the code. The device and the driver seem to work well.

So, what is your opinion?

Regards,
Eugene

Both races may happen when a device (e.g. YOTA 4G LTE Modem) is
unplugged while the system is downloading a large file from the Net.

Hardware breakpoints and Kprobes with delays were used to confirm that
the races do actually happen.

1. The first race is on skb_queue ('next' pointer) between usbnet_stop()
and rx_complete(), which, in turn, calls usbnet_bh().

Here is a part of the call stack with the code where the changes to the
queue happen. The line numbers are for the kernel 4.1.0:

*0 __skb_unlink (skbuff.h:1517)
    prev->next = next;
*1 defer_bh (usbnet.c:430)
    spin_lock_irqsave(&list->lock, flags);
    old_state = entry->state;
    entry->state = state;
    __skb_unlink(skb, list);
    spin_unlock(&list->lock);
    spin_lock(&dev->done.lock);
    __skb_queue_tail(&dev->done, skb);
    if (dev->done.qlen == 1)
        tasklet_schedule(&dev->bh);
    spin_unlock_irqrestore(&dev->done.lock, flags);
*2 rx_complete (usbnet.c:640)
    state = defer_bh(dev, skb, &dev->rxq, state);

At the same time, the following code repeatedly checks if the queue is
empty and reads these values concurrently with the above changes:

*0  usbnet_terminate_urbs (usbnet.c:765)
    /* maybe wait for deletions to finish. */
    while (!skb_queue_empty(&dev->rxq)
        && !skb_queue_empty(&dev->txq)
        && !skb_queue_empty(&dev->done)) {
            schedule_timeout(msecs_to_jiffies(UNLINK_TIMEOUT_MS));
            set_current_state(TASK_UNINTERRUPTIBLE);
            netif_dbg(dev, ifdown, dev->net,
                  "waited for %d urb completions\n", temp);
    }
*1  usbnet_stop (usbnet.c:806)
    if (!(info->flags & FLAG_AVOID_UNLINK_URBS))
        usbnet_terminate_urbs(dev);

As a result, it is possible, for example, that the skb is removed from
dev->rxq by __skb_unlink() before the check
"!skb_queue_empty(&dev->rxq)" in usbnet_terminate_urbs() is made. It is
also possible in this case that the skb is added to dev->done queue
after "!skb_queue_empty(&dev->done)" is checked. So
usbnet_terminate_urbs() may stop waiting and return while dev->done
queue still has an item.

Locking in defer_bh() and usbnet_terminate_urbs() was revisited to avoid
this race.

2. The second race is on dev->flags.

dev->flags is set to 0 here:
*0  usbnet_stop (usbnet.c:816)
    /* deferred work (task, timer, softirq) must also stop.
     * can't flush_scheduled_work() until we drop rtnl (later),
     * else workers could deadlock; so make workers a NOP.
     */
    dev->flags = 0;
    del_timer_sync (&dev->delay);
    tasklet_kill (&dev->bh);

And here, the code clears EVENT_RX_KILL bit in dev->flags, which may
execute concurrently with the above operation:
*0 clear_bit (bitops.h:113, inlined)
*1 usbnet_bh (usbnet.c:1475)
    /* restart RX again after disabling due to high error rate */
    clear_bit(EVENT_RX_KILL, &dev->flags);

It seems, setting dev->flags to 0 is not necessarily atomic w.r.t.
clear_bit() and other bit operations with dev->flags. It is safer to
make it atomic and this way, make the race harmless.

While at it, the checking of EVENT_NO_RUNTIME_PM bit of dev->flags in
usbnet_stop() was fixed too: the bit should be checked before dev->flags
is cleared.

Signed-off-by: Eugene Shatokhin <redacted>
---
 drivers/net/usb/usbnet.c   | 49 ++++++++++++++++++++++++++++++++--------------
 include/linux/usb/usbnet.h | 33 +++++++++++++++++++------------
 2 files changed, 54 insertions(+), 28 deletions(-)

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index 3c86b10..a53124c 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c

@@ -428,12 +428,18 @@ static enum skb_state defer_bh(struct usbnet *dev, struct sk_buff *skb,
 	old_state = entry->state;
 	entry->state = state;
 	__skb_unlink(skb, list);
-	spin_unlock(&list->lock);
-	spin_lock(&dev->done.lock);
+
+	/* defer_bh() is never called with list == &dev->done.
+	 * spin_lock_nested() tells lockdep that it is OK to take
+	 * dev->done.lock here with list->lock held. *
+	 */
+	spin_lock_nested(&dev->done.lock, SINGLE_DEPTH_NESTING);
+
 	__skb_queue_tail(&dev->done, skb);
 	if (dev->done.qlen == 1)
 		tasklet_schedule(&dev->bh);
-	spin_unlock_irqrestore(&dev->done.lock, flags);
+	spin_unlock(&dev->done.lock);
+	spin_unlock_irqrestore(&list->lock, flags);
 	return old_state;
 }
 

@@ -749,6 +755,20 @@ EXPORT_SYMBOL_GPL(usbnet_unlink_rx_urbs);
 
 /*-------------------------------------------------------------------------*/
 
+static void wait_skb_queue_empty(struct sk_buff_head *q)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&q->lock, flags);
+	while (!skb_queue_empty(q)) {
+		spin_unlock_irqrestore(&q->lock, flags);
+		schedule_timeout(msecs_to_jiffies(UNLINK_TIMEOUT_MS));
+		set_current_state(TASK_UNINTERRUPTIBLE);
+		spin_lock_irqsave(&q->lock, flags);
+	}
+	spin_unlock_irqrestore(&q->lock, flags);
+}
+
 // precondition: never called in_interrupt
 static void usbnet_terminate_urbs(struct usbnet *dev)
 {

@@ -762,14 +782,11 @@ static void usbnet_terminate_urbs(struct usbnet *dev)
 		unlink_urbs(dev, &dev->rxq);
 
 	/* maybe wait for deletions to finish. */
-	while (!skb_queue_empty(&dev->rxq)
-		&& !skb_queue_empty(&dev->txq)
-		&& !skb_queue_empty(&dev->done)) {
-			schedule_timeout(msecs_to_jiffies(UNLINK_TIMEOUT_MS));
-			set_current_state(TASK_UNINTERRUPTIBLE);
-			netif_dbg(dev, ifdown, dev->net,
-				  "waited for %d urb completions\n", temp);
-	}
+	wait_skb_queue_empty(&dev->rxq);
+	wait_skb_queue_empty(&dev->txq);
+	wait_skb_queue_empty(&dev->done);
+	netif_dbg(dev, ifdown, dev->net,
+		  "waited for %d urb completions\n", temp);
 	set_current_state(TASK_RUNNING);
 	remove_wait_queue(&dev->wait, &wait);
 }

@@ -778,7 +795,8 @@ int usbnet_stop (struct net_device *net)
 {
 	struct usbnet		*dev = netdev_priv(net);
 	struct driver_info	*info = dev->driver_info;
-	int			retval, pm;
+	int			retval, pm, mpn;
+	int			e;
 
 	clear_bit(EVENT_DEV_OPEN, &dev->flags);
 	netif_stop_queue (net);

@@ -813,14 +831,15 @@ int usbnet_stop (struct net_device *net)
 	 * can't flush_scheduled_work() until we drop rtnl (later),
 	 * else workers could deadlock; so make workers a NOP.
 	 */
-	dev->flags = 0;
+	mpn = !test_and_clear_bit(EVENT_NO_RUNTIME_PM, &dev->flags);
+	for (e = 0; e < EVENT_NUM_EVENTS; ++e)
+		clear_bit(e, &dev->flags);
 	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))
+	if (info->manage_power && mpn)
 		info->manage_power(dev, 0);
 	else
 		usb_autopm_put_interface(dev->intf);

diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h
index 6e0ce8c..39a57f5 100644
--- a/include/linux/usb/usbnet.h
+++ b/include/linux/usb/usbnet.h

@@ -22,6 +22,23 @@
 #ifndef	__LINUX_USB_USBNET_H
 #define	__LINUX_USB_USBNET_H
 
+enum usbnet_event_bits {
+	EVENT_TX_HALT =		0,
+	EVENT_RX_HALT =		1,
+	EVENT_RX_MEMORY =	2,
+	EVENT_STS_SPLIT =	3,
+	EVENT_LINK_RESET =	4,
+	EVENT_RX_PAUSED =	5,
+	EVENT_DEV_ASLEEP =	6,
+	EVENT_DEV_OPEN =	7,
+	EVENT_DEVICE_REPORT_IDLE = 8,
+	EVENT_NO_RUNTIME_PM =	9,
+	EVENT_RX_KILL =		10,
+	EVENT_LINK_CHANGE =	11,
+	EVENT_SET_RX_MODE =	12,
+	EVENT_NUM_EVENTS	/* keep it last */
+};
+
 /* interface from usbnet core to each USB networking link we handle */
 struct usbnet {
 	/* housekeeping */

@@ -65,20 +82,10 @@ struct usbnet {
 	struct tasklet_struct	bh;
 
 	struct work_struct	kevent;
+
+	/* See enum usbnet_event_bits for the meaning of the individial
+	 * bits in 'flags'. */
 	unsigned long		flags;
-#		define EVENT_TX_HALT	0
-#		define EVENT_RX_HALT	1
-#		define EVENT_RX_MEMORY	2
-#		define EVENT_STS_SPLIT	3
-#		define EVENT_LINK_RESET	4
-#		define EVENT_RX_PAUSED	5
-#		define EVENT_DEV_ASLEEP 6
-#		define EVENT_DEV_OPEN	7
-#		define EVENT_DEVICE_REPORT_IDLE	8
-#		define EVENT_NO_RUNTIME_PM	9
-#		define EVENT_RX_KILL	10
-#		define EVENT_LINK_CHANGE	11
-#		define EVENT_SET_RX_MODE	12
 };
 
 static inline struct usb_driver *driver_of(struct usb_interface *intf)

-- 
2.3.2

From: Eugene Shatokhin <redacted>
Date: Fri, 14 Aug 2015 19:58:36 +0300

2. The second race is on dev->flags.

dev->flags is set to 0 here:
*0  usbnet_stop (usbnet.c:816)
    /* deferred work (task, timer, softirq) must also stop.
     * can't flush_scheduled_work() until we drop rtnl (later),
     * else workers could deadlock; so make workers a NOP.
     */
    dev->flags = 0;
    del_timer_sync (&dev->delay);
    tasklet_kill (&dev->bh);

And here, the code clears EVENT_RX_KILL bit in dev->flags, which may
execute concurrently with the above operation:
*0 clear_bit (bitops.h:113, inlined)
*1 usbnet_bh (usbnet.c:1475)
    /* restart RX again after disabling due to high error rate */
    clear_bit(EVENT_RX_KILL, &dev->flags);

It seems, setting dev->flags to 0 is not necessarily atomic w.r.t.
clear_bit() and other bit operations with dev->flags. It is safer to
make it atomic and this way, make the race harmless.

While at it, the checking of EVENT_NO_RUNTIME_PM bit of dev->flags in
usbnet_stop() was fixed too: the bit should be checked before dev->flags
is cleared.

The fix for this is excessive.

Instead of all of this madness, looping over expensive clear_bit()
atomics, just do whatever it takes to make sure that usbnet_bh() is
quiesced and cannot execute any more.  Then you can safely clear
dev->flags normally.

19.08.2015 04:54, David Miller пишет:

From: Eugene Shatokhin <redacted>
Date: Fri, 14 Aug 2015 19:58:36 +0300

quoted

2. The second race is on dev->flags.

dev->flags is set to 0 here:
*0  usbnet_stop (usbnet.c:816)
     /* deferred work (task, timer, softirq) must also stop.
      * can't flush_scheduled_work() until we drop rtnl (later),
      * else workers could deadlock; so make workers a NOP.
      */
     dev->flags = 0;
     del_timer_sync (&dev->delay);
     tasklet_kill (&dev->bh);

And here, the code clears EVENT_RX_KILL bit in dev->flags, which may
execute concurrently with the above operation:
*0 clear_bit (bitops.h:113, inlined)
*1 usbnet_bh (usbnet.c:1475)
     /* restart RX again after disabling due to high error rate */
     clear_bit(EVENT_RX_KILL, &dev->flags);

It seems, setting dev->flags to 0 is not necessarily atomic w.r.t.
clear_bit() and other bit operations with dev->flags. It is safer to
make it atomic and this way, make the race harmless.

While at it, the checking of EVENT_NO_RUNTIME_PM bit of dev->flags in
usbnet_stop() was fixed too: the bit should be checked before dev->flags
is cleared.

The fix for this is excessive.

Instead of all of this madness, looping over expensive clear_bit()
atomics, just do whatever it takes to make sure that usbnet_bh() is
quiesced and cannot execute any more.  Then you can safely clear
dev->flags normally.

If I understand it correctly, it is to make sure usbnet_bh() is not scheduled again that dev->flags should be set to 0 first, one way or another. That is what this madness is for.

tasklet_kill() will wait then for the already running instance of usbnet_bh() (if one is running). After that, it is guaranteed BH is not running and will not be re-scheduled.

As for the performance concerns, I doubt that usbnet_stop() is anywhere on the critical path. I have been testing this patch for some time and haven't seen any new performance issues with it yet.

If needed, it is possible to measure and compare the time needed for usbnet_stop() before and after this patch and try to estimate the impact of this on the overall performance.

Regards,
Eugene

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

The following problems found when investigating races in usbnet module 
are fixed here:

1. EVENT_NO_RUNTIME_PM bit of dev->flags should be read before it is 
cleared by "dev->flags = 0". Thanks to Oliver Neukum for spotting this
problem and providing a fix.

2. A race on on skb_queue between usbnet_stop() and usbnet_bh().

Compared to the combined patch I sent earlier 
("[PATCH] usbnet: Fix two races between usbnet_stop() and the BH"), this 
patch set has the following changes:

* The fix for handling of EVENT_NO_RUNTIME_PM is now in a separate patch.
* The fix for the race on dev->flags has been removed because the race is
not considered harmful.

Regards,
Eugene

It is needed to check EVENT_NO_RUNTIME_PM bit of dev->flags in
usbnet_stop(), but its value should be read before it is cleared
when dev->flags is set to 0.

The problem was spotted and the fix was provided by
Oliver Neukum [off-list ref].

Signed-off-by: Eugene Shatokhin <redacted>
---
 drivers/net/usb/usbnet.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index 3c86b10..e049857 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c

@@ -778,7 +778,7 @@ int usbnet_stop (struct net_device *net)
 {
 	struct usbnet		*dev = netdev_priv(net);
 	struct driver_info	*info = dev->driver_info;
-	int			retval, pm;
+	int			retval, pm, mpn;
 
 	clear_bit(EVENT_DEV_OPEN, &dev->flags);
 	netif_stop_queue (net);

@@ -809,6 +809,8 @@ int usbnet_stop (struct net_device *net)
 
 	usbnet_purge_paused_rxq(dev);
 
+	mpn = !test_and_clear_bit(EVENT_NO_RUNTIME_PM, &dev->flags);
+
 	/* deferred work (task, timer, softirq) must also stop.
 	 * can't flush_scheduled_work() until we drop rtnl (later),
 	 * else workers could deadlock; so make workers a NOP.

@@ -819,8 +821,7 @@ int usbnet_stop (struct net_device *net)
 	if (!pm)
 		usb_autopm_put_interface(dev->intf);
 
-	if (info->manage_power &&
-	    !test_and_clear_bit(EVENT_NO_RUNTIME_PM, &dev->flags))
+	if (info->manage_power && mpn)
 		info->manage_power(dev, 0);
 	else
 		usb_autopm_put_interface(dev->intf);

-- 
2.3.2

The race may happen when a device (e.g. YOTA 4G LTE Modem) is
unplugged while the system is downloading a large file from the Net.

Hardware breakpoints and Kprobes with delays were used to confirm that
the race does actually happen.

The race is on skb_queue ('next' pointer) between usbnet_stop()
and rx_complete(), which, in turn, calls usbnet_bh().

Here is a part of the call stack with the code where the changes to the
queue happen. The line numbers are for the kernel 4.1.0:

*0 __skb_unlink (skbuff.h:1517)
    prev->next = next;
*1 defer_bh (usbnet.c:430)
    spin_lock_irqsave(&list->lock, flags);
    old_state = entry->state;
    entry->state = state;
    __skb_unlink(skb, list);
    spin_unlock(&list->lock);
    spin_lock(&dev->done.lock);
    __skb_queue_tail(&dev->done, skb);
    if (dev->done.qlen == 1)
        tasklet_schedule(&dev->bh);
    spin_unlock_irqrestore(&dev->done.lock, flags);
*2 rx_complete (usbnet.c:640)
    state = defer_bh(dev, skb, &dev->rxq, state);

At the same time, the following code repeatedly checks if the queue is
empty and reads these values concurrently with the above changes:

*0  usbnet_terminate_urbs (usbnet.c:765)
    /* maybe wait for deletions to finish. */
    while (!skb_queue_empty(&dev->rxq)
        && !skb_queue_empty(&dev->txq)
        && !skb_queue_empty(&dev->done)) {
            schedule_timeout(msecs_to_jiffies(UNLINK_TIMEOUT_MS));
            set_current_state(TASK_UNINTERRUPTIBLE);
            netif_dbg(dev, ifdown, dev->net,
                  "waited for %d urb completions\n", temp);
    }
*1  usbnet_stop (usbnet.c:806)
    if (!(info->flags & FLAG_AVOID_UNLINK_URBS))
        usbnet_terminate_urbs(dev);

As a result, it is possible, for example, that the skb is removed from
dev->rxq by __skb_unlink() before the check
"!skb_queue_empty(&dev->rxq)" in usbnet_terminate_urbs() is made. It is
also possible in this case that the skb is added to dev->done queue
after "!skb_queue_empty(&dev->done)" is checked. So
usbnet_terminate_urbs() may stop waiting and return while dev->done
queue still has an item.

Locking in defer_bh() and usbnet_terminate_urbs() was revisited to avoid
this race.

Signed-off-by: Eugene Shatokhin <redacted>
---
 drivers/net/usb/usbnet.c | 39 ++++++++++++++++++++++++++++-----------
 1 file changed, 28 insertions(+), 11 deletions(-)

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index e049857..b4cf107 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c

@@ -428,12 +428,18 @@ static enum skb_state defer_bh(struct usbnet *dev, struct sk_buff *skb,
 	old_state = entry->state;
 	entry->state = state;
 	__skb_unlink(skb, list);
-	spin_unlock(&list->lock);
-	spin_lock(&dev->done.lock);
+
+	/* defer_bh() is never called with list == &dev->done.
+	 * spin_lock_nested() tells lockdep that it is OK to take
+	 * dev->done.lock here with list->lock held.
+	 */
+	spin_lock_nested(&dev->done.lock, SINGLE_DEPTH_NESTING);
+
 	__skb_queue_tail(&dev->done, skb);
 	if (dev->done.qlen == 1)
 		tasklet_schedule(&dev->bh);
-	spin_unlock_irqrestore(&dev->done.lock, flags);
+	spin_unlock(&dev->done.lock);
+	spin_unlock_irqrestore(&list->lock, flags);
 	return old_state;
 }
 

@@ -749,6 +755,20 @@ EXPORT_SYMBOL_GPL(usbnet_unlink_rx_urbs);
 
 /*-------------------------------------------------------------------------*/
 
+static void wait_skb_queue_empty(struct sk_buff_head *q)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&q->lock, flags);
+	while (!skb_queue_empty(q)) {
+		spin_unlock_irqrestore(&q->lock, flags);
+		schedule_timeout(msecs_to_jiffies(UNLINK_TIMEOUT_MS));
+		set_current_state(TASK_UNINTERRUPTIBLE);
+		spin_lock_irqsave(&q->lock, flags);
+	}
+	spin_unlock_irqrestore(&q->lock, flags);
+}
+
 // precondition: never called in_interrupt
 static void usbnet_terminate_urbs(struct usbnet *dev)
 {

@@ -762,14 +782,11 @@ static void usbnet_terminate_urbs(struct usbnet *dev)
 		unlink_urbs(dev, &dev->rxq);
 
 	/* maybe wait for deletions to finish. */
-	while (!skb_queue_empty(&dev->rxq)
-		&& !skb_queue_empty(&dev->txq)
-		&& !skb_queue_empty(&dev->done)) {
-			schedule_timeout(msecs_to_jiffies(UNLINK_TIMEOUT_MS));
-			set_current_state(TASK_UNINTERRUPTIBLE);
-			netif_dbg(dev, ifdown, dev->net,
-				  "waited for %d urb completions\n", temp);
-	}
+	wait_skb_queue_empty(&dev->rxq);
+	wait_skb_queue_empty(&dev->txq);
+	wait_skb_queue_empty(&dev->done);
+	netif_dbg(dev, ifdown, dev->net,
+		  "waited for %d urb completions\n", temp);
 	set_current_state(TASK_RUNNING);
 	remove_wait_queue(&dev->wait, &wait);
 }

-- 
2.3.2

On Mon, 2015-08-24 at 23:13 +0300, Eugene Shatokhin wrote:

It is needed to check EVENT_NO_RUNTIME_PM bit of dev->flags in
usbnet_stop(), but its value should be read before it is cleared
when dev->flags is set to 0.

Can we agree that this at least is good and should go upstream
and into stable?

	Regards
		Oliver

On Mon, 2015-08-24 at 23:13 +0300, Eugene Shatokhin wrote:

It is needed to check EVENT_NO_RUNTIME_PM bit of dev->flags in
usbnet_stop(), but its value should be read before it is cleared
when dev->flags is set to 0.

The problem was spotted and the fix was provided by
Oliver Neukum [off-list ref].

Signed-off-by: Eugene Shatokhin <redacted>

Acked-by: Oliver Neukum <oneukum@suse.com>

From: Eugene Shatokhin <redacted>
Date: Mon, 24 Aug 2015 23:13:42 +0300

It is needed to check EVENT_NO_RUNTIME_PM bit of dev->flags in
usbnet_stop(), but its value should be read before it is cleared
when dev->flags is set to 0.

The problem was spotted and the fix was provided by
Oliver Neukum [off-list ref].

Signed-off-by: Eugene Shatokhin <redacted>

Applied and queued up for -stable.

From: Eugene Shatokhin <redacted>
Date: Mon, 24 Aug 2015 23:13:43 +0300

The race may happen when a device (e.g. YOTA 4G LTE Modem) is
unplugged while the system is downloading a large file from the Net.

Hardware breakpoints and Kprobes with delays were used to confirm that
the race does actually happen.

The race is on skb_queue ('next' pointer) between usbnet_stop()
and rx_complete(), which, in turn, calls usbnet_bh().

Here is a part of the call stack with the code where the changes to the
queue happen. The line numbers are for the kernel 4.1.0:

 ...

It looks like this patch needs more discussion/work.