Re: [PATCH] USB: dwc3: fix memory leak with using debugfs_lookup()

[Thinh Nguyen]

On Thu, Feb 02, 2023, Greg Kroah-Hartman wrote:
> When calling debugfs_lookup() the result must have dput() called on it,
> otherwise the memory will leak over time.  To make things simpler, just
> call debugfs_lookup_and_remove() instead which handles all of the logic
> at once.
> 
> Note, the root dentry for the debugfs directory for the device needs to
> be saved so we don't have to keep looking it up, which required a bit
> more refactoring to properly create and remove it when needed.
> 
> Reported-by: Bruce Chen <bruce.chen@xxxxxxxxxx>
> Reported-by: Cixi Geng <cixi.geng1@xxxxxxxxxx>
> Tested-by: Cixi Geng <gengcixi@xxxxxxxxx>
> Cc: Thinh Nguyen <Thinh.Nguyen@xxxxxxxxxxxx>
> Cc: linux-usb@xxxxxxxxxxxxxxx
> Cc: linux-kernel@xxxxxxxxxxxxxxx
> Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> ---
>  drivers/usb/dwc3/core.h    |  2 ++
>  drivers/usb/dwc3/debug.h   |  3 +++
>  drivers/usb/dwc3/debugfs.c | 19 ++++++++-----------
>  drivers/usb/dwc3/gadget.c  |  4 +---
>  4 files changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 8f9959ba9fd4..582ebd9cf9c2 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -1117,6 +1117,7 @@ struct dwc3_scratchpad_array {
>   *		     address.
>   * @num_ep_resized: carries the current number endpoints which have had its tx
>   *		    fifo resized.
> + * @debug_root: root debugfs directory for this device to put its files in.
>   */
>  struct dwc3 {
>  	struct work_struct	drd_work;
> @@ -1332,6 +1333,7 @@ struct dwc3 {
>  	int			max_cfg_eps;
>  	int			last_fifo_depth;
>  	int			num_ep_resized;
> +	struct dentry		*debug_root;
>  };
>  
>  #define INCRX_BURST_MODE 0
> diff --git a/drivers/usb/dwc3/debug.h b/drivers/usb/dwc3/debug.h
> index 48b44b88dc25..8bb2c9e3b9ac 100644
> --- a/drivers/usb/dwc3/debug.h
> +++ b/drivers/usb/dwc3/debug.h
> @@ -414,11 +414,14 @@ static inline const char *dwc3_gadget_generic_cmd_status_string(int status)
>  
>  #ifdef CONFIG_DEBUG_FS
>  extern void dwc3_debugfs_create_endpoint_dir(struct dwc3_ep *dep);
> +extern void dwc3_debugfs_remove_endpoint_dir(struct dwc3_ep *dep);
>  extern void dwc3_debugfs_init(struct dwc3 *d);
>  extern void dwc3_debugfs_exit(struct dwc3 *d);
>  #else
>  static inline void dwc3_debugfs_create_endpoint_dir(struct dwc3_ep *dep)
>  {  }
> +static inline void dwc3_debugfs_remove_endpoint_dir(struct dwc3_ep *dep)
> +{  }
>  static inline void dwc3_debugfs_init(struct dwc3 *d)
>  {  }
>  static inline void dwc3_debugfs_exit(struct dwc3 *d)
> diff --git a/drivers/usb/dwc3/debugfs.c b/drivers/usb/dwc3/debugfs.c
> index f2b7675c7f62..850df0e6bcab 100644
> --- a/drivers/usb/dwc3/debugfs.c
> +++ b/drivers/usb/dwc3/debugfs.c
> @@ -873,27 +873,23 @@ static const struct dwc3_ep_file_map dwc3_ep_file_map[] = {
>  	{ "GDBGEPINFO", &dwc3_ep_info_register_fops, },
>  };
>  
> -static void dwc3_debugfs_create_endpoint_files(struct dwc3_ep *dep,
> -		struct dentry *parent)
> +void dwc3_debugfs_create_endpoint_dir(struct dwc3_ep *dep)
>  {
> +	struct dentry		*dir;
>  	int			i;
>  
> +	dir = debugfs_create_dir(dep->name, dep->dwc->debug_root);
>  	for (i = 0; i < ARRAY_SIZE(dwc3_ep_file_map); i++) {
>  		const struct file_operations *fops = dwc3_ep_file_map[i].fops;
>  		const char *name = dwc3_ep_file_map[i].name;
>  
> -		debugfs_create_file(name, 0444, parent, dep, fops);
> +		debugfs_create_file(name, 0444, dir, dep, fops);
>  	}
>  }
>  
> -void dwc3_debugfs_create_endpoint_dir(struct dwc3_ep *dep)
> +void dwc3_debugfs_remove_endpoint_dir(struct dwc3_ep *dep)
>  {
> -	struct dentry		*dir;
> -	struct dentry		*root;
> -
> -	root = debugfs_lookup(dev_name(dep->dwc->dev), usb_debug_root);
> -	dir = debugfs_create_dir(dep->name, root);
> -	dwc3_debugfs_create_endpoint_files(dep, dir);
> +	debugfs_lookup_and_remove(dep->name, dep->dwc->debug_root);
>  }
>  
>  void dwc3_debugfs_init(struct dwc3 *dwc)
> @@ -911,6 +907,7 @@ void dwc3_debugfs_init(struct dwc3 *dwc)
>  	dwc->regset->base = dwc->regs - DWC3_GLOBALS_REGS_START;
>  
>  	root = debugfs_create_dir(dev_name(dwc->dev), usb_debug_root);
> +	dwc->debug_root = root;
>  	debugfs_create_regset32("regdump", 0444, root, dwc->regset);
>  	debugfs_create_file("lsp_dump", 0644, root, dwc, &dwc3_lsp_fops);
>  
> @@ -929,6 +926,6 @@ void dwc3_debugfs_init(struct dwc3 *dwc)
>  
>  void dwc3_debugfs_exit(struct dwc3 *dwc)
>  {
> -	debugfs_remove(debugfs_lookup(dev_name(dwc->dev), usb_debug_root));
> +	debugfs_lookup_and_remove(dev_name(dwc->dev), usb_debug_root);
>  	kfree(dwc->regset);
>  }
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 89dcfac01235..3c63fa97a680 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -3194,9 +3194,7 @@ static void dwc3_gadget_free_endpoints(struct dwc3 *dwc)
>  			list_del(&dep->endpoint.ep_list);
>  		}
>  
> -		debugfs_remove_recursive(debugfs_lookup(dep->name,
> -				debugfs_lookup(dev_name(dep->dwc->dev),
> -					       usb_debug_root)));
> +		dwc3_debugfs_remove_endpoint_dir(dep);
>  		kfree(dep);
>  	}
>  }
> -- 
> 2.39.1
> 

Acked-by: Thinh Nguyen <Thinh.Nguyen@xxxxxxxxxxxx>

Thanks,
Thinh