Re: [PATCH] usb: gadget: udc: udc_stop before gadget unbind

[Danilo Krummrich]

On 2017-08-15 14:02, Felipe Balbi wrote:

Hi,

Danilo Krummrich  writes:

thanks for reviewing.


np :-)


On 2017-08-15 12:03, Felipe Balbi wrote:

Hi,

Danilo Krummrich  writes:
udc_stop needs to be called before gadget driver unbind. Otherwise 
it
might happen that udc drivers still call into the gadget driver 
(e.g.
to reset gadget after OTG event). If this happens it is likely to 
get
panics from gadget driver dereferencing NULL ptr, as gadget's 
drvdata

is set to NULL on unbind.


seems like the problem here is with the OTG layer, not UDC core.


I mentioned this just as example, it can happen whenever a UDC driver
calls
the gadget driver (e.g. by calling usb_gadget_udc_reset() in ISR) 
after

gadget
drivers unbind() was called already (e.g. by gadget configfs).
If this happens gadget drivers drvdata was already set to NULL by
unbind()
and reset() could result into a NULL ptr exception.
Therefore my assumption was that it needs to be prevented that the
gadget
driver is getting called after unbind.


We have a known problem in the design of the gadget API that causes 
this

races but we couldn't come up with a solution yet :-)

Inverting these two calls is not the correct way to go about this :-)


Now I see, thanks for explanation below.


Signed-off-by: Danilo Krummrich 
---
Actually there could still be a race:
(CPU1 code taken from dwc3 drivers dwc3_disconnect_gadget() as
exsample)

CPU0CPU1

usb_gadget_disconnect(udc->gadget);
udc->driver->disconnect(udc->gadget);
if (dwc->gadget_driver && 
dwc->gadget_driver->disconnect)
usb_gadget_udc_stop(udc);
udc->driver->unbind(udc->gadget);

dwc->gadget_driver->disconnect(>gadget);

UDC drivers typically set their gadget driver pointer to NULL in
udc_stop
and check for it before calling into the gadget driver. To fix the
issue
above every udc driver could apply a lock around this.

If you see the need for having this or another solutions I can 
provide

further patches. This patch could also just serve as a base for
discussion
if someone knows a smarter solution.

I saw this problem causing a panic on hikey960 board and provided a
quick
workaround for the same problem here:
https://android-review.googlesource.com/#/c/kernel/common/+/457476/
(panic log in the commit message of the linked patch)
---
 drivers/usb/gadget/udc/core.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/udc/core.c
b/drivers/usb/gadget/udc/core.c
index efce68e9a8e0..8155468afc0d 100644
--- a/drivers/usb/gadget/udc/core.c
+++ b/drivers/usb/gadget/udc/core.c
@@ -1234,8 +1234,12 @@ static void usb_gadget_remove_driver(struct
usb_udc *udc)

usb_gadget_disconnect(udc->gadget);
udc->driver->disconnect(udc->gadget);
-   udc->driver->unbind(udc->gadget);
+   /* udc_stop needs to be called before gadget driver unbind to
prevent
+* udc driver calls into gadget driver after unbind which could
cause
+* a nullptr exception.
+*/
usb_gadget_udc_stop(udc);
+   udc->driver->unbind(udc->gadget);


This patch is incorrect, it will prevent us from giving back requests
to
gadget driver properly. ->unbind() has to happen before ->udc_stop().


Do you mean after udc_stop the udc driver can not call the gadget 
driver

anymore? If not, I did not got your point, sorry for that. Can you
please
help me out? Would the changed order raise another issue I'm not aware
of?


right, ->udc_stop() is supposed to completely teardown the USB
controller, including disabling interrupts and all. The only thing it
_can_ do from ->udc_stop() would be giving back any pending requests
that were left (which would cause req->complete() to be called with an
error status). But even that is unlikely in the case you mention since
->unbind() was already called.


Ok, got it. That's why req->context = cdev, to overcome being unbound
already. Thanks for clarification.


If I understood you correctly, without this patch udc driver can not
call
the gadget driver back as well, because this would result in a NULL 
ptr

dereference, as unbind() sets drvdata to NULL.

In any case the race described in my original message can still 
happen,
regardless of the order of udc_stop and unbind. But with this patch 
the

needed locking could easily done within the udc driver only. Without,
the
lock needs to be acquired before udc->driver->unbind(udc->gadget) and
released after usb_gadget_udc_stop(). Otherwise an ISR of the udc 
driver
trying to call into the gadget driver could do this after gadget 
driver

already unbound.


right

