Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752154AbdCCQ4B (ORCPT ); Fri, 3 Mar 2017 11:56:01 -0500 Received: from iolanthe.rowland.org ([192.131.102.54]:33432 "HELO iolanthe.rowland.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1751641AbdCCQzY (ORCPT ); Fri, 3 Mar 2017 11:55:24 -0500 Date: Fri, 3 Mar 2017 11:53:43 -0500 (EST) From: Alan Stern X-X-Sender: stern@iolanthe.rowland.org To: Ajay Kaher cc: "gregkh@linuxfoundation.org" , "linux-usb@vger.kernel.org" , "linux-kernel@vger.kernel.org" , AMAN DEEP , HEMANSHU SRIVASTAVA Subject: 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 In-Reply-To: <20170303144947epcms5p2afa99fe4129042363253bf804e1451cc@epcms5p2> Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=UTF-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2459 Lines: 68 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; >