Re: [RFC PATCH] Allow optional module parameters

Re: [RFC PATCH] Allow optional module parameters

[Rusty Russell]

Andy Lutomirski <luto@amacapital.net> writes:
> On Thu, Mar 14, 2013 at 10:03 PM, Rusty Russell <rusty@rustcorp.com.au> wrote:
>> Andy Lutomirski <luto@amacapital.net> writes:
>>> Current parameter behavior is odd.  Boot parameters that have values
>>> and don't match anything become environment variables, with no
>>> warning.  Boot parameters without values that don't match anything
>>> go into argv_init.  Everything goes into /proc/cmdline.
>>>
>>> The init_module and finit_module syscalls, however, are strict:
>>> parameters that don't match result in -ENOENT.
>>>
>>> kmod (and hence modprobe), when loading a module called foo, look in
>>> /proc/cmdline for foo.x or foo.x=y, strip off the foo., and pass the
>>> rest to init_module.
>>>
>>> The upshot is that booting with module.nonexistent_parameter=1 is
>>> okay if module is built in or missing entirely but prevents module
>>> from loading if it's an actual module.  Similarly, option module
>>> nonexistent_parameter=1 in /etc/modprobe.d prevents the module from
>>> loading the parameter goes away.  This means that removing module
>>> parameters unnecessarily breaks things.
>>
>> Err, yes.  Don't remove module parameters, they're part of the API.  Do
>> you have a particular example?
>
> So things like i915.i915_enable_ppgtt, which is there to enable
> something experimental, needs to stay forever once the relevant
> feature becomes non-experimental and non-optional?  This seems silly.

Well, it's either experimental, in which case you're happy to break
users who use it, or it's not, in which case, don't.

> Having the module parameter go away while still allowing the module to
> load seems like a good solution (possibly with a warning in the logs
> so the user can eventually delete the parameter).

Why not do that for *every* missing parameter then?  Why have this weird
notation where the user must know that the parameter might one day go
away?

> In any case, I find it odd that the only parameters you can set that
> cause errors when the parameter is deleted are parameters for modules
> that are built as modules.

We could definitely expand the check: we should check for unknown
parameters to builtin modules.

This refers back to my question on lkml 'Re: No sysfs directory for
openvswitch module when built-in' as the kernel doesn't currently know
what modules are builtin.  We intuit it from parameter names, which is
sloppy.

Here's a rough patch.  Maybe someone on linux-kbuild can tell me why it
fails?

DOESNT_WORK: kernel: generate list of builtin modnames.

For some reason, the only contents of modules.builtin when this runs is
one line "kernel/kernel/configs.ko"?

This lets us emit an error when invalid boot parameters are used, since
we know what names can never be external modules.

diff --git a/include/linux/module.h b/include/linux/module.h
index 46f1ea0..3433f6b 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -186,6 +186,9 @@ struct notifier_block;
 
 #ifdef CONFIG_MODULES
 
+/* In generated file kernel/modnamelist.c */
+extern const char *builtin_modnames[];
+
 extern int modules_disabled; /* for sysctl */
 /* Get/put a kernel symbol (calls must be symmetric) */
 void *__symbol_get(const char *symbol);
diff --git a/init/main.c b/init/main.c
index 63534a1..e4dec79 100644
--- a/init/main.c
+++ b/init/main.c
@@ -245,12 +245,25 @@ static int __init repair_env_string(char *param, char *val, const char *unused)
 	return 0;
 }
 