Is someone working on this issue, already?
If not, I would like to offer introducing the needed locking to overcome
this issue.
If you are about to refactor the whole 

Re: [PATCH] usb: gadget: udc: udc_stop before gadget unbind

[Danilo Krummrich]

On 2017-08-15 14:02, Felipe Balbi wrote:

Hi,

Danilo Krummrich  writes:

thanks for reviewing.


np :-)


On 2017-08-15 12:03, Felipe Balbi wrote:

Hi,

Danilo Krummrich  writes:
udc_stop needs to be called before gadget driver unbind. Otherwise 
it
might happen that udc drivers still call into the gadget driver 
(e.g.
to reset gadget after OTG event). If this happens it is likely to 
get
panics from gadget driver dereferencing NULL ptr, as gadget's 
drvdata

is set to NULL on unbind.


seems like the problem here is with the OTG layer, not UDC core.


I mentioned this just as example, it can happen whenever a UDC driver
calls
the gadget driver (e.g. by calling usb_gadget_udc_reset() in ISR) 
after

gadget
drivers unbind() was called already (e.g. by gadget configfs).
If this happens gadget drivers drvdata was already set to NULL by
unbind()
and reset() could result into a NULL ptr exception.
Therefore my assumption was that it needs to be prevented that the
gadget
driver is getting called after unbind.


We have a known problem in the design of the gadget API that causes 
this

races but we couldn't come up with a solution yet :-)

Inverting these two calls is not the correct way to go about this :-)


Now I see, thanks for explanation below.


Signed-off-by: Danilo Krummrich 
---
Actually there could still be a race:
(CPU1 code taken from dwc3 drivers dwc3_disconnect_gadget() as
exsample)

CPU0CPU1

usb_gadget_disconnect(udc->gadget);
udc->driver->disconnect(udc->gadget);
if (dwc->gadget_driver && 
dwc->gadget_driver->disconnect)
usb_gadget_udc_stop(udc);
udc->driver->unbind(udc->gadget);

dwc->gadget_driver->disconnect(>gadget);

UDC drivers typically set their gadget driver pointer to NULL in
udc_stop
and check for it before calling into the gadget driver. To fix the
issue
above every udc driver could apply a lock around this.

If you see the need for having this or another solutions I can 
provide

further patches. This patch could also just serve as a base for
discussion
if someone knows a smarter solution.

I saw this problem causing a panic on hikey960 board and provided a
quick
workaround for the same problem here:
https://android-review.googlesource.com/#/c/kernel/common/+/457476/
(panic log in the commit message of the linked patch)
---
 drivers/usb/gadget/udc/core.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/udc/core.c
b/drivers/usb/gadget/udc/core.c
index efce68e9a8e0..8155468afc0d 100644
--- a/drivers/usb/gadget/udc/core.c
+++ b/drivers/usb/gadget/udc/core.c
@@ -1234,8 +1234,12 @@ static void usb_gadget_remove_driver(struct
usb_udc *udc)

usb_gadget_disconnect(udc->gadget);
udc->driver->disconnect(udc->gadget);
-   udc->driver->unbind(udc->gadget);
+   /* udc_stop needs to be called before gadget driver unbind to
prevent
+* udc driver calls into gadget driver after unbind which could
cause
+* a nullptr exception.
+*/
usb_gadget_udc_stop(udc);
+   udc->driver->unbind(udc->gadget);


This patch is incorrect, it will prevent us from giving back requests
to
gadget driver properly. ->unbind() has to happen before ->udc_stop().


Do you mean after udc_stop the udc driver can not call the gadget 
driver

anymore? If not, I did not got your point, sorry for that. Can you
please
help me out? Would the changed order raise another issue I'm not aware
of?


right, ->udc_stop() is supposed to completely teardown the USB
controller, including disabling interrupts and all. The only thing it
_can_ do from ->udc_stop() would be giving back any pending requests
that were left (which would cause req->complete() to be called with an
error status). But even that is unlikely in the case you mention since
->unbind() was already called.


Ok, got it. That's why req->context = cdev, to overcome being unbound
already. Thanks for clarification.


If I understood you correctly, without this patch udc driver can not
call
the gadget driver back as well, because this would result in a NULL 
ptr

dereference, as unbind() sets drvdata to NULL.

In any case the race described in my original message can still 
happen,
regardless of the order of udc_stop and unbind. But with this patch 
the

needed locking could easily done within the udc driver only. Without,
the
lock needs to be acquired before udc->driver->unbind(udc->gadget) and
released after usb_gadget_udc_stop(). Otherwise an ISR of the udc 
driver
trying to call into the gadget driver could do this after gadget 
driver

already unbound.


right

Is someone working on this issue, already?
If not, I would like to offer introducing the needed locking to overcome
this issue.
If you are about to refactor the whole thing already to solve this (as
you state there's a design problem in the gadget API) would it 

Re: [PATCH] usb: gadget: udc: udc_stop before gadget unbind

[Felipe Balbi]

Hi,

Danilo Krummrich  writes:
> thanks for reviewing.

np :-)

> On 2017-08-15 12:03, Felipe Balbi wrote:
>> Hi,
>> 
>> Danilo Krummrich  writes:
>>> udc_stop needs to be called before gadget driver unbind. Otherwise it
>>> might happen that udc drivers still call into the gadget driver (e.g.
>>> to reset gadget after OTG event). If this happens it is likely to get
>>> panics from gadget driver dereferencing NULL ptr, as gadget's drvdata
>>> is set to NULL on unbind.
>> 
>> seems like the problem here is with the OTG layer, not UDC core.
>> 
> I mentioned this just as example, it can happen whenever a UDC driver 
> calls
> the gadget driver (e.g. by calling usb_gadget_udc_reset() in ISR) after 
> gadget
> drivers unbind() was called already (e.g. by gadget configfs).
> If this happens gadget drivers drvdata was already set to NULL by 
> unbind()
> and reset() could result into a NULL ptr exception.
> Therefore my assumption was that it needs to be prevented that the 
> gadget
> driver is getting called after unbind.

We have a known problem in the design of the gadget API that causes this
races but we couldn't come up with a solution yet :-)

Inverting these two calls is not the correct way to go about this :-)

>>> Signed-off-by: Danilo Krummrich 
>>> ---
>>> Actually there could still be a race:
>>> (CPU1 code taken from dwc3 drivers dwc3_disconnect_gadget() as 
>>> exsample)
>>> 
>>> CPU0CPU1
>>> 
>>> usb_gadget_disconnect(udc->gadget);
>>> udc->driver->disconnect(udc->gadget);
>>> if (dwc->gadget_driver && 
>>> dwc->gadget_driver->disconnect)
>>> usb_gadget_udc_stop(udc);
>>> udc->driver->unbind(udc->gadget);
>>> 
>>> dwc->gadget_driver->disconnect(>gadget);
>>> 
>>> UDC drivers typically set their gadget driver pointer to NULL in 
>>> udc_stop
>>> and check for it before calling into the gadget driver. To fix the 
>>> issue
>>> above every udc driver could apply a lock around this.
>>> 
>>> If you see the need for having this or another solutions I can provide
>>> further patches. This patch could also just serve as a base for 
>>> discussion
>>> if someone knows a smarter solution.
>>> 
>>> I saw this problem causing a panic on hikey960 board and provided a 
>>> quick
>>> workaround for the same problem here:
>>> https://android-review.googlesource.com/#/c/kernel/common/+/457476/
>>> (panic log in the commit message of the linked patch)
>>> ---
>>>  drivers/usb/gadget/udc/core.c | 6 +-
>>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>> 
>>> diff --git a/drivers/usb/gadget/udc/core.c 
>>> b/drivers/usb/gadget/udc/core.c
>>> index efce68e9a8e0..8155468afc0d 100644
>>> --- a/drivers/usb/gadget/udc/core.c
>>> +++ b/drivers/usb/gadget/udc/core.c
>>> @@ -1234,8 +1234,12 @@ static void usb_gadget_remove_driver(struct 
>>> usb_udc *udc)
>>> 
>>> usb_gadget_disconnect(udc->gadget);
>>> udc->driver->disconnect(udc->gadget);
>>> -   udc->driver->unbind(udc->gadget);
>>> +   /* udc_stop needs to be called before gadget driver unbind to 
>>> prevent
>>> +* udc driver calls into gadget driver after unbind which could 
>>> cause
>>> +* a nullptr exception.
>>> +*/
>>> usb_gadget_udc_stop(udc);
>>> +   udc->driver->unbind(udc->gadget);
>> 
>> This patch is incorrect, it will prevent us from giving back requests 
>> to
>> gadget driver properly. ->unbind() has to happen before ->udc_stop().
>
> Do you mean after udc_stop the udc driver can not call the gadget driver
> anymore? If not, I did not got your point, sorry for that. Can you 
> please
> help me out? Would the changed order raise another issue I'm not aware 
> of?

right, ->udc_stop() is supposed to completely teardown the USB
controller, including disabling interrupts and all. The only thing it
_can_ do from ->udc_stop() would be giving back any pending requests
that were left (which would cause req->complete() to be called with an
error status). But even that is unlikely in the case you mention since
->unbind() was already called.

