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
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(_usb_class_mutex);
> if (usb_class)
> kref_put(_class->kref, release_usb_class);
> + mutex_unlock(_usb_class_mutex);
> }
>
> int usb_major_init(void)
> @@ -171,7 +174,10 @@ int usb_register_dev(struct usb_interface *intf,
> if (intf->minor >= 0)
> return -EADDRINUSE;
>
> + mutex_lock(_usb_class_mutex);
> retval = init_usb_class();
> + mutex_unlock(_usb_class_mutex);
> +
> if (retval)
> return retval;
>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
FW: FW: RE: Re: FW: RE: Re: Subject: [PATCH v3] USB:Core: BugFix: Proper handling of Race Condition when two USB class drivers try to call init_usb_class simultaneously
> On Thr, 2 Mar 2017, Ajay Kaher wrote:
>> On Wed, 1 Mar 2017, Alan Stern wrote:
>>> On Wed, 1 Mar 2017, Ajay Kaher wrote:
On Mon, 22 Feb 2017, Ajay Kaher wrote:
>
>> Alan, as per my understanding I have shifted the lock from
>> release_usb_class() to destroy_usb_class() in patch v3.
>> If it is not right, please explain in detail which race condition
>> I have missed and also share your suggestions.
>>
>
> Have you considered what would happen if destroy_usb_class() ran, but
> some other CPU was still holding a reference to usb_class? And what if
> the last reference gets dropped later on, while init_usb_class() is
> running?
Access of usb_class->kref is only from either init_usb_class()
or destroy_usb_class(), and both these functions are now protected
with Mutex Locking in patch v3, so there is no chance of race condition
as per above scenarios.
> Maybe that's not possible here, but it is possible in general for
> refcounted objects. So yes, this code is probably okay, but it isn't
> good form.
As per my understanding, I found to be one of the best possible solution
for this problem and this solutiuon don't have any side effect.
>>>
>>> Alan, I had shared modified Patch v3 as per your inputs to prevent
>>> the race condition during simultaneously calling of init_usb_class().
>>> If you think there is scope to improve the patch, please share your inputs.
>>
>> Under the circumstances, your patch is acceptable.
>>
>> If you really want to make the point crystal clear, you could replace
>> usb_class->kref with an ordinary integer counter. Then it would be
>> obvious that there are no references other than the ones taken by
>> init_usb_class() and released by destroy_usb_class().
>
> usb_class->kref is not accessible outside the file.c
> as usb_class is _static_ inside the file.c and
> pointer of usb_class->kref is not passed anywhere.
>
> Hence as you wanted, there are no references of usb_class->kref
> other than taken by init_usb_class() and released by destroy_usb_class().
Verified the code again, I hope my last comments clarifed the things
which came in your mind and helps you to accept the patch :)
thanks,
ajay kaher
Signed-off-by: Ajay Kaher
---
drivers/usb/core/file.c |6 ++
1 file changed, 6 insertions(+)
diff --git a/drivers/usb/core/file.c b/drivers/usb/core/file.c
index 822ced9..a12d184 100644
--- a/drivers/usb/core/file.c
+++ b/drivers/usb/core/file.c
@@ -27,6 +27,7 @@
#define MAX_USB_MINORS 256
static const struct file_operations *usb_minors[MAX_USB_MINORS];
static DECLARE_RWSEM(minor_rwsem);
+static DEFINE_MUTEX(init_usb_class_mutex);
static int usb_open(struct inode *inode, struct file *file)
{
@@ -109,8 +110,10 @@ static void release_usb_class(struct kref *kref)
static void destroy_usb_class(void)
{
+ mutex_lock(_usb_class_mutex);
if (usb_class)
kref_put(_class->kref, release_usb_class);
+ mutex_unlock(_usb_class_mutex);
}
int usb_major_init(void)
@@ -171,7 +174,10 @@ int usb_register_dev(struct usb_interface *intf,
if (intf->minor >= 0)
return -EADDRINUSE;
+ mutex_lock(_usb_class_mutex);
retval = init_usb_class();
+ mutex_unlock(_usb_class_mutex);
+
if (retval)
return retval;
FW: RE: Re: FW: RE: Re: Subject: [PATCH v3] USB:Core: BugFix: Proper handling of Race Condition when two USB class drivers try to call init_usb_class simultaneously
> On Wed, 1 Mar 2017, Alan Stern wrote:
>> On Wed, 1 Mar 2017, Ajay Kaher wrote:
>>> On Mon, 22 Feb 2017, Ajay Kaher wrote:
>>>
> Alan, as per my understanding I have shifted the lock from
> release_usb_class() to destroy_usb_class() in patch v3.
> If it is not right, please explain in detail which race condition
> I have missed and also share your suggestions.
>
Have you considered what would happen if destroy_usb_class() ran, but
some other CPU was still holding a reference to usb_class? And what if
the last reference gets dropped later on, while init_usb_class() is
running?
>>>
>>> Access of usb_class->kref is only from either init_usb_class()
>>> or destroy_usb_class(), and both these functions are now protected
>>> with Mutex Locking in patch v3, so there is no chance of race condition
>>> as per above scenarios.
>>>
Maybe that's not possible here, but it is possible in general for
refcounted objects. So yes, this code is probably okay, but it isn't
good form.
>>>
>>> As per my understanding, I found to be one of the best possible solution
>>> for this problem and this solutiuon don't have any side effect.
>>
>> Alan, I had shared modified Patch v3 as per your inputs to prevent
>> the race condition during simultaneously calling of init_usb_class().
>> If you think there is scope to improve the patch, please share your inputs.
>
> Under the circumstances, your patch is acceptable.
>
> If you really want to make the point crystal clear, you could replace
> usb_class->kref with an ordinary integer counter. Then it would be
> obvious that there are no references other than the ones taken by
> init_usb_class() and released by destroy_usb_class().
usb_class->kref is not accessible outside the file.c
as usb_class is _static_ inside the file.c and
pointer of usb_class->kref is not passed anywhere.
Hence as you wanted, there are no references of usb_class->kref
other than taken by init_usb_class() and released by destroy_usb_class().
thanks,
ajay kaher
Signed-off-by: Ajay Kaher
---
drivers/usb/core/file.c |6 ++
1 file changed, 6 insertions(+)
diff --git a/drivers/usb/core/file.c b/drivers/usb/core/file.c
index 822ced9..a12d184 100644
--- a/drivers/usb/core/file.c
+++ b/drivers/usb/core/file.c
@@ -27,6 +27,7 @@
#define MAX_USB_MINORS 256
static const struct file_operations *usb_minors[MAX_USB_MINORS];
static DECLARE_RWSEM(minor_rwsem);
+static DEFINE_MUTEX(init_usb_class_mutex);
static int usb_open(struct inode *inode, struct file *file)
{
@@ -109,8 +110,10 @@ static void release_usb_class(struct kref *kref)
static void destroy_usb_class(void)
{
+ mutex_lock(_usb_class_mutex);
if (usb_class)
kref_put(_class->kref, release_usb_class);
+ mutex_unlock(_usb_class_mutex);
}
int usb_major_init(void)
@@ -171,7 +174,10 @@ int usb_register_dev(struct usb_interface *intf,
if (intf->minor >= 0)
return -EADDRINUSE;
+ mutex_lock(_usb_class_mutex);
retval = init_usb_class();
+ mutex_unlock(_usb_class_mutex);
+
if (retval)
return retval;
Thanks and Regards,
Ajay Kaher
Re: FW: RE: Re: Subject: [PATCH v3] USB:Core: BugFix: Proper handling of Race Condition when two USB class drivers try to call init_usb_class simultaneously
On Wed, 1 Mar 2017, 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(_usb_class_mutex);
> if (usb_class)
> kref_put(_class->kref, release_usb_class);
> + mutex_unlock(_usb_class_mutex);
> }
>
> int usb_major_init(void)
> @@ -171,7 +174,10 @@ int usb_register_dev(struct usb_interface *intf,
> if (intf->minor >= 0)
> return -EADDRINUSE;
>
> + mutex_lock(_usb_class_mutex);
> retval = init_usb_class();
> + mutex_unlock(_usb_class_mutex);
> +
> if (retval)
> return retval;
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
FW: RE: Re: Subject: [PATCH v3] USB:Core: BugFix: Proper handling of Race Condition when two USB class drivers try to call init_usb_class simultaneously
> On Mon, 22 Feb 2017, Ajay Kaher wrote:
>
>> On Mon, 20 Feb 2017, Ajay Kaher wrote:
>>
>>> Alan, as per my understanding I have shifted the lock from
>>> release_usb_class() to destroy_usb_class() in patch v3.
>>> If it is not right, please explain in detail which race condition
>>> I have missed and also share your suggestions.
>>>
>>
>> Have you considered what would happen if destroy_usb_class() ran, but
>> some other CPU was still holding a reference to usb_class? And what if
>> the last reference gets dropped later on, while init_usb_class() is
>> running?
>
> Access of usb_class->kref is only from either init_usb_class()
> or destroy_usb_class(), and both these functions are now protected
> with Mutex Locking in patch v3, so there is no chance of race condition
> as per above scenarios.
>
>> Maybe that's not possible here, but it is possible in general for
>> refcounted objects. So yes, this code is probably okay, but it isn't
>> good form.
>
> As per my understanding, I found to be one of the best possible solution
> for this problem and this solutiuon don't have any side effect.
Alan, I had shared modified Patch v3 as per your inputs to prevent
the race condition during simultaneously calling of init_usb_class().
If you think there is scope to improve the patch, please share your inputs.
thanks,
ajay kaher
Signed-off-by: Ajay Kaher
---
drivers/usb/core/file.c |6 ++
1 file changed, 6 insertions(+)
diff --git a/drivers/usb/core/file.c b/drivers/usb/core/file.c
index 822ced9..a12d184 100644
--- a/drivers/usb/core/file.c
+++ b/drivers/usb/core/file.c
@@ -27,6 +27,7 @@
#define MAX_USB_MINORS 256
static const struct file_operations *usb_minors[MAX_USB_MINORS];
static DECLARE_RWSEM(minor_rwsem);
+static DEFINE_MUTEX(init_usb_class_mutex);
static int usb_open(struct inode *inode, struct file *file)
{
@@ -109,8 +110,10 @@ static void release_usb_class(struct kref *kref)
static void destroy_usb_class(void)
{
+ mutex_lock(_usb_class_mutex);
if (usb_class)
kref_put(_class->kref, release_usb_class);
+ mutex_unlock(_usb_class_mutex);
}
int usb_major_init(void)
@@ -171,7 +174,10 @@ int usb_register_dev(struct usb_interface *intf,
if (intf->minor >= 0)
return -EADDRINUSE;
+ mutex_lock(_usb_class_mutex);
retval = init_usb_class();
+ mutex_unlock(_usb_class_mutex);
+
if (retval)
return retval;
RE: Re: Subject: [PATCH v3] USB:Core: BugFix: Proper handling of Race Condition when two USB class drivers try to call init_usb_class simultaneously
On Tue, 21 Feb 2017, Alan Stern wrote:
> On Mon, 20 Feb 2017, Ajay Kaher wrote:
>> Alan, as per my understanding I have shifted the lock from
>> release_usb_class() to destroy_usb_class() in patch v3.
>> If it is not right, please explain in detail which race condition
>> I have missed and also share your suggestions.
>>
>
> Have you considered what would happen if destroy_usb_class() ran, but
> some other CPU was still holding a reference to usb_class? And what if
> the last reference gets dropped later on, while init_usb_class() is
> running?
Access of usb_class->kref is only from either init_usb_class()
or destroy_usb_class(), and both these functions are now protected
with Mutex Locking in patch v3, so there is no chance of race condition
as per above scenarios.
> Maybe that's not possible here, but it is possible in general for
> refcounted objects. So yes, this code is probably okay, but it isn't
> good form.
As per my understanding, I found to be one of the best possible solution
for this problem and this solutiuon don't have any side effect.
thanks,
ajay kaher
> Signed-off-by: Ajay Kaher
>
> ---
>
> drivers/usb/core/file.c |6 ++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/usb/core/file.c b/drivers/usb/core/file.c
> index 822ced9..a12d184 100644
> --- a/drivers/usb/core/file.c
> +++ b/drivers/usb/core/file.c
> @@ -27,6 +27,7 @@
> #define MAX_USB_MINORS 256
> static const struct file_operations *usb_minors[MAX_USB_MINORS];
> static DECLARE_RWSEM(minor_rwsem);
> +static DEFINE_MUTEX(init_usb_class_mutex);
>
> static int usb_open(struct inode *inode, struct file *file)
> {
> @@ -109,8 +110,10 @@ static void release_usb_class(struct kref *kref)
>
> static void destroy_usb_class(void)
> {
> + mutex_lock(_usb_class_mutex);
> if (usb_class)
> kref_put(_class->kref, release_usb_class);
> + mutex_unlock(_usb_class_mutex);
> }
>
> int usb_major_init(void)
> @@ -171,7 +174,10 @@ int usb_register_dev(struct usb_interface *intf,
> if (intf->minor >= 0)
> return -EADDRINUSE;
>
> + mutex_lock(_usb_class_mutex);
> retval = init_usb_class();
> + mutex_unlock(_usb_class_mutex);
> +
> if (retval)
> return retval;
Re: Subject: [PATCH v3] USB:Core: BugFix: Proper handling of Race Condition when two USB class drivers try to call init_usb_class simultaneously
On 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.
>
> thanks,
> ajay kaher
>
> Signed-off-by: Ajay Kaher
>
> ---
>
> drivers/usb/core/file.c |6 ++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/usb/core/file.c b/drivers/usb/core/file.c
> index 822ced9..a12d184 100644
> --- a/drivers/usb/core/file.c
> +++ b/drivers/usb/core/file.c
> @@ -27,6 +27,7 @@
> #define MAX_USB_MINORS 256
> static const struct file_operations *usb_minors[MAX_USB_MINORS];
> static DECLARE_RWSEM(minor_rwsem);
> +static DEFINE_MUTEX(init_usb_class_mutex);
>
> static int usb_open(struct inode *inode, struct file *file)
> {
> @@ -109,8 +110,10 @@ static void release_usb_class(struct kref *kref)
>
> static void destroy_usb_class(void)
> {
> + mutex_lock(_usb_class_mutex);
> if (usb_class)
> kref_put(_class->kref, release_usb_class);
> + mutex_unlock(_usb_class_mutex);
> }
>
> int usb_major_init(void)
> @@ -171,7 +174,10 @@ int usb_register_dev(struct usb_interface *intf,
> if (intf->minor >= 0)
> return -EADDRINUSE;
>
> + mutex_lock(_usb_class_mutex);
> retval = init_usb_class();
> + mutex_unlock(_usb_class_mutex);
> +
> if (retval)
> return retval;
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?
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.
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 v3] USB:Core: BugFix: Proper handling of Race Condition when two USB class drivers try to call init_usb_class simultaneously
On Thu, 16 Feb 2017, Alan Stern wrote:
> On Thu, 16 Feb 2017, Ajay Kaher wrote:
>
>> > On Thu, 14 Feb 2017, Alan Stern wrote:
>> >
>> > I think Ajay's argument is correct and a patch is needed. But this
>> > patch misses the race between init_usb_class() and release_usb_class().
>>
>> Thanks Alan for your comments, in patch v2 I have taken care for
>> release_usb_class() also. Please review again.
>>
>> > The basic problem is that reference counting doesn't work when you try
>> > to use the same global pointer (usb_class) to refer to multiple
>> > generations of a dynamically allocated entity. We had the same sort of
>> > problem many years ago with the usb_interface structure (and we
>> > ultimately fixed it by creating a separate usb_interface_cache
>> > structure).
>> >
>> > The best approach here would be to forget about all the reference
>> > counting. Get rid of usb_class entirely, and create the "usbmisc"
>> > class structure just once, when usbcore initializes. Or, if you
>> > prefer, use a mutex to protect a routine that allocates the class
>> > structure dynamically, just once. Either way, don't deallocate it
>> > until usbcore is unloaded.
>>
>> usbmisc class creation should not require everytime when USB core
>> initializes. So better to keep usbmisc class creation as it is.
>> And to prevent the race conditions just protect it with Mutex locking
>> as per patch v2.
>
> This is not right. What happens if usb_register_dev() runs just before
> release_usb_class() calls mutex_lock()?
Alan, as per my understanding I have shifted the lock from
release_usb_class() to destroy_usb_class() in patch v3.
If it is not right, please explain in detail which race condition
I have missed and also share your suggestions.
thanks,
ajay kaher
Signed-off-by: Ajay Kaher
---
drivers/usb/core/file.c |6 ++
1 file changed, 6 insertions(+)
diff --git a/drivers/usb/core/file.c b/drivers/usb/core/file.c
index 822ced9..a12d184 100644
--- a/drivers/usb/core/file.c
+++ b/drivers/usb/core/file.c
@@ -27,6 +27,7 @@
#define MAX_USB_MINORS 256
static const struct file_operations *usb_minors[MAX_USB_MINORS];
static DECLARE_RWSEM(minor_rwsem);
+static DEFINE_MUTEX(init_usb_class_mutex);
static int usb_open(struct inode *inode, struct file *file)
{
@@ -109,8 +110,10 @@ static void release_usb_class(struct kref *kref)
static void destroy_usb_class(void)
{
+ mutex_lock(_usb_class_mutex);
if (usb_class)
kref_put(_class->kref, release_usb_class);
+ mutex_unlock(_usb_class_mutex);
}
int usb_major_init(void)
@@ -171,7 +174,10 @@ int usb_register_dev(struct usb_interface *intf,
if (intf->minor >= 0)
return -EADDRINUSE;
+ mutex_lock(_usb_class_mutex);
retval = init_usb_class();
+ mutex_unlock(_usb_class_mutex);
+
if (retval)
return retval;
