[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <55DB0C1C.5030607@rosalab.ru>
Date: Mon, 24 Aug 2015 15:20:44 +0300
From: Eugene Shatokhin <eugene.shatokhin@...alab.ru>
To: Bjørn Mork <bjorn@...k.no>
Cc: David Miller <davem@...emloft.net>, oneukum@...e.com,
netdev@...r.kernel.org, linux-usb@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] usbnet: Fix two races between usbnet_stop() and the BH
19.08.2015 15:31, Bjørn Mork пишет:
> Eugene Shatokhin <eugene.shatokhin@...alab.ru> writes:
>
>> The problem is not in the reordering but rather in the fact that
>> "dev->flags = 0" is not necessarily atomic
>> w.r.t. "clear_bit(EVENT_RX_KILL, &dev->flags)", and vice versa.
>>
>> So the following might be possible, although unlikely:
>>
>> CPU0 CPU1
>> clear_bit: read dev->flags
>> clear_bit: clear EVENT_RX_KILL in the read value
>>
>> dev->flags=0;
>>
>> clear_bit: write updated dev->flags
>>
>> As a result, dev->flags may become non-zero again.
>
> Ah, right. Thanks for explaining.
>
>> I cannot prove yet that this is an impossible situation. If anyone
>> can, please explain. If so, this part of the patch will not be needed.
>
> I wonder if we could simply move the dev->flags = 0 down a few lines to
> fix both issues? It doesn't seem to do anything useful except for
> resetting the flags to a sane initial state after the device is down.
>
> Stopping the tasklet rescheduling etc depends only on netif_running(),
> which will be false when usbnet_stop is called. There is no need to
> touch dev->flags for this to happen.
That was one of the first ideas we discussed here. Unfortunately, it is
probably not so simple.
Setting dev->flags to 0 makes some delayed operations do nothing and,
among other things, not to reschedule usbnet_bh().
As you can see in drivers/net/usb/usbnet.c, usbnet_bh() can be called as
a tasklet function and as a timer function in a number of situations
(look for the usage of dev->bh and dev->delay there).
netif_running() is indeed false when usbnet_stop() runs, usbnet_stop()
also disables Tx. This seems to be enough for many cases where
usbnet_bh() is scheduled, but I am not so sure about the remaining ones,
namely:
1. A work function, usbnet_deferred_kevent(), may reschedule
usbnet_bh(). Looks like the workqueue is only stopped in
usbnet_disconnect(), so a work item might be processed while
usbnet_stop() works. Setting dev->flags to 0 makes the work function do
nothing, by the way. See also the comment in usbnet_stop() about this.
A work item may be placed to this workqueue in a number of ways, by both
usbnet module and the mini-drivers. It is not too easy to track all
these situations.
2. rx_complete() and tx_complete() may schedule execution of usbnet_bh()
as a tasklet or a timer function. These two are URB completion callbacks.
It seems, new Rx and Tx URBs cannot be submitted when usbnet_stop()
clears dev->flags, indeed. But it does not prevent the completion
handlers for the previously submitted URBs from running concurrently
with usbnet_stop(). The latter waits for them to complete (via
usbnet_terminate_urbs(dev)) but only if FLAG_AVOID_UNLINK_URBS is not
set in info->flags. rndis_wlan, however, sets this flag for a few
hardware models. So - no guarantees here as well.
If someone could list the particular bits of dev->flags that should be
cleared to make sure no deferred call could reschedule usbnet_bh(),
etc... Well, it would be enough to clear these first and use dev->flags
= 0 later, after tasklet_kill() and del_timer_sync(). I cannot point out
these particular bits now.
Besides, it is possible, although unlikely, that new event bits will be
added to dev->flags in the future. And one will need to keep track of
these to see if they should be cleared as well. I'd prever to play safer
for now and clear them all.
>
>>> The EVENT_NO_RUNTIME_PM bug should definitely be fixed. Please split
>>> that out as a separate fix. It's a separate issue, and should be
>>> backported to all maintained stable releases it applies to (anything
>>> from v3.8 and newer)
>>
>> Yes, that makes sense. However, this fix was originally provided by
>> Oliver Neukum rather than me, so I would like to hear his opinion as
>> well first.
>
> If what I write above is correct (please help me verify...), then maybe
> it does make sense to do these together anyway.
I think, your suggestion to make it a separate patch is right. Will do
it in the next version of the patchset, hopefully soon.
Regards,
Eugene
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists