[bug report] usb: gadget: add raw-gadget interface

[bug report] usb: gadget: add raw-gadget interface

[Dan Carpenter]

Hello Andrey Konovalov,

The patch f2c2e717642c: "usb: gadget: add raw-gadget interface" from
Feb 24, 2020, leads to the following static checker warning:

    drivers/usb/gadget/legacy/raw_gadget.c:102 raw_event_queue_fetch() warn: inconsistent returns 'queue->sema'.
      Locked on  : 96,102
      Unlocked on: 93

drivers/usb/gadget/legacy/raw_gadget.c
    81  static struct usb_raw_event *raw_event_queue_fetch(
    82                                  struct raw_event_queue *queue)
    83  {
    84          unsigned long flags;
    85          struct usb_raw_event *event;
    86  
    87          /*
    88           * This function can be called concurrently. We first check that
    89           * there's at least one event queued by decrementing the semaphore,
    90           * and then take the lock to protect queue struct fields.
    91           */
    92          if (down_interruptible(&queue->sema))
    93                  return NULL;
    94          spin_lock_irqsave(&queue->lock, flags);
    95          if (WARN_ON(!queue->size))
    96                  return NULL;

I'm going to have investigate to see why Smatch doesn't complain that
we have disabled IRQs on this path...  Anyway, this doesn't seem like it
can be correct.  I get that this is a WARN_ON() path, but we're leaving
the computer in a very screwed up state we don't at least enable IRQs.

    97          event = queue->events[0];
    98          queue->size--;
    99          memmove(&queue->events[0], &queue->events[1],
   100                          queue->size * sizeof(queue->events[0]));
   101          spin_unlock_irqrestore(&queue->lock, flags);
   102          return event;
   103  }

regards,
dan carpenter

Re: [bug report] usb: gadget: add raw-gadget interface

[Andrey Konovalov]

On Mon, Apr 6, 2020 at 12:55 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> Hello Andrey Konovalov,
>
> The patch f2c2e717642c: "usb: gadget: add raw-gadget interface" from
> Feb 24, 2020, leads to the following static checker warning:
>
>     drivers/usb/gadget/legacy/raw_gadget.c:102 raw_event_queue_fetch() warn: inconsistent returns 'queue->sema'.
>       Locked on  : 96,102
>       Unlocked on: 93
>
> drivers/usb/gadget/legacy/raw_gadget.c
>     81  static struct usb_raw_event *raw_event_queue_fetch(
>     82                                  struct raw_event_queue *queue)
>     83  {
>     84          unsigned long flags;
>     85          struct usb_raw_event *event;
>     86
>     87          /*
>     88           * This function can be called concurrently. We first check that
>     89           * there's at least one event queued by decrementing the semaphore,
>     90           * and then take the lock to protect queue struct fields.
>     91           */
>     92          if (down_interruptible(&queue->sema))
>     93                  return NULL;
>     94          spin_lock_irqsave(&queue->lock, flags);
>     95          if (WARN_ON(!queue->size))
>     96                  return NULL;
>
> I'm going to have investigate to see why Smatch doesn't complain that
> we have disabled IRQs on this path...  Anyway, this doesn't seem like it
> can be correct.  I get that this is a WARN_ON() path, but we're leaving
> the computer in a very screwed up state we don't at least enable IRQs.

Hi Dan,

Oh, right, I'll send a patch to add spin_lock_irqsave() there.

I don't really get the warning about queue->sema though, is there
something wrong with it, or is it actually a warning about
queue->lock?

Thanks for the report!

>
>     97          event = queue->events[0];
>     98          queue->size--;
>     99          memmove(&queue->events[0], &queue->events[1],
>    100                          queue->size * sizeof(queue->events[0]));
>    101          spin_unlock_irqrestore(&queue->lock, flags);
>    102          return event;
>    103  }

Re: [bug report] usb: gadget: add raw-gadget interface

[Dan Carpenter]

