[01/12] module: Move modprobe_path and modules_disabled ctl_tables into the module subsys

Move module sysctl (modprobe_path and modules_disabled) out of sysctl.c
and into the modules subsystem. Make the modprobe_path variable static
as it no longer needs to be exported. Remove module.h from the includes
in sysctl as it no longer uses any module exported variables.

This is part of a greater effort to move ctl tables into their
respective subsystems which will reduce the merge conflicts in
kernel/sysctl.c.

Signed-off-by: Joel Granados <joel.granados@kernel.org>
---
 include/linux/kmod.h |  1 -
 kernel/module/kmod.c | 32 +++++++++++++++++++++++++++++++-
 kernel/sysctl.c      | 20 --------------------
 3 files changed, 31 insertions(+), 22 deletions(-)

On 5/9/25 14:54, Joel Granados wrote:
> Move module sysctl (modprobe_path and modules_disabled) out of sysctl.c
> and into the modules subsystem. Make the modprobe_path variable static
> as it no longer needs to be exported. Remove module.h from the includes
> in sysctl as it no longer uses any module exported variables.
> 
> This is part of a greater effort to move ctl tables into their
> respective subsystems which will reduce the merge conflicts in
> kernel/sysctl.c.
> 
> Signed-off-by: Joel Granados <joel.granados@kernel.org>
> [...]
> --- a/kernel/module/kmod.c
> +++ b/kernel/module/kmod.c
> @@ -60,7 +60,7 @@ static DEFINE_SEMAPHORE(kmod_concurrent_max, MAX_KMOD_CONCURRENT);
>  /*
>  	modprobe_path is set via /proc/sys.
>  */
> -char modprobe_path[KMOD_PATH_LEN] = CONFIG_MODPROBE_PATH;
> +static char modprobe_path[KMOD_PATH_LEN] = CONFIG_MODPROBE_PATH;
>  
>  static void free_modprobe_argv(struct subprocess_info *info)
>  {
> @@ -177,3 +177,33 @@ int __request_module(bool wait, const char *fmt, ...)
>  	return ret;
>  }
>  EXPORT_SYMBOL(__request_module);
> +
> +#ifdef CONFIG_MODULES
> +static const struct ctl_table kmod_sysctl_table[] = {
> +	{
> +		.procname	= "modprobe",
> +		.data		= &modprobe_path,
> +		.maxlen		= KMOD_PATH_LEN,
> +		.mode		= 0644,
> +		.proc_handler	= proc_dostring,
> +	},
> +	{
> +		.procname	= "modules_disabled",
> +		.data		= &modules_disabled,
> +		.maxlen		= sizeof(int),
> +		.mode		= 0644,
> +		/* only handle a transition from default "0" to "1" */
> +		.proc_handler	= proc_dointvec_minmax,
> +		.extra1		= SYSCTL_ONE,
> +		.extra2		= SYSCTL_ONE,
> +	},

This is minor.. but the file kernel/module/kmod.c contains the logic to
request direct modprobe invocation by the kernel. Registering the
modprobe_path sysctl here is appropriate. However, the modules_disabled
setting affects the entire module loader so I don't think it's best to
register it here.

I suggest keeping a single table for the module sysctl values but moving
it to kernel/module/main.c. This means the variable modprobe_path must
retain external linkage, on the other hand, modules_disabled can be made
static.

On 5/15/25 12:04, Joel Granados wrote:
> On Thu, May 15, 2025 at 10:04:53AM +0200, Petr Pavlu wrote:
>> On 5/9/25 14:54, Joel Granados wrote:
>>> Move module sysctl (modprobe_path and modules_disabled) out of sysctl.c
>>> and into the modules subsystem. Make the modprobe_path variable static
>>> as it no longer needs to be exported. Remove module.h from the includes
>>> in sysctl as it no longer uses any module exported variables.
>>>
>>> This is part of a greater effort to move ctl tables into their
>>> respective subsystems which will reduce the merge conflicts in
>>> kernel/sysctl.c.
>>>
>>> Signed-off-by: Joel Granados <joel.granados@kernel.org>
>>> [...]
>>> --- a/kernel/module/kmod.c
>>> +++ b/kernel/module/kmod.c
>>> @@ -60,7 +60,7 @@ static DEFINE_SEMAPHORE(kmod_concurrent_max, MAX_KMOD_CONCURRENT);
>>>  /*
>>>  	modprobe_path is set via /proc/sys.
>>>  */
>>> -char modprobe_path[KMOD_PATH_LEN] = CONFIG_MODPROBE_PATH;
>>> +static char modprobe_path[KMOD_PATH_LEN] = CONFIG_MODPROBE_PATH;
>>>  
>>>  static void free_modprobe_argv(struct subprocess_info *info)
>>>  {
>>> @@ -177,3 +177,33 @@ int __request_module(bool wait, const char *fmt, ...)
>>>  	return ret;
>>>  }
>>>  EXPORT_SYMBOL(__request_module);
>>> +
>>> +#ifdef CONFIG_MODULES
>>> +static const struct ctl_table kmod_sysctl_table[] = {
>>> +	{
>>> +		.procname	= "modprobe",
>>> +		.data		= &modprobe_path,
>>> +		.maxlen		= KMOD_PATH_LEN,
>>> +		.mode		= 0644,
>>> +		.proc_handler	= proc_dostring,
>>> +	},
>>> +	{
>>> +		.procname	= "modules_disabled",
>>> +		.data		= &modules_disabled,
>>> +		.maxlen		= sizeof(int),
>>> +		.mode		= 0644,
>>> +		/* only handle a transition from default "0" to "1" */
>>> +		.proc_handler	= proc_dointvec_minmax,
>>> +		.extra1		= SYSCTL_ONE,
>>> +		.extra2		= SYSCTL_ONE,
>>> +	},
>>
>> This is minor.. but the file kernel/module/kmod.c contains the logic to
>> request direct modprobe invocation by the kernel. Registering the
>> modprobe_path sysctl here is appropriate. However, the modules_disabled
>> setting affects the entire module loader so I don't think it's best to
>> register it here.
>>
>> I suggest keeping a single table for the module sysctl values but moving
>> it to kernel/module/main.c. This means the variable modprobe_path must
>> retain external linkage, on the other hand, modules_disabled can be made
>> static.
> 
> Like this?:
> [...]

Let's also move the KMOD_PATH_LEN definition and the modprobe_path
declaration from include/linux/kmod.h to kernel/module/internal.h, as
they are now fully internal to the module loader, and use "module"
instead of "kmod" in the sysctl registration to avoid confusion with the
modprobe logic.

The adjusted patch is below.

Reviewed-by: Petr Pavlu <petr.pavlu@suse.com>