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

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

[Ajay Kaher]

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

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(&init_usb_class_mutex);
>         if (usb_class)
>                 kref_put(&usb_class->kref, release_usb_class);
> +       mutex_unlock(&init_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(&init_usb_class_mutex);
>         retval = init_usb_class();
> +       mutex_unlock(&init_usb_class_mutex);
> +
>         if (retval)
>                 return retval;
 
 
 
 
 
 
 

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

[Ajay Kaher]

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


> 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(&init_usb_class_mutex);
        if (usb_class)
                kref_put(&usb_class->kref, release_usb_class);
+       mutex_unlock(&init_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(&init_usb_class_mutex);
        retval = init_usb_class();
+       mutex_unlock(&init_usb_class_mutex);
+
        if (retval)
                return retval;

 

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

[Alan Stern]

On Wed, 1 Mar 2017, Ajay Kaher wrote:

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

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

Alan Stern

> 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(&init_usb_class_mutex);
>         if (usb_class)
>                 kref_put(&usb_class->kref, release_usb_class);
> +       mutex_unlock(&init_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(&init_usb_class_mutex);
>         retval = init_usb_class();
> +       mutex_unlock(&init_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

[Ajay Kaher]

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


> 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(&init_usb_class_mutex);
        if (usb_class)
                kref_put(&usb_class->kref, release_usb_class);
+       mutex_unlock(&init_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(&init_usb_class_mutex);
        retval = init_usb_class();
+       mutex_unlock(&init_usb_class_mutex);
+
        if (retval)
                return retval;
 
 
 
Thanks and Regards,
Ajay Kaher

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

[Ajay Kaher]

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


> 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(&init_usb_class_mutex);
        if (usb_class)
                kref_put(&usb_class->kref, release_usb_class);
+       mutex_unlock(&init_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(&init_usb_class_mutex);
        retval = init_usb_class();
+       mutex_unlock(&init_usb_class_mutex);
+
        if (retval)
                return retval;

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

[Alan Stern]

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.

Also, why does destroy_usb_class() have that "if (usb_class) "test?  
Isn't it true that usb_class can never be NULL there?

Alan Stern

> 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(&init_usb_class_mutex);
>         if (usb_class)
>                 kref_put(&usb_class->kref, release_usb_class);
> +       mutex_unlock(&init_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(&init_usb_class_mutex);
>         retval = init_usb_class();
> +       mutex_unlock(&init_usb_class_mutex);
> +
>         if (retval)
>                 return retval;
> 

Re: Subject: [PATCH v4] 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: 2850 bytes --]

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

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

[Alan Stern]

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

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

[gregkh]

On Tue, Mar 07, 2017 at 04:35:37AM +0000, 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

Re: Subject: [PATCH v4] 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: 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

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

[gregkh]

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

Re: [PATCH v4] USB: 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: 1643 bytes --]

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

FW: Re: [PATCH v4] USB: 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: 1798 bytes --]


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

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

[gregkh]

On Fri, Mar 17, 2017 at 10:56:37AM +0000, Ajay Kaher wrote:
> 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;
>  

I get the following errors when trying to apply this patch:

Applying: USB: Proper handling of Race Condition when two USB class drivers try to call init_usb_class simultaneously
.git/rebase-apply/patch:13: trailing whitespace.
static DEFINE_MUTEX(init_usb_class_mutex);
.git/rebase-apply/patch:23: trailing whitespace.
        mutex_lock(&init_usb_class_mutex);
.git/rebase-apply/patch:24: trailing whitespace.
        kref_put(&usb_class->kref, release_usb_class);
.git/rebase-apply/patch:25: trailing whitespace.
        mutex_unlock(&init_usb_class_mutex);
.git/rebase-apply/patch:33: trailing whitespace.
        mutex_lock(&init_usb_class_mutex);
error: patch failed: drivers/usb/core/file.c:27
error: drivers/usb/core/file.c: patch does not apply
Patch failed at 0001 USB: Proper handling of Race Condition when two USB class drivers try to call init_usb_class simultaneously


Are you sure you made this in the correct format?  Seems that the patch
has dos line-ends :(

Please fix up and resend.

thanks,

greg k-h

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

[Greg KH]

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

[PATCH v4] USB: Proper handling of Race Condition when two USB class drivers try to call init_usb_class simultaneously

[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