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

[Felipe Balbi <balbi@ti.com>]

[-- 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 --]