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

[Alan Stern]

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