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

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

[gregkh]

On Mon, Jan 30, 2017 at 08:25:25AM +0000, Ajay Kaher wrote:
>  

First off, you are sending html email, which the mailing list keeps
rejecting, why are you ignoring that?



> 
> There is race condition when two USB class drivers try to call
> 
> init_usb_class at the same time and leads to crash.
> 
>  
> 
> The main reason for this is one of the Class drivers allocates memory
> for usb_class structure and initializes its member. In the meantime NULL
> check for usb_class structure fails and assumes that usb_class structure
> is properly initialized and crashed while trying to access its members.
> 
>  
> 
> To avoid this race condition locking required before calling
> init_usb_class from function usb_register_dev.
> 
>  
> 
>  
> 
> Signed-off-by: Ajay Kaher

Does this look correct?  Please work with some of the samsung kernel
developers for how to properly submit a patch.

And finally, how are two drivers calling init_usb_class() at the same
time?  What code path causes that?  Have you seen this happen, and if
so, what drivers caused it?

thanks,

greg k-h

RE: Re: Subject: [PATCH v1] 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: 1923 bytes --]


 
At boot time, probe function of multiple connected devices
(proprietary devices) execute simultaneously. And because
of the following code path race condition happens:
probe->usb_register_dev->init_usb_class

Tested with these changes, and problem has been solved.

thanks,
ajay kaher


--------- Original Message ---------
Sender : gregkh@linuxfoundation.org <gregkh@linuxfoundation.org>
Date   : 2017-01-30 14:36 (GMT+5:30)
Title  : Re: Subject: [PATCH v1] USB:Core: BugFix: Proper handling of Race Condition when two USB class drivers try to call init_usb_class simultaneously
 
On Mon, Jan 30, 2017 at 08:25:25AM +0000, Ajay Kaher wrote:
>  
 
First off, you are sending html email, which the mailing list keeps
rejecting, why are you ignoring that?
 
 
 
> 
> There is race condition when two USB class drivers try to call
> 
> init_usb_class at the same time and leads to crash.
> 
>  
> 
> The main reason for this is one of the Class drivers allocates memory
> for usb_class structure and initializes its member. In the meantime NULL
> check for usb_class structure fails and assumes that usb_class structure
> is properly initialized and crashed while trying to access its members.
> 
>  
> 
> To avoid this race condition locking required before calling
> init_usb_class from function usb_register_dev.
> 
>  
> 
>  
> 
> Signed-off-by: Ajay Kaher
 
Does this look correct?  Please work with some of the samsung kernel
developers for how to properly submit a patch.
 
And finally, how are two drivers calling init_usb_class() at the same
time?  What code path causes that?  Have you seen this happen, and if
so, what drivers caused it?
 
thanks,
 
greg k-h
 
 

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

[gregkh]

A: http://en.wikipedia.org/wiki/Top_post
Q: Were do I find info about this thing called top-posting?
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?

A: No.
Q: Should I include quotations after my reply?


http://daringfireball.net/2007/07/on_top

On Tue, Jan 31, 2017 at 05:21:46AM +0000, Ajay Kaher wrote:
> 
>  
> 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?

And what do you mean exactly by "(proprietary devices)"?

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

thanks,

greg k-h

RE: Re: Re: Subject: [PATCH v1] 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: 2941 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.

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

thanks,

ajay kaher


 
--------- Original Message ---------
Sender : gregkh@linuxfoundation.org <gregkh@linuxfoundation.org>
Date   : 2017-01-31 12:31 (GMT+5:30)
Title  : Re: Re: Subject: [PATCH v1] USB:Core: BugFix: Proper handling of Race Condition when two USB class drivers try to call init_usb_class simultaneously
 
A: http://en.wikipedia.org/wiki/Top_post
Q: Were do I find info about this thing called top-posting?
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?
 
A: No.
Q: Should I include quotations after my reply?
 
 
http://daringfireball.net/2007/07/on_top
 
On Tue, Jan 31, 2017 at 05:21:46AM +0000, Ajay Kaher wrote:
> 
>  
> 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?
 
And what do you mean exactly by "(proprietary devices)"?
 
> 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?
 
thanks,
 
greg k-h
 
 
Thanks and Regards,
Ajay Kaher

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

[gregkh]

On Wed, Feb 01, 2017 at 07:24:44AM +0000, Ajay Kaher wrote:
>  
> >> 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?

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?

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?

And what for what drivers specifically?

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

thanks,

greg k-h

RE: Re: Re: Re: Subject: [PATCH v1] 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: 4253 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


Signed-off-by: Ajay Kaher

---

 drivers/usb/core/file.c |    4 ++++
 1 file changed, 4 insertions(+)


diff --git a/drivers/usb/core/file.c b/drivers/usb/core/file.c
index 822ced9..dedc47c 100644
--- a/drivers/usb/core/file.c
+++ b/drivers/usb/core/file.c
@@ -156,6 +156,7 @@ int usb_register_dev(struct usb_interface *intf,
  int minor_base = class_driver->minor_base;
  int minor;
  char name[20];
+ static DEFINE_MUTEX(init_usb_class_mutex);
 
 #ifdef CONFIG_USB_DYNAMIC_MINORS
  /*
@@ -171,7 +172,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: Subject: [PATCH v1] USB:Core: BugFix: Proper handling of Race Condition when two USB class drivers try to call init_usb_class simultaneously

[Alan Stern]

On Thu, 2 Feb 2017, Ajay Kaher wrote:

> >>>> 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
> 
> 
> Signed-off-by: Ajay Kaher

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().  

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.

Alan Stern