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
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
Acked-by: Alan Stern
---
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
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 Thu, Mar 09, 2017 at 11:34:25AM +, Ajay Kaher wrote:
> From febeb10887d5026a489658fd9e911656e76038ac Mon Sep 17 00:00:00 2001
> From: Ajay Kaher
> 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
> Acked-by: Alan Stern
> ---
> 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
--
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
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 Tue, Mar 07, 2017 at 04:35:37AM +, 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. > > > 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 > Can you resend this in a format that I can apply it in? I suggest reading Documentation/SubmittingPatches. If you have any questions about the correct format, please let me know. Also add Alan's ack to it. thanks, greg k-h -- 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
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 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
Alan Stern
--
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
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(&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;
