diff mbox series

[2/4] module: sysfs: Simplify section attribute allocation

Message ID 20241216-sysfs-const-bin_attr-module-v1-2-f81e49e54ce4@weissschuh.net (mailing list archive)
State New
Headers show
Series module: sysfs: Two cleanups and preparation for const struct bin_attribute | expand

Checks

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-5 fail Logs for setup / Setup kdevops environment
mcgrof/vmtest-modules-next-VM_Test-1 success Logs for Run CI tests
mcgrof/vmtest-modules-next-VM_Test-2 success 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-3 success Logs for cleanup / Archive results and cleanup

Commit Message

Thomas Weißschuh Dec. 16, 2024, 7:16 p.m. UTC 
The existing allocation logic manually stuffs two allocations into one.
This is hard to understand and of limited value, given that all the
section names are allocated on their own anyways.
Une one allocation per datastructure.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 kernel/module/sysfs.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

Comments

Petr Pavlu Dec. 18, 2024, 11:28 a.m. UTC | #1 
On 12/16/24 20:16, Thomas Weißschuh wrote:
> The existing allocation logic manually stuffs two allocations into one.
> This is hard to understand and of limited value, given that all the
> section names are allocated on their own anyways.
> Une one allocation per datastructure.
> 
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> ---
>  kernel/module/sysfs.c | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/kernel/module/sysfs.c b/kernel/module/sysfs.c
> index b7841f76a933114e6dbd0fc2d32a60b66b7966b6..935629ac21fa16504ddb5f3762af5539572c3bf1 100644
> --- a/kernel/module/sysfs.c
> +++ b/kernel/module/sysfs.c
> @@ -65,34 +65,37 @@ static void free_sect_attrs(struct module_sect_attrs *sect_attrs)
>  
>  	for (bin_attr = sect_attrs->grp.bin_attrs; *bin_attr; bin_attr++)
>  		kfree((*bin_attr)->attr.name);
> +	kfree(sect_attrs->grp.bin_attrs);
>  	kfree(sect_attrs);
>  }
>  
>  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;
> +	unsigned int nloaded = 0, i;
>  	int ret;
>  
>  	/* Count loaded sections and allocate structures */
>  	for (i = 0; i < info->hdr->e_shnum; i++)
>  		if (!sect_empty(&info->sechdrs[i]))
>  			nloaded++;
> -	size[0] = ALIGN(struct_size(sect_attrs, attrs, nloaded),
> -			sizeof(sect_attrs->grp.bin_attrs[0]));
> -	size[1] = (nloaded + 1) * sizeof(sect_attrs->grp.bin_attrs[0]);
> -	sect_attrs = kzalloc(size[0] + size[1], GFP_KERNEL);
> +	sect_attrs = kzalloc(struct_size(sect_attrs, attrs, nloaded), GFP_KERNEL);
>  	if (!sect_attrs)
>  		return -ENOMEM;
>  
> +	gattr = kcalloc(nloaded + 1, sizeof(*gattr), GFP_KERNEL);
> +	if (!gattr) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +

Member sect_attrs->grp.bin_attrs is NULL at this point. If the above
kcalloc() call fails, the control goes to the out label which invokes
free_sect_attrs() and its code
"for (bin_attr = sect_attrs->grp.bin_attrs; *bin_attr; ..."
results in a NULL dereference.

>  	/* Setup section attributes. */
>  	sect_attrs->grp.name = "sections";
> -	sect_attrs->grp.bin_attrs = (void *)sect_attrs + size[0];
> +	sect_attrs->grp.bin_attrs = gattr;
>  
>  	sattr = &sect_attrs->attrs[0];
> -	gattr = &sect_attrs->grp.bin_attrs[0];
>  	for (i = 0; i < info->hdr->e_shnum; i++) {
>  		Elf_Shdr *sec = &info->sechdrs[i];
>  
> @@ -111,7 +114,6 @@ static int add_sect_attrs(struct module *mod, const struct load_info *info)
>  		sattr->battr.attr.mode = 0400;
>  		*(gattr++) = &(sattr++)->battr;
>  	}
> -	*gattr = NULL;
>  
>  	ret = sysfs_create_group(&mod->mkobj.kobj, &sect_attrs->grp);
>  	if (ret)
>
diff mbox series

Patch

diff --git a/kernel/module/sysfs.c b/kernel/module/sysfs.c
index b7841f76a933114e6dbd0fc2d32a60b66b7966b6..935629ac21fa16504ddb5f3762af5539572c3bf1 100644
--- a/kernel/module/sysfs.c
+++ b/kernel/module/sysfs.c
@@ -65,34 +65,37 @@  static void free_sect_attrs(struct module_sect_attrs *sect_attrs)
 
 	for (bin_attr = sect_attrs->grp.bin_attrs; *bin_attr; bin_attr++)
 		kfree((*bin_attr)->attr.name);
+	kfree(sect_attrs->grp.bin_attrs);
 	kfree(sect_attrs);
 }
 
 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;
+	unsigned int nloaded = 0, i;
 	int ret;
 
 	/* Count loaded sections and allocate structures */
 	for (i = 0; i < info->hdr->e_shnum; i++)
 		if (!sect_empty(&info->sechdrs[i]))
 			nloaded++;
-	size[0] = ALIGN(struct_size(sect_attrs, attrs, nloaded),
-			sizeof(sect_attrs->grp.bin_attrs[0]));
-	size[1] = (nloaded + 1) * sizeof(sect_attrs->grp.bin_attrs[0]);
-	sect_attrs = kzalloc(size[0] + size[1], GFP_KERNEL);
+	sect_attrs = kzalloc(struct_size(sect_attrs, attrs, nloaded), GFP_KERNEL);
 	if (!sect_attrs)
 		return -ENOMEM;
 
+	gattr = kcalloc(nloaded + 1, sizeof(*gattr), GFP_KERNEL);
+	if (!gattr) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
 	/* Setup section attributes. */
 	sect_attrs->grp.name = "sections";
-	sect_attrs->grp.bin_attrs = (void *)sect_attrs + size[0];
+	sect_attrs->grp.bin_attrs = gattr;
 
 	sattr = &sect_attrs->attrs[0];
-	gattr = &sect_attrs->grp.bin_attrs[0];
 	for (i = 0; i < info->hdr->e_shnum; i++) {
 		Elf_Shdr *sec = &info->sechdrs[i];
 
@@ -111,7 +114,6 @@  static int add_sect_attrs(struct module *mod, const struct load_info *info)
 		sattr->battr.attr.mode = 0400;
 		*(gattr++) = &(sattr++)->battr;
 	}
-	*gattr = NULL;
 
 	ret = sysfs_create_group(&mod->mkobj.kobj, &sect_attrs->grp);
 	if (ret)