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 <[email protected]>
---
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
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 <[email protected]>
which commit introduced the bug ? Which kernels are affected by this bug ?
--
balbi
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 <[email protected]>
> 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
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 <[email protected]>
> > 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
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 <[email protected]>
>>> 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
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 <[email protected]>
> >>> 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
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 <[email protected]>
> > >>> 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 <[email protected]>
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 <[email protected]>
---
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