mbox series

[v3,0/2] usb: udc: indroduce more api for lower gadget driver

Message ID 20210619154309.52127-1-linyyuan@codeaurora.org
Headers show
Series usb: udc: indroduce more api for lower gadget driver | expand

Message

Linyu Yuan June 19, 2021, 3:43 p.m. UTC 
void usb_gadget_udc_disconnect(struct usb_gadget *);
void usb_gadget_udc_suspend(struct usb_gadget *);
void usb_gadget_udc_resume(struct usb_gadget *);
int usb_gadget_udc_setup(struct usb_gadget *,
			const struct usb_ctrlrequest *);

dwc3 is first driver to use these apis.

v3: fix mail format
v3: fix mail format

Linyu Yuan (2):
  usb: udc: core: hide struct usb_gadget_driver to gadget driver
  usb: dwc3: fix race of usb_gadget_driver operation

 drivers/usb/dwc3/core.h       |  2 --
 drivers/usb/dwc3/ep0.c        |  6 +---
 drivers/usb/dwc3/gadget.c     | 53 +++++++++--------------------------
 drivers/usb/gadget/udc/core.c | 47 ++++++++++++++++++++++++++++++-
 include/linux/usb/gadget.h    |  6 ++++
 5 files changed, 66 insertions(+), 48 deletions(-)

Comments

Alan Stern June 20, 2021, 2:13 a.m. UTC | #1 
On Sat, Jun 19, 2021 at 11:43:08PM +0800, Linyu Yuan wrote:
> currently most gadget driver have a pointer to save

> struct usb_gadget_driver from upper layer,

> it allow upper layer set and unset of the pointer.

> 

> there is race that upper layer unset the pointer first,

> but gadget driver use the pointer later,

> and it cause system crash due to NULL pointer access.


