[PATCH v4] USB: Proper handling of Race Condition when two USB class drivers try to call init_usb_class simultaneously
Greg, sending patch again using git send-email, please apply.
Let me know if still any issue.
There is race condition when two USB class drivers try to call
init_usb_class at the same time and leads to crash.
code path: probe->usb_register_dev->init_usb_class
To solve this, mutex locking has been added in init_usb_class() and
destroy_usb_class().
As pointed by Alan, removed "if (usb_class)" test from destroy_usb_class()
because usb_class can never be NULL there.
Signed-off-by: Ajay Kaher <ajay.ka...@samsung.com>
Acked-by: Alan Stern <st...@rowland.harvard.edu>
---
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(_class->kref, release_usb_class);
+ mutex_lock(_usb_class_mutex);
+ kref_put(_class->kref, release_usb_class);
+ mutex_unlock(_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(_usb_class_mutex);
retval = init_usb_class();
+ mutex_unlock(_usb_class_mutex);
+
if (retval)
return retval;
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
FW: Re: [PATCH v4] USB: Proper handling of Race Condition when two USB class drivers try to call init_usb_class simultaneously
Greg, hope you had not faced any issue (tab converted to spaces) with this
patch.
In case still facing any issue please let me know.
> There is race condition when two USB class drivers try to call
> init_usb_class at the same time and leads to crash.
> code path: probe->usb_register_dev->init_usb_class
>
> To solve this, mutex locking has been added in init_usb_class() and
> destroy_usb_class().
>
> As pointed by Alan, removed "if (usb_class)" test from destroy_usb_class()
> because usb_class can never be NULL there.
Signed-off-by: Ajay Kaher <ajay.ka...@samsung.com>
Acked-by: Alan Stern <st...@rowland.harvard.edu>
---
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(_class->kref, release_usb_class);
+ mutex_lock(_usb_class_mutex);
+ kref_put(_class->kref, release_usb_class);
+ mutex_unlock(_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(_usb_class_mutex);
retval = init_usb_class();
+ mutex_unlock(_usb_class_mutex);
+
if (retval)
return retval;
--
1.7.9.5
Re: [PATCH v4] USB: Proper handling of Race Condition when two USB class drivers try to call init_usb_class simultaneously
There is race condition when two USB class drivers try to call
init_usb_class at the same time and leads to crash.
code path: probe->usb_register_dev->init_usb_class
To solve this, mutex locking has been added in init_usb_class() and
destroy_usb_class().
As pointed by Alan, removed "if (usb_class)" test from destroy_usb_class()
because usb_class can never be NULL there.
Signed-off-by: Ajay Kaher <ajay.ka...@samsung.com>
Acked-by: Alan Stern <st...@rowland.harvard.edu>
---
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(_class->kref, release_usb_class);
+ mutex_lock(_usb_class_mutex);
+ kref_put(_class->kref, release_usb_class);
+ mutex_unlock(_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(_usb_class_mutex);
retval = init_usb_class();
+ mutex_unlock(_usb_class_mutex);
+
if (retval)
return retval;
--
1.7.9.5
Re: Subject: [PATCH v4] USB:Core: BugFix: Proper handling of Race Condition when two USB class drivers try to call init_usb_class simultaneously
From febeb10887d5026a489658fd9e911656e76038ac Mon Sep 17 00:00:00 2001
From: Ajay Kaher <ajay.ka...@samsung.com>
Date: Thu, 9 Mar 2017 16:07:54 +0530
Subject: [PATCH v4] USB:Core: BugFix: Proper handling of Race Condition when two
USB class drivers try to call init_usb_class simultaneously
There is race condition when two USB class drivers try to call
init_usb_class at the same time and leads to crash.
code path: probe->usb_register_dev->init_usb_class
To solve this, mutex locking has been added in init_usb_class() and
destroy_usb_class().
As pointed by Alan, removed "if (usb_class)" test from destroy_usb_class()
because usb_class can never be NULL there.
Signed-off-by: Ajay Kaher <ajay.ka...@samsung.com>
Acked-by: Alan Stern <st...@rowland.harvard.edu>
---
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(_class->kref, release_usb_class);
+ mutex_lock(_usb_class_mutex);
+ kref_put(_class->kref, release_usb_class);
+ mutex_unlock(_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(_usb_class_mutex);
retval = init_usb_class();
+ mutex_unlock(_usb_class_mutex);
+
if (retval)
return retval;
--
1.7.9.5
Re: Subject: [PATCH v4] USB:Core: BugFix: Proper handling of Race Condition when two USB class drivers try to call init_usb_class simultaneously
> 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(_class->kref, release_usb_class);
+ mutex_lock(_usb_class_mutex);
+ kref_put(_class->kref, release_usb_class);
+ mutex_unlock(_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(_usb_class_mutex);
retval = init_usb_class();
+ mutex_unlock(_usb_class_mutex);
+
if (retval)
return retval;
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
> On Thr, 2 Mar 2017, Ajay Kaher wrote:
>> On Wed, 1 Mar 2017, Alan Stern wrote:
>>> On Wed, 1 Mar 2017, Ajay Kaher wrote:
>>>> On Mon, 22 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().
>
> 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 :)
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(_usb_class_mutex);
if (usb_class)
kref_put(_class->kref, release_usb_class);
+ mutex_unlock(_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(_usb_class_mutex);
retval = init_usb_class();
+ mutex_unlock(_usb_class_mutex);
+
if (retval)
return retval;
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
> On Wed, 1 Mar 2017, Alan Stern wrote:
>> On Wed, 1 Mar 2017, Ajay Kaher wrote:
>>> On Mon, 22 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().
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().
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(_usb_class_mutex);
if (usb_class)
kref_put(_class->kref, release_usb_class);
+ mutex_unlock(_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(_usb_class_mutex);
retval = init_usb_class();
+ mutex_unlock(_usb_class_mutex);
+
if (retval)
return retval;
Thanks and Regards,
Ajay Kaher
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
> 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.
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(_usb_class_mutex);
if (usb_class)
kref_put(_class->kref, release_usb_class);
+ mutex_unlock(_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(_usb_class_mutex);
retval = init_usb_class();
+ mutex_unlock(_usb_class_mutex);
+
if (retval)
return retval;
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
On Tue, 21 Feb 2017, Alan Stern 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.
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(_usb_class_mutex);
> if (usb_class)
> kref_put(_class->kref, release_usb_class);
> + mutex_unlock(_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(_usb_class_mutex);
> retval = init_usb_class();
> + mutex_unlock(_usb_class_mutex);
> +
> if (retval)
> return retval;
Re: Subject: [PATCH v3] USB:Core: BugFix: Proper handling of Race Condition when two USB class drivers try to call init_usb_class simultaneously
On Thu, 16 Feb 2017, Alan Stern wrote:
> 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.
>
> This is not right. What happens if usb_register_dev() runs just before
> release_usb_class() calls mutex_lock()?
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.
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(_usb_class_mutex);
if (usb_class)
kref_put(_class->kref, release_usb_class);
+ mutex_unlock(_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(_usb_class_mutex);
retval = init_usb_class();
+ mutex_unlock(_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
> >>>>> 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 ri
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
>> 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 : gre...@linuxfoundation.org <gre...@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 +, 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: Subject: [PATCH v1] USB:Core: BugFix: Proper handling of Race Condition when two USB class drivers try to call init_usb_class simultaneously
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 : gre...@linuxfoundation.org <gre...@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 +, 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
