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

[Ajay Kaher]

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

---

 drivers/usb/core/file.c |    4 ++++
 1 file changed, 4 insertions(+)


diff --git a/drivers/usb/core/file.c b/drivers/usb/core/file.c
index 822ced9..dedc47c 100644
--- a/drivers/usb/core/file.c
+++ b/drivers/usb/core/file.c
@@ -156,6 +156,7 @@ int usb_register_dev(struct usb_interface *intf,
  int minor_base = class_driver->minor_base;
  int minor;
  char name[20];
+ static DEFINE_MUTEX(init_usb_class_mutex);
 
 #ifdef CONFIG_USB_DYNAMIC_MINORS
  /*
@@ -171,7 +172,10 @@ int usb_register_dev(struct usb_interface *intf,
  if (intf->minor >= 0)
   return -EADDRINUSE;
 
+ mutex_lock(&init_usb_class_mutex);
  retval = init_usb_class();
+ mutex_unlock(&init_usb_class_mutex);
+
  if (retval)
   return retval;