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

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

[Eugene Shatokhin]

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

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

[Oliver Neukum]

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

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

[Oliver Neukum]

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

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

[Oliver Neukum]

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@suse.com>
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@suse.com>
---
 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

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

[Eugene Shatokhin]

21.07.2015 17:22, Oliver Neukum пишет:
> 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
>

Thanks for the quick replies!
My comments are below.

>>From 1c4e685b3a9c183e04c46b661830e5c7ed35b513 Mon Sep 17 00:00:00 2001
> From: Oliver Neukum <oneukum@suse.com>
> 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@suse.com>
> ---
>   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);

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;

>   	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);
>

Regards,
Eugene

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

[Oliver Neukum]

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

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

[Oliver Neukum]

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

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

[Eugene Shatokhin]

23.07.2015 12:43, Oliver Neukum пишет:
> 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?

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

Regards,
Eugene

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

[Eugene Shatokhin]

23.07.2015 12:15, Oliver Neukum пишет:
> 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.
>>
>>> +     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

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

[Eugene Shatokhin]

21.07.2015 15:04, Oliver Neukum пишет:
> 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?

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

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

[Oliver Neukum]

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

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

[Oliver Neukum]

On Fri, 2015-07-24 at 20:38 +0300, Eugene Shatokhin wrote:
> 21.07.2015 15:04, Oliver Neukum пишет:

> > 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?

> 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

> +		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
		Oliver

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

[Eugene Shatokhin]

27.07.2015 15:29, Oliver Neukum пишет:
> On Fri, 2015-07-24 at 20:38 +0300, Eugene Shatokhin wrote:
>> 21.07.2015 15:04, Oliver Neukum пишет:
>
>>> 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.

>
>> 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));
?

>
>> +		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

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

[Eugene Shatokhin]

27.07.2015 13:00, Oliver Neukum пишет:
> 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.

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

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

Regards,
Eugene

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

[Eugene Shatokhin]

Hi,

21.07.2015 17:22, Oliver Neukum пишет:
> 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@suse.com>
> 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@suse.com>
> ---
>   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);
>

 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

[PATCH] usbnet: Fix two races between usbnet_stop() and the BH

[Eugene Shatokhin]

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 <eugene.shatokhin@rosalab.ru>
---
 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

Re: [PATCH] usbnet: Fix two races between usbnet_stop() and the BH

[David Miller]

From: Eugene Shatokhin <eugene.shatokhin@rosalab.ru>
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.

Re: [PATCH] usbnet: Fix two races between usbnet_stop() and the BH

[Eugene Shatokhin]

19.08.2015 04:54, David Miller пишет:
> From: Eugene Shatokhin <eugene.shatokhin@rosalab.ru>
> 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.
>

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

Re: [PATCH] usbnet: Fix two races between usbnet_stop() and the BH

[Bjørn Mork]

Eugene Shatokhin <eugene.shatokhin@rosalab.ru> writes:

> 19.08.2015 04:54, David Miller пишет:
>> From: Eugene Shatokhin <eugene.shatokhin@rosalab.ru>
>> 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.
>>
>
> 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.

Assuming there is a race which may reorder these, exactly what
difference does it make wrt EVENT_RX_KILL if you do

a)  clear_bit(EVENT_RX_KILL, &dev->flags);
    dev->flags = 0;

or

b)  dev->flags = 0;
    clear_bit(EVENT_RX_KILL, &dev->flags);


AFAICS, the result will be a cleared EVENT_RX_KILL bit in either case.


The EVENT_NO_RUNTIME_PM bug should definitely be fixed.  Please split
that out as a separate fix.  It's a separate issue, and should be
backported to all maintained stable releases it applies to (anything
from v3.8 and newer)


Bjørn

Re: [PATCH] usbnet: Fix two races between usbnet_stop() and the BH

[Eugene Shatokhin]

19.08.2015 13:54, Bjørn Mork пишет:
> Eugene Shatokhin <eugene.shatokhin@rosalab.ru> writes:
>
>> 19.08.2015 04:54, David Miller пишет:
>>> From: Eugene Shatokhin <eugene.shatokhin@rosalab.ru>
>>> 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.
>>>
>>
>> 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.
>
> Assuming there is a race which may reorder these, exactly what
> difference does it make wrt EVENT_RX_KILL if you do
>
> a)  clear_bit(EVENT_RX_KILL, &dev->flags);
>      dev->flags = 0;
>
> or
>
> b)  dev->flags = 0;
>      clear_bit(EVENT_RX_KILL, &dev->flags);
>
>
> AFAICS, the result will be a cleared EVENT_RX_KILL bit in either case.
>

Thanks for the review!

The problem is not in the reordering but rather in the fact that 
"dev->flags = 0" is not necessarily atomic w.r.t. 
"clear_bit(EVENT_RX_KILL, &dev->flags)", and vice versa.

So the following might be possible, although unlikely:

CPU0             CPU1
                  clear_bit: read dev->flags
                  clear_bit: clear EVENT_RX_KILL in the read value

dev->flags=0;

                  clear_bit: write updated dev->flags

As a result, dev->flags may become non-zero again.

I cannot prove yet that this is an impossible situation. If anyone can, 
please explain. If so, this part of the patch will not be needed.

>
> The EVENT_NO_RUNTIME_PM bug should definitely be fixed.  Please split
> that out as a separate fix.  It's a separate issue, and should be
> backported to all maintained stable releases it applies to (anything
> from v3.8 and newer)

Yes, that makes sense. However, this fix was originally provided by 
Oliver Neukum rather than me, so I would like to hear his opinion as 
well first.
>
>
> Bjørn
>

Regards,
Eugene

Re: [PATCH] usbnet: Fix two races between usbnet_stop() and the BH

[Bjørn Mork]

Eugene Shatokhin <eugene.shatokhin@rosalab.ru> writes:

> The problem is not in the reordering but rather in the fact that
> "dev->flags = 0" is not necessarily atomic
> w.r.t. "clear_bit(EVENT_RX_KILL, &dev->flags)", and vice versa.
>
> So the following might be possible, although unlikely:
>
> CPU0             CPU1
>                  clear_bit: read dev->flags
>                  clear_bit: clear EVENT_RX_KILL in the read value
>
> dev->flags=0;
>
>                  clear_bit: write updated dev->flags
>
> As a result, dev->flags may become non-zero again.

