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 <li...@weissschuh.net>
> ---
> 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
