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

[Andrey Konovalov]

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.