Ah, right.  Thanks for explaining.

> I cannot prove yet that this is an impossible situation. If anyone
> can, please explain. If so, this part of the patch will not be needed.

I wonder if we could simply move the dev->flags = 0 down a few lines to
fix both issues?  It doesn't seem to do anything useful except for
resetting the flags to a sane initial state after the device is down.

Stopping the tasklet rescheduling etc depends only on netif_running(),
which will be false when usbnet_stop is called.  There is no need to
touch dev->flags for this to happen.

>> The EVENT_NO_RUNTIME_PM bug should definitely be fixed.  Please split
>> that out as a separate fix.  It's a separate issue, and should be
>> backported to all maintained stable releases it applies to (anything
>> from v3.8 and newer)
>
> Yes, that makes sense. However, this fix was originally provided by
> Oliver Neukum rather than me, so I would like to hear his opinion as
> well first.

If what I write above is correct (please help me verify...), then maybe
it does make sense to do these together anyway.



Bjørn

Re: [PATCH] usbnet: Fix two races between usbnet_stop() and the BH

[Eugene Shatokhin]

19.08.2015 15:31, Bjørn Mork пишет:
> Eugene Shatokhin <eugene.shatokhin@rosalab.ru> writes:
>
>> The problem is not in the reordering but rather in the fact that
>> "dev->flags = 0" is not necessarily atomic
>> w.r.t. "clear_bit(EVENT_RX_KILL, &dev->flags)", and vice versa.
>>
>> So the following might be possible, although unlikely:
>>
>> CPU0             CPU1
>>                   clear_bit: read dev->flags
>>                   clear_bit: clear EVENT_RX_KILL in the read value
>>
>> dev->flags=0;
>>
>>                   clear_bit: write updated dev->flags
>>
>> As a result, dev->flags may become non-zero again.
>
> Ah, right.  Thanks for explaining.
>
>> I cannot prove yet that this is an impossible situation. If anyone
>> can, please explain. If so, this part of the patch will not be needed.
>
> I wonder if we could simply move the dev->flags = 0 down a few lines to
> fix both issues?  It doesn't seem to do anything useful except for
> resetting the flags to a sane initial state after the device is down.
>
> Stopping the tasklet rescheduling etc depends only on netif_running(),
> which will be false when usbnet_stop is called.  There is no need to
> touch dev->flags for this to happen.

That was one of the first ideas we discussed here. Unfortunately, it is 
probably not so simple.

Setting dev->flags to 0 makes some delayed operations do nothing and, 
among other things, not to reschedule usbnet_bh().

As you can see in drivers/net/usb/usbnet.c, usbnet_bh() can be called as 
a tasklet function and as a timer function in a number of situations 
(look for the usage of dev->bh and dev->delay there).

netif_running() is indeed false when usbnet_stop() runs, usbnet_stop() 
also disables Tx. This seems to be enough for many cases where 
usbnet_bh() is scheduled, but I am not so sure about the remaining ones, 
namely:

1. A work function, usbnet_deferred_kevent(), may reschedule 
usbnet_bh(). Looks like the workqueue is only stopped in 
usbnet_disconnect(), so a work item might be processed while 
usbnet_stop() works. Setting dev->flags to 0 makes the work function do 
nothing, by the way. See also the comment in usbnet_stop() about this.

A work item may be placed to this workqueue in a number of ways, by both 
usbnet module and the mini-drivers. It is not too easy to track all 
these situations.

2. rx_complete() and tx_complete() may schedule execution of usbnet_bh() 
as a tasklet or a timer function. These two are URB completion callbacks.

It seems, new Rx and Tx URBs cannot be submitted when usbnet_stop() 
clears dev->flags, indeed. But it does not prevent the completion 
handlers for the previously submitted URBs from running concurrently 
with usbnet_stop(). The latter waits for them to complete (via 
usbnet_terminate_urbs(dev)) but only if FLAG_AVOID_UNLINK_URBS is not 
set in info->flags. rndis_wlan, however, sets this flag for a few 
hardware models. So - no guarantees here as well.

If someone could list the particular bits of dev->flags that should be 
cleared to make sure no deferred call could reschedule usbnet_bh(), 
etc... Well, it would be enough to clear these first and use dev->flags 
= 0 later, after tasklet_kill() and del_timer_sync(). I cannot point out 
these particular bits now.

Besides, it is possible, although unlikely, that new event bits will be 
added to dev->flags in the future. And one will need to keep track of 
these to see if they should be cleared as well. I'd prever to play safer 
for now and clear them all.

>
>>> The EVENT_NO_RUNTIME_PM bug should definitely be fixed.  Please split
>>> that out as a separate fix.  It's a separate issue, and should be
>>> backported to all maintained stable releases it applies to (anything
>>> from v3.8 and newer)
>>
>> Yes, that makes sense. However, this fix was originally provided by
>> Oliver Neukum rather than me, so I would like to hear his opinion as
>> well first.
>
> If what I write above is correct (please help me verify...), then maybe
> it does make sense to do these together anyway.

I think, your suggestion to make it a separate patch is right. Will do 
it in the next version of the patchset, hopefully soon.

Regards,
Eugene

Re: [PATCH] usbnet: Fix two races between usbnet_stop() and the BH

[Bjørn Mork]

Eugene Shatokhin <eugene.shatokhin@rosalab.ru> writes:

