On 04/29/19 at 12:48pm, Pingfan Liu wrote:

On Mon, Apr 29, 2019 at 11:04 AM Pingfan Liu [off-list ref] wrote:

quoted

On Sun, Apr 28, 2019 at 4:37 PM Dave Young [off-list ref] wrote:

quoted

On 04/25/19 at 04:20pm, Pingfan Liu wrote:

quoted

On Wed, Apr 24, 2019 at 4:31 PM Matthias Brugger [off-list ref] wrote:

quoted

[...]

quoted

quoted

@@ -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.

simple_strtoull is obsolete, move to kstrtoull is the right way.

$ git grep memparse|wc
    158     950   10479

Except some documentation/tools etc there are still a log of callers
which directly use the return value as the ull number without error
checking.

So it would be good to mark memparse as obsolete as well in
lib/cmdline.c, and introduce a new function eg. kmemparse() to use
kstrtoull,  and return a real error code, and save the size in an
argument like &size.  Then update X86 crashkernel code to use it.

Thank for your good suggestion.

Go through the v5.0 kernel code, I think it will be a huge job.

The difference between unsigned long long simple_strtoull(const char
*cp, char **endp, unsigned int base) and int _kstrtoull(const char *s,
unsigned int base, unsigned long long *res) is bigger than expected,
especially the output parameter @res. Many references to
memparse(const char *ptr, char **retptr) rely on @retptr to work. A
typical example from arch/x86/kernel/e820.c
        mem_size = memparse(p, &p);
        if (p == oldp)
                return -EINVAL;

        userdef = 1;
        if (*p == '@') {  <----------- here
                start_at = memparse(p+1, &p);
                e820__range_add(start_at, mem_size, E820_TYPE_RAM);
        } else if (*p == '#') {
                start_at = memparse(p+1, &p);
                e820__range_add(start_at, mem_size, E820_TYPE_ACPI);
        } else if (*p == '$') {
                start_at = memparse(p+1, &p);
                e820__range_add(start_at, mem_size, E820_TYPE_RESERVED);
        }

So we need to resolve the prototype of kstrtoull() firstly, and maybe
kstrtouint() etc too. All of them have lots of references in kernel.

Any idea about this?

Not only this place, a lot of other places, I think no hurry to fix them
all at one time.

As we talked just do it according to previous reply,  mark memparse as
obsolete, and create a new function to use kstrtoull, and make it used
in crashkernel code first.

Thanks
Dave