Re: [PATCH 1/1] x86: sanitize argument of clearcpuid command-line option

[Ingo Molnar <mingo@kernel.org>]

* Odzioba, Lukasz <lukasz.odzioba@intel.com> wrote:

> >	pr_warn("x86/cpu: Ignoring invalid "clearcpuid=%s' option!\n", arg)
> >
> > Which would save quite a bit of head scratching and frustration when someone has a 
> > bad enough day to add silly typos to the kernel cmdline.
> 
> Is there any particular reason why we have such warnings only for early params?
> early_param handlers return non-zero values on success:
> 	linux/init.h: " * Emits warning if fn returns non-zero."
> __setup handlers in most cases seem to return 1 on success, is the expected
> behaviour documented somewhere?
> 
> After looking at some of the ~500 usages of __setup macro it seems that handler's ret
> code doesn't matter so much, because it is treated differently in various parts
> of the kernel. If we make it consistent possibly it could be solved similarly to 
> early params by something like this: 
> 
> diff --git a/init/main.c b/init/main.c
> index b0c9d6f..261178e 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -182,8 +182,12 @@ static bool __init obsolete_checksetup(char *line)
>                                 pr_warn("Parameter %s is obsolete, ignored\n",
>                                         p->str);
>                                 return true;
> -                       } else if (p->setup_func(line + n))
> -                               return true;
> +                       } else {
> +                               if (p->setup_func(line + n))
> +                                       return true;
> +                               else
> +                                       pr_warn("Malformed option '%s'\n", line);
> +                       }

That looks sensible to me! I'd tweak the message slightly:

	pr_warn("error: Ignoring invalid boot parameter '%s'\n", line);

to make it more clear that it's a boot option that has a problem (there are many 
other types of options), and to make sure the user knows that we ignored that 
option.

Mind sending this as a proper patch, with akpm Cc:-ed?

Thanks,

	Ingo