* [PATCH v2 0/2] fix parsing of reboot= cmdline
@ 2020-10-27 13:35 Matteo Croce
2020-10-27 13:35 ` [PATCH v2 1/2] reboot: fix overflow parsing reboot cpu number Matteo Croce
` (2 more replies)
0 siblings, 3 replies; 15+ messages in thread
From: Matteo Croce @ 2020-10-27 13:35 UTC (permalink / raw)
To: linux-kernel
Cc: Guenter Roeck, Petr Mladek, Arnd Bergmann, Mike Rapoport,
Kees Cook, Pavel Tatashin, Robin Holt, Fabian Frederick, stable
From: Matteo Croce <mcroce@microsoft.com>
The parsing of the reboot= cmdline has two major errors:
- a missing bound check can crash the system on reboot
- parsing of the cpu number only works if specified last
Fix both, along with a small code refactor.
v1->v2:
As Petr suggested, don't force base 10 in simple_strtoul(),
so hex values are accepted as well.
Matteo Croce (2):
reboot: fix overflow parsing reboot cpu number
reboot: fix parsing of reboot cpu number
kernel/reboot.c | 24 +++++++++++++-----------
1 file changed, 13 insertions(+), 11 deletions(-)
--
2.28.0
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 1/2] reboot: fix overflow parsing reboot cpu number
2020-10-27 13:35 [PATCH v2 0/2] fix parsing of reboot= cmdline Matteo Croce
@ 2020-10-27 13:35 ` Matteo Croce
2020-10-27 13:42 ` Greg KH
2020-10-27 13:35 ` [PATCH v2 2/2] reboot: fix parsing of " Matteo Croce
2020-10-27 13:42 ` [PATCH v2 0/2] fix parsing of reboot= cmdline Greg KH
2 siblings, 1 reply; 15+ messages in thread
From: Matteo Croce @ 2020-10-27 13:35 UTC (permalink / raw)
To: linux-kernel
Cc: Guenter Roeck, Petr Mladek, Arnd Bergmann, Mike Rapoport,
Kees Cook, Pavel Tatashin, Robin Holt, Fabian Frederick, stable
From: Matteo Croce <mcroce@microsoft.com>
Limit the CPU number to num_possible_cpus(), because setting it
to a value lower than INT_MAX but higher than NR_CPUS produces the
following error on reboot and shutdown:
BUG: unable to handle page fault for address: ffffffff90ab1bb0
#PF: supervisor read access in kernel mode
#PF: error_code(0x0000) - not-present page
PGD 1c09067 P4D 1c09067 PUD 1c0a063 PMD 0
Oops: 0000 [#1] SMP
CPU: 1 PID: 1 Comm: systemd-shutdow Not tainted 5.9.0-rc8-kvm #110
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.13.0-2.fc32 04/01/2014
RIP: 0010:migrate_to_reboot_cpu+0xe/0x60
Code: ea ea 00 48 89 fa 48 c7 c7 30 57 f1 81 e9 fa ef ff ff 66 2e 0f 1f 84 00 00 00 00 00 53 8b 1d d5 ea ea 00 e8 14 33 fe ff 89 da <48> 0f a3 15 ea fc bd 00 48 89 d0 73 29 89 c2 c1 e8 06 65 48 8b 3c
RSP: 0018:ffffc90000013e08 EFLAGS: 00010246
RAX: ffff88801f0a0000 RBX: 0000000077359400 RCX: 0000000000000000
RDX: 0000000077359400 RSI: 0000000000000002 RDI: ffffffff81c199e0
RBP: ffffffff81c1e3c0 R08: ffff88801f41f000 R09: ffffffff81c1e348
R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000
R13: 00007f32bedf8830 R14: 00000000fee1dead R15: 0000000000000000
FS: 00007f32bedf8980(0000) GS:ffff88801f480000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: ffffffff90ab1bb0 CR3: 000000001d057000 CR4: 00000000000006a0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
__do_sys_reboot.cold+0x34/0x5b
? vfs_writev+0x92/0xc0
? do_writev+0x52/0xd0
do_syscall_64+0x2d/0x40
entry_SYSCALL_64_after_hwframe+0x44/0xa9
RIP: 0033:0x7f32bfaaecd3
Code: 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 89 fa be 69 19 12 28 bf ad de e1 fe b8 a9 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 05 c3 0f 1f 40 00 48 8b 15 89 81 0c 00 f7 d8
RSP: 002b:00007fff6265fb58 EFLAGS: 00000202 ORIG_RAX: 00000000000000a9
RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007f32bfaaecd3
RDX: 0000000001234567 RSI: 0000000028121969 RDI: 00000000fee1dead
RBP: 0000000000000000 R08: 0000000000008020 R09: 00007fff6265ef60
R10: 00007f32bedf8830 R11: 0000000000000202 R12: 0000000000000000
R13: 0000557bba2c51c0 R14: 0000000000000000 R15: 00007fff6265fbc8
CR2: ffffffff90ab1bb0
---[ end trace b813e80157136563 ]---
RIP: 0010:migrate_to_reboot_cpu+0xe/0x60
Code: ea ea 00 48 89 fa 48 c7 c7 30 57 f1 81 e9 fa ef ff ff 66 2e 0f 1f 84 00 00 00 00 00 53 8b 1d d5 ea ea 00 e8 14 33 fe ff 89 da <48> 0f a3 15 ea fc bd 00 48 89 d0 73 29 89 c2 c1 e8 06 65 48 8b 3c
RSP: 0018:ffffc90000013e08 EFLAGS: 00010246
RAX: ffff88801f0a0000 RBX: 0000000077359400 RCX: 0000000000000000
RDX: 0000000077359400 RSI: 0000000000000002 RDI: ffffffff81c199e0
RBP: ffffffff81c1e3c0 R08: ffff88801f41f000 R09: ffffffff81c1e348
R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000
R13: 00007f32bedf8830 R14: 00000000fee1dead R15: 0000000000000000
FS: 00007f32bedf8980(0000) GS:ffff88801f480000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: ffffffff90ab1bb0 CR3: 000000001d057000 CR4: 00000000000006a0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Kernel panic - not syncing: Attempted to kill init! exitcode=0x00000009
Kernel Offset: disabled
---[ end Kernel panic - not syncing: Attempted to kill init! exitcode=0x00000009 ]---
Fixes: 1b3a5d02ee07 ("reboot: move arch/x86 reboot= handling to generic kernel")
Signed-off-by: Matteo Croce <mcroce@microsoft.com>
---
kernel/reboot.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/kernel/reboot.c b/kernel/reboot.c
index e7b78d5ae1ab..c4e7965c39b9 100644
--- a/kernel/reboot.c
+++ b/kernel/reboot.c
@@ -558,11 +558,19 @@ static int __init reboot_setup(char *str)
rc = kstrtoint(str+1, 0, &reboot_cpu);
if (rc)
return rc;
+ if (reboot_cpu >= num_possible_cpus()) {
+ reboot_cpu = 0;
+ return -ERANGE;
+ }
} else if (str[1] == 'm' && str[2] == 'p' &&
isdigit(*(str+3))) {
rc = kstrtoint(str+3, 0, &reboot_cpu);
if (rc)
return rc;
+ if (reboot_cpu >= num_possible_cpus()) {
+ reboot_cpu = 0;
+ return -ERANGE;
+ }
} else
*mode = REBOOT_SOFT;
break;
--
2.28.0
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v2 2/2] reboot: fix parsing of reboot cpu number
2020-10-27 13:35 [PATCH v2 0/2] fix parsing of reboot= cmdline Matteo Croce
2020-10-27 13:35 ` [PATCH v2 1/2] reboot: fix overflow parsing reboot cpu number Matteo Croce
@ 2020-10-27 13:35 ` Matteo Croce
2020-10-27 13:42 ` Greg KH
2020-10-30 14:30 ` Petr Mladek
2020-10-27 13:42 ` [PATCH v2 0/2] fix parsing of reboot= cmdline Greg KH
2 siblings, 2 replies; 15+ messages in thread
From: Matteo Croce @ 2020-10-27 13:35 UTC (permalink / raw)
To: linux-kernel
Cc: Guenter Roeck, Petr Mladek, Arnd Bergmann, Mike Rapoport,
Kees Cook, Pavel Tatashin, Robin Holt, Fabian Frederick, stable
From: Matteo Croce <mcroce@microsoft.com>
The kernel cmdline reboot= argument allows to specify the CPU used
for rebooting, with the syntax `s####` among the other flags, e.g.
reboot=soft,s4
reboot=warm,s31,force
In the early days the parsing was done with simple_strtoul(), later
deprecated in favor of the safer kstrtoint() which handles overflow.
But kstrtoint() returns -EINVAL if there are non-digit characters
in a string, so if this flag is not the last given, it's silently
ignored as well as the subsequent ones.
To fix it, revert the usage of simple_strtoul(), which is no longer
deprecated, and restore the old behaviour.
While at it, merge two identical code blocks into one.
Fixes: 616feab75397 ("kernel/reboot.c: convert simple_strtoul to kstrtoint")
Signed-off-by: Matteo Croce <mcroce@microsoft.com>
---
kernel/reboot.c | 30 ++++++++++++------------------
1 file changed, 12 insertions(+), 18 deletions(-)
diff --git a/kernel/reboot.c b/kernel/reboot.c
index c4e7965c39b9..a09c5937c0b6 100644
--- a/kernel/reboot.c
+++ b/kernel/reboot.c
@@ -552,25 +552,19 @@ static int __init reboot_setup(char *str)
case 's':
{
- int rc;
-
- if (isdigit(*(str+1))) {
- rc = kstrtoint(str+1, 0, &reboot_cpu);
- if (rc)
- return rc;
- if (reboot_cpu >= num_possible_cpus()) {
- reboot_cpu = 0;
- return -ERANGE;
- }
- } else if (str[1] == 'm' && str[2] == 'p' &&
- isdigit(*(str+3))) {
- rc = kstrtoint(str+3, 0, &reboot_cpu);
- if (rc)
- return rc;
- if (reboot_cpu >= num_possible_cpus()) {
- reboot_cpu = 0;
+ int cpu;
+
+ /*
+ * reboot_cpu is s[mp]#### with #### being the processor
+ * to be used for rebooting. Skip 's' or 'smp' prefix.
+ */
+ str += str[1] == 'm' && str[2] == 'p' ? 3 : 1;
+
+ if (isdigit(str[0])) {
+ cpu = simple_strtoul(str, NULL, 0);
+ if (cpu >= num_possible_cpus())
return -ERANGE;
- }
+ reboot_cpu = cpu;
} else
*mode = REBOOT_SOFT;
break;
--
2.28.0
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v2 0/2] fix parsing of reboot= cmdline
2020-10-27 13:35 [PATCH v2 0/2] fix parsing of reboot= cmdline Matteo Croce
2020-10-27 13:35 ` [PATCH v2 1/2] reboot: fix overflow parsing reboot cpu number Matteo Croce
2020-10-27 13:35 ` [PATCH v2 2/2] reboot: fix parsing of " Matteo Croce
@ 2020-10-27 13:42 ` Greg KH
2 siblings, 0 replies; 15+ messages in thread
From: Greg KH @ 2020-10-27 13:42 UTC (permalink / raw)
To: Matteo Croce
Cc: linux-kernel, Guenter Roeck, Petr Mladek, Arnd Bergmann,
Mike Rapoport, Kees Cook, Pavel Tatashin, Robin Holt,
Fabian Frederick, stable
On Tue, Oct 27, 2020 at 02:35:43PM +0100, Matteo Croce wrote:
> From: Matteo Croce <mcroce@microsoft.com>
>
> The parsing of the reboot= cmdline has two major errors:
> - a missing bound check can crash the system on reboot
> - parsing of the cpu number only works if specified last
>
> Fix both, along with a small code refactor.
>
> v1->v2:
> As Petr suggested, don't force base 10 in simple_strtoul(),
> so hex values are accepted as well.
>
> Matteo Croce (2):
> reboot: fix overflow parsing reboot cpu number
> reboot: fix parsing of reboot cpu number
>
> kernel/reboot.c | 24 +++++++++++++-----------
> 1 file changed, 13 insertions(+), 11 deletions(-)
>
> --
> 2.28.0
>
<formletter>
This is not the correct way to submit patches for inclusion in the
stable kernel tree. Please read:
https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
for how to do this properly.
</formletter>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 2/2] reboot: fix parsing of reboot cpu number
2020-10-27 13:35 ` [PATCH v2 2/2] reboot: fix parsing of " Matteo Croce
@ 2020-10-27 13:42 ` Greg KH
2020-10-30 14:30 ` Petr Mladek
1 sibling, 0 replies; 15+ messages in thread
From: Greg KH @ 2020-10-27 13:42 UTC (permalink / raw)
To: Matteo Croce
Cc: linux-kernel, Guenter Roeck, Petr Mladek, Arnd Bergmann,
Mike Rapoport, Kees Cook, Pavel Tatashin, Robin Holt,
Fabian Frederick, stable
On Tue, Oct 27, 2020 at 02:35:45PM +0100, Matteo Croce wrote:
> From: Matteo Croce <mcroce@microsoft.com>
>
> The kernel cmdline reboot= argument allows to specify the CPU used
> for rebooting, with the syntax `s####` among the other flags, e.g.
>
> reboot=soft,s4
> reboot=warm,s31,force
>
> In the early days the parsing was done with simple_strtoul(), later
> deprecated in favor of the safer kstrtoint() which handles overflow.
>
> But kstrtoint() returns -EINVAL if there are non-digit characters
> in a string, so if this flag is not the last given, it's silently
> ignored as well as the subsequent ones.
>
> To fix it, revert the usage of simple_strtoul(), which is no longer
> deprecated, and restore the old behaviour.
>
> While at it, merge two identical code blocks into one.
>
> Fixes: 616feab75397 ("kernel/reboot.c: convert simple_strtoul to kstrtoint")
> Signed-off-by: Matteo Croce <mcroce@microsoft.com>
> ---
> kernel/reboot.c | 30 ++++++++++++------------------
> 1 file changed, 12 insertions(+), 18 deletions(-)
>
<formletter>
This is not the correct way to submit patches for inclusion in the
stable kernel tree. Please read:
https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
for how to do this properly.
</formletter>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 1/2] reboot: fix overflow parsing reboot cpu number
2020-10-27 13:35 ` [PATCH v2 1/2] reboot: fix overflow parsing reboot cpu number Matteo Croce
@ 2020-10-27 13:42 ` Greg KH
2020-10-30 14:13 ` Petr Mladek
0 siblings, 1 reply; 15+ messages in thread
From: Greg KH @ 2020-10-27 13:42 UTC (permalink / raw)
To: Matteo Croce
Cc: linux-kernel, Guenter Roeck, Petr Mladek, Arnd Bergmann,
Mike Rapoport, Kees Cook, Pavel Tatashin, Robin Holt,
Fabian Frederick, stable
On Tue, Oct 27, 2020 at 02:35:44PM +0100, Matteo Croce wrote:
> From: Matteo Croce <mcroce@microsoft.com>
>
> Limit the CPU number to num_possible_cpus(), because setting it
> to a value lower than INT_MAX but higher than NR_CPUS produces the
> following error on reboot and shutdown:
>
> BUG: unable to handle page fault for address: ffffffff90ab1bb0
> #PF: supervisor read access in kernel mode
> #PF: error_code(0x0000) - not-present page
> PGD 1c09067 P4D 1c09067 PUD 1c0a063 PMD 0
> Oops: 0000 [#1] SMP
> CPU: 1 PID: 1 Comm: systemd-shutdow Not tainted 5.9.0-rc8-kvm #110
> Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.13.0-2.fc32 04/01/2014
> RIP: 0010:migrate_to_reboot_cpu+0xe/0x60
> Code: ea ea 00 48 89 fa 48 c7 c7 30 57 f1 81 e9 fa ef ff ff 66 2e 0f 1f 84 00 00 00 00 00 53 8b 1d d5 ea ea 00 e8 14 33 fe ff 89 da <48> 0f a3 15 ea fc bd 00 48 89 d0 73 29 89 c2 c1 e8 06 65 48 8b 3c
> RSP: 0018:ffffc90000013e08 EFLAGS: 00010246
> RAX: ffff88801f0a0000 RBX: 0000000077359400 RCX: 0000000000000000
> RDX: 0000000077359400 RSI: 0000000000000002 RDI: ffffffff81c199e0
> RBP: ffffffff81c1e3c0 R08: ffff88801f41f000 R09: ffffffff81c1e348
> R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000
> R13: 00007f32bedf8830 R14: 00000000fee1dead R15: 0000000000000000
> FS: 00007f32bedf8980(0000) GS:ffff88801f480000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: ffffffff90ab1bb0 CR3: 000000001d057000 CR4: 00000000000006a0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> Call Trace:
> __do_sys_reboot.cold+0x34/0x5b
> ? vfs_writev+0x92/0xc0
> ? do_writev+0x52/0xd0
> do_syscall_64+0x2d/0x40
> entry_SYSCALL_64_after_hwframe+0x44/0xa9
> RIP: 0033:0x7f32bfaaecd3
> Code: 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 89 fa be 69 19 12 28 bf ad de e1 fe b8 a9 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 05 c3 0f 1f 40 00 48 8b 15 89 81 0c 00 f7 d8
> RSP: 002b:00007fff6265fb58 EFLAGS: 00000202 ORIG_RAX: 00000000000000a9
> RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007f32bfaaecd3
> RDX: 0000000001234567 RSI: 0000000028121969 RDI: 00000000fee1dead
> RBP: 0000000000000000 R08: 0000000000008020 R09: 00007fff6265ef60
> R10: 00007f32bedf8830 R11: 0000000000000202 R12: 0000000000000000
> R13: 0000557bba2c51c0 R14: 0000000000000000 R15: 00007fff6265fbc8
> CR2: ffffffff90ab1bb0
> ---[ end trace b813e80157136563 ]---
> RIP: 0010:migrate_to_reboot_cpu+0xe/0x60
> Code: ea ea 00 48 89 fa 48 c7 c7 30 57 f1 81 e9 fa ef ff ff 66 2e 0f 1f 84 00 00 00 00 00 53 8b 1d d5 ea ea 00 e8 14 33 fe ff 89 da <48> 0f a3 15 ea fc bd 00 48 89 d0 73 29 89 c2 c1 e8 06 65 48 8b 3c
> RSP: 0018:ffffc90000013e08 EFLAGS: 00010246
> RAX: ffff88801f0a0000 RBX: 0000000077359400 RCX: 0000000000000000
> RDX: 0000000077359400 RSI: 0000000000000002 RDI: ffffffff81c199e0
> RBP: ffffffff81c1e3c0 R08: ffff88801f41f000 R09: ffffffff81c1e348
> R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000
> R13: 00007f32bedf8830 R14: 00000000fee1dead R15: 0000000000000000
> FS: 00007f32bedf8980(0000) GS:ffff88801f480000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: ffffffff90ab1bb0 CR3: 000000001d057000 CR4: 00000000000006a0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> Kernel panic - not syncing: Attempted to kill init! exitcode=0x00000009
> Kernel Offset: disabled
> ---[ end Kernel panic - not syncing: Attempted to kill init! exitcode=0x00000009 ]---
>
> Fixes: 1b3a5d02ee07 ("reboot: move arch/x86 reboot= handling to generic kernel")
> Signed-off-by: Matteo Croce <mcroce@microsoft.com>
> ---
> kernel/reboot.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
<formletter>
This is not the correct way to submit patches for inclusion in the
stable kernel tree. Please read:
https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
for how to do this properly.
</formletter>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 1/2] reboot: fix overflow parsing reboot cpu number
2020-10-27 13:42 ` Greg KH
@ 2020-10-30 14:13 ` Petr Mladek
2020-10-30 14:18 ` Matteo Croce
0 siblings, 1 reply; 15+ messages in thread
From: Petr Mladek @ 2020-10-30 14:13 UTC (permalink / raw)
To: Greg KH
Cc: Matteo Croce, linux-kernel, Guenter Roeck, Arnd Bergmann,
Mike Rapoport, Kees Cook, Pavel Tatashin, Robin Holt,
Fabian Frederick, stable
On Tue 2020-10-27 14:42:43, Greg KH wrote:
> On Tue, Oct 27, 2020 at 02:35:44PM +0100, Matteo Croce wrote:
> > From: Matteo Croce <mcroce@microsoft.com>
> >
> > Limit the CPU number to num_possible_cpus(), because setting it
> > to a value lower than INT_MAX but higher than NR_CPUS produces the
> > following error on reboot and shutdown:
> >
> > Fixes: 1b3a5d02ee07 ("reboot: move arch/x86 reboot= handling to generic kernel")
> > Signed-off-by: Matteo Croce <mcroce@microsoft.com>
Reviewed-by: Petr Mladek <pmladek@suse.com>
> <formletter>
>
> This is not the correct way to submit patches for inclusion in the
> stable kernel tree. Please read:
> https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
> for how to do this properly.
>
> </formletter>
The best way is to add the following line before Signed-off-by line:
Cc: stable@vger.kernel.org
Best Regards,
Petr
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 1/2] reboot: fix overflow parsing reboot cpu number
2020-10-30 14:13 ` Petr Mladek
@ 2020-10-30 14:18 ` Matteo Croce
0 siblings, 0 replies; 15+ messages in thread
From: Matteo Croce @ 2020-10-30 14:18 UTC (permalink / raw)
To: Petr Mladek
Cc: Greg KH, linux-kernel, Guenter Roeck, Arnd Bergmann,
Mike Rapoport, Kees Cook, Pavel Tatashin, Robin Holt,
Fabian Frederick
On Fri, Oct 30, 2020 at 3:13 PM Petr Mladek <pmladek@suse.com> wrote:
>
> On Tue 2020-10-27 14:42:43, Greg KH wrote:
> > On Tue, Oct 27, 2020 at 02:35:44PM +0100, Matteo Croce wrote:
> > > From: Matteo Croce <mcroce@microsoft.com>
> > >
> > > Limit the CPU number to num_possible_cpus(), because setting it
> > > to a value lower than INT_MAX but higher than NR_CPUS produces the
> > > following error on reboot and shutdown:
> > >
> > > Fixes: 1b3a5d02ee07 ("reboot: move arch/x86 reboot= handling to generic kernel")
> > > Signed-off-by: Matteo Croce <mcroce@microsoft.com>
>
> Reviewed-by: Petr Mladek <pmladek@suse.com>
>
> > <formletter>
> >
> > This is not the correct way to submit patches for inclusion in the
> > stable kernel tree. Please read:
> > https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
> > for how to do this properly.
> >
> > </formletter>
>
> The best way is to add the following line before Signed-off-by line:
>
> Cc: stable@vger.kernel.org
>
I see, sorry for the noise. Should I resend, or I eventually write to
stable@vger.kernel.org after the merge?
--
per aspera ad upstream
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 2/2] reboot: fix parsing of reboot cpu number
2020-10-27 13:35 ` [PATCH v2 2/2] reboot: fix parsing of " Matteo Croce
2020-10-27 13:42 ` Greg KH
@ 2020-10-30 14:30 ` Petr Mladek
2020-11-01 1:57 ` Matteo Croce
1 sibling, 1 reply; 15+ messages in thread
From: Petr Mladek @ 2020-10-30 14:30 UTC (permalink / raw)
To: Matteo Croce
Cc: linux-kernel, Guenter Roeck, Arnd Bergmann, Mike Rapoport,
Kees Cook, Pavel Tatashin, Robin Holt, Fabian Frederick, stable
On Tue 2020-10-27 14:35:45, Matteo Croce wrote:
> From: Matteo Croce <mcroce@microsoft.com>
>
> The kernel cmdline reboot= argument allows to specify the CPU used
> for rebooting, with the syntax `s####` among the other flags, e.g.
>
> reboot=soft,s4
> reboot=warm,s31,force
>
> In the early days the parsing was done with simple_strtoul(), later
> deprecated in favor of the safer kstrtoint() which handles overflow.
>
> But kstrtoint() returns -EINVAL if there are non-digit characters
> in a string, so if this flag is not the last given, it's silently
> ignored as well as the subsequent ones.
>
> To fix it, revert the usage of simple_strtoul(), which is no longer
> deprecated, and restore the old behaviour.
>
> While at it, merge two identical code blocks into one.
> --- a/kernel/reboot.c
> +++ b/kernel/reboot.c
> @@ -552,25 +552,19 @@ static int __init reboot_setup(char *str)
>
> case 's':
> {
> - int rc;
> -
> - if (isdigit(*(str+1))) {
> - rc = kstrtoint(str+1, 0, &reboot_cpu);
> - if (rc)
> - return rc;
> - if (reboot_cpu >= num_possible_cpus()) {
> - reboot_cpu = 0;
> - return -ERANGE;
> - }
> - } else if (str[1] == 'm' && str[2] == 'p' &&
> - isdigit(*(str+3))) {
> - rc = kstrtoint(str+3, 0, &reboot_cpu);
> - if (rc)
> - return rc;
> - if (reboot_cpu >= num_possible_cpus()) {
> - reboot_cpu = 0;
^^^^^^
> + int cpu;
> +
> + /*
> + * reboot_cpu is s[mp]#### with #### being the processor
> + * to be used for rebooting. Skip 's' or 'smp' prefix.
> + */
> + str += str[1] == 'm' && str[2] == 'p' ? 3 : 1;
> +
> + if (isdigit(str[0])) {
> + cpu = simple_strtoul(str, NULL, 0);
> + if (cpu >= num_possible_cpus())
> return -ERANGE;
> - }
> + reboot_cpu = cpu;
The original value stays when the new one is out of range. It is
small functional change that should get mentioned in the commit
message or better fixed separately.
Hmm, I suggest to split this into 3 patches and switch the order:
+ 1st patch should simply revert the commit 616feab75397
("kernel/reboot.c: convert simple_strtoul to kstrtoint").
+ 2nd patch should merge the two branches without any
functional change.
+ 3rd patch should add the check for num_possible_cpus()
and update the value only when it is valid.
I am sorry that I did not suggested this when reviewed v1.
I have missed this functional change at that time.
Best Regards,
Petr
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 2/2] reboot: fix parsing of reboot cpu number
2020-10-30 14:30 ` Petr Mladek
@ 2020-11-01 1:57 ` Matteo Croce
2020-11-02 11:01 ` Petr Mladek
0 siblings, 1 reply; 15+ messages in thread
From: Matteo Croce @ 2020-11-01 1:57 UTC (permalink / raw)
To: Petr Mladek
Cc: linux-kernel, Guenter Roeck, Arnd Bergmann, Mike Rapoport,
Kees Cook, Pavel Tatashin, Robin Holt
On Fri, Oct 30, 2020 at 3:30 PM Petr Mladek <pmladek@suse.com> wrote:
>
> On Tue 2020-10-27 14:35:45, Matteo Croce wrote:
> > From: Matteo Croce <mcroce@microsoft.com>
> >
> > The kernel cmdline reboot= argument allows to specify the CPU used
> > for rebooting, with the syntax `s####` among the other flags, e.g.
> >
> > reboot=soft,s4
> > reboot=warm,s31,force
> >
> > In the early days the parsing was done with simple_strtoul(), later
> > deprecated in favor of the safer kstrtoint() which handles overflow.
> >
> > But kstrtoint() returns -EINVAL if there are non-digit characters
> > in a string, so if this flag is not the last given, it's silently
> > ignored as well as the subsequent ones.
> >
> > To fix it, revert the usage of simple_strtoul(), which is no longer
> > deprecated, and restore the old behaviour.
> >
> > While at it, merge two identical code blocks into one.
>
> > --- a/kernel/reboot.c
> > +++ b/kernel/reboot.c
> > @@ -552,25 +552,19 @@ static int __init reboot_setup(char *str)
> >
> > case 's':
> > {
> > - int rc;
> > -
> > - if (isdigit(*(str+1))) {
> > - rc = kstrtoint(str+1, 0, &reboot_cpu);
> > - if (rc)
> > - return rc;
> > - if (reboot_cpu >= num_possible_cpus()) {
> > - reboot_cpu = 0;
> > - return -ERANGE;
> > - }
> > - } else if (str[1] == 'm' && str[2] == 'p' &&
> > - isdigit(*(str+3))) {
> > - rc = kstrtoint(str+3, 0, &reboot_cpu);
> > - if (rc)
> > - return rc;
> > - if (reboot_cpu >= num_possible_cpus()) {
> > - reboot_cpu = 0;
>
> ^^^^^^
>
> > + int cpu;
> > +
> > + /*
> > + * reboot_cpu is s[mp]#### with #### being the processor
> > + * to be used for rebooting. Skip 's' or 'smp' prefix.
> > + */
> > + str += str[1] == 'm' && str[2] == 'p' ? 3 : 1;
> > +
> > + if (isdigit(str[0])) {
> > + cpu = simple_strtoul(str, NULL, 0);
> > + if (cpu >= num_possible_cpus())
> > return -ERANGE;
> > - }
> > + reboot_cpu = cpu;
>
> The original value stays when the new one is out of range. It is
> small functional change that should get mentioned in the commit
> message or better fixed separately.
>
Hi,
I didn't understand, the original value is 0 since reboot_cpu is global.
reboot_setup() is only called once at boot to parse the cmdline,
indeed it's __init.
I don't know any way to call it more than once (exept passing multiple
reboot= arguments)
> Hmm, I suggest to split this into 3 patches and switch the order:
>
> + 1st patch should simply revert the commit 616feab75397
> ("kernel/reboot.c: convert simple_strtoul to kstrtoint").
>
> + 2nd patch should merge the two branches without any
> functional change.
>
> + 3rd patch should add the check for num_possible_cpus()
> and update the value only when it is valid.
>
> I am sorry that I did not suggested this when reviewed v1.
> I have missed this functional change at that time.
>
Np :)
Bye,
--
per aspera ad upstream
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 2/2] reboot: fix parsing of reboot cpu number
2020-11-01 1:57 ` Matteo Croce
@ 2020-11-02 11:01 ` Petr Mladek
2020-11-03 11:43 ` Matteo Croce
0 siblings, 1 reply; 15+ messages in thread
From: Petr Mladek @ 2020-11-02 11:01 UTC (permalink / raw)
To: Matteo Croce
Cc: linux-kernel, Guenter Roeck, Arnd Bergmann, Mike Rapoport,
Kees Cook, Pavel Tatashin, Robin Holt
On Sun 2020-11-01 02:57:40, Matteo Croce wrote:
> On Fri, Oct 30, 2020 at 3:30 PM Petr Mladek <pmladek@suse.com> wrote:
> >
> > On Tue 2020-10-27 14:35:45, Matteo Croce wrote:
> > > From: Matteo Croce <mcroce@microsoft.com>
> > >
> > > The kernel cmdline reboot= argument allows to specify the CPU used
> > > for rebooting, with the syntax `s####` among the other flags, e.g.
> > >
> > > reboot=soft,s4
> > > reboot=warm,s31,force
> > >
> > > In the early days the parsing was done with simple_strtoul(), later
> > > deprecated in favor of the safer kstrtoint() which handles overflow.
> > >
> > > But kstrtoint() returns -EINVAL if there are non-digit characters
> > > in a string, so if this flag is not the last given, it's silently
> > > ignored as well as the subsequent ones.
> > >
> > > To fix it, revert the usage of simple_strtoul(), which is no longer
> > > deprecated, and restore the old behaviour.
> > >
> > > While at it, merge two identical code blocks into one.
> >
> > > --- a/kernel/reboot.c
> > > +++ b/kernel/reboot.c
> > > @@ -552,25 +552,19 @@ static int __init reboot_setup(char *str)
> > >
> > > case 's':
> > > {
> > > - int rc;
> > > -
> > > - if (isdigit(*(str+1))) {
> > > - rc = kstrtoint(str+1, 0, &reboot_cpu);
> > > - if (rc)
> > > - return rc;
> > > - if (reboot_cpu >= num_possible_cpus()) {
> > > - reboot_cpu = 0;
> > > - return -ERANGE;
> > > - }
> > > - } else if (str[1] == 'm' && str[2] == 'p' &&
> > > - isdigit(*(str+3))) {
> > > - rc = kstrtoint(str+3, 0, &reboot_cpu);
> > > - if (rc)
> > > - return rc;
> > > - if (reboot_cpu >= num_possible_cpus()) {
> > > - reboot_cpu = 0;
> >
> > ^^^^^^
> >
> > > + int cpu;
> > > +
> > > + /*
> > > + * reboot_cpu is s[mp]#### with #### being the processor
> > > + * to be used for rebooting. Skip 's' or 'smp' prefix.
> > > + */
> > > + str += str[1] == 'm' && str[2] == 'p' ? 3 : 1;
> > > +
> > > + if (isdigit(str[0])) {
> > > + cpu = simple_strtoul(str, NULL, 0);
> > > + if (cpu >= num_possible_cpus())
> > > return -ERANGE;
> > > - }
> > > + reboot_cpu = cpu;
> >
> > The original value stays when the new one is out of range. It is
> > small functional change that should get mentioned in the commit
> > message or better fixed separately.
Ah, I see. From some reason, I assumed that it was defined as
module_param() or core_param(). Then it would be possible to modify
it later via /sys.
I am sorry for the noise.
Best Regards,
Petr
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 2/2] reboot: fix parsing of reboot cpu number
2020-11-02 11:01 ` Petr Mladek
@ 2020-11-03 11:43 ` Matteo Croce
2020-11-03 14:25 ` Petr Mladek
0 siblings, 1 reply; 15+ messages in thread
From: Matteo Croce @ 2020-11-03 11:43 UTC (permalink / raw)
To: Petr Mladek
Cc: linux-kernel, Guenter Roeck, Arnd Bergmann, Mike Rapoport,
Kees Cook, Pavel Tatashin, Robin Holt
On Mon, Nov 2, 2020 at 12:01 PM Petr Mladek <pmladek@suse.com> wrote:
>
> On Sun 2020-11-01 02:57:40, Matteo Croce wrote:
> > On Fri, Oct 30, 2020 at 3:30 PM Petr Mladek <pmladek@suse.com> wrote:
> > >
> > > On Tue 2020-10-27 14:35:45, Matteo Croce wrote:
> > > > From: Matteo Croce <mcroce@microsoft.com>
> > > >
> > > > The kernel cmdline reboot= argument allows to specify the CPU used
> > > > for rebooting, with the syntax `s####` among the other flags, e.g.
> > > >
> > > > reboot=soft,s4
> > > > reboot=warm,s31,force
> > > >
> > > > In the early days the parsing was done with simple_strtoul(), later
> > > > deprecated in favor of the safer kstrtoint() which handles overflow.
> > > >
> > > > But kstrtoint() returns -EINVAL if there are non-digit characters
> > > > in a string, so if this flag is not the last given, it's silently
> > > > ignored as well as the subsequent ones.
> > > >
> > > > To fix it, revert the usage of simple_strtoul(), which is no longer
> > > > deprecated, and restore the old behaviour.
> > > >
> > > > While at it, merge two identical code blocks into one.
> > >
> > > > --- a/kernel/reboot.c
> > > > +++ b/kernel/reboot.c
> > > > @@ -552,25 +552,19 @@ static int __init reboot_setup(char *str)
> > > >
> > > > case 's':
> > > > {
> > > > - int rc;
> > > > -
> > > > - if (isdigit(*(str+1))) {
> > > > - rc = kstrtoint(str+1, 0, &reboot_cpu);
> > > > - if (rc)
> > > > - return rc;
> > > > - if (reboot_cpu >= num_possible_cpus()) {
> > > > - reboot_cpu = 0;
> > > > - return -ERANGE;
> > > > - }
> > > > - } else if (str[1] == 'm' && str[2] == 'p' &&
> > > > - isdigit(*(str+3))) {
> > > > - rc = kstrtoint(str+3, 0, &reboot_cpu);
> > > > - if (rc)
> > > > - return rc;
> > > > - if (reboot_cpu >= num_possible_cpus()) {
> > > > - reboot_cpu = 0;
> > >
> > > ^^^^^^
> > >
> > > > + int cpu;
> > > > +
> > > > + /*
> > > > + * reboot_cpu is s[mp]#### with #### being the processor
> > > > + * to be used for rebooting. Skip 's' or 'smp' prefix.
> > > > + */
> > > > + str += str[1] == 'm' && str[2] == 'p' ? 3 : 1;
> > > > +
> > > > + if (isdigit(str[0])) {
> > > > + cpu = simple_strtoul(str, NULL, 0);
> > > > + if (cpu >= num_possible_cpus())
> > > > return -ERANGE;
> > > > - }
> > > > + reboot_cpu = cpu;
> > >
> > > The original value stays when the new one is out of range. It is
> > > small functional change that should get mentioned in the commit
> > > message or better fixed separately.
>
> Ah, I see. From some reason, I assumed that it was defined as
> module_param() or core_param(). Then it would be possible to modify
> it later via /sys.
>
> I am sorry for the noise.
>
Never mind :)
So, is this an ack? Or I need to prepare a v3 with the revert as first patch?
Regards,
--
per aspera ad upstream
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 2/2] reboot: fix parsing of reboot cpu number
2020-11-03 11:43 ` Matteo Croce
@ 2020-11-03 14:25 ` Petr Mladek
2020-11-03 15:43 ` Matteo Croce
0 siblings, 1 reply; 15+ messages in thread
From: Petr Mladek @ 2020-11-03 14:25 UTC (permalink / raw)
To: Matteo Croce
Cc: linux-kernel, Guenter Roeck, Arnd Bergmann, Mike Rapoport,
Kees Cook, Pavel Tatashin, Robin Holt
On Tue 2020-11-03 12:43:32, Matteo Croce wrote:
> On Mon, Nov 2, 2020 at 12:01 PM Petr Mladek <pmladek@suse.com> wrote:
> >
> > On Sun 2020-11-01 02:57:40, Matteo Croce wrote:
> > > On Fri, Oct 30, 2020 at 3:30 PM Petr Mladek <pmladek@suse.com> wrote:
> > > >
> > > > On Tue 2020-10-27 14:35:45, Matteo Croce wrote:
> > > > > From: Matteo Croce <mcroce@microsoft.com>
> > > > >
> > > > > The kernel cmdline reboot= argument allows to specify the CPU used
> > > > > for rebooting, with the syntax `s####` among the other flags, e.g.
> > > > >
> > > > > reboot=soft,s4
> > > > > reboot=warm,s31,force
> > > > >
> > > > > In the early days the parsing was done with simple_strtoul(), later
> > > > > deprecated in favor of the safer kstrtoint() which handles overflow.
> > > > >
> > > > > But kstrtoint() returns -EINVAL if there are non-digit characters
> > > > > in a string, so if this flag is not the last given, it's silently
> > > > > ignored as well as the subsequent ones.
> > > > >
> > > > > To fix it, revert the usage of simple_strtoul(), which is no longer
> > > > > deprecated, and restore the old behaviour.
> > > > >
> > > > > While at it, merge two identical code blocks into one.
> > > >
> > > > > --- a/kernel/reboot.c
> > > > > +++ b/kernel/reboot.c
> > > > > @@ -552,25 +552,19 @@ static int __init reboot_setup(char *str)
> > > > >
> > > > > case 's':
> > > > > {
> > > > > - int rc;
> > > > > -
> > > > > - if (isdigit(*(str+1))) {
> > > > > - rc = kstrtoint(str+1, 0, &reboot_cpu);
> > > > > - if (rc)
> > > > > - return rc;
> > > > > - if (reboot_cpu >= num_possible_cpus()) {
> > > > > - reboot_cpu = 0;
> > > > > - return -ERANGE;
> > > > > - }
> > > > > - } else if (str[1] == 'm' && str[2] == 'p' &&
> > > > > - isdigit(*(str+3))) {
> > > > > - rc = kstrtoint(str+3, 0, &reboot_cpu);
> > > > > - if (rc)
> > > > > - return rc;
> > > > > - if (reboot_cpu >= num_possible_cpus()) {
> > > > > - reboot_cpu = 0;
> > > >
> > > > ^^^^^^
> > > >
> > > > > + int cpu;
> > > > > +
> > > > > + /*
> > > > > + * reboot_cpu is s[mp]#### with #### being the processor
> > > > > + * to be used for rebooting. Skip 's' or 'smp' prefix.
> > > > > + */
> > > > > + str += str[1] == 'm' && str[2] == 'p' ? 3 : 1;
> > > > > +
> > > > > + if (isdigit(str[0])) {
> > > > > + cpu = simple_strtoul(str, NULL, 0);
> > > > > + if (cpu >= num_possible_cpus())
> > > > > return -ERANGE;
> > > > > - }
> > > > > + reboot_cpu = cpu;
> > > >
> > > > The original value stays when the new one is out of range. It is
> > > > small functional change that should get mentioned in the commit
> > > > message or better fixed separately.
> >
> > Ah, I see. From some reason, I assumed that it was defined as
> > module_param() or core_param(). Then it would be possible to modify
> > it later via /sys.
> >
> > I am sorry for the noise.
> >
>
> Never mind :)
>
> So, is this an ack? Or I need to prepare a v3 with the revert as first patch?
Good question ;-) It would be nice to do it the cleaner way but I do
not resist on it. Feel free to use:
Reviewed-by: Petr Mladek <pmladek@suse.com>
Now, the question is who would actually push this upstream. These
patches often go via Andrew Morton. He actually committed both
changes that are fixed here.
I suggest to resend the patchset with my Reviewed-by and
Cc: stable@vger.kernel.org lines. And put Andrew and Greg into Cc.
Best Regards,
Petr
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 2/2] reboot: fix parsing of reboot cpu number
2020-11-03 14:25 ` Petr Mladek
@ 2020-11-03 15:43 ` Matteo Croce
2020-11-03 16:22 ` Petr Mladek
0 siblings, 1 reply; 15+ messages in thread
From: Matteo Croce @ 2020-11-03 15:43 UTC (permalink / raw)
To: Petr Mladek
Cc: linux-kernel, Guenter Roeck, Arnd Bergmann, Mike Rapoport,
Kees Cook, Pavel Tatashin, Robin Holt
On Tue, Nov 3, 2020 at 3:25 PM Petr Mladek <pmladek@suse.com> wrote:
>
> On Tue 2020-11-03 12:43:32, Matteo Croce wrote:
> > On Mon, Nov 2, 2020 at 12:01 PM Petr Mladek <pmladek@suse.com> wrote:
> > >
> > > On Sun 2020-11-01 02:57:40, Matteo Croce wrote:
> > > > On Fri, Oct 30, 2020 at 3:30 PM Petr Mladek <pmladek@suse.com> wrote:
> > > > >
> > > > > On Tue 2020-10-27 14:35:45, Matteo Croce wrote:
> > > > > > From: Matteo Croce <mcroce@microsoft.com>
> > > > > >
> > > > > > The kernel cmdline reboot= argument allows to specify the CPU used
> > > > > > for rebooting, with the syntax `s####` among the other flags, e.g.
> > > > > >
> > > > > > reboot=soft,s4
> > > > > > reboot=warm,s31,force
> > > > > >
> > > > > > In the early days the parsing was done with simple_strtoul(), later
> > > > > > deprecated in favor of the safer kstrtoint() which handles overflow.
> > > > > >
> > > > > > But kstrtoint() returns -EINVAL if there are non-digit characters
> > > > > > in a string, so if this flag is not the last given, it's silently
> > > > > > ignored as well as the subsequent ones.
> > > > > >
> > > > > > To fix it, revert the usage of simple_strtoul(), which is no longer
> > > > > > deprecated, and restore the old behaviour.
> > > > > >
> > > > > > While at it, merge two identical code blocks into one.
> > > > >
> > > > > > --- a/kernel/reboot.c
> > > > > > +++ b/kernel/reboot.c
> > > > > > @@ -552,25 +552,19 @@ static int __init reboot_setup(char *str)
> > > > > >
> > > > > > case 's':
> > > > > > {
> > > > > > - int rc;
> > > > > > -
> > > > > > - if (isdigit(*(str+1))) {
> > > > > > - rc = kstrtoint(str+1, 0, &reboot_cpu);
> > > > > > - if (rc)
> > > > > > - return rc;
> > > > > > - if (reboot_cpu >= num_possible_cpus()) {
> > > > > > - reboot_cpu = 0;
> > > > > > - return -ERANGE;
> > > > > > - }
> > > > > > - } else if (str[1] == 'm' && str[2] == 'p' &&
> > > > > > - isdigit(*(str+3))) {
> > > > > > - rc = kstrtoint(str+3, 0, &reboot_cpu);
> > > > > > - if (rc)
> > > > > > - return rc;
> > > > > > - if (reboot_cpu >= num_possible_cpus()) {
> > > > > > - reboot_cpu = 0;
> > > > >
> > > > > ^^^^^^
> > > > >
> > > > > > + int cpu;
> > > > > > +
> > > > > > + /*
> > > > > > + * reboot_cpu is s[mp]#### with #### being the processor
> > > > > > + * to be used for rebooting. Skip 's' or 'smp' prefix.
> > > > > > + */
> > > > > > + str += str[1] == 'm' && str[2] == 'p' ? 3 : 1;
> > > > > > +
> > > > > > + if (isdigit(str[0])) {
> > > > > > + cpu = simple_strtoul(str, NULL, 0);
> > > > > > + if (cpu >= num_possible_cpus())
> > > > > > return -ERANGE;
> > > > > > - }
> > > > > > + reboot_cpu = cpu;
> > > > >
> > > > > The original value stays when the new one is out of range. It is
> > > > > small functional change that should get mentioned in the commit
> > > > > message or better fixed separately.
> > >
> > > Ah, I see. From some reason, I assumed that it was defined as
> > > module_param() or core_param(). Then it would be possible to modify
> > > it later via /sys.
> > >
> > > I am sorry for the noise.
> > >
> >
> > Never mind :)
> >
> > So, is this an ack? Or I need to prepare a v3 with the revert as first patch?
>
> Good question ;-) It would be nice to do it the cleaner way but I do
> not resist on it. Feel free to use:
>
> Reviewed-by: Petr Mladek <pmladek@suse.com>
>
> Now, the question is who would actually push this upstream. These
> patches often go via Andrew Morton. He actually committed both
> changes that are fixed here.
>
> I suggest to resend the patchset with my Reviewed-by and
> Cc: stable@vger.kernel.org lines. And put Andrew and Greg into Cc.
>
I see that by doing the revert first, makes the patch very small.
It's worth it.
I'm thinking, what should be the right action to do when the supplied
cpu number is too big?
With my patch I stop the parsing, while the previous behavior (other
than setting a wrong cpu number) was to continue parsing other fields.
Maybe I should just continue the loop and continue the parsing?
Maybe with a pr_warn "cpu X exceeds possible cpu number Y" etc.
--
per aspera ad upstream
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 2/2] reboot: fix parsing of reboot cpu number
2020-11-03 15:43 ` Matteo Croce
@ 2020-11-03 16:22 ` Petr Mladek
0 siblings, 0 replies; 15+ messages in thread
From: Petr Mladek @ 2020-11-03 16:22 UTC (permalink / raw)
To: Matteo Croce
Cc: linux-kernel, Guenter Roeck, Arnd Bergmann, Mike Rapoport,
Kees Cook, Pavel Tatashin, Robin Holt
On Tue 2020-11-03 16:43:59, Matteo Croce wrote:
> On Tue, Nov 3, 2020 at 3:25 PM Petr Mladek <pmladek@suse.com> wrote:
> >
> > On Tue 2020-11-03 12:43:32, Matteo Croce wrote:
> > > On Mon, Nov 2, 2020 at 12:01 PM Petr Mladek <pmladek@suse.com> wrote:
> > > >
> > > > On Sun 2020-11-01 02:57:40, Matteo Croce wrote:
> > > > > On Fri, Oct 30, 2020 at 3:30 PM Petr Mladek <pmladek@suse.com> wrote:
> > > > > >
> > > > > > On Tue 2020-10-27 14:35:45, Matteo Croce wrote:
> > > > > > > From: Matteo Croce <mcroce@microsoft.com>
> > > > > > >
> > > > > > > The kernel cmdline reboot= argument allows to specify the CPU used
> > > > > > > for rebooting, with the syntax `s####` among the other flags, e.g.
> > > > > > >
> > > > > > > reboot=soft,s4
> > > > > > > reboot=warm,s31,force
> > > > > > >
> > > > > > > In the early days the parsing was done with simple_strtoul(), later
> > > > > > > deprecated in favor of the safer kstrtoint() which handles overflow.
> > > > > > >
> > > > > > > But kstrtoint() returns -EINVAL if there are non-digit characters
> > > > > > > in a string, so if this flag is not the last given, it's silently
> > > > > > > ignored as well as the subsequent ones.
> > > > > > >
> > > > > > > To fix it, revert the usage of simple_strtoul(), which is no longer
> > > > > > > deprecated, and restore the old behaviour.
> > > > > > >
> > > > > > > While at it, merge two identical code blocks into one.
> > > > > >
> > > > > > > --- a/kernel/reboot.c
> > > > > > > +++ b/kernel/reboot.c
> > > > > > > @@ -552,25 +552,19 @@ static int __init reboot_setup(char *str)
> > > > > > >
> > > > > > > case 's':
> > > > > > > {
> > > > > > > - int rc;
> > > > > > > -
> > > > > > > - if (isdigit(*(str+1))) {
> > > > > > > - rc = kstrtoint(str+1, 0, &reboot_cpu);
> > > > > > > - if (rc)
> > > > > > > - return rc;
> > > > > > > - if (reboot_cpu >= num_possible_cpus()) {
> > > > > > > - reboot_cpu = 0;
> > > > > > > - return -ERANGE;
> > > > > > > - }
> > > > > > > - } else if (str[1] == 'm' && str[2] == 'p' &&
> > > > > > > - isdigit(*(str+3))) {
> > > > > > > - rc = kstrtoint(str+3, 0, &reboot_cpu);
> > > > > > > - if (rc)
> > > > > > > - return rc;
> > > > > > > - if (reboot_cpu >= num_possible_cpus()) {
> > > > > > > - reboot_cpu = 0;
> > > > > >
> > > > > > ^^^^^^
> > > > > >
> > > > > > > + int cpu;
> > > > > > > +
> > > > > > > + /*
> > > > > > > + * reboot_cpu is s[mp]#### with #### being the processor
> > > > > > > + * to be used for rebooting. Skip 's' or 'smp' prefix.
> > > > > > > + */
> > > > > > > + str += str[1] == 'm' && str[2] == 'p' ? 3 : 1;
> > > > > > > +
> > > > > > > + if (isdigit(str[0])) {
> > > > > > > + cpu = simple_strtoul(str, NULL, 0);
> > > > > > > + if (cpu >= num_possible_cpus())
> > > > > > > return -ERANGE;
> > > > > > > - }
> > > > > > > + reboot_cpu = cpu;
> > > > > >
> > > > > > The original value stays when the new one is out of range. It is
> > > > > > small functional change that should get mentioned in the commit
> > > > > > message or better fixed separately.
> > > >
> > > > Ah, I see. From some reason, I assumed that it was defined as
> > > > module_param() or core_param(). Then it would be possible to modify
> > > > it later via /sys.
> > > >
> > > > I am sorry for the noise.
> > > >
> > >
> > > Never mind :)
> > >
> > > So, is this an ack? Or I need to prepare a v3 with the revert as first patch?
> >
> > Good question ;-) It would be nice to do it the cleaner way but I do
> > not resist on it. Feel free to use:
> >
> > Reviewed-by: Petr Mladek <pmladek@suse.com>
> >
> > Now, the question is who would actually push this upstream. These
> > patches often go via Andrew Morton. He actually committed both
> > changes that are fixed here.
> >
> > I suggest to resend the patchset with my Reviewed-by and
> > Cc: stable@vger.kernel.org lines. And put Andrew and Greg into Cc.
> >
>
> I see that by doing the revert first, makes the patch very small.
> It's worth it.
Great.
> I'm thinking, what should be the right action to do when the supplied
> cpu number is too big?
> With my patch I stop the parsing, while the previous behavior (other
> than setting a wrong cpu number) was to continue parsing other fields.
> Maybe I should just continue the loop and continue the parsing?
> Maybe with a pr_warn "cpu X exceeds possible cpu number Y" etc.
Sounds good to me. Please, make it clear that the error message is
printed when parsing reboot= commandline option. For example:
pr_err("Ignoring CPU number in reboot= option. CPU X exceeds possible
cpu number Y", ...)
or something like this.
Best Regards,
Petr
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2020-11-03 16:22 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-27 13:35 [PATCH v2 0/2] fix parsing of reboot= cmdline Matteo Croce
2020-10-27 13:35 ` [PATCH v2 1/2] reboot: fix overflow parsing reboot cpu number Matteo Croce
2020-10-27 13:42 ` Greg KH
2020-10-30 14:13 ` Petr Mladek
2020-10-30 14:18 ` Matteo Croce
2020-10-27 13:35 ` [PATCH v2 2/2] reboot: fix parsing of " Matteo Croce
2020-10-27 13:42 ` Greg KH
2020-10-30 14:30 ` Petr Mladek
2020-11-01 1:57 ` Matteo Croce
2020-11-02 11:01 ` Petr Mladek
2020-11-03 11:43 ` Matteo Croce
2020-11-03 14:25 ` Petr Mladek
2020-11-03 15:43 ` Matteo Croce
2020-11-03 16:22 ` Petr Mladek
2020-10-27 13:42 ` [PATCH v2 0/2] fix parsing of reboot= cmdline Greg KH
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).