RE: [PATCH v2] panic: prevent panic_timeout * 1000 from overflow
> > Since panic_timeout is an integer passed-in through sysctl,
> > the loop boundary panic_timeout * 1000 could overflow and
> > result in a zero-delay panic when panic_timeout is greater
> > than INT_MAX/1000.
> >
> > Fix this by moving 1000 to the left, also in case i/1000
> > might never be greater than panic_timeout, change i to
> > long long so that it strictly has more bits.
> >
> > ...
> >
> > --- a/kernel/panic.c
> > +++ b/kernel/panic.c
> > @@ -178,7 +178,8 @@ void panic(const char *fmt, ...)
> > {
> > static char buf[1024];
> > va_list args;
> > - long i, i_next = 0, len;
> > + long long i;
> > + long i_next = 0, len;
> > int state = 0;
> > int old_cpu, this_cpu;
> > bool _crash_kexec_post_notifiers = crash_kexec_post_notifiers;
> > @@ -315,7 +316,7 @@ void panic(const char *fmt, ...)
> > */
> > pr_emerg("Rebooting in %d seconds..\n", panic_timeout);
> >
> > - for (i = 0; i < panic_timeout * 1000; i += PANIC_TIMER_STEP)
{
> > + for (i = 0; i / 1000 < panic_timeout; i += PANIC_TIMER_STEP)
{
>
> Problem is, 32-bit machines generally cannot perform 64-bit divides.
> So a call is emitted to the library function __divsi64() (I forget the
exact
> name) which Linux doesn't implement (because it's so slow, and we don't
> want to be calling it by accident).
>
It's good to know, thanks for letting me know why 64-bit division
is slow, and 64-multiplication is fast, surely doing so many
64-bit division will drag a lot, and should be prevented.
> So a fix would be to call do_div() or something from
> include/linux/div64.h but it's all a great mess.
>
> However we can do native 64-bit multiplication on 32-bit! So how about
> something like
>
> --- a/kernel/panic.c~a
> +++ a/kernel/panic.c
> @@ -313,13 +313,16 @@ void panic(const char *fmt, ...)
>* Delay timeout seconds before rebooting the machine.
>* We can't use the "normal" timers since we just panicked.
>*/
> + u64 timeout = panic_timeout * 1000; /* avoid overflow */
> + u64 timer;
> pr_emerg("Rebooting in %d seconds..\n", panic_timeout);
> - for (i = 0; i < panic_timeout * 1000; i += PANIC_TIMER_STEP)
{
> + for (timer = 0; timer < timeout; timer += PANIC_TIMER_STEP)
{
If using u64 as the loop boundary, would it be a problem if
panic_timeout is negative? Since in the current code, if
panic_timeout is negative, the loop will not be executed;
as in the patched code, the loop boundary will be a huge
unsigned value. I guess s64 should do？
If it's not a problem, I'll submit another patch enforcing
the change, including the changes suggested by Matthew here:
> > + u64 timeout = panic_timeout * 1000; /* avoid overflow */
> 1000ULL to not truncate before the assignment.
> > + u64 timer;
> ... as you implied lateru64 timer, timer_next;
Thank you guys so much for your valuable feedback, I learned a lot!
Best,
Changming
Re: [PATCH v2] panic: prevent panic_timeout * 1000 from overflow
On Mon, Jul 13, 2020 at 06:57:39PM -0700, Andrew Morton wrote: > +++ a/kernel/panic.c > @@ -313,13 +313,16 @@ void panic(const char *fmt, ...) >* Delay timeout seconds before rebooting the machine. >* We can't use the "normal" timers since we just panicked. >*/ > + u64 timeout = panic_timeout * 1000; /* avoid overflow */ 1000ULL to not truncate before the assignment. > + u64 timer; ... as you implied later u64 timer, timer_next; > (untested) Well, there's the rub. Testing requires 49 days (2^32 / 1000 seconds).
Re: [PATCH v2] panic: prevent panic_timeout * 1000 from overflow
On Sat, 11 Jul 2020 01:17:28 -0400 Changming
wrote:
> From: Changming Liu
>
> Since panic_timeout is an integer passed-in through sysctl,
> the loop boundary panic_timeout * 1000 could overflow and
> result in a zero-delay panic when panic_timeout is greater
> than INT_MAX/1000.
>
> Fix this by moving 1000 to the left, also in case i/1000
> might never be greater than panic_timeout, change i to
> long long so that it strictly has more bits.
>
> ...
>
> --- a/kernel/panic.c
> +++ b/kernel/panic.c
> @@ -178,7 +178,8 @@ void panic(const char *fmt, ...)
> {
> static char buf[1024];
> va_list args;
> - long i, i_next = 0, len;
> + long long i;
> + long i_next = 0, len;
> int state = 0;
> int old_cpu, this_cpu;
> bool _crash_kexec_post_notifiers = crash_kexec_post_notifiers;
> @@ -315,7 +316,7 @@ void panic(const char *fmt, ...)
>*/
> pr_emerg("Rebooting in %d seconds..\n", panic_timeout);
>
> - for (i = 0; i < panic_timeout * 1000; i += PANIC_TIMER_STEP) {
> + for (i = 0; i / 1000 < panic_timeout; i += PANIC_TIMER_STEP) {
Problem is, 32-bit machines generally cannot perform 64-bit divides.
So a call is emitted to the library function __divsi64() (I forget the exact
name) which Linux doesn't implement (because it's so slow, and we don't
want to be calling it by accident).
So a fix would be to call do_div() or something from
include/linux/div64.h but it's all a great mess.
However we can do native 64-bit multiplication on 32-bit! So how about
something like
--- a/kernel/panic.c~a
+++ a/kernel/panic.c
@@ -313,13 +313,16 @@ void panic(const char *fmt, ...)
* Delay timeout seconds before rebooting the machine.
* We can't use the "normal" timers since we just panicked.
*/
+ u64 timeout = panic_timeout * 1000; /* avoid overflow */
+ u64 timer;
+
pr_emerg("Rebooting in %d seconds..\n", panic_timeout);
- for (i = 0; i < panic_timeout * 1000; i += PANIC_TIMER_STEP) {
+ for (timer = 0; timer < timeout; timer += PANIC_TIMER_STEP) {
touch_nmi_watchdog();
- if (i >= i_next) {
- i += panic_blink(state ^= 1);
- i_next = i + 3600 / PANIC_BLINK_SPD;
+ if (timer >= i_next) {
+ timer += panic_blink(state ^= 1);
+ i_next = timer + 3600 / PANIC_BLINK_SPD;
}
mdelay(PANIC_TIMER_STEP);
}
(untested)
There's still the 3600/PANIC_BLINK_SPD in there, but a) that will be
done at compile-time and b) the 64-bit promotion should be done after
the division.
And... oh crap, i_next needs to be 64-bit as well.
