linux-kernel - Re: [PATCH v3 1/1] usb: gadget: add raw-gadget interface

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 linux-cve-announce 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:   Wed, 18 Dec 2019 21:06:21 +0100
From:   Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:     Andrey Konovalov <andreyknvl@...gle.com>
Cc:     USB list <linux-usb@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Alan Stern <stern@...land.harvard.edu>,
        Jonathan Corbet <corbet@....net>,
        Felipe Balbi <balbi@...nel.org>,
        Dmitry Vyukov <dvyukov@...gle.com>,
        Alexander Potapenko <glider@...gle.com>,
        Marco Elver <elver@...gle.com>
Subject: Re: [PATCH v3 1/1] usb: gadget: add raw-gadget interface

On Wed, Dec 18, 2019 at 08:22:47PM +0100, Andrey Konovalov wrote:
> > > > > +static void gadget_unbind(struct usb_gadget *gadget)
> > > > > +{
> > > > > +     struct raw_dev *dev = get_gadget_data(gadget);
> > > > > +     unsigned long flags;
> > > > > +
> > > > > +     spin_lock_irqsave(&dev->lock, flags);
> > > > > +     set_gadget_data(gadget, NULL);
> > > > > +     spin_unlock_irqrestore(&dev->lock, flags);
> > > > > +     /* Matches kref_get() in gadget_bind(). */
> > > > > +     kref_put(&dev->count, dev_free);
> > > >
> > > > What protects the kref from being called 'put' twice on the same
> > > > pointer at the same time?  There should be some lock somewhere, right?
> > >
> > > Hm, kref_put() does refcount_dec_and_test(), which in turns calls
> > > atomic_dec_and_test(), so this is protected against concurrent puts
> > > (which is the whole idea of kref?), and no locking is needed. Unless I
> > > misunderstand something.
> >
> > It's late, but there should be some lock somewhere to prevent a race
> > around this type of thing.  That's why we have kref_put_mutex() and
> > kref_put_lock().
> >
> > Odds are you are fine here, but just something to be aware of...
> 
> Ah, I see. So AFAIU kref_put_lock/mutex() are meant to be used in
> cases when there might be a concurrent user that doesn't have the
> reference counter incremented, but holds the lock? We don't do this
> kind of stuff here.

Ok, as long as there is a lock somewhere preventing this type of thing
from happening.  Last time I looked at this, it took me and 2 grad
students an hour with a whiteboard to work it all out.  Which is why the
lock variants are there now :)

thanks,

greg k-h