On 12/16/24 20:16, Thomas Weißschuh wrote: > A kobject is meant to manage the lifecycle of some resource. > However the module sysfs code only creates a kobject to get a > "notes" subdirectory in sysfs. > This can be achieved easier and cheaper by using a sysfs group. > Switch the notes attribute code to such a group, similar to how the > section allocation in the same file already works. > > Signed-off-by: Thomas Weißschuh <linux@xxxxxxxxxxxxxx> > --- > kernel/module/sysfs.c | 48 +++++++++++++++++++++++------------------------- > 1 file changed, 23 insertions(+), 25 deletions(-) > > diff --git a/kernel/module/sysfs.c b/kernel/module/sysfs.c > index 935629ac21fa16504ddb5f3762af5539572c3bf1..4f37970f99c999589257713926395686f03103e6 100644 > --- a/kernel/module/sysfs.c > +++ b/kernel/module/sysfs.c > @@ -145,20 +145,17 @@ static void remove_sect_attrs(struct module *mod) > */ > > struct module_notes_attrs { > - struct kobject *dir; > - unsigned int notes; > - struct bin_attribute attrs[] __counted_by(notes); > + struct attribute_group grp; > + struct bin_attribute attrs[]; > }; > > -static void free_notes_attrs(struct module_notes_attrs *notes_attrs, > - unsigned int i) > +static void free_notes_attrs(struct module_notes_attrs *notes_attrs) > { > - if (notes_attrs->dir) { > - while (i-- > 0) > - sysfs_remove_bin_file(notes_attrs->dir, > - ¬es_attrs->attrs[i]); > - kobject_put(notes_attrs->dir); > - } > + struct bin_attribute **bin_attr; > + > + for (bin_attr = notes_attrs->grp.bin_attrs; *bin_attr; bin_attr++) Similarly as commented on patch #2, this results in a NULL dereference when add_notes_attrs() fails to allocate gattr. > + kfree((*bin_attr)->attr.name); This line causes that the name string is freed twice on a module unload, here and in free_sect_attrs(). Notice that function add_notes_attrs() takes each name directly from mod->sect_attrs, without calling kstrdup(): nattr->attr.name = mod->sect_attrs->attrs[loaded].battr.attr.name; > + kfree(notes_attrs->grp.bin_attrs); > kfree(notes_attrs); > } > > @@ -166,6 +163,7 @@ 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 **gattr; > struct bin_attribute *nattr; > int ret; > > @@ -184,7 +182,15 @@ static int add_notes_attrs(struct module *mod, const struct load_info *info) > if (!notes_attrs) > return -ENOMEM; > > - notes_attrs->notes = notes; > + gattr = kcalloc(notes + 1, sizeof(*gattr), GFP_KERNEL); > + if (!gattr) { > + ret = -ENOMEM; > + goto out; > + } > + > + notes_attrs->grp.name = "notes"; > + notes_attrs->grp.bin_attrs = gattr; > + > nattr = ¬es_attrs->attrs[0]; > for (loaded = i = 0; i < info->hdr->e_shnum; ++i) { > if (sect_empty(&info->sechdrs[i])) > @@ -196,35 +202,27 @@ static int add_notes_attrs(struct module *mod, const struct load_info *info) > nattr->size = info->sechdrs[i].sh_size; > nattr->private = (void *)info->sechdrs[i].sh_addr; > nattr->read = sysfs_bin_attr_simple_read; > - ++nattr; > + *(gattr++) = nattr++; > } > ++loaded; > } > > - notes_attrs->dir = kobject_create_and_add("notes", &mod->mkobj.kobj); > - if (!notes_attrs->dir) { > - ret = -ENOMEM; > + ret = sysfs_create_group(&mod->mkobj.kobj, ¬es_attrs->grp); > + if (ret) > goto out; > - } > - > - for (i = 0; i < notes; ++i) { > - ret = sysfs_create_bin_file(notes_attrs->dir, ¬es_attrs->attrs[i]); > - if (ret) > - goto out; > - } > > mod->notes_attrs = notes_attrs; > return 0; > > out: > - free_notes_attrs(notes_attrs, i); > + free_notes_attrs(notes_attrs); > return ret; > } > > static void remove_notes_attrs(struct module *mod) > { > if (mod->notes_attrs) > - free_notes_attrs(mod->notes_attrs, mod->notes_attrs->notes); > + free_notes_attrs(mod->notes_attrs); > } If the patch tries to unify handling of sect_attrs and notes_attrs, should remove_notes_attrs() call also sysfs_remove_group() and reset mod->notes_attrs to match what is done in remove_sect_attrs()? > > #else /* !CONFIG_KALLSYMS */ > -- Thanks, Petr