[RFC] Allow optional module parameters

Commit Message

Rusty Russell July 1, 2013, 6:50 a.m. UTC

Rusty Russell <[e-mail]> writes:
> Lucas De Marchi <[e-mail]> writes:
>> Hi Rusty,
>>
>> On Mon, Mar 18, 2013 at 11:32 PM, Rusty Russell <[e-mail]> wrote:
>>> Andy Lutomirski <[e-mail]> writes:
>>>> On Sun, Mar 17, 2013 at 7:24 PM, Rusty Russell <[e-mail]> wrote:
>>>>> Andy Lutomirski <[e-mail]> writes:
>>>>>> On Thu, Mar 14, 2013 at 10:03 PM, Rusty Russell <[e-mail]> 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 <[e-mail]>
Signed-off-by: Rusty Russell <[e-mail]>


--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Jon Masters July 1, 2013, 4:33 p.m. UTC | #1

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

Rusty Russell July 3, 2013, 12:28 a.m. UTC | #2

Jonathan Masters <[e-mail]> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Michal Marek July 3, 2013, 9:03 p.m. UTC | #3

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Andy Lutomirski July 3, 2013, 9:17 p.m. UTC | #4

On Wed, Jul 3, 2013 at 2:03 PM, Michal Marek <[e-mail]> 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

Michal Marek July 3, 2013, 9:23 p.m. UTC | #5

Dne 3.7.2013 23:17, Andy Lutomirski napsal(a):
> On Wed, Jul 3, 2013 at 2:03 PM, Michal Marek <[e-mail]> 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

--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Yann E. MORIN July 3, 2013, 9:30 p.m. UTC | #6

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 <[e-mail]> 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.

Lucas De Marchi July 3, 2013, 9:31 p.m. UTC | #7

On Wed, Jul 3, 2013 at 6:23 PM, Michal Marek <[e-mail]> wrote:
> Dne 3.7.2013 23:17, Andy Lutomirski napsal(a):
>> On Wed, Jul 3, 2013 at 2:03 PM, Michal Marek <[e-mail]> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Andy Lutomirski July 3, 2013, 9:36 p.m. UTC | #8

On Wed, Jul 3, 2013 at 2:31 PM, Lucas De Marchi
<[e-mail]> wrote:
> On Wed, Jul 3, 2013 at 6:23 PM, Michal Marek <[e-mail]> wrote:
>> Dne 3.7.2013 23:17, Andy Lutomirski napsal(a):
>>> On Wed, Jul 3, 2013 at 2:03 PM, Michal Marek <[e-mail]> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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;