Re: [PATCH v3 1/2] usb: roles: fix NULL pointer issue when put module's reference

[Heikki Krogerus <heikki.krogerus@linux.intel.com>]

On Mon, Jan 29, 2024 at 05:37:38PM +0800, Xu Yang wrote:
> In current design, usb role class driver will get usb_role_switch parent's
> module reference after the user get usb_role_switch device and put the
> reference after the user put the usb_role_switch device. However, the
> parent device of usb_role_switch may be removed before the user put the
> usb_role_switch. If so, then, NULL pointer issue will be met when the user
> put the parent module's reference.
> 
> This will save the module pointer in structure of usb_role_switch. Then,
> we don't need to find module by iterating long relations.
> 
> Fixes: 5c54fcac9a9d ("usb: roles: Take care of driver module reference counting")
> cc: <stable@vger.kernel.org>
> Signed-off-by: Xu Yang <xu.yang_2@nxp.com>

Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
> Changes in v2:
>  - save module pointer as a member of usb_role_switch as suggested by Alan
> Changes in v3:
>  - no changes
> ---
>  drivers/usb/roles/class.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/usb/roles/class.c b/drivers/usb/roles/class.c
> index ae41578bd014..2bad038fb9ad 100644
> --- a/drivers/usb/roles/class.c
> +++ b/drivers/usb/roles/class.c
> @@ -21,6 +21,7 @@ static const struct class role_class = {
>  struct usb_role_switch {
>  	struct device dev;
>  	struct mutex lock; /* device lock*/
> +	struct module *module; /* the module this device depends on */
>  	enum usb_role role;
>  
>  	/* From descriptor */
> @@ -135,7 +136,7 @@ struct usb_role_switch *usb_role_switch_get(struct device *dev)
>  						  usb_role_switch_match);
>  
>  	if (!IS_ERR_OR_NULL(sw))
> -		WARN_ON(!try_module_get(sw->dev.parent->driver->owner));
> +		WARN_ON(!try_module_get(sw->module));
>  
>  	return sw;
>  }
> @@ -157,7 +158,7 @@ struct usb_role_switch *fwnode_usb_role_switch_get(struct fwnode_handle *fwnode)
>  		sw = fwnode_connection_find_match(fwnode, "usb-role-switch",
>  						  NULL, usb_role_switch_match);
>  	if (!IS_ERR_OR_NULL(sw))
> -		WARN_ON(!try_module_get(sw->dev.parent->driver->owner));
> +		WARN_ON(!try_module_get(sw->module));
>  
>  	return sw;
>  }
> @@ -172,7 +173,7 @@ EXPORT_SYMBOL_GPL(fwnode_usb_role_switch_get);
>  void usb_role_switch_put(struct usb_role_switch *sw)
>  {
>  	if (!IS_ERR_OR_NULL(sw)) {
> -		module_put(sw->dev.parent->driver->owner);
> +		module_put(sw->module);
>  		put_device(&sw->dev);
>  	}
>  }
> @@ -189,15 +190,18 @@ struct usb_role_switch *
>  usb_role_switch_find_by_fwnode(const struct fwnode_handle *fwnode)
>  {
>  	struct device *dev;
> +	struct usb_role_switch *sw = NULL;
>  
>  	if (!fwnode)
>  		return NULL;
>  
>  	dev = class_find_device_by_fwnode(&role_class, fwnode);
> -	if (dev)
> -		WARN_ON(!try_module_get(dev->parent->driver->owner));
> +	if (dev) {
> +		sw = to_role_switch(dev);
> +		WARN_ON(!try_module_get(sw->module));
> +	}
>  
> -	return dev ? to_role_switch(dev) : NULL;
> +	return sw;
>  }
>  EXPORT_SYMBOL_GPL(usb_role_switch_find_by_fwnode);
>  
> @@ -338,6 +342,7 @@ usb_role_switch_register(struct device *parent,
>  	sw->set = desc->set;
>  	sw->get = desc->get;
>  
> +	sw->module = parent->driver->owner;
>  	sw->dev.parent = parent;
>  	sw->dev.fwnode = desc->fwnode;
>  	sw->dev.class = &role_class;
> -- 
> 2.34.1

-- 
heikki