Re: [PATCH] init: fix in-place parameter modification regression

[Pawel Moll <pawel.moll@arm.com>]

On Sat, 2013-10-12 at 19:05 +0100, Krzysztof Mazur wrote:
> Before commit 026cee0086fe1df4cf74691cf273062cc769617d
> ("params: <level>_initcall-like kernel parameters") the __setup
> parameter parsing code could modify parameter in the
> static_command_line buffer and such modifications were kept. After
> that commit such modifications are destroyed during per-initcall level
> parameter parsing because the same static_command_line buffer is used
> and only parameters for appropriate initcall level are parsed.
> 
> That change broke at least parsing "ubd" parameter in the ubd driver
> when the COW file is used.
> 
> Now the separate buffer is used for per-initcall parameter parsing,
> like in parsing early params.
> 
> Signed-off-by: Krzysztof Mazur <krzysiek@podlesie.net>
> ---
>
>  init/main.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/init/main.c b/init/main.c
> index 63d3e8f..e5b322a 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -742,12 +742,13 @@ static char *initcall_level_names[] __initdata = {
>  
>  static void __init do_initcall_level(int level)
>  {
> +	static __initdata char tmp_cmdline[COMMAND_LINE_SIZE];
>  	extern const struct kernel_param __start___param[], __stop___param[];
>  	initcall_t *fn;
>  
> -	strcpy(static_command_line, saved_command_line);
> +	strcpy(tmp_cmdline, saved_command_line);
>  	parse_args(initcall_level_names[level],
> -		   static_command_line, __start___param,
> +		   tmp_cmdline, __start___param,
>  		   __stop___param - __start___param,
>  		   level, level,
>  		   &repair_env_string);

Hold on. static_command_line can be only accessed within init/main.c. As
far as I can say, it is only used by unknown_bootoption() (so your
__setup callbacks) and then in the do_initcall_level().

So, assuming that it is actually legal to modify static_command_line in
__setup()-s (and I must say I have rather mixed feelings about it ;-),
the only place that will be able to make use of this changes will be
do_initcall_level().

But your change actually uses *saved*_command_line instead of
*static*_command_line as the base for parse_args.

Generally I agree that the commit in question changed the semantics in a
subtle way - it makes the do_initcalls() use saved_command_line as a
string to be parsed instead of static_command_line. I was convinced that
at this stage of execution they must be identical (and the
static_command_line is a de-facto scratch buffer). You're saying that is
may not be the case, which can be true, but you're keeping the same
behaviour :-)

So either you have some extra changes in your kernel actually using
static_command_line for some other reason, or your change makes no
difference. The third option is me being brain dead today, which is not
impossible ;-)

Paweł