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

[Petr Pavlu <petr.pavlu@suse.com>]

On 3/9/26 12:26 PM, Christophe Leroy (CS GROUP) wrote:
> 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.

I intentionally kept the `extern` keyword and the `unsigned` parameter
to maintain consistency with the surrounding style in moduleparam.h.
I'll send a v2 to correct this in my patch. Additionally, I can include
cleanup patches for moduleparam.h to remove the unnecessary `extern` and
change `unsigned` to `unsigned int` in other occurrences, so the content
of the file remains consistent in this regard.

-- 
Thanks,
Petr