Re: Several races in usbnet module (kernel 4.1.x)
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 netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Several races in usbnet module (kernel 4.1.x)
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 netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Several races in usbnet module (kernel 4.1.x)
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 netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Several races in usbnet module (kernel 4.1.x)
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 netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Several races in usbnet module (kernel 4.1.x)
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 netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Several races in usbnet module (kernel 4.1.x)
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 netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Several races in usbnet module (kernel 4.1.x)
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)
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 netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Several races in usbnet module (kernel 4.1.x)
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 netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Several races in usbnet module (kernel 4.1.x)
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 netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Several races in usbnet module (kernel 4.1.x)
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 netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Several races in usbnet module (kernel 4.1.x)
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 netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Several races in usbnet module (kernel 4.1.x)
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 netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Several races in usbnet module (kernel 4.1.x)
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 netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Several races in usbnet module (kernel 4.1.x)
Hi, I have recently found several data races in usbnet module, checked on vanilla kernel 4.1.0 on x86_64. The races do actually happen, I have confirmed it by adding delays and using hardware breakpoints to detect the conflicting memory accesses (with RaceHound tool, https://github.com/winnukem/racehound). I have not analyzed yet how harmful these races are (if they are), but it is better to report them anyway, I think. Everything was checked using YOTA 4G LTE Modem that works via usbnet and cdc_ether kernel modules. -- [Race #1] Race on skb_queue ('next' pointer) between usbnet_stop() and rx_complete(). Reproduced that by unplugging the device while the system was downloading a large file from the Net. Here is part of the call stack with the code where the changes to the queue happen: #0 __skb_unlink (skbuff.h:1517) prev-next = next; #1 defer_bh (usbnet.c:430) spin_lock_irqsave(list-lock, flags); old_state = entry-state; entry-state = state; __skb_unlink(skb, list); spin_unlock(list-lock); spin_lock(dev-done.lock); __skb_queue_tail(dev-done, skb); if (dev-done.qlen == 1) tasklet_schedule(dev-bh); spin_unlock_irqrestore(dev-done.lock, flags); #2 rx_complete (usbnet.c:640) state = defer_bh(dev, skb, dev-rxq, state); At the same time, the following code repeatedly checks if the queue is empty and reads the same values concurrently with the above changes: #0 usbnet_terminate_urbs (usbnet.c:765) /* maybe wait for deletions to finish. */ while (!skb_queue_empty(dev-rxq) !skb_queue_empty(dev-txq) !skb_queue_empty(dev-done)) { schedule_timeout(msecs_to_jiffies(UNLINK_TIMEOUT_MS)); set_current_state(TASK_UNINTERRUPTIBLE); netif_dbg(dev, ifdown, dev-net, waited for %d urb completions\n, temp); } #1 usbnet_stop (usbnet.c:806) if (!(info-flags FLAG_AVOID_UNLINK_URBS)) usbnet_terminate_urbs(dev); For example, it is possible that the skb is removed from dev-rxq by __skb_unlink() before the check !skb_queue_empty(dev-rxq) in usbnet_terminate_urbs() is made. It is also possible in this case that the skb is added to dev-done queue after !skb_queue_empty(dev-done) is checked. So usbnet_terminate_urbs() may stop waiting and return while dev-done queue still has an item. -- Unrelated the that race, if the goal of that while loop in usbnet_terminate_urbs() is to wait till all three queues (dev-rxq, dev-txq, dev-done) become empty, perhaps the code should be changed as follows: while (!skb_queue_empty(dev-rxq) -!skb_queue_empty(dev-txq) -!skb_queue_empty(dev-done)) { + || !skb_queue_empty(dev-txq) + || !skb_queue_empty(dev-done)) { schedule_timeout(...)); -- [Race #2] Races on dev-rx_qlen. Reproduced these by repeatedly changing MTU (1500 - 1400) while downloading large files. dev-rx_qlen is written to here: #0 usbnet_update_max_qlen (usbnet.c:351) case USB_SPEED_HIGH: dev-rx_qlen = MAX_QUEUE_MEMORY / dev-rx_urb_size; #1 __handle_link_change (usbnet.c:1049) /* hard_mtu or rx_urb_size may change during link change */ usbnet_update_max_qlen(dev); #2 usbnet_deferred_kevent (usbnet.c:1172) if (test_bit (EVENT_LINK_CHANGE, dev-flags)) __handle_link_change(dev); Here are the conflicting reads from dev-rx_qlen (via RX_QLEN(dev)), 3 code locations: * usbnet_bh (usbnet.c:1492) if (temp RX_QLEN(dev)) { ... * usbnet_bh (usbnet.c:1499) if (dev-rxq.qlen RX_QLEN(dev)) ... * rx_alloc_submit (usbnet.c:1431) for (i = 0; i 10 dev-rxq.qlen RX_QLEN(dev); i++) { ... -- [Race #3] Similar to race #2 but on dev-tx_qlen. I reproduced it the same way. dev-tx_qlen is written to here: #0 usbnet_update_max_qlen (usbnet.c:352) case USB_SPEED_HIGH: dev-rx_qlen = MAX_QUEUE_MEMORY / dev-rx_urb_size; dev-tx_qlen = MAX_QUEUE_MEMORY / dev-hard_mtu; #1 __handle_link_change (usbnet.c:1049) /* hard_mtu or rx_urb_size may change during link change */ usbnet_update_max_qlen(dev); #2 usbnet_deferred_kevent (usbnet.c:1172) if (test_bit (EVENT_LINK_CHANGE, dev-flags)) __handle_link_change(dev); Here are the conflicting reads from dev-tx_qlen (via TX_QLEN(dev)), 2 code locations: * usbnet_bh (usbnet.c:1502) if (dev-txq.qlen TX_QLEN (dev)) netif_wake_queue (dev-net); * usbnet_start_xmit (usbnet.c:1398) if (dev-txq.qlen = TX_QLEN (dev)) netif_stop_queue (net); --