Re: [PATCH] usb: raw-gadget: fix raw_event_queue_fetch locking

[Alan Stern]

On Mon, 6 Apr 2020, Andrey Konovalov wrote:

> On Mon, Apr 6, 2020 at 8:20 PM Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
> >
> > On Mon, 6 Apr 2020, Andrey Konovalov wrote:
> >
> > > If queue->size check in raw_event_queue_fetch() fails (which normally
> > > shouldn't happen, that check is a fail-safe), the function returns
> > > without reenabling interrupts. This patch fixes that issue, along with
> > > propagating the cause of failure to the function caller.
> > >
> > > Fixes: f2c2e717642c ("usb: gadget: add raw-gadget interface"
> > > Reported-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
> > > Signed-off-by: Andrey Konovalov <andreyknvl@xxxxxxxxxx>
> > > ---
> > >
> > > Greg, this should apply cleanly on top of Dan's "usb: raw-gadget: Fix
> > > copy_to/from_user() checks" patch.
> > >
> > > ---
> > >  drivers/usb/gadget/legacy/raw_gadget.c | 19 +++++++++++++++----
> > >  1 file changed, 15 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/usb/gadget/legacy/raw_gadget.c b/drivers/usb/gadget/legacy/raw_gadget.c
> > > index e490ffa1f58b..1582521ec774 100644
> > > --- a/drivers/usb/gadget/legacy/raw_gadget.c
> > > +++ b/drivers/usb/gadget/legacy/raw_gadget.c
> > > @@ -81,6 +81,7 @@ static int raw_event_queue_add(struct raw_event_queue *queue,
> > >  static struct usb_raw_event *raw_event_queue_fetch(
> > >                               struct raw_event_queue *queue)
> > >  {
> > > +     int ret;
> > >       unsigned long flags;
> > >       struct usb_raw_event *event;
> > >
> > > @@ -89,11 +90,14 @@ static struct usb_raw_event *raw_event_queue_fetch(
> > >        * there's at least one event queued by decrementing the semaphore,
> > >        * and then take the lock to protect queue struct fields.
> > >        */
> > > -     if (down_interruptible(&queue->sema))
> > > -             return NULL;
> > > +     ret = down_interruptible(&queue->sema);
> > > +     if (ret)
> > > +             return ERR_PTR(ret);
> > >       spin_lock_irqsave(&queue->lock, flags);
> > > -     if (WARN_ON(!queue->size))
> > > +     if (WARN_ON(!queue->size)) {
> > > +             spin_unlock_irqrestore(&queue->lock, flags);
> > >               return NULL;
> >
> > Suppose the WARN_ON triggers, and you return NULL here.  Then where do
> > you reverse the down_interruptible() on queue->sema?
> >
> > > +     }
> > >       event = queue->events[0];
> > >       queue->size--;
> > >       memmove(&queue->events[0], &queue->events[1],
> > > @@ -522,10 +526,17 @@ static int raw_ioctl_event_fetch(struct raw_dev *dev, unsigned long value)
> > >       spin_unlock_irqrestore(&dev->lock, flags);
> > >
> > >       event = raw_event_queue_fetch(&dev->queue);
> > > -     if (!event) {
> > > +     if (PTR_ERR(event) == -EINTR) {
> > >               dev_dbg(&dev->gadget->dev, "event fetching interrupted\n");
> > >               return -EINTR;
> > >       }
> > > +     if (IS_ERR_OR_NULL(event)) {
> > > +             dev_err(&dev->gadget->dev, "failed to fetch event\n");
> > > +             spin_lock_irqsave(&dev->lock, flags);
> > > +             dev->state = STATE_DEV_FAILED;
> > > +             spin_unlock_irqrestore(&dev->lock, flags);
> > > +             return -ENODEV;
> > > +     }
> >
> > Not here, obviously.  Does the semaphore ever get released?
> 
> If this warning triggered, something has already gone horribly wrong,
> so we set the device stated to "failed".
> 
> But even if we ignore that, should the semaphore be "released"? The
> initial semaphore's counter value is 0, so one up()+down() sequence of
> events leaves it with the initial value. So it's the down() event that
> brings it to the initial state (unless there were multiple up()s of
> course). Unless I misunderstand something.

Okay, now I get it.  It's an invariant of the driver: the semaphore's
value is always equal to queue->size.  You might consider putting this
in a comment, in some future update.

Incidentally, how often do you expect the queue to contain more than
one entry?  If that happens a lot, it would be more efficient to
implement the queue as a ring (with first and last pointers) than to
call memmove() every time an entry is removed.

Alan Stern