FW: 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 Thr, 2 Mar 2017, Ajay Kaher wrote:
>>Â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().

Verified the code again, I hope my last comments clarifed the things
which came in your mind and helps you to accept the patch :)
Â
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;