From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751143AbdBKUGP (ORCPT ); Sat, 11 Feb 2017 15:06:15 -0500 Received: from Galois.linutronix.de ([146.0.238.70]:35599 "EHLO Galois.linutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751047AbdBKUGO (ORCPT ); Sat, 11 Feb 2017 15:06:14 -0500 Date: Sat, 11 Feb 2017 21:06:03 +0100 (CET) From: Thomas Gleixner To: Kishore Karanala cc: tglx@linutronix.de, mingo@redhat.com, "H. Peter Anvin" , x86@kernel.org, LKML , matt@codeblueprint.co.uk, rafael.j.wysocki@intel.com, thgarnie@google.com, arbab@linux.vnet.ibm.com, dave.hansen@linux.intel.com Subject: Re: [PATCH] Staging: setup.c : boot loader kernel arguments are secured over cmdline.txt arguments for some systems In-Reply-To: <1486841400-5063-1-git-send-email-kishore.karanala@gmail.com> Message-ID: References: <1486841400-5063-1-git-send-email-kishore.karanala@gmail.com> User-Agent: Alpine 2.20 (DEB 67 2015-01-07) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sun, 12 Feb 2017, Kishore Karanala wrote: 1;2802;0c The proper subsystem for x86 is x86 and not staging. Also your subject line is a way too long sentence instead of a short and precise summary of the change. > boot loader kernel arguments are secured over cmdline.txt > arguments for some systems Repeating it does not make it any better. What's missing here is an explanation WHY this changes is needed and which problem it solves. > Signed-off-by: Kishore Karanala > --- > arch/x86/Kconfig | 3 +++ > arch/x86/kernel/setup.c | 7 ++++++- > 2 files changed, 9 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig > index e487493..3077fb0 100644 > --- a/arch/x86/Kconfig > +++ b/arch/x86/Kconfig > @@ -2224,6 +2224,9 @@ config CMDLINE > In most cases, the command line (whether built-in or provided > by the boot loader) should specify the device for the root > file system. > + In some of the systems boot loader arguments needs dominated over > + cmdline arguments in systems like automotive , this can be done using > + CMDLINE="!root=/dev/mmcblk0p1 ro" I really have no idea what that sentence means. Also it does not explain what the exclamation mark stands for. Aside of that the indentation of that paragraph is wrong. > config CMDLINE_OVERRIDE > bool "Built-in command line overrides boot loader arguments" > diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c > index 4cfba94..5fa3194 100644 > --- a/arch/x86/kernel/setup.c > +++ b/arch/x86/kernel/setup.c > @@ -962,11 +962,16 @@ void __init setup_arch(char **cmdline_p) > #ifdef CONFIG_CMDLINE_OVERRIDE > strlcpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE); > #else > - if (builtin_cmdline[0]) { > + if (builtin_cmdline[0] != '!') { > /* append boot loader cmdline to builtin */ > strlcat(builtin_cmdline, " ", COMMAND_LINE_SIZE); > strlcat(builtin_cmdline, boot_command_line, COMMAND_LINE_SIZE); > strlcpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE); > + } else { > + /* This will provide additional secuirty to cmdline */ I assume you mean security. What has this to do with security? The empty changelog does not explain it neither does this comment. > + /* arguments not overriding bootloader arguments */ Multiline comments are formatted like this /* * This is the first line of a multiline comment which * continues on the second line. */ > + strlcat(boot_command_line, " ", COMMAND_LINE_SIZE); > + strlcat(boot_command_line, &builtin_cmdline[1], COMMAND_LINE_SIZE); So this is just the reverse order of the exsiting mechanism. What's the point of this exercise? Thanks, tglx