Re: [PATCH] module: Fix freeing of charp module parameters when CONFIG_SYSFS=n

["Christophe Leroy (CS GROUP)" <chleroy@kernel.org>]

Le 06/03/2026 à 12:10, Petr Pavlu a écrit :
> When setting a charp module parameter, the param_set_charp() function
> allocates memory to store a copy of the input value. Later, when the module
> is potentially unloaded, the destroy_params() function is called to free
> this allocated memory.
> 
> However, destroy_params() is available only when CONFIG_SYSFS=y, otherwise
> only a dummy variant is present. In the unlikely case that the kernel is
> configured with CONFIG_MODULES=y and CONFIG_SYSFS=n, this results in
> a memory leak of charp values when a module is unloaded.
> 
> Fix this issue by making destroy_params() always available when
> CONFIG_MODULES=y. Rename the function to module_destroy_params() to clarify
> that it is intended for use by the module loader.
> 
> Fixes: e180a6b7759a ("param: fix charp parameters set via sysfs")
> Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
> ---
>   include/linux/moduleparam.h | 12 ++++--------
>   kernel/module/main.c        |  4 ++--
>   kernel/params.c             | 27 ++++++++++++++++++---------
>   3 files changed, 24 insertions(+), 19 deletions(-)
> 
> diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
> index 7d22d4c4ea2e..6283665ec614 100644
> --- a/include/linux/moduleparam.h
> +++ b/include/linux/moduleparam.h
> @@ -426,14 +426,10 @@ extern char *parse_args(const char *name,
>   		      void *arg, parse_unknown_fn unknown);
>   
>   /* Called by module remove. */
> -#ifdef CONFIG_SYSFS
> -extern void destroy_params(const struct kernel_param *params, unsigned num);
> -#else
> -static inline void destroy_params(const struct kernel_param *params,
> -				  unsigned num)
> -{
> -}
> -#endif /* !CONFIG_SYSFS */
> +#ifdef CONFIG_MODULES
> +extern void module_destroy_params(const struct kernel_param *params,
> +				  unsigned num);

'extern' is pointless for function prototypes, don't add new ones.

num has no type.

> +#endif
>   
>   /* All the helper functions */
>   /* The macros to do compile-time type checking stolen from Jakub
> diff --git a/kernel/module/main.c b/kernel/module/main.c
> index c3ce106c70af..ef2e2130972f 100644
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c
> @@ -1408,7 +1408,7 @@ static void free_module(struct module *mod)
>   	module_unload_free(mod);
>   
>   	/* Free any allocated parameters. */
> -	destroy_params(mod->kp, mod->num_kp);
> +	module_destroy_params(mod->kp, mod->num_kp);
>   
>   	if (is_livepatch_module(mod))
>   		free_module_elf(mod);
> @@ -3519,7 +3519,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
>   	mod_sysfs_teardown(mod);
>    coming_cleanup:
>   	mod->state = MODULE_STATE_GOING;
> -	destroy_params(mod->kp, mod->num_kp);
> +	module_destroy_params(mod->kp, mod->num_kp);
>   	blocking_notifier_call_chain(&module_notify_list,
>   				     MODULE_STATE_GOING, mod);
>   	klp_module_going(mod);
> diff --git a/kernel/params.c b/kernel/params.c
> index 7188a12dbe86..1a436c9d6140 100644
> --- a/kernel/params.c
> +++ b/kernel/params.c
> @@ -745,15 +745,6 @@ void module_param_sysfs_remove(struct module *mod)
>   }
>   #endif
>   
> -void destroy_params(const struct kernel_param *params, unsigned num)
> -{
> -	unsigned int i;
> -
> -	for (i = 0; i < num; i++)
> -		if (params[i].ops->free)
> -			params[i].ops->free(params[i].arg);
> -}
> -
>   struct module_kobject * __init_or_module
>   lookup_or_create_module_kobject(const char *name)
>   {
> @@ -985,3 +976,21 @@ static int __init param_sysfs_builtin_init(void)
>   late_initcall(param_sysfs_builtin_init);
>   
>   #endif /* CONFIG_SYSFS */
> +
> +#ifdef CONFIG_MODULES
> +
> +/*
> + * module_destroy_params - free all parameters for one module
> + * @params: module parameters (array)
> + * @num: number of module parameters
> + */
> +void module_destroy_params(const struct kernel_param *params, unsigned num)

num has no type

> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < num; i++)
> +		if (params[i].ops->free)
> +			params[i].ops->free(params[i].arg);
> +}
> +
> +#endif /* CONFIG_MODULES */
> 
> base-commit: c107785c7e8dbabd1c18301a1c362544b5786282

Note, checkpatch reports the same:

CHECK: extern prototypes should be avoided in .h files
#47: FILE: include/linux/moduleparam.h:430:
+extern void module_destroy_params(const struct kernel_param *params,

WARNING: Prefer 'unsigned int' to bare use of 'unsigned'
#48: FILE: include/linux/moduleparam.h:431:
+				  unsigned num);

WARNING: Prefer 'unsigned int' to bare use of 'unsigned'
#107: FILE: kernel/params.c:987:
+void module_destroy_params(const struct kernel_param *params, unsigned num)

total: 0 errors, 2 warnings, 1 checks, 70 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
       mechanically convert to the typical style using --fix or 
--fix-inplace.

Commit 4aad08ba007a ("module: Fix freeing of charp module parameters 
when CONFIG_SYSFS=n") has style problems, please review.

NOTE: If any of the errors are false positives, please report
       them to the maintainer, see CHECKPATCH in MAINTAINERS.


Christophe