| Message ID | 20240830054400.26622-1-chunhui.li@mediatek.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | module: abort module loading when sysfs setup suffer errors | expand |
Hi Chunhui, kernel test robot noticed the following build errors: [auto build test ERROR on mcgrof/modules-next] [also build test ERROR on linus/master v6.11-rc5 next-20240830] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Chunhui-Li/module-abort-module-loading-when-sysfs-setup-suffer-errors/20240830-134417 base: https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux.git modules-next patch link: https://lore.kernel.org/r/20240830054400.26622-1-chunhui.li%40mediatek.com patch subject: [PATCH] module: abort module loading when sysfs setup suffer errors config: openrisc-defconfig (https://download.01.org/0day-ci/archive/20240901/202409010016.3XIFSmRA-lkp@intel.com/config) compiler: or1k-linux-gcc (GCC) 14.1.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240901/202409010016.3XIFSmRA-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202409010016.3XIFSmRA-lkp@intel.com/ All errors (new ones prefixed by >>): kernel/module/sysfs.c: In function 'mod_sysfs_setup': >> kernel/module/sysfs.c:400:13: error: void value not ignored as it ought to be 400 | err = add_sect_attrs(mod, info); | ^ kernel/module/sysfs.c:404:13: error: void value not ignored as it ought to be 404 | err = add_notes_attrs(mod, info); | ^ vim +400 kernel/module/sysfs.c 370 371 int mod_sysfs_setup(struct module *mod, 372 const struct load_info *info, 373 struct kernel_param *kparam, 374 unsigned int num_params) 375 { 376 int err; 377 378 err = mod_sysfs_init(mod); 379 if (err) 380 goto out; 381 382 mod->holders_dir = kobject_create_and_add("holders", &mod->mkobj.kobj); 383 if (!mod->holders_dir) { 384 err = -ENOMEM; 385 goto out_unreg; 386 } 387 388 err = module_param_sysfs_setup(mod, kparam, num_params); 389 if (err) 390 goto out_unreg_holders; 391 392 err = module_add_modinfo_attrs(mod); 393 if (err) 394 goto out_unreg_param; 395 396 err = add_usage_links(mod); 397 if (err) 398 goto out_unreg_modinfo_attrs; 399 > 400 err = add_sect_attrs(mod, info); 401 if (err) 402 goto out_unreg_sect_attrs; 403 404 err = add_notes_attrs(mod, info); 405 if (err) 406 goto out_unreg_notes_attrs; 407 408 return 0; 409 410 out_unreg_notes_attrs: 411 remove_notes_attrs(mod); 412 out_unreg_sect_attrs: 413 remove_sect_attrs(mod); 414 out_unreg_modinfo_attrs: 415 module_remove_modinfo_attrs(mod, -1); 416 out_unreg_param: 417 module_param_sysfs_remove(mod); 418 out_unreg_holders: 419 kobject_put(mod->holders_dir); 420 out_unreg: 421 mod_kobject_put(mod); 422 out: 423 return err; 424 } 425
Hi Chunhui,
kernel test robot noticed the following build errors:
[auto build test ERROR on mcgrof/modules-next]
[also build test ERROR on linus/master v6.11-rc5 next-20240830]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Chunhui-Li/module-abort-module-loading-when-sysfs-setup-suffer-errors/20240830-134417
base: https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux.git modules-next
patch link: https://lore.kernel.org/r/20240830054400.26622-1-chunhui.li%40mediatek.com
patch subject: [PATCH] module: abort module loading when sysfs setup suffer errors
config: powerpc-mpc834x_itx_defconfig (https://download.01.org/0day-ci/archive/20240901/202409010209.FqYOzYEa-lkp@intel.com/config)
compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project 46fe36a4295f05d5d3731762e31fc4e6e99863e9)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240901/202409010209.FqYOzYEa-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409010209.FqYOzYEa-lkp@intel.com/
All errors (new ones prefixed by >>):
In file included from kernel/module/sysfs.c:13:
In file included from include/linux/kallsyms.h:13:
In file included from include/linux/mm.h:2228:
include/linux/vmstat.h:517:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
517 | return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
| ~~~~~~~~~~~ ^ ~~~
>> kernel/module/sysfs.c:400:6: error: assigning to 'int' from incompatible type 'void'
400 | err = add_sect_attrs(mod, info);
| ^ ~~~~~~~~~~~~~~~~~~~~~~~~~
kernel/module/sysfs.c:404:6: error: assigning to 'int' from incompatible type 'void'
404 | err = add_notes_attrs(mod, info);
| ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning and 2 errors generated.
vim +400 kernel/module/sysfs.c
370
371 int mod_sysfs_setup(struct module *mod,
372 const struct load_info *info,
373 struct kernel_param *kparam,
374 unsigned int num_params)
375 {
376 int err;
377
378 err = mod_sysfs_init(mod);
379 if (err)
380 goto out;
381
382 mod->holders_dir = kobject_create_and_add("holders", &mod->mkobj.kobj);
383 if (!mod->holders_dir) {
384 err = -ENOMEM;
385 goto out_unreg;
386 }
387
388 err = module_param_sysfs_setup(mod, kparam, num_params);
389 if (err)
390 goto out_unreg_holders;
391
392 err = module_add_modinfo_attrs(mod);
393 if (err)
394 goto out_unreg_param;
395
396 err = add_usage_links(mod);
397 if (err)
398 goto out_unreg_modinfo_attrs;
399
> 400 err = add_sect_attrs(mod, info);
401 if (err)
402 goto out_unreg_sect_attrs;
403
404 err = add_notes_attrs(mod, info);
405 if (err)
406 goto out_unreg_notes_attrs;
407
408 return 0;
409
410 out_unreg_notes_attrs:
411 remove_notes_attrs(mod);
412 out_unreg_sect_attrs:
413 remove_sect_attrs(mod);
414 out_unreg_modinfo_attrs:
415 module_remove_modinfo_attrs(mod, -1);
416 out_unreg_param:
417 module_param_sysfs_remove(mod);
418 out_unreg_holders:
419 kobject_put(mod->holders_dir);
420 out_unreg:
421 mod_kobject_put(mod);
422 out:
423 return err;
424 }
425
On 8/30/24 07:43, Chunhui Li wrote: > When insmod a kernel module, if fails in add_notes_attrs or > add_sysfs_attrs such as memory allocation fail, mod_sysfs_setup > will still return success, but we can't access user interface > on android device. > > Patch for make mod_sysfs_setup can check the error of > add_notes_attrs and add_sysfs_attrs s/add_sysfs_attrs/add_sect_attrs/ I think it makes sense to propagate errors from these functions upward, although I wonder if the authors of this code didn't intentionally make the errors silent, possibly because the interface was mostly intended for debugging only? The original commits which added add_sect_attrs() and add_notes_attrs() don't mention anything explicitly in this regard: https://github.com/mpe/linux-fullhistory/commit/db939b519bea9b88ae1c95c3b479c0b07145f2a0 https://github.com/torvalds/linux/commit/6d76013381ed28979cd122eb4b249a88b5e384fa > > Signed-off-by: Xion Wang <xion.wang@mediatek.com> > Signed-off-by: Chunhui Li <chunhui.li@mediatek.com> > --- > kernel/module/sysfs.c | 49 ++++++++++++++++++++++++++++++------------- > 1 file changed, 35 insertions(+), 14 deletions(-) > > diff --git a/kernel/module/sysfs.c b/kernel/module/sysfs.c > index 26efe1305c12..a9ee650d995d 100644 > --- a/kernel/module/sysfs.c > +++ b/kernel/module/sysfs.c > @@ -69,12 +69,13 @@ static void free_sect_attrs(struct module_sect_attrs *sect_attrs) > kfree(sect_attrs); > } > > -static void add_sect_attrs(struct module *mod, const struct load_info *info) > +static int add_sect_attrs(struct module *mod, const struct load_info *info) > { > unsigned int nloaded = 0, i, size[2]; > struct module_sect_attrs *sect_attrs; > struct module_sect_attr *sattr; > struct bin_attribute **gattr; > + int ret = 0; Nit: It isn't necessary to initialize this variable to 0 because the code explicitly does "return 0;" on success. While on the error path, the variable is always assigned. > > /* Count loaded sections and allocate structures */ > for (i = 0; i < info->hdr->e_shnum; i++) > @@ -85,7 +86,7 @@ static void add_sect_attrs(struct module *mod, const struct load_info *info) > size[1] = (nloaded + 1) * sizeof(sect_attrs->grp.bin_attrs[0]); > sect_attrs = kzalloc(size[0] + size[1], GFP_KERNEL); > if (!sect_attrs) > - return; > + return -ENOMEM; > > /* Setup section attributes. */ > sect_attrs->grp.name = "sections"; > @@ -103,8 +104,10 @@ static void add_sect_attrs(struct module *mod, const struct load_info *info) > sattr->address = sec->sh_addr; > sattr->battr.attr.name = > kstrdup(info->secstrings + sec->sh_name, GFP_KERNEL); > - if (!sattr->battr.attr.name) > + if (!sattr->battr.attr.name) { > + ret = -ENOMEM; > goto out; > + } > sect_attrs->nsections++; > sattr->battr.read = module_sect_read; > sattr->battr.size = MODULE_SECT_READ_SIZE; > @@ -113,13 +116,16 @@ static void add_sect_attrs(struct module *mod, const struct load_info *info) > } > *gattr = NULL; > > - if (sysfs_create_group(&mod->mkobj.kobj, §_attrs->grp)) > + if (sysfs_create_group(&mod->mkobj.kobj, §_attrs->grp)) { > + ret = -EIO; > goto out; > + } Why does the logic return -EIO instead of propagating the error code from sysfs_create_group()? > > mod->sect_attrs = sect_attrs; > - return; > + return 0; > out: > free_sect_attrs(sect_attrs); > + return ret; > } > > static void remove_sect_attrs(struct module *mod) > @@ -158,15 +164,16 @@ static void free_notes_attrs(struct module_notes_attrs *notes_attrs, > kfree(notes_attrs); > } > > -static void add_notes_attrs(struct module *mod, const struct load_info *info) > +static int add_notes_attrs(struct module *mod, const struct load_info *info) > { > unsigned int notes, loaded, i; > struct module_notes_attrs *notes_attrs; > struct bin_attribute *nattr; > + int ret = 0; Similarly here, the initialization is not necessary. > > /* failed to create section attributes, so can't create notes */ > if (!mod->sect_attrs) > - return; > + return -EINVAL; Since the patch modifies mod_sysfs_setup() to bail out when registering section attributes fails, this condition can no longer be true and the check can be removed. > > /* Count notes sections and allocate structures. */ > notes = 0; > @@ -176,12 +183,12 @@ static void add_notes_attrs(struct module *mod, const struct load_info *info) > ++notes; > > if (notes == 0) > - return; > + return 0; > > notes_attrs = kzalloc(struct_size(notes_attrs, attrs, notes), > GFP_KERNEL); > if (!notes_attrs) > - return; > + return -ENOMEM; > > notes_attrs->notes = notes; > nattr = ¬es_attrs->attrs[0]; > @@ -201,19 +208,24 @@ static void add_notes_attrs(struct module *mod, const struct load_info *info) > } > > notes_attrs->dir = kobject_create_and_add("notes", &mod->mkobj.kobj); > - if (!notes_attrs->dir) > + if (!notes_attrs->dir) { > + ret = -ENOMEM; > goto out; > + } > > for (i = 0; i < notes; ++i) > if (sysfs_create_bin_file(notes_attrs->dir, > - ¬es_attrs->attrs[i])) > + ¬es_attrs->attrs[i])) { > + ret = -EIO; > goto out; > + } Similarly here, the actual error from sysfs_create_bin_file() can be returned. > > mod->notes_attrs = notes_attrs; > - return; > + return 0; > > out: > free_notes_attrs(notes_attrs, i); > + return ret; > } > > static void remove_notes_attrs(struct module *mod) > @@ -385,11 +397,20 @@ int mod_sysfs_setup(struct module *mod, > if (err) > goto out_unreg_modinfo_attrs; > > - add_sect_attrs(mod, info); > - add_notes_attrs(mod, info); > + err = add_sect_attrs(mod, info); > + if (err) > + goto out_unreg_sect_attrs; > + > + err = add_notes_attrs(mod, info); > + if (err) > + goto out_unreg_notes_attrs; > > return 0; > > +out_unreg_notes_attrs: > + remove_notes_attrs(mod); > +out_unreg_sect_attrs: > + remove_sect_attrs(mod); Upon a failure from add_sect_attrs(), the caller doesn't need to unwind its operation. It is the responsibility of add_sect_attrs() to clean up after itself on error. Instead, the code in mod_sysfs_setup() needs to unwind all previous successful operations leading up to this point, which means here additionally to invoke del_usage_links(). I think you want something as follows: err = add_sect_attrs(mod, info); if (err) goto out_unreg_usage_links; err = add_notes_attrs(mod, info); if (err) goto out_unreg_sect_attrs; return 0; out_unreg_sect_attrs: remove_sect_attrs(mod); out_unreg_usage_links: del_usage_links(mod); [...] > out_unreg_modinfo_attrs: > module_remove_modinfo_attrs(mod, -1); > out_unreg_param:
diff --git a/kernel/module/sysfs.c b/kernel/module/sysfs.c index 26efe1305c12..a9ee650d995d 100644 --- a/kernel/module/sysfs.c +++ b/kernel/module/sysfs.c @@ -69,12 +69,13 @@ static void free_sect_attrs(struct module_sect_attrs *sect_attrs) kfree(sect_attrs); } -static void add_sect_attrs(struct module *mod, const struct load_info *info) +static int add_sect_attrs(struct module *mod, const struct load_info *info) { unsigned int nloaded = 0, i, size[2]; struct module_sect_attrs *sect_attrs; struct module_sect_attr *sattr; struct bin_attribute **gattr; + int ret = 0; /* Count loaded sections and allocate structures */ for (i = 0; i < info->hdr->e_shnum; i++) @@ -85,7 +86,7 @@ static void add_sect_attrs(struct module *mod, const struct load_info *info) size[1] = (nloaded + 1) * sizeof(sect_attrs->grp.bin_attrs[0]); sect_attrs = kzalloc(size[0] + size[1], GFP_KERNEL); if (!sect_attrs) - return; + return -ENOMEM; /* Setup section attributes. */ sect_attrs->grp.name = "sections"; @@ -103,8 +104,10 @@ static void add_sect_attrs(struct module *mod, const struct load_info *info) sattr->address = sec->sh_addr; sattr->battr.attr.name = kstrdup(info->secstrings + sec->sh_name, GFP_KERNEL); - if (!sattr->battr.attr.name) + if (!sattr->battr.attr.name) { + ret = -ENOMEM; goto out; + } sect_attrs->nsections++; sattr->battr.read = module_sect_read; sattr->battr.size = MODULE_SECT_READ_SIZE; @@ -113,13 +116,16 @@ static void add_sect_attrs(struct module *mod, const struct load_info *info) } *gattr = NULL; - if (sysfs_create_group(&mod->mkobj.kobj, §_attrs->grp)) + if (sysfs_create_group(&mod->mkobj.kobj, §_attrs->grp)) { + ret = -EIO; goto out; + } mod->sect_attrs = sect_attrs; - return; + return 0; out: free_sect_attrs(sect_attrs); + return ret; } static void remove_sect_attrs(struct module *mod) @@ -158,15 +164,16 @@ static void free_notes_attrs(struct module_notes_attrs *notes_attrs, kfree(notes_attrs); } -static void add_notes_attrs(struct module *mod, const struct load_info *info) +static int add_notes_attrs(struct module *mod, const struct load_info *info) { unsigned int notes, loaded, i; struct module_notes_attrs *notes_attrs; struct bin_attribute *nattr; + int ret = 0; /* failed to create section attributes, so can't create notes */ if (!mod->sect_attrs) - return; + return -EINVAL; /* Count notes sections and allocate structures. */ notes = 0; @@ -176,12 +183,12 @@ static void add_notes_attrs(struct module *mod, const struct load_info *info) ++notes; if (notes == 0) - return; + return 0; notes_attrs = kzalloc(struct_size(notes_attrs, attrs, notes), GFP_KERNEL); if (!notes_attrs) - return; + return -ENOMEM; notes_attrs->notes = notes; nattr = ¬es_attrs->attrs[0]; @@ -201,19 +208,24 @@ static void add_notes_attrs(struct module *mod, const struct load_info *info) } notes_attrs->dir = kobject_create_and_add("notes", &mod->mkobj.kobj); - if (!notes_attrs->dir) + if (!notes_attrs->dir) { + ret = -ENOMEM; goto out; + } for (i = 0; i < notes; ++i) if (sysfs_create_bin_file(notes_attrs->dir, - ¬es_attrs->attrs[i])) + ¬es_attrs->attrs[i])) { + ret = -EIO; goto out; + } mod->notes_attrs = notes_attrs; - return; + return 0; out: free_notes_attrs(notes_attrs, i); + return ret; } static void remove_notes_attrs(struct module *mod) @@ -385,11 +397,20 @@ int mod_sysfs_setup(struct module *mod, if (err) goto out_unreg_modinfo_attrs; - add_sect_attrs(mod, info); - add_notes_attrs(mod, info); + err = add_sect_attrs(mod, info); + if (err) + goto out_unreg_sect_attrs; + + err = add_notes_attrs(mod, info); + if (err) + goto out_unreg_notes_attrs; return 0; +out_unreg_notes_attrs: + remove_notes_attrs(mod); +out_unreg_sect_attrs: + remove_sect_attrs(mod); out_unreg_modinfo_attrs: module_remove_modinfo_attrs(mod, -1); out_unreg_param: