[RFC] [PATCH] Allow overriding module parameters from kernel command_line

[RFC] [PATCH] Allow overriding module parameters from kernel command_line

[Kyle McMartin]

With the move to initramfs and heavily modular configs, which include
loading storage drivers from early userspace, it's becoming harder
to provide users with a way of overriding module parameters at boot.

Currently, users would have to break into the initramfs, edit the
modprobe options, and then let boot continue. They have a much easier time
dealing with adding options on the command line from Grub or what have you.

I hacked out this patch quickly to re-parse saved_command_line[] when we
load a module in an attempt to rectify this.

(The specific use-case I was looking at here was HPA commands failing on
 sata_nv controllers, and needing to pass the adma=0 option to the module...
 Users had a hard time testing without an easy way of overriding the module.)

Clearly this is not entirely optimal, because we're parsing command_line
after the module params are parsed. This ends of being a policy decision,
whether the /sbin/modprobe commandline should override the kernel
command_line, or vice versa.

Anyway, please comment...

Cheers,
	Kyle

diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
index c83588c..e8465db 100644
--- a/include/linux/moduleparam.h
+++ b/include/linux/moduleparam.h
@@ -98,7 +98,7 @@ struct kparam_array
 extern int parse_args(const char *name,
 		      char *args,
 		      struct kernel_param *params,
-		      unsigned num,
+		      unsigned num, unsigned strip_module,
 		      int (*unknown)(char *param, char *val));
 
 /* All the helper functions */
diff --git a/init/main.c b/init/main.c
index a92989e..a3d647f 100644
--- a/init/main.c
+++ b/init/main.c
@@ -478,7 +478,7 @@ void __init parse_early_param(void)
 
 	/* All fall through to do_early_param. */
 	strlcpy(tmp_cmdline, boot_command_line, COMMAND_LINE_SIZE);
-	parse_args("early options", tmp_cmdline, NULL, 0, do_early_param);
+	parse_args("early options", tmp_cmdline, NULL, 0, 0, do_early_param);
 	done = 1;
 }
 
@@ -549,7 +549,7 @@ asmlinkage void __init start_kernel(void)
 	printk(KERN_NOTICE "Kernel command line: %s\n", boot_command_line);
 	parse_early_param();
 	parse_args("Booting kernel", static_command_line, __start___param,
-		   __stop___param - __start___param,
+		   __stop___param - __start___param, 0,
 		   &unknown_bootoption);
 	if (!irqs_disabled()) {
 		printk(KERN_WARNING "start_kernel(): bug: interrupts were "
diff --git a/kernel/module.c b/kernel/module.c
index dcdb32b..e576242 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1914,10 +1914,21 @@ static struct module *load_module(void __user *umod,
 			 sechdrs[setupindex].sh_addr,
 			 sechdrs[setupindex].sh_size
 			 / sizeof(struct kernel_param),
-			 NULL);
+			 0, NULL);
 	if (err < 0)
 		goto arch_cleanup;
 
+	/* Allow user to override module params from the command line. */
+	err = parse_args(mod->name, saved_command_line,
+			 (struct kernel_param *)
+			 sechdrs[setupindex].sh_addr,
+			 sechdrs[setupindex].sh_size
+			 / sizeof(struct kernel_param),
+			 1, NULL);
+	if (err < 0)
+		printk(KERN_WARNING "%s: Ignoring bad module parameters passed on "
+		       "kernel command line\n", mod->name);
+
 	err = mod_sysfs_setup(mod, 
 			      (struct kernel_param *)
 			      sechdrs[setupindex].sh_addr,
diff --git a/kernel/params.c b/kernel/params.c
index 1fc4ac7..7c9d3a0 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -50,10 +50,17 @@ static int parse_one(char *param,
 		     char *val,
 		     struct kernel_param *params, 
 		     unsigned num_params,
-		     int (*handle_unknown)(char *param, char *val))
+		     int (*handle_unknown)(char *param, char *val),
+		     unsigned strip_module)
 {
 	unsigned int i;
 
+	/* If we're parsing command_line[] for the module,
+	 * strip off the module name before looking the param up.
+	 */
+	if (strip_module)
+		strsep(&param, ".");
+
 	/* Find parameter */
 	for (i = 0; i < num_params; i++) {
 		if (parameq(param, params[i].name)) {
@@ -130,7 +137,7 @@ static char *next_arg(char *args, char **param, char **val)
 int parse_args(const char *name,
 	       char *args,
 	       struct kernel_param *params,
-	       unsigned num,
+	       unsigned num, unsigned strip_module,
 	       int (*unknown)(char *param, char *val))
 {
 	char *param, *val;
@@ -147,7 +154,7 @@ int parse_args(const char *name,
 
 		args = next_arg(args, &param, &val);
 		irq_was_disabled = irqs_disabled();
-		ret = parse_one(param, val, params, num, unknown);
+		ret = parse_one(param, val, params, num, unknown, strip_module);
 		if (irq_was_disabled && !irqs_disabled()) {
 			printk(KERN_WARNING "parse_args(): option '%s' enabled "
 					"irq's!\n", param);

Re: [RFC] [PATCH] Allow overriding module parameters from kernel command_line

[Ben Collins]

On Wed, 2007-04-18 at 11:55 -0400, Kyle McMartin wrote:
> With the move to initramfs and heavily modular configs, which include
> loading storage drivers from early userspace, it's becoming harder
> to provide users with a way of overriding module parameters at boot.
> 
> Currently, users would have to break into the initramfs, edit the
> modprobe options, and then let boot continue. They have a much easier time
> dealing with adding options on the command line from Grub or what have you.
> 
> I hacked out this patch quickly to re-parse saved_command_line[] when we
> load a module in an attempt to rectify this.
> 
> (The specific use-case I was looking at here was HPA commands failing on
>  sata_nv controllers, and needing to pass the adma=0 option to the module...
>  Users had a hard time testing without an easy way of overriding the module.)
> 
> Clearly this is not entirely optimal, because we're parsing command_line
> after the module params are parsed. This ends of being a policy decision,
> whether the /sbin/modprobe commandline should override the kernel
> command_line, or vice versa.
> 
> Anyway, please comment...

Just pasting my comments to kyle on IRC for benefit of discussion:

"I'm of the opinion that cmdline args from the kernel should override
modprobe args...mainly because adding those args to the kernel cmdline
is clearly an attempt to override userspace"

"maybe have some sysfs attr to disable this feature, so userspace can go
back to normal after whatever needed fixing"

The use case for us is that we've had to add a couple of initramfs
scripts to parse common module params (all_generic_ide for example).

We'd like to move away from this and have a clear way of passing any
module param on the kernel command line. Workarounds for users are much
easier than having to roll new CDs for every little quirk. Plus it's
easier for testing (if the quirk is a bug that keeps them from
installing at all, then we can't just hand them new kernels very
easily).


-- 
Ubuntu:    http://www.ubuntu.com/
Linux1394: http://www.linux1394.org/

Re: [RFC] [PATCH] Allow overriding module parameters from kernel command_line

[Andrew Morton]

> On Wed, 18 Apr 2007 11:55:52 -0400 Kyle McMartin <kyle@canonical.com> wrote:
> With the move to initramfs and heavily modular configs, which include
> loading storage drivers from early userspace, it's becoming harder
> to provide users with a way of overriding module parameters at boot.
> 
> Currently, users would have to break into the initramfs, edit the
> modprobe options, and then let boot continue. They have a much easier time
> dealing with adding options on the command line from Grub or what have you.
> 
> I hacked out this patch quickly to re-parse saved_command_line[] when we
> load a module in an attempt to rectify this.
> 
> (The specific use-case I was looking at here was HPA commands failing on
>  sata_nv controllers, and needing to pass the adma=0 option to the module...
>  Users had a hard time testing without an easy way of overriding the module.)
> 
> Clearly this is not entirely optimal, because we're parsing command_line
> after the module params are parsed. This ends of being a policy decision,
> whether the /sbin/modprobe commandline should override the kernel
> command_line, or vice versa.

Similar-but-different: I was trying to persuade a Fedora system to use ext2
for the root filesystem the other day.  Turns out that we somehow managed
to break `rootfstype=' in this situation and it cheerfully continued to use
ext3.

Fixed by changing /etc/fstab and rebuilding initrd, but IMO rootfstype=
should have worked.

Re: [RFC] [PATCH] Allow overriding module parameters from kernel command_line

[Neil Brown]

On Wednesday April 18, akpm@linux-foundation.org wrote:
> > On Wed, 18 Apr 2007 11:55:52 -0400 Kyle McMartin <kyle@canonical.com> wrote:
> > With the move to initramfs and heavily modular configs, which include
> > loading storage drivers from early userspace, it's becoming harder
> > to provide users with a way of overriding module parameters at boot.
> > 
> > Currently, users would have to break into the initramfs, edit the
> > modprobe options, and then let boot continue. They have a much easier time
> > dealing with adding options on the command line from Grub or what have you.
> > 
> > I hacked out this patch quickly to re-parse saved_command_line[] when we
> > load a module in an attempt to rectify this.
> > 
> > (The specific use-case I was looking at here was HPA commands failing on
> >  sata_nv controllers, and needing to pass the adma=0 option to the module...
> >  Users had a hard time testing without an easy way of overriding the module.)
> > 
> > Clearly this is not entirely optimal, because we're parsing command_line
> > after the module params are parsed. This ends of being a policy decision,
> > whether the /sbin/modprobe commandline should override the kernel
> > command_line, or vice versa.
> 
> Similar-but-different: I was trying to persuade a Fedora system to use ext2
> for the root filesystem the other day.  Turns out that we somehow managed
> to break `rootfstype=' in this situation and it cheerfully continued to use
> ext3.
> 
> Fixed by changing /etc/fstab and rebuilding initrd, but IMO rootfstype=
> should have worked.

I think these are both issues that should be solved by smarts in the
initrd.

All of the (unused) kernel parameters are in the environment aren't
they? (if not, they can easily be put there).

So maybe insmod/modprobe could be updated to extract relevant options
from the environment.
And the mount of the root filesystem should be called as:

    mount ${rootfstype+-t $rootfstype} $dev $mountpoint

We are depending more and more on initrd and I think it hurts not
having a reference implementation.
Currently each distro makes their own and while I'm sure they are all
quite good in their own way, the fact that they are independent makes
community input harder.

What we really need is a single reference implementation of "mkinitrd"
which each distro can fiddle with to their heart's content.  Then
sensible ideas like the above can be incorporated into the reference,
and all distros will ultimately pick them up.

But unfortunately I don't have the time to volunteer for this role...

NeilBrown

mkinitrd. (was Re: [RFC] [PATCH] Allow overriding module parameters from kernel command_line)

[Dave Jones]

On Thu, Apr 19, 2007 at 08:47:13AM +1000, Neil Brown wrote:
 
 > > Fixed by changing /etc/fstab and rebuilding initrd, but IMO rootfstype=
 > > should have worked.
 > 
 > I think these are both issues that should be solved by smarts in the
 > initrd.

This is getting away from the intent of Kyle's original patch
(Which I think is worthwhile fwiw, having recently hit the exact
 same sata_nv bug that prompted him to write it)

 > What we really need is a single reference implementation of "mkinitrd"
 > which each distro can fiddle with to their heart's content.  Then
 > sensible ideas like the above can be incorporated into the reference,
 > and all distros will ultimately pick them up.
 > 
 > But unfortunately I don't have the time to volunteer for this role...

The problem I see with such a 'one mkinitrd to rule them all', is that
it would suffer from the same thing that stopped any vendor stepping
up and getting behind hpa's klibc project...  Apathy due to "our current
stuff works, why would we throw it all away and start again"

It's a great idea in theory, in practise however, initrd construction
for every distro now contains years of custom hacks and workarounds
(that may not even be relevant on other distros).

Given the critical nature of mkinitrd (get something wrong, and your
system doesn't boot), unsurprisingly, people are reluctant to change
away from something they're familar with, unless there's a *really*
compelling reason.

	Dave

-- 
http://www.codemonkey.org.uk