[PATCH] usb: gadget: mv_u3d_core: fix violation of locking discipline in mv_u3d_ep_disable()

[PATCH] usb: gadget: mv_u3d_core: fix violation of locking discipline in mv_u3d_ep_disable()

[Alexey Khoroshilov]

mv_u3d_nuke() expects to be calles with ep->u3d->lock held,
because mv_u3d_done() does. But mv_u3d_ep_disable() calls it
without lock that can lead to unpleasant consequences.

Found by Linux Driver Verification project (linuxtesting.org).

Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
---
 drivers/usb/gadget/mv_u3d_core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/usb/gadget/mv_u3d_core.c b/drivers/usb/gadget/mv_u3d_core.c
index 07fdb3e..650847d 100644
--- a/drivers/usb/gadget/mv_u3d_core.c
+++ b/drivers/usb/gadget/mv_u3d_core.c
@@ -645,6 +645,7 @@ static int  mv_u3d_ep_disable(struct usb_ep *_ep)
 	struct mv_u3d_ep *ep;
 	struct mv_u3d_ep_context *ep_context;
 	u32 epxcr, direction;
+	unsigned long flags;
 
 	if (!_ep)
 		return -EINVAL;
@@ -661,7 +662,9 @@ static int  mv_u3d_ep_disable(struct usb_ep *_ep)
 	direction = mv_u3d_ep_dir(ep);
 
 	/* nuke all pending requests (does flush) */
+	spin_lock_irqsave(&u3d->lock, flags);
 	mv_u3d_nuke(ep, -ESHUTDOWN);
+	spin_unlock_irqrestore(&u3d->lock, flags);
 
 	/* Disable the endpoint for Rx or Tx and reset the endpoint type */
 	if (direction == MV_U3D_EP_DIR_OUT) {
-- 
1.8.1.2

Re: [PATCH] usb: gadget: mv_u3d_core: fix violation of locking discipline in mv_u3d_ep_disable()

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 488 bytes --]

Hi,

On Wed, Jul 24, 2013 at 12:20:17AM +0400, Alexey Khoroshilov wrote:
> mv_u3d_nuke() expects to be calles with ep->u3d->lock held,
> because mv_u3d_done() does. But mv_u3d_ep_disable() calls it
> without lock that can lead to unpleasant consequences.
> 
> Found by Linux Driver Verification project (linuxtesting.org).
> 
> Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>

which commit introduced the bug ? Which kernels are affected by this bug ?

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] usb: gadget: mv_u3d_core: fix violation of locking discipline in mv_u3d_ep_disable()

[Alexey Khoroshilov]

Hi Felipe,

On 07/25/2013 09:30 PM, Felipe Balbi wrote:
> On Wed, Jul 24, 2013 at 12:20:17AM +0400, Alexey Khoroshilov wrote:
>> mv_u3d_nuke() expects to be calles with ep->u3d->lock held,
>> because mv_u3d_done() does. But mv_u3d_ep_disable() calls it
>> without lock that can lead to unpleasant consequences.
>>
>> Found by Linux Driver Verification project (linuxtesting.org).
>>
>> Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
> which commit introduced the bug ? Which kernels are affected by this bug ?
The bug is present from the very beginning: commit 3d4eb9d of 15 June 2012.
So it is in the mainline since v3.5.

--
Alexey

Re: [PATCH] usb: gadget: mv_u3d_core: fix violation of locking discipline in mv_u3d_ep_disable()

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 920 bytes --]

Hi,

On Fri, Jul 26, 2013 at 07:26:05PM +0400, Alexey Khoroshilov wrote:
> On 07/25/2013 09:30 PM, Felipe Balbi wrote:
> > On Wed, Jul 24, 2013 at 12:20:17AM +0400, Alexey Khoroshilov wrote:
> >> mv_u3d_nuke() expects to be calles with ep->u3d->lock held,
> >> because mv_u3d_done() does. But mv_u3d_ep_disable() calls it
> >> without lock that can lead to unpleasant consequences.
> >>
> >> Found by Linux Driver Verification project (linuxtesting.org).
> >>
> >> Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
> > which commit introduced the bug ? Which kernels are affected by this bug ?
> The bug is present from the very beginning: commit 3d4eb9d of 15 June 2012.
> So it is in the mainline since v3.5.

Alright, do you want to have the same fix in stable kernels ? Is it
necessary at all or do we consider it 'never worked before' and send it
in the next merge window ?

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] usb: gadget: mv_u3d_core: fix violation of locking discipline in mv_u3d_ep_disable()

[Alexey Khoroshilov]

On 07/29/2013 04:52 PM, Felipe Balbi wrote:
> Hi,
>
> On Fri, Jul 26, 2013 at 07:26:05PM +0400, Alexey Khoroshilov wrote:
>> On 07/25/2013 09:30 PM, Felipe Balbi wrote:
>>> On Wed, Jul 24, 2013 at 12:20:17AM +0400, Alexey Khoroshilov wrote:
>>>> mv_u3d_nuke() expects to be calles with ep->u3d->lock held,
>>>> because mv_u3d_done() does. But mv_u3d_ep_disable() calls it
>>>> without lock that can lead to unpleasant consequences.
>>>>
>>>> Found by Linux Driver Verification project (linuxtesting.org).
>>>>
>>>> Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
>>> which commit introduced the bug ? Which kernels are affected by this bug ?
>> The bug is present from the very beginning: commit 3d4eb9d of 15 June 2012.
>> So it is in the mainline since v3.5.
> Alright, do you want to have the same fix in stable kernels ? Is it
> necessary at all or do we consider it 'never worked before' and send it
> in the next merge window ?

