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

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

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>

-- 
Thanks,
Petr


diff --git a/include/linux/kmod.h b/include/linux/kmod.h
index 68f69362d427..9a07c3215389 100644
--- a/include/linux/kmod.h
+++ b/include/linux/kmod.h
@@ -14,10 +14,7 @@
 #include <linux/workqueue.h>
 #include <linux/sysctl.h>
 
-#define KMOD_PATH_LEN 256
-
 #ifdef CONFIG_MODULES
-extern char modprobe_path[]; /* for sysctl */
 /* modprobe exit status on success, -ve on error.  Return value
  * usually useless though. */
 extern __printf(2, 3)
diff --git a/include/linux/module.h b/include/linux/module.h
index 8050f77c3b64..f4ab8d90c475 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -304,7 +304,6 @@ struct notifier_block;
 
 #ifdef CONFIG_MODULES
 
-extern int modules_disabled; /* for sysctl */
 /* Get/put a kernel symbol (calls must be symmetric) */
 void *__symbol_get(const char *symbol);
 void *__symbol_get_gpl(const char *symbol);
diff --git a/kernel/module/internal.h b/kernel/module/internal.h
index 626cf8668a7e..0954c8de00c2 100644
--- a/kernel/module/internal.h
+++ b/kernel/module/internal.h
@@ -58,6 +58,9 @@ extern const struct kernel_symbol __stop___ksymtab_gpl[];
 extern const u32 __start___kcrctab[];
 extern const u32 __start___kcrctab_gpl[];
 
+#define KMOD_PATH_LEN 256
+extern char modprobe_path[];
+
 struct load_info {
 	const char *name;
 	/* pointer to module in temporary copy, freed at end of load_module() */
diff --git a/kernel/module/main.c b/kernel/module/main.c
index a2859dc3eea6..a336b7b3fb23 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -126,9 +126,37 @@ static void mod_update_bounds(struct module *mod)
 }
 
 /* Block module loading/unloading? */
-int modules_disabled;
+static int modules_disabled;
 core_param(nomodule, modules_disabled, bint, 0);
 
+static const struct ctl_table module_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,
+	},
+};
+
+static int __init init_module_sysctl(void)
+{
+	register_sysctl_init("kernel", module_sysctl_table);
+	return 0;
+}
+
+subsys_initcall(init_module_sysctl);
+
 /* Waiting for a module to finish initializing? */
 static DECLARE_WAIT_QUEUE_HEAD(module_wq);
 
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 9b4f0cff76ea..473133d9651e 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -19,7 +19,6 @@
  *  Removed it and replaced it with older style, 03/23/00, Bill Wendling
  */
 
-#include <linux/module.h>
 #include <linux/sysctl.h>
 #include <linux/bitmap.h>
 #include <linux/printk.h>
@@ -1616,25 +1615,6 @@ static const struct ctl_table kern_table[] = {
 		.proc_handler	= proc_dointvec,
 	},
 #endif
-#ifdef CONFIG_MODULES
-	{
-		.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,
-	},
-#endif
 #ifdef CONFIG_UEVENT_HELPER
 	{
 		.procname	= "hotplug",