lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  PHC 
Open Source and information security mailing list archives
 
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:   Mon, 20 Feb 2017 14:20:15 -0500 (EST)
From:   Alan Stern <stern@...land.harvard.edu>
To:     Ajay Kaher <ajay.kaher@...sung.com>
cc:     "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        AMAN DEEP <aman.deep@...sung.com>,
        HEMANSHU SRIVASTAVA <hemanshu.s@...sung.com>
Subject: 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(&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;

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

Powered by blists - more mailing lists