On Mon, Apr 06, 2020 at 03:06:12PM +0200, Andrey Konovalov wrote:
> On Mon, Apr 6, 2020 at 12:55 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> >
> > Hello Andrey Konovalov,
> >
> > The patch f2c2e717642c: "usb: gadget: add raw-gadget interface" from
> > Feb 24, 2020, leads to the following static checker warning:
> >
> >     drivers/usb/gadget/legacy/raw_gadget.c:102 raw_event_queue_fetch() warn: inconsistent returns 'queue->sema'.
> >       Locked on  : 96,102
> >       Unlocked on: 93
> >
> > drivers/usb/gadget/legacy/raw_gadget.c
> >     81  static struct usb_raw_event *raw_event_queue_fetch(
> >     82                                  struct raw_event_queue *queue)
> >     83  {
> >     84          unsigned long flags;
> >     85          struct usb_raw_event *event;
> >     86
> >     87          /*
> >     88           * This function can be called concurrently. We first check that
> >     89           * there's at least one event queued by decrementing the semaphore,
> >     90           * and then take the lock to protect queue struct fields.
> >     91           */
> >     92          if (down_interruptible(&queue->sema))
> >     93                  return NULL;
> >     94          spin_lock_irqsave(&queue->lock, flags);
> >     95          if (WARN_ON(!queue->size))
> >     96                  return NULL;
> >
> > I'm going to have investigate to see why Smatch doesn't complain that
> > we have disabled IRQs on this path...  Anyway, this doesn't seem like it
> > can be correct.  I get that this is a WARN_ON() path, but we're leaving
> > the computer in a very screwed up state we don't at least enable IRQs.
> 
> Hi Dan,
> 
> Oh, right, I'll send a patch to add spin_lock_irqsave() there.
> 
> I don't really get the warning about queue->sema though, is there
> something wrong with it, or is it actually a warning about
> queue->lock?

The rule here is that we return NULL here and on line 93, so how does
the caller know if we took that "queue->sema" lock?

regards,
dan carpenter

Re: [bug report] usb: gadget: add raw-gadget interface

[Andrey Konovalov]

On Mon, Apr 6, 2020 at 4:07 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> On Mon, Apr 06, 2020 at 03:06:12PM +0200, Andrey Konovalov wrote:
> > On Mon, Apr 6, 2020 at 12:55 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > >
> > > Hello Andrey Konovalov,
> > >
> > > The patch f2c2e717642c: "usb: gadget: add raw-gadget interface" from
> > > Feb 24, 2020, leads to the following static checker warning:
> > >
> > >     drivers/usb/gadget/legacy/raw_gadget.c:102 raw_event_queue_fetch() warn: inconsistent returns 'queue->sema'.
> > >       Locked on  : 96,102
> > >       Unlocked on: 93
> > >
> > > drivers/usb/gadget/legacy/raw_gadget.c
> > >     81  static struct usb_raw_event *raw_event_queue_fetch(
> > >     82                                  struct raw_event_queue *queue)
> > >     83  {
> > >     84          unsigned long flags;
> > >     85          struct usb_raw_event *event;
> > >     86
> > >     87          /*
> > >     88           * This function can be called concurrently. We first check that
> > >     89           * there's at least one event queued by decrementing the semaphore,
> > >     90           * and then take the lock to protect queue struct fields.
> > >     91           */
> > >     92          if (down_interruptible(&queue->sema))
> > >     93                  return NULL;
> > >     94          spin_lock_irqsave(&queue->lock, flags);
> > >     95          if (WARN_ON(!queue->size))
> > >     96                  return NULL;
> > >
> > > I'm going to have investigate to see why Smatch doesn't complain that
> > > we have disabled IRQs on this path...  Anyway, this doesn't seem like it
> > > can be correct.  I get that this is a WARN_ON() path, but we're leaving
> > > the computer in a very screwed up state we don't at least enable IRQs.
> >
> > Hi Dan,
> >
> > Oh, right, I'll send a patch to add spin_lock_irqsave() there.
> >
> > I don't really get the warning about queue->sema though, is there
> > something wrong with it, or is it actually a warning about
> > queue->lock?
>
> The rule here is that we return NULL here and on line 93, so how does
> the caller know if we took that "queue->sema" lock?

OK, I see, will send a fix soon, thanks!