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

[Rusty Russell]

Krzysztof Mazur <krzysiek@xxxxxxxxxxxx> writes:
> On Fri, Oct 18, 2013 at 02:20:38PM +1030, Rusty Russell wrote:
>> Back when there was almost no parameter parsing support, everyone got
>> used to keeping pointers into the original.  Making everyone kstrdup()
>> seems like gratuitous churn which is likely to make more bugs.
>> 
>> Your fix means __setup() gets treated specially, in that only it can
>> mangle the command line.  That makes sense.  But it introduces another
>> regression: normal parsing functions can't keep pointers, since that's
>> now __initdata.
>> 
>> There are two possible solutions:
>> (1) Audit all __setup to make sure they copy if they want to mangle.
>>     There are about 750 of them, but many are trivial.
>> (2) alloc_bootmem() a third commandline for parsing.
>> 
>> Now, many functions of form __setup("XXX=") should be turned into
>> module_param anyway.
>> 
>> I suggest we do (2) for the moment, and start sweeping through cleaning
>> up __setup() in the longer term.
>> 
>
> Yes, the buffer cannot be __initdata. I'm sending an updated patch.
>
>
> However, keeping pointers to buffer, that will be reinitialized
> in next initcall parameters parsing pass, might cause some race
> conditions.

Thanks, applied.

Cheers,
Rusty.

> Thanks,
> Krzysiek
>
> -- >8 --
> Subject: [PATCH v2] init: fix in-place parameter modification regression
>
> 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.
>
> Signed-off-by: Krzysztof Mazur <krzysiek@xxxxxxxxxxxx>
> ---
>  init/main.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/init/main.c b/init/main.c
> index 63d3e8f..c093b5c 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -132,6 +132,8 @@ char __initdata boot_command_line[COMMAND_LINE_SIZE];
>  char *saved_command_line;
>  /* Command line for parameter parsing */
>  static char *static_command_line;
> +/* Command line for per-initcall parameter parsing */
> +static char *initcall_command_line;
>  
>  static char *execute_command;
>  static char *ramdisk_execute_command;
> @@ -348,6 +350,7 @@ static inline void smp_prepare_cpus(unsigned int maxcpus) { }
>  static void __init setup_command_line(char *command_line)
>  {
>  	saved_command_line = alloc_bootmem(strlen (boot_command_line)+1);
> +	initcall_command_line = alloc_bootmem(strlen (boot_command_line)+1);
>  	static_command_line = alloc_bootmem(strlen (command_line)+1);
>  	strcpy (saved_command_line, boot_command_line);
>  	strcpy (static_command_line, command_line);
> @@ -745,9 +748,9 @@ static void __init do_initcall_level(int level)
>  	extern const struct kernel_param __start___param[], __stop___param[];
>  	initcall_t *fn;
>  
> -	strcpy(static_command_line, saved_command_line);
> +	strcpy(initcall_command_line, saved_command_line);
>  	parse_args(initcall_level_names[level],
> -		   static_command_line, __start___param,
> +		   initcall_command_line, __start___param,
>  		   __stop___param - __start___param,
>  		   level, level,
>  		   &repair_env_string);
> -- 
> 1.8.4.1.635.g55556a5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/