> If I understood you correctly, without this patch udc driver can not 
> call
> the gadget driver back as well, because this would result in a NULL ptr
> dereference, as unbind() sets drvdata to NULL.
>
> In any case the race described in my original message can still happen,
> regardless of the order of udc_stop and unbind. But with this patch the
> needed locking could easily done within the udc driver only. Without, 
> the
> lock needs to be acquired before udc->driver->unbind(udc->gadget) and
> released after usb_gadget_udc_stop(). Otherwise an ISR of the udc driver
> trying to call into the gadget driver could do this after gadget driver
> already unbound.

right

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: gadget: udc: udc_stop before gadget unbind

[Felipe Balbi]

Hi,

Danilo Krummrich  writes:
> thanks for reviewing.

np :-)

> On 2017-08-15 12:03, Felipe Balbi wrote:
>> Hi,
>> 
>> Danilo Krummrich  writes:
>>> udc_stop needs to be called before gadget driver unbind. Otherwise it
>>> might happen that udc drivers still call into the gadget driver (e.g.
>>> to reset gadget after OTG event). If this happens it is likely to get
>>> panics from gadget driver dereferencing NULL ptr, as gadget's drvdata
>>> is set to NULL on unbind.
>> 
>> seems like the problem here is with the OTG layer, not UDC core.
>> 
> I mentioned this just as example, it can happen whenever a UDC driver 
> calls
> the gadget driver (e.g. by calling usb_gadget_udc_reset() in ISR) after 
> gadget
> drivers unbind() was called already (e.g. by gadget configfs).
> If this happens gadget drivers drvdata was already set to NULL by 
> unbind()
> and reset() could result into a NULL ptr exception.
> Therefore my assumption was that it needs to be prevented that the 
> gadget
> driver is getting called after unbind.

We have a known problem in the design of the gadget API that causes this
races but we couldn't come up with a solution yet :-)

Inverting these two calls is not the correct way to go about this :-)

>>> Signed-off-by: Danilo Krummrich 
>>> ---
>>> Actually there could still be a race:
>>> (CPU1 code taken from dwc3 drivers dwc3_disconnect_gadget() as 
>>> exsample)
>>> 
>>> CPU0CPU1
>>> 
>>> usb_gadget_disconnect(udc->gadget);
>>> udc->driver->disconnect(udc->gadget);
>>> if (dwc->gadget_driver && 
>>> dwc->gadget_driver->disconnect)
>>> usb_gadget_udc_stop(udc);
>>> udc->driver->unbind(udc->gadget);
>>> 
>>> dwc->gadget_driver->disconnect(>gadget);
>>> 
>>> UDC drivers typically set their gadget driver pointer to NULL in 
>>> udc_stop
>>> and check for it before calling into the gadget driver. To fix the 
>>> issue
>>> above every udc driver could apply a lock around this.
>>> 
>>> If you see the need for having this or another solutions I can provide
>>> further patches. This patch could also just serve as a base for 
>>> discussion
>>> if someone knows a smarter solution.
>>> 
>>> I saw this problem causing a panic on hikey960 board and provided a 
>>> quick
>>> workaround for the same problem here:
>>> https://android-review.googlesource.com/#/c/kernel/common/+/457476/
>>> (panic log in the commit message of the linked patch)
>>> ---
>>>  drivers/usb/gadget/udc/core.c | 6 +-
>>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>> 
>>> diff --git a/drivers/usb/gadget/udc/core.c 
>>> b/drivers/usb/gadget/udc/core.c
>>> index efce68e9a8e0..8155468afc0d 100644
>>> --- a/drivers/usb/gadget/udc/core.c
>>> +++ b/drivers/usb/gadget/udc/core.c
>>> @@ -1234,8 +1234,12 @@ static void usb_gadget_remove_driver(struct 
>>> usb_udc *udc)
>>> 
>>> usb_gadget_disconnect(udc->gadget);
>>> udc->driver->disconnect(udc->gadget);
>>> -   udc->driver->unbind(udc->gadget);
>>> +   /* udc_stop needs to be called before gadget driver unbind to 
>>> prevent
>>> +* udc driver calls into gadget driver after unbind which could 
>>> cause
>>> +* a nullptr exception.
>>> +*/
>>> usb_gadget_udc_stop(udc);
>>> +   udc->driver->unbind(udc->gadget);
>> 
>> This patch is incorrect, it will prevent us from giving back requests 
>> to
>> gadget driver properly. ->unbind() has to happen before ->udc_stop().
>
> Do you mean after udc_stop the udc driver can not call the gadget driver
> anymore? If not, I did not got your point, sorry for that. Can you 
> please
> help me out? Would the changed order raise another issue I'm not aware 
> of?

