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 Wed, 1 Mar 2017, Ajay Kaher wrote:

> > On Mon, 22 Feb 2017, Ajay Kaher wrote:
> >Â
> >>ÂOnÂMon,Â20Â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().

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