| Message ID | 20250211214842.1806521-3-shyamsaini@linux.microsoft.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Properly handle module_kobject creation | expand |
| Context | Check | Description |
|---|---|---|
| mcgrof/vmtest-modules-next-PR | fail | PR summary |
| mcgrof/vmtest-modules-next-VM_Test-0 | success | Logs for Run CI tests |
| mcgrof/vmtest-modules-next-VM_Test-1 | success | Logs for Run CI tests |
| mcgrof/vmtest-modules-next-VM_Test-2 | fail | Logs for cleanup / Archive results and cleanup |
| mcgrof/vmtest-modules-next-VM_Test-3 | fail | Logs for cleanup / Archive results and cleanup |
| mcgrof/vmtest-modules-next-VM_Test-4 | fail | Logs for setup / Setup kdevops environment |
| mcgrof/vmtest-modules-next-VM_Test-5 | fail | Logs for setup / Setup kdevops environment |
On 2/11/25 22:48, Shyam Saini wrote: > In the unlikely event of the allocation failing, it is better to let > the machine boot with a not fully populated sysfs than to kill it with > this BUG_ON(). All callers are already prepared for > lookup_or_create_module_kobject() returning NULL. > > This is also preparation for calling this function from non __init > code, where using BUG_ON for allocation failure handling is not > acceptable. I think some error reporting should be cleaned up here. The current situation is that locate_module_kobject() can fail in several cases and all these situations are loudly reported by the function, either by BUG_ON() or pr_crit(). Consistently with that, both its current callers version_sysfs_builtin() and kernel_add_sysfs_param() don't do any reporting if locate_module_kobject() fails; they simply return. The series seems to introduce two somewhat suboptimal cases. With this patch, when either version_sysfs_builtin() or kernel_add_sysfs_param() calls lookup_or_create_module_kobject() and it fails because of a potential kzalloc() error, the problem is silently ignored. Similarly, in the patch #4, when module_add_driver() calls lookup_or_create_module_kobject() and the function fails, the problem may or may not be reported, depending on the error. I'd suggest something as follows: * Drop the pr_crit() reporting in lookup_or_create_module_kobject(). * Have version_sysfs_builtin() and kernel_add_sysfs_param() log an error when lookup_or_create_module_kobject() fails. Using BUG_ON() might be appropriate, as that is already what is used in kernel_add_sysfs_param()? * Update module_add_driver() to propagate any error from lookup_or_create_module_kobject() up the stack.
diff --git a/kernel/params.c b/kernel/params.c index 4b43baaf7c83..6e87aef876b2 100644 --- a/kernel/params.c +++ b/kernel/params.c @@ -770,31 +770,28 @@ static struct module_kobject * __init lookup_or_create_module_kobject(const char int err; kobj = kset_find_obj(module_kset, name); - if (kobj) { - mk = to_module_kobject(kobj); - } else { - mk = kzalloc(sizeof(struct module_kobject), GFP_KERNEL); - BUG_ON(!mk); - - mk->mod = THIS_MODULE; - mk->kobj.kset = module_kset; - err = kobject_init_and_add(&mk->kobj, &module_ktype, NULL, - "%s", name); -#ifdef CONFIG_MODULES - if (!err) - err = sysfs_create_file(&mk->kobj, &module_uevent.attr); -#endif - if (err) { - kobject_put(&mk->kobj); - pr_crit("Adding module '%s' to sysfs failed (%d), the system may be unstable.\n", - name, err); - return NULL; - } + if (kobj) + return to_module_kobject(kobj); - /* So that we hold reference in both cases. */ - kobject_get(&mk->kobj); + mk = kzalloc(sizeof(struct module_kobject), GFP_KERNEL); + if (!mk) + return NULL; + + mk->mod = THIS_MODULE; + mk->kobj.kset = module_kset; + err = kobject_init_and_add(&mk->kobj, &module_ktype, NULL, "%s", name); + if (IS_ENABLED(CONFIG_MODULES) && !err) + err = sysfs_create_file(&mk->kobj, &module_uevent.attr); + if (err) { + kobject_put(&mk->kobj); + pr_crit("Adding module '%s' to sysfs failed (%d), the system may be unstable.\n", + name, err); + return NULL; } + /* So that we hold reference in both cases. */ + kobject_get(&mk->kobj); + return mk; }
In the unlikely event of the allocation failing, it is better to let the machine boot with a not fully populated sysfs than to kill it with this BUG_ON(). All callers are already prepared for lookup_or_create_module_kobject() returning NULL. This is also preparation for calling this function from non __init code, where using BUG_ON for allocation failure handling is not acceptable. Since we are here, also start using IS_ENABLED instead of #ifdef construct. Suggested-by: Thomas Weißschuh <linux@weissschuh.net> Suggested-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> Signed-off-by: Shyam Saini <shyamsaini@linux.microsoft.com> --- kernel/params.c | 41 +++++++++++++++++++---------------------- 1 file changed, 19 insertions(+), 22 deletions(-)