diff mbox series

usb: gadget: f_uac2: fixup feedback endpoint stop

Message ID 20210827075853.266912-1-jbrunet@baylibre.com
State New
Headers show
Series usb: gadget: f_uac2: fixup feedback endpoint stop | expand

Commit Message

Jerome Brunet Aug. 27, 2021, 7:58 a.m. UTC
When the uac2 function is stopped, there seems to be an issue reported on
some platforms (Intel Merrifield at least)

BUG: kernel NULL pointer dereference, address: 0000000000000008
...
RIP: 0010:dwc3_gadget_del_and_unmap_request+0x19/0xe0
...
Call Trace:
 dwc3_remove_requests.constprop.0+0x12f/0x170
 __dwc3_gadget_ep_disable+0x7a/0x160
 dwc3_gadget_ep_disable+0x3d/0xd0
 usb_ep_disable+0x1c/0x70
 u_audio_stop_capture+0x79/0x120 [u_audio]
 afunc_set_alt+0x73/0x80 [usb_f_uac2]
 composite_setup+0x224/0x1b90 [libcomposite]

The issue happens only when the gadget is using the sync type "async", not
"adaptive". This indicates that problem is coming from the feedback
endpoint, which is only used with async synchronization mode.

The problem is that request is freed regardless of usb_ep_dequeue(), which
ends up badly if the request is not actually dequeued yet.

Update the feedback endpoint free function to release the endpoint the same
way it is done for the data endpoint, which takes care of the problem.

Reported-by: Ferry Toth <ftoth@exalondelft.nl>
Tested-by: Ferry Toth <ftoth@exalondelft.nl>

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>

---
 Hi Felipe,

 This solves the issue reported here [0] and makes revert [1]
 unnecessary.

 [0]: https://lore.kernel.org/r/20210824201433.11385-1-ftoth@exalondelft.nl
 [1]: https://lore.kernel.org/r/20210826185739.3868-1-ftoth@exalondelft.nl

 drivers/usb/gadget/function/u_audio.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

-- 
2.33.0

Comments

Felipe Balbi Aug. 27, 2021, 8:05 a.m. UTC | #1
Hi,

Jerome Brunet <jbrunet@baylibre.com> writes:

> When the uac2 function is stopped, there seems to be an issue reported on

> some platforms (Intel Merrifield at least)

>

> BUG: kernel NULL pointer dereference, address: 0000000000000008

> ...

> RIP: 0010:dwc3_gadget_del_and_unmap_request+0x19/0xe0

> ...

> Call Trace:

>  dwc3_remove_requests.constprop.0+0x12f/0x170

>  __dwc3_gadget_ep_disable+0x7a/0x160

>  dwc3_gadget_ep_disable+0x3d/0xd0

>  usb_ep_disable+0x1c/0x70

>  u_audio_stop_capture+0x79/0x120 [u_audio]

>  afunc_set_alt+0x73/0x80 [usb_f_uac2]

>  composite_setup+0x224/0x1b90 [libcomposite]

>

> The issue happens only when the gadget is using the sync type "async", not

> "adaptive". This indicates that problem is coming from the feedback

> endpoint, which is only used with async synchronization mode.

>

> The problem is that request is freed regardless of usb_ep_dequeue(), which

> ends up badly if the request is not actually dequeued yet.

>

> Update the feedback endpoint free function to release the endpoint the same

> way it is done for the data endpoint, which takes care of the problem.

>

> Reported-by: Ferry Toth <ftoth@exalondelft.nl>

> Tested-by: Ferry Toth <ftoth@exalondelft.nl>

> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>

> ---

>  Hi Felipe,

>

>  This solves the issue reported here [0] and makes revert [1]

>  unnecessary.


awesome work! Thanks

Acked-by: Felipe Balbi <balbi@kernel.org>


-- 
balbi
Jerome Brunet Aug. 27, 2021, 9:26 a.m. UTC | #2
On Fri 27 Aug 2021 at 11:05, Felipe Balbi <balbi@kernel.org> wrote:

> Hi,

>

> Jerome Brunet <jbrunet@baylibre.com> writes:

>

>> When the uac2 function is stopped, there seems to be an issue reported on

>> some platforms (Intel Merrifield at least)

>>

>> BUG: kernel NULL pointer dereference, address: 0000000000000008

>> ...

>> RIP: 0010:dwc3_gadget_del_and_unmap_request+0x19/0xe0

>> ...

>> Call Trace:

>>  dwc3_remove_requests.constprop.0+0x12f/0x170

>>  __dwc3_gadget_ep_disable+0x7a/0x160

>>  dwc3_gadget_ep_disable+0x3d/0xd0

>>  usb_ep_disable+0x1c/0x70

>>  u_audio_stop_capture+0x79/0x120 [u_audio]

>>  afunc_set_alt+0x73/0x80 [usb_f_uac2]

>>  composite_setup+0x224/0x1b90 [libcomposite]

>>

>> The issue happens only when the gadget is using the sync type "async", not

>> "adaptive". This indicates that problem is coming from the feedback

>> endpoint, which is only used with async synchronization mode.

>>

>> The problem is that request is freed regardless of usb_ep_dequeue(), which

>> ends up badly if the request is not actually dequeued yet.

>>

>> Update the feedback endpoint free function to release the endpoint the same

>> way it is done for the data endpoint, which takes care of the problem.

>>

>> Reported-by: Ferry Toth <ftoth@exalondelft.nl>

>> Tested-by: Ferry Toth <ftoth@exalondelft.nl>

>> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>

>> ---

>>  Hi Felipe,

>>

>>  This solves the issue reported here [0] and makes revert [1]

>>  unnecessary.

>

> awesome work! Thanks

>

> Acked-by: Felipe Balbi <balbi@kernel.org>


Forgot the fixes tag

Fixes: 24f779dac8f3 ("usb: gadget: f_uac2/u_audio: add feedback endpoint support")
diff mbox series

Patch

diff --git a/drivers/usb/gadget/function/u_audio.c b/drivers/usb/gadget/function/u_audio.c
index 018dd0978995..63d9340f008e 100644
--- a/drivers/usb/gadget/function/u_audio.c
+++ b/drivers/usb/gadget/function/u_audio.c
@@ -230,7 +230,13 @@  static void u_audio_iso_fback_complete(struct usb_ep *ep,
 	int status = req->status;
 
 	/* i/f shutting down */
-	if (!prm->fb_ep_enabled || req->status == -ESHUTDOWN)
+	if (!prm->fb_ep_enabled) {
+		kfree(req->buf);
+		usb_ep_free_request(ep, req);
+		return;
+	}
+
+	if (req->status == -ESHUTDOWN)
 		return;
 
 	/*
@@ -421,9 +427,10 @@  static inline void free_ep_fback(struct uac_rtd_params *prm, struct usb_ep *ep)
 	prm->fb_ep_enabled = false;
 
 	if (prm->req_fback) {
-		usb_ep_dequeue(ep, prm->req_fback);
-		kfree(prm->req_fback->buf);
-		usb_ep_free_request(ep, prm->req_fback);
+		if (usb_ep_dequeue(ep, prm->req_fback)) {
+			kfree(prm->req_fback->buf);
+			usb_ep_free_request(ep, prm->req_fback);
+		}
 		prm->req_fback = NULL;
 	}