From: He Zhe <[email protected]>
debug_guardpage_minorder_setup and cmdline_parse_kernelcore do not check
input argument before using it. The argument would be a NULL pointer if
"debug_guardpage_minorder" or "kernelcore", without its value, is set in
command line and thus causes the following panic.
PANIC: early exception 0xe3 IP 10:ffffffffa08146f1 error 0 cr2 0x0
[ 0.000000] CPU: 0 PID: 0 Comm: swapper Not tainted 4.19.0-rc4-yocto-standard+ #11
[ 0.000000] RIP: 0010:parse_option_str+0x11/0x90
...
[ 0.000000] Call Trace:
[ 0.000000] cmdline_parse_kernelcore+0x19/0x41
[ 0.000000] do_early_param+0x57/0x8e
[ 0.000000] parse_args+0x208/0x320
[ 0.000000] ? rdinit_setup+0x30/0x30
[ 0.000000] parse_early_options+0x29/0x2d
[ 0.000000] ? rdinit_setup+0x30/0x30
[ 0.000000] parse_early_param+0x36/0x4d
[ 0.000000] setup_arch+0x336/0x99e
[ 0.000000] start_kernel+0x6f/0x4ee
[ 0.000000] x86_64_start_reservations+0x24/0x26
[ 0.000000] x86_64_start_kernel+0x6f/0x72
[ 0.000000] secondary_startup_64+0xa4/0xb0
This patch adds a check to prevent the panic and adds KBUILD_MODNAME to
prints.
Signed-off-by: He Zhe <[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]
Cc: [email protected]
---
v2:
Use more clear error info
Split the addition of KBUILD_MODNAME out
mm/page_alloc.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 89d2a2a..f34cae1 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -630,6 +630,12 @@ static int __init debug_guardpage_minorder_setup(char *buf)
{
unsigned long res;
+ if (!buf) {
+ pr_err("kernel option debug_guardpage_minorder requires an \
+ argument\n");
+ return -EINVAL;
+ }
+
if (kstrtoul(buf, 10, &res) < 0 || res > MAX_ORDER / 2) {
pr_err("Bad debug_guardpage_minorder value\n");
return 0;
@@ -6952,6 +6958,11 @@ static int __init cmdline_parse_core(char *p, unsigned long *core,
*/
static int __init cmdline_parse_kernelcore(char *p)
{
+ if (!p) {
+ pr_err("kernel option kernelcore requires an argument\n");
+ return -EINVAL;
+ }
+
/* parse kernelcore=mirror */
if (parse_option_str(p, "mirror")) {
mirrored_kernelcore = true;
--
2.7.4
From: He Zhe <[email protected]>
Add KBUILD_MODNAME to make prints more clear.
Signed-off-by: He Zhe <[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]
---
v2:
Split the addition of KBUILD_MODNAME out
mm/page_alloc.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index f34cae1..ead9556 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -14,6 +14,8 @@
* (lots of bits borrowed from Ingo Molnar & Andrew Morton)
*/
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
#include <linux/stddef.h>
#include <linux/mm.h>
#include <linux/swap.h>
--
2.7.4
On Sat 22-09-18 22:53:32, [email protected] wrote:
> From: He Zhe <[email protected]>
>
> debug_guardpage_minorder_setup and cmdline_parse_kernelcore do not check
> input argument before using it. The argument would be a NULL pointer if
> "debug_guardpage_minorder" or "kernelcore", without its value, is set in
> command line and thus causes the following panic.
>
> PANIC: early exception 0xe3 IP 10:ffffffffa08146f1 error 0 cr2 0x0
> [ 0.000000] CPU: 0 PID: 0 Comm: swapper Not tainted 4.19.0-rc4-yocto-standard+ #11
> [ 0.000000] RIP: 0010:parse_option_str+0x11/0x90
> ...
> [ 0.000000] Call Trace:
> [ 0.000000] cmdline_parse_kernelcore+0x19/0x41
> [ 0.000000] do_early_param+0x57/0x8e
> [ 0.000000] parse_args+0x208/0x320
> [ 0.000000] ? rdinit_setup+0x30/0x30
> [ 0.000000] parse_early_options+0x29/0x2d
> [ 0.000000] ? rdinit_setup+0x30/0x30
> [ 0.000000] parse_early_param+0x36/0x4d
> [ 0.000000] setup_arch+0x336/0x99e
> [ 0.000000] start_kernel+0x6f/0x4ee
> [ 0.000000] x86_64_start_reservations+0x24/0x26
> [ 0.000000] x86_64_start_kernel+0x6f/0x72
> [ 0.000000] secondary_startup_64+0xa4/0xb0
>
> This patch adds a check to prevent the panic
Is this something we deeply care about? The kernel command line
interface is to be used by admins who know what they are doing. Using
random or wrong values for these parameters can have detrimental effects
on the system. This particular case would blow up early, good. At least
it is visible immediately. This and many other parameters could have a
seemingly valid input (e.g. not a missing value) and subtle runtime
effect. You won't blow up immediately but the system is hardly usable
and the early checking cannot possible catch all those cases. Take a
mem=$N copied from one machine to another with a different memory
layout. While 2G can be perfectly fine on one a different machine might
result on a completely unusable system because the available RAM is
place higher.
So I am really wondering. Do we really want a lot of code to catch
kernel command line incorrect inputs? Does it really lead to better
quality overall? IMHO, we do have a proper documentation and we should
trust those starting the kernel.
> and adds KBUILD_MODNAME to prints.
This doesn't seem to be done in this patch. Probably a left over
from the previous version.
> Signed-off-by: He Zhe <[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]
> Cc: [email protected]
> ---
> v2:
> Use more clear error info
> Split the addition of KBUILD_MODNAME out
>
> mm/page_alloc.c | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 89d2a2a..f34cae1 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -630,6 +630,12 @@ static int __init debug_guardpage_minorder_setup(char *buf)
> {
> unsigned long res;
>
> + if (!buf) {
> + pr_err("kernel option debug_guardpage_minorder requires an \
> + argument\n");
> + return -EINVAL;
> + }
> +
> if (kstrtoul(buf, 10, &res) < 0 || res > MAX_ORDER / 2) {
> pr_err("Bad debug_guardpage_minorder value\n");
> return 0;
> @@ -6952,6 +6958,11 @@ static int __init cmdline_parse_core(char *p, unsigned long *core,
> */
> static int __init cmdline_parse_kernelcore(char *p)
> {
> + if (!p) {
> + pr_err("kernel option kernelcore requires an argument\n");
> + return -EINVAL;
> + }
> +
> /* parse kernelcore=mirror */
> if (parse_option_str(p, "mirror")) {
> mirrored_kernelcore = true;
> --
> 2.7.4
>
--
Michal Hocko
SUSE Labs
On Sat 22-09-18 22:53:33, [email protected] wrote:
> From: He Zhe <[email protected]>
>
> Add KBUILD_MODNAME to make prints more clear.
Please be more explicit. Examples of before and after would be really
helpful.
> Signed-off-by: He Zhe <[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]
> ---
> v2:
> Split the addition of KBUILD_MODNAME out
>
> mm/page_alloc.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index f34cae1..ead9556 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -14,6 +14,8 @@
> * (lots of bits borrowed from Ingo Molnar & Andrew Morton)
> */
>
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> #include <linux/stddef.h>
> #include <linux/mm.h>
> #include <linux/swap.h>
> --
> 2.7.4
>
--
Michal Hocko
SUSE Labs
On Mon, 24 Sep 2018 16:24:08 +0200 Michal Hocko <[email protected]> wrote:
> On Sat 22-09-18 22:53:32, [email protected] wrote:
> > From: He Zhe <[email protected]>
> >
> > debug_guardpage_minorder_setup and cmdline_parse_kernelcore do not check
> > input argument before using it. The argument would be a NULL pointer if
> > "debug_guardpage_minorder" or "kernelcore", without its value, is set in
> > command line and thus causes the following panic.
> >
> > PANIC: early exception 0xe3 IP 10:ffffffffa08146f1 error 0 cr2 0x0
> > [ 0.000000] CPU: 0 PID: 0 Comm: swapper Not tainted 4.19.0-rc4-yocto-standard+ #11
> > [ 0.000000] RIP: 0010:parse_option_str+0x11/0x90
> > ...
> > [ 0.000000] Call Trace:
> > [ 0.000000] cmdline_parse_kernelcore+0x19/0x41
> > [ 0.000000] do_early_param+0x57/0x8e
> > [ 0.000000] parse_args+0x208/0x320
> > [ 0.000000] ? rdinit_setup+0x30/0x30
> > [ 0.000000] parse_early_options+0x29/0x2d
> > [ 0.000000] ? rdinit_setup+0x30/0x30
> > [ 0.000000] parse_early_param+0x36/0x4d
> > [ 0.000000] setup_arch+0x336/0x99e
> > [ 0.000000] start_kernel+0x6f/0x4ee
> > [ 0.000000] x86_64_start_reservations+0x24/0x26
> > [ 0.000000] x86_64_start_kernel+0x6f/0x72
> > [ 0.000000] secondary_startup_64+0xa4/0xb0
> >
> > This patch adds a check to prevent the panic
>
> Is this something we deeply care about? The kernel command line
> interface is to be used by admins who know what they are doing. Using
> random or wrong values for these parameters can have detrimental effects
> on the system. This particular case would blow up early, good. At least
> it is visible immediately. This and many other parameters could have a
> seemingly valid input (e.g. not a missing value) and subtle runtime
> effect. You won't blow up immediately but the system is hardly usable
> and the early checking cannot possible catch all those cases. Take a
> mem=$N copied from one machine to another with a different memory
> layout. While 2G can be perfectly fine on one a different machine might
> result on a completely unusable system because the available RAM is
> place higher.
>
> So I am really wondering. Do we really want a lot of code to catch
> kernel command line incorrect inputs? Does it really lead to better
> quality overall? IMHO, we do have a proper documentation and we should
> trust those starting the kernel.
No, it's not very important. It might help some people understand why
their kernel went splat in rare circumstances. And it's __init code so
the runtime impact is nil.
It bothers me that there are many other kernel parameters which have
the same undesirable behaviour. I'd much prefer a general fixup which
gave all of them this treatment, but it's unclear how to do this.
On Mon 24-09-18 14:42:17, Andrew Morton wrote:
> On Mon, 24 Sep 2018 16:24:08 +0200 Michal Hocko <[email protected]> wrote:
>
> > On Sat 22-09-18 22:53:32, [email protected] wrote:
> > > From: He Zhe <[email protected]>
> > >
> > > debug_guardpage_minorder_setup and cmdline_parse_kernelcore do not check
> > > input argument before using it. The argument would be a NULL pointer if
> > > "debug_guardpage_minorder" or "kernelcore", without its value, is set in
> > > command line and thus causes the following panic.
> > >
> > > PANIC: early exception 0xe3 IP 10:ffffffffa08146f1 error 0 cr2 0x0
> > > [ 0.000000] CPU: 0 PID: 0 Comm: swapper Not tainted 4.19.0-rc4-yocto-standard+ #11
> > > [ 0.000000] RIP: 0010:parse_option_str+0x11/0x90
> > > ...
> > > [ 0.000000] Call Trace:
> > > [ 0.000000] cmdline_parse_kernelcore+0x19/0x41
> > > [ 0.000000] do_early_param+0x57/0x8e
> > > [ 0.000000] parse_args+0x208/0x320
> > > [ 0.000000] ? rdinit_setup+0x30/0x30
> > > [ 0.000000] parse_early_options+0x29/0x2d
> > > [ 0.000000] ? rdinit_setup+0x30/0x30
> > > [ 0.000000] parse_early_param+0x36/0x4d
> > > [ 0.000000] setup_arch+0x336/0x99e
> > > [ 0.000000] start_kernel+0x6f/0x4ee
> > > [ 0.000000] x86_64_start_reservations+0x24/0x26
> > > [ 0.000000] x86_64_start_kernel+0x6f/0x72
> > > [ 0.000000] secondary_startup_64+0xa4/0xb0
> > >
> > > This patch adds a check to prevent the panic
> >
> > Is this something we deeply care about? The kernel command line
> > interface is to be used by admins who know what they are doing. Using
> > random or wrong values for these parameters can have detrimental effects
> > on the system. This particular case would blow up early, good. At least
> > it is visible immediately. This and many other parameters could have a
> > seemingly valid input (e.g. not a missing value) and subtle runtime
> > effect. You won't blow up immediately but the system is hardly usable
> > and the early checking cannot possible catch all those cases. Take a
> > mem=$N copied from one machine to another with a different memory
> > layout. While 2G can be perfectly fine on one a different machine might
> > result on a completely unusable system because the available RAM is
> > place higher.
> >
> > So I am really wondering. Do we really want a lot of code to catch
> > kernel command line incorrect inputs? Does it really lead to better
> > quality overall? IMHO, we do have a proper documentation and we should
> > trust those starting the kernel.
>
> No, it's not very important. It might help some people understand why
> their kernel went splat in rare circumstances. And it's __init code so
> the runtime impact is nil.
>
> It bothers me that there are many other kernel parameters which have
> the same undesirable behaviour. I'd much prefer a general fixup which
> gave all of them this treatment, but it's unclear how to do this.
If early_param took an additional argument to tell "this really requires
a parameter" then we could do it in the common code.
$ git grep "early_param(\"" | wc -l
251
quite a lot of work for something that hasn't been a problem for years I
guess. But maybe this would allow to remove ad-hoc checks in handlers
and reduce the overal code size (in LOC) in the end.
--
Michal Hocko
SUSE Labs