lkml.org 
[lkml]   [2017]   [Mar]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectFW: 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
    From
    Date

    > 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
    \
     
     \ /
      Last update: 2017-03-02 09:35    [W:6.057 / U:0.024 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site