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

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

[Ajay Kaher]

[-- Attachment #1: Type: text/plain, Size: 6641 bytes --]


 
> >>>>> At boot time, probe function of multiple connected devices
> >>>>> (proprietary devices) execute simultaneously.
> >>>>
> >>>> What exactly do you mean here?  How can probe happen "simultaneously"?
> >>>> The USB core prevents this, right?
> >>>> 
>> >>> I have observed two scenarios to call probe function:
>> >>> 
>> >>> Scenario #1: Driver inserted and attaching USB Device:
>> >>> Yes, you are right, two probes at same time is not happening
>> >> in this scenario.
>> >> 
>> >> Scenario #2: USB Device attached and inserting Driver:
>> >> In this case probe has been called in context of insmod,
>> >> refer following code flow:
>> >> init -> usb_register_driver -> driver_register -> bus_add_driver ->
>> >> driver_attach -> bus_for_each_dev -> __driver_attach ->
>> >> driver_probe_device -> usb_probe_interface -> probe -> usb_register_dev
>> >> 
>> >> I have observed the crash in Scenario #2, as two probes executes at
>> >> same time in this scenario. And init_usb_class_mutex lock require to
>> >> prevent race condition.
>> > 
>> > What about the fact that in __driver_attach() we call device_lock() so
>> > that probe never gets called at the same time for the same device?
>> 
>> Devices are different.
>> 
>> > Or are you saying that you can load multiple USB modules at the same
>> > time?  If so, how is insmod running on multiple cpus at the same time?
>> > I thought we had a global lock there to prevent that from happening
>> > (i.e. only one module can be loaded at a time.)  Or is that what has
>> > recently changed?
>> 
>> Yes, we can load multiple USB modules at the same time using insmod.
>> Tested on ARM Architecture with Linux kernel 4.1.10, that we can have 
>> multiple insmod on multiple cpus at same time. Also reviewed load_module and 
>> do_init_module functions and couldn't find any global lock.
>> 
>> > 
>> > What is causing your modules to be loaded from userspace?  What type of
>> > device is this happening for?  And why haven't we seen this before?
>> > What kernel versions have you had a problem with this?
>> 
>> Earlier we execute insmod in foreground as "insmod aaa.ko ; insmod bbb.ko"
>> and that's why insmod executed sequentially. Now we modified to execute in 
>> parallel/background as "insmod aaa.ko & ; insmod bbb.ko &". 
>> 
>> > And what for what drivers specifically?
>> 
>> This problem observed for drivers in which usb_register_dev called from their
>> probe function, and there are many such drivers.
>> 
>> As per my opinion, usb_class structure is global and allocated + initialized
>> in usb_register_dev->init_usb_class function. Also as per scenario #2
>> concurrency is possible, so protection using init_usb_class_mutex lock requires.
>> Don't you think so?
>> 
>> >>>> And because of the following code path race condition happens:
>> >>>> probe->usb_register_dev->init_usb_class
>> >>>
>> >>> Why is this just showing up now, and hasn't been an issue for the decade
>> >>> or so this code has been around?  What changed?
>> >>>
>> >>>> Tested with these changes, and problem has been solved.
>> >>>
>> >>> What changes?
>> >> 
>> >> Tested with my patch (i.e. locking with init_usb_class_mutex).
>> > 
>> > I don't see a patch here :(
>> 
>> Sorry, appending the patch again with this mail.
>>  
>> thanks,
>>  
>> ajay kaher
>> 
>> 
>
> On Thu, 14 Feb 2017, Alan Stern wrote:
> 
> I think Ajay's argument is correct and a patch is needed.  But this
> patch misses the race between init_usb_class() and release_usb_class().  

Thanks Alan for your comments, in patch v2 I have taken care for
release_usb_class() also. Please review again.

> The basic problem is that reference counting doesn't work when you try
> to use the same global pointer (usb_class) to refer to multiple
> generations of a dynamically allocated entity.  We had the same sort of
> problem many years ago with the usb_interface structure (and we
> ultimately fixed it by creating a separate usb_interface_cache
> structure).
>  
> The best approach here would be to forget about all the reference
> counting.  Get rid of usb_class entirely, and create the "usbmisc"
> class structure just once, when usbcore initializes.  Or, if you
> prefer, use a mutex to protect a routine that allocates the class
> structure dynamically, just once.  Either way, don't deallocate it
> until usbcore is unloaded.

usbmisc class creation should not require everytime when USB core
initializes. So better to keep usbmisc class creation as it is. 
And to prevent the race conditions just protect it with Mutex locking
as per patch v2.

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..56a151b 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)
 {
@@ -102,9 +103,11 @@ static int init_usb_class(void)
 static void release_usb_class(struct kref *kref)
 {
        /* Ok, we cheat as we know we only have one usb_class */
+       mutex_lock(&init_usb_class_mutex);
        class_destroy(usb_class->class);
        kfree(usb_class);
        usb_class = NULL;
+       mutex_unlock(&init_usb_class_mutex);
 }

 static void destroy_usb_class(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;

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

[Alan Stern]

On Thu, 16 Feb 2017, Ajay Kaher wrote:

> > On Thu, 14 Feb 2017, Alan Stern wrote:
> > 
> > I think Ajay's argument is correct and a patch is needed.  But this
> > patch misses the race between init_usb_class() and release_usb_class().  
> 
> Thanks Alan for your comments, in patch v2 I have taken care for
> release_usb_class() also. Please review again.
> 
> > The basic problem is that reference counting doesn't work when you try
> > to use the same global pointer (usb_class) to refer to multiple
> > generations of a dynamically allocated entity.  We had the same sort of
> > problem many years ago with the usb_interface structure (and we
> > ultimately fixed it by creating a separate usb_interface_cache
> > structure).
> >  
> > The best approach here would be to forget about all the reference
> > counting.  Get rid of usb_class entirely, and create the "usbmisc"
> > class structure just once, when usbcore initializes.  Or, if you
> > prefer, use a mutex to protect a routine that allocates the class
> > structure dynamically, just once.  Either way, don't deallocate it
> > until usbcore is unloaded.
> 
> usbmisc class creation should not require everytime when USB core
> initializes. So better to keep usbmisc class creation as it is. 
> And to prevent the race conditions just protect it with Mutex locking
> as per patch v2.
> 
> 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..56a151b 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)
>  {
> @@ -102,9 +103,11 @@ static int init_usb_class(void)
>  static void release_usb_class(struct kref *kref)
>  {
>         /* Ok, we cheat as we know we only have one usb_class */
> +       mutex_lock(&init_usb_class_mutex);
>         class_destroy(usb_class->class);
>         kfree(usb_class);
>         usb_class = NULL;
> +       mutex_unlock(&init_usb_class_mutex);
>  }
> 
>  static void destroy_usb_class(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;

This is not right.  What happens if usb_register_dev() runs just before
release_usb_class() calls mutex_lock()?

Alan Stern