* [PATCH] mm/page_alloc: Fix panic caused by passing debug_guardpage_minorder or kernelcore to command line
@ 2018-09-18 15:33 zhe.he
2018-09-18 21:19 ` Andrew Morton
0 siblings, 1 reply; 3+ messages in thread
From: zhe.he @ 2018-09-18 15:33 UTC (permalink / raw)
To: akpm, mhocko, vbabka, pasha.tatashin, mgorman, aaron.lu,
osalvador, iamjoonsoo.kim, linux-mm, linux-kernel
Cc: 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+ #1
[ 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
---
mm/page_alloc.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 89d2a2a..d4cda06 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>
@@ -630,6 +632,11 @@ static int __init debug_guardpage_minorder_setup(char *buf)
{
unsigned long res;
+ if (!buf) {
+ pr_err("Config string not provided\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 +6959,11 @@ static int __init cmdline_parse_core(char *p, unsigned long *core,
*/
static int __init cmdline_parse_kernelcore(char *p)
{
+ if (!p) {
+ pr_err("Config string not provided\n");
+ return -EINVAL;
+ }
+
/* parse kernelcore=mirror */
if (parse_option_str(p, "mirror")) {
mirrored_kernelcore = true;
--
2.7.4
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH] mm/page_alloc: Fix panic caused by passing debug_guardpage_minorder or kernelcore to command line
2018-09-18 15:33 [PATCH] mm/page_alloc: Fix panic caused by passing debug_guardpage_minorder or kernelcore to command line zhe.he
@ 2018-09-18 21:19 ` Andrew Morton
2018-09-19 9:51 ` He Zhe
0 siblings, 1 reply; 3+ messages in thread
From: Andrew Morton @ 2018-09-18 21:19 UTC (permalink / raw)
To: zhe.he
Cc: mhocko, vbabka, pasha.tatashin, mgorman, aaron.lu, osalvador,
iamjoonsoo.kim, linux-mm, linux-kernel
On Tue, 18 Sep 2018 23:33:08 +0800 <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+ #1
> [ 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
From my quick reading, more than half of the __setup handlers in mm/
will crash in the same way if misused in this fashion.
> This patch adds a check to prevent the panic and adds KBUILD_MODNAME to
> prints.
So a better solution might be to add a check into the calling code
(presumably in init/main.c) to print a warning if we have kernel
command line arguments such as "kernelcore=". That way, users will see
the warning immediately before the oops and will know how to fix things
up.
> --- 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>
> @@ -630,6 +632,11 @@ static int __init debug_guardpage_minorder_setup(char *buf)
> {
> unsigned long res;
>
> + if (!buf) {
> + pr_err("Config string not provided\n");
If were going to do it this way, we should tell the operator which
argument was bad. pr_err("kernel option debug_guardpage_minorder
requires an argument").
And then perhaps we should just let the kernel crash anyway. That
seems better than hoping that the user will notice that line in the
logs one day.
And note that the preceding two paragraphs will produce the same result
as my do-it-in-init/main.c suggestion!
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] mm/page_alloc: Fix panic caused by passing debug_guardpage_minorder or kernelcore to command line
2018-09-18 21:19 ` Andrew Morton
@ 2018-09-19 9:51 ` He Zhe
0 siblings, 0 replies; 3+ messages in thread
From: He Zhe @ 2018-09-19 9:51 UTC (permalink / raw)
To: Andrew Morton
Cc: mhocko, vbabka, pasha.tatashin, mgorman, aaron.lu, osalvador,
iamjoonsoo.kim, linux-mm, linux-kernel
On 2018年09月19日 05:19, Andrew Morton wrote:
> On Tue, 18 Sep 2018 23:33:08 +0800 <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+ #1
>> [ 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
> >From my quick reading, more than half of the __setup handlers in mm/
> will crash in the same way if misused in this fashion.
>
>> This patch adds a check to prevent the panic and adds KBUILD_MODNAME to
>> prints.
> So a better solution might be to add a check into the calling code
> (presumably in init/main.c) to print a warning if we have kernel
> command line arguments such as "kernelcore=". That way, users will see
> the warning immediately before the oops and will know how to fix things
> up.
Thank you for your suggestion.
"kernelcore=" would not cause crash, "kernelcore' would. Andmany users of
early_param, e.g. the following two, depend on the validity of the "xxx"
format. If we fixed in the calling code, those parameters would become
invalid and need to be changed to a new format. That might affect too much.
Soit might be better to correct the users who misuse it.
static int __init cmdline_parse_movable_node(char *p)
{
#ifdef CONFIG_HAVE_MEMBLOCK_NODE_MAP
movable_node_enabled = true;
#else
pr_warn("movable_node parameter depends on CONFIG_HAVE_MEMBLOCK_NODE_MAP to work properly\n");
#endif
return 0;
}
early_param("movable_node", cmdline_parse_movable_node);
static int __init parse_alloc_mptable_opt(char *p)
{
enable_update_mptable = 1;
#ifdef CONFIG_PCI
pci_routeirq = 1;
#endif
alloc_mptable = 1;
if (!p)
return 0;
mpc_new_length = memparse(p, &p);
return 0;
}
early_param("alloc_mptable", parse_alloc_mptable_opt);
>
>> --- 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>
>> @@ -630,6 +632,11 @@ static int __init debug_guardpage_minorder_setup(char *buf)
>> {
>> unsigned long res;
>>
>> + if (!buf) {
>> + pr_err("Config string not provided\n");
> If were going to do it this way, we should tell the operator which
> argument was bad. pr_err("kernel option debug_guardpage_minorder
> requires an argument").
Yes, this makes it more clear for users.I'd like to do in this way.
>
> And then perhaps we should just let the kernel crash anyway. That
> seems better than hoping that the user will notice that line in the
> logs one day.
If we want the PANIC info for these early parameters when crashing,
the parameter earlyprintk needs to be set correctly, especially in
embedded scenarios. Otherwise the system will hang without any error
info. Letting it boot up with an invalid parameter seems better than
that. And the owner of the parameter may give more errors to the
users if they don't get a valid value.
Thanks,
Zhe
>
> And note that the preceding two paragraphs will produce the same result
> as my do-it-in-init/main.c suggestion!
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-09-19 9:55 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-09-18 15:33 [PATCH] mm/page_alloc: Fix panic caused by passing debug_guardpage_minorder or kernelcore to command line zhe.he
2018-09-18 21:19 ` Andrew Morton
2018-09-19 9:51 ` He Zhe
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).