> 19.08.2015 15:31, Bjørn Mork пишет:
>> Eugene Shatokhin <eugene.shatokhin@rosalab.ru> writes:
>>
>>> The problem is not in the reordering but rather in the fact that
>>> "dev->flags = 0" is not necessarily atomic
>>> w.r.t. "clear_bit(EVENT_RX_KILL, &dev->flags)", and vice versa.
>>>
>>> So the following might be possible, although unlikely:
>>>
>>> CPU0             CPU1
>>>                   clear_bit: read dev->flags
>>>                   clear_bit: clear EVENT_RX_KILL in the read value
>>>
>>> dev->flags=0;
>>>
>>>                   clear_bit: write updated dev->flags
>>>
>>> As a result, dev->flags may become non-zero again.
>>
>> Ah, right.  Thanks for explaining.
>>
>>> I cannot prove yet that this is an impossible situation. If anyone
>>> can, please explain. If so, this part of the patch will not be needed.
>>
>> I wonder if we could simply move the dev->flags = 0 down a few lines to
>> fix both issues?  It doesn't seem to do anything useful except for
>> resetting the flags to a sane initial state after the device is down.
>>
>> Stopping the tasklet rescheduling etc depends only on netif_running(),
>> which will be false when usbnet_stop is called.  There is no need to
>> touch dev->flags for this to happen.
>
> That was one of the first ideas we discussed here. Unfortunately, it
> is probably not so simple.
>
> Setting dev->flags to 0 makes some delayed operations do nothing and,
> among other things, not to reschedule usbnet_bh().

Yes, but I believe that is merely a side effect.  You should never need
to clear multiple flags to get the desired behaviour.

> As you can see in drivers/net/usb/usbnet.c, usbnet_bh() can be called
> as a tasklet function and as a timer function in a number of
> situations (look for the usage of dev->bh and dev->delay there).
>
> netif_running() is indeed false when usbnet_stop() runs, usbnet_stop()
> also disables Tx. This seems to be enough for many cases where
> usbnet_bh() is scheduled, but I am not so sure about the remaining
> ones, namely:
>
> 1. A work function, usbnet_deferred_kevent(), may reschedule
> usbnet_bh(). Looks like the workqueue is only stopped in
> usbnet_disconnect(), so a work item might be processed while
> usbnet_stop() works. Setting dev->flags to 0 makes the work function
> do nothing, by the way. See also the comment in usbnet_stop() about
> this.
>
> A work item may be placed to this workqueue in a number of ways, by
> both usbnet module and the mini-drivers. It is not too easy to track
> all these situations.

That's an understatement :)



> 2. rx_complete() and tx_complete() may schedule execution of
> usbnet_bh() as a tasklet or a timer function. These two are URB
> completion callbacks.
>
> It seems, new Rx and Tx URBs cannot be submitted when usbnet_stop()
> clears dev->flags, indeed. But it does not prevent the completion
> handlers for the previously submitted URBs from running concurrently
> with usbnet_stop(). The latter waits for them to complete (via
> usbnet_terminate_urbs(dev)) but only if FLAG_AVOID_UNLINK_URBS is not
> set in info->flags. rndis_wlan, however, sets this flag for a few
> hardware models. So - no guarantees here as well.

FLAG_AVOID_UNLINK_URBS looks like it should be replaced by the newer
ability to keep the status urb active. I believe that must have been the
real reason for adding it, based on the commit message and the effect
the flag will have:

 commit 1487cd5e76337555737cbc55d7d83f41460d198f
 Author: Jussi Kivilinna <jussi.kivilinna@mbnet.fi>
 Date:   Thu Jul 30 19:41:20 2009 +0300

    usbnet: allow "minidriver" to prevent urb unlinking on usbnet_stop
    
    rndis_wlan devices freeze after running usbnet_stop several times. It appears
    that firmware freezes in state where it does not respond to any RNDIS commands
    and device have to be physically unplugged/replugged. This patch lets
    minidrivers to disable unlink_urbs on usbnet_stop through new info flag.
    
    Signed-off-by: Jussi Kivilinna <jussi.kivilinna@mbnet.fi>
    Cc: David Brownell <dbrownell@users.sourceforge.net>
    Signed-off-by: John W. Linville <linville@tuxdriver.com>



The rx urbs will not be resubmitted in any case, and there are of course
no tx urbs being submitted.  So the only effect of this flag is on the
status/interrupt urb, which I can imagine some RNDIS devices wants
active all the time. 

So FLAG_AVOID_UNLINK_URBS should probably be removed and replaced calls
to usbnet_status_start() and usbnet_status_stop().  This will require
testing on some of the devices with the original firmware problem
however.

In any case: I do not think this flag should be considered when trying
to make usbnet_stop behaviour saner.  It's only purpose is to
deliberately break usbnet_stop by not actually stopping.


> If someone could list the particular bits of dev->flags that should be
> cleared to make sure no deferred call could reschedule usbnet_bh(),
> etc... Well, it would be enough to clear these first and use
> dev->flags = 0 later, after tasklet_kill() and del_timer_sync(). I
> cannot point out these particular bits now.


I don't think any of the flags must be cleared.  The sequence

        dev_close(dev->net);
	usbnet_terminate_urbs(dev);
	usbnet_purge_paused_rxq(dev);
	del_timer_sync (&dev->delay);
	tasklet_kill (&dev->bh);

should prevent any rescheduling of usbnet_bh

> Besides, it is possible, although unlikely, that new event bits will
> be added to dev->flags in the future. And one will need to keep track
> of these to see if they should be cleared as well. I'd prever to play
> safer for now and clear them all.

I don't think we should ever make a flag which will _have_ to be reset
for usbnet_stop.  The only reason for clearing all flags is to reset the
state before the next open.

Yes, I see that we currently need to clear EVENT_DEV_OPEN in
usbnet_stop, but I really don't see what this flag gives us which isn't
already provided by netif_running().  It looks like a duplicate.


Bjørn

Re: [PATCH] usbnet: Fix two races between usbnet_stop() and the BH

[Eugene Shatokhin]

