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@xxxxxxxxxxxx> > Signed-off-by: Chunhui Li <chunhui.li@xxxxxxxxxxxx> > --- > 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: -- Cheers, Petr
