* [PATCH v7 1/4] powerpc/fadump: reduce memory consumption for capture kernel @ 2017-08-17 20:14 Michal Suchanek 2017-08-17 20:14 ` [PATCH v7 2/4] powerpc/fadump: update documentation about 'fadump_extra_args=' parameter Michal Suchanek ` (3 more replies) 0 siblings, 4 replies; 14+ messages in thread From: Michal Suchanek @ 2017-08-17 20:14 UTC (permalink / raw) To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman, Jonathan Corbet, Jessica Yu, Rusty Russell, Jason Baron, Hari Bathini, Mahesh J Salgaonkar, Daniel Axtens, Andrew Morton, Michal Suchanek, Thiago Jung Bauermann, Balbir Singh, Nicholas Piggin, Michael Neuling, Aneesh Kumar K.V, Christophe Leroy, Scott Wood, Oliver O'Halloran, David Howells, Sylvain 'ythier' Hitier, Ingo Molnar, Kees Cook, Thomas Gleixner Cc: Steven Rostedt,, Viresh Kumar, Tejun Heo, Lokesh Vutla, Baoquan He, Ilya Matveychikov, linuxppc-dev, linux-doc, linux-kernel From: Hari Bathini <hbathini@linux.vnet.ibm.com> With fadump (dump capture) kernel booting like a regular kernel, it needs almost the same amount of memory to boot as the production kernel, which is unwarranted for a dump capture kernel. But with no option to disable some of the unnecessary subsystems in fadump kernel, that much memory is wasted on fadump, depriving the production kernel of that memory. Introduce kernel parameter 'fadump_extra_args=' that would take regular parameters as a space separated quoted string, to be enforced when fadump is active. This 'fadump_extra_args=' parameter can be leveraged to pass parameters like nr_cpus=1, cgroup_disable=memory and numa=off, to disable unwarranted resources/subsystems. Also, ensure the log "Firmware-assisted dump is active" is printed early in the boot process to put the subsequent fadump messages in context. Suggested-by: Michael Ellerman <mpe@ellerman.id.au> Signed-off-by: Hari Bathini <hbathini@linux.vnet.ibm.com> Signed-off-by: Michal Suchanek <msuchanek@suse.de> --- Changes from v6: Correct and simplify quote handling. Ideally I would like to extend parse_args to give the length of the original quoted value to callback. However, parse_args removes at most one doubel-quote from the start and one from the end so that is easy to detect. Otherwise all other users will have to be updated to trash the new argument. --- arch/powerpc/include/asm/fadump.h | 2 + arch/powerpc/kernel/fadump.c | 109 ++++++++++++++++++++++++++++++++++++-- arch/powerpc/kernel/prom.c | 7 +++ 3 files changed, 115 insertions(+), 3 deletions(-) diff --git a/arch/powerpc/include/asm/fadump.h b/arch/powerpc/include/asm/fadump.h index ce88bbe1d809..98ae00943fb3 100644 --- a/arch/powerpc/include/asm/fadump.h +++ b/arch/powerpc/include/asm/fadump.h @@ -208,11 +208,13 @@ extern int early_init_dt_scan_fw_dump(unsigned long node, const char *uname, int depth, void *data); extern int fadump_reserve_mem(void); extern int setup_fadump(void); +extern void enforce_fadump_extra_args(char *cmdline); extern int is_fadump_active(void); extern void crash_fadump(struct pt_regs *, const char *); extern void fadump_cleanup(void); #else /* CONFIG_FA_DUMP */ +static inline void enforce_fadump_extra_args(char *cmdline) { } static inline int is_fadump_active(void) { return 0; } static inline void crash_fadump(struct pt_regs *regs, const char *str) { } #endif diff --git a/arch/powerpc/kernel/fadump.c b/arch/powerpc/kernel/fadump.c index dc0c49cfd90a..a1614d9b8a21 100644 --- a/arch/powerpc/kernel/fadump.c +++ b/arch/powerpc/kernel/fadump.c @@ -78,8 +78,10 @@ int __init early_init_dt_scan_fw_dump(unsigned long node, * dump data waiting for us. */ fdm_active = of_get_flat_dt_prop(node, "ibm,kernel-dump", NULL); - if (fdm_active) + if (fdm_active) { + pr_info("Firmware-assisted dump is active.\n"); fw_dump.dump_active = 1; + } /* Get the sizes required to store dump data for the firmware provided * dump sections. @@ -332,8 +334,11 @@ int __init fadump_reserve_mem(void) { unsigned long base, size, memory_boundary; - if (!fw_dump.fadump_enabled) + if (!fw_dump.fadump_enabled) { + if (fw_dump.dump_active) + pr_warn("Firmware-assisted dump was active but kernel booted with fadump disabled!\n"); return 0; + } if (!fw_dump.fadump_supported) { printk(KERN_INFO "Firmware-assisted dump is not supported on" @@ -373,7 +378,6 @@ int __init fadump_reserve_mem(void) memory_boundary = memblock_end_of_DRAM(); if (fw_dump.dump_active) { - printk(KERN_INFO "Firmware-assisted dump is active.\n"); /* * If last boot has crashed then reserve all the memory * above boot_memory_size so that we don't touch it until @@ -460,6 +464,105 @@ static int __init early_fadump_reserve_mem(char *p) } early_param("fadump_reserve_mem", early_fadump_reserve_mem); +#define FADUMP_EXTRA_ARGS_PARAM "fadump_extra_args=" +#define FADUMP_EXTRA_ARGS_LEN (strlen(FADUMP_EXTRA_ARGS_PARAM) - 1) + +struct param_info { + char *cmdline; + char *tmp_cmdline; + int shortening; +}; + +static void __init fadump_update_params(struct param_info *param_info, + char *param, char *val) +{ + ptrdiff_t param_offset = param - param_info->tmp_cmdline; + size_t vallen = val ? strlen(val) : 0; + char *tgt = param_info->cmdline + param_offset + + FADUMP_EXTRA_ARGS_LEN - param_info->shortening; + int shortening = 0; + + if (!val) + return; + + /* remove '=' */ + *tgt++ = ' '; + + /* next_arg removes one leading and one trailing '"' */ + if (*tgt == '"') + shortening += 1; + if (*(tgt + vallen + shortening) == '"') + shortening += 1; + + /* remove one leading and one trailing quote if both are present */ + if ((val[0] == '"') && (val[vallen - 1] == '"')) { + shortening += 2; + vallen -= 2; + val++; + } + + /* some characters were removed - move the trailing part of cmdline */ + if (shortening) { + char *src; + + strncpy(tgt, val, vallen); + tgt += vallen; + src = tgt + shortening; + memmove(tgt, src, strlen(src) + 1); + } + + param_info->shortening += shortening; +} + +/* + * Reworks command line parameters and splits 'fadump_extra_args=' param + * to enforce the parameters passed through it + */ +static int __init fadump_rework_cmdline_params(char *param, char *val, + const char *unused, void *arg) +{ + struct param_info *param_info = (struct param_info *)arg; + + if (strncmp(param, FADUMP_EXTRA_ARGS_PARAM, + strlen(FADUMP_EXTRA_ARGS_PARAM) - 1)) + return 0; + + fadump_update_params(param_info, param, val); + + return 0; +} + +/* + * Replace every occurrence of 'fadump_extra_args="param1 param2 param3"' + * in cmdline with 'fadump_extra_args param1 param2 param3' by stripping + * off '=' and quotes, if any. This ensures that the additional parameters + * passed with 'fadump_extra_args=' are enforced. + */ +void __init enforce_fadump_extra_args(char *cmdline) +{ + static char tmp_cmdline[COMMAND_LINE_SIZE] __initdata; + static char init_cmdline[COMMAND_LINE_SIZE] __initdata; + struct param_info param_info; + + if (strstr(cmdline, FADUMP_EXTRA_ARGS_PARAM) == NULL) + return; + + pr_info("Modifying command line to enforce the additional parameters passed through 'fadump_extra_args='"); + + param_info.cmdline = cmdline; + param_info.tmp_cmdline = tmp_cmdline; + param_info.shortening = 0; + + strlcpy(init_cmdline, cmdline, COMMAND_LINE_SIZE); + + strlcpy(tmp_cmdline, cmdline, COMMAND_LINE_SIZE); + parse_args("fadump params", tmp_cmdline, NULL, 0, 0, 0, + ¶m_info, &fadump_rework_cmdline_params); + + pr_info("Original command line: %s\n", init_cmdline); + pr_info("Modified command line: %s\n", cmdline); +} + static int register_fw_dump(struct fadump_mem_struct *fdm) { int rc, err; diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c index f83056297441..2e6f40217266 100644 --- a/arch/powerpc/kernel/prom.c +++ b/arch/powerpc/kernel/prom.c @@ -693,6 +693,13 @@ void __init early_init_devtree(void *params) of_scan_flat_dt(early_init_dt_scan_root, NULL); of_scan_flat_dt(early_init_dt_scan_memory_ppc, NULL); + /* + * Look for 'fadump_extra_args=' parameter and enfore the additional + * parameters passed to it if fadump is active. + */ + if (is_fadump_active()) + enforce_fadump_extra_args(boot_command_line); + parse_early_param(); /* make sure we've parsed cmdline for mem= before this */ -- 2.10.2 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v7 2/4] powerpc/fadump: update documentation about 'fadump_extra_args=' parameter 2017-08-17 20:14 [PATCH v7 1/4] powerpc/fadump: reduce memory consumption for capture kernel Michal Suchanek @ 2017-08-17 20:14 ` Michal Suchanek 2017-08-17 20:14 ` [PATCH v7 3/4] lib/cmdline.c Remove quotes symmetrically Michal Suchanek ` (2 subsequent siblings) 3 siblings, 0 replies; 14+ messages in thread From: Michal Suchanek @ 2017-08-17 20:14 UTC (permalink / raw) To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman, Jonathan Corbet, Jessica Yu, Rusty Russell, Jason Baron, Hari Bathini, Mahesh J Salgaonkar, Daniel Axtens, Andrew Morton, Michal Suchanek, Thiago Jung Bauermann, Balbir Singh, Nicholas Piggin, Michael Neuling, Aneesh Kumar K.V, Christophe Leroy, Scott Wood, Oliver O'Halloran, David Howells, Sylvain 'ythier' Hitier, Ingo Molnar, Kees Cook, Thomas Gleixner Cc: Steven Rostedt,, Viresh Kumar, Tejun Heo, Lokesh Vutla, Baoquan He, Ilya Matveychikov, linuxppc-dev, linux-doc, linux-kernel From: Hari Bathini <hbathini@linux.vnet.ibm.com> With the introduction of 'fadump_extra_args=' parameter to pass additional parameters to fadump (capture) kernel, update documentation about it. Signed-off-by: Hari Bathini <hbathini@linux.vnet.ibm.com> Signed-off-by: Michal Suchanek <msuchanek@suse.de> --- Documentation/powerpc/firmware-assisted-dump.txt | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/Documentation/powerpc/firmware-assisted-dump.txt b/Documentation/powerpc/firmware-assisted-dump.txt index bdd344aa18d9..2df88524d2c7 100644 --- a/Documentation/powerpc/firmware-assisted-dump.txt +++ b/Documentation/powerpc/firmware-assisted-dump.txt @@ -162,7 +162,19 @@ How to enable firmware-assisted dump (fadump): 1. Set config option CONFIG_FA_DUMP=y and build kernel. 2. Boot into linux kernel with 'fadump=on' kernel cmdline option. -3. Optionally, user can also set 'crashkernel=' kernel cmdline +3. A user can pass additional command line parameters as a space + separated quoted list through 'fadump_extra_args=' parameter, + to be enforced when fadump is active. For example, parameter + 'fadump_extra_args="nr_cpus=1 numa=off udev.children-max=2"' + will be changed to 'fadump_extra_args nr_cpus=1 numa=off + udev.children-max=2' in-place when fadump is active. This + parameter has no affect when fadump is not active. Multiple + instances of 'fadump_extra_args=' can be passed. This provision + can be used to reduce memory consumption during dump capture by + disabling unwarranted resources/subsystems like CPUs, NUMA + and such. Value with spaces can be passed as + 'fadump_extra_args=""parameter="value with spaces"""' +4. Optionally, user can also set 'crashkernel=' kernel cmdline to specify size of the memory to reserve for boot memory dump preservation. @@ -172,6 +184,12 @@ NOTE: 1. 'fadump_reserve_mem=' parameter has been deprecated. Instead 2. If firmware-assisted dump fails to reserve memory then it will fallback to existing kdump mechanism if 'crashkernel=' option is set at kernel cmdline. + 3. Special parameters like '--' passed inside fadump_extra_args are also + just left in-place. So, the user is advised to consider this while + specifying such parameters. It may be required to quote the argument + to fadump_extra_args when the bootloader uses double-quotes as + argument delimiter as well. eg + append = " fadump_extra_args=\"nr_cpus=1 numa=off udev.children-max=2\"" Sysfs/debugfs files: ------------ -- 2.10.2 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v7 3/4] lib/cmdline.c Remove quotes symmetrically. 2017-08-17 20:14 [PATCH v7 1/4] powerpc/fadump: reduce memory consumption for capture kernel Michal Suchanek 2017-08-17 20:14 ` [PATCH v7 2/4] powerpc/fadump: update documentation about 'fadump_extra_args=' parameter Michal Suchanek @ 2017-08-17 20:14 ` Michal Suchanek 2017-08-17 21:27 ` msuchanek ` (2 more replies) 2017-08-17 20:14 ` [PATCH v7 4/4] boot/param: add pointer to next argument to unknown parameter callback Michal Suchanek 2017-08-18 10:50 ` [PATCH v7 1/4] powerpc/fadump: reduce memory consumption for capture kernel Hari Bathini 3 siblings, 3 replies; 14+ messages in thread From: Michal Suchanek @ 2017-08-17 20:14 UTC (permalink / raw) To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman, Jonathan Corbet, Jessica Yu, Rusty Russell, Jason Baron, Hari Bathini, Mahesh J Salgaonkar, Daniel Axtens, Andrew Morton, Michal Suchanek, Thiago Jung Bauermann, Balbir Singh, Nicholas Piggin, Michael Neuling, Aneesh Kumar K.V, Christophe Leroy, Scott Wood, Oliver O'Halloran, David Howells, Sylvain 'ythier' Hitier, Ingo Molnar, Kees Cook, Thomas Gleixner Cc: Steven Rostedt,, Viresh Kumar, Tejun Heo, Lokesh Vutla, Baoquan He, Ilya Matveychikov, linuxppc-dev, linux-doc, linux-kernel Remove quotes from argument value only if there is qoute on both sides. Signed-off-by: Michal Suchanek <msuchanek@suse.de> --- arch/powerpc/kernel/fadump.c | 6 ++---- lib/cmdline.c | 7 ++----- 2 files changed, 4 insertions(+), 9 deletions(-) diff --git a/arch/powerpc/kernel/fadump.c b/arch/powerpc/kernel/fadump.c index a1614d9b8a21..d7da4ce9f7ae 100644 --- a/arch/powerpc/kernel/fadump.c +++ b/arch/powerpc/kernel/fadump.c @@ -489,10 +489,8 @@ static void __init fadump_update_params(struct param_info *param_info, *tgt++ = ' '; /* next_arg removes one leading and one trailing '"' */ - if (*tgt == '"') - shortening += 1; - if (*(tgt + vallen + shortening) == '"') - shortening += 1; + if ((*tgt == '"') && (*(tgt + vallen + shortening) == '"')) + shortening += 2; /* remove one leading and one trailing quote if both are present */ if ((val[0] == '"') && (val[vallen - 1] == '"')) { diff --git a/lib/cmdline.c b/lib/cmdline.c index 4c0888c4a68d..01e701b2afe8 100644 --- a/lib/cmdline.c +++ b/lib/cmdline.c @@ -227,14 +227,11 @@ char *next_arg(char *args, char **param, char **val) *val = args + equals + 1; /* Don't include quotes in value. */ - if (**val == '"') { + if ((**val == '"') && (args[i-1] == '"')) { (*val)++; - if (args[i-1] == '"') - args[i-1] = '\0'; + args[i-1] = '\0'; } } - if (quoted && args[i-1] == '"') - args[i-1] = '\0'; if (args[i]) { args[i] = '\0'; -- 2.10.2 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v7 3/4] lib/cmdline.c Remove quotes symmetrically. 2017-08-17 20:14 ` [PATCH v7 3/4] lib/cmdline.c Remove quotes symmetrically Michal Suchanek @ 2017-08-17 21:27 ` msuchanek 2017-08-21 10:27 ` Baoquan He 2017-08-24 11:02 ` Michael Ellerman 2 siblings, 0 replies; 14+ messages in thread From: msuchanek @ 2017-08-17 21:27 UTC (permalink / raw) To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman, Jonathan Corbet, Jessica Yu, Rusty Russell, Jason Baron, Hari Bathini, Mahesh J Salgaonkar, Daniel Axtens, Andrew Morton, Michal Suchanek, Thiago Jung Bauermann, Balbir Singh, Nicholas Piggin, Michael Neuling, Aneesh Kumar K.V, Christophe Leroy, Scott Wood, Oliver O'Halloran, David Howells, Sylvain 'ythier' Hitier, Ingo Molnar, Kees Cook, Thomas Gleixner Cc: Baoquan He, linux-doc, Lokesh Vutla, Viresh Kumar, linux-kernel, Steven Rostedt, , Tejun Heo, Ilya Matveychikov, linuxppc-dev On Thu, 17 Aug 2017 22:14:30 +0200 Michal Suchanek <msuchanek@suse.de> wrote: > Remove quotes from argument value only if there is qoute on both > sides. > > Signed-off-by: Michal Suchanek <msuchanek@suse.de> > --- > arch/powerpc/kernel/fadump.c | 6 ++---- > lib/cmdline.c | 7 ++----- > 2 files changed, 4 insertions(+), 9 deletions(-) > > diff --git a/arch/powerpc/kernel/fadump.c > b/arch/powerpc/kernel/fadump.c index a1614d9b8a21..d7da4ce9f7ae 100644 > --- a/arch/powerpc/kernel/fadump.c > +++ b/arch/powerpc/kernel/fadump.c > @@ -489,10 +489,8 @@ static void __init fadump_update_params(struct > param_info *param_info, *tgt++ = ' '; > > /* next_arg removes one leading and one trailing '"' */ > - if (*tgt == '"') > - shortening += 1; > - if (*(tgt + vallen + shortening) == '"') > - shortening += 1; > + if ((*tgt == '"') && (*(tgt + vallen + shortening) == '"')) s/shortening/1/ ^ in case somebody would want this patch and not the following one that removes the code. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v7 3/4] lib/cmdline.c Remove quotes symmetrically. 2017-08-17 20:14 ` [PATCH v7 3/4] lib/cmdline.c Remove quotes symmetrically Michal Suchanek 2017-08-17 21:27 ` msuchanek @ 2017-08-21 10:27 ` Baoquan He 2017-08-21 11:30 ` Michal Suchánek 2017-08-24 11:02 ` Michael Ellerman 2 siblings, 1 reply; 14+ messages in thread From: Baoquan He @ 2017-08-21 10:27 UTC (permalink / raw) To: Michal Suchanek Cc: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman, Jonathan Corbet, Jessica Yu, Rusty Russell, Jason Baron, Hari Bathini, Mahesh J Salgaonkar, Daniel Axtens, Andrew Morton, Thiago Jung Bauermann, Balbir Singh, Nicholas Piggin, Michael Neuling, Aneesh Kumar K.V, Christophe Leroy, Scott Wood, Oliver O'Halloran, David Howells, Sylvain 'ythier' Hitier, Ingo Molnar, Kees Cook, Thomas Gleixner, Steven Rostedt,, Viresh Kumar, Tejun Heo, Lokesh Vutla, Ilya Matveychikov, linuxppc-dev, linux-doc, linux-kernel On 08/17/17 at 10:14pm, Michal Suchanek wrote: > Remove quotes from argument value only if there is qoute on both sides. > > Signed-off-by: Michal Suchanek <msuchanek@suse.de> Sounds reasonable. Just for curiosity, do we have chance to pass in option with a single '"'? > --- > arch/powerpc/kernel/fadump.c | 6 ++---- > lib/cmdline.c | 7 ++----- > 2 files changed, 4 insertions(+), 9 deletions(-) > > diff --git a/arch/powerpc/kernel/fadump.c b/arch/powerpc/kernel/fadump.c > index a1614d9b8a21..d7da4ce9f7ae 100644 > --- a/arch/powerpc/kernel/fadump.c > +++ b/arch/powerpc/kernel/fadump.c > @@ -489,10 +489,8 @@ static void __init fadump_update_params(struct param_info *param_info, > *tgt++ = ' '; > > /* next_arg removes one leading and one trailing '"' */ > - if (*tgt == '"') > - shortening += 1; > - if (*(tgt + vallen + shortening) == '"') > - shortening += 1; > + if ((*tgt == '"') && (*(tgt + vallen + shortening) == '"')) > + shortening += 2; > > /* remove one leading and one trailing quote if both are present */ > if ((val[0] == '"') && (val[vallen - 1] == '"')) { > diff --git a/lib/cmdline.c b/lib/cmdline.c > index 4c0888c4a68d..01e701b2afe8 100644 > --- a/lib/cmdline.c > +++ b/lib/cmdline.c > @@ -227,14 +227,11 @@ char *next_arg(char *args, char **param, char **val) > *val = args + equals + 1; > > /* Don't include quotes in value. */ > - if (**val == '"') { > + if ((**val == '"') && (args[i-1] == '"')) { > (*val)++; > - if (args[i-1] == '"') > - args[i-1] = '\0'; > + args[i-1] = '\0'; > } > } > - if (quoted && args[i-1] == '"') > - args[i-1] = '\0'; > > if (args[i]) { > args[i] = '\0'; > -- > 2.10.2 > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v7 3/4] lib/cmdline.c Remove quotes symmetrically. 2017-08-21 10:27 ` Baoquan He @ 2017-08-21 11:30 ` Michal Suchánek 0 siblings, 0 replies; 14+ messages in thread From: Michal Suchánek @ 2017-08-21 11:30 UTC (permalink / raw) To: Baoquan He Cc: linux-doc, Lokesh Vutla, Viresh Kumar, David Howells, Sylvain 'ythier' Hitier, Paul Mackerras, Hari Bathini, Ingo Molnar, Michael Neuling, Jonathan Corbet, Ilya Matveychikov, Mahesh J Salgaonkar, Thiago Jung Bauermann, Kees Cook, Rusty Russell, Nicholas Piggin, Scott Wood, Jason Baron, Steven Rostedt, , Thomas Gleixner, Daniel Axtens, linux-kernel, Aneesh Kumar K.V, Jessica Yu, Tejun Heo, Oliver O'Halloran, Andrew Morton, linuxppc-dev On Mon, 21 Aug 2017 18:27:12 +0800 Baoquan He <bhe@redhat.com> wrote: > On 08/17/17 at 10:14pm, Michal Suchanek wrote: > > Remove quotes from argument value only if there is qoute on both > > sides. > > > > Signed-off-by: Michal Suchanek <msuchanek@suse.de> > > Sounds reasonable. Just for curiosity, do we have chance to pass in > option with a single '"'? No, we don't. Perhaps it would work if it was at the end of the commandline. next_arg checks that quoting is closed. It was possible but undocumented with previous behavior - you would place the quote in the middle and the closing quote at start or end so that next_arg would remove the closing quote but not the one in the middle. It would be also possible with shell-like backslash escaping but that is not implemented. Thanks Michal ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v7 3/4] lib/cmdline.c Remove quotes symmetrically. 2017-08-17 20:14 ` [PATCH v7 3/4] lib/cmdline.c Remove quotes symmetrically Michal Suchanek 2017-08-17 21:27 ` msuchanek 2017-08-21 10:27 ` Baoquan He @ 2017-08-24 11:02 ` Michael Ellerman 2017-08-24 12:15 ` msuchanek 2 siblings, 1 reply; 14+ messages in thread From: Michael Ellerman @ 2017-08-24 11:02 UTC (permalink / raw) To: Michal Suchanek, Benjamin Herrenschmidt, Paul Mackerras, Jonathan Corbet, Jessica Yu, Rusty Russell, Jason Baron, Hari Bathini, Mahesh J Salgaonkar, Daniel Axtens, Andrew Morton, Michal Suchanek, Thiago Jung Bauermann, Balbir Singh, Nicholas Piggin, Michael Neuling, Aneesh Kumar K.V, Christophe Leroy, Scott Wood, Oliver O'Halloran, David Howells, Sylvain 'ythier' Hitier, Ingo Molnar, Kees Cook, Thomas Gleixner Cc: Steven Rostedt, , Viresh Kumar, Tejun Heo, Lokesh Vutla, Baoquan He, Ilya Matveychikov, linuxppc-dev, linux-doc, linux-kernel Michal Suchanek <msuchanek@suse.de> writes: > Remove quotes from argument value only if there is qoute on both sides. > > Signed-off-by: Michal Suchanek <msuchanek@suse.de> > --- > arch/powerpc/kernel/fadump.c | 6 ++---- > lib/cmdline.c | 7 ++----- Can you split that into two patches? cheers > 2 files changed, 4 insertions(+), 9 deletions(-) > > diff --git a/arch/powerpc/kernel/fadump.c b/arch/powerpc/kernel/fadump.c > index a1614d9b8a21..d7da4ce9f7ae 100644 > --- a/arch/powerpc/kernel/fadump.c > +++ b/arch/powerpc/kernel/fadump.c > @@ -489,10 +489,8 @@ static void __init fadump_update_params(struct param_info *param_info, > *tgt++ = ' '; > > /* next_arg removes one leading and one trailing '"' */ > - if (*tgt == '"') > - shortening += 1; > - if (*(tgt + vallen + shortening) == '"') > - shortening += 1; > + if ((*tgt == '"') && (*(tgt + vallen + shortening) == '"')) > + shortening += 2; > > /* remove one leading and one trailing quote if both are present */ > if ((val[0] == '"') && (val[vallen - 1] == '"')) { > diff --git a/lib/cmdline.c b/lib/cmdline.c > index 4c0888c4a68d..01e701b2afe8 100644 > --- a/lib/cmdline.c > +++ b/lib/cmdline.c > @@ -227,14 +227,11 @@ char *next_arg(char *args, char **param, char **val) > *val = args + equals + 1; > > /* Don't include quotes in value. */ > - if (**val == '"') { > + if ((**val == '"') && (args[i-1] == '"')) { > (*val)++; > - if (args[i-1] == '"') > - args[i-1] = '\0'; > + args[i-1] = '\0'; > } > } > - if (quoted && args[i-1] == '"') > - args[i-1] = '\0'; > > if (args[i]) { > args[i] = '\0'; > -- > 2.10.2 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v7 3/4] lib/cmdline.c Remove quotes symmetrically. 2017-08-24 11:02 ` Michael Ellerman @ 2017-08-24 12:15 ` msuchanek 0 siblings, 0 replies; 14+ messages in thread From: msuchanek @ 2017-08-24 12:15 UTC (permalink / raw) To: Michael Ellerman Cc: Benjamin Herrenschmidt, Paul Mackerras, Jonathan Corbet, Jessica Yu, Rusty Russell, Jason Baron, Hari Bathini, Mahesh J Salgaonkar, Daniel Axtens, Andrew Morton, Thiago Jung Bauermann, Balbir Singh, Nicholas Piggin, Michael Neuling, Aneesh Kumar K.V, Christophe Leroy, Scott Wood, Oliver O'Halloran, David Howells, Sylvain 'ythier' Hitier, Ingo Molnar, Kees Cook, Thomas Gleixner, Baoquan He, linux-doc, Lokesh Vutla, Viresh Kumar, linux-kernel, Steven Rostedt, , Tejun Heo, Ilya Matveychikov, linuxppc-dev On Thu, 24 Aug 2017 21:02:47 +1000 Michael Ellerman <mpe@ellerman.id.au> wrote: > Michal Suchanek <msuchanek@suse.de> writes: > > > Remove quotes from argument value only if there is qoute on both > > sides. > > > > Signed-off-by: Michal Suchanek <msuchanek@suse.de> > > --- > > arch/powerpc/kernel/fadump.c | 6 ++---- > > lib/cmdline.c | 7 ++----- > > Can you split that into two patches? Not really. There is logic in lib/cmdline.c which is duplicated in arch/powerpc/kernel/fadump.c and so the two places should be updated in sync. Thanks Michal > > cheers > > > 2 files changed, 4 insertions(+), 9 deletions(-) > > > > diff --git a/arch/powerpc/kernel/fadump.c > > b/arch/powerpc/kernel/fadump.c index a1614d9b8a21..d7da4ce9f7ae > > 100644 --- a/arch/powerpc/kernel/fadump.c > > +++ b/arch/powerpc/kernel/fadump.c > > @@ -489,10 +489,8 @@ static void __init fadump_update_params(struct > > param_info *param_info, *tgt++ = ' '; > > > > /* next_arg removes one leading and one trailing '"' */ > > - if (*tgt == '"') > > - shortening += 1; > > - if (*(tgt + vallen + shortening) == '"') > > - shortening += 1; > > + if ((*tgt == '"') && (*(tgt + vallen + shortening) == '"')) > > + shortening += 2; > > > > /* remove one leading and one trailing quote if both are > > present */ if ((val[0] == '"') && (val[vallen - 1] == '"')) { > > diff --git a/lib/cmdline.c b/lib/cmdline.c > > index 4c0888c4a68d..01e701b2afe8 100644 > > --- a/lib/cmdline.c > > +++ b/lib/cmdline.c > > @@ -227,14 +227,11 @@ char *next_arg(char *args, char **param, char > > **val) *val = args + equals + 1; > > > > /* Don't include quotes in value. */ > > - if (**val == '"') { > > + if ((**val == '"') && (args[i-1] == '"')) { > > (*val)++; > > - if (args[i-1] == '"') > > - args[i-1] = '\0'; > > + args[i-1] = '\0'; > > } > > } > > - if (quoted && args[i-1] == '"') > > - args[i-1] = '\0'; > > > > if (args[i]) { > > args[i] = '\0'; > > -- > > 2.10.2 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v7 4/4] boot/param: add pointer to next argument to unknown parameter callback 2017-08-17 20:14 [PATCH v7 1/4] powerpc/fadump: reduce memory consumption for capture kernel Michal Suchanek 2017-08-17 20:14 ` [PATCH v7 2/4] powerpc/fadump: update documentation about 'fadump_extra_args=' parameter Michal Suchanek 2017-08-17 20:14 ` [PATCH v7 3/4] lib/cmdline.c Remove quotes symmetrically Michal Suchanek @ 2017-08-17 20:14 ` Michal Suchanek 2017-08-24 11:04 ` Michael Ellerman 2017-08-18 10:50 ` [PATCH v7 1/4] powerpc/fadump: reduce memory consumption for capture kernel Hari Bathini 3 siblings, 1 reply; 14+ messages in thread From: Michal Suchanek @ 2017-08-17 20:14 UTC (permalink / raw) To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman, Jonathan Corbet, Jessica Yu, Rusty Russell, Jason Baron, Hari Bathini, Mahesh J Salgaonkar, Daniel Axtens, Andrew Morton, Michal Suchanek, Thiago Jung Bauermann, Balbir Singh, Nicholas Piggin, Michael Neuling, Aneesh Kumar K.V, Christophe Leroy, Scott Wood, Oliver O'Halloran, David Howells, Sylvain 'ythier' Hitier, Ingo Molnar, Kees Cook, Thomas Gleixner Cc: Steven Rostedt,, Viresh Kumar, Tejun Heo, Lokesh Vutla, Baoquan He, Ilya Matveychikov, linuxppc-dev, linux-doc, linux-kernel The fadump parameter processing re-does the logic of next_arg quote stripping to determine where the argument ends. Pass pointer to the next argument instead to make this more robust. Signed-off-by: Michal Suchanek <msuchanek@suse.de> --- arch/powerpc/kernel/fadump.c | 13 +++++-------- arch/powerpc/mm/hugetlbpage.c | 4 ++-- include/linux/moduleparam.h | 2 +- init/main.c | 12 ++++++------ kernel/module.c | 4 ++-- kernel/params.c | 19 +++++++++++-------- lib/dynamic_debug.c | 2 +- 7 files changed, 28 insertions(+), 28 deletions(-) diff --git a/arch/powerpc/kernel/fadump.c b/arch/powerpc/kernel/fadump.c index d7da4ce9f7ae..6ef96711ee9a 100644 --- a/arch/powerpc/kernel/fadump.c +++ b/arch/powerpc/kernel/fadump.c @@ -474,13 +474,14 @@ struct param_info { }; static void __init fadump_update_params(struct param_info *param_info, - char *param, char *val) + char *param, char *val, char *next) { ptrdiff_t param_offset = param - param_info->tmp_cmdline; size_t vallen = val ? strlen(val) : 0; char *tgt = param_info->cmdline + param_offset + FADUMP_EXTRA_ARGS_LEN - param_info->shortening; - int shortening = 0; + int shortening = ((next - 1) - (param)) + - (FADUMP_EXTRA_ARGS_LEN + 1 + vallen); if (!val) return; @@ -488,10 +489,6 @@ static void __init fadump_update_params(struct param_info *param_info, /* remove '=' */ *tgt++ = ' '; - /* next_arg removes one leading and one trailing '"' */ - if ((*tgt == '"') && (*(tgt + vallen + shortening) == '"')) - shortening += 2; - /* remove one leading and one trailing quote if both are present */ if ((val[0] == '"') && (val[vallen - 1] == '"')) { shortening += 2; @@ -517,7 +514,7 @@ static void __init fadump_update_params(struct param_info *param_info, * to enforce the parameters passed through it */ static int __init fadump_rework_cmdline_params(char *param, char *val, - const char *unused, void *arg) + char *next, const char *unused, void *arg) { struct param_info *param_info = (struct param_info *)arg; @@ -525,7 +522,7 @@ static int __init fadump_rework_cmdline_params(char *param, char *val, strlen(FADUMP_EXTRA_ARGS_PARAM) - 1)) return 0; - fadump_update_params(param_info, param, val); + fadump_update_params(param_info, param, val, next); return 0; } diff --git a/arch/powerpc/mm/hugetlbpage.c b/arch/powerpc/mm/hugetlbpage.c index e1bf5ca397fe..3a4cce552906 100644 --- a/arch/powerpc/mm/hugetlbpage.c +++ b/arch/powerpc/mm/hugetlbpage.c @@ -268,8 +268,8 @@ int alloc_bootmem_huge_page(struct hstate *hstate) unsigned long gpage_npages[MMU_PAGE_COUNT]; -static int __init do_gpage_early_setup(char *param, char *val, - const char *unused, void *arg) +static int __init do_gpage_early_setup(char *param, char *val, char *unused1, + const char *unused2, void *arg) { static phys_addr_t size; unsigned long npages; diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h index 1ee7b30dafec..fec05a186c08 100644 --- a/include/linux/moduleparam.h +++ b/include/linux/moduleparam.h @@ -326,7 +326,7 @@ extern char *parse_args(const char *name, s16 level_min, s16 level_max, void *arg, - int (*unknown)(char *param, char *val, + int (*unknown)(char *param, char *val, char *next, const char *doing, void *arg)); /* Called by module remove. */ diff --git a/init/main.c b/init/main.c index 052481fbe363..920c3564b2f0 100644 --- a/init/main.c +++ b/init/main.c @@ -239,7 +239,7 @@ static int __init loglevel(char *str) early_param("loglevel", loglevel); /* Change NUL term back to "=", to make "param" the whole string. */ -static int __init repair_env_string(char *param, char *val, +static int __init repair_env_string(char *param, char *val, char *unused2, const char *unused, void *arg) { if (val) { @@ -257,7 +257,7 @@ static int __init repair_env_string(char *param, char *val, } /* Anything after -- gets handed straight to init. */ -static int __init set_init_arg(char *param, char *val, +static int __init set_init_arg(char *param, char *val, char *unused2, const char *unused, void *arg) { unsigned int i; @@ -265,7 +265,7 @@ static int __init set_init_arg(char *param, char *val, if (panic_later) return 0; - repair_env_string(param, val, unused, NULL); + repair_env_string(param, val, unused2, unused, NULL); for (i = 0; argv_init[i]; i++) { if (i == MAX_INIT_ARGS) { @@ -282,10 +282,10 @@ static int __init set_init_arg(char *param, char *val, * Unknown boot options get handed to init, unless they look like * unused parameters (modprobe will find them in /proc/cmdline). */ -static int __init unknown_bootoption(char *param, char *val, +static int __init unknown_bootoption(char *param, char *val, char *unused2, const char *unused, void *arg) { - repair_env_string(param, val, unused, NULL); + repair_env_string(param, val, unused2, unused, NULL); /* Handle obsolete-style parameters */ if (obsolete_checksetup(param)) @@ -437,7 +437,7 @@ static noinline void __ref rest_init(void) } /* Check for early params. */ -static int __init do_early_param(char *param, char *val, +static int __init do_early_param(char *param, char *val, char *unused2, const char *unused, void *arg) { const struct obs_kernel_param *p; diff --git a/kernel/module.c b/kernel/module.c index 40f983cbea81..5e241ba95a91 100644 --- a/kernel/module.c +++ b/kernel/module.c @@ -3609,8 +3609,8 @@ static int prepare_coming_module(struct module *mod) return 0; } -static int unknown_module_param_cb(char *param, char *val, const char *modname, - void *arg) +static int unknown_module_param_cb(char *param, char *val, char *next, + const char *modname, void *arg) { struct module *mod = arg; int ret; diff --git a/kernel/params.c b/kernel/params.c index 60b2d8101355..9f568fbf5287 100644 --- a/kernel/params.c +++ b/kernel/params.c @@ -119,13 +119,14 @@ static void param_check_unsafe(const struct kernel_param *kp) static int parse_one(char *param, char *val, + char *next, const char *doing, const struct kernel_param *params, unsigned num_params, s16 min_level, s16 max_level, void *arg, - int (*handle_unknown)(char *param, char *val, + int (*handle_unknown)(char *param, char *val, char *next, const char *doing, void *arg)) { unsigned int i; @@ -153,7 +154,7 @@ static int parse_one(char *param, if (handle_unknown) { pr_debug("doing %s: %s='%s'\n", doing, param, val); - return handle_unknown(param, val, doing, arg); + return handle_unknown(param, val, next, doing, arg); } pr_debug("Unknown argument '%s'\n", param); @@ -168,10 +169,10 @@ char *parse_args(const char *doing, s16 min_level, s16 max_level, void *arg, - int (*unknown)(char *param, char *val, + int (*unknown)(char *param, char *val, char *next, const char *doing, void *arg)) { - char *param, *val, *err = NULL; + char *param, *val, *next, *err = NULL; /* Chew leading spaces */ args = skip_spaces(args); @@ -179,16 +180,18 @@ char *parse_args(const char *doing, if (*args) pr_debug("doing %s, parsing ARGS: '%s'\n", doing, args); - while (*args) { + next = next_arg(args, ¶m, &val); + do { int ret; int irq_was_disabled; - args = next_arg(args, ¶m, &val); + args = next; + next = next_arg(args, ¶m, &val); /* Stop at -- */ if (!val && strcmp(param, "--") == 0) return err ?: args; irq_was_disabled = irqs_disabled(); - ret = parse_one(param, val, doing, params, num, + ret = parse_one(param, val, next, doing, params, num, min_level, max_level, arg, unknown); if (irq_was_disabled && !irqs_disabled()) pr_warn("%s: option '%s' enabled irq's!\n", @@ -211,7 +214,7 @@ char *parse_args(const char *doing, } err = ERR_PTR(ret); - } + } while (*next); return err; } diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c index da796e2dc4f5..bdedc1ca5bf6 100644 --- a/lib/dynamic_debug.c +++ b/lib/dynamic_debug.c @@ -888,7 +888,7 @@ static int ddebug_dyndbg_param_cb(char *param, char *val, } /* handle both dyndbg and $module.dyndbg params at boot */ -static int ddebug_dyndbg_boot_param_cb(char *param, char *val, +static int ddebug_dyndbg_boot_param_cb(char *param, char *val, char *unused2, const char *unused, void *arg) { vpr_info("%s=\"%s\"\n", param, val); -- 2.10.2 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v7 4/4] boot/param: add pointer to next argument to unknown parameter callback 2017-08-17 20:14 ` [PATCH v7 4/4] boot/param: add pointer to next argument to unknown parameter callback Michal Suchanek @ 2017-08-24 11:04 ` Michael Ellerman 2017-08-24 12:17 ` msuchanek 0 siblings, 1 reply; 14+ messages in thread From: Michael Ellerman @ 2017-08-24 11:04 UTC (permalink / raw) To: Michal Suchanek, Benjamin Herrenschmidt, Paul Mackerras, Jonathan Corbet, Jessica Yu, Rusty Russell, Jason Baron, Hari Bathini, Mahesh J Salgaonkar, Daniel Axtens, Andrew Morton, Michal Suchanek, Thiago Jung Bauermann, Balbir Singh, Nicholas Piggin, Michael Neuling, Aneesh Kumar K.V, Christophe Leroy, Scott Wood, Oliver O'Halloran, David Howells, Sylvain 'ythier' Hitier, Ingo Molnar, Kees Cook, Thomas Gleixner Cc: Steven Rostedt, , Viresh Kumar, Tejun Heo, Lokesh Vutla, Baoquan He, Ilya Matveychikov, linuxppc-dev, linux-doc, linux-kernel Michal Suchanek <msuchanek@suse.de> writes: > The fadump parameter processing re-does the logic of next_arg quote > stripping to determine where the argument ends. Pass pointer to the > next argument instead to make this more robust. > > Signed-off-by: Michal Suchanek <msuchanek@suse.de> > --- > arch/powerpc/kernel/fadump.c | 13 +++++-------- > arch/powerpc/mm/hugetlbpage.c | 4 ++-- > include/linux/moduleparam.h | 2 +- > init/main.c | 12 ++++++------ > kernel/module.c | 4 ++-- > kernel/params.c | 19 +++++++++++-------- > lib/dynamic_debug.c | 2 +- > 7 files changed, 28 insertions(+), 28 deletions(-) Can you split out a patch that adds the next argument and updates the callers. And then a patch for the fadump to use the new arg. cheers > diff --git a/arch/powerpc/kernel/fadump.c b/arch/powerpc/kernel/fadump.c > index d7da4ce9f7ae..6ef96711ee9a 100644 > --- a/arch/powerpc/kernel/fadump.c > +++ b/arch/powerpc/kernel/fadump.c > @@ -474,13 +474,14 @@ struct param_info { > }; > > static void __init fadump_update_params(struct param_info *param_info, > - char *param, char *val) > + char *param, char *val, char *next) > { > ptrdiff_t param_offset = param - param_info->tmp_cmdline; > size_t vallen = val ? strlen(val) : 0; > char *tgt = param_info->cmdline + param_offset + > FADUMP_EXTRA_ARGS_LEN - param_info->shortening; > - int shortening = 0; > + int shortening = ((next - 1) - (param)) > + - (FADUMP_EXTRA_ARGS_LEN + 1 + vallen); > > if (!val) > return; > @@ -488,10 +489,6 @@ static void __init fadump_update_params(struct param_info *param_info, > /* remove '=' */ > *tgt++ = ' '; > > - /* next_arg removes one leading and one trailing '"' */ > - if ((*tgt == '"') && (*(tgt + vallen + shortening) == '"')) > - shortening += 2; > - > /* remove one leading and one trailing quote if both are present */ > if ((val[0] == '"') && (val[vallen - 1] == '"')) { > shortening += 2; > @@ -517,7 +514,7 @@ static void __init fadump_update_params(struct param_info *param_info, > * to enforce the parameters passed through it > */ > static int __init fadump_rework_cmdline_params(char *param, char *val, > - const char *unused, void *arg) > + char *next, const char *unused, void *arg) > { > struct param_info *param_info = (struct param_info *)arg; > > @@ -525,7 +522,7 @@ static int __init fadump_rework_cmdline_params(char *param, char *val, > strlen(FADUMP_EXTRA_ARGS_PARAM) - 1)) > return 0; > > - fadump_update_params(param_info, param, val); > + fadump_update_params(param_info, param, val, next); > > return 0; > } > diff --git a/arch/powerpc/mm/hugetlbpage.c b/arch/powerpc/mm/hugetlbpage.c > index e1bf5ca397fe..3a4cce552906 100644 > --- a/arch/powerpc/mm/hugetlbpage.c > +++ b/arch/powerpc/mm/hugetlbpage.c > @@ -268,8 +268,8 @@ int alloc_bootmem_huge_page(struct hstate *hstate) > > unsigned long gpage_npages[MMU_PAGE_COUNT]; > > -static int __init do_gpage_early_setup(char *param, char *val, > - const char *unused, void *arg) > +static int __init do_gpage_early_setup(char *param, char *val, char *unused1, > + const char *unused2, void *arg) > { > static phys_addr_t size; > unsigned long npages; > diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h > index 1ee7b30dafec..fec05a186c08 100644 > --- a/include/linux/moduleparam.h > +++ b/include/linux/moduleparam.h > @@ -326,7 +326,7 @@ extern char *parse_args(const char *name, > s16 level_min, > s16 level_max, > void *arg, > - int (*unknown)(char *param, char *val, > + int (*unknown)(char *param, char *val, char *next, > const char *doing, void *arg)); > > /* Called by module remove. */ > diff --git a/init/main.c b/init/main.c > index 052481fbe363..920c3564b2f0 100644 > --- a/init/main.c > +++ b/init/main.c > @@ -239,7 +239,7 @@ static int __init loglevel(char *str) > early_param("loglevel", loglevel); > > /* Change NUL term back to "=", to make "param" the whole string. */ > -static int __init repair_env_string(char *param, char *val, > +static int __init repair_env_string(char *param, char *val, char *unused2, > const char *unused, void *arg) > { > if (val) { > @@ -257,7 +257,7 @@ static int __init repair_env_string(char *param, char *val, > } > > /* Anything after -- gets handed straight to init. */ > -static int __init set_init_arg(char *param, char *val, > +static int __init set_init_arg(char *param, char *val, char *unused2, > const char *unused, void *arg) > { > unsigned int i; > @@ -265,7 +265,7 @@ static int __init set_init_arg(char *param, char *val, > if (panic_later) > return 0; > > - repair_env_string(param, val, unused, NULL); > + repair_env_string(param, val, unused2, unused, NULL); > > for (i = 0; argv_init[i]; i++) { > if (i == MAX_INIT_ARGS) { > @@ -282,10 +282,10 @@ static int __init set_init_arg(char *param, char *val, > * Unknown boot options get handed to init, unless they look like > * unused parameters (modprobe will find them in /proc/cmdline). > */ > -static int __init unknown_bootoption(char *param, char *val, > +static int __init unknown_bootoption(char *param, char *val, char *unused2, > const char *unused, void *arg) > { > - repair_env_string(param, val, unused, NULL); > + repair_env_string(param, val, unused2, unused, NULL); > > /* Handle obsolete-style parameters */ > if (obsolete_checksetup(param)) > @@ -437,7 +437,7 @@ static noinline void __ref rest_init(void) > } > > /* Check for early params. */ > -static int __init do_early_param(char *param, char *val, > +static int __init do_early_param(char *param, char *val, char *unused2, > const char *unused, void *arg) > { > const struct obs_kernel_param *p; > diff --git a/kernel/module.c b/kernel/module.c > index 40f983cbea81..5e241ba95a91 100644 > --- a/kernel/module.c > +++ b/kernel/module.c > @@ -3609,8 +3609,8 @@ static int prepare_coming_module(struct module *mod) > return 0; > } > > -static int unknown_module_param_cb(char *param, char *val, const char *modname, > - void *arg) > +static int unknown_module_param_cb(char *param, char *val, char *next, > + const char *modname, void *arg) > { > struct module *mod = arg; > int ret; > diff --git a/kernel/params.c b/kernel/params.c > index 60b2d8101355..9f568fbf5287 100644 > --- a/kernel/params.c > +++ b/kernel/params.c > @@ -119,13 +119,14 @@ static void param_check_unsafe(const struct kernel_param *kp) > > static int parse_one(char *param, > char *val, > + char *next, > const char *doing, > const struct kernel_param *params, > unsigned num_params, > s16 min_level, > s16 max_level, > void *arg, > - int (*handle_unknown)(char *param, char *val, > + int (*handle_unknown)(char *param, char *val, char *next, > const char *doing, void *arg)) > { > unsigned int i; > @@ -153,7 +154,7 @@ static int parse_one(char *param, > > if (handle_unknown) { > pr_debug("doing %s: %s='%s'\n", doing, param, val); > - return handle_unknown(param, val, doing, arg); > + return handle_unknown(param, val, next, doing, arg); > } > > pr_debug("Unknown argument '%s'\n", param); > @@ -168,10 +169,10 @@ char *parse_args(const char *doing, > s16 min_level, > s16 max_level, > void *arg, > - int (*unknown)(char *param, char *val, > + int (*unknown)(char *param, char *val, char *next, > const char *doing, void *arg)) > { > - char *param, *val, *err = NULL; > + char *param, *val, *next, *err = NULL; > > /* Chew leading spaces */ > args = skip_spaces(args); > @@ -179,16 +180,18 @@ char *parse_args(const char *doing, > if (*args) > pr_debug("doing %s, parsing ARGS: '%s'\n", doing, args); > > - while (*args) { > + next = next_arg(args, ¶m, &val); > + do { > int ret; > int irq_was_disabled; > > - args = next_arg(args, ¶m, &val); > + args = next; > + next = next_arg(args, ¶m, &val); > /* Stop at -- */ > if (!val && strcmp(param, "--") == 0) > return err ?: args; > irq_was_disabled = irqs_disabled(); > - ret = parse_one(param, val, doing, params, num, > + ret = parse_one(param, val, next, doing, params, num, > min_level, max_level, arg, unknown); > if (irq_was_disabled && !irqs_disabled()) > pr_warn("%s: option '%s' enabled irq's!\n", > @@ -211,7 +214,7 @@ char *parse_args(const char *doing, > } > > err = ERR_PTR(ret); > - } > + } while (*next); > > return err; > } > diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c > index da796e2dc4f5..bdedc1ca5bf6 100644 > --- a/lib/dynamic_debug.c > +++ b/lib/dynamic_debug.c > @@ -888,7 +888,7 @@ static int ddebug_dyndbg_param_cb(char *param, char *val, > } > > /* handle both dyndbg and $module.dyndbg params at boot */ > -static int ddebug_dyndbg_boot_param_cb(char *param, char *val, > +static int ddebug_dyndbg_boot_param_cb(char *param, char *val, char *unused2, > const char *unused, void *arg) > { > vpr_info("%s=\"%s\"\n", param, val); > -- > 2.10.2 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v7 4/4] boot/param: add pointer to next argument to unknown parameter callback 2017-08-24 11:04 ` Michael Ellerman @ 2017-08-24 12:17 ` msuchanek 0 siblings, 0 replies; 14+ messages in thread From: msuchanek @ 2017-08-24 12:17 UTC (permalink / raw) To: Michael Ellerman Cc: Benjamin Herrenschmidt, Paul Mackerras, Jonathan Corbet, Jessica Yu, Rusty Russell, Jason Baron, Hari Bathini, Mahesh J Salgaonkar, Daniel Axtens, Andrew Morton, Thiago Jung Bauermann, Balbir Singh, Nicholas Piggin, Michael Neuling, Aneesh Kumar K.V, Christophe Leroy, Scott Wood, Oliver O'Halloran, David Howells, Sylvain 'ythier' Hitier, Ingo Molnar, Kees Cook, Thomas Gleixner, Steven Rostedt,, Viresh Kumar, Tejun Heo, Lokesh Vutla, Baoquan He, Ilya Matveychikov, linuxppc-dev, linux-doc, linux-kernel On Thu, 24 Aug 2017 21:04:51 +1000 Michael Ellerman <mpe@ellerman.id.au> wrote: > Michal Suchanek <msuchanek@suse.de> writes: > > > The fadump parameter processing re-does the logic of next_arg quote > > stripping to determine where the argument ends. Pass pointer to the > > next argument instead to make this more robust. > > > > Signed-off-by: Michal Suchanek <msuchanek@suse.de> > > --- > > arch/powerpc/kernel/fadump.c | 13 +++++-------- > > arch/powerpc/mm/hugetlbpage.c | 4 ++-- > > include/linux/moduleparam.h | 2 +- > > init/main.c | 12 ++++++------ > > kernel/module.c | 4 ++-- > > kernel/params.c | 19 +++++++++++-------- > > lib/dynamic_debug.c | 2 +- > > 7 files changed, 28 insertions(+), 28 deletions(-) > > Can you split out a patch that adds the next argument and updates the > callers. And then a patch for the fadump to use the new arg. > > cheers Yes, that makes sense. Thanks Michal ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v7 1/4] powerpc/fadump: reduce memory consumption for capture kernel 2017-08-17 20:14 [PATCH v7 1/4] powerpc/fadump: reduce memory consumption for capture kernel Michal Suchanek ` (2 preceding siblings ...) 2017-08-17 20:14 ` [PATCH v7 4/4] boot/param: add pointer to next argument to unknown parameter callback Michal Suchanek @ 2017-08-18 10:50 ` Hari Bathini 2017-08-18 11:57 ` Michal Suchánek 3 siblings, 1 reply; 14+ messages in thread From: Hari Bathini @ 2017-08-18 10:50 UTC (permalink / raw) To: Michal Suchanek, Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman, Jonathan Corbet, Jessica Yu, Rusty Russell, Jason Baron, Mahesh J Salgaonkar, Daniel Axtens, Andrew Morton, Thiago Jung Bauermann, Balbir Singh, Nicholas Piggin, Michael Neuling, Aneesh Kumar K.V, Christophe Leroy, Scott Wood, Oliver O'Halloran, David Howells, Sylvain 'ythier' Hitier, Ingo Molnar, Kees Cook, Thomas Gleixner Cc: Baoquan He, linux-doc, Lokesh Vutla, Viresh Kumar, linux-kernel, Steven Rostedt,, Tejun Heo, Ilya Matveychikov, linuxppc-dev Hi Michal, Thanks for the patches. I tried testing with the patches: [ 0.000000] fadump: Firmware-assisted dump is active. [ 0.000000] fadump: Modifying command line to enforce the additional parameters passed through 'fadump_extra_args=' [ 0.000000] fadump: Original command line: BOOT_IMAGE=/vmlinux-4.13.0-rc1-bz155783+ root=/dev/mapper/rhel-root ro crashkernel=2048M fadump=on fadump_reserve_mem=1024M "fadump_extra_args=nr_cpus=1 numa=off udev.childern-max=2" rd.lvm.lv=rhel/root rd.lvm.lv=rhel/swap [ 0.000000] fadump: Modified command line: BOOT_IMAGE=/vmlinux-4.13.0-rc1-bz155783+ root=/dev/mapper/rhel-root ro crashkernel=2048M fadump=on fadump_reserve_mem=1024M "fadump_extra_args nr_cpus=1 numa=off udev.childern-max=2" rd.lvm.lv=rhel/root rd.lvm.lv=rhel/swap Looks like the quotes are retained not enforcing the parameters... I am yet to test the patches in other scenarios though.. Thanks Hari On Friday 18 August 2017 01:44 AM, Michal Suchanek wrote: > From: Hari Bathini <hbathini@linux.vnet.ibm.com> > > With fadump (dump capture) kernel booting like a regular kernel, it needs > almost the same amount of memory to boot as the production kernel, which is > unwarranted for a dump capture kernel. But with no option to disable some > of the unnecessary subsystems in fadump kernel, that much memory is wasted > on fadump, depriving the production kernel of that memory. > > Introduce kernel parameter 'fadump_extra_args=' that would take regular > parameters as a space separated quoted string, to be enforced when fadump > is active. This 'fadump_extra_args=' parameter can be leveraged to pass > parameters like nr_cpus=1, cgroup_disable=memory and numa=off, to disable > unwarranted resources/subsystems. > > Also, ensure the log "Firmware-assisted dump is active" is printed early > in the boot process to put the subsequent fadump messages in context. > > Suggested-by: Michael Ellerman <mpe@ellerman.id.au> > Signed-off-by: Hari Bathini <hbathini@linux.vnet.ibm.com> > Signed-off-by: Michal Suchanek <msuchanek@suse.de> > --- > Changes from v6: > Correct and simplify quote handling. Ideally I would like to extend > parse_args to give the length of the original quoted value to callback. > However, parse_args removes at most one doubel-quote from the start and > one from the end so that is easy to detect. Otherwise all other users > will have to be updated to trash the new argument. > --- > arch/powerpc/include/asm/fadump.h | 2 + > arch/powerpc/kernel/fadump.c | 109 ++++++++++++++++++++++++++++++++++++-- > arch/powerpc/kernel/prom.c | 7 +++ > 3 files changed, 115 insertions(+), 3 deletions(-) > > diff --git a/arch/powerpc/include/asm/fadump.h b/arch/powerpc/include/asm/fadump.h > index ce88bbe1d809..98ae00943fb3 100644 > --- a/arch/powerpc/include/asm/fadump.h > +++ b/arch/powerpc/include/asm/fadump.h > @@ -208,11 +208,13 @@ extern int early_init_dt_scan_fw_dump(unsigned long node, > const char *uname, int depth, void *data); > extern int fadump_reserve_mem(void); > extern int setup_fadump(void); > +extern void enforce_fadump_extra_args(char *cmdline); > extern int is_fadump_active(void); > extern void crash_fadump(struct pt_regs *, const char *); > extern void fadump_cleanup(void); > > #else /* CONFIG_FA_DUMP */ > +static inline void enforce_fadump_extra_args(char *cmdline) { } > static inline int is_fadump_active(void) { return 0; } > static inline void crash_fadump(struct pt_regs *regs, const char *str) { } > #endif > diff --git a/arch/powerpc/kernel/fadump.c b/arch/powerpc/kernel/fadump.c > index dc0c49cfd90a..a1614d9b8a21 100644 > --- a/arch/powerpc/kernel/fadump.c > +++ b/arch/powerpc/kernel/fadump.c > @@ -78,8 +78,10 @@ int __init early_init_dt_scan_fw_dump(unsigned long node, > * dump data waiting for us. > */ > fdm_active = of_get_flat_dt_prop(node, "ibm,kernel-dump", NULL); > - if (fdm_active) > + if (fdm_active) { > + pr_info("Firmware-assisted dump is active.\n"); > fw_dump.dump_active = 1; > + } > > /* Get the sizes required to store dump data for the firmware provided > * dump sections. > @@ -332,8 +334,11 @@ int __init fadump_reserve_mem(void) > { > unsigned long base, size, memory_boundary; > > - if (!fw_dump.fadump_enabled) > + if (!fw_dump.fadump_enabled) { > + if (fw_dump.dump_active) > + pr_warn("Firmware-assisted dump was active but kernel booted with fadump disabled!\n"); > return 0; > + } > > if (!fw_dump.fadump_supported) { > printk(KERN_INFO "Firmware-assisted dump is not supported on" > @@ -373,7 +378,6 @@ int __init fadump_reserve_mem(void) > memory_boundary = memblock_end_of_DRAM(); > > if (fw_dump.dump_active) { > - printk(KERN_INFO "Firmware-assisted dump is active.\n"); > /* > * If last boot has crashed then reserve all the memory > * above boot_memory_size so that we don't touch it until > @@ -460,6 +464,105 @@ static int __init early_fadump_reserve_mem(char *p) > } > early_param("fadump_reserve_mem", early_fadump_reserve_mem); > > +#define FADUMP_EXTRA_ARGS_PARAM "fadump_extra_args=" > +#define FADUMP_EXTRA_ARGS_LEN (strlen(FADUMP_EXTRA_ARGS_PARAM) - 1) > + > +struct param_info { > + char *cmdline; > + char *tmp_cmdline; > + int shortening; > +}; > + > +static void __init fadump_update_params(struct param_info *param_info, > + char *param, char *val) > +{ > + ptrdiff_t param_offset = param - param_info->tmp_cmdline; > + size_t vallen = val ? strlen(val) : 0; > + char *tgt = param_info->cmdline + param_offset + > + FADUMP_EXTRA_ARGS_LEN - param_info->shortening; > + int shortening = 0; > + > + if (!val) > + return; > + > + /* remove '=' */ > + *tgt++ = ' '; > + > + /* next_arg removes one leading and one trailing '"' */ > + if (*tgt == '"') > + shortening += 1; > + if (*(tgt + vallen + shortening) == '"') > + shortening += 1; > + > + /* remove one leading and one trailing quote if both are present */ > + if ((val[0] == '"') && (val[vallen - 1] == '"')) { > + shortening += 2; > + vallen -= 2; > + val++; > + } > + > + /* some characters were removed - move the trailing part of cmdline */ > + if (shortening) { > + char *src; > + > + strncpy(tgt, val, vallen); > + tgt += vallen; > + src = tgt + shortening; > + memmove(tgt, src, strlen(src) + 1); > + } > + > + param_info->shortening += shortening; > +} > + > +/* > + * Reworks command line parameters and splits 'fadump_extra_args=' param > + * to enforce the parameters passed through it > + */ > +static int __init fadump_rework_cmdline_params(char *param, char *val, > + const char *unused, void *arg) > +{ > + struct param_info *param_info = (struct param_info *)arg; > + > + if (strncmp(param, FADUMP_EXTRA_ARGS_PARAM, > + strlen(FADUMP_EXTRA_ARGS_PARAM) - 1)) > + return 0; > + > + fadump_update_params(param_info, param, val); > + > + return 0; > +} > + > +/* > + * Replace every occurrence of 'fadump_extra_args="param1 param2 param3"' > + * in cmdline with 'fadump_extra_args param1 param2 param3' by stripping > + * off '=' and quotes, if any. This ensures that the additional parameters > + * passed with 'fadump_extra_args=' are enforced. > + */ > +void __init enforce_fadump_extra_args(char *cmdline) > +{ > + static char tmp_cmdline[COMMAND_LINE_SIZE] __initdata; > + static char init_cmdline[COMMAND_LINE_SIZE] __initdata; > + struct param_info param_info; > + > + if (strstr(cmdline, FADUMP_EXTRA_ARGS_PARAM) == NULL) > + return; > + > + pr_info("Modifying command line to enforce the additional parameters passed through 'fadump_extra_args='"); > + > + param_info.cmdline = cmdline; > + param_info.tmp_cmdline = tmp_cmdline; > + param_info.shortening = 0; > + > + strlcpy(init_cmdline, cmdline, COMMAND_LINE_SIZE); > + > + strlcpy(tmp_cmdline, cmdline, COMMAND_LINE_SIZE); > + parse_args("fadump params", tmp_cmdline, NULL, 0, 0, 0, > + ¶m_info, &fadump_rework_cmdline_params); > + > + pr_info("Original command line: %s\n", init_cmdline); > + pr_info("Modified command line: %s\n", cmdline); > +} > + > static int register_fw_dump(struct fadump_mem_struct *fdm) > { > int rc, err; > diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c > index f83056297441..2e6f40217266 100644 > --- a/arch/powerpc/kernel/prom.c > +++ b/arch/powerpc/kernel/prom.c > @@ -693,6 +693,13 @@ void __init early_init_devtree(void *params) > of_scan_flat_dt(early_init_dt_scan_root, NULL); > of_scan_flat_dt(early_init_dt_scan_memory_ppc, NULL); > > + /* > + * Look for 'fadump_extra_args=' parameter and enfore the additional > + * parameters passed to it if fadump is active. > + */ > + if (is_fadump_active()) > + enforce_fadump_extra_args(boot_command_line); > + > parse_early_param(); > > /* make sure we've parsed cmdline for mem= before this */ ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v7 1/4] powerpc/fadump: reduce memory consumption for capture kernel 2017-08-18 10:50 ` [PATCH v7 1/4] powerpc/fadump: reduce memory consumption for capture kernel Hari Bathini @ 2017-08-18 11:57 ` Michal Suchánek 2017-08-18 14:00 ` Hari Bathini 0 siblings, 1 reply; 14+ messages in thread From: Michal Suchánek @ 2017-08-18 11:57 UTC (permalink / raw) To: Hari Bathini Cc: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman, Jonathan Corbet, Jessica Yu, Rusty Russell, Jason Baron, Mahesh J Salgaonkar, Daniel Axtens, Andrew Morton, Thiago Jung Bauermann, Balbir Singh, Nicholas Piggin, Michael Neuling, Aneesh Kumar K.V, Christophe Leroy, Scott Wood, Oliver O'Halloran, David Howells, Sylvain 'ythier' Hitier, Ingo Molnar, Kees Cook, Thomas Gleixner, Baoquan He, linux-doc, Lokesh Vutla, Viresh Kumar, linux-kernel, Steven Rostedt, , Tejun Heo, Ilya Matveychikov, linuxppc-dev On Fri, 18 Aug 2017 16:20:53 +0530 Hari Bathini <hbathini@linux.vnet.ibm.com> wrote: > Hi Michal, > > > Thanks for the patches. I tried testing with the patches: > > [ 0.000000] fadump: Firmware-assisted dump is active. > [ 0.000000] fadump: Modifying command line to enforce the > additional parameters passed through 'fadump_extra_args=' > [ 0.000000] fadump: Original command line: > BOOT_IMAGE=/vmlinux-4.13.0-rc1-bz155783+ root=/dev/mapper/rhel-root > ro crashkernel=2048M fadump=on fadump_reserve_mem=1024M > "fadump_extra_args=nr_cpus=1 numa=off udev.childern-max=2" > rd.lvm.lv=rhel/root rd.lvm.lv=rhel/swap > [ 0.000000] fadump: Modified command line: > BOOT_IMAGE=/vmlinux-4.13.0-rc1-bz155783+ root=/dev/mapper/rhel-root > ro crashkernel=2048M fadump=on fadump_reserve_mem=1024M > "fadump_extra_args nr_cpus=1 numa=off udev.childern-max=2" > rd.lvm.lv=rhel/root rd.lvm.lv=rhel/swap > > Looks like the quotes are retained not enforcing the parameters... Hello, You are passing an argument >>"fadump_extra_args<< - that is an argument containing a quote in the name. I did not test this scenario assuming the argument name would not match in this case. It would probably not match if the quote was in the middle of the argument name but at the start it is skipped. Note that due to the requirement to remove quotes symmetrically which is added in the third patch this case does not break the commandline - it merely makes the arguments ineffective. The format suggested in the documentation is >>fadump_extra_args="nr_cpus=1 numa=off udev.childern-max=2"<< - that is quotes around the value. This format worked in my testing. Unfortunately, the format with quote before argument name would probably require another extra parameter to the callback to detect properly. Thanks Michal > > I am yet to test the patches in other scenarios though.. > > > Thanks > > Hari > > > On Friday 18 August 2017 01:44 AM, Michal Suchanek wrote: > > From: Hari Bathini <hbathini@linux.vnet.ibm.com> > > > > With fadump (dump capture) kernel booting like a regular kernel, it > > needs almost the same amount of memory to boot as the production > > kernel, which is unwarranted for a dump capture kernel. But with no > > option to disable some of the unnecessary subsystems in fadump > > kernel, that much memory is wasted on fadump, depriving the > > production kernel of that memory. > > > > Introduce kernel parameter 'fadump_extra_args=' that would take > > regular parameters as a space separated quoted string, to be > > enforced when fadump is active. This 'fadump_extra_args=' parameter > > can be leveraged to pass parameters like nr_cpus=1, > > cgroup_disable=memory and numa=off, to disable unwarranted > > resources/subsystems. > > > > Also, ensure the log "Firmware-assisted dump is active" is printed > > early in the boot process to put the subsequent fadump messages in > > context. > > > > Suggested-by: Michael Ellerman <mpe@ellerman.id.au> > > Signed-off-by: Hari Bathini <hbathini@linux.vnet.ibm.com> > > Signed-off-by: Michal Suchanek <msuchanek@suse.de> > > --- > > Changes from v6: > > Correct and simplify quote handling. Ideally I would like to extend > > parse_args to give the length of the original quoted value to > > callback. However, parse_args removes at most one doubel-quote from > > the start and one from the end so that is easy to detect. Otherwise > > all other users will have to be updated to trash the new argument. > > --- > > arch/powerpc/include/asm/fadump.h | 2 + > > arch/powerpc/kernel/fadump.c | 109 > > ++++++++++++++++++++++++++++++++++++-- > > arch/powerpc/kernel/prom.c | 7 +++ 3 files changed, 115 > > insertions(+), 3 deletions(-) > > > > diff --git a/arch/powerpc/include/asm/fadump.h > > b/arch/powerpc/include/asm/fadump.h index > > ce88bbe1d809..98ae00943fb3 100644 --- > > a/arch/powerpc/include/asm/fadump.h +++ > > b/arch/powerpc/include/asm/fadump.h @@ -208,11 +208,13 @@ extern > > int early_init_dt_scan_fw_dump(unsigned long node, const char > > *uname, int depth, void *data); extern int fadump_reserve_mem(void); > > extern int setup_fadump(void); > > +extern void enforce_fadump_extra_args(char *cmdline); > > extern int is_fadump_active(void); > > extern void crash_fadump(struct pt_regs *, const char *); > > extern void fadump_cleanup(void); > > > > #else /* CONFIG_FA_DUMP */ > > +static inline void enforce_fadump_extra_args(char *cmdline) { } > > static inline int is_fadump_active(void) { return 0; } > > static inline void crash_fadump(struct pt_regs *regs, const char > > *str) { } #endif > > diff --git a/arch/powerpc/kernel/fadump.c > > b/arch/powerpc/kernel/fadump.c index dc0c49cfd90a..a1614d9b8a21 > > 100644 --- a/arch/powerpc/kernel/fadump.c > > +++ b/arch/powerpc/kernel/fadump.c > > @@ -78,8 +78,10 @@ int __init early_init_dt_scan_fw_dump(unsigned > > long node, > > * dump data waiting for us. > > */ > > fdm_active = of_get_flat_dt_prop(node, "ibm,kernel-dump", > > NULL); > > - if (fdm_active) > > + if (fdm_active) { > > + pr_info("Firmware-assisted dump is active.\n"); > > fw_dump.dump_active = 1; > > + } > > > > /* Get the sizes required to store dump data for the > > firmware provided > > * dump sections. > > @@ -332,8 +334,11 @@ int __init fadump_reserve_mem(void) > > { > > unsigned long base, size, memory_boundary; > > > > - if (!fw_dump.fadump_enabled) > > + if (!fw_dump.fadump_enabled) { > > + if (fw_dump.dump_active) > > + pr_warn("Firmware-assisted dump was active > > but kernel booted with fadump disabled!\n"); return 0; > > + } > > > > if (!fw_dump.fadump_supported) { > > printk(KERN_INFO "Firmware-assisted dump is not > > supported on" @@ -373,7 +378,6 @@ int __init > > fadump_reserve_mem(void) memory_boundary = memblock_end_of_DRAM(); > > > > if (fw_dump.dump_active) { > > - printk(KERN_INFO "Firmware-assisted dump is > > active.\n"); /* > > * If last boot has crashed then reserve all the > > memory > > * above boot_memory_size so that we don't touch > > it until @@ -460,6 +464,105 @@ static int __init > > early_fadump_reserve_mem(char *p) } > > early_param("fadump_reserve_mem", early_fadump_reserve_mem); > > > > +#define FADUMP_EXTRA_ARGS_PARAM "fadump_extra_args=" > > +#define FADUMP_EXTRA_ARGS_LEN > > (strlen(FADUMP_EXTRA_ARGS_PARAM) - 1) + > > +struct param_info { > > + char *cmdline; > > + char *tmp_cmdline; > > + int shortening; > > +}; > > + > > +static void __init fadump_update_params(struct param_info > > *param_info, > > + char *param, char *val) > > +{ > > + ptrdiff_t param_offset = param - param_info->tmp_cmdline; > > + size_t vallen = val ? strlen(val) : 0; > > + char *tgt = param_info->cmdline + param_offset + > > + FADUMP_EXTRA_ARGS_LEN - param_info->shortening; > > + int shortening = 0; > > + > > + if (!val) > > + return; > > + > > + /* remove '=' */ > > + *tgt++ = ' '; > > + > > + /* next_arg removes one leading and one trailing '"' */ > > + if (*tgt == '"') > > + shortening += 1; > > + if (*(tgt + vallen + shortening) == '"') > > + shortening += 1; > > + > > + /* remove one leading and one trailing quote if both are > > present */ > > + if ((val[0] == '"') && (val[vallen - 1] == '"')) { > > + shortening += 2; > > + vallen -= 2; > > + val++; > > + } > > + > > + /* some characters were removed - move the trailing part > > of cmdline */ > > + if (shortening) { > > + char *src; > > + > > + strncpy(tgt, val, vallen); > > + tgt += vallen; > > + src = tgt + shortening; > > + memmove(tgt, src, strlen(src) + 1); > > + } > > + > > + param_info->shortening += shortening; > > +} > > + > > +/* > > + * Reworks command line parameters and splits 'fadump_extra_args=' > > param > > + * to enforce the parameters passed through it > > + */ > > +static int __init fadump_rework_cmdline_params(char *param, char > > *val, > > + const char *unused, > > void *arg) +{ > > + struct param_info *param_info = (struct param_info *)arg; > > + > > + if (strncmp(param, FADUMP_EXTRA_ARGS_PARAM, > > + strlen(FADUMP_EXTRA_ARGS_PARAM) - 1)) > > + return 0; > > + > > + fadump_update_params(param_info, param, val); > > + > > + return 0; > > +} > > + > > +/* > > + * Replace every occurrence of 'fadump_extra_args="param1 param2 > > param3"' > > + * in cmdline with 'fadump_extra_args param1 param2 param3' by > > stripping > > + * off '=' and quotes, if any. This ensures that the additional > > parameters > > + * passed with 'fadump_extra_args=' are enforced. > > + */ > > +void __init enforce_fadump_extra_args(char *cmdline) > > +{ > > + static char tmp_cmdline[COMMAND_LINE_SIZE] __initdata; > > + static char init_cmdline[COMMAND_LINE_SIZE] __initdata; > > + struct param_info param_info; > > + > > + if (strstr(cmdline, FADUMP_EXTRA_ARGS_PARAM) == NULL) > > + return; > > + > > + pr_info("Modifying command line to enforce the additional > > parameters passed through 'fadump_extra_args='"); + > > + param_info.cmdline = cmdline; > > + param_info.tmp_cmdline = tmp_cmdline; > > + param_info.shortening = 0; > > + > > + strlcpy(init_cmdline, cmdline, COMMAND_LINE_SIZE); > > + > > + strlcpy(tmp_cmdline, cmdline, COMMAND_LINE_SIZE); > > + parse_args("fadump params", tmp_cmdline, NULL, 0, 0, 0, > > + ¶m_info, > > &fadump_rework_cmdline_params); + > > + pr_info("Original command line: %s\n", init_cmdline); > > + pr_info("Modified command line: %s\n", cmdline); > > +} > > + > > static int register_fw_dump(struct fadump_mem_struct *fdm) > > { > > int rc, err; > > diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c > > index f83056297441..2e6f40217266 100644 > > --- a/arch/powerpc/kernel/prom.c > > +++ b/arch/powerpc/kernel/prom.c > > @@ -693,6 +693,13 @@ void __init early_init_devtree(void *params) > > of_scan_flat_dt(early_init_dt_scan_root, NULL); > > of_scan_flat_dt(early_init_dt_scan_memory_ppc, NULL); > > > > + /* > > + * Look for 'fadump_extra_args=' parameter and enfore the > > additional > > + * parameters passed to it if fadump is active. > > + */ > > + if (is_fadump_active()) > > + enforce_fadump_extra_args(boot_command_line); > > + > > parse_early_param(); > > > > /* make sure we've parsed cmdline for mem= before this > > */ > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v7 1/4] powerpc/fadump: reduce memory consumption for capture kernel 2017-08-18 11:57 ` Michal Suchánek @ 2017-08-18 14:00 ` Hari Bathini 0 siblings, 0 replies; 14+ messages in thread From: Hari Bathini @ 2017-08-18 14:00 UTC (permalink / raw) To: Michal Suchánek Cc: linux-doc, Lokesh Vutla, Viresh Kumar, David Howells, Sylvain 'ythier' Hitier, Paul Mackerras, Ingo Molnar, Michael Neuling, Baoquan He, Jonathan Corbet, Ilya Matveychikov, Mahesh J Salgaonkar, Thiago Jung Bauermann, Kees Cook, Rusty Russell, Nicholas Piggin, Scott Wood, Jason Baron, Steven Rostedt,, Thomas Gleixner, Daniel Axtens, linux-kernel, Aneesh Kumar K.V, Jessica Yu, Tejun Heo, Oliver O'Halloran, Andrew Morton, linuxppc-dev On Friday 18 August 2017 05:27 PM, Michal Suchánek wrote: > On Fri, 18 Aug 2017 16:20:53 +0530 > Hari Bathini <hbathini@linux.vnet.ibm.com> wrote: > >> Hi Michal, >> >> >> Thanks for the patches. I tried testing with the patches: >> >> [ 0.000000] fadump: Firmware-assisted dump is active. >> [ 0.000000] fadump: Modifying command line to enforce the >> additional parameters passed through 'fadump_extra_args=' >> [ 0.000000] fadump: Original command line: >> BOOT_IMAGE=/vmlinux-4.13.0-rc1-bz155783+ root=/dev/mapper/rhel-root >> ro crashkernel=2048M fadump=on fadump_reserve_mem=1024M >> "fadump_extra_args=nr_cpus=1 numa=off udev.childern-max=2" >> rd.lvm.lv=rhel/root rd.lvm.lv=rhel/swap >> [ 0.000000] fadump: Modified command line: >> BOOT_IMAGE=/vmlinux-4.13.0-rc1-bz155783+ root=/dev/mapper/rhel-root >> ro crashkernel=2048M fadump=on fadump_reserve_mem=1024M >> "fadump_extra_args nr_cpus=1 numa=off udev.childern-max=2" >> rd.lvm.lv=rhel/root rd.lvm.lv=rhel/swap >> >> Looks like the quotes are retained not enforcing the parameters... > Hello, > > You are passing an argument >>"fadump_extra_args<< - that is an > argument containing a quote in the name. I did not test this scenario Actually, this was not intentional.. I passed fadump_extra_args="nr_cpus=1 numa=off udev.childern-max=2" through grub loader but chosen/bootargs ended up having "fadump_extra_args=nr_cpus=1 numa=off udev.childern-max=2". Need to check why that is.. Thanks Hari > assuming the argument name would not match in this case. It would > probably not match if the quote was in the middle of the argument > name but at the start it is skipped. Note that due to the requirement > to remove quotes symmetrically which is added in the third patch this > case does not break the commandline - it merely makes the arguments > ineffective. > > The format suggested in the documentation is >>> fadump_extra_args="nr_cpus=1 numa=off udev.childern-max=2"<< - that > is quotes around the value. This format worked in my testing. > > Unfortunately, the format with quote before argument name would > probably require another extra parameter to the callback to detect > properly. > > Thanks > > Michal > >> I am yet to test the patches in other scenarios though.. >> >> >> Thanks >> >> Hari >> >> >> On Friday 18 August 2017 01:44 AM, Michal Suchanek wrote: >>> From: Hari Bathini <hbathini@linux.vnet.ibm.com> >>> >>> With fadump (dump capture) kernel booting like a regular kernel, it >>> needs almost the same amount of memory to boot as the production >>> kernel, which is unwarranted for a dump capture kernel. But with no >>> option to disable some of the unnecessary subsystems in fadump >>> kernel, that much memory is wasted on fadump, depriving the >>> production kernel of that memory. >>> >>> Introduce kernel parameter 'fadump_extra_args=' that would take >>> regular parameters as a space separated quoted string, to be >>> enforced when fadump is active. This 'fadump_extra_args=' parameter >>> can be leveraged to pass parameters like nr_cpus=1, >>> cgroup_disable=memory and numa=off, to disable unwarranted >>> resources/subsystems. >>> >>> Also, ensure the log "Firmware-assisted dump is active" is printed >>> early in the boot process to put the subsequent fadump messages in >>> context. >>> >>> Suggested-by: Michael Ellerman <mpe@ellerman.id.au> >>> Signed-off-by: Hari Bathini <hbathini@linux.vnet.ibm.com> >>> Signed-off-by: Michal Suchanek <msuchanek@suse.de> >>> --- >>> Changes from v6: >>> Correct and simplify quote handling. Ideally I would like to extend >>> parse_args to give the length of the original quoted value to >>> callback. However, parse_args removes at most one doubel-quote from >>> the start and one from the end so that is easy to detect. Otherwise >>> all other users will have to be updated to trash the new argument. >>> --- >>> arch/powerpc/include/asm/fadump.h | 2 + >>> arch/powerpc/kernel/fadump.c | 109 >>> ++++++++++++++++++++++++++++++++++++-- >>> arch/powerpc/kernel/prom.c | 7 +++ 3 files changed, 115 >>> insertions(+), 3 deletions(-) >>> >>> diff --git a/arch/powerpc/include/asm/fadump.h >>> b/arch/powerpc/include/asm/fadump.h index >>> ce88bbe1d809..98ae00943fb3 100644 --- >>> a/arch/powerpc/include/asm/fadump.h +++ >>> b/arch/powerpc/include/asm/fadump.h @@ -208,11 +208,13 @@ extern >>> int early_init_dt_scan_fw_dump(unsigned long node, const char >>> *uname, int depth, void *data); extern int fadump_reserve_mem(void); >>> extern int setup_fadump(void); >>> +extern void enforce_fadump_extra_args(char *cmdline); >>> extern int is_fadump_active(void); >>> extern void crash_fadump(struct pt_regs *, const char *); >>> extern void fadump_cleanup(void); >>> >>> #else /* CONFIG_FA_DUMP */ >>> +static inline void enforce_fadump_extra_args(char *cmdline) { } >>> static inline int is_fadump_active(void) { return 0; } >>> static inline void crash_fadump(struct pt_regs *regs, const char >>> *str) { } #endif >>> diff --git a/arch/powerpc/kernel/fadump.c >>> b/arch/powerpc/kernel/fadump.c index dc0c49cfd90a..a1614d9b8a21 >>> 100644 --- a/arch/powerpc/kernel/fadump.c >>> +++ b/arch/powerpc/kernel/fadump.c >>> @@ -78,8 +78,10 @@ int __init early_init_dt_scan_fw_dump(unsigned >>> long node, >>> * dump data waiting for us. >>> */ >>> fdm_active = of_get_flat_dt_prop(node, "ibm,kernel-dump", >>> NULL); >>> - if (fdm_active) >>> + if (fdm_active) { >>> + pr_info("Firmware-assisted dump is active.\n"); >>> fw_dump.dump_active = 1; >>> + } >>> >>> /* Get the sizes required to store dump data for the >>> firmware provided >>> * dump sections. >>> @@ -332,8 +334,11 @@ int __init fadump_reserve_mem(void) >>> { >>> unsigned long base, size, memory_boundary; >>> >>> - if (!fw_dump.fadump_enabled) >>> + if (!fw_dump.fadump_enabled) { >>> + if (fw_dump.dump_active) >>> + pr_warn("Firmware-assisted dump was active >>> but kernel booted with fadump disabled!\n"); return 0; >>> + } >>> >>> if (!fw_dump.fadump_supported) { >>> printk(KERN_INFO "Firmware-assisted dump is not >>> supported on" @@ -373,7 +378,6 @@ int __init >>> fadump_reserve_mem(void) memory_boundary = memblock_end_of_DRAM(); >>> >>> if (fw_dump.dump_active) { >>> - printk(KERN_INFO "Firmware-assisted dump is >>> active.\n"); /* >>> * If last boot has crashed then reserve all the >>> memory >>> * above boot_memory_size so that we don't touch >>> it until @@ -460,6 +464,105 @@ static int __init >>> early_fadump_reserve_mem(char *p) } >>> early_param("fadump_reserve_mem", early_fadump_reserve_mem); >>> >>> +#define FADUMP_EXTRA_ARGS_PARAM "fadump_extra_args=" >>> +#define FADUMP_EXTRA_ARGS_LEN >>> (strlen(FADUMP_EXTRA_ARGS_PARAM) - 1) + >>> +struct param_info { >>> + char *cmdline; >>> + char *tmp_cmdline; >>> + int shortening; >>> +}; >>> + >>> +static void __init fadump_update_params(struct param_info >>> *param_info, >>> + char *param, char *val) >>> +{ >>> + ptrdiff_t param_offset = param - param_info->tmp_cmdline; >>> + size_t vallen = val ? strlen(val) : 0; >>> + char *tgt = param_info->cmdline + param_offset + >>> + FADUMP_EXTRA_ARGS_LEN - param_info->shortening; >>> + int shortening = 0; >>> + >>> + if (!val) >>> + return; >>> + >>> + /* remove '=' */ >>> + *tgt++ = ' '; >>> + >>> + /* next_arg removes one leading and one trailing '"' */ >>> + if (*tgt == '"') >>> + shortening += 1; >>> + if (*(tgt + vallen + shortening) == '"') >>> + shortening += 1; >>> + >>> + /* remove one leading and one trailing quote if both are >>> present */ >>> + if ((val[0] == '"') && (val[vallen - 1] == '"')) { >>> + shortening += 2; >>> + vallen -= 2; >>> + val++; >>> + } >>> + >>> + /* some characters were removed - move the trailing part >>> of cmdline */ >>> + if (shortening) { >>> + char *src; >>> + >>> + strncpy(tgt, val, vallen); >>> + tgt += vallen; >>> + src = tgt + shortening; >>> + memmove(tgt, src, strlen(src) + 1); >>> + } >>> + >>> + param_info->shortening += shortening; >>> +} >>> + >>> +/* >>> + * Reworks command line parameters and splits 'fadump_extra_args=' >>> param >>> + * to enforce the parameters passed through it >>> + */ >>> +static int __init fadump_rework_cmdline_params(char *param, char >>> *val, >>> + const char *unused, >>> void *arg) +{ >>> + struct param_info *param_info = (struct param_info *)arg; >>> + >>> + if (strncmp(param, FADUMP_EXTRA_ARGS_PARAM, >>> + strlen(FADUMP_EXTRA_ARGS_PARAM) - 1)) >>> + return 0; >>> + >>> + fadump_update_params(param_info, param, val); >>> + >>> + return 0; >>> +} >>> + >>> +/* >>> + * Replace every occurrence of 'fadump_extra_args="param1 param2 >>> param3"' >>> + * in cmdline with 'fadump_extra_args param1 param2 param3' by >>> stripping >>> + * off '=' and quotes, if any. This ensures that the additional >>> parameters >>> + * passed with 'fadump_extra_args=' are enforced. >>> + */ >>> +void __init enforce_fadump_extra_args(char *cmdline) >>> +{ >>> + static char tmp_cmdline[COMMAND_LINE_SIZE] __initdata; >>> + static char init_cmdline[COMMAND_LINE_SIZE] __initdata; >>> + struct param_info param_info; >>> + >>> + if (strstr(cmdline, FADUMP_EXTRA_ARGS_PARAM) == NULL) >>> + return; >>> + >>> + pr_info("Modifying command line to enforce the additional >>> parameters passed through 'fadump_extra_args='"); + >>> + param_info.cmdline = cmdline; >>> + param_info.tmp_cmdline = tmp_cmdline; >>> + param_info.shortening = 0; >>> + >>> + strlcpy(init_cmdline, cmdline, COMMAND_LINE_SIZE); >>> + >>> + strlcpy(tmp_cmdline, cmdline, COMMAND_LINE_SIZE); >>> + parse_args("fadump params", tmp_cmdline, NULL, 0, 0, 0, >>> + ¶m_info, >>> &fadump_rework_cmdline_params); + >>> + pr_info("Original command line: %s\n", init_cmdline); >>> + pr_info("Modified command line: %s\n", cmdline); >>> +} >>> + >>> static int register_fw_dump(struct fadump_mem_struct *fdm) >>> { >>> int rc, err; >>> diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c >>> index f83056297441..2e6f40217266 100644 >>> --- a/arch/powerpc/kernel/prom.c >>> +++ b/arch/powerpc/kernel/prom.c >>> @@ -693,6 +693,13 @@ void __init early_init_devtree(void *params) >>> of_scan_flat_dt(early_init_dt_scan_root, NULL); >>> of_scan_flat_dt(early_init_dt_scan_memory_ppc, NULL); >>> >>> + /* >>> + * Look for 'fadump_extra_args=' parameter and enfore the >>> additional >>> + * parameters passed to it if fadump is active. >>> + */ >>> + if (is_fadump_active()) >>> + enforce_fadump_extra_args(boot_command_line); >>> + >>> parse_early_param(); >>> >>> /* make sure we've parsed cmdline for mem= before this >>> */ ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2017-08-24 12:17 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-08-17 20:14 [PATCH v7 1/4] powerpc/fadump: reduce memory consumption for capture kernel Michal Suchanek 2017-08-17 20:14 ` [PATCH v7 2/4] powerpc/fadump: update documentation about 'fadump_extra_args=' parameter Michal Suchanek 2017-08-17 20:14 ` [PATCH v7 3/4] lib/cmdline.c Remove quotes symmetrically Michal Suchanek 2017-08-17 21:27 ` msuchanek 2017-08-21 10:27 ` Baoquan He 2017-08-21 11:30 ` Michal Suchánek 2017-08-24 11:02 ` Michael Ellerman 2017-08-24 12:15 ` msuchanek 2017-08-17 20:14 ` [PATCH v7 4/4] boot/param: add pointer to next argument to unknown parameter callback Michal Suchanek 2017-08-24 11:04 ` Michael Ellerman 2017-08-24 12:17 ` msuchanek 2017-08-18 10:50 ` [PATCH v7 1/4] powerpc/fadump: reduce memory consumption for capture kernel Hari Bathini 2017-08-18 11:57 ` Michal Suchánek 2017-08-18 14:00 ` Hari Bathini
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).