+static bool builtin_modname(const char *name, size_t len)
+{
+	const char **n;
+
+	for (n = builtin_modnames; *n; n++) {
+		if (strncmp(name, *n, len) == 0 && !(*n)[len])
+			return true;
+	}
+	return false;
+}
+
 /*
  * Unknown boot options get handed to init, unless they look like
  * unused parameters (modprobe will find them in /proc/cmdline).
  */
 static int __init unknown_bootoption(char *param, char *val, const char *unused)
 {
+	size_t modname_len = strcspn(param, ".");
+
 	repair_env_string(param, val, unused);
 
 	/* Handle obsolete-style parameters */
@@ -258,8 +271,13 @@ static int __init unknown_bootoption(char *param, char *val, const char *unused)
 		return 0;
 
 	/* Unused module parameter. */
-	if (strchr(param, '.') && (!val || strchr(param, '.') < val))
+	if (param[modname_len] == '.') {
+		if (builtin_modname(param, modname_len)) {
+			printk(KERN_ERR "Unknown kernel parameter %s\n", param);
+			return -EINVAL;
+		}
 		return 0;
+	}
 
 	if (panic_later)
 		return 0;
diff --git a/kernel/.gitignore b/kernel/.gitignore
index ab4f109..df5b7c7 100644
--- a/kernel/.gitignore
+++ b/kernel/.gitignore
@@ -4,3 +4,4 @@
 config_data.h
 config_data.gz
 timeconst.h
+modnamelist.c
diff --git a/kernel/Makefile b/kernel/Makefile
index bbde5f1..3f3dad7 100644
--- a/kernel/Makefile
+++ b/kernel/Makefile
@@ -10,7 +10,7 @@ obj-y     = fork.o exec_domain.o panic.o printk.o \
 	    kthread.o wait.o sys_ni.o posix-cpu-timers.o mutex.o \
 	    hrtimer.o rwsem.o nsproxy.o srcu.o semaphore.o \
 	    notifier.o ksysfs.o cred.o \
-	    async.o range.o groups.o lglock.o smpboot.o
+	    async.o range.o groups.o lglock.o smpboot.o modnamelist.o
 
 ifdef CONFIG_FUNCTION_TRACER
 # Do not trace debug files and internal ftrace files
@@ -139,6 +139,13 @@ targets += timeconst.h
 $(obj)/timeconst.h: $(obj)/hz.bc $(src)/timeconst.bc FORCE
 	$(call if_changed,bc)
 
+quiet_cmd_modnamelist  = MODNAMELIST      $@
+      cmd_modnamelist  = sh scripts/modnamelist.sh $@ $(src)/modules.builtin
+
+# Must match name-fix in scripts/Makefile.lib!
+kernel/modnamelist.c: modules.builtin FORCE
+	$(call if_changed,modnamelist)
+
 ifeq ($(CONFIG_MODULE_SIG),y)
 #
 # Pull the signing certificate and any extra certificates into the kernel
diff --git a/scripts/modnamelist.sh b/scripts/modnamelist.sh
new file mode 100755
index 0000000..5d06820
--- /dev/null
+++ b/scripts/modnamelist.sh
@@ -0,0 +1,22 @@
+#! /bin/sh
+set -e
+
+OUT="$1"
+LIST="$2"
+
+trap "rm -f $OUT.tmp" 0
+
+cat > $OUT.tmp <<EOF
+/* Autogenerated list of builtin modules. */
+#include <linux/module.h>
+
+const char *builtin_modnames[] = {
+EOF
+
+sed -e 's@.*/\(.*\)\.ko@\t"\1"@' -e 's@[,-]@_@g' -e 's@$@,@' < $LIST >> $OUT.tmp
+cat >> $OUT.tmp <<EOF
+	NULL
+};
+EOF
+
+mv $OUT.tmp $OUT

Re: [RFC PATCH] Allow optional module parameters

[Andy Lutomirski]

On Sun, Mar 17, 2013 at 7:24 PM, Rusty Russell <rusty@rustcorp.com.au> wrote:
> Andy Lutomirski <luto@amacapital.net> writes:
>> On Thu, Mar 14, 2013 at 10:03 PM, Rusty Russell <rusty@rustcorp.com.au> wrote:
>>> Andy Lutomirski <luto@amacapital.net> writes:
>>>> Current parameter behavior is odd.  Boot parameters that have values
>>>> and don't match anything become environment variables, with no
>>>> warning.  Boot parameters without values that don't match anything
>>>> go into argv_init.  Everything goes into /proc/cmdline.
>>>>
>>>> The init_module and finit_module syscalls, however, are strict:
>>>> parameters that don't match result in -ENOENT.
>>>>
>>>> kmod (and hence modprobe), when loading a module called foo, look in
>>>> /proc/cmdline for foo.x or foo.x=y, strip off the foo., and pass the
>>>> rest to init_module.
>>>>
>>>> The upshot is that booting with module.nonexistent_parameter=1 is
>>>> okay if module is built in or missing entirely but prevents module
>>>> from loading if it's an actual module.  Similarly, option module
>>>> nonexistent_parameter=1 in /etc/modprobe.d prevents the module from
>>>> loading the parameter goes away.  This means that removing module
>>>> parameters unnecessarily breaks things.
>>>
>>> Err, yes.  Don't remove module parameters, they're part of the API.  Do
>>> you have a particular example?
>>
>> So things like i915.i915_enable_ppgtt, which is there to enable
>> something experimental, needs to stay forever once the relevant
>> feature becomes non-experimental and non-optional?  This seems silly.
>
> Well, it's either experimental, in which case you're happy to break
> users who use it, or it's not, in which case, don't.

Sure.  The main issue for me annoyance.  Install kernel with
experimental option.  Set that option in grub's config.  Boot another
kernel.  Grr, the option got propagated there and now I have no
graphics.

>
>> Having the module parameter go away while still allowing the module to
>> load seems like a good solution (possibly with a warning in the logs
>> so the user can eventually delete the parameter).
>
> Why not do that for *every* missing parameter then?  Why have this weird
> notation where the user must know that the parameter might one day go
> away?

Fair enough.  What about the other approach, then?  Always warn if an
option doesn't match (built-in or otherwise) but load the module
anyways.

--Andy

Re: [RFC PATCH] Allow optional module parameters

[Rusty Russell]

Andy Lutomirski <luto@amacapital.net> writes:
> On Sun, Mar 17, 2013 at 7:24 PM, Rusty Russell <rusty@rustcorp.com.au> wrote:
>> Andy Lutomirski <luto@amacapital.net> writes:
>>> On Thu, Mar 14, 2013 at 10:03 PM, Rusty Russell <rusty@rustcorp.com.au> wrote:
>>>> Err, yes.  Don't remove module parameters, they're part of the API.  Do
>>>> you have a particular example?
>>>
>>> So things like i915.i915_enable_ppgtt, which is there to enable
>>> something experimental, needs to stay forever once the relevant
>>> feature becomes non-experimental and non-optional?  This seems silly.
...
>>> Having the module parameter go away while still allowing the module to
>>> load seems like a good solution (possibly with a warning in the logs
>>> so the user can eventually delete the parameter).
>>
>> Why not do that for *every* missing parameter then?  Why have this weird
>> notation where the user must know that the parameter might one day go
>> away?
>
> Fair enough.  What about the other approach, then?  Always warn if an
> option doesn't match (built-in or otherwise) but load the module
> anyways.

What does everyone think of this?  Jon, Lucas, does this match your
experience?

Thanks,
Rusty.

Subject: modules: don't fail to load on unknown parameters.

Although parameters are supposed to be part of the kernel API, experimental
parameters are often removed.  In addition, downgrading a kernel might cause
previously-working modules to fail to load.

On balance, it's probably better to warn, and load the module anyway.

Reported-by: Andy Lutomirski <luto@amacapital.net>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

diff --git a/kernel/module.c b/kernel/module.c
index 3c2c72d..46db10a 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3206,6 +3206,17 @@ out:
 	return err;
 }
 
