[PATCH 0/2] module: Remove unnecessary module::args

[PATCH 0/2] module: Remove unnecessary module::args

[Petr Pavlu]

Historically, various parameter-handling code kept pointers into
module::args, most notably the charp support. However, in 2009,
commit e180a6b7759a ("param: fix charp parameters set via sysfs") changed
charp parameters to kstrdup() the input string as well. As a result,
module::args now mostly wastes memory.

Reviewing all kernel_param_ops and module_param_call instances shows that
the last code still relying on module::args remaining valid for the
module's lifetime is simdisk_param_ops_filename. Update it to use kstrdup()
and then remove module::args.

Petr Pavlu (2):
  xtensa/simdisk: Avoid referring to module::args
  module: Remove unnecessary module::args

 arch/xtensa/platforms/iss/simdisk.c | 38 +++++++++++++++++++++++++----
 include/linux/module.h              |  4 ---
 kernel/module/main.c                | 15 ++++++------
 3 files changed, 41 insertions(+), 16 deletions(-)


base-commit: e43ffb69e0438cddd72aaa30898b4dc446f664f8
-- 
2.54.0

[PATCH 1/2] xtensa/simdisk: Avoid referring to module::args

[Petr Pavlu]

When simdisk support is built as a loadable module,
simdisk_param_set_filename() receives a pointer into module::args and
stores each filename pointer as is.

In preparation for removing module::args, update the simdisk.filename
parameter code to copy the provided string. This is somewhat complicated by
the fact that simdisk support can also be built-in, in which case the
parameters are parsed during early boot before slab is available. In that
case, the command line itself is preserved for the lifetime of the kernel,
so continue storing the incoming pointer directly.

Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
---
 arch/xtensa/platforms/iss/simdisk.c | 38 +++++++++++++++++++++++++----
 1 file changed, 33 insertions(+), 5 deletions(-)

diff --git a/arch/xtensa/platforms/iss/simdisk.c b/arch/xtensa/platforms/iss/simdisk.c
index 7c7a2aa749f8..a2ddb49c269d 100644
--- a/arch/xtensa/platforms/iss/simdisk.c
+++ b/arch/xtensa/platforms/iss/simdisk.c
@@ -41,7 +41,7 @@ module_param(simdisk_count, int, S_IRUGO);
 MODULE_PARM_DESC(simdisk_count, "Number of simdisk units.");
 
 static int n_files;
