RE: RE: Re: Re: Re: Subject: [PATCH v2] 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
>>Â
>>Â
>
> On Thu, 14 Feb 2017, Alan Stern wrote:
>Â
> 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().ÂÂ

Thanks Alan for your comments, in patch v2 I have taken care for
release_usb_class() also. Please review again.

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

usbmisc class creation should not require everytime when USB core
initializes. So better to keep usbmisc class creation as it is. 
And to prevent the race conditions just protect it with Mutex locking
as per patch v2.

thanks,
ajay kaher

Signed-off-by: Ajay Kaher

---

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

diff --git a/drivers/usb/core/file.c b/drivers/usb/core/file.c
index 822ced9..56a151b 100644
--- a/drivers/usb/core/file.c
+++ b/drivers/usb/core/file.c
@@ -27,6 +27,7 @@
 #define MAX_USB_MINORS 256
 static const struct file_operations *usb_minors[MAX_USB_MINORS];
 static DECLARE_RWSEM(minor_rwsem);
+static DEFINE_MUTEX(init_usb_class_mutex);

 static int usb_open(struct inode *inode, struct file *file)
 {
@@ -102,9 +103,11 @@ static int init_usb_class(void)
 static void release_usb_class(struct kref *kref)
 {
        /* Ok, we cheat as we know we only have one usb_class */
+       mutex_lock(&init_usb_class_mutex);
        class_destroy(usb_class->class);
        kfree(usb_class);
        usb_class = NULL;
+       mutex_unlock(&init_usb_class_mutex);
 }

 static void destroy_usb_class(void)
@@ -171,7 +174,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;