Re: [PATCH] usb: gadget: u_audio: fix race condition on endpoint stop

[Thinh Nguyen <Thinh.Nguyen@synopsys.com>]

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__);
>  }
>