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

[Michal Suchánek]

Hello,

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