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

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

[Andrey Konovalov]

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@oracle.com>
Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
---

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;
+	}
 	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;
+	}
 	length = min(arg.length, event->length);
 	if (copy_to_user((void __user *)value, event, sizeof(*event) + length))
 		return -EFAULT;
-- 
2.26.0.292.g33ef6b2f38-goog

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

[Alan Stern]

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@oracle.com>
> Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
> ---
> 
> 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?

Alan Stern

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@rowland.harvard.edu> 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@oracle.com>
> > Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
> > ---
> >
> > 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.

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@rowland.harvard.edu> 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@oracle.com>
> > > Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
> > > ---
> > >
> > > 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

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

[Andrey Konovalov]

On Mon, Apr 6, 2020 at 9:26 PM Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Mon, 6 Apr 2020, Andrey Konovalov wrote:
>
> > On Mon, Apr 6, 2020 at 8:20 PM Alan Stern <stern@rowland.harvard.edu> 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@oracle.com>
> > > > Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
> > > > ---
> > > >
> > > > 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.

Correct. Sent v2 with a comment.

> 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.

Currently not often (we can have one CONNECT and one CONTROL in the
queue at the same time), but that might change if we ever add more
event types. I'm not sure if it makes sense to change the
implementation right now, as this isn't a bottleneck. But if it ever
becomes one, I'll certainly do that, thanks for a suggestion!