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.
With this patch, module parameters can be made explicitly optional.
This approach is IMO silly, but it's unlikely to break anything,
since I doubt that anyone needs init parameters or init environment
variables that end in a tilde.
Signed-off-by: Andy Lutomirski <[email protected]>
---
This is, IMO, rather ugly. Better ideas are welcome, but I think that this
is a significant improvement over the status quo.
Documentation/kernel-parameters.txt | 11 +++++++++++
kernel/params.c | 29 ++++++++++++++++++++++-------
2 files changed, 33 insertions(+), 7 deletions(-)
diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 4609e81..605240c 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -22,6 +22,17 @@ Hyphens (dashes) and underscores are equivalent in parameter names, so
can also be entered as
log-buf-len=1M print_fatal_signals=1
+Parameter settings may be suffixed by '~' to make them optional; non-optional
+parameters that don't match anything can cause modprobe to fail. For example,
+modprobe usbcore typoedlights=1 will fail, but modprobe usbcore typoedlights~=1
+or modprobe usbcore typoedlights~ will merely print a warning.
+
+Note that module parameters on the kernel command line are parsed internally
+by the kernel if the module is built in and by kmod if not. In either case,
+
+ usbcore.typoedlights~=1
+
+will result in an optional parameter setting.
This document may not be entirely up to date and comprehensive. The command
"modinfo -p ${modulename}" shows a current list of all parameters of a loadable
diff --git a/kernel/params.c b/kernel/params.c
index ed35345..83664b3 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -126,9 +126,9 @@ static int parse_one(char *param,
/* You can use " around spaces, but can't escape ". */
/* Hyphens and underscores equivalent in parameter names. */
-static char *next_arg(char *args, char **param, char **val)
+static char *next_arg(char *args, char **param, char **val, bool *optional)
{
- unsigned int i, equals = 0;
+ unsigned int i, equals = 0, param_len;
int in_quote = 0, quoted = 0;
char *next;
@@ -150,11 +150,13 @@ static char *next_arg(char *args, char **param, char **val)
}
*param = args;
- if (!equals)
+ if (!equals) {
*val = NULL;
- else {
+ param_len = i;
+ } else {
args[equals] = '\0';
*val = args + equals + 1;
+ param_len = equals;
/* Don't include quotes in value. */
if (**val == '"') {
@@ -166,6 +168,13 @@ static char *next_arg(char *args, char **param, char **val)
args[i-1] = '\0';
}
+ if (param_len && (*param)[param_len-1] == '~') {
+ *optional = true;
+ (*param)[param_len-1] = '\0';
+ } else {
+ *optional = false;
+ }
+
if (args[i]) {
args[i] = '\0';
next = args + i + 1;
@@ -196,8 +205,9 @@ int parse_args(const char *doing,
while (*args) {
int ret;
int irq_was_disabled;
+ bool optional;
- args = next_arg(args, ¶m, &val);
+ args = next_arg(args, ¶m, &val, &optional);
irq_was_disabled = irqs_disabled();
ret = parse_one(param, val, doing, params, num,
min_level, max_level, unknown);
@@ -207,8 +217,13 @@ int parse_args(const char *doing,
switch (ret) {
case -ENOENT:
- pr_err("%s: Unknown parameter `%s'\n", doing, param);
- return ret;
+ if (optional) {
+ pr_warn("%s: Ignoring unknown optional parameter `%s'\n", doing, param);
+ break;
+ } else {
+ pr_err("%s: Unknown parameter `%s'\n", doing, param);
+ return ret;
+ }
case -ENOSPC:
pr_err("%s: `%s' too large for parameter `%s'\n",
doing, val ?: "", param);
--
1.8.1.4
Andy Lutomirski <[email protected]> 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?
> With this patch, module parameters can be made explicitly optional.
> This approach is IMO silly, but it's unlikely to break anything,
> since I doubt that anyone needs init parameters or init environment
> variables that end in a tilde.
It's silly for the removal problem: that should be handled in the
kernel. How would the poor user know that the option is going away?
So how about we add a module_param_obsolete(name) macro?
If a parameter were introduced, and the user wanted to specify it *if*
it was supported, that might justify this approach rather than using
complex install commands. But I don't believe that's common, is it?
Thanks,
Rusty.
On Thu, Mar 14, 2013 at 10:03 PM, Rusty Russell <[email protected]> wrote:
> Andy Lutomirski <[email protected]> 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.
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).
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.
>
>> With this patch, module parameters can be made explicitly optional.
>> This approach is IMO silly, but it's unlikely to break anything,
>> since I doubt that anyone needs init parameters or init environment
>> variables that end in a tilde.
>
> It's silly for the removal problem: that should be handled in the
> kernel. How would the poor user know that the option is going away?
> So how about we add a module_param_obsolete(name) macro?
>
> If a parameter were introduced, and the user wanted to specify it *if*
> it was supported, that might justify this approach rather than using
> complex install commands. But I don't believe that's common, is it?
>
It's happened multiple times to me with things like
pcie_aspm.force_aspm and i915.whatever.
--Andy
Andy Lutomirski <[email protected]> writes:
> On Thu, Mar 14, 2013 at 10:03 PM, Rusty Russell <[email protected]> wrote:
>> Andy Lutomirski <[email protected]> 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
On Sun, Mar 17, 2013 at 7:24 PM, Rusty Russell <[email protected]> wrote:
> Andy Lutomirski <[email protected]> writes:
>> On Thu, Mar 14, 2013 at 10:03 PM, Rusty Russell <[email protected]> wrote:
>>> Andy Lutomirski <[email protected]> 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
Andy Lutomirski <[email protected]> writes:
> On Sun, Mar 17, 2013 at 7:24 PM, Rusty Russell <[email protected]> wrote:
>> Andy Lutomirski <[email protected]> writes:
>>> On Thu, Mar 14, 2013 at 10:03 PM, Rusty Russell <[email protected]> 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 <[email protected]>
Signed-off-by: Rusty Russell <[email protected]>
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;
On Tue, 2013-03-19 at 13:02 +1030, Rusty Russell wrote:
> Andy Lutomirski <[email protected]> writes:
> > On Sun, Mar 17, 2013 at 7:24 PM, Rusty Russell <[email protected]> wrote:
> >> Andy Lutomirski <[email protected]> writes:
> >>> On Thu, Mar 14, 2013 at 10:03 PM, Rusty Russell <[email protected]> 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 <[email protected]>
> Signed-off-by: Rusty Russell <[email protected]>
[...]
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.
Hi Rusty,
On Mon, Mar 18, 2013 at 11:32 PM, Rusty Russell <[email protected]> wrote:
> Andy Lutomirski <[email protected]> writes:
>> On Sun, Mar 17, 2013 at 7:24 PM, Rusty Russell <[email protected]> wrote:
>>> Andy Lutomirski <[email protected]> writes:
>>>> On Thu, Mar 14, 2013 at 10:03 PM, Rusty Russell <[email protected]> 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
On Tue, Mar 19, 2013 at 8:26 PM, Lucas De Marchi
<[email protected]> wrote:
> Hi Rusty,
>
> On Mon, Mar 18, 2013 at 11:32 PM, Rusty Russell <[email protected]> wrote:
>> Andy Lutomirski <[email protected]> writes:
>>> On Sun, Mar 17, 2013 at 7:24 PM, Rusty Russell <[email protected]> wrote:
>>>> Andy Lutomirski <[email protected]> writes:
>>>>> On Thu, Mar 14, 2013 at 10:03 PM, Rusty Russell <[email protected]> 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
On Tue, Mar 19, 2013 at 8:32 PM, Andy Lutomirski <[email protected]> wrote:
> On Tue, Mar 19, 2013 at 8:26 PM, Lucas De Marchi
> <[email protected]> wrote:
>> Hi Rusty,
>>
>> On Mon, Mar 18, 2013 at 11:32 PM, Rusty Russell <[email protected]> wrote:
>>> Andy Lutomirski <[email protected]> writes:
>>>> On Sun, Mar 17, 2013 at 7:24 PM, Rusty Russell <[email protected]> wrote:
>>>>> Andy Lutomirski <[email protected]> writes:
>>>>>> On Thu, Mar 14, 2013 at 10:03 PM, Rusty Russell <[email protected]> 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
Ben Hutchings <[email protected]> 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.
Lucas De Marchi <[email protected]> writes:
> Hi Rusty,
>
> On Mon, Mar 18, 2013 at 11:32 PM, Rusty Russell <[email protected]> wrote:
>> Andy Lutomirski <[email protected]> writes:
>>> On Sun, Mar 17, 2013 at 7:24 PM, Rusty Russell <[email protected]> wrote:
>>>> Andy Lutomirski <[email protected]> writes:
>>>>> On Thu, Mar 14, 2013 at 10:03 PM, Rusty Russell <[email protected]> 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.
Rusty Russell <[email protected]> writes:
> Lucas De Marchi <[email protected]> writes:
>> Hi Rusty,
>>
>> On Mon, Mar 18, 2013 at 11:32 PM, Rusty Russell <[email protected]> wrote:
>>> Andy Lutomirski <[email protected]> writes:
>>>> On Sun, Mar 17, 2013 at 7:24 PM, Rusty Russell <[email protected]> wrote:
>>>>> Andy Lutomirski <[email protected]> writes:
>>>>>> On Thu, Mar 14, 2013 at 10:03 PM, Rusty Russell <[email protected]> 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 <[email protected]>
Signed-off-by: Rusty Russell <[email protected]>
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;
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 <[email protected]> wrote:
> Rusty Russell <[email protected]> writes:
>> Lucas De Marchi <[email protected]> writes:
>>> Hi Rusty,
>>>
>>> On Mon, Mar 18, 2013 at 11:32 PM, Rusty Russell <[email protected]> wrote:
>>>> Andy Lutomirski <[email protected]> writes:
>>>>> On Sun, Mar 17, 2013 at 7:24 PM, Rusty Russell <[email protected]> wrote:
>>>>>> Andy Lutomirski <[email protected]> writes:
>>>>>>> On Thu, Mar 14, 2013 at 10:03 PM, Rusty Russell <[email protected]> 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 <[email protected]>
> Signed-off-by: Rusty Russell <[email protected]>
>
> 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;
>
>
Jonathan Masters <[email protected]> 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.
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
On Wed, Jul 3, 2013 at 2:03 PM, Michal Marek <[email protected]> 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
Dne 3.7.2013 23:17, Andy Lutomirski napsal(a):
> On Wed, Jul 3, 2013 at 2:03 PM, Michal Marek <[email protected]> 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
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 <[email protected]> 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. |
'------------------------------^-------^------------------^--------------------'
On Wed, Jul 3, 2013 at 6:23 PM, Michal Marek <[email protected]> wrote:
> Dne 3.7.2013 23:17, Andy Lutomirski napsal(a):
>> On Wed, Jul 3, 2013 at 2:03 PM, Michal Marek <[email protected]> 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
On Wed, Jul 3, 2013 at 2:31 PM, Lucas De Marchi
<[email protected]> wrote:
> On Wed, Jul 3, 2013 at 6:23 PM, Michal Marek <[email protected]> wrote:
>> Dne 3.7.2013 23:17, Andy Lutomirski napsal(a):
>>> On Wed, Jul 3, 2013 at 2:03 PM, Michal Marek <[email protected]> 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