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

[Ajay Kaher]

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

At many places in Linux kernel, instances of Kref have been used within
Mutex, SpinLock and donât have any side effect.

Making to int and handle (i.e. get/put) it within file.c seems
not good as we have Kref. Instead, we can have non_atomic version of kref.
We can discuss about non_atomic kref in another thread, if you are interested.

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

Removed in Patch v4.

thanks,
ajayÂkaher
Â
ÂÂ
Signed-off-by:ÂAjayÂKaher
Â
---

 drivers/usb/core/file.c |    9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/core/file.c b/drivers/usb/core/file.c
index 822ced9..422ce7b 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,9 @@ static void release_usb_class(struct kref *kref)

 static void destroy_usb_class(void)
 {
-       if (usb_class)
-               kref_put(&usb_class->kref, release_usb_class);
+       mutex_lock(&init_usb_class_mutex);
+       kref_put(&usb_class->kref, release_usb_class);
+       mutex_unlock(&init_usb_class_mutex);
 }

 int usb_major_init(void)
@@ -171,7 +173,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;