| Message ID | 20240129093739.2371530-1-xu.yang_2@nxp.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 1c9be13846c0b2abc2480602f8ef421360e1ad9e |
| Headers | show |
| Series | [v3,1/2] usb: roles: fix NULL pointer issue when put module's reference | expand |
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
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;
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> --- 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(-)