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: 5008 bytes --]

On Thu, May 15, 2025 at 02:45:22PM +0200, Petr Pavlu wrote:
> 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.
...
> > 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>
Thx for review and patch. Have applied it to my current V2 branch [1]

Best

[1] https://git.kernel.org/pub/scm/linux/kernel/git/joel.granados/linux.git/log/?h=jag/mv_ctltables_iter2

> 
> -- 
> 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",
> 

-- 

Joel Granados

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