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;
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
