| Message ID | b666b75fa732407e7e390ba27ebacaf663e93f7d.1701791668.git.nabijaczleweli@nabijaczleweli.xyz (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [kmod,1/3] Add kmod_new_flags() variant of kmod_new(), with abiver 32 | expand |
On Tue, Dec 05, 2023 at 04:55:22PM +0100, Ahelenia Ziemiańska wrote: >This can be passed to kmod_new_flags() to disable loading configuration >from /proc/cmdline touch foo sudo mount --bind foo /proc/cmdline This has been what I always used for local/test scenarios. I wonder if this really needs a more "official" way like your are doing to be added to the library rather than just the tools. Lucas De Marchi > >Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz> >--- > libkmod/libkmod-config.c | 3 ++- > libkmod/libkmod-internal.h | 2 +- > libkmod/libkmod.c | 2 +- > libkmod/libkmod.h | 4 +++- > 4 files changed, 7 insertions(+), 4 deletions(-) > >diff --git a/libkmod/libkmod-config.c b/libkmod/libkmod-config.c >index e24dab1..600d666 100644 >--- a/libkmod/libkmod-config.c >+++ b/libkmod/libkmod-config.c >@@ -949,7 +949,8 @@ int kmod_config_new(struct kmod_ctx *ctx, struct kmod_config **p_config, > free(cf); > } > >- kmod_config_parse_kcmdline(config); >+ if (!(options & KMOD_NEW_IGNORE_CMDLINE)) >+ kmod_config_parse_kcmdline(config); > > return 0; > >diff --git a/libkmod/libkmod-internal.h b/libkmod/libkmod-internal.h >index 26a7e28..1d0bd92 100644 >--- a/libkmod/libkmod-internal.h >+++ b/libkmod/libkmod-internal.h >@@ -132,7 +132,7 @@ struct kmod_config { > struct kmod_list *paths; > }; > >-int kmod_config_new(struct kmod_ctx *ctx, struct kmod_config **config, const char * const *config_paths) __attribute__((nonnull(1, 2,3))); >+int kmod_config_new(struct kmod_ctx *ctx, struct kmod_config **config, const char * const *config_paths, enum kmod_new_flags options) __attribute__((nonnull(1, 2,3))); > void kmod_config_free(struct kmod_config *config) __attribute__((nonnull(1))); > const char *kmod_blacklist_get_modname(const struct kmod_list *l) __attribute__((nonnull(1))); > const char *kmod_alias_get_name(const struct kmod_list *l) __attribute__((nonnull(1))); >diff --git a/libkmod/libkmod.c b/libkmod/libkmod.c >index 279b35d..126a66c 100644 >--- a/libkmod/libkmod.c >+++ b/libkmod/libkmod.c >@@ -317,7 +317,7 @@ KMOD_EXPORT struct kmod_ctx *kmod_new_flags(const char *dirname, > > if (config_paths == NULL) > config_paths = default_config_paths; >- err = kmod_config_new(ctx, &ctx->config, config_paths); >+ err = kmod_config_new(ctx, &ctx->config, config_paths, options); > if (err < 0) { > ERR(ctx, "could not create config\n"); > goto fail; >diff --git a/libkmod/libkmod.h b/libkmod/libkmod.h >index 72cd7a2..29637dd 100644 >--- a/libkmod/libkmod.h >+++ b/libkmod/libkmod.h >@@ -30,7 +30,9 @@ > extern "C" { > #endif > >-enum kmod_new_flags {}; >+enum kmod_new_flags { >+ KMOD_NEW_IGNORE_CMDLINE = 0x00001, >+}; > > /* > * kmod_ctx >-- >2.39.2 >
On Wed, Dec 06, 2023 at 09:14:55AM -0600, Lucas De Marchi wrote: > On Tue, Dec 05, 2023 at 04:55:22PM +0100, Ahelenia Ziemiańska wrote: > touch foo > sudo mount --bind foo /proc/cmdline > This has been what I always used for local/test scenarios. This is also what I came up with post factum, but it's clearly not obvious, since the responding user did resort to rebooting. > I wonder if > this really needs a more "official" way like your are doing to be > added to the library rather than just the tools. I don't disagree; below is a scissor-patch that effectivaly canonicalises modprobe -I ... to be "unshare -rm sh -c 'mount --bind /dev/null /proc/cmdline; modprobe ...'" (the -r is removed if you're already root). I've used this approach (exactly this snippet in various arrangements) extensively in various test suites to string up fake procfses, and it ought to work in all environments you'd be validly running modprobe (it won't work if you're non-root in a chroot: unlikely). Best, -- >8 -- Subject: [PATCH v2] Add modprobe -I/--ignore-cmdline Previously, if you'd misconfigured the cmdline your system would be completely poisoned. In this real scenario, ixgbe.allow_supported_sfp=1,1,1,1 was set. This yielded [ 3852.901900] ixgbe: `1,1,1,1' invalid for parameter `allow_unsupported_sfp' [ 3852.904595] ixgbe: unknown parameter 'allow_supported_sfp' ignored and # modprobe -r ixgbe # modprobe ixgbe allow_supported_sfp=1 since, indeed, # modprobe -nv ixgbe insmod /lib/modules/5.16.0-1-amd64/kernel/drivers/net/ethernet/intel/ixgbe/ixgbe.ko allow_unsupported_sfp=1,1,1,1 # modprobe -nv ixgbe allow_supported_sfp=1 insmod /lib/modules/5.16.0-1-amd64/kernel/drivers/net/ethernet/intel/ixgbe/ixgbe.ko allow_unsupported_sfp=1,1,1,1 allow_supported_sfp=1 this leaves you with a tens-of-minutes-long reboot (or with an explicit insmod, which no-one came up with at the time, and which requires manual dependency-chasing). With -I, the module can be correctly loaded since the cmdline-derived parameter no longer stops the module loading: # modprobe -nvI ixgbe allow_supported_sfp=1 insmod /lib/modules/5.16.0-1-amd64/kernel/drivers/net/ethernet/intel/ixgbe/ixgbe.ko allow_supported_sfp=1 # modprobe -I ixgbe allow_supported_sfp=1 [ 4497.032342] ixgbe: Intel(R) 10 Gigabit PCI Express Network Driver [ 4497.034624] ixgbe: Copyright (c) 1999-2016 Intel Corporation. This in many ways mirrors -C /dev/null and -i. Yes, you could do this manually with unshare -m; mount --bind /dev/null /proc/cmdline but if you aren't primed to look for it, or aren't familiar with the mechanism in the first place, you can't Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz> --- man/modprobe.xml | 16 ++++++++++++++++ tools/modprobe.c | 43 ++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 58 insertions(+), 1 deletion(-) diff --git a/man/modprobe.xml b/man/modprobe.xml index 91f9e27..ab9dbb0 100644 --- a/man/modprobe.xml +++ b/man/modprobe.xml @@ -47,6 +47,7 @@ <arg><option>-C <replaceable>config-file</replaceable></option></arg> <arg><option>-n</option></arg> <arg><option>-i</option></arg> + <arg><option>-I</option></arg> <arg><option>-q</option></arg> <arg><option>-b</option></arg> <arg><replaceable>modulename</replaceable></arg> @@ -58,6 +59,7 @@ <arg><option>-v</option></arg> <arg><option>-n</option></arg> <arg><option>-i</option></arg> + <arg><option>-I</option></arg> <arg rep='repeat'><option><replaceable>modulename</replaceable></option></arg> </cmdsynopsis> <cmdsynopsis> @@ -318,6 +320,20 @@ </para> </listitem> </varlistentry> + <varlistentry> + <term> + <option>-I</option> + </term> + <term> + <option>--ignore-cmdline</option> + </term> + <listitem> + <para> + This option causes <command>modprobe</command> to ignore + any configuration specified via the kernel command line. + </para> + </listitem> + </varlistentry> <varlistentry> <term> <option>-n</option> diff --git a/tools/modprobe.c b/tools/modprobe.c index e891028..de013b1 100644 --- a/tools/modprobe.c +++ b/tools/modprobe.c @@ -21,11 +21,13 @@ #include <errno.h> #include <getopt.h> #include <limits.h> +#include <sched.h> #include <stdbool.h> #include <stdio.h> #include <stdlib.h> #include <string.h> #include <unistd.h> +#include <sys/mount.h> #include <sys/stat.h> #include <sys/types.h> #include <sys/utsname.h> @@ -59,7 +61,7 @@ static int remove_holders = 0; static unsigned long long wait_msec = 0; static int quiet_inuse = 0; -static const char cmdopts_s[] = "arw:RibfDcnC:d:S:sqvVh"; +static const char cmdopts_s[] = "arw:RiIbfDcnC:d:S:sqvVh"; static const struct option cmdopts[] = { {"all", no_argument, 0, 'a'}, @@ -72,6 +74,7 @@ static const struct option cmdopts[] = { {"first-time", no_argument, 0, 3}, {"ignore-install", no_argument, 0, 'i'}, {"ignore-remove", no_argument, 0, 'i'}, + {"ignore-cmdline", no_argument, 0, 'I'}, {"use-blacklist", no_argument, 0, 'b'}, {"force", no_argument, 0, 'f'}, {"force-modversion", no_argument, 0, 2}, @@ -825,6 +828,32 @@ static char **prepend_options_from_env(int *p_argc, char **orig_argv) return new_argv; } +#define UNSHARE_REQ(...) if(!(__VA_ARGS__)) return false; +#define UNSHARE_FILE(path, ...) \ + { \ + FILE * f = fopen(path, "we"); \ + UNSHARE_REQ(f); \ + fprintf(f, __VA_ARGS__); \ + fclose(f); \ + } +static bool clear_cmdline(void) +{ + int uid = geteuid(); + if(uid) { + int gid = getegid(); + UNSHARE_REQ(!unshare(CLONE_NEWUSER)); + UNSHARE_FILE("/proc/self/setgroups", "deny"); + UNSHARE_FILE("/proc/self/uid_map", "0 %d 1", uid); + UNSHARE_FILE("/proc/self/gid_map", "0 %d 1", gid); + } + + UNSHARE_REQ(!unshare(CLONE_NEWNS)); + UNSHARE_REQ(!mount("none", "/", NULL, MS_REC | MS_PRIVATE, NULL)); + + UNSHARE_REQ(!mount("/dev/null", "/proc/cmdline", NULL, MS_BIND, NULL)); + return true; +} + static int do_modprobe(int argc, char **orig_argv) { struct kmod_ctx *ctx; @@ -835,6 +864,7 @@ static int do_modprobe(int argc, char **orig_argv) const char *dirname = NULL; const char *root = NULL; const char *kversion = NULL; + int ignore_cmdline = 0; int use_all = 0; int do_remove = 0; int do_show_config = 0; @@ -881,6 +911,9 @@ static int do_modprobe(int argc, char **orig_argv) case 'i': ignore_commands = 1; break; + case 'I': + ignore_cmdline = 1; + break; case 'b': use_blacklist = 1; break; @@ -1004,6 +1037,14 @@ static int do_modprobe(int argc, char **orig_argv) dirname = dirname_buf; } + if (ignore_cmdline) { + if (!clear_cmdline()) { + ERR("clear_cmdline() failed!\n"); + err = -1; + goto done; + } + } + ctx = kmod_new(dirname, config_paths); if (!ctx) { ERR("kmod_new() failed!\n");
On Wed, Dec 06, 2023 at 09:37:21PM +0100, Ahelenia Ziemiańska wrote: >On Wed, Dec 06, 2023 at 09:14:55AM -0600, Lucas De Marchi wrote: >> On Tue, Dec 05, 2023 at 04:55:22PM +0100, Ahelenia Ziemiańska wrote: >> touch foo >> sudo mount --bind foo /proc/cmdline >> This has been what I always used for local/test scenarios. >This is also what I came up with post factum, but it's clearly not obvious, >since the responding user did resort to rebooting. > >> I wonder if >> this really needs a more "official" way like your are doing to be >> added to the library rather than just the tools. >I don't disagree; below is a scissor-patch that effectivaly >canonicalises modprobe -I ... to be >"unshare -rm sh -c 'mount --bind /dev/null /proc/cmdline; modprobe ...'" >(the -r is removed if you're already root). > >I've used this approach (exactly this snippet in various arrangements) >extensively in various test suites to string up fake procfses, >and it ought to work in all environments you'd be validly running modprobe >(it won't work if you're non-root in a chroot: unlikely). > >Best, >-- >8 -- >Subject: [PATCH v2] Add modprobe -I/--ignore-cmdline > >Previously, if you'd misconfigured the cmdline your system would be >completely poisoned. I as mentioning that as a way for people/sysadmins to workaround that and get out of the situation "now my system is completely poisoned". > >In this real scenario, ixgbe.allow_supported_sfp=1,1,1,1 was set. >This yielded > [ 3852.901900] ixgbe: `1,1,1,1' invalid for parameter `allow_unsupported_sfp' > [ 3852.904595] ixgbe: unknown parameter 'allow_supported_sfp' ignored >and > # modprobe -r ixgbe > # modprobe ixgbe allow_supported_sfp=1 >since, indeed, > # modprobe -nv ixgbe > insmod /lib/modules/5.16.0-1-amd64/kernel/drivers/net/ethernet/intel/ixgbe/ixgbe.ko allow_unsupported_sfp=1,1,1,1 > # modprobe -nv ixgbe allow_supported_sfp=1 > insmod /lib/modules/5.16.0-1-amd64/kernel/drivers/net/ethernet/intel/ixgbe/ixgbe.ko allow_unsupported_sfp=1,1,1,1 allow_supported_sfp=1 >this leaves you with a tens-of-minutes-long reboot >(or with an explicit insmod, which no-one came up with at the time, > and which requires manual dependency-chasing). > >With -I, the module can be correctly loaded since the cmdline-derived >parameter no longer stops the module loading: > # modprobe -nvI ixgbe allow_supported_sfp=1 > insmod /lib/modules/5.16.0-1-amd64/kernel/drivers/net/ethernet/intel/ixgbe/ixgbe.ko allow_supported_sfp=1 > # modprobe -I ixgbe allow_supported_sfp=1 > [ 4497.032342] ixgbe: Intel(R) 10 Gigabit PCI Express Network Driver > [ 4497.034624] ixgbe: Copyright (c) 1999-2016 Intel Corporation. > >This in many ways mirrors -C /dev/null and -i. yeah... but if we are indeed adding it to modprobe, then the solution with libkmod change is probably better. I will think a bit after the next version that should be released this week. thanks and sorry for the delay on replying. Lucas De Marchi > >Yes, you could do this manually with > unshare -m; mount --bind /dev/null /proc/cmdline >but if you aren't primed to look for it, >or aren't familiar with the mechanism in the first place, >you can't > >Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz> >--- > man/modprobe.xml | 16 ++++++++++++++++ > tools/modprobe.c | 43 ++++++++++++++++++++++++++++++++++++++++++- > 2 files changed, 58 insertions(+), 1 deletion(-) > >diff --git a/man/modprobe.xml b/man/modprobe.xml >index 91f9e27..ab9dbb0 100644 >--- a/man/modprobe.xml >+++ b/man/modprobe.xml >@@ -47,6 +47,7 @@ > <arg><option>-C <replaceable>config-file</replaceable></option></arg> > <arg><option>-n</option></arg> > <arg><option>-i</option></arg> >+ <arg><option>-I</option></arg> > <arg><option>-q</option></arg> > <arg><option>-b</option></arg> > <arg><replaceable>modulename</replaceable></arg> >@@ -58,6 +59,7 @@ > <arg><option>-v</option></arg> > <arg><option>-n</option></arg> > <arg><option>-i</option></arg> >+ <arg><option>-I</option></arg> > <arg rep='repeat'><option><replaceable>modulename</replaceable></option></arg> > </cmdsynopsis> > <cmdsynopsis> >@@ -318,6 +320,20 @@ > </para> > </listitem> > </varlistentry> >+ <varlistentry> >+ <term> >+ <option>-I</option> >+ </term> >+ <term> >+ <option>--ignore-cmdline</option> >+ </term> >+ <listitem> >+ <para> >+ This option causes <command>modprobe</command> to ignore >+ any configuration specified via the kernel command line. >+ </para> >+ </listitem> >+ </varlistentry> > <varlistentry> > <term> > <option>-n</option> >diff --git a/tools/modprobe.c b/tools/modprobe.c >index e891028..de013b1 100644 >--- a/tools/modprobe.c >+++ b/tools/modprobe.c >@@ -21,11 +21,13 @@ > #include <errno.h> > #include <getopt.h> > #include <limits.h> >+#include <sched.h> > #include <stdbool.h> > #include <stdio.h> > #include <stdlib.h> > #include <string.h> > #include <unistd.h> >+#include <sys/mount.h> > #include <sys/stat.h> > #include <sys/types.h> > #include <sys/utsname.h> >@@ -59,7 +61,7 @@ static int remove_holders = 0; > static unsigned long long wait_msec = 0; > static int quiet_inuse = 0; > >-static const char cmdopts_s[] = "arw:RibfDcnC:d:S:sqvVh"; >+static const char cmdopts_s[] = "arw:RiIbfDcnC:d:S:sqvVh"; > static const struct option cmdopts[] = { > {"all", no_argument, 0, 'a'}, > >@@ -72,6 +74,7 @@ static const struct option cmdopts[] = { > {"first-time", no_argument, 0, 3}, > {"ignore-install", no_argument, 0, 'i'}, > {"ignore-remove", no_argument, 0, 'i'}, >+ {"ignore-cmdline", no_argument, 0, 'I'}, > {"use-blacklist", no_argument, 0, 'b'}, > {"force", no_argument, 0, 'f'}, > {"force-modversion", no_argument, 0, 2}, >@@ -825,6 +828,32 @@ static char **prepend_options_from_env(int *p_argc, char **orig_argv) > return new_argv; > } > >+#define UNSHARE_REQ(...) if(!(__VA_ARGS__)) return false; >+#define UNSHARE_FILE(path, ...) \ >+ { \ >+ FILE * f = fopen(path, "we"); \ >+ UNSHARE_REQ(f); \ >+ fprintf(f, __VA_ARGS__); \ >+ fclose(f); \ >+ } >+static bool clear_cmdline(void) >+{ >+ int uid = geteuid(); >+ if(uid) { >+ int gid = getegid(); >+ UNSHARE_REQ(!unshare(CLONE_NEWUSER)); >+ UNSHARE_FILE("/proc/self/setgroups", "deny"); >+ UNSHARE_FILE("/proc/self/uid_map", "0 %d 1", uid); >+ UNSHARE_FILE("/proc/self/gid_map", "0 %d 1", gid); >+ } >+ >+ UNSHARE_REQ(!unshare(CLONE_NEWNS)); >+ UNSHARE_REQ(!mount("none", "/", NULL, MS_REC | MS_PRIVATE, NULL)); >+ >+ UNSHARE_REQ(!mount("/dev/null", "/proc/cmdline", NULL, MS_BIND, NULL)); >+ return true; >+} >+ > static int do_modprobe(int argc, char **orig_argv) > { > struct kmod_ctx *ctx; >@@ -835,6 +864,7 @@ static int do_modprobe(int argc, char **orig_argv) > const char *dirname = NULL; > const char *root = NULL; > const char *kversion = NULL; >+ int ignore_cmdline = 0; > int use_all = 0; > int do_remove = 0; > int do_show_config = 0; >@@ -881,6 +911,9 @@ static int do_modprobe(int argc, char **orig_argv) > case 'i': > ignore_commands = 1; > break; >+ case 'I': >+ ignore_cmdline = 1; >+ break; > case 'b': > use_blacklist = 1; > break; >@@ -1004,6 +1037,14 @@ static int do_modprobe(int argc, char **orig_argv) > dirname = dirname_buf; > } > >+ if (ignore_cmdline) { >+ if (!clear_cmdline()) { >+ ERR("clear_cmdline() failed!\n"); >+ err = -1; >+ goto done; >+ } >+ } >+ > ctx = kmod_new(dirname, config_paths); > if (!ctx) { > ERR("kmod_new() failed!\n"); >-- >2.39.2 >
diff --git a/libkmod/libkmod-config.c b/libkmod/libkmod-config.c index e24dab1..600d666 100644 --- a/libkmod/libkmod-config.c +++ b/libkmod/libkmod-config.c @@ -949,7 +949,8 @@ int kmod_config_new(struct kmod_ctx *ctx, struct kmod_config **p_config, free(cf); } - kmod_config_parse_kcmdline(config); + if (!(options & KMOD_NEW_IGNORE_CMDLINE)) + kmod_config_parse_kcmdline(config); return 0; diff --git a/libkmod/libkmod-internal.h b/libkmod/libkmod-internal.h index 26a7e28..1d0bd92 100644 --- a/libkmod/libkmod-internal.h +++ b/libkmod/libkmod-internal.h @@ -132,7 +132,7 @@ struct kmod_config { struct kmod_list *paths; }; -int kmod_config_new(struct kmod_ctx *ctx, struct kmod_config **config, const char * const *config_paths) __attribute__((nonnull(1, 2,3))); +int kmod_config_new(struct kmod_ctx *ctx, struct kmod_config **config, const char * const *config_paths, enum kmod_new_flags options) __attribute__((nonnull(1, 2,3))); void kmod_config_free(struct kmod_config *config) __attribute__((nonnull(1))); const char *kmod_blacklist_get_modname(const struct kmod_list *l) __attribute__((nonnull(1))); const char *kmod_alias_get_name(const struct kmod_list *l) __attribute__((nonnull(1))); diff --git a/libkmod/libkmod.c b/libkmod/libkmod.c index 279b35d..126a66c 100644 --- a/libkmod/libkmod.c +++ b/libkmod/libkmod.c @@ -317,7 +317,7 @@ KMOD_EXPORT struct kmod_ctx *kmod_new_flags(const char *dirname, if (config_paths == NULL) config_paths = default_config_paths; - err = kmod_config_new(ctx, &ctx->config, config_paths); + err = kmod_config_new(ctx, &ctx->config, config_paths, options); if (err < 0) { ERR(ctx, "could not create config\n"); goto fail; diff --git a/libkmod/libkmod.h b/libkmod/libkmod.h index 72cd7a2..29637dd 100644 --- a/libkmod/libkmod.h +++ b/libkmod/libkmod.h @@ -30,7 +30,9 @@ extern "C" { #endif -enum kmod_new_flags {}; +enum kmod_new_flags { + KMOD_NEW_IGNORE_CMDLINE = 0x00001, +}; /* * kmod_ctx
This can be passed to kmod_new_flags() to disable loading configuration from /proc/cmdline Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz> --- libkmod/libkmod-config.c | 3 ++- libkmod/libkmod-internal.h | 2 +- libkmod/libkmod.c | 2 +- libkmod/libkmod.h | 4 +++- 4 files changed, 7 insertions(+), 4 deletions(-)