+static int unknown_module_param_cb(char *param, char *val, const char *modname)
+{
+	/* Check for magic 'dyndbg' arg */ 
+	int ret = ddebug_dyndbg_module_param_cb(param, val, modname);
+	if (ret != 0) {
+		printk(KERN_WARNING "%s: unknown parameter '%s' ignored\n",
+		       modname, param);
+	}
+	return 0;
+}
+
 /* Allocate and load the module: note that size of section 0 is always
    zero, and we rely on this for optional sections. */
 static int load_module(struct load_info *info, const char __user *uargs,
@@ -3292,7 +3303,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
 
 	/* Module is ready to execute: parsing args may do that. */
 	err = parse_args(mod->name, mod->args, mod->kp, mod->num_kp,
-			 -32768, 32767, &ddebug_dyndbg_module_param_cb);
+			 -32768, 32767, unknown_module_param_cb);
 	if (err < 0)
 		goto bug_cleanup;
 

Re: [RFC PATCH] Allow optional module parameters

[Ben Hutchings]

On Tue, 2013-03-19 at 13:02 +1030, Rusty Russell wrote:
> Andy Lutomirski <luto@amacapital.net> writes:
> > On Sun, Mar 17, 2013 at 7:24 PM, Rusty Russell <rusty@rustcorp.com.au> wrote:
> >> Andy Lutomirski <luto@amacapital.net> writes:
> >>> On Thu, Mar 14, 2013 at 10:03 PM, Rusty Russell <rusty@rustcorp.com.au> wrote:
> >>>> Err, yes.  Don't remove module parameters, they're part of the API.  Do
> >>>> you have a particular example?
> >>>
> >>> So things like i915.i915_enable_ppgtt, which is there to enable
> >>> something experimental, needs to stay forever once the relevant
> >>> feature becomes non-experimental and non-optional?  This seems silly.
> ...
> >>> Having the module parameter go away while still allowing the module to
> >>> load seems like a good solution (possibly with a warning in the logs
> >>> so the user can eventually delete the parameter).
> >>
> >> Why not do that for *every* missing parameter then?  Why have this weird
> >> notation where the user must know that the parameter might one day go
> >> away?
> >
> > Fair enough.  What about the other approach, then?  Always warn if an
> > option doesn't match (built-in or otherwise) but load the module
> > anyways.
> 
> What does everyone think of this?  Jon, Lucas, does this match your
> experience?

I'm not sure why I'm being cc'd on this, though I did recently remove a
module parameter (sfc.rx_alloc_method).  For what it's worth:

> Subject: modules: don't fail to load on unknown parameters.
> 
> Although parameters are supposed to be part of the kernel API, experimental
> parameters are often removed.  In addition, downgrading a kernel might cause
> previously-working modules to fail to load.
> 
> On balance, it's probably better to warn, and load the module anyway.

I agree with this.

> Reported-by: Andy Lutomirski <luto@amacapital.net>
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
[...]

This should also go to stable, so the downgrading issue doesn't continue
to bite people.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [RFC PATCH] Allow optional module parameters

[Rusty Russell]

Ben Hutchings <bhutchings@solarflare.com> writes:
> This should also go to stable, so the downgrading issue doesn't continue
> to bite people.

Andy was complaining about experimental params going away: I haven't
heard a single complaint about the downgrading issue.  I think it's a
nice to have, which is why I mentioned it.  I also CC'd the two
maintainers most likely to know if it *is* a current problem.  And note
that this code has been this way for over ten years!

No bug report, no cc:stable.

What if people were relying on detecting module parameters by the load
failing?  I'd rather find out when they upgrade to a new kernel instead
of poisoning the old ones, too.

Cheers,
Rusty.

Re: [RFC PATCH] Allow optional module parameters

[Lucas De Marchi]

Hi Rusty,

