From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754237AbdBNPjM (ORCPT ); Tue, 14 Feb 2017 10:39:12 -0500 Received: from iolanthe.rowland.org ([192.131.102.54]:58268 "HELO iolanthe.rowland.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1752969AbdBNPjI (ORCPT ); Tue, 14 Feb 2017 10:39:08 -0500 Date: Tue, 14 Feb 2017 10:39:07 -0500 (EST) From: Alan Stern X-X-Sender: stern@iolanthe.rowland.org To: Ajay Kaher cc: "gregkh@linuxfoundation.org" , "linux-usb@vger.kernel.org" , "linux-kernel@vger.kernel.org" , AMAN DEEP , HEMANSHU SRIVASTAVA Subject: RE: Re: Re: Re: Subject: [PATCH v1] USB:Core: BugFix: Proper handling of Race Condition when two USB class drivers try to call init_usb_class simultaneously In-Reply-To: <20170202123640epcms5p40e39c110f2df2f0ef470ec201119e3c9@epcms5p4> Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=UTF-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, 2 Feb 2017, Ajay Kaher wrote: > >>>> At boot time, probe function of multiple connected devices > >>>> (proprietary devices) execute simultaneously. > >>> > >>> What exactly do you mean here?  How can probe happen "simultaneously"? > >>> The USB core prevents this, right? > >>  > >> I have observed two scenarios to call probe function: > >>  > >> Scenario #1: Driver inserted and attaching USB Device: > >> Yes, you are right, two probes at same time is not happening > >> in this scenario. > >>  > >> Scenario #2: USB Device attached and inserting Driver: > >> In this case probe has been called in context of insmod, > >> refer following code flow: > >> init -> usb_register_driver -> driver_register -> bus_add_driver -> > >> driver_attach -> bus_for_each_dev -> __driver_attach -> > >> driver_probe_device -> usb_probe_interface -> probe -> usb_register_dev > >>  > >> I have observed the crash in Scenario #2, as two probes executes at > >> same time in this scenario. And init_usb_class_mutex lock require to > >> prevent race condition. > >  > > What about the fact that in __driver_attach() we call device_lock() so > > that probe never gets called at the same time for the same device? > > Devices are different. > > > Or are you saying that you can load multiple USB modules at the same > > time?  If so, how is insmod running on multiple cpus at the same time? > > I thought we had a global lock there to prevent that from happening > > (i.e. only one module can be loaded at a time.)  Or is that what has > > recently changed? > > Yes, we can load multiple USB modules at the same time using insmod. > Tested on ARM Architecture with Linux kernel 4.1.10, that we can have > multiple insmod on multiple cpus at same time. Also reviewed load_module and > do_init_module functions and couldn't find any global lock. > > >  > > What is causing your modules to be loaded from userspace?  What type of > > device is this happening for?  And why haven't we seen this before? > > What kernel versions have you had a problem with this? > > Earlier we execute insmod in foreground as "insmod aaa.ko ; insmod bbb.ko" > and that's why insmod executed sequentially. Now we modified to execute in > parallel/background as "insmod aaa.ko & ; insmod bbb.ko &". > > > And what for what drivers specifically? > > This problem observed for drivers in which usb_register_dev called from their > probe function, and there are many such drivers. > > As per my opinion, usb_class structure is global and allocated + initialized > in usb_register_dev->init_usb_class function. Also as per scenario #2 > concurrency is possible, so protection using init_usb_class_mutex lock requires. > Don't you think so? > > >>>> And because of the following code path race condition happens: > >>>> probe->usb_register_dev->init_usb_class > >>> > >>> Why is this just showing up now, and hasn't been an issue for the decade > >>> or so this code has been around?  What changed? > >>> > >>>> Tested with these changes, and problem has been solved. > >>> > >>> What changes? > >>  > >> Tested with my patch (i.e. locking with init_usb_class_mutex). > >  > > I don't see a patch here :( > > Sorry, appending the patch again with this mail. >   > thanks, >   > ajay kaher > > > Signed-off-by: Ajay Kaher I think Ajay's argument is correct and a patch is needed. But this patch misses the race between init_usb_class() and release_usb_class(). The basic problem is that reference counting doesn't work when you try to use the same global pointer (usb_class) to refer to multiple generations of a dynamically allocated entity. We had the same sort of problem many years ago with the usb_interface structure (and we ultimately fixed it by creating a separate usb_interface_cache structure). The best approach here would be to forget about all the reference counting. Get rid of usb_class entirely, and create the "usbmisc" class structure just once, when usbcore initializes. Or, if you prefer, use a mutex to protect a routine that allocates the class structure dynamically, just once. Either way, don't deallocate it until usbcore is unloaded. Alan Stern