24.08.2015 16:29, Bjørn Mork пишет:
> Eugene Shatokhin <eugene.shatokhin@rosalab.ru> writes:
>
>> 19.08.2015 15:31, Bjørn Mork пишет:
>>> Eugene Shatokhin <eugene.shatokhin@rosalab.ru> writes:
>>>
>>>> The problem is not in the reordering but rather in the fact that
>>>> "dev->flags = 0" is not necessarily atomic
>>>> w.r.t. "clear_bit(EVENT_RX_KILL, &dev->flags)", and vice versa.
>>>>
>>>> So the following might be possible, although unlikely:
>>>>
>>>> CPU0             CPU1
>>>>                    clear_bit: read dev->flags
>>>>                    clear_bit: clear EVENT_RX_KILL in the read value
>>>>
>>>> dev->flags=0;
>>>>
>>>>                    clear_bit: write updated dev->flags
>>>>
>>>> As a result, dev->flags may become non-zero again.
>>>
>>> Ah, right.  Thanks for explaining.
>>>
>>>> I cannot prove yet that this is an impossible situation. If anyone
>>>> can, please explain. If so, this part of the patch will not be needed.
>>>
>>> I wonder if we could simply move the dev->flags = 0 down a few lines to
>>> fix both issues?  It doesn't seem to do anything useful except for
>>> resetting the flags to a sane initial state after the device is down.
>>>
>>> Stopping the tasklet rescheduling etc depends only on netif_running(),
>>> which will be false when usbnet_stop is called.  There is no need to
>>> touch dev->flags for this to happen.
>>
>> That was one of the first ideas we discussed here. Unfortunately, it
>> is probably not so simple.
>>
>> Setting dev->flags to 0 makes some delayed operations do nothing and,
>> among other things, not to reschedule usbnet_bh().
>
> Yes, but I believe that is merely a side effect.  You should never need
> to clear multiple flags to get the desired behaviour.
>
>> As you can see in drivers/net/usb/usbnet.c, usbnet_bh() can be called
>> as a tasklet function and as a timer function in a number of
>> situations (look for the usage of dev->bh and dev->delay there).
>>
>> netif_running() is indeed false when usbnet_stop() runs, usbnet_stop()
>> also disables Tx. This seems to be enough for many cases where
>> usbnet_bh() is scheduled, but I am not so sure about the remaining
>> ones, namely:
>>
>> 1. A work function, usbnet_deferred_kevent(), may reschedule
>> usbnet_bh(). Looks like the workqueue is only stopped in
>> usbnet_disconnect(), so a work item might be processed while
>> usbnet_stop() works. Setting dev->flags to 0 makes the work function
>> do nothing, by the way. See also the comment in usbnet_stop() about
>> this.
>>
>> A work item may be placed to this workqueue in a number of ways, by
>> both usbnet module and the mini-drivers. It is not too easy to track
>> all these situations.
>
> That's an understatement :)
>
>
>
>> 2. rx_complete() and tx_complete() may schedule execution of
>> usbnet_bh() as a tasklet or a timer function. These two are URB
>> completion callbacks.
>>
>> It seems, new Rx and Tx URBs cannot be submitted when usbnet_stop()
>> clears dev->flags, indeed. But it does not prevent the completion
>> handlers for the previously submitted URBs from running concurrently
>> with usbnet_stop(). The latter waits for them to complete (via
>> usbnet_terminate_urbs(dev)) but only if FLAG_AVOID_UNLINK_URBS is not
>> set in info->flags. rndis_wlan, however, sets this flag for a few
>> hardware models. So - no guarantees here as well.
>
> FLAG_AVOID_UNLINK_URBS looks like it should be replaced by the newer
> ability to keep the status urb active. I believe that must have been the
> real reason for adding it, based on the commit message and the effect
> the flag will have:
>
>   commit 1487cd5e76337555737cbc55d7d83f41460d198f
>   Author: Jussi Kivilinna <jussi.kivilinna@mbnet.fi>
>   Date:   Thu Jul 30 19:41:20 2009 +0300
>
>      usbnet: allow "minidriver" to prevent urb unlinking on usbnet_stop
>
>      rndis_wlan devices freeze after running usbnet_stop several times. It appears
>      that firmware freezes in state where it does not respond to any RNDIS commands
>      and device have to be physically unplugged/replugged. This patch lets
>      minidrivers to disable unlink_urbs on usbnet_stop through new info flag.
>
>      Signed-off-by: Jussi Kivilinna <jussi.kivilinna@mbnet.fi>
>      Cc: David Brownell <dbrownell@users.sourceforge.net>
>      Signed-off-by: John W. Linville <linville@tuxdriver.com>
>
>
>
> The rx urbs will not be resubmitted in any case, and there are of course
> no tx urbs being submitted.  So the only effect of this flag is on the
> status/interrupt urb, which I can imagine some RNDIS devices wants
> active all the time.
>
> So FLAG_AVOID_UNLINK_URBS should probably be removed and replaced calls
> to usbnet_status_start() and usbnet_status_stop().  This will require
> testing on some of the devices with the original firmware problem
> however.
>
> In any case: I do not think this flag should be considered when trying
> to make usbnet_stop behaviour saner.  It's only purpose is to
> deliberately break usbnet_stop by not actually stopping.
>
>
>> If someone could list the particular bits of dev->flags that should be
>> cleared to make sure no deferred call could reschedule usbnet_bh(),
>> etc... Well, it would be enough to clear these first and use
>> dev->flags = 0 later, after tasklet_kill() and del_timer_sync(). I
>> cannot point out these particular bits now.
>
>
> I don't think any of the flags must be cleared.  The sequence
>
>          dev_close(dev->net);
> 	usbnet_terminate_urbs(dev);
> 	usbnet_purge_paused_rxq(dev);
> 	del_timer_sync (&dev->delay);
> 	tasklet_kill (&dev->bh);
>
> should prevent any rescheduling of usbnet_bh

If so, then, I suppose, one could ignore that FLAG_AVOID_UNLINK_URBS for 
now and just move dev->flags = 0 down as you suggested and as we thought 
before.

The patch will become simpler, indeed.

>
>> Besides, it is possible, although unlikely, that new event bits will
>> be added to dev->flags in the future. And one will need to keep track
>> of these to see if they should be cleared as well. I'd prever to play
>> safer for now and clear them all.
>
> I don't think we should ever make a flag which will _have_ to be reset
> for usbnet_stop.  The only reason for clearing all flags is to reset the
> state before the next open.
>
> Yes, I see that we currently need to clear EVENT_DEV_OPEN in
> usbnet_stop, but I really don't see what this flag gives us which isn't
> already provided by netif_running().  It looks like a duplicate.
>

Regards,
Eugene

Re: [PATCH] usbnet: Fix two races between usbnet_stop() and the BH

[David Miller]

From: Eugene Shatokhin <eugene.shatokhin@rosalab.ru>
Date: Wed, 19 Aug 2015 14:59:01 +0300

