Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752314Ab3JRJT1 (ORCPT ); Fri, 18 Oct 2013 05:19:27 -0400 Received: from shrek-wifi.podlesie.net ([93.179.225.50]:36871 "EHLO shrek.podlesie.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751643Ab3JRJT0 (ORCPT ); Fri, 18 Oct 2013 05:19:26 -0400 Date: Fri, 18 Oct 2013 11:19:21 +0200 From: Krzysztof Mazur To: Rusty Russell Cc: Pawel Moll , "linux-kernel@vger.kernel.org" , Andrew Morton Subject: Re: [PATCH] init: fix in-place parameter modification regression Message-ID: <20131018091921.GA7301@shrek.podlesie.net> References: <1381601100-5622-1-git-send-email-krzysiek@podlesie.net> <1381750442.3247.48.camel@hornet> <20131014125001.GA6015@shrek.podlesie.net> <87wqlbjkz5.fsf@rustcorp.com.au> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <87wqlbjkz5.fsf@rustcorp.com.au> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3643 Lines: 94 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, Krzysiek -- >8 -- Subject: [PATCH v2] init: fix in-place parameter modification regression Before commit 026cee0086fe1df4cf74691cf273062cc769617d ("params: _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 --- 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/