right, ->udc_stop() is supposed to completely teardown the USB
controller, including disabling interrupts and all. The only thing it
_can_ do from ->udc_stop() would be giving back any pending requests
that were left (which would cause req->complete() to be called with an
error status). But even that is unlikely in the case you mention since
->unbind() was already called.

> If I understood you correctly, without this patch udc driver can not 
> call
> the gadget driver back as well, because this would result in a NULL ptr
> dereference, as unbind() sets drvdata to NULL.
>
> In any case the race described in my original message can still happen,
> regardless of the order of udc_stop and unbind. But with this patch the
> needed locking could easily done within the udc driver only. Without, 
> the
> lock needs to be acquired before udc->driver->unbind(udc->gadget) and
> released after usb_gadget_udc_stop(). Otherwise an ISR of the udc driver
> trying to call into the gadget driver could do this after gadget driver
> already unbound.

right

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: gadget: udc: udc_stop before gadget unbind

[Danilo Krummrich]

Hi,

thanks for reviewing.

On 2017-08-15 12:03, Felipe Balbi wrote:

Hi,

Danilo Krummrich  writes:

udc_stop needs to be called before gadget driver unbind. Otherwise it
might happen that udc drivers still call into the gadget driver (e.g.
to reset gadget after OTG event). If this happens it is likely to get
panics from gadget driver dereferencing NULL ptr, as gadget's drvdata
is set to NULL on unbind.


seems like the problem here is with the OTG layer, not UDC core.

I mentioned this just as example, it can happen whenever a UDC driver 
calls
the gadget driver (e.g. by calling usb_gadget_udc_reset() in ISR) after 
gadget

drivers unbind() was called already (e.g. by gadget configfs).
If this happens gadget drivers drvdata was already set to NULL by 
unbind()

and reset() could result into a NULL ptr exception.
Therefore my assumption was that it needs to be prevented that the 
gadget

driver is getting called after unbind.

Signed-off-by: Danilo Krummrich 
---
Actually there could still be a race:
(CPU1 code taken from dwc3 drivers dwc3_disconnect_gadget() as 
exsample)


CPU0CPU1

usb_gadget_disconnect(udc->gadget);
udc->driver->disconnect(udc->gadget);
if (dwc->gadget_driver && 
dwc->gadget_driver->disconnect)
usb_gadget_udc_stop(udc);
udc->driver->unbind(udc->gadget);

dwc->gadget_driver->disconnect(>gadget);

UDC drivers typically set their gadget driver pointer to NULL in 
udc_stop
and check for it before calling into the gadget driver. To fix the 
issue

above every udc driver could apply a lock around this.

If you see the need for having this or another solutions I can provide
further patches. This patch could also just serve as a base for 
discussion

if someone knows a smarter solution.

I saw this problem causing a panic on hikey960 board and provided a 
quick

workaround for the same problem here:
https://android-review.googlesource.com/#/c/kernel/common/+/457476/
(panic log in the commit message of the linked patch)
---
 drivers/usb/gadget/udc/core.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/udc/core.c 
b/drivers/usb/gadget/udc/core.c

index efce68e9a8e0..8155468afc0d 100644
--- a/drivers/usb/gadget/udc/core.c
+++ b/drivers/usb/gadget/udc/core.c
@@ -1234,8 +1234,12 @@ static void usb_gadget_remove_driver(struct 
usb_udc *udc)


usb_gadget_disconnect(udc->gadget);
udc->driver->disconnect(udc->gadget);
-   udc->driver->unbind(udc->gadget);
+	/* udc_stop needs to be called before gadget driver unbind to 
prevent
+	 * udc driver calls into gadget driver after unbind which could 
cause

+* a nullptr exception.
+*/
usb_gadget_udc_stop(udc);
+   udc->driver->unbind(udc->gadget);


This patch is incorrect, it will prevent us from giving back requests 
to

gadget driver properly. ->unbind() has to happen before ->udc_stop().


Do you mean after udc_stop the udc driver can not call the gadget driver
anymore? If not, I did not got your point, sorry for that. Can you 
please
help me out? Would the changed order raise another issue I'm not aware 
of?


If I understood you correctly, without this patch udc driver can not 
call

the gadget driver back as well, because this would result in a NULL ptr
dereference, as unbind() sets drvdata to NULL.

In any case the race described in my original message can still happen,
regardless of the order of udc_stop and unbind. But with this patch the
needed locking could easily done within the udc driver only. Without, 
the

