* 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 [not found] <CGME20170220192040epcas3p37ded8ece49ed9385df80d3b813a953bc@epcas3p3.samsung.com> @ 2017-02-22 12:57 ` Ajay Kaher 2017-03-01 11:12 ` FW: " Ajay Kaher 0 siblings, 1 reply; 14+ messages in thread From: Ajay Kaher @ 2017-02-22 12:57 UTC (permalink / raw) To: Alan Stern Cc: gregkh@linuxfoundation.org, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, AMAN DEEP, HEMANSHU SRIVASTAVA [-- 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; ^ permalink raw reply [flat|nested] 14+ messages in thread
* 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 2017-02-22 12:57 ` 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 @ 2017-03-01 11:12 ` Ajay Kaher 2017-03-01 15:15 ` Alan Stern 0 siblings, 1 reply; 14+ messages in thread From: Ajay Kaher @ 2017-03-01 11:12 UTC (permalink / raw) To: Alan Stern Cc: gregkh@linuxfoundation.org, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, AMAN DEEP, HEMANSHU SRIVASTAVA [-- 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; ^ permalink raw reply related [flat|nested] 14+ messages in thread
* 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 2017-03-01 11:12 ` FW: " Ajay Kaher @ 2017-03-01 15:15 ` Alan Stern [not found] ` <CGME20170301151552epcas5p3ecf8f7dc55323b2ceaff8c8a9b9644c4@epcms5p1> 0 siblings, 1 reply; 14+ messages in thread From: Alan Stern @ 2017-03-01 15:15 UTC (permalink / raw) To: Ajay Kaher Cc: gregkh@linuxfoundation.org, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, AMAN DEEP, HEMANSHU SRIVASTAVA 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; > > > ^ permalink raw reply [flat|nested] 14+ messages in thread
[parent not found: <CGME20170301151552epcas5p3ecf8f7dc55323b2ceaff8c8a9b9644c4@epcms5p1>]
[parent not found: <20170302074430epcms5p342df59b8e9a4ecc72f4789ed07fc2f21@epcms5p3>]
* 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 [not found] ` <20170302074430epcms5p342df59b8e9a4ecc72f4789ed07fc2f21@epcms5p3> @ 2017-03-02 7:49 ` Ajay Kaher 2017-03-03 14:49 ` FW: " Ajay Kaher 0 siblings, 1 reply; 14+ messages in thread From: Ajay Kaher @ 2017-03-02 7:49 UTC (permalink / raw) To: Alan Stern, gregkh@linuxfoundation.org Cc: linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, AMAN DEEP, HEMANSHU SRIVASTAVA [-- 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* 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 2017-03-02 7:49 ` FW: RE: " Ajay Kaher @ 2017-03-03 14:49 ` Ajay Kaher 2017-03-03 16:53 ` Alan Stern 0 siblings, 1 reply; 14+ messages in thread From: Ajay Kaher @ 2017-03-03 14:49 UTC (permalink / raw) To: Alan Stern, gregkh@linuxfoundation.org Cc: linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, AMAN DEEP, HEMANSHU SRIVASTAVA [-- 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; ^ permalink raw reply [flat|nested] 14+ messages in thread
* 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 2017-03-03 14:49 ` FW: " Ajay Kaher @ 2017-03-03 16:53 ` Alan Stern 2017-03-07 4:35 ` Subject: [PATCH v4] " Ajay Kaher 0 siblings, 1 reply; 14+ messages in thread From: Alan Stern @ 2017-03-03 16:53 UTC (permalink / raw) To: Ajay Kaher Cc: gregkh@linuxfoundation.org, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, AMAN DEEP, HEMANSHU SRIVASTAVA 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; > ^ permalink raw reply [flat|nested] 14+ 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-03 16:53 ` Alan Stern @ 2017-03-07 4:35 ` Ajay Kaher 2017-03-07 15:00 ` Alan Stern 2017-03-08 18:05 ` Subject: [PATCH v4] USB:Core: BugFix: " gregkh 0 siblings, 2 replies; 14+ messages in thread From: Ajay Kaher @ 2017-03-07 4:35 UTC (permalink / raw) To: Alan Stern, gregkh@linuxfoundation.org Cc: linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, AMAN DEEP, HEMANSHU SRIVASTAVA [-- 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; ^ permalink raw reply related [flat|nested] 14+ 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 4:35 ` Subject: [PATCH v4] " Ajay Kaher @ 2017-03-07 15:00 ` Alan Stern 2017-03-09 11:34 ` Ajay Kaher 2017-03-08 18:05 ` Subject: [PATCH v4] USB:Core: BugFix: " gregkh 1 sibling, 1 reply; 14+ messages in thread From: Alan Stern @ 2017-03-07 15:00 UTC (permalink / raw) To: Ajay Kaher Cc: gregkh@linuxfoundation.org, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, 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] 14+ 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 2017-03-09 12:10 ` gregkh 0 siblings, 1 reply; 14+ messages in thread From: Ajay Kaher @ 2017-03-09 11:34 UTC (permalink / raw) To: gregkh@linuxfoundation.org Cc: Alan Stern, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, 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] 14+ 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-17 10:56 ` [PATCH v4] USB: " Ajay Kaher 0 siblings, 1 reply; 14+ messages in thread From: gregkh @ 2017-03-09 12:10 UTC (permalink / raw) To: Ajay Kaher Cc: Alan Stern, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, 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] 14+ 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-09 12:10 ` gregkh @ 2017-03-17 10:56 ` Ajay Kaher 2017-03-21 14:49 ` FW: " Ajay Kaher 2017-03-23 7:18 ` gregkh 0 siblings, 2 replies; 14+ messages in thread From: Ajay Kaher @ 2017-03-17 10:56 UTC (permalink / raw) To: gregkh@linuxfoundation.org Cc: Alan Stern, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, AMAN DEEP, HEMANSHU SRIVASTAVA [-- 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* FW: Re: [PATCH v4] USB: Proper handling of Race Condition when two USB class drivers try to call init_usb_class simultaneously 2017-03-17 10:56 ` [PATCH v4] USB: " Ajay Kaher @ 2017-03-21 14:49 ` Ajay Kaher 2017-03-23 7:18 ` gregkh 1 sibling, 0 replies; 14+ messages in thread From: Ajay Kaher @ 2017-03-21 14:49 UTC (permalink / raw) To: gregkh@linuxfoundation.org Cc: Alan Stern, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, AMAN DEEP, HEMANSHU SRIVASTAVA [-- 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 ^ permalink raw reply related [flat|nested] 14+ 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-17 10:56 ` [PATCH v4] USB: " Ajay Kaher 2017-03-21 14:49 ` FW: " Ajay Kaher @ 2017-03-23 7:18 ` gregkh 1 sibling, 0 replies; 14+ messages in thread From: gregkh @ 2017-03-23 7:18 UTC (permalink / raw) To: Ajay Kaher Cc: Alan Stern, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, AMAN DEEP, HEMANSHU SRIVASTAVA 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 ^ permalink raw reply [flat|nested] 14+ 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 4:35 ` Subject: [PATCH v4] " Ajay Kaher 2017-03-07 15:00 ` Alan Stern @ 2017-03-08 18:05 ` gregkh 1 sibling, 0 replies; 14+ messages in thread From: gregkh @ 2017-03-08 18:05 UTC (permalink / raw) To: Ajay Kaher Cc: Alan Stern, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, AMAN DEEP, HEMANSHU SRIVASTAVA 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2017-03-23 7:37 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <CGME20170220192040epcas3p37ded8ece49ed9385df80d3b813a953bc@epcas3p3.samsung.com>
2017-02-22 12:57 ` 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
2017-03-01 11:12 ` FW: " Ajay Kaher
2017-03-01 15:15 ` Alan Stern
[not found] ` <CGME20170301151552epcas5p3ecf8f7dc55323b2ceaff8c8a9b9644c4@epcms5p1>
[not found] ` <20170302074430epcms5p342df59b8e9a4ecc72f4789ed07fc2f21@epcms5p3>
2017-03-02 7:49 ` FW: RE: " Ajay Kaher
2017-03-03 14:49 ` FW: " Ajay Kaher
2017-03-03 16:53 ` Alan Stern
2017-03-07 4:35 ` Subject: [PATCH v4] " Ajay Kaher
2017-03-07 15:00 ` Alan Stern
2017-03-09 11:34 ` Ajay Kaher
2017-03-09 12:10 ` gregkh
2017-03-17 10:56 ` [PATCH v4] USB: " Ajay Kaher
2017-03-21 14:49 ` FW: " Ajay Kaher
2017-03-23 7:18 ` gregkh
2017-03-08 18:05 ` Subject: [PATCH v4] USB:Core: BugFix: " gregkh
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.