* [PATCH v2 1/2] mm/page_alloc: Fix panic caused by passing debug_guardpage_minorder or kernelcore to command line
@ 2018-09-22 14:53 zhe.he
2018-09-22 14:53 ` [PATCH v2 2/2] mm/page_alloc: Add KBUILD_MODNAME zhe.he
2018-09-24 14:24 ` [PATCH v2 1/2] mm/page_alloc: Fix panic caused by passing debug_guardpage_minorder or kernelcore to command line Michal Hocko
0 siblings, 2 replies; 6+ messages in thread
From: zhe.he @ 2018-09-22 14:53 UTC (permalink / raw)
To: akpm, mhocko, vbabka, pasha.tatashin, mgorman, aaron.lu,
osalvador, iamjoonsoo.kim, linux-mm, linux-kernel, zhe.he
From: He Zhe <zhe.he@windriver.com>
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 <zhe.he@windriver.com>
Cc: stable@vger.kernel.org
Cc: akpm@linux-foundation.org
Cc: mhocko@suse.com
Cc: vbabka@suse.cz
Cc: pasha.tatashin@oracle.com
Cc: mgorman@techsingularity.net
Cc: aaron.lu@intel.com
Cc: osalvador@suse.de
Cc: iamjoonsoo.kim@lge.com
---
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v2 2/2] mm/page_alloc: Add KBUILD_MODNAME
2018-09-22 14:53 [PATCH v2 1/2] mm/page_alloc: Fix panic caused by passing debug_guardpage_minorder or kernelcore to command line zhe.he
@ 2018-09-22 14:53 ` zhe.he
2018-09-24 14:26 ` Michal Hocko
2018-09-24 14:24 ` [PATCH v2 1/2] mm/page_alloc: Fix panic caused by passing debug_guardpage_minorder or kernelcore to command line Michal Hocko
1 sibling, 1 reply; 6+ messages in thread
From: zhe.he @ 2018-09-22 14:53 UTC (permalink / raw)
To: akpm, mhocko, vbabka, pasha.tatashin, mgorman, aaron.lu,
osalvador, iamjoonsoo.kim, linux-mm, linux-kernel, zhe.he
From: He Zhe <zhe.he@windriver.com>
Add KBUILD_MODNAME to make prints more clear.
Signed-off-by: He Zhe <zhe.he@windriver.com>
Cc: akpm@linux-foundation.org
Cc: mhocko@suse.com
Cc: vbabka@suse.cz
Cc: pasha.tatashin@oracle.com
Cc: mgorman@techsingularity.net
Cc: aaron.lu@intel.com
Cc: osalvador@suse.de
Cc: iamjoonsoo.kim@lge.com
---
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/2] mm/page_alloc: Fix panic caused by passing debug_guardpage_minorder or kernelcore to command line
2018-09-22 14:53 [PATCH v2 1/2] mm/page_alloc: Fix panic caused by passing debug_guardpage_minorder or kernelcore to command line zhe.he
2018-09-22 14:53 ` [PATCH v2 2/2] mm/page_alloc: Add KBUILD_MODNAME zhe.he
@ 2018-09-24 14:24 ` Michal Hocko
2018-09-24 21:42 ` Andrew Morton
1 sibling, 1 reply; 6+ messages in thread
From: Michal Hocko @ 2018-09-24 14:24 UTC (permalink / raw)
To: zhe.he
Cc: akpm, vbabka, pasha.tatashin, mgorman, aaron.lu, osalvador,
iamjoonsoo.kim, linux-mm, linux-kernel
On Sat 22-09-18 22:53:32, zhe.he@windriver.com wrote:
> From: He Zhe <zhe.he@windriver.com>
>
> 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 <zhe.he@windriver.com>
> Cc: stable@vger.kernel.org
> Cc: akpm@linux-foundation.org
> Cc: mhocko@suse.com
> Cc: vbabka@suse.cz
> Cc: pasha.tatashin@oracle.com
> Cc: mgorman@techsingularity.net
> Cc: aaron.lu@intel.com
> Cc: osalvador@suse.de
> Cc: iamjoonsoo.kim@lge.com
> ---
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] mm/page_alloc: Add KBUILD_MODNAME
2018-09-22 14:53 ` [PATCH v2 2/2] mm/page_alloc: Add KBUILD_MODNAME zhe.he
@ 2018-09-24 14:26 ` Michal Hocko
0 siblings, 0 replies; 6+ messages in thread
From: Michal Hocko @ 2018-09-24 14:26 UTC (permalink / raw)
To: zhe.he
Cc: akpm, vbabka, pasha.tatashin, mgorman, aaron.lu, osalvador,
iamjoonsoo.kim, linux-mm, linux-kernel
On Sat 22-09-18 22:53:33, zhe.he@windriver.com wrote:
> From: He Zhe <zhe.he@windriver.com>
>
> 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 <zhe.he@windriver.com>
> Cc: akpm@linux-foundation.org
> Cc: mhocko@suse.com
> Cc: vbabka@suse.cz
> Cc: pasha.tatashin@oracle.com
> Cc: mgorman@techsingularity.net
> Cc: aaron.lu@intel.com
> Cc: osalvador@suse.de
> Cc: iamjoonsoo.kim@lge.com
> ---
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/2] mm/page_alloc: Fix panic caused by passing debug_guardpage_minorder or kernelcore to command line
2018-09-24 14:24 ` [PATCH v2 1/2] mm/page_alloc: Fix panic caused by passing debug_guardpage_minorder or kernelcore to command line Michal Hocko
@ 2018-09-24 21:42 ` Andrew Morton
2018-09-25 5:59 ` Michal Hocko
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Morton @ 2018-09-24 21:42 UTC (permalink / raw)
To: Michal Hocko
Cc: zhe.he, vbabka, pasha.tatashin, mgorman, aaron.lu, osalvador,
iamjoonsoo.kim, linux-mm, linux-kernel
On Mon, 24 Sep 2018 16:24:08 +0200 Michal Hocko <mhocko@kernel.org> wrote:
> On Sat 22-09-18 22:53:32, zhe.he@windriver.com wrote:
> > From: He Zhe <zhe.he@windriver.com>
> >
> > 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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/2] mm/page_alloc: Fix panic caused by passing debug_guardpage_minorder or kernelcore to command line
2018-09-24 21:42 ` Andrew Morton
@ 2018-09-25 5:59 ` Michal Hocko
0 siblings, 0 replies; 6+ messages in thread
From: Michal Hocko @ 2018-09-25 5:59 UTC (permalink / raw)
To: Andrew Morton
Cc: zhe.he, vbabka, pasha.tatashin, mgorman, aaron.lu, osalvador,
iamjoonsoo.kim, linux-mm, linux-kernel
On Mon 24-09-18 14:42:17, Andrew Morton wrote:
> On Mon, 24 Sep 2018 16:24:08 +0200 Michal Hocko <mhocko@kernel.org> wrote:
>
> > On Sat 22-09-18 22:53:32, zhe.he@windriver.com wrote:
> > > From: He Zhe <zhe.he@windriver.com>
> > >
> > > 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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-09-25 5:59 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-09-22 14:53 [PATCH v2 1/2] mm/page_alloc: Fix panic caused by passing debug_guardpage_minorder or kernelcore to command line zhe.he
2018-09-22 14:53 ` [PATCH v2 2/2] mm/page_alloc: Add KBUILD_MODNAME zhe.he
2018-09-24 14:26 ` Michal Hocko
2018-09-24 14:24 ` [PATCH v2 1/2] mm/page_alloc: Fix panic caused by passing debug_guardpage_minorder or kernelcore to command line Michal Hocko
2018-09-24 21:42 ` Andrew Morton
2018-09-25 5:59 ` Michal Hocko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).