Re: 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

[Alan Stern]

On Fri, 3 Mar 2017, Ajay Kaher wrote:

> > 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 :)

Your main point is that usb_class->kref is accessed from only two
points, both of which are protected by the new mutex.  This means there
is no reason for the value to be a struct kref at all.  You should
change it to an int (and change its name).  Leaving it as a kref will
make readers wonder why it needs to be updated atomically.

Also, why does destroy_usb_class() have that "ifÂ(usb_class) "test?  
Isn't it true that usb_class can never be NULL there?

Alan Stern

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