Re: [PATCH] usb: gadget: u_audio: fix race condition on endpoint stop
From: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
Date: 2021-08-27 23:59:53
Also in:
lkml
Jerome Brunet wrote:
quoted hunk ↗ jump to hunk
If the endpoint completion callback is call right after the ep_enabled flag is cleared and before usb_ep_dequeue() is call, we could do a double free on the request and the associated buffer. Fix this by clearing ep_enabled after all the endpoint requests have been dequeued. Fixes: 7de8681be2cd ("usb: gadget: u_audio: Free requests only after callback") Reported-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> --- drivers/usb/gadget/function/u_audio.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)diff --git a/drivers/usb/gadget/function/u_audio.c b/drivers/usb/gadget/function/u_audio.c index 63d9340f008e..9e5c950612d0 100644 --- a/drivers/usb/gadget/function/u_audio.c +++ b/drivers/usb/gadget/function/u_audio.c@@ -394,8 +394,6 @@ static inline void free_ep(struct uac_rtd_params *prm, struct usb_ep *ep) if (!prm->ep_enabled) return; - prm->ep_enabled = false; - audio_dev = uac->audio_dev; params = &audio_dev->params;@@ -413,6 +411,8 @@ static inline void free_ep(struct uac_rtd_params *prm, struct usb_ep *ep) } } + prm->ep_enabled = false; +
Hm... this looks a little odd. If the cancelled request completes before prm->ep_enabled = false, then the audio driver will re-queue the request. It will eventually be freed later when the endpoint is disabled and when the controller driver completes and gives back any outstanding request. But is this what you intended? If it is, why even usb_ep_dequeue()? Also, another concern I have is I don't see any lock or memory barrier when writing/reading prm->ep_enabled. Are we always reading prm->ep_enabled in the right order as we expected? Would it be simpler to free the request base on the request completion status as suggested? BR, Thinh
quoted hunk ↗ jump to hunk
if (usb_ep_disable(ep)) dev_err(uac->card->dev, "%s:%d Error!\n", __func__, __LINE__); }@@ -424,8 +424,6 @@ static inline void free_ep_fback(struct uac_rtd_params *prm, struct usb_ep *ep) if (!prm->fb_ep_enabled) return; - prm->fb_ep_enabled = false; - if (prm->req_fback) { if (usb_ep_dequeue(ep, prm->req_fback)) { kfree(prm->req_fback->buf);@@ -434,6 +432,8 @@ static inline void free_ep_fback(struct uac_rtd_params *prm, struct usb_ep *ep) prm->req_fback = NULL; } + prm->fb_ep_enabled = false; + if (usb_ep_disable(ep)) dev_err(uac->card->dev, "%s:%d Error!\n", __func__, __LINE__); }