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

[Joel Granados <joel.granados@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 5204 bytes --]

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?:

---
 include/linux/module.h |  1 -
 kernel/module/main.c   | 30 +++++++++++++++++++++++++++++-
 kernel/sysctl.c        | 20 --------------------
 3 files changed, 29 insertions(+), 22 deletions(-)

diff --git a/include/linux/module.h b/include/linux/module.h
index d94b196d5a34..25476168e012 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -302,7 +302,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/main.c b/kernel/module/main.c
index a2859dc3eea6..13055ef65f15 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 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,
+	},
+};
+
+static int __init init_kmod_sysctl(void)
+{
+	register_sysctl_init("kernel", kmod_sysctl_table);
+	return 0;
+}
+
+subsys_initcall(init_kmod_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",
-- 
2.47.2

-- 

Joel Granados

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]