Message ID | 149383576571.1694.13692135068122879322.stgit@hbathini.in.ibm.com (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Hello, On Wed, 03 May 2017 23:52:52 +0530 Hari Bathini <hbathini@linux.vnet.ibm.com> wrote: > With the introduction of 'fadump_append=' parameter to pass additional > parameters to fadump (capture) kernel, update documentation about it. > > Signed-off-by: Hari Bathini <hbathini@linux.vnet.ibm.com> > --- > > Changes from v4: > * Based on top of patchset that includes > https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?h=akpm&id=05f383cdfba8793240e73f9a9fbff4e25d66003f > > > Documentation/powerpc/firmware-assisted-dump.txt | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/Documentation/powerpc/firmware-assisted-dump.txt > b/Documentation/powerpc/firmware-assisted-dump.txt index > 8394bc8..6327193 100644 --- > a/Documentation/powerpc/firmware-assisted-dump.txt +++ > b/Documentation/powerpc/firmware-assisted-dump.txt @@ -162,7 +162,15 > @@ 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 comma > + separated list through 'fadump_append=' parameter, to be enforced > + when fadump is active. For example, if parameters like nr_cpus=1, > + numa=off & udev.children-max=2 are to be enforced when fadump is > + active, 'fadump_append=nr_cpus=1,numa=off,udev.children-max=2' > + can be passed in command line, which will be replaced with > + "nr_cpus=1 numa=off udev.children-max=2" when fadump is active. > + This helps in reducing memory consumption during dump capture. > +4. Optionally, user can also set 'crashkernel=' kernel cmdline > to specify size of the memory to reserve for boot memory dump > preservation. > > Writing your own deficient parser for comma separated arguments when perfectly fine parser for space separated quoted arguments exists in the kernel and the bootloader does not seem like a good idea to me. I also do not see how this addresses On Wed, 26 Apr 2017 20:32:55 +1000 Michael Ellerman <mpe@ellerman.id.au> wrote: > Hari Bathini <hbathini@linux.vnet.ibm.com> writes: > > > diff --git a/arch/powerpc/kernel/fadump.c > > b/arch/powerpc/kernel/fadump.c index 8ff0dd4..87edc7b 100644 > > --- a/arch/powerpc/kernel/fadump.c > > +++ b/arch/powerpc/kernel/fadump.c > > @@ -338,6 +343,36 @@ unsigned long __init > > arch_reserved_kernel_pages(void) return memblock_reserved_size() / > > PAGE_SIZE; } > > > > +static void __init parse_fadump_append_params(const char *p) > > +{ > > + static char fadump_cmdline[COMMAND_LINE_SIZE] __initdata; > > + char *token; > > + > > + strlcpy(fadump_cmdline, p, COMMAND_LINE_SIZE); > > + token = strchr(fadump_cmdline, ','); > > + while (token) { > > + *token = ' '; > > + token = strchr(token, ','); > > + } > > + > > + pr_info("enforcing additional parameters (%s) passed > > through " > > + "'fadump_append=' parameter\n", fadump_cmdline); > > + parse_early_options(fadump_cmdline); > > Using parse_early_options() means it only works for parameters defined > with early_param() or __setup() doesn't it? > > That might be OK but at least you need to clearly document it. which was your gripe with v4 of the patchset. Unfortunately the flags in Documentation/kernel-parameters.txt do not include a flag that denotes parameters available with parse_early_options - unless it is the KNL flag and is quite bitrotten. Thanks Michal
Hello Michal, On Wednesday 10 May 2017 09:31 PM, Michal Suchánek wrote: > Hello, > > On Wed, 03 May 2017 23:52:52 +0530 > Hari Bathini <hbathini@linux.vnet.ibm.com> wrote: > >> With the introduction of 'fadump_append=' parameter to pass additional >> parameters to fadump (capture) kernel, update documentation about it. >> >> Signed-off-by: Hari Bathini <hbathini@linux.vnet.ibm.com> >> --- >> >> Changes from v4: >> * Based on top of patchset that includes >> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?h=akpm&id=05f383cdfba8793240e73f9a9fbff4e25d66003f >> >> >> Documentation/powerpc/firmware-assisted-dump.txt | 10 +++++++++- >> 1 file changed, 9 insertions(+), 1 deletion(-) >> >> diff --git a/Documentation/powerpc/firmware-assisted-dump.txt >> b/Documentation/powerpc/firmware-assisted-dump.txt index >> 8394bc8..6327193 100644 --- >> a/Documentation/powerpc/firmware-assisted-dump.txt +++ >> b/Documentation/powerpc/firmware-assisted-dump.txt @@ -162,7 +162,15 >> @@ 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 comma >> + separated list through 'fadump_append=' parameter, to be enforced >> + when fadump is active. For example, if parameters like nr_cpus=1, >> + numa=off & udev.children-max=2 are to be enforced when fadump is >> + active, 'fadump_append=nr_cpus=1,numa=off,udev.children-max=2' >> + can be passed in command line, which will be replaced with >> + "nr_cpus=1 numa=off udev.children-max=2" when fadump is active. >> + This helps in reducing memory consumption during dump capture. >> +4. Optionally, user can also set 'crashkernel=' kernel cmdline >> to specify size of the memory to reserve for boot memory dump >> preservation. >> >> > Writing your own deficient parser for comma separated arguments when > perfectly fine parser for space separated quoted arguments exists in > the kernel and the bootloader does not seem like a good idea to me. Couple of things that prompted me for v5 are: 1. Using parse_early_options() limits the kind of parameters that can be passed to fadump capture kernel. Passing parameters like systemd.unit= & udev.childern.max= has no effect with v4. Updating boot_command_line parameter, when fadump is active, seems a better alternative. 2. Passing space-separated quoted arguments is not working as intended with lilo. Updating bootloader with the below entry in /etc/lilo.conf file results in a missing append entry in /etc/yaboot.conf file. append = "quiet sysrq=1 insmod=sym53c8xx insmod=ipr crashkernel=512M-:256M fadump_append=\"nr_cpus=1 numa=off udev.children-max=2\"" Because lilo doesn't seem to handle quoted arguments as intended, comma-separated list syntax was the obvious choice. Quoted arguments for a comma-separated list sounds plausible though, provided the bootloader can handle it. Will try and address that.. > I also do not see how this addresses > > On Wed, 26 Apr 2017 20:32:55 +1000 > Michael Ellerman <mpe@ellerman.id.au> wrote: > >> Hari Bathini <hbathini@linux.vnet.ibm.com> writes: >> >>> diff --git a/arch/powerpc/kernel/fadump.c >>> b/arch/powerpc/kernel/fadump.c index 8ff0dd4..87edc7b 100644 >>> --- a/arch/powerpc/kernel/fadump.c >>> +++ b/arch/powerpc/kernel/fadump.c >>> @@ -338,6 +343,36 @@ unsigned long __init >>> arch_reserved_kernel_pages(void) return memblock_reserved_size() / >>> PAGE_SIZE; } >>> >>> +static void __init parse_fadump_append_params(const char *p) >>> +{ >>> + static char fadump_cmdline[COMMAND_LINE_SIZE] __initdata; >>> + char *token; >>> + >>> + strlcpy(fadump_cmdline, p, COMMAND_LINE_SIZE); >>> + token = strchr(fadump_cmdline, ','); >>> + while (token) { >>> + *token = ' '; >>> + token = strchr(token, ','); >>> + } >>> + >>> + pr_info("enforcing additional parameters (%s) passed >>> through " >>> + "'fadump_append=' parameter\n", fadump_cmdline); >>> + parse_early_options(fadump_cmdline); >> Using parse_early_options() means it only works for parameters defined >> with early_param() or __setup() doesn't it? >> >> That might be OK but at least you need to clearly document it. > which was your gripe with v4 of the patchset. Unfortunately the flags > in Documentation/kernel-parameters.txt do not include a flag that > denotes parameters available with parse_early_options - unless it is > the KNL flag and is quite bitrotten. > > The addtional comments Michael was asking for was when fadump_append= is limited to early parsing only. But with this approach, we don't have such limitation. Though I do agree that there is always scope to improve the documentation further.. Thanks Hari
On Thu, 11 May 2017 02:00:11 +0530 Hari Bathini <hbathini@linux.vnet.ibm.com> wrote: > Hello Michal, > > On Wednesday 10 May 2017 09:31 PM, Michal Suchánek wrote: > > Hello, > > > > On Wed, 03 May 2017 23:52:52 +0530 > > Hari Bathini <hbathini@linux.vnet.ibm.com> wrote: > > > >> With the introduction of 'fadump_append=' parameter to pass > >> additional parameters to fadump (capture) kernel, update > >> documentation about it. > >> > >> Signed-off-by: Hari Bathini <hbathini@linux.vnet.ibm.com> > >> --- > >> > >> Changes from v4: > >> * Based on top of patchset that includes > >> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?h=akpm&id=05f383cdfba8793240e73f9a9fbff4e25d66003f > >> > >> > >> Documentation/powerpc/firmware-assisted-dump.txt | 10 > >> +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) > >> > >> diff --git a/Documentation/powerpc/firmware-assisted-dump.txt > >> b/Documentation/powerpc/firmware-assisted-dump.txt index > >> 8394bc8..6327193 100644 --- > >> a/Documentation/powerpc/firmware-assisted-dump.txt +++ > >> b/Documentation/powerpc/firmware-assisted-dump.txt @@ -162,7 > >> +162,15 @@ 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 comma > >> + separated list through 'fadump_append=' parameter, to be > >> enforced > >> + when fadump is active. For example, if parameters like > >> nr_cpus=1, > >> + numa=off & udev.children-max=2 are to be enforced when fadump > >> is > >> + active, 'fadump_append=nr_cpus=1,numa=off,udev.children-max=2' > >> + can be passed in command line, which will be replaced with > >> + "nr_cpus=1 numa=off udev.children-max=2" when fadump is active. > >> + This helps in reducing memory consumption during dump capture. > >> +4. Optionally, user can also set 'crashkernel=' kernel cmdline > >> to specify size of the memory to reserve for boot memory dump > >> preservation. > >> > >> > > Writing your own deficient parser for comma separated arguments when > > perfectly fine parser for space separated quoted arguments exists in > > the kernel and the bootloader does not seem like a good idea to > > me. > > > Couple of things that prompted me for v5 are: > 1. Using parse_early_options() limits the kind of parameters > that can be passed to fadump capture kernel. Passing parameters > like systemd.unit= & udev.childern.max= has no effect with v4. > Updating > boot_command_line parameter, when fadump is active, seems a > better alternative. > > 2. Passing space-separated quoted arguments is not working > as intended with lilo. Updating bootloader with the below > entry in /etc/lilo.conf file results in a missing append > entry in /etc/yaboot.conf file. > > append = "quiet sysrq=1 insmod=sym53c8xx insmod=ipr > crashkernel=512M-:256M fadump_append=\"nr_cpus=1 numa=off > udev.children-max=2\"" Meaning that a script that emulates LILO semantics on top of yaboot which is completely capable of passing qouted space separated arguments fails. IMHO it is more reasonable to fix the script or whatever adaptation layer or use yaboot directly than working around bug in said script by introducing a new argument parser in the kernel. Thanks Michal
On Thu, 11 May 2017 02:00:11 +0530 Hari Bathini <hbathini@linux.vnet.ibm.com> wrote: > Hello Michal, > > On Wednesday 10 May 2017 09:31 PM, Michal Suchánek wrote: > > Hello, > > > > On Wed, 03 May 2017 23:52:52 +0530 > > Hari Bathini <hbathini@linux.vnet.ibm.com> wrote: > > > >> With the introduction of 'fadump_append=' parameter to pass > >> additional parameters to fadump (capture) kernel, update > >> documentation about it. > >> > >> Signed-off-by: Hari Bathini <hbathini@linux.vnet.ibm.com> > >> --- > >> > >> Changes from v4: > >> * Based on top of patchset that includes > >> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?h=akpm&id=05f383cdfba8793240e73f9a9fbff4e25d66003f > >> > >> > >> Documentation/powerpc/firmware-assisted-dump.txt | 10 > >> +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) > >> > >> diff --git a/Documentation/powerpc/firmware-assisted-dump.txt > >> b/Documentation/powerpc/firmware-assisted-dump.txt index > >> 8394bc8..6327193 100644 --- > >> a/Documentation/powerpc/firmware-assisted-dump.txt +++ > >> b/Documentation/powerpc/firmware-assisted-dump.txt @@ -162,7 > >> +162,15 @@ 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 comma > >> + separated list through 'fadump_append=' parameter, to be > >> enforced > >> + when fadump is active. For example, if parameters like > >> nr_cpus=1, > >> + numa=off & udev.children-max=2 are to be enforced when fadump > >> is > >> + active, 'fadump_append=nr_cpus=1,numa=off,udev.children-max=2' > >> + can be passed in command line, which will be replaced with > >> + "nr_cpus=1 numa=off udev.children-max=2" when fadump is active. > >> + This helps in reducing memory consumption during dump capture. > >> +4. Optionally, user can also set 'crashkernel=' kernel cmdline > >> to specify size of the memory to reserve for boot memory dump > >> preservation. > >> > >> > > Writing your own deficient parser for comma separated arguments when > > perfectly fine parser for space separated quoted arguments exists in > > the kernel and the bootloader does not seem like a good idea to > > me. > > > Couple of things that prompted me for v5 are: > 1. Using parse_early_options() limits the kind of parameters > that can be passed to fadump capture kernel. Passing parameters > like systemd.unit= & udev.childern.max= has no effect with v4. > Updating > boot_command_line parameter, when fadump is active, seems a > better alternative. > > 2. Passing space-separated quoted arguments is not working > as intended with lilo. Updating bootloader with the below > entry in /etc/lilo.conf file results in a missing append > entry in /etc/yaboot.conf file. > > append = "quiet sysrq=1 insmod=sym53c8xx insmod=ipr > crashkernel=512M-:256M fadump_append=\"nr_cpus=1 numa=off > udev.children-max=2\"" Meaning that a script that emulates LILO semantics on top of yaboot which is completely capable of passing qouted space separated arguments fails. IMHO it is more reasonable to fix the script or whatever adaptation layer or use yaboot directly than working around bug in said script by introducing a new argument parser in the kernel. Thanks Michal
On Thursday 11 May 2017 06:46 PM, Michal Suchánek wrote: > On Thu, 11 May 2017 02:00:11 +0530 > Hari Bathini <hbathini@linux.vnet.ibm.com> wrote: > >> Hello Michal, >> >> On Wednesday 10 May 2017 09:31 PM, Michal Suchánek wrote: >>> Hello, >>> >>> On Wed, 03 May 2017 23:52:52 +0530 >>> Hari Bathini <hbathini@linux.vnet.ibm.com> wrote: >>> >>>> With the introduction of 'fadump_append=' parameter to pass >>>> additional parameters to fadump (capture) kernel, update >>>> documentation about it. >>>> >>>> Signed-off-by: Hari Bathini <hbathini@linux.vnet.ibm.com> >>>> --- >>>> >>>> Changes from v4: >>>> * Based on top of patchset that includes >>>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?h=akpm&id=05f383cdfba8793240e73f9a9fbff4e25d66003f >>>> >>>> >>>> Documentation/powerpc/firmware-assisted-dump.txt | 10 >>>> +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/Documentation/powerpc/firmware-assisted-dump.txt >>>> b/Documentation/powerpc/firmware-assisted-dump.txt index >>>> 8394bc8..6327193 100644 --- >>>> a/Documentation/powerpc/firmware-assisted-dump.txt +++ >>>> b/Documentation/powerpc/firmware-assisted-dump.txt @@ -162,7 >>>> +162,15 @@ 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 comma >>>> + separated list through 'fadump_append=' parameter, to be >>>> enforced >>>> + when fadump is active. For example, if parameters like >>>> nr_cpus=1, >>>> + numa=off & udev.children-max=2 are to be enforced when fadump >>>> is >>>> + active, 'fadump_append=nr_cpus=1,numa=off,udev.children-max=2' >>>> + can be passed in command line, which will be replaced with >>>> + "nr_cpus=1 numa=off udev.children-max=2" when fadump is active. >>>> + This helps in reducing memory consumption during dump capture. >>>> +4. Optionally, user can also set 'crashkernel=' kernel cmdline >>>> to specify size of the memory to reserve for boot memory dump >>>> preservation. >>>> >>>> >>> Writing your own deficient parser for comma separated arguments when >>> perfectly fine parser for space separated quoted arguments exists in >>> the kernel and the bootloader does not seem like a good idea to >>> me. >> >> Couple of things that prompted me for v5 are: >> 1. Using parse_early_options() limits the kind of parameters >> that can be passed to fadump capture kernel. Passing parameters >> like systemd.unit= & udev.childern.max= has no effect with v4. >> Updating >> boot_command_line parameter, when fadump is active, seems a >> better alternative. >> >> 2. Passing space-separated quoted arguments is not working >> as intended with lilo. Updating bootloader with the below >> entry in /etc/lilo.conf file results in a missing append >> entry in /etc/yaboot.conf file. >> >> append = "quiet sysrq=1 insmod=sym53c8xx insmod=ipr >> crashkernel=512M-:256M fadump_append=\"nr_cpus=1 numa=off >> udev.children-max=2\"" > Meaning that a script that emulates LILO semantics on top of yaboot > which is completely capable of passing qouted space separated arguments > fails. IMHO it is more reasonable to fix the script or whatever > adaptation layer or use yaboot directly than working around bug in said > script by introducing a new argument parser in the kernel. > > Hmmm.. while trying to implement space-separated parameter list with quotes as syntax for fadump_append parameter, noticed that it can make implemenation more vulnerable. Here are some problems I am facing while implementing this.. As the boot_command_line needs to be updated before parse_early_param, we still need some kind of parser. Now this parser has to look for quotes. But grub2 is updating fadump_append=\"nr_cpus=1 numa=off\" in /etc/default/grub to "fadump_append=nr_cpus=1 numa=off" cmdline. This may make parsing complicated in cases where only a single parameter is passed without quotes like fadump_append=nr_cpus=1. It can get more complicated, if there is some other parameter after fadump_append with quotes like below: fadump_append=nr_cpus=1 paramx paramy "another_param=something with spaces" Can't say the same about a comma-separated parameter list. Though I won't dare say it is the best way but maybe the lesser of two evils.. Thanks Hari
On Fri, 12 May 2017 15:15:33 +0530 Hari Bathini <hbathini@linux.vnet.ibm.com> wrote: > On Thursday 11 May 2017 06:46 PM, Michal Suchánek wrote: > > On Thu, 11 May 2017 02:00:11 +0530 > > Hari Bathini <hbathini@linux.vnet.ibm.com> wrote: > > > >> Hello Michal, > >> > >> On Wednesday 10 May 2017 09:31 PM, Michal Suchánek wrote: > >>> Hello, > >>> > >>> On Wed, 03 May 2017 23:52:52 +0530 > >>> Hari Bathini <hbathini@linux.vnet.ibm.com> wrote: > >>> > >>>> With the introduction of 'fadump_append=' parameter to pass > >>>> additional parameters to fadump (capture) kernel, update > >>>> documentation about it. > >>>> > >>>> Signed-off-by: Hari Bathini <hbathini@linux.vnet.ibm.com> > >>>> --- > >>>> > >>>> Changes from v4: > >>>> * Based on top of patchset that includes > >>>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?h=akpm&id=05f383cdfba8793240e73f9a9fbff4e25d66003f > >>>> > >>>> > >>>> Documentation/powerpc/firmware-assisted-dump.txt | 10 > >>>> +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) > >>>> > >>>> diff --git a/Documentation/powerpc/firmware-assisted-dump.txt > >>>> b/Documentation/powerpc/firmware-assisted-dump.txt index > >>>> 8394bc8..6327193 100644 --- > >>>> a/Documentation/powerpc/firmware-assisted-dump.txt +++ > >>>> b/Documentation/powerpc/firmware-assisted-dump.txt @@ -162,7 > >>>> +162,15 @@ 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 comma > >>>> + separated list through 'fadump_append=' parameter, to be > >>>> enforced > >>>> + when fadump is active. For example, if parameters like > >>>> nr_cpus=1, > >>>> + numa=off & udev.children-max=2 are to be enforced when fadump > >>>> is > >>>> + active, > >>>> 'fadump_append=nr_cpus=1,numa=off,udev.children-max=2' > >>>> + can be passed in command line, which will be replaced with > >>>> + "nr_cpus=1 numa=off udev.children-max=2" when fadump is > >>>> active. > >>>> + This helps in reducing memory consumption during dump > >>>> capture. +4. Optionally, user can also set 'crashkernel=' kernel > >>>> cmdline to specify size of the memory to reserve for boot memory > >>>> dump preservation. > >>>> > >>>> > >>> Writing your own deficient parser for comma separated arguments > >>> when perfectly fine parser for space separated quoted arguments > >>> exists in the kernel and the bootloader does not seem like a good > >>> idea to me. > >> > >> Couple of things that prompted me for v5 are: > >> 1. Using parse_early_options() limits the kind of parameters > >> that can be passed to fadump capture kernel. Passing > >> parameters like systemd.unit= & udev.childern.max= has no effect > >> with v4. Updating > >> boot_command_line parameter, when fadump is active, seems a > >> better alternative. > >> > >> 2. Passing space-separated quoted arguments is not working > >> as intended with lilo. Updating bootloader with the below > >> entry in /etc/lilo.conf file results in a missing append > >> entry in /etc/yaboot.conf file. > >> > >> append = "quiet sysrq=1 insmod=sym53c8xx insmod=ipr > >> crashkernel=512M-:256M fadump_append=\"nr_cpus=1 numa=off > >> udev.children-max=2\"" > > Meaning that a script that emulates LILO semantics on top of yaboot > > which is completely capable of passing qouted space separated > > arguments fails. IMHO it is more reasonable to fix the script or > > whatever adaptation layer or use yaboot directly than working > > around bug in said script by introducing a new argument parser in > > the kernel. > > > > > > Hmmm.. while trying to implement space-separated parameter list with > quotes as syntax for fadump_append parameter, noticed that it can > make implemenation > more vulnerable. Here are some problems I am facing while > implementing this.. How so? presumably you can reuse parse_args even if you do not register with early_param and call it yourself. Then your parsing of fadump_append is as vulnerable as parsing any other kernel argument since it is using same parser. Thanks Michal
On Friday 12 May 2017 09:12 PM, Michal Suchánek wrote: > On Fri, 12 May 2017 15:15:33 +0530 > Hari Bathini <hbathini@linux.vnet.ibm.com> wrote: > >> On Thursday 11 May 2017 06:46 PM, Michal Suchánek wrote: >>> On Thu, 11 May 2017 02:00:11 +0530 >>> Hari Bathini <hbathini@linux.vnet.ibm.com> wrote: >>> >>>> Hello Michal, >>>> >>>> On Wednesday 10 May 2017 09:31 PM, Michal Suchánek wrote: >>>>> Hello, >>>>> >>>>> On Wed, 03 May 2017 23:52:52 +0530 >>>>> Hari Bathini <hbathini@linux.vnet.ibm.com> wrote: >>>>> >>>>>> With the introduction of 'fadump_append=' parameter to pass >>>>>> additional parameters to fadump (capture) kernel, update >>>>>> documentation about it. >>>>>> >>>>>> Signed-off-by: Hari Bathini <hbathini@linux.vnet.ibm.com> >>>>>> --- >>>>>> >>>>>> Changes from v4: >>>>>> * Based on top of patchset that includes >>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?h=akpm&id=05f383cdfba8793240e73f9a9fbff4e25d66003f >>>>>> >>>>>> >>>>>> Documentation/powerpc/firmware-assisted-dump.txt | 10 >>>>>> +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/Documentation/powerpc/firmware-assisted-dump.txt >>>>>> b/Documentation/powerpc/firmware-assisted-dump.txt index >>>>>> 8394bc8..6327193 100644 --- >>>>>> a/Documentation/powerpc/firmware-assisted-dump.txt +++ >>>>>> b/Documentation/powerpc/firmware-assisted-dump.txt @@ -162,7 >>>>>> +162,15 @@ 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 comma >>>>>> + separated list through 'fadump_append=' parameter, to be >>>>>> enforced >>>>>> + when fadump is active. For example, if parameters like >>>>>> nr_cpus=1, >>>>>> + numa=off & udev.children-max=2 are to be enforced when fadump >>>>>> is >>>>>> + active, >>>>>> 'fadump_append=nr_cpus=1,numa=off,udev.children-max=2' >>>>>> + can be passed in command line, which will be replaced with >>>>>> + "nr_cpus=1 numa=off udev.children-max=2" when fadump is >>>>>> active. >>>>>> + This helps in reducing memory consumption during dump >>>>>> capture. +4. Optionally, user can also set 'crashkernel=' kernel >>>>>> cmdline to specify size of the memory to reserve for boot memory >>>>>> dump preservation. >>>>>> >>>>>> >>>>> Writing your own deficient parser for comma separated arguments >>>>> when perfectly fine parser for space separated quoted arguments >>>>> exists in the kernel and the bootloader does not seem like a good >>>>> idea to me. >>>> Couple of things that prompted me for v5 are: >>>> 1. Using parse_early_options() limits the kind of parameters >>>> that can be passed to fadump capture kernel. Passing >>>> parameters like systemd.unit= & udev.childern.max= has no effect >>>> with v4. Updating >>>> boot_command_line parameter, when fadump is active, seems a >>>> better alternative. >>>> >>>> 2. Passing space-separated quoted arguments is not working >>>> as intended with lilo. Updating bootloader with the below >>>> entry in /etc/lilo.conf file results in a missing append >>>> entry in /etc/yaboot.conf file. >>>> >>>> append = "quiet sysrq=1 insmod=sym53c8xx insmod=ipr >>>> crashkernel=512M-:256M fadump_append=\"nr_cpus=1 numa=off >>>> udev.children-max=2\"" >>> Meaning that a script that emulates LILO semantics on top of yaboot >>> which is completely capable of passing qouted space separated >>> arguments fails. IMHO it is more reasonable to fix the script or >>> whatever adaptation layer or use yaboot directly than working >>> around bug in said script by introducing a new argument parser in >>> the kernel. >>> >>> >> Hmmm.. while trying to implement space-separated parameter list with >> quotes as syntax for fadump_append parameter, noticed that it can >> make implemenation >> more vulnerable. Here are some problems I am facing while >> implementing this.. > How so? > > presumably you can reuse parse_args even if you do not register with > early_param and call it yourself. Then your parsing of fadump_append is I wasn't aware of that. Thanks for pointing it out, Michal. Will try to use parse_args and get back. Thanks Hari
Hello, On Mon, 15 May 2017 12:59:46 +0530 Hari Bathini <hbathini@linux.vnet.ibm.com> wrote: > On Friday 12 May 2017 09:12 PM, Michal Suchánek wrote: > > On Fri, 12 May 2017 15:15:33 +0530 > > Hari Bathini <hbathini@linux.vnet.ibm.com> wrote: > > > >> On Thursday 11 May 2017 06:46 PM, Michal Suchánek wrote: > >>> On Thu, 11 May 2017 02:00:11 +0530 > >>> Hari Bathini <hbathini@linux.vnet.ibm.com> wrote: > >>> > >>>> Hello Michal, > >>>> > >>>> On Wednesday 10 May 2017 09:31 PM, Michal Suchánek wrote: > >>>>> Hello, > >>>>> > >>>>> On Wed, 03 May 2017 23:52:52 +0530 > >>>>> Hari Bathini <hbathini@linux.vnet.ibm.com> wrote: > >>>>> > >>>>>> With the introduction of 'fadump_append=' parameter to pass > >>>>>> additional parameters to fadump (capture) kernel, update > >>>>>> documentation about it. > >>>>>> > >>>>>> Signed-off-by: Hari Bathini <hbathini@linux.vnet.ibm.com> > >>>>>> --- > >>>>>> > >>>>>> Changes from v4: > >>>>>> * Based on top of patchset that includes > >>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?h=akpm&id=05f383cdfba8793240e73f9a9fbff4e25d66003f > >>>>>> > >>>>>> > >>>>>> Documentation/powerpc/firmware-assisted-dump.txt | 10 > >>>>>> +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) > >>>>>> > >>>>>> diff --git a/Documentation/powerpc/firmware-assisted-dump.txt > >>>>>> b/Documentation/powerpc/firmware-assisted-dump.txt index > >>>>>> 8394bc8..6327193 100644 --- > >>>>>> a/Documentation/powerpc/firmware-assisted-dump.txt +++ > >>>>>> b/Documentation/powerpc/firmware-assisted-dump.txt @@ -162,7 > >>>>>> +162,15 @@ 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 comma > >>>>>> + separated list through 'fadump_append=' parameter, to be > >>>>>> enforced > >>>>>> + when fadump is active. For example, if parameters like > >>>>>> nr_cpus=1, > >>>>>> + numa=off & udev.children-max=2 are to be enforced when > >>>>>> fadump is > >>>>>> + active, > >>>>>> 'fadump_append=nr_cpus=1,numa=off,udev.children-max=2' > >>>>>> + can be passed in command line, which will be replaced with > >>>>>> + "nr_cpus=1 numa=off udev.children-max=2" when fadump is > >>>>>> active. > >>>>>> + This helps in reducing memory consumption during dump > >>>>>> capture. +4. Optionally, user can also set 'crashkernel=' > >>>>>> kernel cmdline to specify size of the memory to reserve for > >>>>>> boot memory dump preservation. > >>>>>> > >>>>>> > >>>>> Writing your own deficient parser for comma separated arguments > >>>>> when perfectly fine parser for space separated quoted arguments > >>>>> exists in the kernel and the bootloader does not seem like a > >>>>> good idea to me. > >>>> Couple of things that prompted me for v5 are: > >>>> 1. Using parse_early_options() limits the kind of parameters > >>>> that can be passed to fadump capture kernel. Passing > >>>> parameters like systemd.unit= & udev.childern.max= has no effect > >>>> with v4. Updating > >>>> boot_command_line parameter, when fadump is active, > >>>> seems a better alternative. > >>>> > >>>> 2. Passing space-separated quoted arguments is not working > >>>> as intended with lilo. Updating bootloader with the below > >>>> entry in /etc/lilo.conf file results in a missing append > >>>> entry in /etc/yaboot.conf file. > >>>> > >>>> append = "quiet sysrq=1 insmod=sym53c8xx insmod=ipr > >>>> crashkernel=512M-:256M fadump_append=\"nr_cpus=1 numa=off > >>>> udev.children-max=2\"" > >>> Meaning that a script that emulates LILO semantics on top of > >>> yaboot which is completely capable of passing qouted space > >>> separated arguments fails. IMHO it is more reasonable to fix the > >>> script or whatever adaptation layer or use yaboot directly than > >>> working around bug in said script by introducing a new argument > >>> parser in the kernel. > >>> > >>> > >> Hmmm.. while trying to implement space-separated parameter list > >> with quotes as syntax for fadump_append parameter, noticed that it > >> can make implemenation > >> more vulnerable. Here are some problems I am facing while > >> implementing this.. > > How so? > > > > presumably you can reuse parse_args even if you do not register with > > early_param and call it yourself. Then your parsing of > > fadump_append is > > > I wasn't aware of that. Thanks for pointing it out, Michal. > Will try to use parse_args and get back. > I was thinking a bit more about the uses of the commandline and how fadump_append potentially breaks it. The first thing that should be addressed and is the special -- argument which denotes the start of init arguments that are not to be parsed by the kernel. Incidentally the initial implementation using early_param happened to handles that without issue. parse_args surely handles that so adding a hook somewhere should give you location of that argument (if any). And interesting thing that can happen is passing an -- inside the fadump_append argument. It should be handled (or not) in some way or other and the handling documented. The second thing that breaks is reusing content of /proc/cmdline in kexec calls for passing arguments to the loaded kernel. It works flawlessly passing the same arguments the currently running kernel was started with unless fadump_append argument handler appends content of the fadump_append argument to actual commandline that appears there. So this would be an argument against modifying the commandline. You could argue using fadump and kexec together is an exotic use case but I would expect that the very machines that require fadump_append to reduce dump memory requirement benefit from using kexec for reboots because the BIOS probing the hardware takes quite a while as well. If rewriting the commandline is desired then this side effect of recursively adding the content of fadump_append on kexecs which reuse /proc/cmdline should be documented. Thanks Michal
Hi Michal, Sorry for taking this long to respond. I was working on a few other things. On Monday 15 May 2017 02:59 PM, Michal Suchánek wrote: > Hello, > > On Mon, 15 May 2017 12:59:46 +0530 > Hari Bathini <hbathini@linux.vnet.ibm.com> wrote: > >> On Friday 12 May 2017 09:12 PM, Michal Suchánek wrote: >>> On Fri, 12 May 2017 15:15:33 +0530 >>> Hari Bathini <hbathini@linux.vnet.ibm.com> wrote: >>> >>>> On Thursday 11 May 2017 06:46 PM, Michal Suchánek wrote: >>>>> On Thu, 11 May 2017 02:00:11 +0530 >>>>> Hari Bathini <hbathini@linux.vnet.ibm.com> wrote: >>>>> >>>>>> Hello Michal, >>>>>> >>>>>> On Wednesday 10 May 2017 09:31 PM, Michal Suchánek wrote: >>>>>>> Hello, >>>>>>> >>>>>>> On Wed, 03 May 2017 23:52:52 +0530 >>>>>>> Hari Bathini <hbathini@linux.vnet.ibm.com> wrote: >>>>>>> >>>>>>>> With the introduction of 'fadump_append=' parameter to pass >>>>>>>> additional parameters to fadump (capture) kernel, update >>>>>>>> documentation about it. >>>>>>>> >>>>>>>> Signed-off-by: Hari Bathini <hbathini@linux.vnet.ibm.com> >>>>>>>> --- >>>>>>>> >>>>>>>> Changes from v4: >>>>>>>> * Based on top of patchset that includes >>>>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?h=akpm&id=05f383cdfba8793240e73f9a9fbff4e25d66003f >>>>>>>> >>>>>>>> >>>>>>>> Documentation/powerpc/firmware-assisted-dump.txt | 10 >>>>>>>> +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) >>>>>>>> >>>>>>>> diff --git a/Documentation/powerpc/firmware-assisted-dump.txt >>>>>>>> b/Documentation/powerpc/firmware-assisted-dump.txt index >>>>>>>> 8394bc8..6327193 100644 --- >>>>>>>> a/Documentation/powerpc/firmware-assisted-dump.txt +++ >>>>>>>> b/Documentation/powerpc/firmware-assisted-dump.txt @@ -162,7 >>>>>>>> +162,15 @@ 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 comma >>>>>>>> + separated list through 'fadump_append=' parameter, to be >>>>>>>> enforced >>>>>>>> + when fadump is active. For example, if parameters like >>>>>>>> nr_cpus=1, >>>>>>>> + numa=off & udev.children-max=2 are to be enforced when >>>>>>>> fadump is >>>>>>>> + active, >>>>>>>> 'fadump_append=nr_cpus=1,numa=off,udev.children-max=2' >>>>>>>> + can be passed in command line, which will be replaced with >>>>>>>> + "nr_cpus=1 numa=off udev.children-max=2" when fadump is >>>>>>>> active. >>>>>>>> + This helps in reducing memory consumption during dump >>>>>>>> capture. +4. Optionally, user can also set 'crashkernel=' >>>>>>>> kernel cmdline to specify size of the memory to reserve for >>>>>>>> boot memory dump preservation. >>>>>>>> >>>>>>>> >>>>>>> Writing your own deficient parser for comma separated arguments >>>>>>> when perfectly fine parser for space separated quoted arguments >>>>>>> exists in the kernel and the bootloader does not seem like a >>>>>>> good idea to me. >>>>>> Couple of things that prompted me for v5 are: >>>>>> 1. Using parse_early_options() limits the kind of parameters >>>>>> that can be passed to fadump capture kernel. Passing >>>>>> parameters like systemd.unit= & udev.childern.max= has no effect >>>>>> with v4. Updating >>>>>> boot_command_line parameter, when fadump is active, >>>>>> seems a better alternative. >>>>>> >>>>>> 2. Passing space-separated quoted arguments is not working >>>>>> as intended with lilo. Updating bootloader with the below >>>>>> entry in /etc/lilo.conf file results in a missing append >>>>>> entry in /etc/yaboot.conf file. >>>>>> >>>>>> append = "quiet sysrq=1 insmod=sym53c8xx insmod=ipr >>>>>> crashkernel=512M-:256M fadump_append=\"nr_cpus=1 numa=off >>>>>> udev.children-max=2\"" >>>>> Meaning that a script that emulates LILO semantics on top of >>>>> yaboot which is completely capable of passing qouted space >>>>> separated arguments fails. IMHO it is more reasonable to fix the >>>>> script or whatever adaptation layer or use yaboot directly than >>>>> working around bug in said script by introducing a new argument >>>>> parser in the kernel. >>>>> >>>>> >>>> Hmmm.. while trying to implement space-separated parameter list >>>> with quotes as syntax for fadump_append parameter, noticed that it >>>> can make implemenation >>>> more vulnerable. Here are some problems I am facing while >>>> implementing this.. >>> How so? >>> >>> presumably you can reuse parse_args even if you do not register with >>> early_param and call it yourself. Then your parsing of >>> fadump_append is >> >> I wasn't aware of that. Thanks for pointing it out, Michal. >> Will try to use parse_args and get back. >> > I was thinking a bit more about the uses of the commandline and how > fadump_append potentially breaks it. > > The first thing that should be addressed and is the special -- argument > which denotes the start of init arguments that are not to be parsed by > the kernel. Incidentally the initial implementation using early_param > happened to handles that without issue. parse_args surely handles that > so adding a hook somewhere should give you location of that argument > (if any). And interesting thing that can happen is passing an -- inside > the fadump_append argument. It should be handled (or not) in some way > or other and the handling documented. The intention with this patch is to replace "root=/dev/sda2 ro fadump_append=nr_cpus=1,numa=off crashkernel=1024M" with "root=/dev/sda2 ro nr_cpus=1 numa=off crashkernel=1024M" when kernel is booting for dump capture. While parse_args() is making parsing relatively easy, the main idea is to replace parameters as above when fadump capture kernel is booting. The code is relatively complicated to replace parameters using space-separated quoted string even though parse_args() may parse them easily. Comma-separated list was easier to implement and seemed less error prone for replacing parameters. Want to replace fadump_append=, to avoid command line overflow issues and also to not have to deal with special cases like --. Actually, fadump_append= seems to be confusing in that sense. How about fadump_args=comma-separated-list- of-params-for-capture-kernel ? Please share your thoughts. > The second thing that breaks is reusing content of /proc/cmdline in > kexec calls for passing arguments to the loaded kernel. It works > flawlessly passing the same arguments the currently running kernel was > started with unless fadump_append argument handler appends content of > the fadump_append argument to actual commandline that appears there. So > this would be an argument against modifying the commandline. You could > argue using fadump and kexec together is an exotic use case but I would > expect that the very machines that require fadump_append to reduce dump > memory requirement benefit from using kexec for reboots because the BIOS > probing the hardware takes quite a while as well. If rewriting the > commandline is desired then this side effect of recursively adding the > content of fadump_append on kexecs which reuse /proc/cmdline should be > documented. > > fadump capture kernel (the kernel where we expand "fadump_append=x,y,z" to "x y z") only advised to be used for saving dump (just like kdump kernel). We are expected to boot into production kernel immediately after saving dump. Only in special cases where a user configures to stay in fadump capture kernel will he encounter the above problem. And someone choosing such scenario is most likely aware of what to make of /proc/cmdline? Thanks Hari
On Thu, 8 Jun 2017 23:30:37 +0530 Hari Bathini <hbathini@linux.vnet.ibm.com> wrote: > Hi Michal, > > Sorry for taking this long to respond. I was working on a few other > things. > > On Monday 15 May 2017 02:59 PM, Michal Suchánek wrote: > > Hello, > > > > On Mon, 15 May 2017 12:59:46 +0530 > > Hari Bathini <hbathini@linux.vnet.ibm.com> wrote: > > > >> On Friday 12 May 2017 09:12 PM, Michal Suchánek wrote: > >>> On Fri, 12 May 2017 15:15:33 +0530 > >>> Hari Bathini <hbathini@linux.vnet.ibm.com> wrote: > >>> > >>>> On Thursday 11 May 2017 06:46 PM, Michal Suchánek wrote: > >>>>> On Thu, 11 May 2017 02:00:11 +0530 > >>>>> Hari Bathini <hbathini@linux.vnet.ibm.com> wrote: > >>>>> > >>>>>> Hello Michal, > >>>>>> > >>>>>> On Wednesday 10 May 2017 09:31 PM, Michal Suchánek wrote: > >>>>>>> Hello, > >>>>>>> > >>>>>>> On Wed, 03 May 2017 23:52:52 +0530 > >>>>>>> Hari Bathini <hbathini@linux.vnet.ibm.com> wrote: > >>>>>>> > >>>>>>>> With the introduction of 'fadump_append=' parameter to pass > >>>>>>>> additional parameters to fadump (capture) kernel, update > >>>>>>>> documentation about it. > >>>>>>>> > >>>>>>>> Signed-off-by: Hari Bathini <hbathini@linux.vnet.ibm.com> > >>>>>>>> --- > >>>>>>>> > >>>>>>>> Changes from v4: > >>>>>>>> * Based on top of patchset that includes > >>>>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?h=akpm&id=05f383cdfba8793240e73f9a9fbff4e25d66003f > >>>>>>>> > >>>>>>>> > >>>>>>>> Documentation/powerpc/firmware-assisted-dump.txt | 10 > >>>>>>>> +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) > >>>>>>>> > >>>>>>>> diff --git a/Documentation/powerpc/firmware-assisted-dump.txt > >>>>>>>> b/Documentation/powerpc/firmware-assisted-dump.txt index > >>>>>>>> 8394bc8..6327193 100644 --- > >>>>>>>> a/Documentation/powerpc/firmware-assisted-dump.txt +++ > >>>>>>>> b/Documentation/powerpc/firmware-assisted-dump.txt @@ -162,7 > >>>>>>>> +162,15 @@ 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 comma > >>>>>>>> + separated list through 'fadump_append=' parameter, to be > >>>>>>>> enforced > >>>>>>>> + when fadump is active. For example, if parameters like > >>>>>>>> nr_cpus=1, > >>>>>>>> + numa=off & udev.children-max=2 are to be enforced when > >>>>>>>> fadump is > >>>>>>>> + active, > >>>>>>>> 'fadump_append=nr_cpus=1,numa=off,udev.children-max=2' > >>>>>>>> + can be passed in command line, which will be replaced > >>>>>>>> with > >>>>>>>> + "nr_cpus=1 numa=off udev.children-max=2" when fadump is > >>>>>>>> active. > >>>>>>>> + This helps in reducing memory consumption during dump > >>>>>>>> capture. +4. Optionally, user can also set 'crashkernel=' > >>>>>>>> kernel cmdline to specify size of the memory to reserve for > >>>>>>>> boot memory dump preservation. > >>>>>>>> > >>>>>>>> > >>>>>>> Writing your own deficient parser for comma separated > >>>>>>> arguments when perfectly fine parser for space separated > >>>>>>> quoted arguments exists in the kernel and the bootloader does > >>>>>>> not seem like a good idea to me. > >>>>>> Couple of things that prompted me for v5 are: > >>>>>> 1. Using parse_early_options() limits the kind of > >>>>>> parameters that can be passed to fadump capture kernel. Passing > >>>>>> parameters like systemd.unit= & udev.childern.max= has no > >>>>>> effect with v4. Updating > >>>>>> boot_command_line parameter, when fadump is active, > >>>>>> seems a better alternative. > >>>>>> > >>>>>> 2. Passing space-separated quoted arguments is not > >>>>>> working as intended with lilo. Updating bootloader with the > >>>>>> below entry in /etc/lilo.conf file results in a missing append > >>>>>> entry in /etc/yaboot.conf file. > >>>>>> > >>>>>> append = "quiet sysrq=1 insmod=sym53c8xx insmod=ipr > >>>>>> crashkernel=512M-:256M fadump_append=\"nr_cpus=1 numa=off > >>>>>> udev.children-max=2\"" > >>>>> Meaning that a script that emulates LILO semantics on top of > >>>>> yaboot which is completely capable of passing qouted space > >>>>> separated arguments fails. IMHO it is more reasonable to fix the > >>>>> script or whatever adaptation layer or use yaboot directly than > >>>>> working around bug in said script by introducing a new argument > >>>>> parser in the kernel. > >>>>> > >>>>> > >>>> Hmmm.. while trying to implement space-separated parameter list > >>>> with quotes as syntax for fadump_append parameter, noticed that > >>>> it can make implemenation > >>>> more vulnerable. Here are some problems I am facing while > >>>> implementing this.. > >>> How so? > >>> > >>> presumably you can reuse parse_args even if you do not register > >>> with early_param and call it yourself. Then your parsing of > >>> fadump_append is > >> > >> I wasn't aware of that. Thanks for pointing it out, Michal. > >> Will try to use parse_args and get back. > >> > > I was thinking a bit more about the uses of the commandline and how > > fadump_append potentially breaks it. > > > > The first thing that should be addressed and is the special -- > > argument which denotes the start of init arguments that are not to > > be parsed by the kernel. Incidentally the initial implementation > > using early_param happened to handles that without issue. > > parse_args surely handles that so adding a hook somewhere should > > give you location of that argument (if any). And interesting thing > > that can happen is passing an -- inside the fadump_append argument. > > It should be handled (or not) in some way or other and the handling > > documented. > > > The intention with this patch is to replace > > "root=/dev/sda2 ro fadump_append=nr_cpus=1,numa=off > crashkernel=1024M" > > with > > "root=/dev/sda2 ro nr_cpus=1 numa=off crashkernel=1024M" > > when kernel is booting for dump capture. > While parse_args() is making parsing relatively easy, the main idea is > to replace parameters as above when fadump capture kernel is booting. > The code is relatively complicated to replace parameters using > space-separated > quoted string even though parse_args() may parse them easily. > Comma-separated > list was easier to implement and seemed less error prone for > replacing parameters. You can still do that relatively easily. parse_args() gives you the content of fadump_capture argument. You should probably add a variant that gives you also the position of the fadump_capture argument in case user specified it multiple times - picking the wrong one would cause errors. Then you can just replace the fadump_append="appended arguments" with the dequoted "appended arguments" parse_args() gives you. Dequating should shorten the arguments as would removing the fadump_append= prefix so it should be quite possible in-place. It is in fact even easier than the comma separated list since you do not need to do any parsing yourself - only strlen, memset and memcpy. That said, if you replace the fadump_append argument the running kernel will have extra arguments without knowing where they came from making detection of this happening all the more difficult. > > Want to replace fadump_append=, to avoid command line overflow issues > and also to not have to deal with special cases like --. Actually, > fadump_append= > seems to be confusing in that sense. How about > fadump_args=comma-separated-list- > of-params-for-capture-kernel ? Please share your thoughts. The comma separated list still can contain a -- argument so you still have to do something about that. Or not and just document that people should not use it. As for the parameter name fadump_args or fadump_extra_args is more generic than fadump_append giving you more room for changing the implementation details without making the argument name non-sensical. > > > > The second thing that breaks is reusing content of /proc/cmdline in > > kexec calls for passing arguments to the loaded kernel. It works > > flawlessly passing the same arguments the currently running kernel > > was started with unless fadump_append argument handler appends > > content of the fadump_append argument to actual commandline that > > appears there. So this would be an argument against modifying the > > commandline. You could argue using fadump and kexec together is an > > exotic use case but I would expect that the very machines that > > require fadump_append to reduce dump memory requirement benefit > > from using kexec for reboots because the BIOS probing the hardware > > takes quite a while as well. If rewriting the commandline is > > desired then this side effect of recursively adding the content of > > fadump_append on kexecs which reuse /proc/cmdline should be > > documented. > > > > > > fadump capture kernel (the kernel where we expand > "fadump_append=x,y,z" to "x y z") only advised to be used for saving > dump (just like kdump kernel). > We are expected to boot into production kernel immediately after > saving dump. Which you can do for example using kexec. > Only in special cases where a user configures to stay in fadump > capture kernel > will he encounter the above problem. And someone choosing such > scenario is most > likely aware of what to make of /proc/cmdline? Why? They leave that to the initscripts and since you just changed the semantics of the kernel parameters significantly those can produce entertaining results. Thanks Michal
On Friday 09 June 2017 05:34 PM, Michal Suchánek wrote: > On Thu, 8 Jun 2017 23:30:37 +0530 > Hari Bathini <hbathini@linux.vnet.ibm.com> wrote: > >> Hi Michal, >> >> Sorry for taking this long to respond. I was working on a few other >> things. >> >> On Monday 15 May 2017 02:59 PM, Michal Suchánek wrote: >>> Hello, >>> >>> On Mon, 15 May 2017 12:59:46 +0530 >>> Hari Bathini <hbathini@linux.vnet.ibm.com> wrote: >>> >>>> On Friday 12 May 2017 09:12 PM, Michal Suchánek wrote: >>>>> On Fri, 12 May 2017 15:15:33 +0530 >>>>> Hari Bathini <hbathini@linux.vnet.ibm.com> wrote: >>>>> >>>>>> On Thursday 11 May 2017 06:46 PM, Michal Suchánek wrote: >>>>>>> On Thu, 11 May 2017 02:00:11 +0530 >>>>>>> Hari Bathini <hbathini@linux.vnet.ibm.com> wrote: >>>>>>> >>>>>>>> Hello Michal, >>>>>>>> >>>>>>>> On Wednesday 10 May 2017 09:31 PM, Michal Suchánek wrote: >>>>>>>>> Hello, >>>>>>>>> >>>>>>>>> On Wed, 03 May 2017 23:52:52 +0530 >>>>>>>>> Hari Bathini <hbathini@linux.vnet.ibm.com> wrote: >>>>>>>>> >>>>>>>>>> With the introduction of 'fadump_append=' parameter to pass >>>>>>>>>> additional parameters to fadump (capture) kernel, update >>>>>>>>>> documentation about it. >>>>>>>>>> >>>>>>>>>> Signed-off-by: Hari Bathini <hbathini@linux.vnet.ibm.com> >>>>>>>>>> --- >>>>>>>>>> >>>>>>>>>> Changes from v4: >>>>>>>>>> * Based on top of patchset that includes >>>>>>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?h=akpm&id=05f383cdfba8793240e73f9a9fbff4e25d66003f >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> Documentation/powerpc/firmware-assisted-dump.txt | 10 >>>>>>>>>> +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) >>>>>>>>>> >>>>>>>>>> diff --git a/Documentation/powerpc/firmware-assisted-dump.txt >>>>>>>>>> b/Documentation/powerpc/firmware-assisted-dump.txt index >>>>>>>>>> 8394bc8..6327193 100644 --- >>>>>>>>>> a/Documentation/powerpc/firmware-assisted-dump.txt +++ >>>>>>>>>> b/Documentation/powerpc/firmware-assisted-dump.txt @@ -162,7 >>>>>>>>>> +162,15 @@ 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 comma >>>>>>>>>> + separated list through 'fadump_append=' parameter, to be >>>>>>>>>> enforced >>>>>>>>>> + when fadump is active. For example, if parameters like >>>>>>>>>> nr_cpus=1, >>>>>>>>>> + numa=off & udev.children-max=2 are to be enforced when >>>>>>>>>> fadump is >>>>>>>>>> + active, >>>>>>>>>> 'fadump_append=nr_cpus=1,numa=off,udev.children-max=2' >>>>>>>>>> + can be passed in command line, which will be replaced >>>>>>>>>> with >>>>>>>>>> + "nr_cpus=1 numa=off udev.children-max=2" when fadump is >>>>>>>>>> active. >>>>>>>>>> + This helps in reducing memory consumption during dump >>>>>>>>>> capture. +4. Optionally, user can also set 'crashkernel=' >>>>>>>>>> kernel cmdline to specify size of the memory to reserve for >>>>>>>>>> boot memory dump preservation. >>>>>>>>>> >>>>>>>>>> >>>>>>>>> Writing your own deficient parser for comma separated >>>>>>>>> arguments when perfectly fine parser for space separated >>>>>>>>> quoted arguments exists in the kernel and the bootloader does >>>>>>>>> not seem like a good idea to me. >>>>>>>> Couple of things that prompted me for v5 are: >>>>>>>> 1. Using parse_early_options() limits the kind of >>>>>>>> parameters that can be passed to fadump capture kernel. Passing >>>>>>>> parameters like systemd.unit= & udev.childern.max= has no >>>>>>>> effect with v4. Updating >>>>>>>> boot_command_line parameter, when fadump is active, >>>>>>>> seems a better alternative. >>>>>>>> >>>>>>>> 2. Passing space-separated quoted arguments is not >>>>>>>> working as intended with lilo. Updating bootloader with the >>>>>>>> below entry in /etc/lilo.conf file results in a missing append >>>>>>>> entry in /etc/yaboot.conf file. >>>>>>>> >>>>>>>> append = "quiet sysrq=1 insmod=sym53c8xx insmod=ipr >>>>>>>> crashkernel=512M-:256M fadump_append=\"nr_cpus=1 numa=off >>>>>>>> udev.children-max=2\"" >>>>>>> Meaning that a script that emulates LILO semantics on top of >>>>>>> yaboot which is completely capable of passing qouted space >>>>>>> separated arguments fails. IMHO it is more reasonable to fix the >>>>>>> script or whatever adaptation layer or use yaboot directly than >>>>>>> working around bug in said script by introducing a new argument >>>>>>> parser in the kernel. >>>>>>> >>>>>>> >>>>>> Hmmm.. while trying to implement space-separated parameter list >>>>>> with quotes as syntax for fadump_append parameter, noticed that >>>>>> it can make implemenation >>>>>> more vulnerable. Here are some problems I am facing while >>>>>> implementing this.. >>>>> How so? >>>>> >>>>> presumably you can reuse parse_args even if you do not register >>>>> with early_param and call it yourself. Then your parsing of >>>>> fadump_append is >>>> I wasn't aware of that. Thanks for pointing it out, Michal. >>>> Will try to use parse_args and get back. >>>> >>> I was thinking a bit more about the uses of the commandline and how >>> fadump_append potentially breaks it. >>> >>> The first thing that should be addressed and is the special -- >>> argument which denotes the start of init arguments that are not to >>> be parsed by the kernel. Incidentally the initial implementation >>> using early_param happened to handles that without issue. >>> parse_args surely handles that so adding a hook somewhere should >>> give you location of that argument (if any). And interesting thing >>> that can happen is passing an -- inside the fadump_append argument. >>> It should be handled (or not) in some way or other and the handling >>> documented. >> >> The intention with this patch is to replace >> >> "root=/dev/sda2 ro fadump_append=nr_cpus=1,numa=off >> crashkernel=1024M" >> >> with >> >> "root=/dev/sda2 ro nr_cpus=1 numa=off crashkernel=1024M" >> >> when kernel is booting for dump capture. >> While parse_args() is making parsing relatively easy, the main idea is >> to replace parameters as above when fadump capture kernel is booting. >> The code is relatively complicated to replace parameters using >> space-separated >> quoted string even though parse_args() may parse them easily. >> Comma-separated >> list was easier to implement and seemed less error prone for >> replacing parameters. > > You can still do that relatively easily. parse_args() gives you the > content of fadump_capture argument. You should probably add a variant > that gives you also the position of the fadump_capture argument in case > user specified it multiple times - picking the wrong one would cause > errors. There is no denying that this can be done with the use of parse_args() function. But my contention is a custom parser is no more error prone, considering it only has to search/replace commas in fadump_extra_args= till the first occurrence of space/nul, without having to deal with the naunces of parse_args() function. Also, to get the last occurence, could use something like get_last_crashkernel() instead of adding a variant to parse_args(). > Then you can just replace the fadump_append="appended arguments" with > the dequoted "appended arguments" parse_args() gives you. Dequating > should shorten the arguments as would removing the fadump_append= > prefix so it should be quite possible in-place. It is in fact even > easier than the comma separated list since you do not need to do any > parsing yourself - only strlen, memset and memcpy. > > That said, if you replace the fadump_append argument the running kernel > will have extra arguments without knowing where they came from making > detection of this happening all the more difficult. How about replacing "fadump_extra_args=x,y,z" with "fadump_extra_args x y z"? Presence of fadump_extra_args hints the user about the extra parameters passed to fadump capture kernel and also lets parsing/replacing be simple - replace the first '=' and subsequent commas with ' ' in this fadump_extra_args= parameter and we are good to go. No additional handling. Even if we go with space-separated quoted string, simply replacing "fadump_extra_args=" with "fadump_extra_args " should suffice, no need to worry about parse_args as well. I prefer comma-separated list though, for it leaves no dependency on how bootloader handles space-separated quoted strings, considering my encounter with yast bootloader which is not so impressive in handling quoted strings. Also, the code change is not so complicated. >> Want to replace fadump_append=, to avoid command line overflow issues >> and also to not have to deal with special cases like --. Actually, >> fadump_append= >> seems to be confusing in that sense. How about >> fadump_args=comma-separated-list- >> of-params-for-capture-kernel ? Please share your thoughts. > The comma separated list still can contain a -- argument so you still > have to do something about that. Or not and just document that people > should not use it. The user has the flexibility to use fadump_extra_args= to pass special parameters keeping in mind that "fadump_extra_args=x,y,z" will be replaced in-place with "fadump_extra_args x y z" and it happens only in fadump capture kernel. No additional code changes needed to handle this except for documentation talking about it? > As for the parameter name fadump_args or fadump_extra_args is more > generic than fadump_append giving you more room for changing the > implementation details without making the argument name non-sensical. > >> >>> The second thing that breaks is reusing content of /proc/cmdline in >>> kexec calls for passing arguments to the loaded kernel. It works >>> flawlessly passing the same arguments the currently running kernel >>> was started with unless fadump_append argument handler appends >>> content of the fadump_append argument to actual commandline that >>> appears there. So this would be an argument against modifying the >>> commandline. You could argue using fadump and kexec together is an >>> exotic use case but I would expect that the very machines that >>> require fadump_append to reduce dump memory requirement benefit >>> from using kexec for reboots because the BIOS probing the hardware >>> takes quite a while as well. If rewriting the commandline is >>> desired then this side effect of recursively adding the content of >>> fadump_append on kexecs which reuse /proc/cmdline should be >>> documented. >>> >>> >> fadump capture kernel (the kernel where we expand >> "fadump_append=x,y,z" to "x y z") only advised to be used for saving >> dump (just like kdump kernel). >> We are expected to boot into production kernel immediately after >> saving dump. > Which you can do for example using kexec. > >> Only in special cases where a user configures to stay in fadump >> capture kernel >> will he encounter the above problem. And someone choosing such >> scenario is most >> likely aware of what to make of /proc/cmdline? > Why? They leave that to the initscripts and since you just changed the > semantics of the kernel parameters significantly those can produce > entertaining results. > > Yeah. That is tricky but we are talking about a capture kernel here which should typically reboot after dump capture (just like kdump kernel does). So, the case you are mentioning about can be considered an exception a script should be able to tackle? Thanks Hari
Hello, On Tue, 20 Jun 2017 21:14:08 +0530 Hari Bathini <hbathini@linux.vnet.ibm.com> wrote: > On Friday 09 June 2017 05:34 PM, Michal Suchánek wrote: > > On Thu, 8 Jun 2017 23:30:37 +0530 > > Hari Bathini <hbathini@linux.vnet.ibm.com> wrote: > >> On Monday 15 May 2017 02:59 PM, Michal Suchánek wrote: > >>> On Mon, 15 May 2017 12:59:46 +0530 > >>> Hari Bathini <hbathini@linux.vnet.ibm.com> wrote: > >>>> On Friday 12 May 2017 09:12 PM, Michal Suchánek wrote: > >>>>> On Fri, 12 May 2017 15:15:33 +0530 > >>>>> Hari Bathini <hbathini@linux.vnet.ibm.com> wrote: > >>>>>> On Thursday 11 May 2017 06:46 PM, Michal Suchánek wrote: > >>>>>>> On Thu, 11 May 2017 02:00:11 +0530 > >>>>>>> Hari Bathini <hbathini@linux.vnet.ibm.com> wrote: > >>>>>>>> On Wednesday 10 May 2017 09:31 PM, Michal Suchánek wrote: > >>>>>>>>> On Wed, 03 May 2017 23:52:52 +0530 > >>>>>>>>> Hari Bathini <hbathini@linux.vnet.ibm.com> wrote: > >>>>>>>>> > >>>>>>>>>> With the introduction of 'fadump_append=' parameter to pass > >>>>>>>>>> additional parameters to fadump (capture) kernel, update > >>>>>>>>>> documentation about it. > >>>>>>>>>> > >>>>>>>>>> Signed-off-by: Hari Bathini <hbathini@linux.vnet.ibm.com> > >>>>>>>>>> --- > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>> Writing your own deficient parser for comma separated > >>>>>>>>> arguments when perfectly fine parser for space separated > >>>>>>>>> quoted arguments exists in the kernel and the bootloader > >>>>>>>>> does not seem like a good idea to me. > >>>>>>>> Couple of things that prompted me for v5 are: > >>>>>>>> 1. Using parse_early_options() limits the kind of > >>>>>>>> parameters that can be passed to fadump capture kernel. > >>>>>>>> Passing parameters like systemd.unit= & udev.childern.max= > >>>>>>>> has no effect with v4. Updating > >>>>>>>> boot_command_line parameter, when fadump is active, > >>>>>>>> seems a better alternative. > >>>>>>>> > >>>>>>>> 2. Passing space-separated quoted arguments is not > >>>>>>>> working as intended with lilo. Updating bootloader with the > >>>>>>>> below entry in /etc/lilo.conf file results in a missing > >>>>>>>> append entry in /etc/yaboot.conf file. > >>>>>>>> > >>>>>>>> append = "quiet sysrq=1 insmod=sym53c8xx > >>>>>>>> insmod=ipr crashkernel=512M-:256M fadump_append=\"nr_cpus=1 > >>>>>>>> numa=off udev.children-max=2\"" > >>>>>>> Meaning that a script that emulates LILO semantics on top of > >>>>>>> yaboot which is completely capable of passing qouted space > >>>>>>> separated arguments fails. IMHO it is more reasonable to fix > >>>>>>> the script or whatever adaptation layer or use yaboot > >>>>>>> directly than working around bug in said script by > >>>>>>> introducing a new argument parser in the kernel. > >>>>>>> > >>>>>>> > >> The intention with this patch is to replace > >> > >> "root=/dev/sda2 ro fadump_append=nr_cpus=1,numa=off > >> crashkernel=1024M" > >> > >> with > >> > >> "root=/dev/sda2 ro nr_cpus=1 numa=off crashkernel=1024M" > >> > >> when kernel is booting for dump capture. > >> While parse_args() is making parsing relatively easy, the main > >> idea is to replace parameters as above when fadump capture kernel > >> is booting. The code is relatively complicated to replace > >> parameters using space-separated > >> quoted string even though parse_args() may parse them easily. > >> Comma-separated > >> list was easier to implement and seemed less error prone for > >> replacing parameters. > > > > You can still do that relatively easily. parse_args() gives you the > > content of fadump_capture argument. You should probably add a > > variant that gives you also the position of the fadump_capture > > argument in case user specified it multiple times - picking the > > wrong one would cause errors. > > There is no denying that this can be done with the use of > parse_args() function. > But my contention is a custom parser is no more error prone, > considering it only has to search/replace commas in > fadump_extra_args= till the first occurrence of space/nul, without > having to deal with the naunces of parse_args() function. If you do that you 1) introduce new parser which is error-prone even for not very complex parsers 2) does not handle quoting properly meaning that a) the passed arguments cannot include a comma (which is the case for many existing kernel parameters) b) your parser, parse_args and any parser analyzing the commandline before you replace the arguments do not agree on the content and length of the fadump_extra_args argument 3) you introduce completely new syntax for arguments which the user has to research and understand for no good reason > Also, to > get the last occurence, could use something like > get_last_crashkernel() instead of adding a variant to parse_args(). Why are you talking about last occurence all the time? Does that mean that if user specifies fadump_extra_args in both the 'default' and 'extra' kernel arguments to the grub configuration script only the one from 'extra' arguments will be expanded and the on from 'default' arguments will be just left there? Why? > > > Then you can just replace the fadump_append="appended arguments" > > with the dequoted "appended arguments" parse_args() gives you. > > Dequating should shorten the arguments as would removing the > > fadump_append= prefix so it should be quite possible in-place. It > > is in fact even easier than the comma separated list since you do > > not need to do any parsing yourself - only strlen, memset and > > memcpy. > > > > That said, if you replace the fadump_append argument the running > > kernel will have extra arguments without knowing where they came > > from making detection of this happening all the more difficult. > > How about replacing "fadump_extra_args=x,y,z" with "fadump_extra_args > x y z"? How does this handle x that includes a comma? How does this handle x that includes a space? How does it handle x that includes a double quote? - Well, presumably it will just stay there with the fadump_extra_args expanded or not and will cause equal breakage. > Presence of fadump_extra_args hints the user about the extra > parameters passed > to fadump capture kernel and also lets parsing/replacing be simple - > replace the first '=' and subsequent commas with ' ' in this > fadump_extra_args= parameter > and we are good to go. No additional handling. Even if we go with > space-separated > quoted string, simply replacing "fadump_extra_args=" with > "fadump_extra_args " Leaving the empty fadump_extra_args argument might be good for indication that the replacement possibly took place and comes for free. > should suffice, no need to worry about parse_args as well. If you use parse_args it can give you the content of the every fadump_extra_args argument and dequotes it for you. So in the case fadump_extra_args does include double quotes you get them handled correctly for you by existing parser. And you want to make a variant of parse_args that tells you where the argument starts and ends in the parsed commandline so you know which part you should replace - which it certainly does know but no current users need the information so it does not tell you. When you do that users can read (or for the most part have already read) about the kernel argument syntax and apply that to fadump_extra_args. No additional specification and exceptions are needed. > I prefer > comma-separated > list though, for it leaves no dependency on how bootloader handles > space-separated > quoted strings, considering my encounter with yast bootloader which > is not so > impressive in handling quoted strings. Also, the code change is not > so complicated. If yast cannot handle kernel parameters correctly report a bug. The quoting of arguments with spaces is part of the kernel parameter specification so yast should handle it. > > > >> Want to replace fadump_append=, to avoid command line overflow > >> issues and also to not have to deal with special cases like --. > >> Actually, fadump_append= > >> seems to be confusing in that sense. How about > >> fadump_args=comma-separated-list- > >> of-params-for-capture-kernel ? Please share your thoughts. > > The comma separated list still can contain a -- argument so you > > still have to do something about that. Or not and just document > > that people should not use it. > > The user has the flexibility to use fadump_extra_args= to pass > special parameters > keeping in mind that "fadump_extra_args=x,y,z" will be replaced > in-place with > "fadump_extra_args x y z" and it happens only in fadump capture > kernel. No additional > code changes needed to handle this except for documentation talking > about it? Presumably if y is "--" then z and any subsequent kernel arguments after fadump_extra_args are passed to init when fadump_extra_args is expanded and not so when left as is. This may be unexpected - the user may expect that if fadump_extra_args contains a "--" as y then z is appended after existing "--" in commandline or if none is present the whole "-- z" is moved to the end. This would be clean way of handling the extra arguments - no text only preprocessing allowing for dirty tricks moving arguments from kernel to init by in-place text replacement. Note that any unclosed quoting in arguments after fadump_extra_args may make appending "-- z" quite difficult if you wanted to do it correctly. If you do not want to bother (with either) it is an understandable shortcoming in the implementation and should be documented. Thanks Michal
Hi Michal, Thanks for the review.. On Monday 26 June 2017 05:45 PM, Michal Suchánek wrote: > Hello, > > On Tue, 20 Jun 2017 21:14:08 +0530 > Hari Bathini <hbathini@linux.vnet.ibm.com> wrote: > >> On Friday 09 June 2017 05:34 PM, Michal Suchánek wrote: >>> On Thu, 8 Jun 2017 23:30:37 +0530 >>> Hari Bathini <hbathini@linux.vnet.ibm.com> wrote: >>>> On Monday 15 May 2017 02:59 PM, Michal Suchánek wrote: >>>>> On Mon, 15 May 2017 12:59:46 +0530 >>>>> Hari Bathini <hbathini@linux.vnet.ibm.com> wrote: >>>>>> On Friday 12 May 2017 09:12 PM, Michal Suchánek wrote: >>>>>>> On Fri, 12 May 2017 15:15:33 +0530 >>>>>>> Hari Bathini <hbathini@linux.vnet.ibm.com> wrote: >>>>>>>> On Thursday 11 May 2017 06:46 PM, Michal Suchánek wrote: >>>>>>>>> On Thu, 11 May 2017 02:00:11 +0530 >>>>>>>>> Hari Bathini <hbathini@linux.vnet.ibm.com> wrote: >>>>>>>>>> On Wednesday 10 May 2017 09:31 PM, Michal Suchánek wrote: >>>>>>>>>>> On Wed, 03 May 2017 23:52:52 +0530 >>>>>>>>>>> Hari Bathini <hbathini@linux.vnet.ibm.com> wrote: >>>>>>>>>>> >>>>>>>>>>>> With the introduction of 'fadump_append=' parameter to pass >>>>>>>>>>>> additional parameters to fadump (capture) kernel, update >>>>>>>>>>>> documentation about it. >>>>>>>>>>>> >>>>>>>>>>>> Signed-off-by: Hari Bathini <hbathini@linux.vnet.ibm.com> >>>>>>>>>>>> --- >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>> Writing your own deficient parser for comma separated >>>>>>>>>>> arguments when perfectly fine parser for space separated >>>>>>>>>>> quoted arguments exists in the kernel and the bootloader >>>>>>>>>>> does not seem like a good idea to me. >>>>>>>>>> Couple of things that prompted me for v5 are: >>>>>>>>>> 1. Using parse_early_options() limits the kind of >>>>>>>>>> parameters that can be passed to fadump capture kernel. >>>>>>>>>> Passing parameters like systemd.unit= & udev.childern.max= >>>>>>>>>> has no effect with v4. Updating >>>>>>>>>> boot_command_line parameter, when fadump is active, >>>>>>>>>> seems a better alternative. >>>>>>>>>> >>>>>>>>>> 2. Passing space-separated quoted arguments is not >>>>>>>>>> working as intended with lilo. Updating bootloader with the >>>>>>>>>> below entry in /etc/lilo.conf file results in a missing >>>>>>>>>> append entry in /etc/yaboot.conf file. >>>>>>>>>> >>>>>>>>>> append = "quiet sysrq=1 insmod=sym53c8xx >>>>>>>>>> insmod=ipr crashkernel=512M-:256M fadump_append=\"nr_cpus=1 >>>>>>>>>> numa=off udev.children-max=2\"" >>>>>>>>> Meaning that a script that emulates LILO semantics on top of >>>>>>>>> yaboot which is completely capable of passing qouted space >>>>>>>>> separated arguments fails. IMHO it is more reasonable to fix >>>>>>>>> the script or whatever adaptation layer or use yaboot >>>>>>>>> directly than working around bug in said script by >>>>>>>>> introducing a new argument parser in the kernel. >>>>>>>>> >>>>>>>>> >>>> The intention with this patch is to replace >>>> >>>> "root=/dev/sda2 ro fadump_append=nr_cpus=1,numa=off >>>> crashkernel=1024M" >>>> >>>> with >>>> >>>> "root=/dev/sda2 ro nr_cpus=1 numa=off crashkernel=1024M" >>>> >>>> when kernel is booting for dump capture. >>>> While parse_args() is making parsing relatively easy, the main >>>> idea is to replace parameters as above when fadump capture kernel >>>> is booting. The code is relatively complicated to replace >>>> parameters using space-separated >>>> quoted string even though parse_args() may parse them easily. >>>> Comma-separated >>>> list was easier to implement and seemed less error prone for >>>> replacing parameters. >>> You can still do that relatively easily. parse_args() gives you the >>> content of fadump_capture argument. You should probably add a >>> variant that gives you also the position of the fadump_capture >>> argument in case user specified it multiple times - picking the >>> wrong one would cause errors. >> There is no denying that this can be done with the use of >> parse_args() function. >> But my contention is a custom parser is no more error prone, >> considering it only has to search/replace commas in >> fadump_extra_args= till the first occurrence of space/nul, without >> having to deal with the naunces of parse_args() function. > If you do that you > > 1) introduce new parser which is error-prone even for not very complex > parsers > > 2) does not handle quoting properly meaning that > a) the passed arguments cannot include a comma (which is the case for > many existing kernel parameters) > b) your parser, parse_args and any parser analyzing the commandline > before you replace the arguments do not agree on the content and > length of the fadump_extra_args argument > > 3) you introduce completely new syntax for arguments which the user > has to research and understand for no good reason >> Also, to >> get the last occurence, could use something like >> get_last_crashkernel() instead of adding a variant to parse_args(). > Why are you talking about last occurence all the time? Does that mean > that if user specifies fadump_extra_args in both the 'default' and > 'extra' kernel arguments to the grub configuration script only the one > from 'extra' arguments will be expanded and the on from 'default' > arguments will be just left there? Why? > >>> Then you can just replace the fadump_append="appended arguments" >>> with the dequoted "appended arguments" parse_args() gives you. >>> Dequating should shorten the arguments as would removing the >>> fadump_append= prefix so it should be quite possible in-place. It >>> is in fact even easier than the comma separated list since you do >>> not need to do any parsing yourself - only strlen, memset and >>> memcpy. >>> >>> That said, if you replace the fadump_append argument the running >>> kernel will have extra arguments without knowing where they came >>> from making detection of this happening all the more difficult. >> How about replacing "fadump_extra_args=x,y,z" with "fadump_extra_args >> x y z"? > How does this handle x that includes a comma? > > How does this handle x that includes a space? > > How does it handle x that includes a double quote? - Well, presumably > it will just stay there with the fadump_extra_args expanded or not and > will cause equal breakage. > >> Presence of fadump_extra_args hints the user about the extra >> parameters passed >> to fadump capture kernel and also lets parsing/replacing be simple - >> replace the first '=' and subsequent commas with ' ' in this >> fadump_extra_args= parameter >> and we are good to go. No additional handling. Even if we go with >> space-separated >> quoted string, simply replacing "fadump_extra_args=" with >> "fadump_extra_args" > Leaving the empty fadump_extra_args argument might be good for > indication that the replacement possibly took place and comes for free. > >> should suffice, no need to worry about parse_args as well. > If you use parse_args it can give you the content of the every > fadump_extra_args argument and dequotes it for you. So in the case > fadump_extra_args does include double quotes you get them handled > correctly for you by existing parser. > > And you want to make a variant of parse_args that tells you where the > argument starts and ends in the parsed commandline so you know which > part you should replace - which it certainly does know but no current > users need the information so it does not tell you. > > When you do that users can read (or for the most part have already > read) about the kernel argument syntax and apply that to > fadump_extra_args. No additional specification and exceptions are > needed. > >> I prefer >> comma-separated >> list though, for it leaves no dependency on how bootloader handles >> space-separated >> quoted strings, considering my encounter with yast bootloader which >> is not so >> impressive in handling quoted strings. Also, the code change is not >> so complicated. > If yast cannot handle kernel parameters correctly report a bug. The > quoting of arguments with spaces is part of the kernel parameter > specification so yast should handle it. > >> >>>> Want to replace fadump_append=, to avoid command line overflow >>>> issues and also to not have to deal with special cases like --. >>>> Actually, fadump_append= >>>> seems to be confusing in that sense. How about >>>> fadump_args=comma-separated-list- >>>> of-params-for-capture-kernel ? Please share your thoughts. >>> The comma separated list still can contain a -- argument so you >>> still have to do something about that. Or not and just document >>> that people should not use it. >> The user has the flexibility to use fadump_extra_args= to pass >> special parameters >> keeping in mind that "fadump_extra_args=x,y,z" will be replaced >> in-place with >> "fadump_extra_args x y z" and it happens only in fadump capture >> kernel. No additional >> code changes needed to handle this except for documentation talking >> about it? > Presumably if y is "--" then z and any subsequent kernel arguments after > fadump_extra_args are passed to init when fadump_extra_args is > expanded and not so when left as is. This may be unexpected - the user > may expect that if fadump_extra_args contains a "--" as y then z is > appended after existing "--" in commandline or if none is present the > whole "-- z" is moved to the end. This would be clean way of handling > the extra arguments - no text only preprocessing allowing for dirty > tricks moving arguments from kernel to init by in-place text > replacement. Note that any unclosed quoting in arguments after > fadump_extra_args may make appending "-- z" quite difficult if you > wanted to do it correctly. If you do not want to bother (with either) > it is an understandable shortcoming in the implementation and should be > documented. > I would prefer documenting over a complex implementation. Actually, I am considering a simple approach of replacing every occurrence of "fadump_extra_args=" with "fadump_extra_args " in fadump capture kernel. The cmdline "root=/dev/sda2 ro fadump_extra_args="a b c" crashkernel=512M fadump_extra_args=d" becomes "root=/dev/sda2 ro fadump_extra_args "a b c" crashkernel=512M fadump_extra_args d" in fadump capture kernel. This must take care of the pitfalls with the current approach and also, doesn't rely on parse_args() which was not designed for this scenario to start with..? Will report a bug for yast handling of kernel parameters with quotes. Thanks Hari
Hello, On Wed, 12 Jul 2017 00:00:57 +0530 Hari Bathini <hbathini@linux.vnet.ibm.com> wrote: > Hi Michal, > > > Thanks for the review.. > > > On Monday 26 June 2017 05:45 PM, Michal Suchánek wrote: > > Hello, > > > > On Tue, 20 Jun 2017 21:14:08 +0530 > > Hari Bathini <hbathini@linux.vnet.ibm.com> wrote: > > > I would prefer documenting over a complex implementation. Actually, I > am considering a simple approach of replacing every occurrence of > "fadump_extra_args=" with "fadump_extra_args " in fadump capture > kernel. The cmdline > > "root=/dev/sda2 ro fadump_extra_args="a b c" crashkernel=512M > fadump_extra_args=d" > > becomes > > "root=/dev/sda2 ro fadump_extra_args "a b c" crashkernel=512M > fadump_extra_args d" which is totally broken > > in fadump capture kernel. This must take care of the pitfalls with > the current approach and also, > doesn't rely on parse_args() which was not designed for this scenario > to start with..? It was designed for parsing arguments. To handle replacing arguments you have to extend it. You need to get more information from it for this case. Best regards Michal
On Wednesday 12 July 2017 05:01 PM, msuchanek wrote: > Hello, > > On Wed, 12 Jul 2017 00:00:57 +0530 > Hari Bathini <hbathini@linux.vnet.ibm.com> wrote: > >> Hi Michal, >> >> >> Thanks for the review.. >> >> >> On Monday 26 June 2017 05:45 PM, Michal Suchánek wrote: >>> Hello, >>> >>> On Tue, 20 Jun 2017 21:14:08 +0530 >>> Hari Bathini <hbathini@linux.vnet.ibm.com> wrote: >>> >> I would prefer documenting over a complex implementation. Actually, I >> am considering a simple approach of replacing every occurrence of >> "fadump_extra_args=" with "fadump_extra_args " in fadump capture >> kernel. The cmdline >> >> "root=/dev/sda2 ro fadump_extra_args="a b c" crashkernel=512M >> fadump_extra_args=d" >> >> becomes >> >> "root=/dev/sda2 ro fadump_extra_args "a b c" crashkernel=512M >> fadump_extra_args d" > which is totally broken My bad! I don't know what I was thinking when I expected that to work.. :-[ >> in fadump capture kernel. This must take care of the pitfalls with >> the current approach and also, >> doesn't rely on parse_args() which was not designed for this scenario >> to start with..? > It was designed for parsing arguments. To handle replacing arguments > you have to extend it. You need to get more information from it for > this case. I will try to work on this. Also, want to replace in-place without moving other parameters. I guess, I could do that by replacing the leftover length after processing with spaces.. Thanks Hari
diff --git a/Documentation/powerpc/firmware-assisted-dump.txt b/Documentation/powerpc/firmware-assisted-dump.txt index 8394bc8..6327193 100644 --- a/Documentation/powerpc/firmware-assisted-dump.txt +++ b/Documentation/powerpc/firmware-assisted-dump.txt @@ -162,7 +162,15 @@ 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 comma + separated list through 'fadump_append=' parameter, to be enforced + when fadump is active. For example, if parameters like nr_cpus=1, + numa=off & udev.children-max=2 are to be enforced when fadump is + active, 'fadump_append=nr_cpus=1,numa=off,udev.children-max=2' + can be passed in command line, which will be replaced with + "nr_cpus=1 numa=off udev.children-max=2" when fadump is active. + This helps in reducing memory consumption during dump capture. +4. Optionally, user can also set 'crashkernel=' kernel cmdline to specify size of the memory to reserve for boot memory dump preservation.
With the introduction of 'fadump_append=' parameter to pass additional parameters to fadump (capture) kernel, update documentation about it. Signed-off-by: Hari Bathini <hbathini@linux.vnet.ibm.com> --- Changes from v4: * Based on top of patchset that includes https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?h=akpm&id=05f383cdfba8793240e73f9a9fbff4e25d66003f Documentation/powerpc/firmware-assisted-dump.txt | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)