From: He Zhe <[email protected]>
log_buf_len_setup does not check input argument before passing it to
simple_strtoull. The argument would be a NULL pointer if "log_buf_len",
without its value, is set in command line and thus causes the following
panic.
PANIC: early exception 0xe3 IP 10:ffffffffaaeacd0d 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_integer_fixup_radix+0xd/0x70
...
[ 0.000000] Call Trace:
[ 0.000000] simple_strtoull+0x29/0x70
[ 0.000000] memparse+0x26/0x90
[ 0.000000] log_buf_len_setup+0x17/0x22
[ 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.
Signed-off-by: He Zhe <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
---
v2:
Split out the addition of pr_fmt and the unsigned update
v3:
Remove error message for NULL pointer
Add check and error message for over 4G use
v4:
Split each piece into one patch
v5:
Remove a redundant print prefix
kernel/printk/printk.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 9bf5404..06045ab 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -1048,7 +1048,12 @@ static void __init log_buf_len_update(unsigned size)
/* save requested log_buf_len since it's too early to process it */
static int __init log_buf_len_setup(char *str)
{
- unsigned size = memparse(str, &str);
+ unsigned int size;
+
+ if (!str)
+ return -EINVAL;
+
+ size = memparse(str, &str);
log_buf_len_update(size);
--
2.7.4
From: He Zhe <[email protected]>
Give explicit error for users who want to use larger log buffer.
Signed-off-by: He Zhe <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
---
kernel/printk/printk.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index b84aac0..5ccfd5d 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -1039,18 +1039,23 @@ void log_buf_vmcoreinfo_setup(void)
static unsigned long __initdata new_log_buf_len;
/* we practice scaling the ring buffer by powers of 2 */
-static void __init log_buf_len_update(unsigned size)
+static void __init log_buf_len_update(u64 size)
{
+ if (size > UINT_MAX) {
+ size = UINT_MAX;
+ pr_err("log_buf over 4G is not supported.\n");
+ }
+
if (size)
size = roundup_pow_of_two(size);
if (size > log_buf_len)
- new_log_buf_len = size;
+ new_log_buf_len = (unsigned long)size;
}
/* save requested log_buf_len since it's too early to process it */
static int __init log_buf_len_setup(char *str)
{
- unsigned int size;
+ u64 size;
if (!str)
return -EINVAL;
--
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]
---
kernel/printk/printk.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index c9a0be3..0f24d7f 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -16,6 +16,8 @@
* 01Mar01 Andrew Morton
*/
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
#include <linux/kernel.h>
#include <linux/mm.h>
#include <linux/tty.h>
@@ -2695,7 +2697,7 @@ void register_console(struct console *newcon)
if (newcon->flags & CON_EXTENDED)
if (!nr_ext_console_drivers++)
- pr_info("printk: continuation disabled due to ext consoles, expect more fragments in /dev/kmsg\n");
+ pr_info("continuation disabled due to ext consoles, expect more fragments in /dev/kmsg\n");
if (newcon->flags & CON_PRINTBUFFER) {
/*
--
2.7.4
From: He Zhe <[email protected]>
Correct wrong casting that might cut off the normal output.
Signed-off-by: He Zhe <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
---
kernel/printk/printk.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 06045ab..c9a0be3 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2356,8 +2356,9 @@ void console_unlock(void)
printk_safe_enter_irqsave(flags);
raw_spin_lock(&logbuf_lock);
if (console_seq < log_first_seq) {
- len = sprintf(text, "** %u printk messages dropped **\n",
- (unsigned)(log_first_seq - console_seq));
+ len = sprintf(text,
+ "** %llu printk messages dropped **\n",
+ log_first_seq - console_seq);
/* messages are gone, move to first one */
console_seq = log_first_seq;
--
2.7.4
On (09/30/18 00:45), [email protected] wrote:
>
> This patch adds a check to prevent the panic.
>
> Signed-off-by: He Zhe <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
Reviewed-by: Sergey Senozhatsky <[email protected]>
-ss
On (09/30/18 00:45), [email protected] wrote:
> Correct wrong casting that might cut off the normal output.
A commit message probably could have been a bit more descriptive,
mentioning that log_first_seq and console_seq are 64-bit unsigned
integers, this is a Cc material potentially.
> Signed-off-by: He Zhe <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
Reviewed-by: Sergey Senozhatsky <[email protected]>
-ss
On (09/30/18 00:45), [email protected] wrote:
> From: He Zhe <[email protected]>
>
> Give explicit error for users who want to use larger log buffer.
>
> Signed-off-by: He Zhe <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
Suggested-by: Sergey Senozhatsky <[email protected]>
So people will know who to blame ;)
Reviewed-by: Sergey Senozhatsky <[email protected]>
-ss
On (09/30/18 00:45), [email protected] wrote:
> 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]
Reviewed-by: Sergey Senozhatsky <[email protected]>
-ss
On Sun 2018-09-30 00:45:50, [email protected] wrote:
> From: He Zhe <[email protected]>
>
> log_buf_len_setup does not check input argument before passing it to
> simple_strtoull. The argument would be a NULL pointer if "log_buf_len",
> without its value, is set in command line and thus causes the following
> panic.
>
> PANIC: early exception 0xe3 IP 10:ffffffffaaeacd0d 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_integer_fixup_radix+0xd/0x70
> ...
> [ 0.000000] Call Trace:
> [ 0.000000] simple_strtoull+0x29/0x70
> [ 0.000000] memparse+0x26/0x90
> [ 0.000000] log_buf_len_setup+0x17/0x22
> [ 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.
>
> Signed-off-by: He Zhe <[email protected]>
JFYI, I have pushed this patch into printk.git, branch for-4.20.
Best Regards,
Petr
On Sun 2018-09-30 00:45:52, [email protected] wrote:
> From: He Zhe <[email protected]>
>
> Add KBUILD_MODNAME to make prints more clear.
>
> Signed-off-by: He Zhe <[email protected]>
JFYI, I have pushed this patch into printk.git, for-4.20 branch.
Best Regards,
Petr
On Tue 2018-10-02 14:50:18, Sergey Senozhatsky wrote:
> On (09/30/18 00:45), [email protected] wrote:
> > Correct wrong casting that might cut off the normal output.
>
> A commit message probably could have been a bit more descriptive,
> mentioning that log_first_seq and console_seq are 64-bit unsigned
> integers, this is a Cc material potentially.
I have used the following text:
log_first_seq and console_seq are 64-bit unsigned integers.
Correct a wrong casting that might cut off the output.
> > Signed-off-by: He Zhe <[email protected]>
> > Cc: [email protected]
> > Cc: [email protected]
> > Cc: [email protected]
> > Cc: [email protected]
>
> Reviewed-by: Sergey Senozhatsky <[email protected]>
And pushed this patch into printk.git, branch for-4.20.
Best Regards,
Petr
On Sun 2018-09-30 00:45:53, [email protected] wrote:
> From: He Zhe <[email protected]>
>
> Give explicit error for users who want to use larger log buffer.
>
> Signed-off-by: He Zhe <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> ---
> kernel/printk/printk.c | 11 ++++++++---
> 1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index b84aac0..5ccfd5d 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -1039,18 +1039,23 @@ void log_buf_vmcoreinfo_setup(void)
> static unsigned long __initdata new_log_buf_len;
>
> /* we practice scaling the ring buffer by powers of 2 */
> -static void __init log_buf_len_update(unsigned size)
> +static void __init log_buf_len_update(u64 size)
> {
> + if (size > UINT_MAX) {
> + size = UINT_MAX;
> + pr_err("log_buf over 4G is not supported.\n");
I tried this patch with log_buf_len=5G. The kernel did not crash
but dmesg shown some mess. There are several 32-bit variables
to store the size, for example:
static u32 log_buf_len = __LOG_BUF_LEN;
u32 log_buf_len_get(void)
static u32 log_first_idx;
static u32 log_next_idx;
I guess that the code somewhere does not detect an overflows
correctly.
I am not motivated enought to add support for such huge message
buffer. Therefore I suggest to limit it to 32G for now.
This patch worked for me:
From d63b781b596ccb3d205801b2ba944797fa7ab0ce Mon Sep 17 00:00:00 2001
From: He Zhe <[email protected]>
Date: Sun, 30 Sep 2018 00:45:53 +0800
Subject: [PATCH] printk: Give error on attempt to set log buffer length to
over 2G
The current printk() is ready to handle log buffer size up to 2G.
Give an explicit error for users who want to use larger log buffer.
Also fix printk formatting to show the 2G as a positive number.
Suggested-by: Sergey Senozhatsky <[email protected]>
Signed-off-by: He Zhe <[email protected]>
Signed-off-by: Petr Mladek <[email protected]>
---
kernel/printk/printk.c | 17 +++++++++++------
1 file changed, 11 insertions(+), 6 deletions(-)
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 15f3e70be448..f595107ddf42 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -1040,18 +1040,23 @@ void log_buf_vmcoreinfo_setup(void)
static unsigned long __initdata new_log_buf_len;
/* we practice scaling the ring buffer by powers of 2 */
-static void __init log_buf_len_update(unsigned size)
+static void __init log_buf_len_update(u64 size)
{
+ if (size > UINT_MAX) {
+ size = UINT_MAX;
+ pr_err("log_buf over 2G is not supported.\n");
+ }
+
if (size)
size = roundup_pow_of_two(size);
if (size > log_buf_len)
- new_log_buf_len = size;
+ new_log_buf_len = (unsigned long)size;
}
/* save requested log_buf_len since it's too early to process it */
static int __init log_buf_len_setup(char *str)
{
- unsigned int size;
+ u64 size;
if (!str)
return -EINVAL;
@@ -1121,7 +1126,7 @@ void __init setup_log_buf(int early)
}
if (unlikely(!new_log_buf)) {
- pr_err("log_buf_len: %ld bytes not available\n",
+ pr_err("log_buf_len: %lu bytes not available\n",
new_log_buf_len);
return;
}
@@ -1134,8 +1139,8 @@ void __init setup_log_buf(int early)
memcpy(log_buf, __log_buf, __LOG_BUF_LEN);
logbuf_unlock_irqrestore(flags);
- pr_info("log_buf_len: %d bytes\n", log_buf_len);
- pr_info("early log buf free: %d(%d%%)\n",
+ pr_info("log_buf_len: %u bytes\n", log_buf_len);
+ pr_info("early log buf free: %u(%u%%)\n",
free, (free * 100) / __LOG_BUF_LEN);
}
--
2.13.7
On (10/08/18 15:59), Petr Mladek wrote:
> I tried this patch with log_buf_len=5G. The kernel did not crash
> but dmesg shown some mess. There are several 32-bit variables
> to store the size, for example:
>
> static u32 log_buf_len = __LOG_BUF_LEN;
> u32 log_buf_len_get(void)
> static u32 log_first_idx;
> static u32 log_next_idx;
>
> I guess that the code somewhere does not detect an overflows
> correctly.
>
> I am not motivated enought to add support for such huge message
> buffer. Therefore I suggest to limit it to 32G for now.
Yeah, I think we gonna have problems even with a 4G logbuf and a 32-bit
user-space doing syslog(int len).
I agree on the "not motivated enough" part ;)
-ss
On 2018年10月08日 21:59, Petr Mladek wrote:
> I tried this patch with log_buf_len=5G. The kernel did not crash
> but dmesg shown some mess. There are several 32-bit variables
> to store the size, for example:
>
> static u32 log_buf_len = __LOG_BUF_LEN;
> u32 log_buf_len_get(void)
> static u32 log_first_idx;
> static u32 log_next_idx;
>
> I guess that the code somewhere does not detect an overflows
> correctly.
>
> I am not motivated enought to add support for such huge message
> buffer. Therefore I suggest to limit it to 32G for now.
I'm also fine with this suggestion. Thanks.
Zhe
On Mon 2018-10-08 23:59:50, Sergey Senozhatsky wrote:
> On (10/08/18 15:59), Petr Mladek wrote:
> > I tried this patch with log_buf_len=5G. The kernel did not crash
> > but dmesg shown some mess. There are several 32-bit variables
> > to store the size, for example:
> >
> > static u32 log_buf_len = __LOG_BUF_LEN;
> > u32 log_buf_len_get(void)
> > static u32 log_first_idx;
> > static u32 log_next_idx;
> >
> > I guess that the code somewhere does not detect an overflows
> > correctly.
> >
> > I am not motivated enought to add support for such huge message
> > buffer. Therefore I suggest to limit it to 32G for now.
>
> Yeah, I think we gonna have problems even with a 4G logbuf and a 32-bit
> user-space doing syslog(int len).
>
> I agree on the "not motivated enough" part ;)
OK, I have pushed an updated patch that has the limit 2GB
into printk.git, for-4.20 branch.
Note that it is slightly different than the yesterday's proposal.
I made a mistake in testing and still compared with UNIT_MAX.
The pushed version can be seen at
https://git.kernel.org/pub/scm/linux/kernel/git/pmladek/printk.git/commit/?h=for-4.20&id=e6fe3e5b7d16e8f146a4ae7fe481bc6e97acde1e
Best Regards,
Petr
On (10/09/18 15:05), Petr Mladek wrote:
> >
> > Yeah, I think we gonna have problems even with a 4G logbuf and a 32-bit
> > user-space doing syslog(int len).
> >
> > I agree on the "not motivated enough" part ;)
>
> OK, I have pushed an updated patch that has the limit 2GB
> into printk.git, for-4.20 branch.
>
> Note that it is slightly different than the yesterday's proposal.
> I made a mistake in testing and still compared with UNIT_MAX.
>
> The pushed version can be seen at
> https://git.kernel.org/pub/scm/linux/kernel/git/pmladek/printk.git/commit/?h=for-4.20&id=e6fe3e5b7d16e8f146a4ae7fe481bc6e97acde1e
---
> +#define LOG_BUF_LEN_MAX (u32)(1 << 31)
[..]
> + if (size > (u64)LOG_BUF_LEN_MAX) {
> + size = (u64)LOG_BUF_LEN_MAX;
> + pr_err("log_buf over 2G is not supported.\n");
> + }
Why not INT_MAX?
> + pr_info("log_buf_len: %u bytes\n", log_buf_len);
> + pr_info("early log buf free: %u(%u%%)\n",
> free, (free * 100) / __LOG_BUF_LEN);
Can 'free * 100' overflow?
-ss
On Tue 2018-10-09 22:57:58, Sergey Senozhatsky wrote:
> On (10/09/18 15:05), Petr Mladek wrote:
> > >
> > > Yeah, I think we gonna have problems even with a 4G logbuf and a 32-bit
> > > user-space doing syslog(int len).
> > >
> > > I agree on the "not motivated enough" part ;)
> >
> > OK, I have pushed an updated patch that has the limit 2GB
> > into printk.git, for-4.20 branch.
> >
> > Note that it is slightly different than the yesterday's proposal.
> > I made a mistake in testing and still compared with UNIT_MAX.
> >
> > The pushed version can be seen at
> > https://git.kernel.org/pub/scm/linux/kernel/git/pmladek/printk.git/commit/?h=for-4.20&id=e6fe3e5b7d16e8f146a4ae7fe481bc6e97acde1e
>
>
> ---
>
> > +#define LOG_BUF_LEN_MAX (u32)(1 << 31)
> [..]
> > + if (size > (u64)LOG_BUF_LEN_MAX) {
> > + size = (u64)LOG_BUF_LEN_MAX;
> > + pr_err("log_buf over 2G is not supported.\n");
> > + }
>
> Why not INT_MAX?
INT_MAX is 0x7fffffff but we need 0x80000000. I did not find
any predefined macro.
>
> > + pr_info("log_buf_len: %u bytes\n", log_buf_len);
> > + pr_info("early log buf free: %u(%u%%)\n",
> > free, (free * 100) / __LOG_BUF_LEN);
>
> Can 'free * 100' overflow?
Good question. It uses the size of the static buffer. If I count
correctly then we are on the safe side because LOG_BUF_SHIFT
is limited by
range 12 25
Best Regards,
Petr
On (10/10/18 10:09), Petr Mladek wrote:
> > > +#define LOG_BUF_LEN_MAX (u32)(1 << 31)
> > [..]
> > > + if (size > (u64)LOG_BUF_LEN_MAX) {
> > > + size = (u64)LOG_BUF_LEN_MAX;
> > > + pr_err("log_buf over 2G is not supported.\n");
> > > + }
> >
> > Why not INT_MAX?
>
> INT_MAX is 0x7fffffff but we need 0x80000000. I did not find
> any predefined macro.
Hmm, OK. I need to think about it more.
> > > + pr_info("log_buf_len: %u bytes\n", log_buf_len);
> > > + pr_info("early log buf free: %u(%u%%)\n",
> > > free, (free * 100) / __LOG_BUF_LEN);
> >
> > Can 'free * 100' overflow?
>
> Good question. It uses the size of the static buffer. If I count
> correctly then we are on the safe side because LOG_BUF_SHIFT
> is limited by
>
> range 12 25
I didn't know there was Kconfig "range" :)
$ echo $(((1<<25)*100))
3355443200
-ss