-static const char *filename[MAX_SIMDISK_COUNT] = {
+static char *filename[MAX_SIMDISK_COUNT] = {
 #ifdef CONFIG_SIMDISK0_FILENAME
 	CONFIG_SIMDISK0_FILENAME,
 #ifdef CONFIG_SIMDISK1_FILENAME
@@ -50,20 +50,48 @@ static const char *filename[MAX_SIMDISK_COUNT] = {
 #endif
 };
 
+/*
+ * The simdisk code can be built either into the kernel or as a loadable module.
+ * When built-in, CONFIG_SIMDISK{0,1}_FILENAME can be used to specify the
+ * initial simdisk filenames and additional filenames can be provided on the
+ * kernel command line. These arguments are parsed during early boot when slab
+ * is not yet available, but the command line itself is preserved for the
+ * lifetime of the kernel, so the incoming pointer is stored directly.
+ * When built as a loadable module, each value is copied with kstrdup() and all
+ * allocated memory is freed in simdisk_param_free_filename() when the module is
+ * unloaded.
+ */
 static int simdisk_param_set_filename(const char *val,
 		const struct kernel_param *kp)
 {
-	if (n_files < ARRAY_SIZE(filename))
-		filename[n_files++] = val;
-	else
+	char *str;
+
+	if (n_files >= ARRAY_SIZE(filename))
 		return -EINVAL;
+
+#ifdef MODULE
+	str = kstrdup(val, GFP_KERNEL);
+	if (!str)
+		return -ENOMEM;
+#else
+	str = (char *)val;
+#endif
+
+	filename[n_files++] = str;
 	return 0;
 }
 
+static void simdisk_param_free_filename(void *arg)
+{
+	for (int i = 0; i < n_files; i++)
+		kfree(filename[i]);
+}
+
 static const struct kernel_param_ops simdisk_param_ops_filename = {
 	.set = simdisk_param_set_filename,
+	.free = simdisk_param_free_filename,
 };
-module_param_cb(filename, &simdisk_param_ops_filename, &n_files, 0);
+module_param_cb(filename, &simdisk_param_ops_filename, NULL, 0);
 MODULE_PARM_DESC(filename, "Backing storage filename.");
 
 static int simdisk_major = SIMDISK_MAJOR;
-- 
2.54.0

Re: [PATCH 1/2] xtensa/simdisk: Avoid referring to module::args

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Concurrent access to `dev->filename` in `proc_read_simdisk` and `simdisk_detach` causes a Use-After-Free.
--

commit e0a519c0b452f39fc91f2ac38f46b81a32db738a
Author: Petr Pavlu <petr.pavlu@suse.com>

xtensa/simdisk: Avoid referring to module::args

This commit prepares for the removal of module::args by copying the filename
string provided as a parameter instead of storing a direct pointer when built
as a loadable module. It also adds a free function to clean up these copied
strings upon module unload.

> diff --git a/arch/xtensa/platforms/iss/simdisk.c b/arch/xtensa/platforms/iss/simdisk.c
> index 7c7a2aa749f82..a2ddb49c269dc 100644
> --- a/arch/xtensa/platforms/iss/simdisk.c
> +++ b/arch/xtensa/platforms/iss/simdisk.c

[ ... ]

>  static const struct kernel_param_ops simdisk_param_ops_filename = {
>  	.set = simdisk_param_set_filename,
> +	.free = simdisk_param_free_filename,
>  };
> -module_param_cb(filename, &simdisk_param_ops_filename, &n_files, 0);
> +module_param_cb(filename, &simdisk_param_ops_filename, NULL, 0);
>  MODULE_PARM_DESC(filename, "Backing storage filename.");
>  
>  static int simdisk_major = SIMDISK_MAJOR;

[Severity: High]
This isn't a bug introduced by this patch, but is it possible to trigger a
use-after-free here due to concurrent access to dev->filename?

The procfs entry for simdisk devices is created with 0644 permissions, allowing
any user to read it. In proc_read_simdisk(), dev->filename is fetched without
acquiring dev->lock:

arch/xtensa/platforms/iss/simdisk.c:proc_read_simdisk() {
    ...
    const char *s = dev->filename;
    if (s) {
        ssize_t len = strlen(s);
        char *temp = kmalloc(len + 2, GFP_KERNEL);
    ...
}

If an administrator concurrently writes to the same procfs entry, it triggers
proc_write_simdisk() -> simdisk_detach(), which acquires dev->lock and frees
the filename:

arch/xtensa/platforms/iss/simdisk.c:simdisk_detach() {
    ...
    spin_lock(&dev->lock);
    ...
        } else {
            pr_info("SIMDISK: %s detached from %s\n",
                    dev->gd->disk_name, dev->filename);
            dev->fd = -1;
            kfree(dev->filename);
            dev->filename = NULL;
        }
    ...
}

If proc_read_simdisk() fetches the pointer right before simdisk_detach() frees
it, the subsequent strlen() and scnprintf() operations could dereference freed
memory. Does this need locking or RCU protection to prevent a potential oops
or local information leak?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260604110455.1608038-1-petr.pavlu@suse.com?part=1

[PATCH 2/2] module: Remove unnecessary module::args

[Petr Pavlu]

Historically, various parameter-handling code kept pointers into
module::args, most notably the charp support. However, in 2009,
commit e180a6b7759a ("param: fix charp parameters set via sysfs") changed
charp parameters to kstrdup() the input string as well. As a result,
module::args now mostly wastes memory.

The last users that still pointed into module::args have now been cleaned
up, so remove this data.

Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
---
 include/linux/module.h |  4 ----
 kernel/module/main.c   | 15 ++++++++-------
 2 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/include/linux/module.h b/include/linux/module.h
index 7566815fabbe..96cc98568eea 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -477,10 +477,6 @@ struct module {
 	struct module_notes_attrs *notes_attrs;
 #endif
 
-	/* The command line arguments (may be mangled).  People like
-	   keeping pointers to this stuff */
-	char *args;
-
 #ifdef CONFIG_SMP
 	/* Per-cpu data. */
 	void __percpu *percpu;
diff --git a/kernel/module/main.c b/kernel/module/main.c
index 46dd8d25a605..528690ba160b 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -1458,7 +1458,6 @@ static void free_module(struct module *mod)
 
 	/* This may be empty, but that's OK */
 	module_arch_freeing_init(mod);
-	kfree(mod->args);
 	percpu_modfree(mod);
 
 	free_mod_mem(mod);
@@ -3425,7 +3424,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
 	struct module *mod;
 	bool module_allocated = false;
 	long err = 0;
-	char *after_dashes;
+	char *args = NULL, *after_dashes;
 
 	/*
 	 * Do the signature check (if any) first. All that
@@ -3523,9 +3522,9 @@ static int load_module(struct load_info *info, const char __user *uargs,
 	flush_module_icache(mod);
 
 	/* Now copy in args */
-	mod->args = strndup_user(uargs, ~0UL >> 1);
-	if (IS_ERR(mod->args)) {
-		err = PTR_ERR(mod->args);
+	args = strndup_user(uargs, ~0UL >> 1);
+	if (IS_ERR(args)) {
+		err = PTR_ERR(args);
 		goto free_arch_cleanup;
 	}
 
@@ -3546,7 +3545,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
 	mod->async_probe_requested = async_probe;
 
 	/* Module is ready to execute: parsing args may do that. */
-	after_dashes = parse_args(mod->name, mod->args, mod->kp, mod->num_kp,
+	after_dashes = parse_args(mod->name, args, mod->kp, mod->num_kp,
 				  -32768, 32767, mod,
 				  unknown_module_param_cb);
 	if (IS_ERR(after_dashes)) {
@@ -3556,6 +3555,8 @@ static int load_module(struct load_info *info, const char __user *uargs,
 		pr_warn("%s: parameters '%s' after `--' ignored\n",
 		       mod->name, after_dashes);
 	}
+	kfree(args);
+	args = NULL;
 
 	/* Link in to sysfs. */
 	err = mod_sysfs_setup(mod, info, mod->kp, mod->num_kp);
@@ -3597,7 +3598,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
  ddebug_cleanup:
 	ftrace_release_mod(mod);
 	synchronize_rcu();
-	kfree(mod->args);
+	kfree(args);
  free_arch_cleanup:
 	module_arch_cleanup(mod);
  free_modinfo:
-- 
2.54.0

Re: [PATCH 2/2] module: Remove unnecessary module::args

[Aaron Tomlin]

On Thu, Jun 04, 2026 at 01:04:46PM +0200, Petr Pavlu wrote:
> Historically, various parameter-handling code kept pointers into
> module::args, most notably the charp support. However, in 2009,
> commit e180a6b7759a ("param: fix charp parameters set via sysfs") changed
> charp parameters to kstrdup() the input string as well. As a result,
> module::args now mostly wastes memory.
> 
> The last users that still pointed into module::args have now been cleaned
> up, so remove this data.
> 
> Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
> ---
>  include/linux/module.h |  4 ----
>  kernel/module/main.c   | 15 ++++++++-------
>  2 files changed, 8 insertions(+), 11 deletions(-)
> 
> diff --git a/include/linux/module.h b/include/linux/module.h
> index 7566815fabbe..96cc98568eea 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -477,10 +477,6 @@ struct module {
>  	struct module_notes_attrs *notes_attrs;
>  #endif
>  
> -	/* The command line arguments (may be mangled).  People like
> -	   keeping pointers to this stuff */
> -	char *args;
> -
>  #ifdef CONFIG_SMP
>  	/* Per-cpu data. */
>  	void __percpu *percpu;
> diff --git a/kernel/module/main.c b/kernel/module/main.c
> index 46dd8d25a605..528690ba160b 100644
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c
> @@ -1458,7 +1458,6 @@ static void free_module(struct module *mod)
>  
>  	/* This may be empty, but that's OK */
>  	module_arch_freeing_init(mod);
> -	kfree(mod->args);
>  	percpu_modfree(mod);
>  
>  	free_mod_mem(mod);
> @@ -3425,7 +3424,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
>  	struct module *mod;
>  	bool module_allocated = false;
>  	long err = 0;
> -	char *after_dashes;
> +	char *args = NULL, *after_dashes;
>  
>  	/*
>  	 * Do the signature check (if any) first. All that
> @@ -3523,9 +3522,9 @@ static int load_module(struct load_info *info, const char __user *uargs,
>  	flush_module_icache(mod);
>  
>  	/* Now copy in args */
> -	mod->args = strndup_user(uargs, ~0UL >> 1);
> -	if (IS_ERR(mod->args)) {
> -		err = PTR_ERR(mod->args);
> +	args = strndup_user(uargs, ~0UL >> 1);
> +	if (IS_ERR(args)) {
> +		err = PTR_ERR(args);
>  		goto free_arch_cleanup;
>  	}
>  
> @@ -3546,7 +3545,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
>  	mod->async_probe_requested = async_probe;
>  
>  	/* Module is ready to execute: parsing args may do that. */
> -	after_dashes = parse_args(mod->name, mod->args, mod->kp, mod->num_kp,
> +	after_dashes = parse_args(mod->name, args, mod->kp, mod->num_kp,
>  				  -32768, 32767, mod,
>  				  unknown_module_param_cb);
>  	if (IS_ERR(after_dashes)) {
> @@ -3556,6 +3555,8 @@ static int load_module(struct load_info *info, const char __user *uargs,
>  		pr_warn("%s: parameters '%s' after `--' ignored\n",
>  		       mod->name, after_dashes);
>  	}
> +	kfree(args);
> +	args = NULL;
>  
>  	/* Link in to sysfs. */
>  	err = mod_sysfs_setup(mod, info, mod->kp, mod->num_kp);
> @@ -3597,7 +3598,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
>   ddebug_cleanup:
>  	ftrace_release_mod(mod);
>  	synchronize_rcu();
> -	kfree(mod->args);
> +	kfree(args);
>   free_arch_cleanup:
>  	module_arch_cleanup(mod);
>   free_modinfo:
> -- 
> 2.54.0
> 

LGTM.

Reviewed-by: Aaron Tomlin <atomlin@atomlin.com>

-- 
Aaron Tomlin