[v5,2/2] powerpc/fadump: update documentation about 'fadump_append=' parameter

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(-)

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