| Message ID | 20240322173930.947963-1-arnd@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] module: don't ignore sysfs_create_link() failures | expand |
On Fri, Mar 22, 2024 at 06:39:11PM +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> > --- > 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 | 2 +- > drivers/base/bus.c | 7 ++++++- > drivers/base/module.c | 42 +++++++++++++++++++++++++++++++----------- > 3 files changed, 38 insertions(+), 13 deletions(-) > > diff --git a/drivers/base/base.h b/drivers/base/base.h > index 0738ccad08b2..0e04bfe02943 100644 > --- a/drivers/base/base.h > +++ b/drivers/base/base.h > @@ -192,7 +192,7 @@ 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, > diff --git a/drivers/base/bus.c b/drivers/base/bus.c > index daee55c9b2d9..7ef75b60d331 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_del_list; Don't we need to walk back the driver_attach() call here if this fails? > + } > > error = driver_create_file(drv, &driver_attr_uevent); > if (error) { > diff --git a/drivers/base/module.c b/drivers/base/module.c > index 46ad4d636731..61282eaed670 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 && ret != -EEXIST) Why would EEXIST happen here? How can this be called twice? > + 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 && ret != -EEXIST) > + goto out; Same EEXIST question here. thanks, greg k-h
On Sat, Mar 23, 2024, at 17:50, Greg Kroah-Hartman wrote: > On Fri, Mar 22, 2024 at 06:39:11PM +0100, Arnd Bergmann wrote: >> diff --git a/drivers/base/bus.c b/drivers/base/bus.c >> index daee55c9b2d9..7ef75b60d331 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_del_list; > > Don't we need to walk back the driver_attach() call here if this fails? Yes, fixed now. There are still some other calls right after it that print an error but don't cause bus_add_driver() to fail though. We may want to add similar unwinding there, but that feels like it should be a separate patch. >> >> if (!mk) >> - return; >> + return 0; >> + >> + ret = sysfs_create_link(&drv->p->kobj, &mk->kobj, "module"); >> + if (ret && ret != -EEXIST) > > Why would EEXIST happen here? How can this be called twice? > My impression was that the lack of error handling and the comment was ab out a case where that might happen intentionally. I've removed it now as I couldn't find any evidence that this is really needed. I suppose we would find out in testing if we do. Arnd
diff --git a/drivers/base/base.h b/drivers/base/base.h index 0738ccad08b2..0e04bfe02943 100644 --- a/drivers/base/base.h +++ b/drivers/base/base.h @@ -192,7 +192,7 @@ 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, diff --git a/drivers/base/bus.c b/drivers/base/bus.c index daee55c9b2d9..7ef75b60d331 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_del_list; + } error = driver_create_file(drv, &driver_attr_uevent); if (error) { diff --git a/drivers/base/module.c b/drivers/base/module.c index 46ad4d636731..61282eaed670 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 && ret != -EEXIST) + 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 && ret != -EEXIST) + 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)