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

[Michal Suchánek]

On Fri, 12 May 2017 15:15:33 +0530
Hari Bathini <hbathini at 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 at 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 at 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 at 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