> So the following might be possible, although unlikely:
> 
> CPU0             CPU1
>                  clear_bit: read dev->flags
>                  clear_bit: clear EVENT_RX_KILL in the read value
> 
> dev->flags=0;
> 
>                  clear_bit: write updated dev->flags
> 
> As a result, dev->flags may become non-zero again.

Is this really possible?

Stores really are "atomic" in the sense that the do their update
in one indivisible operation.

Atomic operations like clear_bit also will behave that way.

If a clear_bit is in progress, the "dev->flags=0" store will not be
able to grab the cache line exclusively until the clear_bit is done.

So I think the above sequent of events is completely impossible.  Once
a clear_bit starts, a write by another foreign agent on the bus is
absolutely impossible to legally occur until the clear_bit completes.

I think this is a non-issue.

Re: [PATCH] usbnet: Fix two races between usbnet_stop() and the BH

[Alan Stern]

On Mon, 24 Aug 2015, David Miller wrote:

> From: Eugene Shatokhin <eugene.shatokhin@rosalab.ru>
> Date: Wed, 19 Aug 2015 14:59:01 +0300
> 
> > So the following might be possible, although unlikely:
> > 
> > CPU0             CPU1
> >                  clear_bit: read dev->flags
> >                  clear_bit: clear EVENT_RX_KILL in the read value
> > 
> > dev->flags=0;
> > 
> >                  clear_bit: write updated dev->flags
> > 
> > As a result, dev->flags may become non-zero again.
> 
> Is this really possible?
> 
> Stores really are "atomic" in the sense that the do their update
> in one indivisible operation.

Provided you use ACCESS_ONCE or WRITE_ONCE or whatever people like to 
call it now.

> Atomic operations like clear_bit also will behave that way.

Are you certain about that?  I couldn't find any mention of it in
Documentation/atomic_ops.txt.

In theory, an architecture could implement atomic bit operations using 
a spinlock to insure atomicity.  I don't know if any architectures do 
this, but if they do then the scenario above could arise.

Alan Stern

Re: [PATCH] usbnet: Fix two races between usbnet_stop() and the BH

[Eugene Shatokhin]

24.08.2015 20:43, David Miller пишет:
> From: Eugene Shatokhin <eugene.shatokhin@rosalab.ru>
> Date: Wed, 19 Aug 2015 14:59:01 +0300
>
>> So the following might be possible, although unlikely:
>>
>> CPU0             CPU1
>>                   clear_bit: read dev->flags
>>                   clear_bit: clear EVENT_RX_KILL in the read value
>>
>> dev->flags=0;
>>
>>                   clear_bit: write updated dev->flags
>>
>> As a result, dev->flags may become non-zero again.
>
> Is this really possible?

On x86, it is not possible, so this is not a problem. Perhaps, for ARM 
too. As for the other architectures supported by the kernel - not sure, 
no common guarantees, it seems. Anyway, this is not a critical issue, I 
agree.

OK, let us leave things as they are for this one and fix the rest.

>
> Stores really are "atomic" in the sense that the do their update
> in one indivisible operation.
>
> Atomic operations like clear_bit also will behave that way.
>
> If a clear_bit is in progress, the "dev->flags=0" store will not be
> able to grab the cache line exclusively until the clear_bit is done.
>
> So I think the above sequent of events is completely impossible.  Once
> a clear_bit starts, a write by another foreign agent on the bus is
> absolutely impossible to legally occur until the clear_bit completes.
>
> I think this is a non-issue.
>

Regards,
Eugene

Re: [PATCH] usbnet: Fix two races between usbnet_stop() and the BH

[Alan Stern]

On Mon, 24 Aug 2015, Alan Stern wrote:

> On Mon, 24 Aug 2015, David Miller wrote:
> 
> > From: Eugene Shatokhin <eugene.shatokhin@rosalab.ru>
> > Date: Wed, 19 Aug 2015 14:59:01 +0300
> > 
> > > So the following might be possible, although unlikely:
> > > 
> > > CPU0             CPU1
> > >                  clear_bit: read dev->flags
> > >                  clear_bit: clear EVENT_RX_KILL in the read value
> > > 
> > > dev->flags=0;
> > > 
> > >                  clear_bit: write updated dev->flags
> > > 
> > > As a result, dev->flags may become non-zero again.
> > 
> > Is this really possible?
> > 
> > Stores really are "atomic" in the sense that the do their update
> > in one indivisible operation.
> 
> Provided you use ACCESS_ONCE or WRITE_ONCE or whatever people like to 
> call it now.
> 
> > Atomic operations like clear_bit also will behave that way.
> 
> Are you certain about that?  I couldn't find any mention of it in
> Documentation/atomic_ops.txt.
> 
> In theory, an architecture could implement atomic bit operations using 
> a spinlock to insure atomicity.  I don't know if any architectures do 
> this, but if they do then the scenario above could arise.

Now that I see this in writing, I realize it's not possible after all.  
clear_bit() et al. will work with a single unsigned long, which doesn't
leave any place for spinlocks or other mechanisms.  I was thinking of 
atomic_t.

So never mind...

Alan Stern

Re: [PATCH] usbnet: Fix two races between usbnet_stop() and the BH

[David Miller]

From: Alan Stern <stern@rowland.harvard.edu>
Date: Mon, 24 Aug 2015 14:06:15 -0400 (EDT)

> On Mon, 24 Aug 2015, David Miller wrote:
>> Atomic operations like clear_bit also will behave that way.
> 
> Are you certain about that?  I couldn't find any mention of it in
> Documentation/atomic_ops.txt.
> 
> In theory, an architecture could implement atomic bit operations using 
> a spinlock to insure atomicity.  I don't know if any architectures do 
> this, but if they do then the scenario above could arise.

Indeed, we do have platforms like 32-bit sparc and parisc that do this.

So, taking that into consideration, this is a bit unfortunate and on
such platforms we do have this problem.

[PATCH 0/2] usbnet: Fix 2 problems in usbnet_stop()

[Eugene Shatokhin]

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

[PATCH 1/2] usbnet: Get EVENT_NO_RUNTIME_PM bit before it is cleared

[Eugene Shatokhin]

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 <oneukum@suse.de>.

Signed-off-by: Eugene Shatokhin <eugene.shatokhin@rosalab.ru>
---
 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

