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 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
