It is easy to foobar setting a kernel parameter on the command line
without realizing it, there's not much output that you can use to
assess what the kernel did with that parameter by default.
Make it a little more explicit which parameters on the command line
_looked_ like a valid parameter for the kernel, but did not match
anything and ultimately got tossed to init. This is very similar to the
unknown parameter message received when loading a module.
This assumes the parameters are processed in a normal fashion, some
parameters (dyndbg= for example) don't register their
parameter with the rest of the kernel's parameters, and therefore
always show up in this list (and are also given to init - like the
rest of this list).
Another example is BOOT_IMAGE= is highlighted as an offender, which it
technically is, but is passed by LILO and GRUB so most systems will see
that complaint.
Suggested-by: Steven Rostedt <[email protected]>
Suggested-by: Borislav Petkov <[email protected]>
Signed-off-by: Andrew Halaney <[email protected]>
---
Hello,
This idea was suggested by rostedt and bpetkov, not sure what they'll
think of the implementation :P Please let me know if you've got
suggestions (or hate the idea in general).
Piggybacking on unknown_bootoption()'s assessment of the
parameters made this pretty straight forward, but I'm a bit unhappy with
dyndbg and BOOT_IMAGE showing up. I didn't want to make an exception for
them, so I left it as is and decided that their oddities ought to be
shown in the output still. The format of the output is borrowed from the
dynamic debug statements for printing init's env/argv lines.
Due to the BOOT_IMAGE bit I didn't actually ever get to exercise the
early return (limited on my test systems right now) but I think I got
that statement correct.
Thanks,
Andrew
init/main.c | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/init/main.c b/init/main.c
index dd11bfd10ead..cd313f1bc7b0 100644
--- a/init/main.c
+++ b/init/main.c
@@ -872,6 +872,20 @@ void __init __weak arch_call_rest_init(void)
rest_init();
}
+void print_unknown_bootoptions(void)
+{
+ const char *const *p;
+
+ if (panic_later || (!argv_init[1] && !envp_init[2]))
+ return;
+
+ pr_notice("Unknown command line parameters:\n");
+ for (p = &argv_init[1]; *p; p++)
+ pr_notice(" %s\n", *p);
+ for (p = &envp_init[2]; *p; p++)
+ pr_notice(" %s\n", *p);
+}
+
asmlinkage __visible void __init __no_sanitize_address start_kernel(void)
{
char *command_line;
@@ -913,6 +927,7 @@ asmlinkage __visible void __init __no_sanitize_address start_kernel(void)
static_command_line, __start___param,
__stop___param - __start___param,
-1, -1, NULL, &unknown_bootoption);
+ print_unknown_bootoptions();
if (!IS_ERR_OR_NULL(after_dashes))
parse_args("Setting init args", after_dashes, NULL, 0, -1, -1,
NULL, set_init_arg);
--
2.30.2
Hi--
On 5/3/21 2:34 PM, Andrew Halaney wrote:
> It is easy to foobar setting a kernel parameter on the command line
> without realizing it, there's not much output that you can use to
> assess what the kernel did with that parameter by default.
>
> Make it a little more explicit which parameters on the command line
> _looked_ like a valid parameter for the kernel, but did not match
> anything and ultimately got tossed to init. This is very similar to the
> unknown parameter message received when loading a module.
>
> This assumes the parameters are processed in a normal fashion, some
> parameters (dyndbg= for example) don't register their
> parameter with the rest of the kernel's parameters, and therefore
> always show up in this list (and are also given to init - like the
> rest of this list).
As you say, unknown parameters are just given to init -- they are not
errors.
However, if someone is experiencing a problem, can't they just boot
with the addition of "debug" to the kernel command line and then they
can see what arguments and environment are being passed to the init
process? E.g.:
[ 9.453693] Run /sbin/init as init process
[ 9.456886] with arguments:
[ 9.460014] /sbin/init
[ 9.463121] showopts
[ 9.466157] fb=font:VGA8x8
[ 9.469172] with environment:
[ 9.472146] HOME=/
[ 9.475152] TERM=linux
[ 9.478103] BOOT_IMAGE=/boot/bzImage-next0409
[ 9.481057] resume=/dev/sda7
[ 9.484006] splash=native
[ 9.495272] v1de0=vesafb
Can you show an example of what this code prints?
thanks.
> Another example is BOOT_IMAGE= is highlighted as an offender, which it
> technically is, but is passed by LILO and GRUB so most systems will see
> that complaint.
>
> Suggested-by: Steven Rostedt <[email protected]>
> Suggested-by: Borislav Petkov <[email protected]>
> Signed-off-by: Andrew Halaney <[email protected]>
> ---
>
> Hello,
>
> This idea was suggested by rostedt and bpetkov, not sure what they'll
> think of the implementation :P Please let me know if you've got
> suggestions (or hate the idea in general).
>
> Piggybacking on unknown_bootoption()'s assessment of the
> parameters made this pretty straight forward, but I'm a bit unhappy with
> dyndbg and BOOT_IMAGE showing up. I didn't want to make an exception for
> them, so I left it as is and decided that their oddities ought to be
> shown in the output still. The format of the output is borrowed from the
> dynamic debug statements for printing init's env/argv lines.
>
> Due to the BOOT_IMAGE bit I didn't actually ever get to exercise the
> early return (limited on my test systems right now) but I think I got
> that statement correct.
>
> Thanks,
> Andrew
>
> init/main.c | 15 +++++++++++++++
> 1 file changed, 15 insertions(+)
>
> diff --git a/init/main.c b/init/main.c
> index dd11bfd10ead..cd313f1bc7b0 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -872,6 +872,20 @@ void __init __weak arch_call_rest_init(void)
> rest_init();
> }
>
> +void print_unknown_bootoptions(void)
> +{
> + const char *const *p;
> +
> + if (panic_later || (!argv_init[1] && !envp_init[2]))
> + return;
> +
> + pr_notice("Unknown command line parameters:\n");
> + for (p = &argv_init[1]; *p; p++)
> + pr_notice(" %s\n", *p);
> + for (p = &envp_init[2]; *p; p++)
> + pr_notice(" %s\n", *p);
> +}
> +
> asmlinkage __visible void __init __no_sanitize_address start_kernel(void)
> {
> char *command_line;
> @@ -913,6 +927,7 @@ asmlinkage __visible void __init __no_sanitize_address start_kernel(void)
> static_command_line, __start___param,
> __stop___param - __start___param,
> -1, -1, NULL, &unknown_bootoption);
> + print_unknown_bootoptions();
> if (!IS_ERR_OR_NULL(after_dashes))
> parse_args("Setting init args", after_dashes, NULL, 0, -1, -1,
> NULL, set_init_arg);
>
--
~Randy
On Mon, 3 May 2021 14:59:31 -0700
Randy Dunlap <[email protected]> wrote:
> Hi--
>
> On 5/3/21 2:34 PM, Andrew Halaney wrote:
> > It is easy to foobar setting a kernel parameter on the command line
> > without realizing it, there's not much output that you can use to
> > assess what the kernel did with that parameter by default.
> >
> > Make it a little more explicit which parameters on the command line
> > _looked_ like a valid parameter for the kernel, but did not match
> > anything and ultimately got tossed to init. This is very similar to the
> > unknown parameter message received when loading a module.
> >
> > This assumes the parameters are processed in a normal fashion, some
> > parameters (dyndbg= for example) don't register their
> > parameter with the rest of the kernel's parameters, and therefore
> > always show up in this list (and are also given to init - like the
> > rest of this list).
>
> As you say, unknown parameters are just given to init -- they are not
> errors.
>
> However, if someone is experiencing a problem, can't they just boot
> with the addition of "debug" to the kernel command line and then they
> can see what arguments and environment are being passed to the init
> process? E.g.:
>
> [ 9.453693] Run /sbin/init as init process
> [ 9.456886] with arguments:
> [ 9.460014] /sbin/init
> [ 9.463121] showopts
> [ 9.466157] fb=font:VGA8x8
> [ 9.469172] with environment:
> [ 9.472146] HOME=/
> [ 9.475152] TERM=linux
> [ 9.478103] BOOT_IMAGE=/boot/bzImage-next0409
> [ 9.481057] resume=/dev/sda7
> [ 9.484006] splash=native
> [ 9.495272] v1de0=vesafb
>
>
> Can you show an example of what this code prints?
Note, the issue this is trying to solve is to catch "typos" when someone
adds a parameter. Perhaps we should add a parameter called "check" and/or a
config option to always check.
This came out of a discussion that a developer was wondering why adding
"trace_events=x,y,z" wasn't working. It ended up being that they used
"trace_events=" and not "trace_event=" (added 's'). Having some output that
denotes this might help, where people can make sure the options they are
adding is indeed the correct ones.
Comment below about the patch.
>
> thanks.
>
> > Another example is BOOT_IMAGE= is highlighted as an offender, which it
> > technically is, but is passed by LILO and GRUB so most systems will see
> > that complaint.
> >
> > Suggested-by: Steven Rostedt <[email protected]>
> > Suggested-by: Borislav Petkov <[email protected]>
> > Signed-off-by: Andrew Halaney <[email protected]>
> > ---
> >
> > Hello,
> >
> > This idea was suggested by rostedt and bpetkov, not sure what they'll
> > think of the implementation :P Please let me know if you've got
> > suggestions (or hate the idea in general).
> >
> > Piggybacking on unknown_bootoption()'s assessment of the
> > parameters made this pretty straight forward, but I'm a bit unhappy with
> > dyndbg and BOOT_IMAGE showing up. I didn't want to make an exception for
> > them, so I left it as is and decided that their oddities ought to be
> > shown in the output still. The format of the output is borrowed from the
> > dynamic debug statements for printing init's env/argv lines.
> >
> > Due to the BOOT_IMAGE bit I didn't actually ever get to exercise the
> > early return (limited on my test systems right now) but I think I got
> > that statement correct.
> >
> > Thanks,
> > Andrew
> >
> > init/main.c | 15 +++++++++++++++
> > 1 file changed, 15 insertions(+)
> >
> > diff --git a/init/main.c b/init/main.c
> > index dd11bfd10ead..cd313f1bc7b0 100644
> > --- a/init/main.c
> > +++ b/init/main.c
> > @@ -872,6 +872,20 @@ void __init __weak arch_call_rest_init(void)
> > rest_init();
> > }
> >
> > +void print_unknown_bootoptions(void)
> > +{
> > + const char *const *p;
> > +
> > + if (panic_later || (!argv_init[1] && !envp_init[2]))
> > + return;
> > +
> > + pr_notice("Unknown command line parameters:\n");
> > + for (p = &argv_init[1]; *p; p++)
> > + pr_notice(" %s\n", *p);
> > + for (p = &envp_init[2]; *p; p++)
> > + pr_notice(" %s\n", *p);
> > +}
Perhaps make this one line, like "Kernel command line:" has.
-- Steve
> > +
> > asmlinkage __visible void __init __no_sanitize_address start_kernel(void)
> > {
> > char *command_line;
> > @@ -913,6 +927,7 @@ asmlinkage __visible void __init __no_sanitize_address start_kernel(void)
> > static_command_line, __start___param,
> > __stop___param - __start___param,
> > -1, -1, NULL, &unknown_bootoption);
> > + print_unknown_bootoptions();
> > if (!IS_ERR_OR_NULL(after_dashes))
> > parse_args("Setting init args", after_dashes, NULL, 0, -1, -1,
> > NULL, set_init_arg);
> >
>
>
On Tue, May 04, 2021 at 05:00:51PM +0200, Borislav Petkov wrote:
> On Mon, May 03, 2021 at 06:46:06PM -0400, Steven Rostedt wrote:
> > Note, the issue this is trying to solve is to catch "typos" when someone
> > adds a parameter. Perhaps we should add a parameter called "check" and/or a
> > config option to always check.
>
> Well, actually, you want this checking to always happen and
> unconditionally at that.
>
> The fact that we cannot differentiate between params given to init vs
> mistyped params, makes it harder to solve elegantly.
>
I tend to agree, while the "debug" option Randy mentioned gives you the
same info, I lean towards notifying about any unrecognized parameter
personally in an unconditional manner that is clearly mentioning your
command line arguments - it is a bit round about to have to find them
from init. Definitely a matter of opinion, but with the kernel having
specific ways to denote init destined parameters (anything after "--") I
think an unconditional message is acceptable.
> > > > +void print_unknown_bootoptions(void)
>
> static
>
Good catch, will fix this in v2 - thank you!
> > > > +{
> > > > + const char *const *p;
> > > > +
> > > > + if (panic_later || (!argv_init[1] && !envp_init[2]))
> > > > + return;
> > > > +
> > > > + pr_notice("Unknown command line parameters:\n");
> > > > + for (p = &argv_init[1]; *p; p++)
> > > > + pr_notice(" %s\n", *p);
> > > > + for (p = &envp_init[2]; *p; p++)
> > > > + pr_notice(" %s\n", *p);
> > > > +}
> >
> > Perhaps make this one line, like "Kernel command line:" has.
>
> Yap, only one newline at the end pls.
>
I'm outvoted, will adjust to a single line!
> --
> Regards/Gruss,
> Boris.
>
> SUSE Software Solutions Germany GmbH, GF: Felix Imend?rffer, HRB 36809, AG N?rnberg
>
Thanks,
Andrew
On Mon, May 03, 2021 at 06:46:06PM -0400, Steven Rostedt wrote:
> Note, the issue this is trying to solve is to catch "typos" when someone
> adds a parameter. Perhaps we should add a parameter called "check" and/or a
> config option to always check.
Well, actually, you want this checking to always happen and
unconditionally at that.
The fact that we cannot differentiate between params given to init vs
mistyped params, makes it harder to solve elegantly.
> > > +void print_unknown_bootoptions(void)
static
> > > +{
> > > + const char *const *p;
> > > +
> > > + if (panic_later || (!argv_init[1] && !envp_init[2]))
> > > + return;
> > > +
> > > + pr_notice("Unknown command line parameters:\n");
> > > + for (p = &argv_init[1]; *p; p++)
> > > + pr_notice(" %s\n", *p);
> > > + for (p = &envp_init[2]; *p; p++)
> > > + pr_notice(" %s\n", *p);
> > > +}
>
> Perhaps make this one line, like "Kernel command line:" has.
Yap, only one newline at the end pls.
--
Regards/Gruss,
Boris.
SUSE Software Solutions Germany GmbH, GF: Felix Imendörffer, HRB 36809, AG Nürnberg
On Tue, May 04, 2021 at 10:26:14AM -0500, Andrew Halaney wrote:
> Definitely a matter of opinion, but with the kernel having specific
> ways to denote init destined parameters (anything after "--") I think
> an unconditional message is acceptable.
Or - and I had alluded to that on IRC - you *actually* know which params
are kernel params:
#define __setup_param(str, unique_id, fn, early) \
static const char __setup_str_##unique_id[] __initconst \
__aligned(1) = str; \
static struct obs_kernel_param __setup_##unique_id \
__used __section(".init.setup") \
^^^^^^^^^^^^^^^^^^
__aligned(__alignof__(struct obs_kernel_param)) \
= { __setup_str_##unique_id, fn, early }
all those guys in the above section.
So you'd have iterate over those and do some cheap version of those
autocorrect algorithms which guess which words you meant. For example,
if you have:
panik_on_oops instead of
panic_on_oops
the difference is one letter so it is likely a mistyped param rather
than something which goes to init or other random garbage and then you
warn.
Something like that.
It would need a lot of experimentation first, though, to see whether
this makes sense and it is workable at all.
Thx.
--
Regards/Gruss,
Boris.
SUSE Software Solutions Germany GmbH, GF: Felix Imendörffer, HRB 36809, AG Nürnberg
On Wed, May 05, 2021 at 04:20:47PM +0200, Borislav Petkov wrote:
> On Tue, May 04, 2021 at 10:26:14AM -0500, Andrew Halaney wrote:
> > Definitely a matter of opinion, but with the kernel having specific
> > ways to denote init destined parameters (anything after "--") I think
> > an unconditional message is acceptable.
>
> Or - and I had alluded to that on IRC - you *actually* know which params
> are kernel params:
>
> #define __setup_param(str, unique_id, fn, early) \
> static const char __setup_str_##unique_id[] __initconst \
> __aligned(1) = str; \
> static struct obs_kernel_param __setup_##unique_id \
> __used __section(".init.setup") \
> ^^^^^^^^^^^^^^^^^^
>
> __aligned(__alignof__(struct obs_kernel_param)) \
> = { __setup_str_##unique_id, fn, early }
>
>
> all those guys in the above section.
I actually did use that recommendation essentially, the patch I've sent
is riding on the work done by unknown_bootoption() which is populated by
iterating over over the different sections parameters can live in - so
this is only printing out arguments that didn't match a known kernel
parameter. Sorry if I didn't make that clear earlier, definitely was
trying to listen to your advice.
>
> So you'd have iterate over those and do some cheap version of those
> autocorrect algorithms which guess which words you meant. For example,
> if you have:
>
> panik_on_oops instead of
> panic_on_oops
>
> the difference is one letter so it is likely a mistyped param rather
> than something which goes to init or other random garbage and then you
> warn.
>
> Something like that.
>
> It would need a lot of experimentation first, though, to see whether
> this makes sense and it is workable at all.
I'll have to think about this some more (the "did you mean this
parameter" part).. that seems like it might be more trouble than it is
worth, but I admittedly haven't looked into those cheap algorithms you
mentioned yet. The reason I say it might be more trouble than it is
worth is because it is easy to say "why didn't my param work", then grep
for it in dmesg and find it in the "Unknown command line parameters"
list - that's sort of the workflow I imagined would happen when someone
mucks with their kernel cli and doesn't get the intended result.
Thanks,
Andrew
On Wed, May 05, 2021 at 11:37:28AM -0500, Andrew Halaney wrote:
> I actually did use that recommendation essentially, the patch I've sent
> is riding on the work done by unknown_bootoption() which is populated by
> iterating over over the different sections parameters can live in - so
> this is only printing out arguments that didn't match a known kernel
> parameter. Sorry if I didn't make that clear earlier, definitely was
> trying to listen to your advice.
Bah, don't take my "advice" too seriously - I'm just throwing out
guesses. :-)
So ok, unknown_bootoption() handles those and AFAICT, that gets passed
to parse_args() with the __start___param and __stop___param range.
But then there is that do_early_param() thing for early params, which
are different and which are between __setup_start and __setup_end -
i.e., the ones I meant above.
And that function doesn't do the unknown bootoption handling ;-\
More fun.
> I'll have to think about this some more (the "did you mean this
> parameter" part).. that seems like it might be more trouble than it is
> worth, but I admittedly haven't looked into those cheap algorithms you
> mentioned yet. The reason I say it might be more trouble than it is
> worth is because it is easy to say "why didn't my param work", then grep
> for it in dmesg and find it in the "Unknown command line parameters"
> list - that's sort of the workflow I imagined would happen when someone
> mucks with their kernel cli and doesn't get the intended result.
Oh sure - that's what I meant with "cheap". If it can't be done
elegantly and easily, just forget it. dmesg | grep is a lot easier. :-)
Thx.
--
Regards/Gruss,
Boris.
SUSE Software Solutions Germany GmbH, GF: Felix Imendörffer, HRB 36809, AG Nürnberg
On Wed, May 05, 2021 at 06:50:13PM +0200, Borislav Petkov wrote:
> On Wed, May 05, 2021 at 11:37:28AM -0500, Andrew Halaney wrote:
> > I actually did use that recommendation essentially, the patch I've sent
> > is riding on the work done by unknown_bootoption() which is populated by
> > iterating over over the different sections parameters can live in - so
> > this is only printing out arguments that didn't match a known kernel
> > parameter. Sorry if I didn't make that clear earlier, definitely was
> > trying to listen to your advice.
>
> Bah, don't take my "advice" too seriously - I'm just throwing out
> guesses. :-)
>
> So ok, unknown_bootoption() handles those and AFAICT, that gets passed
> to parse_args() with the __start___param and __stop___param range.
>
> But then there is that do_early_param() thing for early params, which
> are different and which are between __setup_start and __setup_end -
> i.e., the ones I meant above.
>
> And that function doesn't do the unknown bootoption handling ;-\
>
> More fun.
>
Ah, but don't worry! It is handled, just secretly:
unknown_bootoption()->obsolete_checksetup() walks __setup_start
:)
> > I'll have to think about this some more (the "did you mean this
> > parameter" part).. that seems like it might be more trouble than it is
> > worth, but I admittedly haven't looked into those cheap algorithms you
> > mentioned yet. The reason I say it might be more trouble than it is
> > worth is because it is easy to say "why didn't my param work", then grep
> > for it in dmesg and find it in the "Unknown command line parameters"
> > list - that's sort of the workflow I imagined would happen when someone
> > mucks with their kernel cli and doesn't get the intended result.
>
> Oh sure - that's what I meant with "cheap". If it can't be done
> elegantly and easily, just forget it. dmesg | grep is a lot easier. :-)
>
> Thx.
>
Still worth considering, so at least lemme ponder it for a day instead
of being lazy.
Thanks,
Andrew
On Wed, May 05, 2021 at 11:55:42AM -0500, Andrew Halaney wrote:
> Ah, but don't worry! It is handled, just secretly:
> unknown_bootoption()->obsolete_checksetup() walks __setup_start
> :)
Eww, and there's a had_early_param - run-this-function-only-one-time
clumsy thing. Here's another area that could use a cleanup.
> Still worth considering, so at least lemme ponder it for a day instead
> of being lazy.
Take your time - that's one of those fun projects where no one's
breathing down your neck.
And remember to have fun, as we say. :-)
--
Regards/Gruss,
Boris.
SUSE Software Solutions Germany GmbH, GF: Felix Imendörffer, HRB 36809, AG Nürnberg