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

[Eugene Shatokhin <eugene.shatokhin@rosalab.ru>]

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