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

["Michal Suchánek" <msuchanek@suse.de>]

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=C3=A1nek wrote:
> > On Thu, 11 May 2017 02:00:11 +0530
> > Hari Bathini <hbathini@linux.vnet.ibm.com> wrote:
> > =20
> >> Hello Michal,
> >>
> >> On Wednesday 10 May 2017 09:31 PM, Michal Such=C3=A1nek wrote: =20
> >>> Hello,
> >>>
> >>> On Wed, 03 May 2017 23:52:52 +0530
> >>> Hari Bathini <hbathini@linux.vnet.ibm.com> wrote:
> >>>    =20
> >>>> With the introduction of 'fadump_append=3D' 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=3Dakpm&id=3D05f383cdfba8793240e73f9a9fbff4e25d66003f
> >>>>
> >>>>
> >>>>    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=3Dy and build kernel.
> >>>>    2. Boot into linux kernel with 'fadump=3Don' kernel cmdline
> >>>> option. -3. Optionally, user can also set 'crashkernel=3D' kernel
> >>>> cmdline +3. A user can pass additional command line parameters
> >>>> as a comma
> >>>> +   separated list through 'fadump_append=3D' parameter, to be
> >>>> enforced
> >>>> +   when fadump is active. For example, if parameters like
> >>>> nr_cpus=3D1,
> >>>> +   numa=3Doff & udev.children-max=3D2 are to be enforced when fadump
> >>>> is
> >>>> +   active,
> >>>> 'fadump_append=3Dnr_cpus=3D1,numa=3Doff,udev.children-max=3D2'
> >>>> +   can be passed in command line, which will be replaced with
> >>>> +   "nr_cpus=3D1 numa=3Doff udev.children-max=3D2" when fadump is
> >>>> active.
> >>>> +   This helps in reducing memory consumption during dump
> >>>> capture. +4. Optionally, user can also set 'crashkernel=3D' kernel
> >>>> cmdline to specify size of the memory to reserve for boot memory
> >>>> dump preservation.
> >>>>   =20
> >>>>    =20
> >>> 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. =20
> >>
> >> 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=3D & udev.childern.max=3D 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 =3D "quiet sysrq=3D1 insmod=3Dsym53c8xx insmod=3Dipr
> >> crashkernel=3D512M-:256M fadump_append=3D\"nr_cpus=3D1 numa=3Doff
> >> udev.children-max=3D2\"" =20
> > 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.
> >
> > =20
>=20
> 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