lock needs to be acquired before udc->driver->unbind(udc->gadget) and
released after usb_gadget_udc_stop(). Otherwise an ISR of the udc driver
trying to call into the gadget driver could do this after gadget driver
already unbound.

Regards,
Danilo

Re: [PATCH] usb: gadget: udc: udc_stop before gadget unbind

[Danilo Krummrich]

Hi,

thanks for reviewing.

On 2017-08-15 12:03, Felipe Balbi wrote:

Hi,

Danilo Krummrich  writes:

udc_stop needs to be called before gadget driver unbind. Otherwise it
might happen that udc drivers still call into the gadget driver (e.g.
to reset gadget after OTG event). If this happens it is likely to get
panics from gadget driver dereferencing NULL ptr, as gadget's drvdata
is set to NULL on unbind.


seems like the problem here is with the OTG layer, not UDC core.

I mentioned this just as example, it can happen whenever a UDC driver 
calls
the gadget driver (e.g. by calling usb_gadget_udc_reset() in ISR) after 
gadget

drivers unbind() was called already (e.g. by gadget configfs).
If this happens gadget drivers drvdata was already set to NULL by 
unbind()

and reset() could result into a NULL ptr exception.
Therefore my assumption was that it needs to be prevented that the 
gadget

driver is getting called after unbind.

Signed-off-by: Danilo Krummrich 
---
Actually there could still be a race:
(CPU1 code taken from dwc3 drivers dwc3_disconnect_gadget() as 
exsample)


CPU0CPU1

usb_gadget_disconnect(udc->gadget);
udc->driver->disconnect(udc->gadget);
if (dwc->gadget_driver && 
dwc->gadget_driver->disconnect)
usb_gadget_udc_stop(udc);
udc->driver->unbind(udc->gadget);

dwc->gadget_driver->disconnect(>gadget);

UDC drivers typically set their gadget driver pointer to NULL in 
udc_stop
and check for it before calling into the gadget driver. To fix the 
issue

above every udc driver could apply a lock around this.

If you see the need for having this or another solutions I can provide
further patches. This patch could also just serve as a base for 
discussion

if someone knows a smarter solution.

I saw this problem causing a panic on hikey960 board and provided a 
quick

workaround for the same problem here:
https://android-review.googlesource.com/#/c/kernel/common/+/457476/
(panic log in the commit message of the linked patch)
---
 drivers/usb/gadget/udc/core.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/udc/core.c 
b/drivers/usb/gadget/udc/core.c

index efce68e9a8e0..8155468afc0d 100644
--- a/drivers/usb/gadget/udc/core.c
+++ b/drivers/usb/gadget/udc/core.c
@@ -1234,8 +1234,12 @@ static void usb_gadget_remove_driver(struct 
usb_udc *udc)


usb_gadget_disconnect(udc->gadget);
udc->driver->disconnect(udc->gadget);
-   udc->driver->unbind(udc->gadget);
+	/* udc_stop needs to be called before gadget driver unbind to 
prevent
+	 * udc driver calls into gadget driver after unbind which could 
cause

+* a nullptr exception.
+*/
usb_gadget_udc_stop(udc);
+   udc->driver->unbind(udc->gadget);


This patch is incorrect, it will prevent us from giving back requests 
to

gadget driver properly. ->unbind() has to happen before ->udc_stop().


Do you mean after udc_stop the udc driver can not call the gadget driver
anymore? If not, I did not got your point, sorry for that. Can you 
please
help me out? Would the changed order raise another issue I'm not aware 
of?


If I understood you correctly, without this patch udc driver can not 
call

the gadget driver back as well, because this would result in a NULL ptr
dereference, as unbind() sets drvdata to NULL.

In any case the race described in my original message can still happen,
regardless of the order of udc_stop and unbind. But with this patch the
needed locking could easily done within the udc driver only. Without, 
the

lock needs to be acquired before udc->driver->unbind(udc->gadget) and
released after usb_gadget_udc_stop(). Otherwise an ISR of the udc driver
trying to call into the gadget driver could do this after gadget driver
already unbound.

Regards,
Danilo

Re: [PATCH] usb: gadget: udc: udc_stop before gadget unbind

[Felipe Balbi]

Hi,

Danilo Krummrich  writes:
> udc_stop needs to be called before gadget driver unbind. Otherwise it
> might happen that udc drivers still call into the gadget driver (e.g.
> to reset gadget after OTG event). If this happens it is likely to get
> panics from gadget driver dereferencing NULL ptr, as gadget's drvdata
> is set to NULL on unbind.

seems like the problem here is with the OTG layer, not UDC core.