[PATCH 2/2] usbnet: Fix a race between usbnet_stop() and the BH

[Eugene Shatokhin]

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 <eugene.shatokhin@rosalab.ru>
---
 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

Re: [PATCH 2/2] usbnet: Fix a race between usbnet_stop() and the BH

[Bjørn Mork]

Eugene Shatokhin <eugene.shatokhin@rosalab.ru> writes:

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

Exactly what problem will that result in?  The tasklet_kill() will wait
for the processing of the single element done queue, and everything will
be fine.  Or?


Bjørn

Re: [PATCH] usbnet: Fix two races between usbnet_stop() and the BH

[Oliver Neukum]

On Mon, 2015-08-24 at 15:29 +0200, Bjørn Mork wrote:
> Eugene Shatokhin <eugene.shatokhin@rosalab.ru> writes:
> 
> > 19.08.2015 15:31, Bjørn Mork пишет:
> >> Eugene Shatokhin <eugene.shatokhin@rosalab.ru> writes:

> >> Stopping the tasklet rescheduling etc depends only on netif_running(),
> >> which will be false when usbnet_stop is called.  There is no need to
> >> touch dev->flags for this to happen.
> >
> > That was one of the first ideas we discussed here. Unfortunately, it
> > is probably not so simple.
> >
> > Setting dev->flags to 0 makes some delayed operations do nothing and,
> > among other things, not to reschedule usbnet_bh().
> 
> Yes, but I believe that is merely a side effect.  You should never need
> to clear multiple flags to get the desired behaviour.

Why? Is there any reason you cannot have a TX and an RX halt at the same
time?

> > As you can see in drivers/net/usb/usbnet.c, usbnet_bh() can be called
> > as a tasklet function and as a timer function in a number of
> > situations (look for the usage of dev->bh and dev->delay there).
> >
> > netif_running() is indeed false when usbnet_stop() runs, usbnet_stop()
> > also disables Tx. This seems to be enough for many cases where
> > usbnet_bh() is scheduled, but I am not so sure about the remaining
> > ones, namely:
> >
> > 1. A work function, usbnet_deferred_kevent(), may reschedule
> > usbnet_bh(). Looks like the workqueue is only stopped in
> > usbnet_disconnect(), so a work item might be processed while
> > usbnet_stop() works. Setting dev->flags to 0 makes the work function
> > do nothing, by the way. See also the comment in usbnet_stop() about
> > this.

Yes, this is the main reason the flags are collectively cleared.
We could do them all with clear_bit(). Ugly though.

> > A work item may be placed to this workqueue in a number of ways, by
> > both usbnet module and the mini-drivers. It is not too easy to track
> > all these situations.
> 
> That's an understatement :)

Yes.

> So FLAG_AVOID_UNLINK_URBS should probably be removed and replaced calls
> to usbnet_status_start() and usbnet_status_stop().  This will require
> testing on some of the devices with the original firmware problem
> however.

And there you point out the main problem.

> In any case: I do not think this flag should be considered when trying
> to make usbnet_stop behaviour saner.  It's only purpose is to
> deliberately break usbnet_stop by not actually stopping.

Yes.

	Regards
		Oliver

Re: [PATCH] usbnet: Fix two races between usbnet_stop() and the BH

[Oliver Neukum]

On Mon, 2015-08-24 at 14:21 -0400, Alan Stern wrote:
> > In theory, an architecture could implement atomic bit operations
> using 
> > a spinlock to insure atomicity.  I don't know if any architectures
> do 
> > this, but if they do then the scenario above could arise.
> 
> Now that I see this in writing, I realize it's not possible after
> all.  
> clear_bit() et al. will work with a single unsigned long, which
> doesn't
> leave any place for spinlocks or other mechanisms.  I was thinking of 
> atomic_t.

Refuting yourself you are making the assumption that the lock has
to be inside the data structure. That is not true.

	Regards
		Oliver

Re: [PATCH 1/2] usbnet: Get EVENT_NO_RUNTIME_PM bit before it is cleared

[Oliver Neukum]

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

Re: [PATCH 1/2] usbnet: Get EVENT_NO_RUNTIME_PM bit before it is cleared

[Bjørn Mork]

Oliver Neukum <oneukum@suse.com> writes:

> 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?

I definitely agree.


Bjørn

Re: [PATCH 1/2] usbnet: Get EVENT_NO_RUNTIME_PM bit before it is cleared

[Oliver Neukum]

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 <oneukum@suse.de>.
> 
> Signed-off-by: Eugene Shatokhin <eugene.shatokhin@rosalab.ru>
Acked-by: Oliver Neukum <oneukum@suse.com>

Re: [PATCH 1/2] usbnet: Get EVENT_NO_RUNTIME_PM bit before it is cleared

[David Miller]

From: Eugene Shatokhin <eugene.shatokhin@rosalab.ru>
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 <oneukum@suse.de>.
> 
> Signed-off-by: Eugene Shatokhin <eugene.shatokhin@rosalab.ru>

Applied and queued up for -stable.

Re: [PATCH 2/2] usbnet: Fix a race between usbnet_stop() and the BH

[David Miller]

From: Eugene Shatokhin <eugene.shatokhin@rosalab.ru>
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.

Re: [PATCH 2/2] usbnet: Fix a race between usbnet_stop() and the BH

[Eugene Shatokhin]

25.08.2015 00:01, Bjørn Mork пишет:
> Eugene Shatokhin <eugene.shatokhin@rosalab.ru> writes:
>
>> 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.
>
> Exactly what problem will that result in?  The tasklet_kill() will wait
> for the processing of the single element done queue, and everything will
> be fine.  Or?

Given enough time, what prevents defer_bh() from calling 
tasklet_schedule(&dev->bh) *after* usbnet_stop() calls tasklet_kill()?

Consider the following situation (assuming '&&' are changed to '||' in 
that while loop in usbnet_terminate_urbs() as they should be):

CPU0                            CPU1
usbnet_stop()                   defer_bh() with list == dev->rxq
   usbnet_terminate_urbs()
                                 __skb_unlink() removes the last
                                 skb from dev->rxq.
                                 dev->rxq, dev->txq and dev->done
                                 are now empty.
   while (!skb_queue_empty()...)
     The loop ends because all 3
     queues are now empty.

   usbnet_terminate_urbs() ends.

