At present, both return and crash_size should be checked to guarantee the
success of parse_crashkernel().
Take a close look at the cases, which causes crash_size=0. Beside syntax
error, three cases cause parsing to get crash_size=0.
-1st. in parse_crashkernel_mem(), the demanded crash size is bigger than
system ram.
-2nd. in parse_crashkernel_mem(), the system ram size does not match any
item in the range list.
-3rd. "crashkernel=0MB", which is impractical.
All these cases can be treated as invalid argument.
By this way, only need a simple check on return value of
parse_crashkernel().
Signed-off-by: Pingfan Liu <[email protected]>
Cc: Russell King <[email protected]>
Cc: Catalin Marinas <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: Tony Luck <[email protected]>
Cc: Fenghua Yu <[email protected]>
Cc: Ralf Baechle <[email protected]>
Cc: Paul Burton <[email protected]>
Cc: James Hogan <[email protected]>
Cc: Benjamin Herrenschmidt <[email protected]>
Cc: Paul Mackerras <[email protected]>
Cc: Michael Ellerman <[email protected]>
Cc: Martin Schwidefsky <[email protected]>
Cc: Heiko Carstens <[email protected]>
Cc: Yoshinori Sato <[email protected]>
Cc: Rich Felker <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: "H. Peter Anvin" <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Julien Thierry <[email protected]>
Cc: Palmer Dabbelt <[email protected]>
Cc: Ard Biesheuvel <[email protected]>
Cc: Florian Fainelli <[email protected]>
Cc: Logan Gunthorpe <[email protected]>
Cc: Robin Murphy <[email protected]>
Cc: Greg Hackmann <[email protected]>
Cc: Stefan Agner <[email protected]>
Cc: Johannes Weiner <[email protected]>
Cc: David Hildenbrand <[email protected]>
Cc: Jens Axboe <[email protected]>
Cc: Thomas Bogendoerfer <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Hari Bathini <[email protected]>
Cc: Ananth N Mavinakayanahalli <[email protected]>
Cc: Yangtao Li <[email protected]>
Cc: Dave Young <[email protected]>
Cc: Baoquan He <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
---
v1 -> v2: On error, return -EINVAL for all failure cases
arch/arm/kernel/setup.c | 2 +-
arch/arm64/mm/init.c | 2 +-
arch/ia64/kernel/setup.c | 2 +-
arch/mips/kernel/setup.c | 2 +-
arch/powerpc/kernel/fadump.c | 2 +-
arch/powerpc/kernel/machine_kexec.c | 2 +-
arch/s390/kernel/setup.c | 2 +-
arch/sh/kernel/machine_kexec.c | 2 +-
arch/x86/kernel/setup.c | 4 ++--
kernel/crash_core.c | 10 +++++++++-
10 files changed, 19 insertions(+), 11 deletions(-)
diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
index 5d78b6a..2feab13 100644
--- a/arch/arm/kernel/setup.c
+++ b/arch/arm/kernel/setup.c
@@ -997,7 +997,7 @@ static void __init reserve_crashkernel(void)
total_mem = get_total_mem();
ret = parse_crashkernel(boot_command_line, total_mem,
&crash_size, &crash_base);
- if (ret)
+ if (ret < 0)
return;
if (crash_base <= 0) {
diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
index 6bc1350..240918c 100644
--- a/arch/arm64/mm/init.c
+++ b/arch/arm64/mm/init.c
@@ -79,7 +79,7 @@ static void __init reserve_crashkernel(void)
ret = parse_crashkernel(boot_command_line, memblock_phys_mem_size(),
&crash_size, &crash_base);
/* no crashkernel= or invalid value specified */
- if (ret || !crash_size)
+ if (ret < 0)
return;
crash_size = PAGE_ALIGN(crash_size);
diff --git a/arch/ia64/kernel/setup.c b/arch/ia64/kernel/setup.c
index 583a374..3bbb58b 100644
--- a/arch/ia64/kernel/setup.c
+++ b/arch/ia64/kernel/setup.c
@@ -277,7 +277,7 @@ static void __init setup_crashkernel(unsigned long total, int *n)
ret = parse_crashkernel(boot_command_line, total,
&size, &base);
- if (ret == 0 && size > 0) {
+ if (!ret) {
if (!base) {
sort_regions(rsvd_region, *n);
*n = merge_regions(rsvd_region, *n);
diff --git a/arch/mips/kernel/setup.c b/arch/mips/kernel/setup.c
index 8d1dc6c..168571b 100644
--- a/arch/mips/kernel/setup.c
+++ b/arch/mips/kernel/setup.c
@@ -715,7 +715,7 @@ static void __init mips_parse_crashkernel(void)
total_mem = get_total_mem();
ret = parse_crashkernel(boot_command_line, total_mem,
&crash_size, &crash_base);
- if (ret != 0 || crash_size <= 0)
+ if (ret < 0)
return;
if (!memory_region_available(crash_base, crash_size)) {
diff --git a/arch/powerpc/kernel/fadump.c b/arch/powerpc/kernel/fadump.c
index 45a8d0b..3571504 100644
--- a/arch/powerpc/kernel/fadump.c
+++ b/arch/powerpc/kernel/fadump.c
@@ -376,7 +376,7 @@ static inline unsigned long fadump_calculate_reserve_size(void)
*/
ret = parse_crashkernel(boot_command_line, memblock_phys_mem_size(),
&size, &base);
- if (ret == 0 && size > 0) {
+ if (!ret) {
unsigned long max_size;
if (fw_dump.reserve_bootvar)
diff --git a/arch/powerpc/kernel/machine_kexec.c b/arch/powerpc/kernel/machine_kexec.c
index 63f5a93..1697ad2 100644
--- a/arch/powerpc/kernel/machine_kexec.c
+++ b/arch/powerpc/kernel/machine_kexec.c
@@ -122,7 +122,7 @@ void __init reserve_crashkernel(void)
/* use common parsing */
ret = parse_crashkernel(boot_command_line, memblock_phys_mem_size(),
&crash_size, &crash_base);
- if (ret == 0 && crash_size > 0) {
+ if (!ret) {
crashk_res.start = crash_base;
crashk_res.end = crash_base + crash_size - 1;
}
diff --git a/arch/s390/kernel/setup.c b/arch/s390/kernel/setup.c
index 2c642af..d4bd61b 100644
--- a/arch/s390/kernel/setup.c
+++ b/arch/s390/kernel/setup.c
@@ -671,7 +671,7 @@ static void __init reserve_crashkernel(void)
crash_base = ALIGN(crash_base, KEXEC_CRASH_MEM_ALIGN);
crash_size = ALIGN(crash_size, KEXEC_CRASH_MEM_ALIGN);
- if (rc || crash_size == 0)
+ if (rc < 0)
return;
if (memblock.memory.regions[0].size < crash_size) {
diff --git a/arch/sh/kernel/machine_kexec.c b/arch/sh/kernel/machine_kexec.c
index 63d63a3..3c03240 100644
--- a/arch/sh/kernel/machine_kexec.c
+++ b/arch/sh/kernel/machine_kexec.c
@@ -157,7 +157,7 @@ void __init reserve_crashkernel(void)
ret = parse_crashkernel(boot_command_line, memblock_phys_mem_size(),
&crash_size, &crash_base);
- if (ret == 0 && crash_size > 0) {
+ if (!ret) {
crashk_res.start = crash_base;
crashk_res.end = crash_base + crash_size - 1;
}
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index 3d872a5..592d5ad 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -526,11 +526,11 @@ static void __init reserve_crashkernel(void)
/* crashkernel=XM */
ret = parse_crashkernel(boot_command_line, total_mem, &crash_size, &crash_base);
- if (ret != 0 || crash_size <= 0) {
+ if (ret < 0) {
/* crashkernel=X,high */
ret = parse_crashkernel_high(boot_command_line, total_mem,
&crash_size, &crash_base);
- if (ret != 0 || crash_size <= 0)
+ if (ret < 0)
return;
high = true;
}
diff --git a/kernel/crash_core.c b/kernel/crash_core.c
index 093c9f9..83ee4a9 100644
--- a/kernel/crash_core.c
+++ b/kernel/crash_core.c
@@ -108,8 +108,10 @@ static int __init parse_crashkernel_mem(char *cmdline,
return -EINVAL;
}
}
- } else
+ } else {
pr_info("crashkernel size resulted in zero bytes\n");
+ return -EINVAL;
+ }
return 0;
}
@@ -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;
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;
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,
--
2.7.4
On 24/04/2019 08:33, Pingfan Liu wrote:
> At present, both return and crash_size should be checked to guarantee the
> success of parse_crashkernel().
>
> Take a close look at the cases, which causes crash_size=0. Beside syntax
> error, three cases cause parsing to get crash_size=0.
> -1st. in parse_crashkernel_mem(), the demanded crash size is bigger than
> system ram.
> -2nd. in parse_crashkernel_mem(), the system ram size does not match any
> item in the range list.
> -3rd. "crashkernel=0MB", which is impractical.
>
> All these cases can be treated as invalid argument.
>
> By this way, only need a simple check on return value of
> parse_crashkernel().
>
> Signed-off-by: Pingfan Liu <[email protected]>
> Cc: Russell King <[email protected]>
> Cc: Catalin Marinas <[email protected]>
> Cc: Will Deacon <[email protected]>
> Cc: Tony Luck <[email protected]>
> Cc: Fenghua Yu <[email protected]>
> Cc: Ralf Baechle <[email protected]>
> Cc: Paul Burton <[email protected]>
> Cc: James Hogan <[email protected]>
> Cc: Benjamin Herrenschmidt <[email protected]>
> Cc: Paul Mackerras <[email protected]>
> Cc: Michael Ellerman <[email protected]>
> Cc: Martin Schwidefsky <[email protected]>
> Cc: Heiko Carstens <[email protected]>
> Cc: Yoshinori Sato <[email protected]>
> Cc: Rich Felker <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: Borislav Petkov <[email protected]>
> Cc: "H. Peter Anvin" <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Cc: Julien Thierry <[email protected]>
> Cc: Palmer Dabbelt <[email protected]>
> Cc: Ard Biesheuvel <[email protected]>
> Cc: Florian Fainelli <[email protected]>
> Cc: Logan Gunthorpe <[email protected]>
> Cc: Robin Murphy <[email protected]>
> Cc: Greg Hackmann <[email protected]>
> Cc: Stefan Agner <[email protected]>
> Cc: Johannes Weiner <[email protected]>
> Cc: David Hildenbrand <[email protected]>
> Cc: Jens Axboe <[email protected]>
> Cc: Thomas Bogendoerfer <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: Hari Bathini <[email protected]>
> Cc: Ananth N Mavinakayanahalli <[email protected]>
> Cc: Yangtao Li <[email protected]>
> Cc: Dave Young <[email protected]>
> Cc: Baoquan He <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> ---
> v1 -> v2: On error, return -EINVAL for all failure cases
>
> arch/arm/kernel/setup.c | 2 +-
> arch/arm64/mm/init.c | 2 +-
> arch/ia64/kernel/setup.c | 2 +-
> arch/mips/kernel/setup.c | 2 +-
> arch/powerpc/kernel/fadump.c | 2 +-
> arch/powerpc/kernel/machine_kexec.c | 2 +-
> arch/s390/kernel/setup.c | 2 +-
> arch/sh/kernel/machine_kexec.c | 2 +-
> arch/x86/kernel/setup.c | 4 ++--
> kernel/crash_core.c | 10 +++++++++-
> 10 files changed, 19 insertions(+), 11 deletions(-)
>
> diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
> index 5d78b6a..2feab13 100644
> --- a/arch/arm/kernel/setup.c
> +++ b/arch/arm/kernel/setup.c
> @@ -997,7 +997,7 @@ static void __init reserve_crashkernel(void)
> total_mem = get_total_mem();
> ret = parse_crashkernel(boot_command_line, total_mem,
> &crash_size, &crash_base);
> - if (ret)
> + if (ret < 0)
> return;
>
> if (crash_base <= 0) {
> diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
> index 6bc1350..240918c 100644
> --- a/arch/arm64/mm/init.c
> +++ b/arch/arm64/mm/init.c
> @@ -79,7 +79,7 @@ static void __init reserve_crashkernel(void)
> ret = parse_crashkernel(boot_command_line, memblock_phys_mem_size(),
> &crash_size, &crash_base);
> /* no crashkernel= or invalid value specified */
> - if (ret || !crash_size)
> + if (ret < 0)
> return;
>
> crash_size = PAGE_ALIGN(crash_size);
> diff --git a/arch/ia64/kernel/setup.c b/arch/ia64/kernel/setup.c
> index 583a374..3bbb58b 100644
> --- a/arch/ia64/kernel/setup.c
> +++ b/arch/ia64/kernel/setup.c
> @@ -277,7 +277,7 @@ static void __init setup_crashkernel(unsigned long total, int *n)
>
> ret = parse_crashkernel(boot_command_line, total,
> &size, &base);
> - if (ret == 0 && size > 0) {
> + if (!ret) {
> if (!base) {
> sort_regions(rsvd_region, *n);
> *n = merge_regions(rsvd_region, *n);
> diff --git a/arch/mips/kernel/setup.c b/arch/mips/kernel/setup.c
> index 8d1dc6c..168571b 100644
> --- a/arch/mips/kernel/setup.c
> +++ b/arch/mips/kernel/setup.c
> @@ -715,7 +715,7 @@ static void __init mips_parse_crashkernel(void)
> total_mem = get_total_mem();
> ret = parse_crashkernel(boot_command_line, total_mem,
> &crash_size, &crash_base);
> - if (ret != 0 || crash_size <= 0)
> + if (ret < 0)
> return;
>
> if (!memory_region_available(crash_base, crash_size)) {
> diff --git a/arch/powerpc/kernel/fadump.c b/arch/powerpc/kernel/fadump.c
> index 45a8d0b..3571504 100644
> --- a/arch/powerpc/kernel/fadump.c
> +++ b/arch/powerpc/kernel/fadump.c
> @@ -376,7 +376,7 @@ static inline unsigned long fadump_calculate_reserve_size(void)
> */
> ret = parse_crashkernel(boot_command_line, memblock_phys_mem_size(),
> &size, &base);
> - if (ret == 0 && size > 0) {
> + if (!ret) {
> unsigned long max_size;
>
> if (fw_dump.reserve_bootvar)
> diff --git a/arch/powerpc/kernel/machine_kexec.c b/arch/powerpc/kernel/machine_kexec.c
> index 63f5a93..1697ad2 100644
> --- a/arch/powerpc/kernel/machine_kexec.c
> +++ b/arch/powerpc/kernel/machine_kexec.c
> @@ -122,7 +122,7 @@ void __init reserve_crashkernel(void)
> /* use common parsing */
> ret = parse_crashkernel(boot_command_line, memblock_phys_mem_size(),
> &crash_size, &crash_base);
> - if (ret == 0 && crash_size > 0) {
> + if (!ret) {
> crashk_res.start = crash_base;
> crashk_res.end = crash_base + crash_size - 1;
> }
> diff --git a/arch/s390/kernel/setup.c b/arch/s390/kernel/setup.c
> index 2c642af..d4bd61b 100644
> --- a/arch/s390/kernel/setup.c
> +++ b/arch/s390/kernel/setup.c
> @@ -671,7 +671,7 @@ static void __init reserve_crashkernel(void)
>
> crash_base = ALIGN(crash_base, KEXEC_CRASH_MEM_ALIGN);
> crash_size = ALIGN(crash_size, KEXEC_CRASH_MEM_ALIGN);
> - if (rc || crash_size == 0)
> + if (rc < 0)
> return;
>
> if (memblock.memory.regions[0].size < crash_size) {
> diff --git a/arch/sh/kernel/machine_kexec.c b/arch/sh/kernel/machine_kexec.c
> index 63d63a3..3c03240 100644
> --- a/arch/sh/kernel/machine_kexec.c
> +++ b/arch/sh/kernel/machine_kexec.c
> @@ -157,7 +157,7 @@ void __init reserve_crashkernel(void)
>
> ret = parse_crashkernel(boot_command_line, memblock_phys_mem_size(),
> &crash_size, &crash_base);
> - if (ret == 0 && crash_size > 0) {
> + if (!ret) {
> crashk_res.start = crash_base;
> crashk_res.end = crash_base + crash_size - 1;
> }
> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> index 3d872a5..592d5ad 100644
> --- a/arch/x86/kernel/setup.c
> +++ b/arch/x86/kernel/setup.c
> @@ -526,11 +526,11 @@ static void __init reserve_crashkernel(void)
>
> /* crashkernel=XM */
> ret = parse_crashkernel(boot_command_line, total_mem, &crash_size, &crash_base);
> - if (ret != 0 || crash_size <= 0) {
> + if (ret < 0) {
> /* crashkernel=X,high */
> ret = parse_crashkernel_high(boot_command_line, total_mem,
> &crash_size, &crash_base);
> - if (ret != 0 || crash_size <= 0)
> + if (ret < 0)
> return;
> high = true;
> }
> diff --git a/kernel/crash_core.c b/kernel/crash_core.c
> index 093c9f9..83ee4a9 100644
> --- a/kernel/crash_core.c
> +++ b/kernel/crash_core.c
> @@ -108,8 +108,10 @@ static int __init parse_crashkernel_mem(char *cmdline,
> return -EINVAL;
> }
> }
> - } else
> + } else {
> pr_info("crashkernel size resulted in zero bytes\n");
> + return -EINVAL;
> + }
>
> return 0;
> }
> @@ -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.
>
> 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,
>
On Wed, Apr 24, 2019 at 4:31 PM Matthias Brugger <[email protected]> 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,
> >
On 04/25/19 at 04:20pm, Pingfan Liu wrote:
> On Wed, Apr 24, 2019 at 4:31 PM Matthias Brugger <[email protected]> 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.
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.
Thanks
Dave
On Sun, Apr 28, 2019 at 4:37 PM Dave Young <[email protected]> wrote:
>
> On 04/25/19 at 04:20pm, Pingfan Liu wrote:
> > On Wed, Apr 24, 2019 at 4:31 PM Matthias Brugger <[email protected]> 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.
>
> 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.
Regards,
Pingfan
On Mon, Apr 29, 2019 at 11:04 AM Pingfan Liu <[email protected]> wrote:
>
> On Sun, Apr 28, 2019 at 4:37 PM Dave Young <[email protected]> wrote:
> >
> > On 04/25/19 at 04:20pm, Pingfan Liu wrote:
> > > On Wed, Apr 24, 2019 at 4:31 PM Matthias Brugger <[email protected]> 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.
> >
> > 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?
Thanks,
Pingfan
On 04/29/19 at 12:48pm, Pingfan Liu wrote:
> On Mon, Apr 29, 2019 at 11:04 AM Pingfan Liu <[email protected]> wrote:
> >
> > On Sun, Apr 28, 2019 at 4:37 PM Dave Young <[email protected]> wrote:
> > >
> > > On 04/25/19 at 04:20pm, Pingfan Liu wrote:
> > > > On Wed, Apr 24, 2019 at 4:31 PM Matthias Brugger <[email protected]> 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.
> > >
> > > 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
On Thu, Apr 25, 2019 at 4:20 PM Pingfan Liu <[email protected]> wrote:
>
> On Wed, Apr 24, 2019 at 4:31 PM Matthias Brugger <[email protected]> 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.
After a series of work, I suddenly realized that it can not be done
like this way. "0M" causes kstrtoull() to return -EINVAL, but this is
caused by "M", not "0". If passing "0" to kstrtoull(), it will return
0 on success.
> >
> It seems that kstrtoull() treats 0M as invalid parameter, while
> simple_strtoull() does not.
>
My careless going through the code. And I tested with a valid value
"256M" using kstrtoull(), it also returned -EINVAL.
So I think there is no way to distinguish 0 from a positive value
inside this basic math function.
Do I miss anything?
Thanks and regards,
Pingfan
Matthias, ping? Any suggestions?
Thanks,
Pingfan
On Thu, May 2, 2019 at 2:22 PM Pingfan Liu <[email protected]> wrote:
>
> On Thu, Apr 25, 2019 at 4:20 PM Pingfan Liu <[email protected]> wrote:
> >
> > On Wed, Apr 24, 2019 at 4:31 PM Matthias Brugger <[email protected]> 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.
> After a series of work, I suddenly realized that it can not be done
> like this way. "0M" causes kstrtoull() to return -EINVAL, but this is
> caused by "M", not "0". If passing "0" to kstrtoull(), it will return
> 0 on success.
>
> > >
> > It seems that kstrtoull() treats 0M as invalid parameter, while
> > simple_strtoull() does not.
> >
> My careless going through the code. And I tested with a valid value
> "256M" using kstrtoull(), it also returned -EINVAL.
>
> So I think there is no way to distinguish 0 from a positive value
> inside this basic math function.
> Do I miss anything?
>
> Thanks and regards,
> Pingfan