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

2015-08-14 Thread 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, >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 
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 
---
  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, >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, >flags);
dev->flags = 0;
del_timer_sync (>delay);
tasklet_kill (>bh);
+   mpn |= !test_and_clear_bit(EVENT_NO_RUNTIME_PM, >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, >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




--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2015-08-14 Thread 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 oneu...@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 oneu...@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




--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2015-07-27 Thread 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, >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, >flags)
del_timer_sync (>delay);
tasklet_kill (>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


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2015-07-27 Thread 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 >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(>done, skb) called from rx_process() and 
skb_dequeue (>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(>lock);
spin_lock(>done.lock);
__skb_queue_tail(>done, skb);
if (dev->done.qlen == 1)
tasklet_schedule(>bh);
-   spin_unlock_irqrestore(>done.lock, flags);
+   spin_unlock(>done.lock);
+   spin_unlock_irqrestore(>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(>lock, flags);
+   while (!skb_queue_empty(q)) {
+   spin_unlock_irqrestore(>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(>lock, flags);
+   }
+   spin_unlock_irqrestore(>lock, flags);
+}
+


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



Regards,

Eugene

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2015-07-27 Thread 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 >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(>lock);
>   spin_lock(>done.lock);
>   __skb_queue_tail(>done, skb);
>   if (dev->done.qlen == 1)
>   tasklet_schedule(>bh);
> - spin_unlock_irqrestore(>done.lock, flags);
> + spin_unlock(>done.lock);
> + spin_unlock_irqrestore(>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(>lock, flags);
> + while (!skb_queue_empty(q)) {
> + spin_unlock_irqrestore(>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(>lock, flags);
> + }
> + spin_unlock_irqrestore(>lock, flags);
> +}
> +

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

Regards
Oliver


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2015-07-27 Thread 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


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2015-07-27 Thread 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


--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2015-07-27 Thread 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

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2015-07-27 Thread 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


--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2015-07-27 Thread 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


--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2015-07-24 Thread 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(>lock, flags);
old_state = entry->state;
entry->state = state;
__skb_unlink(skb, list);
spin_unlock(>lock);
spin_lock(>done.lock);
__skb_queue_tail(>done, skb);
if (dev->done.qlen == 1)
tasklet_schedule(>bh);
spin_unlock_irqrestore(>done.lock, flags);
#2 rx_complete (usbnet.c:640)
state = defer_bh(dev, skb, >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(>rxq)
&& !skb_queue_empty(>txq)
&& !skb_queue_empty(>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(>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(>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 >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(>lock);
spin_lock(>done.lock);
__skb_queue_tail(>done, skb);
if (dev->done.qlen == 1)
tasklet_schedule(>bh);
-   spin_unlock_irqrestore(>done.lock, flags);
+   spin_unlock(>done.lock);
+   spin_unlock_irqrestore(>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(>lock, flags);
+   while (!skb_queue_empty(q)) {
+   spin_unlock_irqrestore(>lock, flags);
+   schedule_timeout(msecs_to_jiffies(UNLINK_TIMEOUT_MS));
+   set_current_state(TASK_UNINTERRUPTIBLE);
+   spin_lock_irqsave(>lock, flags);
+   }
+   spin_unlock_irqrestore(>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, >rxq);

/* maybe wait for deletions to finish. */
-   while (!skb_queue_empty(>rxq)
-   && !skb_queue_empty(>txq)
-   && !skb_queue_empty(>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(>rxq);
+   wait_skb_queue_empty(>txq);
+   wait_skb_queue_empty(>done);
+

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

2015-07-24 Thread 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, >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, >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, >flags);
dev->flags = 0;
+   smp_mb(); /* make sure the workers see that dev->flags == 0 */
+
del_timer_sync (>delay);
tasklet_kill (>bh);
+
if (!pm)
usb_autopm_put_interface(dev->intf);

-   if (info->manage_power &&
-   !test_and_clear_bit(EVENT_NO_RUNTIME_PM, >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, >flags)) {
unlink_urbs (dev, >txq);


What do you think?

Regards,
Eugene

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2015-07-24 Thread 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

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2015-07-24 Thread 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);
+

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

2015-07-23 Thread 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



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2015-07-23 Thread 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


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2015-07-23 Thread 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, >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


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2015-07-23 Thread 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



--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2015-07-23 Thread 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


--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2015-07-23 Thread 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


--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2015-07-22 Thread 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, >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 
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 
---
  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, >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, >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 (>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 (>delay);
tasklet_kill (>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, >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, >flags))
+   if (info->manage_power && mpn)
info->manage_power(dev, 0);
else
usb_autopm_put_interface(dev->intf);



Regards,
Eugene
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2015-07-22 Thread 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 oneu...@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 oneu...@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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2015-07-21 Thread 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, >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 
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 
---
 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, >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, >flags);
dev->flags = 0;
del_timer_sync (>delay);
tasklet_kill (>bh);
+   mpn |= !test_and_clear_bit(EVENT_NO_RUNTIME_PM, >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, >flags))
+   if (info->manage_power && mpn)
info->manage_power(dev, 0);
else
usb_autopm_put_interface(dev->intf);
-- 
2.1.4



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2015-07-21 Thread 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



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2015-07-21 Thread 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(>lock, flags);
>   old_state = entry->state;
>   entry->state = state;
>   __skb_unlink(skb, list);
>   spin_unlock(>lock);
>   spin_lock(>done.lock);
>   __skb_queue_tail(>done, skb);
>   if (dev->done.qlen == 1)
>   tasklet_schedule(>bh);
>   spin_unlock_irqrestore(>done.lock, flags);
> #2 rx_complete (usbnet.c:640)
>   state = defer_bh(dev, skb, >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(>rxq)
>   && !skb_queue_empty(>txq)
>   && !skb_queue_empty(>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(>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(>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


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2015-07-21 Thread 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


--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2015-07-21 Thread 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



--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2015-07-21 Thread 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 oneu...@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 oneu...@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



--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/