usbnet_stop() continues:
   usbnet_status_stop(dev);
   ...
   del_timer_sync (&dev->delay);
   tasklet_kill (&dev->bh);
                                 __skb_queue_tail(&dev->done, skb);
                                 if (dev->done.qlen == 1)
                                   tasklet_schedule(&dev->bh);

The BH is scheduled at this point, which is not what was intended. The 
race window is small, but still.

Regards,
Eugene

Re: [PATCH 2/2] usbnet: Fix a race between usbnet_stop() and the BH

[Bjørn Mork]

Eugene Shatokhin <eugene.shatokhin@rosalab.ru> writes:

> 25.08.2015 00:01, Bjørn Mork пишет:
>> Eugene Shatokhin <eugene.shatokhin@rosalab.ru> writes:
>>
>>> 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.
>>
>> Exactly what problem will that result in?  The tasklet_kill() will wait
>> for the processing of the single element done queue, and everything will
>> be fine.  Or?
>
> Given enough time, what prevents defer_bh() from calling
> tasklet_schedule(&dev->bh) *after* usbnet_stop() calls tasklet_kill()?
>
> Consider the following situation (assuming '&&' are changed to '||' in
> that while loop in usbnet_terminate_urbs() as they should be):
>
> CPU0                            CPU1
> usbnet_stop()                   defer_bh() with list == dev->rxq
>   usbnet_terminate_urbs()
>                                 __skb_unlink() removes the last
>                                 skb from dev->rxq.
>                                 dev->rxq, dev->txq and dev->done
>                                 are now empty.
>   while (!skb_queue_empty()...)
>     The loop ends because all 3
>     queues are now empty.
>
>   usbnet_terminate_urbs() ends.
>
> usbnet_stop() continues:
>   usbnet_status_stop(dev);
>   ...
>   del_timer_sync (&dev->delay);
>   tasklet_kill (&dev->bh);
>                                 __skb_queue_tail(&dev->done, skb);
>                                 if (dev->done.qlen == 1)
>                                   tasklet_schedule(&dev->bh);
>
> The BH is scheduled at this point, which is not what was intended. The
> race window is small, but still.

I guess you are right.  At least I cannot prove that you are not :)

There is a bit too much complexity involved here for me...



Bjørn

Re: [PATCH 2/2] usbnet: Fix a race between usbnet_stop() and the BH

[Eugene Shatokhin]

28.08.2015 11:55, Bjørn Mork пишет:
> Eugene Shatokhin <eugene.shatokhin@rosalab.ru> writes:
>
>> 25.08.2015 00:01, Bjørn Mork пишет:
>>> Eugene Shatokhin <eugene.shatokhin@rosalab.ru> writes:
>>>
>>>> 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.
>>>
>>> Exactly what problem will that result in?  The tasklet_kill() will wait
>>> for the processing of the single element done queue, and everything will
>>> be fine.  Or?
>>
>> Given enough time, what prevents defer_bh() from calling
>> tasklet_schedule(&dev->bh) *after* usbnet_stop() calls tasklet_kill()?
>>
>> Consider the following situation (assuming '&&' are changed to '||' in
>> that while loop in usbnet_terminate_urbs() as they should be):
>>
>> CPU0                            CPU1
>> usbnet_stop()                   defer_bh() with list == dev->rxq
>>    usbnet_terminate_urbs()
>>                                  __skb_unlink() removes the last
>>                                  skb from dev->rxq.
>>                                  dev->rxq, dev->txq and dev->done
>>                                  are now empty.
>>    while (!skb_queue_empty()...)
>>      The loop ends because all 3
>>      queues are now empty.
>>
>>    usbnet_terminate_urbs() ends.
>>
>> usbnet_stop() continues:
>>    usbnet_status_stop(dev);
>>    ...
>>    del_timer_sync (&dev->delay);
>>    tasklet_kill (&dev->bh);
>>                                  __skb_queue_tail(&dev->done, skb);
>>                                  if (dev->done.qlen == 1)
>>                                    tasklet_schedule(&dev->bh);
>>
>> The BH is scheduled at this point, which is not what was intended. The
>> race window is small, but still.
>
> I guess you are right.  At least I cannot prove that you are not :)
>
> There is a bit too much complexity involved here for me...

:-)

Yes, it is quite complex.

I admit, it was easier for me to find the races in usbnet (the tools 
like KernelStrider and RaceHound do the dirty work) than to analyze 
their consequences. The latter often requires some time and effort, and 
so it did this time.

Well, any objections to this patch?

Regards,

Eugene

Re: [PATCH 2/2] usbnet: Fix a race between usbnet_stop() and the BH

[Bjørn Mork]

Eugene Shatokhin <eugene.shatokhin@rosalab.ru> writes:
> 28.08.2015 11:55, Bjørn Mork пишет:
>
>> I guess you are right.  At least I cannot prove that you are not :)
>>
>> There is a bit too much complexity involved here for me...
>
> :-)
>
> Yes, it is quite complex.
>
> I admit, it was easier for me to find the races in usbnet (the tools
> like KernelStrider and RaceHound do the dirty work) than to analyze
> their consequences. The latter often requires some time and effort,
> and so it did this time.
>
> Well, any objections to this patch?

No objections from me.

But I would have liked it much better if the code became simpler instead
of more complex.


Bjørn

Re: [PATCH 2/2] usbnet: Fix a race between usbnet_stop() and the BH

[Eugene Shatokhin]

31.08.2015 10:32, Bjørn Mork пишет:
> Eugene Shatokhin <eugene.shatokhin@rosalab.ru> writes:
>> 28.08.2015 11:55, Bjørn Mork пишет:
>>
>>> I guess you are right.  At least I cannot prove that you are not :)
>>>
>>> There is a bit too much complexity involved here for me...
>>
>> :-)
>>
>> Yes, it is quite complex.
>>
>> I admit, it was easier for me to find the races in usbnet (the tools
>> like KernelStrider and RaceHound do the dirty work) than to analyze
>> their consequences. The latter often requires some time and effort,
>> and so it did this time.
>>
>> Well, any objections to this patch?
>
> No objections from me.
>
> But I would have liked it much better if the code became simpler instead
> of more complex.

