Messages in this thread | | | From | Rusty Russell <> | Subject | Re: [PATCH] init: fix in-place parameter modification regression | Date | Mon, 21 Oct 2013 12:27:10 +1030 |
| |
Krzysztof Mazur <krzysiek@podlesie.net> 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@podlesie.net> > --- > 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
| |