| Message ID | 20210827092927.366482-1-jbrunet@baylibre.com |
|---|---|
| State | New |
| Headers | show |
| Series | usb: gadget: u_audio: fix race condition on endpoint stop | expand |
Jerome Brunet wrote: > 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 > 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__); > } >
On Fri 27 Aug 2021 at 23:59, Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote: > Jerome Brunet wrote: >> 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()? > Yes, it is what I intended. It's a quick way to address the problem you have reported. > 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? I can see that it would be better. It would use the framework the way it was intended which is certainly better. I just can't do it right now. > > BR, > Thinh > >> 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__); >> } >>
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; + 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__); }
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(-) -- 2.33.0