Me too, but I can see no other way here. The code is simpler without 
locking, indeed, but locking is needed to prevent the problems described 
earlier.

One needs to make sure that checking if txq or rxq is empty in 
usbnet_terminate_urbs() cannot get inbetween of processing of these 
queues and dev->done in defer_bh(). So 'list' and 'dev->done' must be 
updated under a common lock in defer_bh(). list->lock is an obvious 
candidate for this.

For the same reason, skb_queue_empty(q) must be called under q->lock. So 
the code takes it, calls skb_queue_empty(q) and then releases it to wait 
a little. Rinse and repeat.

The last complex piece is that spin_lock_nested() in defer_bh. It is 
safe to take both list->lock and dev->done.lock there (defer_bh can only 
be called for list = dev->rxq or dev->txq but not for dev->done). For 
lockdep, however, this is suspicious because '*list' and 'dev->done' are 
of the same type so the lock class is the same. So it complained.

To tell lockdep it is OK to use such locking scheme in this particular 
case, the recommended pattern was used: spin_lock_nested with 
SINGLE_DEPTH_NESTING.

Regards,
Eugene

Re: [PATCH 2/2] usbnet: Fix a race between usbnet_stop() and the BH

[Oliver Neukum]

On Fri, 2015-08-28 at 11:09 +0300, Eugene Shatokhin wrote:
> > Exactly what problem will that result in?  The tasklet_kill() will
> wait
> > for the processing of the single element done queue, and everything
> will
> > be fine.  Or?
> 
> Given enough time, what prevents defer_bh() from calling 
> tasklet_schedule(&dev->bh) *after* usbnet_stop() calls tasklet_kill()?
> 
> Consider the following situation (assuming '&&' are changed to '||'
> in 
> that while loop in usbnet_terminate_urbs() as they should be):
> 
> CPU0                            CPU1
> usbnet_stop()                   defer_bh() with list == dev->rxq
>    usbnet_terminate_urbs()
>                                  __skb_unlink() removes the last
>                                  skb from dev->rxq.
>                                  dev->rxq, dev->txq and dev->done
>                                  are now empty.
>    while (!skb_queue_empty()...)
>      The loop ends because all 3
>      queues are now empty.
> 
>    usbnet_terminate_urbs() ends.
> 
> usbnet_stop() continues:
>    usbnet_status_stop(dev);
>    ...
>    del_timer_sync (&dev->delay);
>    tasklet_kill (&dev->bh);
>                                  __skb_queue_tail(&dev->done, skb);
>                                  if (dev->done.qlen == 1)
>                                    tasklet_schedule(&dev->bh);
> 
> The BH is scheduled at this point, which is not what was intended.
> The 
> race window is small, but still.

This looks possible. I cannot come up with a better fix, although
it isn't nice. Thanks for finding this.

	Regards
		Oliver

Re: [PATCH 2/2] usbnet: Fix a race between usbnet_stop() and the BH

[Oliver Neukum]

On Mon, 2015-08-31 at 11:50 +0300, Eugene Shatokhin wrote:
> > But I would have liked it much better if the code became simpler
> instead
> > of more complex.
> 
> Me too, but I can see no other way here. The code is simpler without 
> locking, indeed, but locking is needed to prevent the problems
> described 
> earlier.

On a practical note, will you resend the patch?

	Regards
		Oliver

Re: [PATCH 2/2] usbnet: Fix a race between usbnet_stop() and the BH

[Eugene Shatokhin]

01.09.2015 10:58, Oliver Neukum пишет:
> On Mon, 2015-08-31 at 11:50 +0300, Eugene Shatokhin wrote:
>>> But I would have liked it much better if the code became simpler
>> instead
>>> of more complex.
>>
>> Me too, but I can see no other way here. The code is simpler without
>> locking, indeed, but locking is needed to prevent the problems
>> described
>> earlier.
>
> On a practical note, will you resend the patch?

Yes, I will do it shortly. Thanks for your help!

Regards,

Eugene

[PATCH] usbnet: Fix a race between usbnet_stop() and the BH

[Eugene Shatokhin]

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 <eugene.shatokhin@rosalab.ru>
---
 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

Re: [PATCH] usbnet: Fix a race between usbnet_stop() and the BH

[Eugene Shatokhin]

01.09.2015 17:05, Eugene Shatokhin пишет:
> 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 <eugene.shatokhin@rosalab.ru>
> ---
>   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);
>   }
>

I resent the patch to make it separate. What is the status of this now?

Regards,
Eugene

Re: [PATCH] usbnet: Fix a race between usbnet_stop() and the BH

[Bjørn Mork]

Eugene Shatokhin <eugene.shatokhin@rosalab.ru> writes:

> I resent the patch to make it separate. What is the status of this now?

One of the many nice features of patchwork is that you don't need to ask
those questions :)

See http://patchwork.ozlabs.org/patch/512856/

I really don't think it's appropriate for me to ack this, but I can add
my

Reviewed-by: Bjørn Mork <bjorn@mork.no>

if that helps.


Bjørn

Re: [PATCH] usbnet: Fix a race between usbnet_stop() and the BH

[Oliver Neukum]

On Tue, 2015-09-08 at 09:37 +0200, Bjørn Mork wrote:
> Eugene Shatokhin <eugene.shatokhin@rosalab.ru> writes:
> 
> > I resent the patch to make it separate. What is the status of this now?
> 
> One of the many nice features of patchwork is that you don't need to ask
> those questions :)
> 
> See http://patchwork.ozlabs.org/patch/512856/
> 
> I really don't think it's appropriate for me to ack this, but I can add
> my

Well, in case this helps:

> Reviewed-by: Bjørn Mork <bjorn@mork.no>
Acked-by: Oliver Neukum <oneukum@suse.com>

	Regards
		Oliver

Re: [PATCH] usbnet: Fix a race between usbnet_stop() and the BH

[David Miller]

From: Eugene Shatokhin <eugene.shatokhin@rosalab.ru>
Date: Tue,  1 Sep 2015 17:05:33 +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:
 ...
> 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 <eugene.shatokhin@rosalab.ru>

Applied, thanks.