Message-ID: <1440505911.13824.4.camel@suse.com>
Subject: Re: [PATCH] usbnet: Fix two races between usbnet_stop() and the BH
From: Oliver Neukum <oneukum@suse.com>
To: =?ISO-8859-1?Q?Bj=F8rn?= Mork <bjorn@mork.no>
Cc: Eugene Shatokhin <eugene.shatokhin@rosalab.ru>,
        David Miller <davem@davemloft.net>, linux-kernel@vger.kernel.org,
        linux-usb@vger.kernel.org, netdev@vger.kernel.org
Date: Tue, 25 Aug 2015 14:31:51 +0200
In-Reply-To: <87io84byr8.fsf@nemi.mork.no>
References: <55CE1D7E.2070400@rosalab.ru>
	 <1439571516-11862-1-git-send-email-eugene.shatokhin@rosalab.ru>
	 <20150818.185407.1667358232705414236.davem@davemloft.net>
	 <55D436D5.6010105@rosalab.ru> <87k2sreefu.fsf@nemi.mork.no>
	 <55D46F85.50608@rosalab.ru> <877fore9yn.fsf@nemi.mork.no>
	 <55DB0C1C.5030607@rosalab.ru> <87io84byr8.fsf@nemi.mork.no>
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2728
Lines: 71

On Mon, 2015-08-24 at 15:29 +0200, Bjørn Mork wrote:
> Eugene Shatokhin <eugene.shatokhin@rosalab.ru> writes:
> 
> > 19.08.2015 15:31, Bjørn Mork пишет:
> >> Eugene Shatokhin <eugene.shatokhin@rosalab.ru> writes:

> >> 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?

> > 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.

> > 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


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