> Signed-off-by: Danilo Krummrich 
> ---
> Actually there could still be a race:
> (CPU1 code taken from dwc3 drivers dwc3_disconnect_gadget() as exsample)
>
> CPU0  CPU1
>   
> usb_gadget_disconnect(udc->gadget);
> udc->driver->disconnect(udc->gadget);
>   if (dwc->gadget_driver && 
> dwc->gadget_driver->disconnect)
> usb_gadget_udc_stop(udc);
> udc->driver->unbind(udc->gadget);
>   
> dwc->gadget_driver->disconnect(>gadget);
>
> UDC drivers typically set their gadget driver pointer to NULL in udc_stop
> and check for it before calling into the gadget driver. To fix the issue
> above every udc driver could apply a lock around this.
>
> If you see the need for having this or another solutions I can provide
> further patches. This patch could also just serve as a base for discussion
> if someone knows a smarter solution.
>
> I saw this problem causing a panic on hikey960 board and provided a quick
> workaround for the same problem here:
> https://android-review.googlesource.com/#/c/kernel/common/+/457476/
> (panic log in the commit message of the linked patch)
> ---
>  drivers/usb/gadget/udc/core.c | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
> index efce68e9a8e0..8155468afc0d 100644
> --- a/drivers/usb/gadget/udc/core.c
> +++ b/drivers/usb/gadget/udc/core.c
> @@ -1234,8 +1234,12 @@ static void usb_gadget_remove_driver(struct usb_udc 
> *udc)
>  
>   usb_gadget_disconnect(udc->gadget);
>   udc->driver->disconnect(udc->gadget);
> - udc->driver->unbind(udc->gadget);
> + /* udc_stop needs to be called before gadget driver unbind to prevent
> +  * udc driver calls into gadget driver after unbind which could cause
> +  * a nullptr exception.
> +  */
>   usb_gadget_udc_stop(udc);
> + udc->driver->unbind(udc->gadget);

This patch is incorrect, it will prevent us from giving back requests to
gadget driver properly. ->unbind() has to happen before ->udc_stop().

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: gadget: udc: udc_stop before gadget unbind

[Felipe Balbi]

Hi,

Danilo Krummrich  writes:
> udc_stop needs to be called before gadget driver unbind. Otherwise it
> might happen that udc drivers still call into the gadget driver (e.g.
> to reset gadget after OTG event). If this happens it is likely to get
> panics from gadget driver dereferencing NULL ptr, as gadget's drvdata
> is set to NULL on unbind.

seems like the problem here is with the OTG layer, not UDC core.

> Signed-off-by: Danilo Krummrich 
> ---
> Actually there could still be a race:
> (CPU1 code taken from dwc3 drivers dwc3_disconnect_gadget() as exsample)
>
> CPU0  CPU1
>   
> usb_gadget_disconnect(udc->gadget);
> udc->driver->disconnect(udc->gadget);
>   if (dwc->gadget_driver && 
> dwc->gadget_driver->disconnect)
> usb_gadget_udc_stop(udc);
> udc->driver->unbind(udc->gadget);
>   
> dwc->gadget_driver->disconnect(>gadget);
>
> UDC drivers typically set their gadget driver pointer to NULL in udc_stop
> and check for it before calling into the gadget driver. To fix the issue
> above every udc driver could apply a lock around this.
>
> If you see the need for having this or another solutions I can provide
> further patches. This patch could also just serve as a base for discussion
> if someone knows a smarter solution.
>
> I saw this problem causing a panic on hikey960 board and provided a quick
> workaround for the same problem here:
> https://android-review.googlesource.com/#/c/kernel/common/+/457476/
> (panic log in the commit message of the linked patch)
> ---
>  drivers/usb/gadget/udc/core.c | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
> index efce68e9a8e0..8155468afc0d 100644
> --- a/drivers/usb/gadget/udc/core.c
> +++ b/drivers/usb/gadget/udc/core.c
> @@ -1234,8 +1234,12 @@ static void usb_gadget_remove_driver(struct usb_udc 
> *udc)
>  
>   usb_gadget_disconnect(udc->gadget);
>   udc->driver->disconnect(udc->gadget);
> - udc->driver->unbind(udc->gadget);
> + /* udc_stop needs to be called before gadget driver unbind to prevent
> +  * udc driver calls into gadget driver after unbind which could cause
> +  * a nullptr exception.
> +  */
>   usb_gadget_udc_stop(udc);
> + udc->driver->unbind(udc->gadget);

This patch is incorrect, it will prevent us from giving back requests to
gadget driver properly. ->unbind() has to happen before ->udc_stop().

