usb: gadget: udc: udc_stop before gadget unbind

PATCH In review Danilo Krummrich 2 participants 5 messages Aug 14, 2017 – Aug 15, 2017 USB

Share & tools

Danilo Krummrich

author

2017-08-14 16:37 UTC #0 lore raw

PATCH 1/1 usb: gadget: udc: udc_stop before gadget unbind

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

---
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(&dwc->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(-)

drivers/usb/gadget/udc/core.c +5 -1

index efce68e9a8e0..8155468afc0d 100644
@@ -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;

Signature · 1 line

2.14.1

Felipe Balbi

2017-08-15 10:03 UTC #1 lore raw

Hi,

Quoted 5 lines · 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.

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

Quoted 35 lines

Signed-off-by Danilo Krummrich <[email protected]>
---
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(&dwc->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(-)
drivers/usb/gadget/udc/core.c +0 -0
index efce68e9a8e0..8155468afc0d 100644

drivers/usb/gadget/udc/core.c static void usb_gadget_remove_driver(struct usb_udc *udc) · line 1242

Quoted 10 lines

drivers/usb/gadget/udc/core.c +5 -1

@@ -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().

Signature · 1 line

balbi

Danilo Krummrich

author

2017-08-15 11:51 UTC #2 lore raw

Hi,

thanks for reviewing.

Quoted 11 lines · Felipe Balbi

Hi,
Quoted 5 lines · 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.
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.

Quoted 37 lines

Quoted 37 lines
Signed-off-by Danilo Krummrich <[email protected]>
---
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(&dwc->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(-)
drivers/usb/gadget/udc/core.c +0 -0
index efce68e9a8e0..8155468afc0d 100644
@@ -1234,8 +1234,12 @@ static void usb_gadget_remove_driver(struct 
 usb_udc *udc)
 

Quoted 14 lines

drivers/usb/gadget/udc/core.c static void usb_gadget_remove_driver(struct usb_udc *udc) · line 1242
Quoted 11 lines
drivers/usb/gadget/udc/core.c +1 -1
 	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
drivers/usb/gadget/udc/core.c +1 -0
+	 * udc driver calls into gadget driver after unbind which could 
cause
drivers/usb/gadget/udc/core.c +3 -0
+	 * 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

Felipe Balbi

2017-08-15 12:02 UTC #3 lore raw

Hi,

Quoted 1 line · Danilo Krummrich

thanks for reviewing.

np :-)

Quoted 12 lines · Felipe Balbi

Quoted 11 lines
Hi,
Quoted 5 lines · 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.
seems like the problem here is with the OTG layer, not UDC core.

Quoted 11 lines

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 :-)

Quoted 59 lines

Quoted 43 lines
Quoted 43 lines
Signed-off-by Danilo Krummrich <[email protected]>
---
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(&dwc->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(-)
drivers/usb/gadget/udc/core.c +0 -0
index efce68e9a8e0..8155468afc0d 100644
@@ -1234,8 +1234,12 @@ static void usb_gadget_remove_driver(struct 
 usb_udc *udc)
 
Quoted 15 lines
drivers/usb/gadget/udc/core.c static void usb_gadget_remove_driver(struct usb_udc *udc) · line 1242
Quoted 11 lines
drivers/usb/gadget/udc/core.c +1 -1
 	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
drivers/usb/gadget/udc/core.c +1 -0
+	 * udc driver calls into gadget driver after unbind which could 
cause
drivers/usb/gadget/udc/core.c +3 -0
+	 * 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().

Quoted 5 lines

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.

Quoted 5 lines

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.

Quoted 8 lines

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

Signature · 1 line

balbi

Danilo Krummrich

author

2017-08-15 12:51 UTC #4 lore raw

Quoted 31 lines · Felipe Balbi

Hi,
Quoted 1 line · Danilo Krummrich
thanks for reviewing.
np :-)
Quoted 12 lines · Felipe Balbi
Quoted 11 lines
Hi,
Quoted 5 lines · 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.
seems like the problem here is with the OTG layer, not UDC core.
Quoted 11 lines
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.

Quoted 5 lines

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.

Quoted 65 lines

Quoted 59 lines
Quoted 43 lines
Quoted 43 lines
Signed-off-by Danilo Krummrich <[email protected]>
---
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(&dwc->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(-)
drivers/usb/gadget/udc/core.c +0 -0
index efce68e9a8e0..8155468afc0d 100644
@@ -1234,8 +1234,12 @@ static void usb_gadget_remove_driver(struct
 usb_udc *udc)
 
Quoted 15 lines
drivers/usb/gadget/udc/core.c static void usb_gadget_remove_driver(struct usb_udc *udc) · line 1242
Quoted 11 lines
drivers/usb/gadget/udc/core.c +1 -1
 	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
drivers/usb/gadget/udc/core.c +1 -0
+	 * udc driver calls into gadget driver after unbind which could
cause
drivers/usb/gadget/udc/core.c +3 -0
+	 * 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().
Quoted 5 lines
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?

Quoted 7 lines

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.

Quoted 14 lines

Quoted 5 lines
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.
Quoted 8 lines
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.

Quoted 1 line

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 make sense for you to have a workaround meanwhile (maybe not in mainline kernel, but somewhere else)? As e.g. on hikey960 board this causes panics very often.

Thanks,
Danilo