| Message ID | 20240326145733.3413024-1-arnd@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] module: don't ignore sysfs_create_link() failures | expand |
On Tue, Mar 26, 2024 at 03:57:18PM +0100, Arnd Bergmann wrote: > From: Arnd Bergmann <arnd@arndb.de> > > The sysfs_create_link() return code is marked as __must_check, but the > module_add_driver() function tries hard to not care, by assigning the > return code to a variable. When building with 'make W=1', gcc still > warns because this variable is only assigned but not used: > > drivers/base/module.c: In function 'module_add_driver': > drivers/base/module.c:36:6: warning: variable 'no_warn' set but not used [-Wunused-but-set-variable] > > Rework the code to properly unwind and return the error code to the > caller. My reading of the original code was that it tries to > not fail when the links already exist, so keep ignoring -EEXIST > errors. > Cc: Luis Chamberlain <mcgrof@kernel.org> > Cc: linux-modules@vger.kernel.org > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: "Rafael J. Wysocki" <rafael@kernel.org> Wondering if you can move these to be after --- to avoid polluting commit message. This will have the same effect and be archived on lore. But on pros side it will unload the commit message(s) from unneeded noise. ... > + error = module_add_driver(drv->owner, drv); > + if (error) { > + printk(KERN_ERR "%s: failed to create module links for %s\n", > + __func__, drv->name); What's wrong with pr_err()? Even if it's not a style used, in a new pieces of code this can be improved beforehand. So, we will reduce a technical debt, and not adding to it. > + goto out_detach; > + } ... > +int module_add_driver(struct module *mod, struct device_driver *drv) > { > char *driver_name; > - int no_warn; > + int ret; I would move it... > struct module_kobject *mk = NULL; ...to be here.
On Tue, Mar 26, 2024 at 03:57:18PM +0100, Arnd Bergmann wrote: > From: Arnd Bergmann <arnd@arndb.de> > > The sysfs_create_link() return code is marked as __must_check, but the > module_add_driver() function tries hard to not care, by assigning the > return code to a variable. When building with 'make W=1', gcc still > warns because this variable is only assigned but not used: > > drivers/base/module.c: In function 'module_add_driver': > drivers/base/module.c:36:6: warning: variable 'no_warn' set but not used [-Wunused-but-set-variable] > > Rework the code to properly unwind and return the error code to the > caller. My reading of the original code was that it tries to > not fail when the links already exist, so keep ignoring -EEXIST > errors. > > Cc: Luis Chamberlain <mcgrof@kernel.org> > Cc: linux-modules@vger.kernel.org > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: "Rafael J. Wysocki" <rafael@kernel.org> > Fixes: e17e0f51aeea ("Driver core: show drivers in /sys/module/") > See-also: 4a7fb6363f2d ("add __must_check to device management code") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > v3: make error handling stricter, add unwinding, > fix build fail with CONFIG_MODULES=n > v2: rework to actually handle the error. I have not tested the > error handling beyond build testing, so please review carefully. > --- > drivers/base/base.h | 9 ++++++--- > drivers/base/bus.c | 9 ++++++++- > drivers/base/module.c | 42 +++++++++++++++++++++++++++++++----------- > 3 files changed, 45 insertions(+), 15 deletions(-) Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
On Tue, Mar 26, 2024, at 16:29, Andy Shevchenko wrote: > On Tue, Mar 26, 2024 at 03:57:18PM +0100, Arnd Bergmann wrote: >> From: Arnd Bergmann <arnd@arndb.de> >> >> The sysfs_create_link() return code is marked as __must_check, but the >> module_add_driver() function tries hard to not care, by assigning the >> return code to a variable. When building with 'make W=1', gcc still >> warns because this variable is only assigned but not used: >> >> drivers/base/module.c: In function 'module_add_driver': >> drivers/base/module.c:36:6: warning: variable 'no_warn' set but not used [-Wunused-but-set-variable] >> >> Rework the code to properly unwind and return the error code to the >> caller. My reading of the original code was that it tries to >> not fail when the links already exist, so keep ignoring -EEXIST >> errors. > >> Cc: Luis Chamberlain <mcgrof@kernel.org> >> Cc: linux-modules@vger.kernel.org >> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >> Cc: "Rafael J. Wysocki" <rafael@kernel.org> > > Wondering if you can move these to be after --- to avoid polluting commit > message. This will have the same effect and be archived on lore. But on > pros side it will unload the commit message(s) from unneeded noise. Done > >> + error = module_add_driver(drv->owner, drv); >> + if (error) { >> + printk(KERN_ERR "%s: failed to create module links for %s\n", >> + __func__, drv->name); > > What's wrong with pr_err()? Even if it's not a style used, in a new pieces of > code this can be improved beforehand. So, we will reduce a technical debt, and > not adding to it. I think that would be more confusing, and would rather keep the style consistent. There is no practical difference here. >> +int module_add_driver(struct module *mod, struct device_driver *drv) >> { >> char *driver_name; >> - int no_warn; >> + int ret; > > I would move it... > >> struct module_kobject *mk = NULL; > > ...to be here. Done Arnd
diff --git a/drivers/base/base.h b/drivers/base/base.h index 0738ccad08b2..db4f910e8e36 100644 --- a/drivers/base/base.h +++ b/drivers/base/base.h @@ -192,11 +192,14 @@ extern struct kset *devices_kset; void devices_kset_move_last(struct device *dev); #if defined(CONFIG_MODULES) && defined(CONFIG_SYSFS) -void module_add_driver(struct module *mod, struct device_driver *drv); +int module_add_driver(struct module *mod, struct device_driver *drv); void module_remove_driver(struct device_driver *drv); #else -static inline void module_add_driver(struct module *mod, - struct device_driver *drv) { } +static inline int module_add_driver(struct module *mod, + struct device_driver *drv) +{ + return 0; +} static inline void module_remove_driver(struct device_driver *drv) { } #endif diff --git a/drivers/base/bus.c b/drivers/base/bus.c index daee55c9b2d9..ffea0728b8b2 100644 --- a/drivers/base/bus.c +++ b/drivers/base/bus.c @@ -674,7 +674,12 @@ int bus_add_driver(struct device_driver *drv) if (error) goto out_del_list; } - module_add_driver(drv->owner, drv); + error = module_add_driver(drv->owner, drv); + if (error) { + printk(KERN_ERR "%s: failed to create module links for %s\n", + __func__, drv->name); + goto out_detach; + } error = driver_create_file(drv, &driver_attr_uevent); if (error) { @@ -699,6 +704,8 @@ int bus_add_driver(struct device_driver *drv) return 0; +out_detach: + driver_detach(drv); out_del_list: klist_del(&priv->knode_bus); out_unregister: diff --git a/drivers/base/module.c b/drivers/base/module.c index 46ad4d636731..d16b5c8e5473 100644 --- a/drivers/base/module.c +++ b/drivers/base/module.c @@ -30,14 +30,14 @@ static void module_create_drivers_dir(struct module_kobject *mk) mutex_unlock(&drivers_dir_mutex); } -void module_add_driver(struct module *mod, struct device_driver *drv) +int module_add_driver(struct module *mod, struct device_driver *drv) { char *driver_name; - int no_warn; + int ret; struct module_kobject *mk = NULL; if (!drv) - return; + return 0; if (mod) mk = &mod->mkobj; @@ -56,17 +56,37 @@ void module_add_driver(struct module *mod, struct device_driver *drv) } if (!mk) - return; + return 0; + + ret = sysfs_create_link(&drv->p->kobj, &mk->kobj, "module"); + if (ret) + return ret; - /* Don't check return codes; these calls are idempotent */ - no_warn = sysfs_create_link(&drv->p->kobj, &mk->kobj, "module"); driver_name = make_driver_name(drv); - if (driver_name) { - module_create_drivers_dir(mk); - no_warn = sysfs_create_link(mk->drivers_dir, &drv->p->kobj, - driver_name); - kfree(driver_name); + if (!driver_name) { + ret = -ENOMEM; + goto out; + } + + module_create_drivers_dir(mk); + if (!mk->drivers_dir) { + ret = -EINVAL; + goto out; } + + ret = sysfs_create_link(mk->drivers_dir, &drv->p->kobj, driver_name); + if (ret) + goto out; + + kfree(driver_name); + + return 0; +out: + sysfs_remove_link(&drv->p->kobj, "module"); + sysfs_remove_link(mk->drivers_dir, driver_name); + kfree(driver_name); + + return ret; } void module_remove_driver(struct device_driver *drv)