* [PATCH v4] USB: Proper handling of Race Condition when two USB class drivers try to call init_usb_class simultaneously
[not found] <CGME20170328121013epcas5p493f1509064350349fbcdb655793d8d4e@epcas5p4.samsung.com>
@ 2017-03-28 12:09 ` Ajay Kaher
2017-03-29 9:56 ` Greg KH
0 siblings, 1 reply; 4+ messages in thread
From: Ajay Kaher @ 2017-03-28 12:09 UTC (permalink / raw)
To: gregkh; +Cc: stern, linux-usb, linux-kernel, aman.deep, hemanshu.s, ajay.kaher
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.kaher@samsung.com>
Acked-by: Alan Stern <stern@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(&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;
--
2.7.4
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH v4] USB: Proper handling of Race Condition when two USB class drivers try to call init_usb_class simultaneously
2017-03-28 12:09 ` [PATCH v4] USB: Proper handling of Race Condition when two USB class drivers try to call init_usb_class simultaneously Ajay Kaher
@ 2017-03-29 9:56 ` Greg KH
0 siblings, 0 replies; 4+ messages in thread
From: Greg KH @ 2017-03-29 9:56 UTC (permalink / raw)
To: Ajay Kaher; +Cc: stern, linux-usb, linux-kernel, aman.deep, hemanshu.s
On Tue, Mar 28, 2017 at 08:09:32AM -0400, Ajay Kaher wrote:
> Greg, sending patch again using git send-email, please apply.
> Let me know if still any issue.
Note, please put comments like this below the --- line so I don't have
to manually edit the file by hand.
I've now queued this up, thanks.
greg k-h
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: Subject: [PATCH v4] USB:Core: BugFix: Proper handling of Race Condition when two USB class drivers try to call init_usb_class simultaneously
2017-03-09 11:34 ` Ajay Kaher
@ 2017-03-09 12:10 gregkh
2017-03-07 15:00 ` Alan Stern
0 siblings, 1 reply; 4+ messages in thread
From: gregkh @ 2017-03-09 12:10 UTC (permalink / raw)
To: Ajay Kaher
Cc: Alan Stern, linux-usb, linux-kernel, AMAN DEEP, HEMANSHU SRIVASTAVA
On Thu, Mar 09, 2017 at 11:34:25AM +0000, Ajay Kaher wrote:
> From febeb10887d5026a489658fd9e911656e76038ac Mon Sep 17 00:00:00 2001
> From: Ajay Kaher <ajay.kaher@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
Why is your subject line have the word "subject" in it?
Please fix your email client so you don't have the whole git commit
header in the body of the email like you do here.
Also, no need to say "Core:" or "BugFix:"
>
> 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.kaher@samsung.com>
> Acked-by: Alan Stern <stern@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(&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;
>
All tabs were turned into spaces and this patch can not be applied :(
Please fix up and try again. Send a patch to yourself first to see if
it works properly before sending it to us.
greg k-h
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: Subject: [PATCH v4] USB:Core: BugFix: Proper handling of Race Condition when two USB class drivers try to call init_usb_class simultaneously
@ 2017-03-07 15:00 ` Alan Stern
2017-03-09 11:34 ` Ajay Kaher
0 siblings, 1 reply; 4+ messages in thread
From: Alan Stern @ 2017-03-07 15:00 UTC (permalink / raw)
To: Ajay Kaher
Cc: gregkh, linux-usb, linux-kernel, AMAN DEEP, HEMANSHU SRIVASTAVA
On Tue, 7 Mar 2017, Ajay Kaher wrote:
> > 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.
Okay.
> > 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;
Acked-by: Alan Stern <stern@rowland.harvard.edu>
Alan Stern
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: Subject: [PATCH v4] USB:Core: BugFix: Proper handling of Race Condition when two USB class drivers try to call init_usb_class simultaneously
@ 2017-03-09 11:34 ` Ajay Kaher
[not found] ` <CGME20170307150103epcas2p348320fdfbb176978d084d8f2b7b9a049@epcms5p2>
0 siblings, 1 reply; 4+ messages in thread
From: Ajay Kaher @ 2017-03-09 11:34 UTC (permalink / raw)
To: gregkh
Cc: Alan Stern, linux-usb, linux-kernel, AMAN DEEP, HEMANSHU SRIVASTAVA
[-- Attachment #1: Type: text/plain, Size: 1937 bytes --]
From febeb10887d5026a489658fd9e911656e76038ac Mon Sep 17 00:00:00 2001
From: Ajay Kaher <ajay.kaher@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.kaher@samsung.com>
Acked-by: Alan Stern <stern@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(&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;
--
1.7.9.5
^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-03-29 9:56 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <CGME20170328121013epcas5p493f1509064350349fbcdb655793d8d4e@epcas5p4.samsung.com>
2017-03-28 12:09 ` [PATCH v4] USB: Proper handling of Race Condition when two USB class drivers try to call init_usb_class simultaneously Ajay Kaher
2017-03-29 9:56 ` Greg KH
2017-03-09 12:10 Subject: [PATCH v4] USB:Core: BugFix: " gregkh
2017-03-07 15:00 ` Alan Stern
2017-03-09 11:34 ` Ajay Kaher
[not found] ` <CGME20170307150103epcas2p348320fdfbb176978d084d8f2b7b9a049@epcms5p2>
2017-03-17 10:56 ` [PATCH v4] USB: " Ajay Kaher
2017-03-23 7:18 ` gregkh
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).