Re: [PATCH] usb: gadget: configfs: Prevent double delete from purge_configs_funcs

[John Keeping <john@metanate.com>]

On Thu, Nov 17, 2022 at 11:05:48AM +0530, Udipto Goswami wrote:
> Currently the function purge_configs_funcs isn't protected by any lock
> So there is a potential race possible there id udc called composite_unbind
> and say at the same time the configfs_composite_bind also got executed.
> If bind fails and follows the error path, driver will end up calling the
> purge_configfs_funcs again, resulting in double free from func_list.
> 
> Fix this by introducing mutex gi->lock to make sure serial execution
> can be maintained. Also, usage of list_for_each_entry_safe is risky
> here since the tmp variable is still valid for the next iteration, in
> order to ensure list traversal is clean, use list_last_entry.

I don't understand how this can happen.

purge_configs_funcs() is called from the .bind and .unbind
usb_gadget_driver hooks, and those should not be able to run at the same
time - if bind has not succeeded, how does unbinding make sense?

> Fixes: 6cd0fe913879 ("usb: gadget: configfs: Preserve function ordering after bind failure")
> Signed-off-by: Udipto Goswami <quic_ugoswami@quicinc.com>
> ---
>  drivers/usb/gadget/configfs.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
> index 3a6b4926193e..f1ac87906897 100644
> --- a/drivers/usb/gadget/configfs.c
> +++ b/drivers/usb/gadget/configfs.c
> @@ -1246,14 +1246,14 @@ static void purge_configs_funcs(struct gadget_info *gi)
>  {
>  	struct usb_configuration	*c;
>  
> +	mutex_lock(&gi->lock);
>  	list_for_each_entry(c, &gi->cdev.configs, list) {
> -		struct usb_function *f, *tmp;
> +		struct usb_function *f;
>  		struct config_usb_cfg *cfg;
>  
>  		cfg = container_of(c, struct config_usb_cfg, c);
> -
> -		list_for_each_entry_safe_reverse(f, tmp, &c->functions, list) {
> -
> +		while (!list_empty(&c->functions)) {
> +			f = list_last_entry(&c->functions, struct usb_function, list);
>  			list_move(&f->list, &cfg->func_list);
>  			if (f->unbind) {
>  				dev_dbg(&gi->cdev.gadget->dev,
> @@ -1269,6 +1269,7 @@ static void purge_configs_funcs(struct gadget_info *gi)
>  		c->highspeed = 0;
>  		c->fullspeed = 0;
>  	}
> +	mutex_unlock(&gi->lock);
>  }
>  
>  static int configfs_composite_bind(struct usb_gadget *gadget,
> -- 
> 2.17.1
>