Re: [PATCHv2] kernel/crash: make parse_crashkernel()'s return value more indicant

[Pingfan Liu]

On Wed, Apr 24, 2019 at 4:31 PM Matthias Brugger <mbrugger@xxxxxxxx> wrote:
>
>
[...]
> > @@ -139,6 +141,8 @@ static int __init parse_crashkernel_simple(char *cmdline,
> >               pr_warn("crashkernel: unrecognized char: %c\n", *cur);
> >               return -EINVAL;
> >       }
> > +     if (*crash_size == 0)
> > +             return -EINVAL;
>
> This covers the case where I pass an argument like "crashkernel=0M" ?
> Can't we fix that by using kstrtoull() in memparse and check if the return value
> is < 0? In that case we could return without updating the retptr and we will be
> fine.
>
It seems that kstrtoull() treats 0M as invalid parameter, while
simple_strtoull() does not.

If changed like your suggestion, then all the callers of memparse()
will treats 0M as invalid parameter. This affects many components
besides kexec.  Not sure this can be done or not.

Regards,
Pingfan

> >
> >       return 0;
> >  }
> > @@ -181,6 +185,8 @@ static int __init parse_crashkernel_suffix(char *cmdline,
> >               pr_warn("crashkernel: unrecognized char: %c\n", *cur);
> >               return -EINVAL;
> >       }
> > +     if (*crash_size == 0)
> > +             return -EINVAL;
>
> Same here.
>
> >
> >       return 0;
> >  }
> > @@ -266,6 +272,8 @@ static int __init __parse_crashkernel(char *cmdline,
> >  /*
> >   * That function is the entry point for command line parsing and should be
> >   * called from the arch-specific code.
> > + * On success 0. On error for either syntax error or crash_size=0, -EINVAL is
> > + * returned.
> >   */
> >  int __init parse_crashkernel(char *cmdline,
> >                            unsigned long long system_ram,
> >