This race has already been fixed in Greg's usb-next branch.  See commit 
7dc0c55e9f30 ("USB: UDC core: Add udc_async_callbacks gadget op") and 
following commits 04145a03db9d ("USB: UDC: Implement 
udc_async_callbacks in dummy-hcd") and b42e8090ba93 ("USB: UDC: 
Implement udc_async_callbacks in net2280").

You just need to write a corresponding patch implementing the 
async_callbacks op for dwc3.

Alan Stern
Linyu Yuan June 20, 2021, 3:46 a.m. UTC | #2 
On 2021-06-20 10:13, Alan Stern wrote:
> On Sat, Jun 19, 2021 at 11:43:08PM +0800, Linyu Yuan wrote:

>> currently most gadget driver have a pointer to save

>> struct usb_gadget_driver from upper layer,

>> it allow upper layer set and unset of the pointer.

>> 

>> there is race that upper layer unset the pointer first,

>> but gadget driver use the pointer later,

>> and it cause system crash due to NULL pointer access.

> 

> This race has already been fixed in Greg's usb-next branch.  See commit

> 7dc0c55e9f30 ("USB: UDC core: Add udc_async_callbacks gadget op") and

> following commits 04145a03db9d ("USB: UDC: Implement

> udc_async_callbacks in dummy-hcd") and b42e8090ba93 ("USB: UDC:

> Implement udc_async_callbacks in net2280").

> 

thanks, this is better, lower driver only need change several places.
> You just need to write a corresponding patch implementing the

> async_callbacks op for dwc3.

yes, i will do.
> 

> Alan Stern
Linyu Yuan June 20, 2021, 3:53 a.m. UTC | #3 
On 2021-06-20 11:46, linyyuan@codeaurora.org wrote:
> On 2021-06-20 10:13, Alan Stern wrote:

>> On Sat, Jun 19, 2021 at 11:43:08PM +0800, Linyu Yuan wrote:

>>> currently most gadget driver have a pointer to save

>>> struct usb_gadget_driver from upper layer,

>>> it allow upper layer set and unset of the pointer.

>>> 

>>> there is race that upper layer unset the pointer first,

>>> but gadget driver use the pointer later,

>>> and it cause system crash due to NULL pointer access.

>> 

>> This race has already been fixed in Greg's usb-next branch.  See 

>> commit

>> 7dc0c55e9f30 ("USB: UDC core: Add udc_async_callbacks gadget op") and

>> following commits 04145a03db9d ("USB: UDC: Implement

>> udc_async_callbacks in dummy-hcd") and b42e8090ba93 ("USB: UDC:

>> Implement udc_async_callbacks in net2280").

>> 

> thanks, this is better, lower driver only need change several places.

>> You just need to write a corresponding patch implementing the

>> async_callbacks op for dwc3.

> yes, i will do.

>> 

Alan, i want to discuss your suggestion again in b42e8090ba93 ("USB: 
UDC:
Implement udc_async_callbacks in net2280")

+                       if (dev->async_callbacks) { ----> if CPU1 saw 
this is true
+                               spin_unlock(&dev->lock); ---> CPU2 get 
lock after this unlock,
it will set async_callbacks to false, then follow call also crash, right 
?
+                               tmp = dev->driver->setup(&dev->gadget, 
&u.r);
+                               spin_lock(&dev->lock);
+                       }

>> Alan Stern
Alan Stern June 20, 2021, 1:47 p.m. UTC | #4 
On Sun, Jun 20, 2021 at 11:53:18AM +0800, linyyuan@codeaurora.org wrote:
> On 2021-06-20 11:46, linyyuan@codeaurora.org wrote:

> > On 2021-06-20 10:13, Alan Stern wrote:

> > > On Sat, Jun 19, 2021 at 11:43:08PM +0800, Linyu Yuan wrote:

> > > > currently most gadget driver have a pointer to save

> > > > struct usb_gadget_driver from upper layer,

> > > > it allow upper layer set and unset of the pointer.

> > > > 

> > > > there is race that upper layer unset the pointer first,

> > > > but gadget driver use the pointer later,

> > > > and it cause system crash due to NULL pointer access.

> > > 

> > > This race has already been fixed in Greg's usb-next branch.  See

> > > commit

> > > 7dc0c55e9f30 ("USB: UDC core: Add udc_async_callbacks gadget op") and

> > > following commits 04145a03db9d ("USB: UDC: Implement

> > > udc_async_callbacks in dummy-hcd") and b42e8090ba93 ("USB: UDC:

> > > Implement udc_async_callbacks in net2280").

> > > 

> > thanks, this is better, lower driver only need change several places.

> > > You just need to write a corresponding patch implementing the

> > > async_callbacks op for dwc3.

> > yes, i will do.

> > > 

> Alan, i want to discuss your suggestion again in b42e8090ba93 ("USB: UDC:

> Implement udc_async_callbacks in net2280")

> 

> +                       if (dev->async_callbacks) { ----> if CPU1 saw this

> is true

> +                               spin_unlock(&dev->lock); ---> CPU2 get lock

> after this unlock,

> it will set async_callbacks to false, then follow call also crash, right ?

> +                               tmp = dev->driver->setup(&dev->gadget,

> &u.r);

> +                               spin_lock(&dev->lock);

> +                       }


No, this is okay.  The reason is because usb_gadget_remove_driver (CPU2 
in your example) does this:

        usb_gadget_disable_async_callbacks(udc);
        if (udc->gadget->irq)
                synchronize_irq(udc->gadget->irq);
        udc->driver->unbind(udc->gadget);
        usb_gadget_udc_stop(udc);

The synchronize_irq call will make CPU2 wait until CPU1 has finished 
handling the interrupt for the setup packet.  The system won't crash, 
because dev->driver->setup will be called before unbind and udc_stop 
instead of after.

Alan Stern
Linyu Yuan June 21, 2021, 1:37 a.m. UTC | #5 
On 2021-06-20 21:47, Alan Stern wrote:
> On Sun, Jun 20, 2021 at 11:53:18AM +0800, linyyuan@codeaurora.org 

> wrote:

>> On 2021-06-20 11:46, linyyuan@codeaurora.org wrote:

>> > On 2021-06-20 10:13, Alan Stern wrote:

>> > > On Sat, Jun 19, 2021 at 11:43:08PM +0800, Linyu Yuan wrote:

>> > > > currently most gadget driver have a pointer to save

>> > > > struct usb_gadget_driver from upper layer,

>> > > > it allow upper layer set and unset of the pointer.

>> > > >

>> > > > there is race that upper layer unset the pointer first,

>> > > > but gadget driver use the pointer later,

>> > > > and it cause system crash due to NULL pointer access.

>> > >

>> > > This race has already been fixed in Greg's usb-next branch.  See

>> > > commit

>> > > 7dc0c55e9f30 ("USB: UDC core: Add udc_async_callbacks gadget op") and

>> > > following commits 04145a03db9d ("USB: UDC: Implement

>> > > udc_async_callbacks in dummy-hcd") and b42e8090ba93 ("USB: UDC:

>> > > Implement udc_async_callbacks in net2280").

>> > >

>> > thanks, this is better, lower driver only need change several places.

>> > > You just need to write a corresponding patch implementing the

>> > > async_callbacks op for dwc3.

>> > yes, i will do.

>> > >

>> Alan, i want to discuss your suggestion again in b42e8090ba93 ("USB: 

>> UDC:

>> Implement udc_async_callbacks in net2280")

>> 

>> +                       if (dev->async_callbacks) { ----> if CPU1 saw 

>> this

>> is true

>> +                               spin_unlock(&dev->lock); ---> CPU2 get 

>> lock

>> after this unlock,

>> it will set async_callbacks to false, then follow call also crash, 

>> right ?

>> +                               tmp = dev->driver->setup(&dev->gadget,

>> &u.r);

>> +                               spin_lock(&dev->lock);

>> +                       }

> 

> No, this is okay.  The reason is because usb_gadget_remove_driver (CPU2

> in your example) does this:

> 

>         usb_gadget_disable_async_callbacks(udc);

>         if (udc->gadget->irq)

>                 synchronize_irq(udc->gadget->irq);

>         udc->driver->unbind(udc->gadget);

>         usb_gadget_udc_stop(udc);

> 

> The synchronize_irq call will make CPU2 wait until CPU1 has finished

> handling the interrupt for the setup packet.  The system won't crash,

> because dev->driver->setup will be called before unbind and udc_stop

> instead of after.

still several question,
1. how about suspend calll dev->driver->suspend ?
2. will 04145a03db9d ("USB: UDC: Implement udc_async_callbacks in 
dummy-hcd") backport to LTS branch ?
3. how about coding style ? so following code
if (foo->gadget_driver && foo->gadget_driver->resume)
change to
if (foo->asnyc_callbacks && foo->gadget_driver->resume)
> 

> Alan Stern
Alan Stern June 21, 2021, 1:53 p.m. UTC | #6 
On Mon, Jun 21, 2021 at 09:37:34AM +0800, linyyuan@codeaurora.org wrote:
> On 2021-06-20 21:47, Alan Stern wrote:

> > On Sun, Jun 20, 2021 at 11:53:18AM +0800, linyyuan@codeaurora.org wrote:

> > > On 2021-06-20 11:46, linyyuan@codeaurora.org wrote:

> > > > On 2021-06-20 10:13, Alan Stern wrote:

> > > > > On Sat, Jun 19, 2021 at 11:43:08PM +0800, Linyu Yuan wrote:

> > > > > > currently most gadget driver have a pointer to save

> > > > > > struct usb_gadget_driver from upper layer,

> > > > > > it allow upper layer set and unset of the pointer.

> > > > > >

> > > > > > there is race that upper layer unset the pointer first,

> > > > > > but gadget driver use the pointer later,

> > > > > > and it cause system crash due to NULL pointer access.

> > > > >

> > > > > This race has already been fixed in Greg's usb-next branch.  See

> > > > > commit

> > > > > 7dc0c55e9f30 ("USB: UDC core: Add udc_async_callbacks gadget op") and

> > > > > following commits 04145a03db9d ("USB: UDC: Implement

> > > > > udc_async_callbacks in dummy-hcd") and b42e8090ba93 ("USB: UDC:

> > > > > Implement udc_async_callbacks in net2280").

> > > > >

> > > > thanks, this is better, lower driver only need change several places.

> > > > > You just need to write a corresponding patch implementing the

> > > > > async_callbacks op for dwc3.

> > > > yes, i will do.

> > > > >

> > > Alan, i want to discuss your suggestion again in b42e8090ba93 ("USB:

> > > UDC:

> > > Implement udc_async_callbacks in net2280")

> > > 

> > > +                       if (dev->async_callbacks) { ----> if CPU1

> > > saw this

> > > is true

> > > +                               spin_unlock(&dev->lock); ---> CPU2

> > > get lock

> > > after this unlock,

> > > it will set async_callbacks to false, then follow call also crash,

> > > right ?

> > > +                               tmp = dev->driver->setup(&dev->gadget,

> > > &u.r);

> > > +                               spin_lock(&dev->lock);

> > > +                       }

> > 

> > No, this is okay.  The reason is because usb_gadget_remove_driver (CPU2

> > in your example) does this:

> > 

> >         usb_gadget_disable_async_callbacks(udc);

> >         if (udc->gadget->irq)

> >                 synchronize_irq(udc->gadget->irq);

> >         udc->driver->unbind(udc->gadget);

> >         usb_gadget_udc_stop(udc);

> > 

> > The synchronize_irq call will make CPU2 wait until CPU1 has finished

> > handling the interrupt for the setup packet.  The system won't crash,

> > because dev->driver->setup will be called before unbind and udc_stop

> > instead of after.


> still several question,

> 1. how about suspend calll dev->driver->suspend ?


The same reasoning applies.  The synchronize_irq call will make CPU2 
wait until CPU1 has finished handling the interrupt for the USB bus 
suspend.  The system won't crash, because dev->driver->suspend will be 
called before unbind and udc_stop instead of after.

> 2. will 04145a03db9d ("USB: UDC: Implement udc_async_callbacks in

> dummy-hcd") backport to LTS branch ?


None of these commits are marked for back-porting to the -stable 
kernels.  The race they fix does not occur often.

If you the commits to be applied to the LTS stable kernels, you can ask 
Greg KH to do it.

> 3. how about coding style ? so following code

> if (foo->gadget_driver && foo->gadget_driver->resume)

> change to

> if (foo->asnyc_callbacks && foo->gadget_driver->resume)


I don't understand this question.

Alan Stern