On Mon, Mar 18, 2013 at 11:32 PM, Rusty Russell <rusty@rustcorp.com.au> wrote:
> Andy Lutomirski <luto@amacapital.net> writes:
>> On Sun, Mar 17, 2013 at 7:24 PM, Rusty Russell <rusty@rustcorp.com.au> wrote:
>>> Andy Lutomirski <luto@amacapital.net> writes:
>>>> On Thu, Mar 14, 2013 at 10:03 PM, Rusty Russell <rusty@rustcorp.com.au> wrote:
>>>>> Err, yes.  Don't remove module parameters, they're part of the API.  Do
>>>>> you have a particular example?
>>>>
>>>> So things like i915.i915_enable_ppgtt, which is there to enable
>>>> something experimental, needs to stay forever once the relevant
>>>> feature becomes non-experimental and non-optional?  This seems silly.
> ...
>>>> Having the module parameter go away while still allowing the module to
>>>> load seems like a good solution (possibly with a warning in the logs
>>>> so the user can eventually delete the parameter).
>>>
>>> Why not do that for *every* missing parameter then?  Why have this weird
>>> notation where the user must know that the parameter might one day go
>>> away?
>>
>> Fair enough.  What about the other approach, then?  Always warn if an
>> option doesn't match (built-in or otherwise) but load the module
>> anyways.
>
> What does everyone think of this?  Jon, Lucas, does this match your
> experience?
>
> Thanks,
> Rusty.
>
> Subject: modules: don't fail to load on unknown parameters.
>
> Although parameters are supposed to be part of the kernel API, experimental
> parameters are often removed.  In addition, downgrading a kernel might cause
> previously-working modules to fail to load.

I agree with this reasoning

>
> On balance, it's probably better to warn, and load the module anyway.

However loading the module anyway would bring at least one drawback:
if the user made a typo when passing the option the module would load
anyway and he will probably not even look in the log, since there's
was no errors from modprobe.

For finit_module we could put a flag to trigger this behavior and
propagate it to modprobe, but this is not possible with init_module().
I can't think in any other option right now... do you have any?


Lucas De Marchi

Re: [RFC PATCH] Allow optional module parameters

[Andy Lutomirski]

On Tue, Mar 19, 2013 at 8:26 PM, Lucas De Marchi
<lucas.demarchi@profusion.mobi> wrote:
> Hi Rusty,
>
> On Mon, Mar 18, 2013 at 11:32 PM, Rusty Russell <rusty@rustcorp.com.au> wrote:
>> Andy Lutomirski <luto@amacapital.net> writes:
>>> On Sun, Mar 17, 2013 at 7:24 PM, Rusty Russell <rusty@rustcorp.com.au> wrote:
>>>> Andy Lutomirski <luto@amacapital.net> writes:
>>>>> On Thu, Mar 14, 2013 at 10:03 PM, Rusty Russell <rusty@rustcorp.com.au> wrote:
>>>>>> Err, yes.  Don't remove module parameters, they're part of the API.  Do
>>>>>> you have a particular example?
>>>>>
>>>>> So things like i915.i915_enable_ppgtt, which is there to enable
>>>>> something experimental, needs to stay forever once the relevant
>>>>> feature becomes non-experimental and non-optional?  This seems silly.
>> ...
>>>>> Having the module parameter go away while still allowing the module to
>>>>> load seems like a good solution (possibly with a warning in the logs
>>>>> so the user can eventually delete the parameter).
>>>>
>>>> Why not do that for *every* missing parameter then?  Why have this weird
>>>> notation where the user must know that the parameter might one day go
>>>> away?
>>>
>>> Fair enough.  What about the other approach, then?  Always warn if an
>>> option doesn't match (built-in or otherwise) but load the module
>>> anyways.
>>
>> What does everyone think of this?  Jon, Lucas, does this match your
>> experience?
>>
>> Thanks,
>> Rusty.
>>
>> Subject: modules: don't fail to load on unknown parameters.
>>
>> Although parameters are supposed to be part of the kernel API, experimental
>> parameters are often removed.  In addition, downgrading a kernel might cause
>> previously-working modules to fail to load.
>
> I agree with this reasoning
>
>>
>> On balance, it's probably better to warn, and load the module anyway.
>
> However loading the module anyway would bring at least one drawback:
> if the user made a typo when passing the option the module would load
> anyway and he will probably not even look in the log, since there's
> was no errors from modprobe.
>
> For finit_module we could put a flag to trigger this behavior and
> propagate it to modprobe, but this is not possible with init_module().
> I can't think in any other option right now... do you have any?

Have a different finit_module return value for "successfully loaded,
but there were warnings" perhaps?

--Andy

>
>
> Lucas De Marchi



-- 
Andy Lutomirski
AMA Capital Management, LLC

Re: [RFC PATCH] Allow optional module parameters

[Andy Lutomirski]

