FW: RE: Re: FW: RE: Re: Subject: [PATCH v3] USB:Core: BugFix: Proper handling of Race Condition when two USB class drivers try to call init_usb_class simultaneously

[Ajay Kaher]

>ÂOnÂWed,Â1ÂMarÂ2017,ÂAlanÂSternÂwrote:
>>ÂOnÂWed,Â1ÂMarÂ2017,ÂAjayÂKaherÂwrote:
>>>ÂOnÂMon,Â22ÂFebÂ2017,ÂAjayÂKaherÂwrote:
>>>Â
>>>>Â
>>>>>ÂAlan,ÂasÂperÂmyÂunderstandingÂIÂhaveÂshiftedÂtheÂlockÂfrom
>>>>>Ârelease_usb_class()ÂtoÂdestroy_usb_class()ÂinÂpatchÂv3.Â
>>>>>ÂIfÂitÂisÂnotÂright,ÂpleaseÂexplainÂinÂdetailÂwhichÂraceÂcondition
>>>>>ÂIÂhaveÂmissedÂandÂalsoÂshareÂyourÂsuggestions.
>>>>>Â
>>>>Â
>>>>ÂHaveÂyouÂconsideredÂwhatÂwouldÂhappenÂifÂdestroy_usb_class()Âran,ÂbutÂ
>>>>ÂsomeÂotherÂCPUÂwasÂstillÂholdingÂaÂreferenceÂtoÂusb_class?ÂÂAndÂwhatÂifÂ
>>>>ÂtheÂlastÂreferenceÂgetsÂdroppedÂlaterÂon,ÂwhileÂinit_usb_class()ÂisÂ
>>>>Ârunning?
>>>Â
>>>ÂAccessÂofÂusb_class->krefÂisÂonlyÂfromÂeitherÂinit_usb_class()
>>>ÂorÂdestroy_usb_class(),ÂandÂbothÂtheseÂfunctionsÂareÂnowÂprotected
>>>ÂwithÂMutexÂLockingÂinÂpatchÂv3,ÂsoÂthereÂisÂnoÂchanceÂofÂraceÂcondition
>>>ÂasÂperÂaboveÂscenarios.
>>>Â
>>>>ÂMaybeÂthat'sÂnotÂpossibleÂhere,ÂbutÂitÂisÂpossibleÂinÂgeneralÂforÂ
>>>>ÂrefcountedÂobjects.ÂÂSoÂyes,ÂthisÂcodeÂisÂprobablyÂokay,ÂbutÂitÂisn'tÂ
>>>>ÂgoodÂform.
>>>Â
>>>ÂAsÂperÂmyÂunderstanding,ÂIÂfoundÂtoÂbeÂoneÂofÂtheÂbestÂpossibleÂsolution
>>>ÂforÂthisÂproblemÂandÂthisÂsolutiuonÂdon'tÂhaveÂanyÂsideÂeffect.
>>Â
>>ÂAlan,ÂIÂhadÂsharedÂmodifiedÂPatchÂv3ÂasÂperÂyourÂinputsÂtoÂprevent
>>ÂtheÂraceÂconditionÂduringÂsimultaneouslyÂcallingÂofÂinit_usb_class().
>>ÂIfÂyouÂthinkÂthereÂisÂscopeÂtoÂimproveÂtheÂpatch,ÂpleaseÂshareÂyourÂinputs.
>Â
>ÂUnderÂtheÂcircumstances,ÂyourÂpatchÂisÂacceptable.
>Â
>ÂIfÂyouÂreallyÂwantÂtoÂmakeÂtheÂpointÂcrystalÂclear,ÂyouÂcouldÂreplaceÂ
>Âusb_class->krefÂwithÂanÂordinaryÂintegerÂcounter.ÂÂThenÂitÂwouldÂbeÂ
>ÂobviousÂthatÂthereÂareÂnoÂreferencesÂotherÂthanÂtheÂonesÂtakenÂbyÂ
>Âinit_usb_class()ÂandÂreleasedÂbyÂdestroy_usb_class().
Â
usb_class->kref is not accessible outside the file.c
as usb_class is _static_ inside the file.c and
pointer of usb_class->kref is not passed anywhere.

Hence as you wanted, there are no references of usb_class->kref
other than taken by init_usb_class() and released by destroy_usb_class().

Â
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..a12d184Â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)
Â{
@@Â-109,8Â+110,10Â@@ÂstaticÂvoidÂrelease_usb_class(structÂkrefÂ*kref)
Â
ÂstaticÂvoidÂdestroy_usb_class(void)
Â{
+ÂÂÂÂÂÂÂmutex_lock(&init_usb_class_mutex);
ÂÂÂÂÂÂÂÂifÂ(usb_class)
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂkref_put(&usb_class->kref,Ârelease_usb_class);
+ÂÂÂÂÂÂÂmutex_unlock(&init_usb_class_mutex);
Â}
Â
ÂintÂusb_major_init(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;
Â
Â
Â
Thanks and Regards,
Ajay Kaher