It is a tricky point. From one point of view it 'never worked before',
but as far as the bug leads to a data race with unpredictable
consequences I would prefer to have it fixed within 3.11 timeframe.

--
Alexey

Re: [PATCH] usb: gadget: mv_u3d_core: fix violation of locking discipline in mv_u3d_ep_disable()

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1425 bytes --]

On Mon, Jul 29, 2013 at 05:15:58PM +0400, Alexey Khoroshilov wrote:
> On 07/29/2013 04:52 PM, Felipe Balbi wrote:
> > Hi,
> >
> > On Fri, Jul 26, 2013 at 07:26:05PM +0400, Alexey Khoroshilov wrote:
> >> On 07/25/2013 09:30 PM, Felipe Balbi wrote:
> >>> On Wed, Jul 24, 2013 at 12:20:17AM +0400, Alexey Khoroshilov wrote:
> >>>> mv_u3d_nuke() expects to be calles with ep->u3d->lock held,
> >>>> because mv_u3d_done() does. But mv_u3d_ep_disable() calls it
> >>>> without lock that can lead to unpleasant consequences.
> >>>>
> >>>> Found by Linux Driver Verification project (linuxtesting.org).
> >>>>
> >>>> Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
> >>> which commit introduced the bug ? Which kernels are affected by this bug ?
> >> The bug is present from the very beginning: commit 3d4eb9d of 15 June 2012.
> >> So it is in the mainline since v3.5.
> > Alright, do you want to have the same fix in stable kernels ? Is it
> > necessary at all or do we consider it 'never worked before' and send it
> > in the next merge window ?
> 
> It is a tricky point. From one point of view it 'never worked before',
> but as far as the bug leads to a data race with unpredictable
> consequences I would prefer to have it fixed within 3.11 timeframe.

Alright, I have already sent my pull request for v3.11-rc3, so once -rc4
is tagged (in about a week) I'll send this patch.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] usb: gadget: mv_u3d_core: fix violation of locking discipline in mv_u3d_ep_disable()

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2964 bytes --]

On Mon, Jul 29, 2013 at 04:52:12PM +0300, Felipe Balbi wrote:
> On Mon, Jul 29, 2013 at 05:15:58PM +0400, Alexey Khoroshilov wrote:
> > On 07/29/2013 04:52 PM, Felipe Balbi wrote:
> > > Hi,
> > >
> > > On Fri, Jul 26, 2013 at 07:26:05PM +0400, Alexey Khoroshilov wrote:
> > >> On 07/25/2013 09:30 PM, Felipe Balbi wrote:
> > >>> On Wed, Jul 24, 2013 at 12:20:17AM +0400, Alexey Khoroshilov wrote:
> > >>>> mv_u3d_nuke() expects to be calles with ep->u3d->lock held,
> > >>>> because mv_u3d_done() does. But mv_u3d_ep_disable() calls it
> > >>>> without lock that can lead to unpleasant consequences.
> > >>>>
> > >>>> Found by Linux Driver Verification project (linuxtesting.org).
> > >>>>
> > >>>> Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
> > >>> which commit introduced the bug ? Which kernels are affected by this bug ?
> > >> The bug is present from the very beginning: commit 3d4eb9d of 15 June 2012.
> > >> So it is in the mainline since v3.5.
> > > Alright, do you want to have the same fix in stable kernels ? Is it
> > > necessary at all or do we consider it 'never worked before' and send it
> > > in the next merge window ?
> > 
> > It is a tricky point. From one point of view it 'never worked before',
> > but as far as the bug leads to a data race with unpredictable
> > consequences I would prefer to have it fixed within 3.11 timeframe.
> 
> Alright, I have already sent my pull request for v3.11-rc3, so once -rc4
> is tagged (in about a week) I'll send this patch.

BTW, I just found another bug in mv_u3d_core.c, here's a patch:

From f6bb304cff095219ecfa6daa082ea834a9cf52bf Mon Sep 17 00:00:00 2001
From: Felipe Balbi <balbi@ti.com>
Date: Mon, 29 Jul 2013 16:58:29 +0300
Subject: [PATCH] usb: gadget: mv_u3d_core: don't call ->disconnect() from
 stop_activity()

mv_u3d_stop_activity() should not be calling ->disconnect()
directly as udc-core will already handle that for us.

Signed-off-by: Felipe Balbi <balbi@ti.com>
---

Completely untested. It's clear that mv u3d driver
shouldn't be calling gadget_driver->disconnect()
at that time, but perhaps there are other bugs in the
driver which this will uncover.

 drivers/usb/gadget/mv_u3d_core.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/usb/gadget/mv_u3d_core.c b/drivers/usb/gadget/mv_u3d_core.c
index 7acbca9..948f77a 100644
--- a/drivers/usb/gadget/mv_u3d_core.c
+++ b/drivers/usb/gadget/mv_u3d_core.c
@@ -1397,13 +1397,6 @@ void mv_u3d_stop_activity(struct mv_u3d *u3d, struct usb_gadget_driver *driver)
 	list_for_each_entry(ep, &u3d->gadget.ep_list, ep.ep_list) {
 		mv_u3d_nuke(ep, -ESHUTDOWN);
 	}
-
-	/* report disconnect; the driver is already quiesced */
-	if (driver) {
-		spin_unlock(&u3d->lock);
-		driver->disconnect(&u3d->gadget);
-		spin_lock(&u3d->lock);
-	}
 }
 
 static void mv_u3d_irq_process_error(struct mv_u3d *u3d)
-- 
1.8.3.4.840.g6a90778

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]