On Tue, Mar 19, 2013 at 8:32 PM, Andy Lutomirski <luto@amacapital.net> wrote:
> On Tue, Mar 19, 2013 at 8:26 PM, Lucas De Marchi
> <lucas.demarchi@profusion.mobi> wrote:
>> Hi Rusty,
>>
>> On Mon, Mar 18, 2013 at 11:32 PM, Rusty Russell <rusty@rustcorp.com.au> wrote:
>>> Andy Lutomirski <luto@amacapital.net> writes:
>>>> On Sun, Mar 17, 2013 at 7:24 PM, Rusty Russell <rusty@rustcorp.com.au> wrote:
>>>>> Andy Lutomirski <luto@amacapital.net> writes:
>>>>>> On Thu, Mar 14, 2013 at 10:03 PM, Rusty Russell <rusty@rustcorp.com.au> wrote:
>>>>>>> Err, yes.  Don't remove module parameters, they're part of the API.  Do
>>>>>>> you have a particular example?
>>>>>>
>>>>>> So things like i915.i915_enable_ppgtt, which is there to enable
>>>>>> something experimental, needs to stay forever once the relevant
>>>>>> feature becomes non-experimental and non-optional?  This seems silly.
>>> ...
>>>>>> Having the module parameter go away while still allowing the module to
>>>>>> load seems like a good solution (possibly with a warning in the logs
>>>>>> so the user can eventually delete the parameter).
>>>>>
>>>>> Why not do that for *every* missing parameter then?  Why have this weird
>>>>> notation where the user must know that the parameter might one day go
>>>>> away?
>>>>
>>>> Fair enough.  What about the other approach, then?  Always warn if an
>>>> option doesn't match (built-in or otherwise) but load the module
>>>> anyways.
>>>
>>> What does everyone think of this?  Jon, Lucas, does this match your
>>> experience?
>>>
>>> Thanks,
>>> Rusty.
>>>
>>> Subject: modules: don't fail to load on unknown parameters.
>>>
>>> Although parameters are supposed to be part of the kernel API, experimental
>>> parameters are often removed.  In addition, downgrading a kernel might cause
>>> previously-working modules to fail to load.
>>
>> I agree with this reasoning
>>
>>>
>>> On balance, it's probably better to warn, and load the module anyway.
>>
>> However loading the module anyway would bring at least one drawback:
>> if the user made a typo when passing the option the module would load
>> anyway and he will probably not even look in the log, since there's
>> was no errors from modprobe.
>>
>> For finit_module we could put a flag to trigger this behavior and
>> propagate it to modprobe, but this is not possible with init_module().
>> I can't think in any other option right now... do you have any?
>
> Have a different finit_module return value for "successfully loaded,
> but there were warnings" perhaps?

Never mind.  I was thinking that finit_module was new in 3.9.

--Andy

Re: [RFC PATCH] Allow optional module parameters

[Rusty Russell]

Lucas De Marchi <lucas.demarchi@profusion.mobi> writes:
> Hi Rusty,
>
> On Mon, Mar 18, 2013 at 11:32 PM, Rusty Russell <rusty@rustcorp.com.au> wrote:
>> Andy Lutomirski <luto@amacapital.net> writes:
>>> On Sun, Mar 17, 2013 at 7:24 PM, Rusty Russell <rusty@rustcorp.com.au> wrote:
>>>> Andy Lutomirski <luto@amacapital.net> writes:
>>>>> On Thu, Mar 14, 2013 at 10:03 PM, Rusty Russell <rusty@rustcorp.com.au> wrote:
>>>>>> Err, yes.  Don't remove module parameters, they're part of the API.  Do
>>>>>> you have a particular example?
>>>>>
>>>>> So things like i915.i915_enable_ppgtt, which is there to enable
>>>>> something experimental, needs to stay forever once the relevant
>>>>> feature becomes non-experimental and non-optional?  This seems silly.
>> ...
>>>>> Having the module parameter go away while still allowing the module to
>>>>> load seems like a good solution (possibly with a warning in the logs
>>>>> so the user can eventually delete the parameter).
>>>>
>>>> Why not do that for *every* missing parameter then?  Why have this weird
>>>> notation where the user must know that the parameter might one day go
>>>> away?
>>>
>>> Fair enough.  What about the other approach, then?  Always warn if an
>>> option doesn't match (built-in or otherwise) but load the module
>>> anyways.
>>
>> What does everyone think of this?  Jon, Lucas, does this match your
>> experience?
>>
>> Thanks,
>> Rusty.
>>
>> Subject: modules: don't fail to load on unknown parameters.
>>
>> Although parameters are supposed to be part of the kernel API, experimental
>> parameters are often removed.  In addition, downgrading a kernel might cause
>> previously-working modules to fail to load.
>
> I agree with this reasoning
>
>>
>> On balance, it's probably better to warn, and load the module anyway.
>
> However loading the module anyway would bring at least one drawback:
> if the user made a typo when passing the option the module would load
> anyway and he will probably not even look in the log, since there's
> was no errors from modprobe.
>
> For finit_module we could put a flag to trigger this behavior and
> propagate it to modprobe, but this is not possible with init_module().
> I can't think in any other option right now... do you have any?

