* [PATCH v2 1/2] printk: Fix panic caused by passing log_buf_len to command line @ 2018-09-18 17:17 zhe.he 2018-09-18 17:17 ` [PATCH v2 2/2] printk: Add KBUILD_MODNAME and correct bare use of unsigned zhe.he 2018-09-19 1:50 ` [PATCH v2 1/2] printk: Fix panic caused by passing log_buf_len to command line Sergey Senozhatsky 0 siblings, 2 replies; 25+ messages in thread From: zhe.he @ 2018-09-18 17:17 UTC (permalink / raw) To: pmladek, sergey.senozhatsky, rostedt, linux-kernel; +Cc: zhe.he From: He Zhe <zhe.he@windriver.com> 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 <zhe.he@windriver.com> Cc: stable@vger.kernel.org Cc: pmladek@suse.com Cc: sergey.senozhatsky@gmail.com Cc: rostedt@goodmis.org --- v2: Split out the addition of pr_fmt and the unsigned update kernel/printk/printk.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c index 9bf5404..34c0403 100644 --- a/kernel/printk/printk.c +++ b/kernel/printk/printk.c @@ -1048,7 +1048,14 @@ 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 size; + + if (!str) { + pr_err("Config string not provided\n"); + return -EINVAL; + } + + size = memparse(str, &str); log_buf_len_update(size); -- 2.7.4 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v2 2/2] printk: Add KBUILD_MODNAME and correct bare use of unsigned 2018-09-18 17:17 [PATCH v2 1/2] printk: Fix panic caused by passing log_buf_len to command line zhe.he @ 2018-09-18 17:17 ` zhe.he 2018-09-19 2:06 ` Sergey Senozhatsky 2018-09-19 1:50 ` [PATCH v2 1/2] printk: Fix panic caused by passing log_buf_len to command line Sergey Senozhatsky 1 sibling, 1 reply; 25+ messages in thread From: zhe.he @ 2018-09-18 17:17 UTC (permalink / raw) To: pmladek, sergey.senozhatsky, rostedt, linux-kernel; +Cc: zhe.he From: He Zhe <zhe.he@windriver.com> Add KBUILD_MODNAME to make prints more clear. And use 'unsigned int' intead of 'unsigned' according to checkpatch.pl's suggestion. Signed-off-by: He Zhe <zhe.he@windriver.com> Cc: pmladek@suse.com Cc: sergey.senozhatsky@gmail.com Cc: rostedt@goodmis.org --- v2: Correct one more place kernel/printk/printk.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c index 34c0403..ece870f 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> @@ -1048,7 +1050,7 @@ 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; + unsigned int size; if (!str) { pr_err("Config string not provided\n"); @@ -2359,7 +2361,7 @@ void console_unlock(void) 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)); + (unsigned int)(log_first_seq - console_seq)); /* messages are gone, move to first one */ console_seq = log_first_seq; -- 2.7.4 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH v2 2/2] printk: Add KBUILD_MODNAME and correct bare use of unsigned 2018-09-18 17:17 ` [PATCH v2 2/2] printk: Add KBUILD_MODNAME and correct bare use of unsigned zhe.he @ 2018-09-19 2:06 ` Sergey Senozhatsky 2018-09-19 11:20 ` Petr Mladek 0 siblings, 1 reply; 25+ messages in thread From: Sergey Senozhatsky @ 2018-09-19 2:06 UTC (permalink / raw) To: zhe.he; +Cc: pmladek, sergey.senozhatsky, rostedt, linux-kernel On (09/19/18 01:17), zhe.he@windriver.com wrote: > Add KBUILD_MODNAME to make prints more clear. No strong opinion. I'm OK with this change. > And use 'unsigned int' intead of 'unsigned' according to > checkpatch.pl's suggestion. I don't think that "unsigned int" is the right thing to use there. > if (console_seq < log_first_seq) { > len = sprintf(text, "** %u printk messages dropped **\n", > - (unsigned)(log_first_seq - console_seq)); > + (unsigned int)(log_first_seq - console_seq)); Both log_first_seq and console_seq are u64. log_first_seq - console_seq thus, *in theory*, can be larger than "unsigned int". So I'd just avoid cast and use an appropriate for u64 %llu sprintf() specifier. Something like below, perhaps: --- diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c index f73ea9dd6f46..4b8c5832bf14 100644 --- a/kernel/printk/printk.c +++ b/kernel/printk/printk.c @@ -2408,8 +2408,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 int)(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; --- Steven, Petr, any objections? -ss ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH v2 2/2] printk: Add KBUILD_MODNAME and correct bare use of unsigned 2018-09-19 2:06 ` Sergey Senozhatsky @ 2018-09-19 11:20 ` Petr Mladek 0 siblings, 0 replies; 25+ messages in thread From: Petr Mladek @ 2018-09-19 11:20 UTC (permalink / raw) To: Sergey Senozhatsky; +Cc: zhe.he, sergey.senozhatsky, rostedt, linux-kernel On Wed 2018-09-19 11:06:21, Sergey Senozhatsky wrote: > On (09/19/18 01:17), zhe.he@windriver.com wrote: > > Add KBUILD_MODNAME to make prints more clear. > > No strong opinion. I'm OK with this change. > > > And use 'unsigned int' intead of 'unsigned' according to > > checkpatch.pl's suggestion. > > I don't think that "unsigned int" is the right thing to use there. > > > if (console_seq < log_first_seq) { > > len = sprintf(text, "** %u printk messages dropped **\n", > > - (unsigned)(log_first_seq - console_seq)); > > + (unsigned int)(log_first_seq - console_seq)); > > Both log_first_seq and console_seq are u64. > > log_first_seq - console_seq > > thus, *in theory*, can be larger than "unsigned int". So I'd just avoid cast > and use an appropriate for u64 %llu sprintf() specifier. Something like below, > perhaps: > > --- > > diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c > index f73ea9dd6f46..4b8c5832bf14 100644 > --- a/kernel/printk/printk.c > +++ b/kernel/printk/printk.c > @@ -2408,8 +2408,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 int)(log_first_seq - console_seq)); > + len = sprintf(text, > + "** %llu printk messages dropped **\n", > + log_first_seq - console_seq); This looks like a better solution. Also please, define pr_fmt and fix the casting in separate patches. Best Regards, Petr ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2 1/2] printk: Fix panic caused by passing log_buf_len to command line 2018-09-18 17:17 [PATCH v2 1/2] printk: Fix panic caused by passing log_buf_len to command line zhe.he 2018-09-18 17:17 ` [PATCH v2 2/2] printk: Add KBUILD_MODNAME and correct bare use of unsigned zhe.he @ 2018-09-19 1:50 ` Sergey Senozhatsky 2018-09-19 2:27 ` He Zhe 1 sibling, 1 reply; 25+ messages in thread From: Sergey Senozhatsky @ 2018-09-19 1:50 UTC (permalink / raw) To: zhe.he; +Cc: pmladek, sergey.senozhatsky, rostedt, linux-kernel On (09/19/18 01:17), zhe.he@windriver.com wrote: > @@ -1048,7 +1048,14 @@ 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 size; unsigned int size; > + if (!str) { > + pr_err("Config string not provided\n"); pr_debug() may be? It's not clear from this error message which one of the "config strings" was not provided. Besides, I think "config string" is misleading and in fact it's a "boot command line parameter". But, dunno, I guess I'd just drop that print out. Otherwise, Acked-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com> -ss ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2 1/2] printk: Fix panic caused by passing log_buf_len to command line 2018-09-19 1:50 ` [PATCH v2 1/2] printk: Fix panic caused by passing log_buf_len to command line Sergey Senozhatsky @ 2018-09-19 2:27 ` He Zhe 2018-09-19 2:39 ` Sergey Senozhatsky 2018-09-19 6:44 ` Sergey Senozhatsky 0 siblings, 2 replies; 25+ messages in thread From: He Zhe @ 2018-09-19 2:27 UTC (permalink / raw) To: Sergey Senozhatsky; +Cc: pmladek, sergey.senozhatsky, rostedt, linux-kernel On 2018年09月19日 09:50, Sergey Senozhatsky wrote: > On (09/19/18 01:17), zhe.he@windriver.com wrote: >> @@ -1048,7 +1048,14 @@ 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 size; > unsigned int size; This is in v1 but then Steven suggested that it should be split out and only keep the pure fix part here. > >> + if (!str) { >> + pr_err("Config string not provided\n"); > pr_debug() may be? > > It's not clear from this error message which one of the "config strings" > was not provided. Besides, I think "config string" is misleading and in > fact it's a "boot command line parameter". But, dunno, I guess I'd just > drop that print out. I'm OK with dropping it. Anyway it'd get "Malformed early option 'log_buf_len'" from early_param later. I'll send v3. Thank you. Zhe > > Otherwise, > > Acked-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com> > > -ss > ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2 1/2] printk: Fix panic caused by passing log_buf_len to command line 2018-09-19 2:27 ` He Zhe @ 2018-09-19 2:39 ` Sergey Senozhatsky 2018-09-19 2:43 ` Steven Rostedt 2018-09-19 6:44 ` Sergey Senozhatsky 1 sibling, 1 reply; 25+ messages in thread From: Sergey Senozhatsky @ 2018-09-19 2:39 UTC (permalink / raw) To: He Zhe Cc: Sergey Senozhatsky, pmladek, sergey.senozhatsky, rostedt, linux-kernel On (09/19/18 10:27), He Zhe wrote: > On 2018年09月19日 09:50, Sergey Senozhatsky wrote: > > On (09/19/18 01:17), zhe.he@windriver.com wrote: > >> @@ -1048,7 +1048,14 @@ 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 size; > > unsigned int size; > > This is in v1 but then Steven suggested that it should be split out > and only keep the pure fix part here. Ah, I see. Hmm... memparse() returns u64 value. A user *probably* can ask the kernel to allocate log_buf larger than 'unsigned int'. So may be I'd do two fixes here: First - switch to u64 size. Second - check for NULL str. Steven, Petr, what do you think? -ss ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2 1/2] printk: Fix panic caused by passing log_buf_len to command line 2018-09-19 2:39 ` Sergey Senozhatsky @ 2018-09-19 2:43 ` Steven Rostedt 2018-09-19 2:47 ` Sergey Senozhatsky 2018-09-20 16:16 ` He Zhe 0 siblings, 2 replies; 25+ messages in thread From: Steven Rostedt @ 2018-09-19 2:43 UTC (permalink / raw) To: Sergey Senozhatsky; +Cc: He Zhe, pmladek, sergey.senozhatsky, linux-kernel On Wed, 19 Sep 2018 11:39:32 +0900 Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> wrote: > On (09/19/18 10:27), He Zhe wrote: > > On 2018年09月19日 09:50, Sergey Senozhatsky wrote: > > > On (09/19/18 01:17), zhe.he@windriver.com wrote: > > >> @@ -1048,7 +1048,14 @@ 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 size; > > > unsigned int size; > > > > This is in v1 but then Steven suggested that it should be split out > > and only keep the pure fix part here. > > Ah, I see. > > Hmm... memparse() returns u64 value. A user *probably* can ask the kernel > to allocate log_buf larger than 'unsigned int'. > > So may be I'd do two fixes here: > > First - switch to u64 size. > Second - check for NULL str. > > > Steven, Petr, what do you think? > I think I would switch it around. Check for NULL first, and then switch to u64. It was always an int, do we need to backport converting it to u64 to stable? The NULL check is a definite, the overflow of int shouldn't crash anything. -- Steve ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2 1/2] printk: Fix panic caused by passing log_buf_len to command line 2018-09-19 2:43 ` Steven Rostedt @ 2018-09-19 2:47 ` Sergey Senozhatsky 2018-09-19 3:05 ` Steven Rostedt 2018-09-20 16:16 ` He Zhe 1 sibling, 1 reply; 25+ messages in thread From: Sergey Senozhatsky @ 2018-09-19 2:47 UTC (permalink / raw) To: Steven Rostedt Cc: Sergey Senozhatsky, He Zhe, pmladek, sergey.senozhatsky, linux-kernel On (09/18/18 22:43), Steven Rostedt wrote: > > First - switch to u64 size. > > Second - check for NULL str. > > > I think I would switch it around. Check for NULL first, and then switch > to u64. It was always an int, do we need to backport converting it to > u64 to stable? The NULL check is a definite, the overflow of int > shouldn't crash anything. Agreed. This order makes much more sense. Do you mind, tho, to have "unsigned int size" in the first patch along with NULL str check? Just to silent the checkpatch. -ss ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2 1/2] printk: Fix panic caused by passing log_buf_len to command line 2018-09-19 2:47 ` Sergey Senozhatsky @ 2018-09-19 3:05 ` Steven Rostedt 0 siblings, 0 replies; 25+ messages in thread From: Steven Rostedt @ 2018-09-19 3:05 UTC (permalink / raw) To: Sergey Senozhatsky; +Cc: He Zhe, pmladek, sergey.senozhatsky, linux-kernel On Wed, 19 Sep 2018 11:47:54 +0900 Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> wrote: > On (09/18/18 22:43), Steven Rostedt wrote: > > > First - switch to u64 size. > > > Second - check for NULL str. > > > > > I think I would switch it around. Check for NULL first, and then switch > > to u64. It was always an int, do we need to backport converting it to > > u64 to stable? The NULL check is a definite, the overflow of int > > shouldn't crash anything. > > Agreed. This order makes much more sense. Do you mind, tho, to have > "unsigned int size" in the first patch along with NULL str check? > Just to silent the checkpatch. > I guess that doesn't hurt. I'd personally would keep it separate (just fix what's broken), but it's such a slight change, I don't have any strong feelings about it. Thanks, -- Steve ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2 1/2] printk: Fix panic caused by passing log_buf_len to command line 2018-09-19 2:43 ` Steven Rostedt 2018-09-19 2:47 ` Sergey Senozhatsky @ 2018-09-20 16:16 ` He Zhe 2018-09-20 16:30 ` Steven Rostedt 1 sibling, 1 reply; 25+ messages in thread From: He Zhe @ 2018-09-20 16:16 UTC (permalink / raw) To: Steven Rostedt, Sergey Senozhatsky Cc: pmladek, sergey.senozhatsky, linux-kernel On 2018年09月19日 10:43, Steven Rostedt wrote: > On Wed, 19 Sep 2018 11:39:32 +0900 > Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> wrote: > >> On (09/19/18 10:27), He Zhe wrote: >>> On 2018年09月19日 09:50, Sergey Senozhatsky wrote: >>>> On (09/19/18 01:17), zhe.he@windriver.com wrote: >>>>> @@ -1048,7 +1048,14 @@ 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 size; >>>> unsigned int size; >>> This is in v1 but then Steven suggested that it should be split out >>> and only keep the pure fix part here. >> Ah, I see. >> >> Hmm... memparse() returns u64 value. A user *probably* can ask the kernel >> to allocate log_buf larger than 'unsigned int'. >> >> So may be I'd do two fixes here: >> >> First - switch to u64 size. >> Second - check for NULL str. >> >> >> Steven, Petr, what do you think? >> > I think I would switch it around. Check for NULL first, and then switch > to u64. It was always an int, do we need to backport converting it to > u64 to stable? The NULL check is a definite, the overflow of int > shouldn't crash anything. To switch to u64, several variables need to be adjusted to new type to aligned with new_log_buf_len. And currently new_log_buf_len is passed to memblock_virt_alloc(phys_addr_t, phys_addr_t). So we can't simply define new_log_buf_len as u64. We need to define it as phys_addr_t tomake it work well for both 32bit and 64bit arches, since a 32-bit architecture can set ARCH_PHYS_ADDR_T_64BIT if it needs a 64-bit phys_addr_t. What do you think? #ifdef CONFIG_PHYS_ADDR_T_64BIT typedef u64 phys_addr_t; #else typedef u32 phys_addr_t; #endif Thanks, Zhe > -- Steve > ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2 1/2] printk: Fix panic caused by passing log_buf_len to command line 2018-09-20 16:16 ` He Zhe @ 2018-09-20 16:30 ` Steven Rostedt 2018-09-21 7:37 ` Petr Mladek 0 siblings, 1 reply; 25+ messages in thread From: Steven Rostedt @ 2018-09-20 16:30 UTC (permalink / raw) To: He Zhe; +Cc: Sergey Senozhatsky, pmladek, sergey.senozhatsky, linux-kernel On Fri, 21 Sep 2018 00:16:50 +0800 He Zhe <zhe.he@windriver.com> wrote: > On 2018年09月19日 10:43, Steven Rostedt wrote: > > On Wed, 19 Sep 2018 11:39:32 +0900 > > Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> wrote: > > > >> On (09/19/18 10:27), He Zhe wrote: > >>> On 2018年09月19日 09:50, Sergey Senozhatsky wrote: > >>>> On (09/19/18 01:17), zhe.he@windriver.com wrote: > >>>>> @@ -1048,7 +1048,14 @@ 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 size; > >>>> unsigned int size; > >>> This is in v1 but then Steven suggested that it should be split out > >>> and only keep the pure fix part here. > >> Ah, I see. > >> > >> Hmm... memparse() returns u64 value. A user *probably* can ask the kernel > >> to allocate log_buf larger than 'unsigned int'. > >> > >> So may be I'd do two fixes here: > >> > >> First - switch to u64 size. > >> Second - check for NULL str. > >> > >> > >> Steven, Petr, what do you think? > >> > > I think I would switch it around. Check for NULL first, and then switch > > to u64. It was always an int, do we need to backport converting it to > > u64 to stable? The NULL check is a definite, the overflow of int > > shouldn't crash anything. > Hi Zhe, > To switch to u64, several variables need to be adjusted to new type to aligned > with new_log_buf_len. And currently new_log_buf_len is passed to > memblock_virt_alloc(phys_addr_t, phys_addr_t). So we can't simply define > new_log_buf_len as u64. We need to define it as phys_addr_t tomake it work > well for both 32bit and 64bit arches, since a 32-bit architecture can set > ARCH_PHYS_ADDR_T_64BIT if it needs a 64-bit phys_addr_t. The above explanation verifies more that the NULL pointer check needs to be first, and that the change in size should not be backported to stable because it has a high risk to doing the change as compared to it being a problem for older kernels. > > What do you think? Sounds good to me. What do others think? -- Steve ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2 1/2] printk: Fix panic caused by passing log_buf_len to command line 2018-09-20 16:30 ` Steven Rostedt @ 2018-09-21 7:37 ` Petr Mladek 2018-09-22 15:36 ` He Zhe 2018-09-25 12:01 ` Sergey Senozhatsky 0 siblings, 2 replies; 25+ messages in thread From: Petr Mladek @ 2018-09-21 7:37 UTC (permalink / raw) To: Steven Rostedt Cc: He Zhe, Sergey Senozhatsky, sergey.senozhatsky, linux-kernel On Thu 2018-09-20 12:30:56, Steven Rostedt wrote: > On Fri, 21 Sep 2018 00:16:50 +0800 > He Zhe <zhe.he@windriver.com> wrote: > > > On 2018年09月19日 10:43, Steven Rostedt wrote: > > > On Wed, 19 Sep 2018 11:39:32 +0900 > > > Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> wrote: > > > > > >> On (09/19/18 10:27), He Zhe wrote: > > >>> On 2018年09月19日 09:50, Sergey Senozhatsky wrote: > > >>>> On (09/19/18 01:17), zhe.he@windriver.com wrote: > > >>>>> @@ -1048,7 +1048,14 @@ 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 size; > > >>>> unsigned int size; > > >>> This is in v1 but then Steven suggested that it should be split out > > >>> and only keep the pure fix part here. > > >> Ah, I see. > > >> > > >> Hmm... memparse() returns u64 value. A user *probably* can ask the kernel > > >> to allocate log_buf larger than 'unsigned int'. > > >> > > >> So may be I'd do two fixes here: > > >> > > >> First - switch to u64 size. > > >> Second - check for NULL str. > > >> > > >> > > >> Steven, Petr, what do you think? > > >> > > > I think I would switch it around. Check for NULL first, and then switch > > > to u64. It was always an int, do we need to backport converting it to > > > u64 to stable? The NULL check is a definite, the overflow of int > > > shouldn't crash anything. > > > > Hi Zhe, > > > To switch to u64, several variables need to be adjusted to new type to aligned > > with new_log_buf_len. And currently new_log_buf_len is passed to > > memblock_virt_alloc(phys_addr_t, phys_addr_t). So we can't simply define > > new_log_buf_len as u64. We need to define it as phys_addr_t tomake it work > > well for both 32bit and 64bit arches, since a 32-bit architecture can set > > ARCH_PHYS_ADDR_T_64BIT if it needs a 64-bit phys_addr_t. > > The above explanation verifies more that the NULL pointer check needs > to be first, and that the change in size should not be backported to > stable because it has a high risk to doing the change as compared to it > being a problem for older kernels. I agree that the NULL check should go first. I would personally keep the size as unsigned int. IMHO, a support for a log buffer bigger than 4GB is not worth the complexity. Best Regards, Petr ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2 1/2] printk: Fix panic caused by passing log_buf_len to command line 2018-09-21 7:37 ` Petr Mladek @ 2018-09-22 15:36 ` He Zhe 2018-09-25 12:16 ` Sergey Senozhatsky 2018-09-25 12:01 ` Sergey Senozhatsky 1 sibling, 1 reply; 25+ messages in thread From: He Zhe @ 2018-09-22 15:36 UTC (permalink / raw) To: Petr Mladek, Steven Rostedt Cc: Sergey Senozhatsky, sergey.senozhatsky, linux-kernel On 2018年09月21日 15:37, Petr Mladek wrote: > On Thu 2018-09-20 12:30:56, Steven Rostedt wrote: >> On Fri, 21 Sep 2018 00:16:50 +0800 >> He Zhe <zhe.he@windriver.com> wrote: >> >>> On 2018年09月19日 10:43, Steven Rostedt wrote: >>>> On Wed, 19 Sep 2018 11:39:32 +0900 >>>> Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> wrote: >>>> >>>>> On (09/19/18 10:27), He Zhe wrote: >>>>>> On 2018年09月19日 09:50, Sergey Senozhatsky wrote: >>>>>>> On (09/19/18 01:17), zhe.he@windriver.com wrote: >>>>>>>> @@ -1048,7 +1048,14 @@ 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 size; >>>>>>> unsigned int size; >>>>>> This is in v1 but then Steven suggested that it should be split out >>>>>> and only keep the pure fix part here. >>>>> Ah, I see. >>>>> >>>>> Hmm... memparse() returns u64 value. A user *probably* can ask the kernel >>>>> to allocate log_buf larger than 'unsigned int'. >>>>> >>>>> So may be I'd do two fixes here: >>>>> >>>>> First - switch to u64 size. >>>>> Second - check for NULL str. >>>>> >>>>> >>>>> Steven, Petr, what do you think? >>>>> >>>> I think I would switch it around. Check for NULL first, and then switch >>>> to u64. It was always an int, do we need to backport converting it to >>>> u64 to stable? The NULL check is a definite, the overflow of int >>>> shouldn't crash anything. >> Hi Zhe, >> >>> To switch to u64, several variables need to be adjusted to new type to aligned >>> with new_log_buf_len. And currently new_log_buf_len is passed to >>> memblock_virt_alloc(phys_addr_t, phys_addr_t). So we can't simply define >>> new_log_buf_len as u64. We need to define it as phys_addr_t tomake it work >>> well for both 32bit and 64bit arches, since a 32-bit architecture can set >>> ARCH_PHYS_ADDR_T_64BIT if it needs a 64-bit phys_addr_t. >> The above explanation verifies more that the NULL pointer check needs >> to be first, and that the change in size should not be backported to >> stable because it has a high risk to doing the change as compared to it >> being a problem for older kernels. > I agree that the NULL check should go first. > > I would personally keep the size as unsigned int. IMHO, a support > for a log buffer bigger than 4GB is not worth the complexity. Thank you, Petr, Steven and Sergey. I'll send v3 soon for the NULL check and cast correction. For 64 bit log buffer length, the variable changes should be OK. The main obstacle might be that we need to modify the syscall below to make the 64 bit length returned back to user space. "error" is not long enough. int do_syslog(int type, char __user *buf, int len, int source) ... case SYSLOG_ACTION_SIZE_BUFFER: error = log_buf_len; break; ... return error; ... SYSCALL_DEFINE3(syslog, int, type, char __user *, buf, int, len) { return do_syslog(type, buf, len, SYSLOG_FROM_READER); } Besides, in terms of variable type alignment, there has already been a similar problematic case in do_syslog as below. i.e. int = u64 - u64. It seems this needs to be fixed. int do_syslog(int type, char __user *buf, int len, int source) ... case SYSLOG_ACTION_SIZE_UNREAD: if (source == SYSLOG_FROM_PROC) { error = log_next_seq - syslog_seq; ... I'd like to fix the above issues. But can I have your opinions about how to handle the syscall? Thanks, Zhe > > Best Regards, > Petr > ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2 1/2] printk: Fix panic caused by passing log_buf_len to command line 2018-09-22 15:36 ` He Zhe @ 2018-09-25 12:16 ` Sergey Senozhatsky 0 siblings, 0 replies; 25+ messages in thread From: Sergey Senozhatsky @ 2018-09-25 12:16 UTC (permalink / raw) To: He Zhe Cc: Petr Mladek, Steven Rostedt, Sergey Senozhatsky, sergey.senozhatsky, linux-kernel On (09/22/18 23:36), He Zhe wrote: > I'd like to fix the above issues. But can I have your opinions about how to > handle the syscall? Hmm. This part is complex, I need to look at it more. A signed int is a good buffer size limit for 32-bit user space. But this also means that syslog might experience some problems doing SYSLOG_ACTION_READ_ALL on a 32-bit system with a 4G log_buf. Not to mention SYSLOG_ACTION_READ_ALL on a 64-bit system with a log_buf=32G. So *maybe* things are alredy a bit broken in this department. Dunno, need to think more. -ss ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2 1/2] printk: Fix panic caused by passing log_buf_len to command line 2018-09-21 7:37 ` Petr Mladek 2018-09-22 15:36 ` He Zhe @ 2018-09-25 12:01 ` Sergey Senozhatsky 2018-09-25 12:23 ` Petr Mladek 1 sibling, 1 reply; 25+ messages in thread From: Sergey Senozhatsky @ 2018-09-25 12:01 UTC (permalink / raw) To: Petr Mladek Cc: Steven Rostedt, He Zhe, Sergey Senozhatsky, sergey.senozhatsky, linux-kernel On (09/21/18 09:37), Petr Mladek wrote: > > I would personally keep the size as unsigned int. IMHO, a support > for a log buffer bigger than 4GB is not worth the complexity. > ftrace dumps are bothering me. Steven Rostedt wrote [0]: > > Especially when I have a machine with 240 CPUs. But it also has a ton of > RAM, I could easily do log_buf_len=32G > The systems are getting bigger, so log_buf_len=UINT_MAX+ might become a norm at some point. [0] https://lore.kernel.org/lkml/20170428101659.7cd879e7@gandalf.local.home/ -ss ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2 1/2] printk: Fix panic caused by passing log_buf_len to command line 2018-09-25 12:01 ` Sergey Senozhatsky @ 2018-09-25 12:23 ` Petr Mladek 2018-09-25 12:37 ` Sergey Senozhatsky 2018-09-25 13:31 ` Sergey Senozhatsky 0 siblings, 2 replies; 25+ messages in thread From: Petr Mladek @ 2018-09-25 12:23 UTC (permalink / raw) To: Sergey Senozhatsky Cc: Steven Rostedt, He Zhe, Sergey Senozhatsky, linux-kernel On Tue 2018-09-25 21:01:35, Sergey Senozhatsky wrote: > On (09/21/18 09:37), Petr Mladek wrote: > > > > I would personally keep the size as unsigned int. IMHO, a support > > for a log buffer bigger than 4GB is not worth the complexity. > > > > ftrace dumps are bothering me. > > Steven Rostedt wrote [0]: > > > > Especially when I have a machine with 240 CPUs. But it also has a ton of > > RAM, I could easily do log_buf_len=32G > > > > The systems are getting bigger, so log_buf_len=UINT_MAX+ might become > a norm at some point. Thanks for pointing this out. Well, it seems that the change would require a new syscall to pass the buffer size as long. We need to be sure that people would use this in the real life. This thread suggested this change to avoid a checkpatch warning. The 32GB was mentioned as an example one year ego. This is not enough for a new syscall from my point of view. Best Regards, Petr ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2 1/2] printk: Fix panic caused by passing log_buf_len to command line 2018-09-25 12:23 ` Petr Mladek @ 2018-09-25 12:37 ` Sergey Senozhatsky 2018-09-25 13:31 ` Sergey Senozhatsky 1 sibling, 0 replies; 25+ messages in thread From: Sergey Senozhatsky @ 2018-09-25 12:37 UTC (permalink / raw) To: Petr Mladek Cc: Sergey Senozhatsky, Steven Rostedt, He Zhe, Sergey Senozhatsky, linux-kernel On (09/25/18 14:23), Petr Mladek wrote: > > Thanks for pointing this out. Well, it seems that the change would > require a new syscall to pass the buffer size as long. We need to > be sure that people would use this in the real life. Agreed. > This thread suggested this change to avoid a checkpatch warning. Not exactly. I suggested u64 change not because of a checkpatch warning. But because of u64 memparse() return and because of potential log_buf_len=4G+ -ss ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2 1/2] printk: Fix panic caused by passing log_buf_len to command line 2018-09-25 12:23 ` Petr Mladek 2018-09-25 12:37 ` Sergey Senozhatsky @ 2018-09-25 13:31 ` Sergey Senozhatsky 2018-09-25 15:31 ` He Zhe 2018-09-26 11:05 ` Petr Mladek 1 sibling, 2 replies; 25+ messages in thread From: Sergey Senozhatsky @ 2018-09-25 13:31 UTC (permalink / raw) To: Petr Mladek Cc: Sergey Senozhatsky, Steven Rostedt, He Zhe, Sergey Senozhatsky, linux-kernel On (09/25/18 14:23), Petr Mladek wrote: > The 32GB was mentioned as an example one year ego. This is not enough > for a new syscall from my point of view. I agree. I didn't think of syslog(); was merely thinking about logbuf and flushing it to the consoles. syslog() stuff is a bit complex. We sort of don't expect user space to allocate 64G to read all log_buf messages, do we. I'm wondering if we can do something like this --- diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c index cf275f4d7912..1b48b61da8fe 100644 --- a/kernel/printk/printk.c +++ b/kernel/printk/printk.c @@ -1110,9 +1110,15 @@ 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); + u64 size = memparse(str, &str); - log_buf_len_update(size); + if (size > UINT_MAX) { + size = UINT_MAX; + pr_err("log_buf over 4G is not supported. " + "Please contact printk maintainers.\n"); + } + + log_buf_len_update((unsigned int)size); return 0; } --- So we could know that "the day has come". -ss ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH v2 1/2] printk: Fix panic caused by passing log_buf_len to command line 2018-09-25 13:31 ` Sergey Senozhatsky @ 2018-09-25 15:31 ` He Zhe 2018-09-26 11:05 ` Petr Mladek 1 sibling, 0 replies; 25+ messages in thread From: He Zhe @ 2018-09-25 15:31 UTC (permalink / raw) To: Sergey Senozhatsky, Petr Mladek Cc: Steven Rostedt, Sergey Senozhatsky, linux-kernel On 2018年09月25日 21:31, Sergey Senozhatsky wrote: > On (09/25/18 14:23), Petr Mladek wrote: >> The 32GB was mentioned as an example one year ego. This is not enough >> for a new syscall from my point of view. > I agree. I didn't think of syslog(); was merely thinking about logbuf > and flushing it to the consoles. syslog() stuff is a bit complex. We > sort of don't expect user space to allocate 64G to read all log_buf > messages, do we. > > I'm wondering if we can do something like this > > --- > > diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c > index cf275f4d7912..1b48b61da8fe 100644 > --- a/kernel/printk/printk.c > +++ b/kernel/printk/printk.c > @@ -1110,9 +1110,15 @@ 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); > + u64 size = memparse(str, &str); > > - log_buf_len_update(size); > + if (size > UINT_MAX) { > + size = UINT_MAX; > + pr_err("log_buf over 4G is not supported. " > + "Please contact printk maintainers.\n"); > + } > + > + log_buf_len_update((unsigned int)size); > > return 0; > } > > --- > > So we could know that "the day has come". I agree on this idea, a good way to collect the potential requirement. I can send v4 with it if no objection from other people. Thanks, Zhe > > -ss > ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2 1/2] printk: Fix panic caused by passing log_buf_len to command line 2018-09-25 13:31 ` Sergey Senozhatsky 2018-09-25 15:31 ` He Zhe @ 2018-09-26 11:05 ` Petr Mladek 2018-09-28 7:35 ` Sergey Senozhatsky 1 sibling, 1 reply; 25+ messages in thread From: Petr Mladek @ 2018-09-26 11:05 UTC (permalink / raw) To: Sergey Senozhatsky Cc: Steven Rostedt, He Zhe, Sergey Senozhatsky, linux-kernel On Tue 2018-09-25 22:31:43, Sergey Senozhatsky wrote: > On (09/25/18 14:23), Petr Mladek wrote: > > The 32GB was mentioned as an example one year ego. This is not enough > > for a new syscall from my point of view. > > I agree. I didn't think of syslog(); was merely thinking about logbuf > and flushing it to the consoles. syslog() stuff is a bit complex. We > sort of don't expect user space to allocate 64G to read all log_buf > messages, do we. > > I'm wondering if we can do something like this > > --- > > diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c > index cf275f4d7912..1b48b61da8fe 100644 > --- a/kernel/printk/printk.c > +++ b/kernel/printk/printk.c > @@ -1110,9 +1110,15 @@ 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); > + u64 size = memparse(str, &str); > > - log_buf_len_update(size); > + if (size > UINT_MAX) { > + size = UINT_MAX; > + pr_err("log_buf over 4G is not supported. " > + "Please contact printk maintainers.\n"); > + } > + > + log_buf_len_update((unsigned int)size); > > return 0; > } > > --- > > So we could know that "the day has come". Sounds good to me. Just two nits. First, I would move the check into log_buf_len_update() so that we catch the overflow also in other situations. Second, please, keep only the first line. It is enough to describe the problem. Upstream kernel maintainers are not responsible for implementing all missing features. Best Regards, Petr ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2 1/2] printk: Fix panic caused by passing log_buf_len to command line 2018-09-26 11:05 ` Petr Mladek @ 2018-09-28 7:35 ` Sergey Senozhatsky 2018-09-28 8:22 ` He Zhe 0 siblings, 1 reply; 25+ messages in thread From: Sergey Senozhatsky @ 2018-09-28 7:35 UTC (permalink / raw) To: Petr Mladek Cc: Sergey Senozhatsky, Steven Rostedt, He Zhe, Sergey Senozhatsky, linux-kernel On (09/26/18 13:05), Petr Mladek wrote: > First, I would move the check into log_buf_len_update() so that > we catch the overflow also in other situations. OK. > Second, please, keep only the first line. OK. > It is enough to describe the problem. OK. He Zhe, will you pick it up? -ss ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2 1/2] printk: Fix panic caused by passing log_buf_len to command line 2018-09-28 7:35 ` Sergey Senozhatsky @ 2018-09-28 8:22 ` He Zhe 0 siblings, 0 replies; 25+ messages in thread From: He Zhe @ 2018-09-28 8:22 UTC (permalink / raw) To: Sergey Senozhatsky, Petr Mladek Cc: Sergey Senozhatsky, Steven Rostedt, linux-kernel On 2018年09月28日 15:35, Sergey Senozhatsky wrote: > On (09/26/18 13:05), Petr Mladek wrote: >> First, I would move the check into log_buf_len_update() so that >> we catch the overflow also in other situations. > OK. > >> Second, please, keep only the first line. > OK. > >> It is enough to describe the problem. > OK. > He Zhe, will you pick it up? Yes, I agree, I'll send v3 ASAP. Thanks, Zhe > > -ss > ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2 1/2] printk: Fix panic caused by passing log_buf_len to command line 2018-09-19 2:27 ` He Zhe 2018-09-19 2:39 ` Sergey Senozhatsky @ 2018-09-19 6:44 ` Sergey Senozhatsky 2018-09-19 10:09 ` He Zhe 1 sibling, 1 reply; 25+ messages in thread From: Sergey Senozhatsky @ 2018-09-19 6:44 UTC (permalink / raw) To: He Zhe, rostedt, pmladek Cc: Sergey Senozhatsky, sergey.senozhatsky, linux-kernel On (09/19/18 10:27), He Zhe wrote: > On 2018年09月19日 09:50, Sergey Senozhatsky wrote: > > On (09/19/18 01:17), zhe.he@windriver.com wrote: > >> @@ -1048,7 +1048,14 @@ 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 size; > > unsigned int size; > > This is in v1 but then Steven suggested that it should be split out > and only keep the pure fix part here. JFI Seems there are more patches like this. I found this one in MM list: lkml.kernel.org/r/1537284788-428784-1-git-send-email-zhe.he@windriver.com Andrew's response: lkml.kernel.org/r/20180918141917.2cb16b01c122dbe1ead2f657@linux-foundation.org -ss ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2 1/2] printk: Fix panic caused by passing log_buf_len to command line 2018-09-19 6:44 ` Sergey Senozhatsky @ 2018-09-19 10:09 ` He Zhe 0 siblings, 0 replies; 25+ messages in thread From: He Zhe @ 2018-09-19 10:09 UTC (permalink / raw) To: Sergey Senozhatsky, rostedt, pmladek; +Cc: sergey.senozhatsky, linux-kernel On 2018年09月19日 14:44, Sergey Senozhatsky wrote: > On (09/19/18 10:27), He Zhe wrote: >> On 2018年09月19日 09:50, Sergey Senozhatsky wrote: >>> On (09/19/18 01:17), zhe.he@windriver.com wrote: >>>> @@ -1048,7 +1048,14 @@ 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 size; >>> unsigned int size; >> This is in v1 but then Steven suggested that it should be split out >> and only keep the pure fix part here. > JFI > > Seems there are more patches like this. I found this one in MM list: > lkml.kernel.org/r/1537284788-428784-1-git-send-email-zhe.he@windriver.com > > Andrew's response: > lkml.kernel.org/r/20180918141917.2cb16b01c122dbe1ead2f657@linux-foundation.org I just replied. Let's see what Andrew would say. https://lore.kernel.org/lkml/1c32c1d2-a54a-30f7-1afb-ad6d3282f54a@windriver.com/ Thanks, Zhe > > -ss > ^ permalink raw reply [flat|nested] 25+ messages in thread
end of thread, other threads:[~2018-09-28 8:24 UTC | newest] Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-09-18 17:17 [PATCH v2 1/2] printk: Fix panic caused by passing log_buf_len to command line zhe.he 2018-09-18 17:17 ` [PATCH v2 2/2] printk: Add KBUILD_MODNAME and correct bare use of unsigned zhe.he 2018-09-19 2:06 ` Sergey Senozhatsky 2018-09-19 11:20 ` Petr Mladek 2018-09-19 1:50 ` [PATCH v2 1/2] printk: Fix panic caused by passing log_buf_len to command line Sergey Senozhatsky 2018-09-19 2:27 ` He Zhe 2018-09-19 2:39 ` Sergey Senozhatsky 2018-09-19 2:43 ` Steven Rostedt 2018-09-19 2:47 ` Sergey Senozhatsky 2018-09-19 3:05 ` Steven Rostedt 2018-09-20 16:16 ` He Zhe 2018-09-20 16:30 ` Steven Rostedt 2018-09-21 7:37 ` Petr Mladek 2018-09-22 15:36 ` He Zhe 2018-09-25 12:16 ` Sergey Senozhatsky 2018-09-25 12:01 ` Sergey Senozhatsky 2018-09-25 12:23 ` Petr Mladek 2018-09-25 12:37 ` Sergey Senozhatsky 2018-09-25 13:31 ` Sergey Senozhatsky 2018-09-25 15:31 ` He Zhe 2018-09-26 11:05 ` Petr Mladek 2018-09-28 7:35 ` Sergey Senozhatsky 2018-09-28 8:22 ` He Zhe 2018-09-19 6:44 ` Sergey Senozhatsky 2018-09-19 10:09 ` 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).