-- 
balbi


signature.asc
Description: PGP signature

[PATCH] usb: gadget: udc: udc_stop before gadget unbind

[Danilo Krummrich]

udc_stop needs to be called before gadget driver unbind. Otherwise it
might happen that udc drivers still call into the gadget driver (e.g.
to reset gadget after OTG event). If this happens it is likely to get
panics from gadget driver dereferencing NULL ptr, as gadget's drvdata
is set to NULL on unbind.

Signed-off-by: Danilo Krummrich 
---
Actually there could still be a race:
(CPU1 code taken from dwc3 drivers dwc3_disconnect_gadget() as exsample)

CPU0CPU1

usb_gadget_disconnect(udc->gadget);
udc->driver->disconnect(udc->gadget);
if (dwc->gadget_driver && 
dwc->gadget_driver->disconnect)
usb_gadget_udc_stop(udc);
udc->driver->unbind(udc->gadget);

dwc->gadget_driver->disconnect(>gadget);

UDC drivers typically set their gadget driver pointer to NULL in udc_stop
and check for it before calling into the gadget driver. To fix the issue
above every udc driver could apply a lock around this.

If you see the need for having this or another solutions I can provide
further patches. This patch could also just serve as a base for discussion
if someone knows a smarter solution.

I saw this problem causing a panic on hikey960 board and provided a quick
workaround for the same problem here:
https://android-review.googlesource.com/#/c/kernel/common/+/457476/
(panic log in the commit message of the linked patch)
---
 drivers/usb/gadget/udc/core.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
index efce68e9a8e0..8155468afc0d 100644
--- a/drivers/usb/gadget/udc/core.c
+++ b/drivers/usb/gadget/udc/core.c
@@ -1234,8 +1234,12 @@ static void usb_gadget_remove_driver(struct usb_udc *udc)
 
usb_gadget_disconnect(udc->gadget);
udc->driver->disconnect(udc->gadget);
-   udc->driver->unbind(udc->gadget);
+   /* udc_stop needs to be called before gadget driver unbind to prevent
+* udc driver calls into gadget driver after unbind which could cause
+* a nullptr exception.
+*/
usb_gadget_udc_stop(udc);
+   udc->driver->unbind(udc->gadget);
 
udc->driver = NULL;
udc->dev.driver = NULL;
-- 
2.14.1

[PATCH] usb: gadget: udc: udc_stop before gadget unbind

[Danilo Krummrich]

udc_stop needs to be called before gadget driver unbind. Otherwise it
might happen that udc drivers still call into the gadget driver (e.g.
to reset gadget after OTG event). If this happens it is likely to get
panics from gadget driver dereferencing NULL ptr, as gadget's drvdata
is set to NULL on unbind.

Signed-off-by: Danilo Krummrich 
---
Actually there could still be a race:
(CPU1 code taken from dwc3 drivers dwc3_disconnect_gadget() as exsample)

CPU0CPU1

usb_gadget_disconnect(udc->gadget);
udc->driver->disconnect(udc->gadget);
if (dwc->gadget_driver && 
dwc->gadget_driver->disconnect)
usb_gadget_udc_stop(udc);
udc->driver->unbind(udc->gadget);

dwc->gadget_driver->disconnect(>gadget);

UDC drivers typically set their gadget driver pointer to NULL in udc_stop
and check for it before calling into the gadget driver. To fix the issue
above every udc driver could apply a lock around this.

If you see the need for having this or another solutions I can provide
further patches. This patch could also just serve as a base for discussion
if someone knows a smarter solution.

I saw this problem causing a panic on hikey960 board and provided a quick
workaround for the same problem here:
https://android-review.googlesource.com/#/c/kernel/common/+/457476/
(panic log in the commit message of the linked patch)
---
 drivers/usb/gadget/udc/core.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
index efce68e9a8e0..8155468afc0d 100644
--- a/drivers/usb/gadget/udc/core.c
+++ b/drivers/usb/gadget/udc/core.c
@@ -1234,8 +1234,12 @@ static void usb_gadget_remove_driver(struct usb_udc *udc)
 
usb_gadget_disconnect(udc->gadget);
udc->driver->disconnect(udc->gadget);
-   udc->driver->unbind(udc->gadget);
+   /* udc_stop needs to be called before gadget driver unbind to prevent
+* udc driver calls into gadget driver after unbind which could cause
+* a nullptr exception.
+*/
usb_gadget_udc_stop(udc);
+   udc->driver->unbind(udc->gadget);
 
udc->driver = NULL;
udc->dev.driver = NULL;
-- 
2.14.1