[PATCH v3 0/1] fix module sysfs patch defect

[PATCH v3 0/1] fix module sysfs patch defect

[Chunhui Li]

Hi Maintainer,

I have update the patch base on your feedback, please help review again.

V3:
-Fix initial value;
-Remove unnecessary checks about mod->sects_attrs
-Check sysfs_creat_* return value as error code
-Reorder the reveral of successful operations before exiting

We will use the information recorded in mod->sects_attrs for debugging.
   When a panic occurs in a .ko, we can use gdb to load the symbols of the .ko file
along with the coredump, and use the text segment address and init.text segment address
in mod->sects_attrs to debug. 
   If there is an error in add_sysfs_setup and the .ko is still loaded and runs normally,
when the ko suffer panic later, we would not be able to access the loading addresses
of the ko, which would prevent us from being able to debug effectively.
   Therefore, it is desired that if there is an error during load module, it should prevent
the ko from being inserted with insmod. This ensures that as long as the .ko is
successfully loaded, all necessary information for debugging is available.

Thanks for your time.


Chunhui Li (1):
  module: abort module loading when sysfs setup suffer errors

 kernel/module/sysfs.c | 57 +++++++++++++++++++++++++++----------------
 1 file changed, 36 insertions(+), 21 deletions(-)

-- 
2.45.2

[PATCH v3 1/1] module: abort module loading when sysfs setup suffer errors

[Chunhui Li]

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

Acked-by: Luis Chamberlain <mcgrof@kernel.org>
Reviewed-by: Petr Pavlu <petr.pavlu@suse.com>
Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202409010016.3XIFSmRA-lkp@intel.com/
Signed-off-by: Xion Wang <xion.wang@mediatek.com>
Signed-off-by: Chunhui Li <chunhui.li@mediatek.com>
---
 kernel/module/sysfs.c | 57 +++++++++++++++++++++++++++----------------
 1 file changed, 36 insertions(+), 21 deletions(-)

diff --git a/kernel/module/sysfs.c b/kernel/module/sysfs.c
index 26efe1305c12..0e0a9137a273 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;
 
 	/* 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,15 @@ static void add_sect_attrs(struct module *mod, const struct load_info *info)
 	}
 	*gattr = NULL;
 
-	if (sysfs_create_group(&mod->mkobj.kobj, &sect_attrs->grp))
+	ret = sysfs_create_group(&mod->mkobj.kobj, &sect_attrs->grp);
+	if (ret)
 		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 +163,12 @@ 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;
-
-	/* failed to create section attributes, so can't create notes */
-	if (!mod->sect_attrs)
-		return;
+	int ret;
 
 	/* Count notes sections and allocate structures.  */
 	notes = 0;
@@ -176,12 +178,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 = &notes_attrs->attrs[0];
@@ -201,19 +203,23 @@ 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,
-					  &notes_attrs->attrs[i]))
+	for (i = 0; i < notes; ++i) {
+		ret = sysfs_create_bin_file(notes_attrs->dir, &notes_attrs->attrs[i]);
+		if (ret)
 			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)
@@ -223,9 +229,9 @@ static void remove_notes_attrs(struct module *mod)
 }
 
 #else /* !CONFIG_KALLSYMS */
-static inline void add_sect_attrs(struct module *mod, const struct load_info *info) { }
+static inline int add_sect_attrs(struct module *mod, const struct load_info *info) { }
 static inline void remove_sect_attrs(struct module *mod) { }
-static inline void add_notes_attrs(struct module *mod, const struct load_info *info) { }
+static inline int add_notes_attrs(struct module *mod, const struct load_info *info) { }
 static inline void remove_notes_attrs(struct module *mod) { }
 #endif /* CONFIG_KALLSYMS */
 
@@ -385,11 +391,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_del_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_del_usage_links:
+	del_usage_links(mod);
 out_unreg_modinfo_attrs:
 	module_remove_modinfo_attrs(mod, -1);
 out_unreg_param:
-- 
2.45.2

Re: [PATCH v3 1/1] module: abort module loading when sysfs setup suffer errors

[Luis Chamberlain]