No good ones :(

MODULE_PARM_DESC isn't compulsory, so you can't rely on that to tell you
about option names.  

Even if we had a flag, how would you know to set it?  I guess you could
try without then try with, and if it works the second time print a
warning about typos.  But it's still pretty ugly.

We could implement such a flag with a fake "IGNORE_BAD_PARAMS"
parameter, for example.  That would fail nicely on older kernels, too.

Hmmm....
Rusty.

Re: [RFC PATCH] Allow optional module parameters

[Rusty Russell]

Rusty Russell <rusty@rustcorp.com.au> writes:
> Lucas De Marchi <lucas.demarchi@profusion.mobi> writes:
>> Hi Rusty,
>>
>> On Mon, Mar 18, 2013 at 11:32 PM, Rusty Russell <rusty@rustcorp.com.au> wrote:
>>> Andy Lutomirski <luto@amacapital.net> writes:
>>>> On Sun, Mar 17, 2013 at 7:24 PM, Rusty Russell <rusty@rustcorp.com.au> wrote:
>>>>> Andy Lutomirski <luto@amacapital.net> writes:
>>>>>> On Thu, Mar 14, 2013 at 10:03 PM, Rusty Russell <rusty@rustcorp.com.au> wrote:
>>>>>>> Err, yes.  Don't remove module parameters, they're part of the API.  Do
>>>>>>> you have a particular example?
>>>>>>
>>>>>> So things like i915.i915_enable_ppgtt, which is there to enable
>>>>>> something experimental, needs to stay forever once the relevant
>>>>>> feature becomes non-experimental and non-optional?  This seems silly.
>>> ...
>>>>>> Having the module parameter go away while still allowing the module to
>>>>>> load seems like a good solution (possibly with a warning in the logs
>>>>>> so the user can eventually delete the parameter).
>>>>>
>>>>> Why not do that for *every* missing parameter then?  Why have this weird
>>>>> notation where the user must know that the parameter might one day go
>>>>> away?
>>>>
>>>> Fair enough.  What about the other approach, then?  Always warn if an
>>>> option doesn't match (built-in or otherwise) but load the module
>>>> anyways.
>>>
>>> What does everyone think of this?  Jon, Lucas, does this match your
>>> experience?
>>>
>>> Thanks,
>>> Rusty.
>>>
>>> Subject: modules: don't fail to load on unknown parameters.
>>>
>>> Although parameters are supposed to be part of the kernel API, experimental
>>> parameters are often removed.  In addition, downgrading a kernel might cause
>>> previously-working modules to fail to load.
>>
>> I agree with this reasoning
>>
>>>
>>> On balance, it's probably better to warn, and load the module anyway.
>>
>> However loading the module anyway would bring at least one drawback:
>> if the user made a typo when passing the option the module would load
>> anyway and he will probably not even look in the log, since there's
>> was no errors from modprobe.

OK, so I've had this patch on the backburner, but noone has come up with
anything better so I'll queue it into modules-next now.

Thanks,
Rusty.

modules: don't fail to load on unknown parameters.

Although parameters are supposed to be part of the kernel API, experimental
parameters are often removed.  In addition, downgrading a kernel might cause
previously-working modules to fail to load.

On balance, it's probably better to warn, and load the module anyway.
This may let through a typo, but at least the logs will show it.

Reported-by: Andy Lutomirski <luto@amacapital.net>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

diff --git a/kernel/module.c b/kernel/module.c
index 3c2c72d..46db10a 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3206,6 +3206,17 @@ out:
 	return err;
 }
 
+static int unknown_module_param_cb(char *param, char *val, const char *modname)
+{
+	/* Check for magic 'dyndbg' arg */ 
+	int ret = ddebug_dyndbg_module_param_cb(param, val, modname);
+	if (ret != 0) {
+		printk(KERN_WARNING "%s: unknown parameter '%s' ignored\n",
+		       modname, param);
+	}
+	return 0;
+}
+
 /* Allocate and load the module: note that size of section 0 is always
    zero, and we rely on this for optional sections. */
 static int load_module(struct load_info *info, const char __user *uargs,
@@ -3292,7 +3303,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
 
 	/* Module is ready to execute: parsing args may do that. */
 	err = parse_args(mod->name, mod->args, mod->kp, mod->num_kp,
-			 -32768, 32767, &ddebug_dyndbg_module_param_cb);
+			 -32768, 32767, unknown_module_param_cb);
 	if (err < 0)
 		goto bug_cleanup;
 

Re: [RFC PATCH] Allow optional module parameters

[Jonathan Masters]

One caveat. Sometimes we have manufactured parameters intentionally to cause a module to fail. We should standardize that piece.

-- 
Sent from my iPad

On Jul 1, 2013, at 4:53, Rusty Russell <rusty@rustcorp.com.au> wrote:

> Rusty Russell <rusty@rustcorp.com.au> writes:
>> Lucas De Marchi <lucas.demarchi@profusion.mobi> writes:
>>> Hi Rusty,
>>> 
>>> On Mon, Mar 18, 2013 at 11:32 PM, Rusty Russell <rusty@rustcorp.com.au> wrote:
>>>> Andy Lutomirski <luto@amacapital.net> writes:
>>>>> On Sun, Mar 17, 2013 at 7:24 PM, Rusty Russell <rusty@rustcorp.com.au> wrote:
>>>>>> Andy Lutomirski <luto@amacapital.net> writes:
>>>>>>> On Thu, Mar 14, 2013 at 10:03 PM, Rusty Russell <rusty@rustcorp.com.au> wrote:
>>>>>>>> Err, yes.  Don't remove module parameters, they're part of the API.  Do
>>>>>>>> you have a particular example?
>>>>>>> 
>>>>>>> So things like i915.i915_enable_ppgtt, which is there to enable
>>>>>>> something experimental, needs to stay forever once the relevant
>>>>>>> feature becomes non-experimental and non-optional?  This seems silly.
>>>> ...
>>>>>>> Having the module parameter go away while still allowing the module to
>>>>>>> load seems like a good solution (possibly with a warning in the logs
>>>>>>> so the user can eventually delete the parameter).
>>>>>> 
>>>>>> Why not do that for *every* missing parameter then?  Why have this weird
>>>>>> notation where the user must know that the parameter might one day go
>>>>>> away?
>>>>> 
>>>>> Fair enough.  What about the other approach, then?  Always warn if an
>>>>> option doesn't match (built-in or otherwise) but load the module
>>>>> anyways.
>>>> 
>>>> What does everyone think of this?  Jon, Lucas, does this match your
>>>> experience?
>>>> 
>>>> Thanks,
>>>> Rusty.
>>>> 
>>>> Subject: modules: don't fail to load on unknown parameters.
>>>> 
>>>> Although parameters are supposed to be part of the kernel API, experimental
>>>> parameters are often removed.  In addition, downgrading a kernel might cause
>>>> previously-working modules to fail to load.
>>> 
>>> I agree with this reasoning
>>> 
>>>> 
>>>> On balance, it's probably better to warn, and load the module anyway.
>>> 
>>> However loading the module anyway would bring at least one drawback:
>>> if the user made a typo when passing the option the module would load
>>> anyway and he will probably not even look in the log, since there's
>>> was no errors from modprobe.
> 
> OK, so I've had this patch on the backburner, but noone has come up with
> anything better so I'll queue it into modules-next now.
> 
> Thanks,
> Rusty.
> 
> modules: don't fail to load on unknown parameters.
> 
> Although parameters are supposed to be part of the kernel API, experimental
> parameters are often removed.  In addition, downgrading a kernel might cause
> previously-working modules to fail to load.
> 
> On balance, it's probably better to warn, and load the module anyway.
> This may let through a typo, but at least the logs will show it.
> 
> Reported-by: Andy Lutomirski <luto@amacapital.net>
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> 
> diff --git a/kernel/module.c b/kernel/module.c
> index 3c2c72d..46db10a 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -3206,6 +3206,17 @@ out:
>    return err;
> }
> 
> +static int unknown_module_param_cb(char *param, char *val, const char *modname)
> +{
> +    /* Check for magic 'dyndbg' arg */ 
> +    int ret = ddebug_dyndbg_module_param_cb(param, val, modname);
> +    if (ret != 0) {
> +        printk(KERN_WARNING "%s: unknown parameter '%s' ignored\n",
> +               modname, param);
> +    }
> +    return 0;
> +}
> +
> /* Allocate and load the module: note that size of section 0 is always
>    zero, and we rely on this for optional sections. */
> static int load_module(struct load_info *info, const char __user *uargs,
> @@ -3292,7 +3303,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
> 
>    /* Module is ready to execute: parsing args may do that. */
>    err = parse_args(mod->name, mod->args, mod->kp, mod->num_kp,
> -             -32768, 32767, &ddebug_dyndbg_module_param_cb);
> +             -32768, 32767, unknown_module_param_cb);
>    if (err < 0)
>        goto bug_cleanup;
> 
> 

Re: [RFC PATCH] Allow optional module parameters

[Rusty Russell]

Jonathan Masters <jcm@redhat.com> writes:
> One caveat. Sometimes we have manufactured parameters intentionally to cause a module to fail. We should standardize that piece.

Certainly.  Can you give an example?

Cheers,
Rusty.

Re: [RFC PATCH] Allow optional module parameters

[Michal Marek]

Dne 1.7.2013 18:33, Jonathan Masters napsal(a):
> One caveat. Sometimes we have manufactured parameters intentionally
> to cause a module to fail. We should standardize that piece.

You have:

  blacklist foo

to prevent udev from loading a module and

  install foo /bin/true

to prevent modprobe from loading the module at all. What is the
motivation for inventing a third way, through adding invalid parameters?

Thanks,
Michal

Re: [RFC PATCH] Allow optional module parameters

[Andy Lutomirski]

On Wed, Jul 3, 2013 at 2:03 PM, Michal Marek <mmarek@suse.cz> wrote:
> Dne 1.7.2013 18:33, Jonathan Masters napsal(a):
>> One caveat. Sometimes we have manufactured parameters intentionally
>> to cause a module to fail. We should standardize that piece.
>
> You have:
>
>   blacklist foo
>
> to prevent udev from loading a module and
>
>   install foo /bin/true
>
> to prevent modprobe from loading the module at all. What is the
> motivation for inventing a third way, through adding invalid parameters?
>

FWIW, I've occasionally booted with modulename.garbage=1 to prevent
modulename from loading at boot.  It may be worth adding a more
intentional way to do that.

--Andy

-- 
Andy Lutomirski
AMA Capital Management, LLC

Re: [RFC PATCH] Allow optional module parameters

[Michal Marek]

Dne 3.7.2013 23:17, Andy Lutomirski napsal(a):
> On Wed, Jul 3, 2013 at 2:03 PM, Michal Marek <mmarek@suse.cz> wrote:
>> Dne 1.7.2013 18:33, Jonathan Masters napsal(a):
>>> One caveat. Sometimes we have manufactured parameters intentionally
>>> to cause a module to fail. We should standardize that piece.
>>
>> You have:
>>
>>   blacklist foo
>>
>> to prevent udev from loading a module and
>>
>>   install foo /bin/true
>>
>> to prevent modprobe from loading the module at all. What is the
>> motivation for inventing a third way, through adding invalid parameters?
>>
> 
> FWIW, I've occasionally booted with modulename.garbage=1 to prevent
> modulename from loading at boot.  It may be worth adding a more
> intentional way to do that.

Hm, right, there seems to be no clean way to achieve this via a
commandline argument. Maybe define a magic module option to tell the
module loader not to load a module?

Thanks,
Michal

Re: [RFC PATCH] Allow optional module parameters

[Yann E. MORIN]

Michal, All,

On 2013-07-03 23:23 +0200, Michal Marek spake thusly:
> Dne 3.7.2013 23:17, Andy Lutomirski napsal(a):
> > On Wed, Jul 3, 2013 at 2:03 PM, Michal Marek <mmarek@suse.cz> wrote:
> >> Dne 1.7.2013 18:33, Jonathan Masters napsal(a):
> >>> One caveat. Sometimes we have manufactured parameters intentionally
> >>> to cause a module to fail. We should standardize that piece.
> >>
> >> You have:
> >>
> >>   blacklist foo
> >>
> >> to prevent udev from loading a module and
> >>
> >>   install foo /bin/true
> >>
> >> to prevent modprobe from loading the module at all. What is the
> >> motivation for inventing a third way, through adding invalid parameters?
> >>
> > 
> > FWIW, I've occasionally booted with modulename.garbage=1 to prevent
> > modulename from loading at boot.  It may be worth adding a more
> > intentional way to do that.
> 
> Hm, right, there seems to be no clean way to achieve this via a
> commandline argument. Maybe define a magic module option to tell the
> module loader not to load a module?

I was going to suggest that, or a new kernel option:
    noloadmodules=module1[,module2...]

The option may well be cumulative, too, so we could do:
    noloadmodules=module1,module2 noloadmodules=module3

and none of module{1,2,3} would be loaded. This could allow bootloaders
to build up the command line more easily.

(Ab)using the module parameter to avoid loading is hackish at best, IMHO.

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

Re: [RFC PATCH] Allow optional module parameters

[Lucas De Marchi]

On Wed, Jul 3, 2013 at 6:23 PM, Michal Marek <mmarek@suse.cz> wrote:
> Dne 3.7.2013 23:17, Andy Lutomirski napsal(a):
>> On Wed, Jul 3, 2013 at 2:03 PM, Michal Marek <mmarek@suse.cz> wrote:
>>> Dne 1.7.2013 18:33, Jonathan Masters napsal(a):
>>>> One caveat. Sometimes we have manufactured parameters intentionally
>>>> to cause a module to fail. We should standardize that piece.
>>>
>>> You have:
>>>
>>>   blacklist foo
>>>
>>> to prevent udev from loading a module and
>>>
>>>   install foo /bin/true
>>>
>>> to prevent modprobe from loading the module at all. What is the
>>> motivation for inventing a third way, through adding invalid parameters?
>>>
>>
>> FWIW, I've occasionally booted with modulename.garbage=1 to prevent
>> modulename from loading at boot.  It may be worth adding a more
>> intentional way to do that.
>
> Hm, right, there seems to be no clean way to achieve this via a
> commandline argument. Maybe define a magic module option to tell the
> module loader not to load a module?

modprobe.blacklist=modname1,modname2,... is already there, though all
the silliness of blacklist applies unless "-b" is passed (that's the
equivalent behavior of udev)

Lucas De Marchi

Re: [RFC PATCH] Allow optional module parameters

[Andy Lutomirski]

On Wed, Jul 3, 2013 at 2:31 PM, Lucas De Marchi
<lucas.demarchi@profusion.mobi> wrote:
> On Wed, Jul 3, 2013 at 6:23 PM, Michal Marek <mmarek@suse.cz> wrote:
>> Dne 3.7.2013 23:17, Andy Lutomirski napsal(a):
>>> On Wed, Jul 3, 2013 at 2:03 PM, Michal Marek <mmarek@suse.cz> wrote:
>>>> Dne 1.7.2013 18:33, Jonathan Masters napsal(a):
>>>>> One caveat. Sometimes we have manufactured parameters intentionally
>>>>> to cause a module to fail. We should standardize that piece.
>>>>
>>>> You have:
>>>>
>>>>   blacklist foo
>>>>
>>>> to prevent udev from loading a module and
>>>>
>>>>   install foo /bin/true
>>>>
>>>> to prevent modprobe from loading the module at all. What is the
>>>> motivation for inventing a third way, through adding invalid parameters?
>>>>
>>>
>>> FWIW, I've occasionally booted with modulename.garbage=1 to prevent
>>> modulename from loading at boot.  It may be worth adding a more
>>> intentional way to do that.
>>
>> Hm, right, there seems to be no clean way to achieve this via a
>> commandline argument. Maybe define a magic module option to tell the
>> module loader not to load a module?
>
> modprobe.blacklist=modname1,modname2,... is already there, though all
> the silliness of blacklist applies unless "-b" is passed (that's the
> equivalent behavior of udev)

That would probably be good enough for me.

It would be neat if this worked for built-in "modules" as well, but
that would probably be quite intrusive.

--Andy