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

Joel Granados
2025-05-19T15:20:39Z

[-- 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 --]