On Fri, Sep 06, 2024 at 07:57:48PM +0800, 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
> 
> Acked-by: Luis Chamberlain <mcgrof@kernel.org>
> Reviewed-by: Petr Pavlu <petr.pavlu@suse.com>
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202409010016.3XIFSmRA-lkp@intel.com/
> Signed-off-by: Xion Wang <xion.wang@mediatek.com>
> Signed-off-by: Chunhui Li <chunhui.li@mediatek.com>

Please add a Fixes: tag.

  Luis

Re: [PATCH v3 1/1] module: abort module loading when sysfs setup suffer errors

[kernel test robot]

Hi Chunhui,

kernel test robot noticed the following build warnings:

[auto build test WARNING on mcgrof/modules-next]
[also build test WARNING on linus/master v6.11-rc6 next-20240906]
[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/20240907-000002
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux.git modules-next
patch link:    https://lore.kernel.org/r/20240906115748.5367-2-chunhui.li%40mediatek.com
patch subject: [PATCH v3 1/1] module: abort module loading when sysfs setup suffer errors
config: openrisc-defconfig (https://download.01.org/0day-ci/archive/20240907/202409072018.qfEzZbO7-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/20240907/202409072018.qfEzZbO7-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/202409072018.qfEzZbO7-lkp@intel.com/

All warnings (new ones prefixed by >>):

   kernel/module/sysfs.c: In function 'add_sect_attrs':
>> kernel/module/sysfs.c:232:67: warning: no return statement in function returning non-void [-Wreturn-type]
     232 | static inline int add_sect_attrs(struct module *mod, const struct load_info *info) { }
         |                                                                   ^~~~~~~~~
   kernel/module/sysfs.c: In function 'add_notes_attrs':
   kernel/module/sysfs.c:234:68: warning: no return statement in function returning non-void [-Wreturn-type]
     234 | static inline int add_notes_attrs(struct module *mod, const struct load_info *info) { }
         |                                                                    ^~~~~~~~~


vim +232 kernel/module/sysfs.c

   230	
   231	#else /* !CONFIG_KALLSYMS */
 > 232	static inline int add_sect_attrs(struct module *mod, const struct load_info *info) { }
   233	static inline void remove_sect_attrs(struct module *mod) { }
   234	static inline int add_notes_attrs(struct module *mod, const struct load_info *info) { }
   235	static inline void remove_notes_attrs(struct module *mod) { }
   236	#endif /* CONFIG_KALLSYMS */
   237	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v3 1/1] module: abort module loading when sysfs setup suffer errors

[kernel test robot]

Hi Chunhui,

kernel test robot noticed the following build warnings:

[auto build test WARNING on mcgrof/modules-next]
[also build test WARNING on linus/master v6.11-rc6 next-20240906]
[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/20240907-000002
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux.git modules-next
patch link:    https://lore.kernel.org/r/20240906115748.5367-2-chunhui.li%40mediatek.com
patch subject: [PATCH v3 1/1] module: abort module loading when sysfs setup suffer errors
config: powerpc-asp8347_defconfig (https://download.01.org/0day-ci/archive/20240908/202409080534.y2m1URF1-lkp@intel.com/config)
compiler: clang version 17.0.6 (https://github.com/llvm/llvm-project 6009708b4367171ccdbf4b5905cb6a803753fe18)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240908/202409080534.y2m1URF1-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/202409080534.y2m1URF1-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> kernel/module/sysfs.c:232:86: warning: non-void function does not return a value [-Wreturn-type]
     232 | static inline int add_sect_attrs(struct module *mod, const struct load_info *info) { }
         |                                                                                      ^
   kernel/module/sysfs.c:234:87: warning: non-void function does not return a value [-Wreturn-type]
     234 | static inline int add_notes_attrs(struct module *mod, const struct load_info *info) { }
         |                                                                                       ^
   2 warnings generated.


vim +232 kernel/module/sysfs.c

   230	
   231	#else /* !CONFIG_KALLSYMS */
 > 232	static inline int add_sect_attrs(struct module *mod, const struct load_info *info) { }
   233	static inline void remove_sect_attrs(struct module *mod) { }
   234	static inline int add_notes_attrs(struct module *mod, const struct load_info *info) { }
   235	static inline void remove_notes_attrs(struct module *mod) { }
   236	#endif /* CONFIG_KALLSYMS */
   237	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki