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

On Mon, 2015-08-24 at 15:29 +0200, Bjørn Mork wrote:

Eugene Shatokhin [off-list ref] writes:

quoted

19.08.2015 15:31, Bjørn Mork пишет:

quoted

Eugene Shatokhin [off-list ref] writes:

quoted

quoted

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

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

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

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

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

quoted

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

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

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

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

quoted

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

That's an understatement :)

Yes.

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

And there you point out the main problem.

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

Yes.

	Regards
		Oliver