arm64: fix for restoring console loglevel after handling traps

On (07/19/18 15:18), Hari Vyas wrote:
[..]

Not so sure about the patch, sorry.

arch/arm64/kernel/traps.c void die(const char *str, struct pt_regs *regs, int err) · line 216

Quoted 7 lines

arch/arm64/kernel/traps.c +2 -0

@@ -211,6 +211,8 @@ void die(const char *str, struct pt_regs *regs, int err)
 
      bust_spinlocks(0);
      add_taint(TAINT_DIE, LOCKDEP_NOW_UNRELIABLE);
+
+     restore_console_loglevel();
      oops_exit();

This is *probably* the only place where you can roll-back the loglevel, but you restore console loglevel too early - a trap can panic() the system, we need to preserve the console verbose mode which we set at the start of the trap.

arch/arm64/kernel/traps.c asmlinkage void bad_mode(struct pt_regs *regs, int reason, unsigned int esr) · line 629

Quoted 5 lines

arch/arm64/kernel/traps.c +1 -0

@@ -624,6 +626,7 @@ asmlinkage void bad_mode(struct pt_regs *regs, int reason, unsigned int esr)
      die("Oops - bad mode", regs, 0);
      local_daif_mask();
      panic("bad mode");
+     restore_console_loglevel();

panic()

arch/arm64/kernel/traps.c asmlinkage void handle_bad_stack(struct pt_regs *regs) · line 686

Quoted 5 lines

arch/arm64/kernel/traps.c +1 -0

@@ -680,6 +683,7 @@ asmlinkage void handle_bad_stack(struct pt_regs *regs)
       * to get a better stack trace.
       */
      nmi_panic(NULL, "kernel stack overflow");
+     restore_console_loglevel();

panic()

arch/arm64/kernel/traps.c void __noreturn arm64_serror_panic(struct pt_regs *regs, u32 esr) · line 701

Quoted 6 lines

arch/arm64/kernel/traps.c +1 -1

@@ -694,7 +698,7 @@ void __noreturn arm64_serror_panic(struct pt_regs *regs, u32 esr)
              __show_regs(regs);
 
      nmi_panic(regs, "Asynchronous SError Interrupt");
-
+     restore_console_loglevel();

panic()

What's the point of restore_console_loglevel() after panic()?

        -ss

Quoted 5 lines · Sergey Senozhatsky

On (07/19/18 15:18), Hari Vyas wrote:
[..]

Not so sure about the patch, sorry.

Mentioned concern in cover letter and commit message. Console loglevel remains maximum after process die which
needs to be restored.

Quoted 13 lines

arch/arm64/kernel/traps.c void die(const char *str, struct pt_regs *regs, int err) · line 216

Quoted 7 lines

arch/arm64/kernel/traps.c +2 -0

@@ -211,6 +211,8 @@ void die(const char *str, struct pt_regs *regs, int err)
 
      bust_spinlocks(0);
      add_taint(TAINT_DIE, LOCKDEP_NOW_UNRELIABLE);
+
+     restore_console_loglevel();
      oops_exit();

This is *probably* the only place where you can roll-back the loglevel, but you restore console loglevel too early - a trap can panic() the system, we need to preserve the console verbose mode which we set at the start of the trap.

console loglevel is indirectly sets to verbose in oops_enter() so better to restore before oops_exit().
More from ARM maintainers.

Quoted 25 lines

arch/arm64/kernel/traps.c asmlinkage void bad_mode(struct pt_regs *regs, int reason, unsigned int esr) · line 629

Quoted 5 lines

arch/arm64/kernel/traps.c +1 -0

@@ -624,6 +626,7 @@ asmlinkage void bad_mode(struct pt_regs *regs, int reason, unsigned int esr)
      die("Oops - bad mode", regs, 0);
      local_daif_mask();
      panic("bad mode");
+     restore_console_loglevel();

panic()

arch/arm64/kernel/traps.c asmlinkage void handle_bad_stack(struct pt_regs *regs) · line 686

Quoted 5 lines

arch/arm64/kernel/traps.c +1 -0

@@ -680,6 +683,7 @@ asmlinkage void handle_bad_stack(struct pt_regs *regs)
       * to get a better stack trace.
       */
      nmi_panic(NULL, "kernel stack overflow");
+     restore_console_loglevel();

panic()

arch/arm64/kernel/traps.c void __noreturn arm64_serror_panic(struct pt_regs *regs, u32 esr) · line 701

Quoted 6 lines

arch/arm64/kernel/traps.c +1 -1

@@ -694,7 +698,7 @@ void __noreturn arm64_serror_panic(struct pt_regs *regs, u32 esr)
              __show_regs(regs);
 
      nmi_panic(regs, "Asynchronous SError Interrupt");
-
+     restore_console_loglevel();

panic()

Quoted 3 lines

What's the point of restore_console_loglevel() after panic()?

        -ss

Except die(), other places restore call can be avoided but just added for code completeness.

Quoted 23 lines · Hari Vyas

Quoted 5 lines · Sergey Senozhatsky

On (07/19/18 15:18), Hari Vyas wrote:
[..]

Not so sure about the patch, sorry.

Mentioned concern in cover letter and commit message. Console loglevel remains maximum after process die which
needs to be restored.

Quoted 13 lines

arch/arm64/kernel/traps.c void die(const char *str, struct pt_regs *regs, int err) · line 216

Quoted 7 lines

arch/arm64/kernel/traps.c +2 -0

@@ -211,6 +211,8 @@ void die(const char *str, struct pt_regs *regs, int err)
 
      bust_spinlocks(0);
      add_taint(TAINT_DIE, LOCKDEP_NOW_UNRELIABLE);
+
+     restore_console_loglevel();
      oops_exit();

This is *probably* the only place where you can roll-back the loglevel, but you restore console loglevel too early - a trap can panic() the system, we need to preserve the console verbose mode which we set at the start of the trap.

Quoted 3 lines

console loglevel is indirectly sets to verbose in oops_enter() so better to restore before oops_exit().
More from ARM maintainers.

If we've hit die(), something has already gone very wrong within the kernel (not userspace), and we're likely to see subsequent issues anyway.

Rolling back the console loglevel might throw away infromation on subsequent failures, which isn't ideal, so leaving it as-is is probably fine.

Quoted 25 lines

Quoted 25 lines

arch/arm64/kernel/traps.c asmlinkage void bad_mode(struct pt_regs *regs, int reason, unsigned int esr) · line 629

Quoted 5 lines

arch/arm64/kernel/traps.c +1 -0

@@ -624,6 +626,7 @@ asmlinkage void bad_mode(struct pt_regs *regs, int reason, unsigned int esr)
      die("Oops - bad mode", regs, 0);
      local_daif_mask();
      panic("bad mode");
+     restore_console_loglevel();

panic()

arch/arm64/kernel/traps.c asmlinkage void handle_bad_stack(struct pt_regs *regs) · line 686

Quoted 5 lines

arch/arm64/kernel/traps.c +1 -0

@@ -680,6 +683,7 @@ asmlinkage void handle_bad_stack(struct pt_regs *regs)
       * to get a better stack trace.
       */
      nmi_panic(NULL, "kernel stack overflow");
+     restore_console_loglevel();

panic()

arch/arm64/kernel/traps.c void __noreturn arm64_serror_panic(struct pt_regs *regs, u32 esr) · line 701

Quoted 6 lines

arch/arm64/kernel/traps.c +1 -1

@@ -694,7 +698,7 @@ void __noreturn arm64_serror_panic(struct pt_regs *regs, u32 esr)
              __show_regs(regs);
 
      nmi_panic(regs, "Asynchronous SError Interrupt");
-
+     restore_console_loglevel();

panic()

Quoted 5 lines

Quoted 3 lines

What's the point of restore_console_loglevel() after panic()?

        -ss

Except die(), other places restore call can be avoided but just added for code completeness.

As Sergey said, panic() is *fatal*. It does not return, so it makes no sense to place code after it.

Thanks,
Mark.

Quoted 32 lines · Mark Rutland

Quoted 23 lines · Hari Vyas

Quoted 5 lines · Sergey Senozhatsky

On (07/19/18 15:18), Hari Vyas wrote:
[..]

Not so sure about the patch, sorry.

Mentioned concern in cover letter and commit message. Console loglevel remains maximum after process die which
needs to be restored.

Quoted 13 lines

arch/arm64/kernel/traps.c void die(const char *str, struct pt_regs *regs, int err) · line 216

Quoted 7 lines

arch/arm64/kernel/traps.c +2 -0

@@ -211,6 +211,8 @@ void die(const char *str, struct pt_regs *regs, int err)
 
      bust_spinlocks(0);
      add_taint(TAINT_DIE, LOCKDEP_NOW_UNRELIABLE);
+
+     restore_console_loglevel();
      oops_exit();

This is *probably* the only place where you can roll-back the loglevel, but you restore console loglevel too early - a trap can panic() the system, we need to preserve the console verbose mode which we set at the start of the trap.

Quoted 3 lines

console loglevel is indirectly sets to verbose in oops_enter() so better to restore before oops_exit().
More from ARM maintainers.

If we've hit die(), something has already gone very wrong within the kernel (not userspace), and we're likely to see subsequent issues anyway.

Quoted 4 lines

Rolling back the console loglevel might throw away infromation on subsequent failures, which isn't ideal, so leaving it as-is is probably fine.

This die() is called from so many places i.e. entry.S shows so many ways i.e el1_sync_invalid,irq,fiq_invalid,error_invalid.

This concern was identified in 4.14 kernel and was easy to produce.

Quoted 3 lines

Quoted 1 line

This die() is called from so many places i.e. entry.S shows so many ways i.e el1_sync_invalid,irq,fiq_invalid,error_invalid.

Indeed, but *none* of these should fire unless something has gone wrong already.

Quoted 3 lines · Mark Rutland

Quoted 3 lines

Quoted 1 line

This die() is called from so many places i.e. entry.S shows so many ways i.e el1_sync_invalid,irq,fiq_invalid,error_invalid.

[..]

Quoted 1 line

Indeed, but *none* of these should fire unless something has gone wrong already.

[..]

Agreed.

I see that all those arm64/kernel/entry.S el0_sync_invalid, el0_irq_invalid, el0_fiq_invalid, and so on, invoke inv_entry macro which calls into bad_mode, which ends up in panic() anyway:

asmlinkage void bad_mode(struct pt_regs *regs, int reason, unsigned int esr)

{
        console_verbose();

        pr_crit("Bad mode in %s handler detected on CPU%d, code 0x%08x -- %s\n",
                handler[reason], smp_processor_id(), esr,
                esr_get_class_string(esr));

        die("Oops - bad mode", regs, 0);
        local_daif_mask();
        panic("bad mode");
}

bad_mode is always fatal.

When the kernel becomes suicidal and wants to die() we better have verbose consoles. So, no, I don't think I'm buying the patch.

        -ss

Quoted 30 lines · Sergey Senozhatsky

Quoted 3 lines · Mark Rutland

Quoted 3 lines

Quoted 1 line

This die() is called from so many places i.e. entry.S shows so many ways i.e el1_sync_invalid,irq,fiq_invalid,error_invalid.

[..]

Quoted 1 line

Indeed, but *none* of these should fire unless something has gone wrong already.

[..]

Agreed.

I see that all those arm64/kernel/entry.S el0_sync_invalid, el0_irq_invalid, el0_fiq_invalid, and so on, invoke inv_entry macro which calls into bad_mode, which ends up in panic() anyway:

asmlinkage void bad_mode(struct pt_regs *regs, int reason, unsigned int esr)

{
        console_verbose();

        pr_crit("Bad mode in %s handler detected on CPU%d, code 0x%08x -- %s\n",
                handler[reason], smp_processor_id(), esr,
                esr_get_class_string(esr));

        die("Oops - bad mode", regs, 0);
        local_daif_mask();
        panic("bad mode");
}

Quoted 6 lines

bad_mode is always fatal.

When the kernel becomes suicidal and wants to die() we better have verbose consoles. So, no, I don't think I'm buying the patch.

        -ss

This was the behavior observed with 4.14 kernel version and proposed patch. I understand, giving access to system memory access to user is bad and should be avoided so one may argue
simplest but bad example and unpredictable results may be observed. For other examples, I may need to work and find out. Please decide.

root at bcm958802a8021:~# uname -a

Linux bcm958802a8021 4.14.49+g8f1356a #1 SMP Mon Jul 9 22:37:03 UTC 2018 aarch64 aarch64 aarch64 GNU/Linux

root at bcm958802a8021:~# cat /proc/sys/kernel/printk

7 4 1 7 <==CONSOLE LOGLEVEL IS SET TO 7

root at bcm958802a8021:~# echo 1 > /sys/bus/pci/rescan <=== NO PRINT

root at bcm958802a8021:~# devmem 0x0

[   46.166083] Bad mode in Error handler detected on CPU1, code 0xbf000000 -- SError
[   46.173805] Internal error: Oops - bad mode: 0 [#1] SMP

Quoted 51 lines · Hari Vyas

Quoted 30 lines · Sergey Senozhatsky

Quoted 3 lines · Mark Rutland

Quoted 3 lines

Quoted 1 line

This die() is called from so many places i.e. entry.S shows so many ways i.e el1_sync_invalid,irq,fiq_invalid,error_invalid.

[..]

Quoted 1 line

Indeed, but *none* of these should fire unless something has gone wrong already.

[..]

Agreed.

I see that all those arm64/kernel/entry.S el0_sync_invalid, el0_irq_invalid, el0_fiq_invalid, and so on, invoke inv_entry macro which calls into bad_mode, which ends up in panic() anyway:

asmlinkage void bad_mode(struct pt_regs *regs, int reason, unsigned int esr)

{
        console_verbose();

        pr_crit("Bad mode in %s handler detected on CPU%d, code 0x%08x -- %s\n",
                handler[reason], smp_processor_id(), esr,
                esr_get_class_string(esr));

        die("Oops - bad mode", regs, 0);
        local_daif_mask();
        panic("bad mode");
}

Quoted 6 lines

bad_mode is always fatal.

When the kernel becomes suicidal and wants to die() we better have verbose consoles. So, no, I don't think I'm buying the patch.

        -ss

This was the behavior observed with 4.14 kernel version and proposed patch. I understand, giving access to system memory access to user is bad and should be avoided so one may argue
simplest but bad example and unpredictable results may be observed. For other examples, I may need to work and find out. Please decide.

root at bcm958802a8021:~# uname -a

Linux bcm958802a8021 4.14.49+g8f1356a #1 SMP Mon Jul 9 22:37:03 UTC 2018 aarch64 aarch64 aarch64 GNU/Linux

root at bcm958802a8021:~# cat /proc/sys/kernel/printk

Quoted 8 lines

7 4 1 7 <==CONSOLE LOGLEVEL IS SET TO 7

root at bcm958802a8021:~# echo 1 > /sys/bus/pci/rescan <=== NO PRINT

root at bcm958802a8021:~# devmem 0x0

[   46.166083] Bad mode in Error handler detected on CPU1, code 0xbf000000 -- SError
[   46.173805] Internal error: Oops - bad mode: 0 [#1] SMP

As previously discussed, this case *is not* recoverable, and *must* be handled fatally. The fact that old kernels did not handle this fatally is a *bug*.

If you are seeing SError under normal conditions, something is *very* wrong with your system. If you are *deliberately* provoking SError, this is your own fault.

Thanks,
Mark.

Yes. As per earlier conversation, it was a bug in earlier kernel versions but fact here is that bad_mode()-->die()-->__die()--> probably do_exit() handler is returning (in 4.14 wrongly) and latest too if such type of situation arises though looks difficult to create in normal scenario.

From a quick look, it seems that will be rather messy; this code could

Quoted 14 lines · Mark Rutland

Quoted 13 lines · Hari Vyas

Quoted 11 lines · Sergey Senozhatsky

arch/arm64/kernel/traps.c void __noreturn arm64_serror_panic(struct pt_regs *regs, u32 esr) · line 701

Quoted 6 lines

arch/arm64/kernel/traps.c +1 -1

@@ -694,7 +698,7 @@ void __noreturn arm64_serror_panic(struct pt_regs *regs, u32 esr)
              __show_regs(regs);
 
      nmi_panic(regs, "Asynchronous SError Interrupt");
-
+     restore_console_loglevel();

panic()

What's the point of restore_console_loglevel() after panic()?

Quoted 7 lines

Quoted 3 lines

Quoted 1 line

        -ss

Except die(), other places restore call can be avoided but just added for code completeness.

As Sergey said, panic() is *fatal*. It does not return, so it makes no sense to place code after it.

I too pointed in cover letter. I just too added for code completeness.

Quoted 15 lines · Hari Vyas

Quoted 14 lines · Mark Rutland

Quoted 13 lines · Hari Vyas

Quoted 11 lines · Sergey Senozhatsky

arch/arm64/kernel/traps.c void __noreturn arm64_serror_panic(struct pt_regs *regs, u32 esr) · line 701

Quoted 6 lines

arch/arm64/kernel/traps.c +1 -1

@@ -694,7 +698,7 @@ void __noreturn arm64_serror_panic(struct pt_regs *regs, u32 esr)
              __show_regs(regs);
 
      nmi_panic(regs, "Asynchronous SError Interrupt");
-
+     restore_console_loglevel();

panic()

What's the point of restore_console_loglevel() after panic()?

Quoted 8 lines

Quoted 7 lines

Quoted 3 lines

Quoted 1 line

        -ss

Except die(), other places restore call can be avoided but just added for code completeness.

As Sergey said, panic() is *fatal*. It does not return, so it makes no sense to place code after it.

I too pointed in cover letter. I just too added for code completeness.

The code-completeness is not a good reason. It might cause false expectation that panic() is not fatal. It would cause more harm than good.

The same is true also for die(). It should be fatal. I agree with Sergey and Mark that it does not make sense to restore the original level there.

Best Regards,
Petr

Quoted 29 lines · Petr Mladek

Quoted 15 lines · Hari Vyas

Quoted 14 lines · Mark Rutland

Quoted 13 lines · Hari Vyas

Quoted 11 lines · Sergey Senozhatsky

Review void __noreturn arm64_serror_panic(struct pt_regs *regs, u32 esr) · line 701

Quoted 6 lines

(quoted diff) +1 -1

@@ -694,7 +698,7 @@ void __noreturn arm64_serror_panic(struct pt_regs *regs, u32 esr)
              __show_regs(regs);
 
      nmi_panic(regs, "Asynchronous SError Interrupt");
-
+     restore_console_loglevel();

panic()

What's the point of restore_console_loglevel() after panic()?

Quoted 8 lines

Quoted 7 lines

Quoted 3 lines

Quoted 1 line

        -ss

Except die(), other places restore call can be avoided but just added for code completeness.

As Sergey said, panic() is *fatal*. It does not return, so it makes no sense to place code after it.

I too pointed in cover letter. I just too added for code completeness.

The code-completeness is not a good reason. It might cause false expectation that panic() is not fatal. It would cause more harm than good.

Quoted 6 lines

The same is true also for die(). It should be fatal. I agree with Sergey and Mark that it does not make sense to restore the original level there.

Best Regards,
Petr

I can remove restore from not required (3) places.

Other than that, based on discussion, bad_mode() should panic so better die() call itself be removed from bad_mode().
As this(i.e. cleanup of bad_mode) is not related to raised patch so better one another bug/patch be raised for cleaning bad_mode() and probably die() too.

Currently this die() is called from some other places also but not completely sure whether restore_console_loglevel is required there also or not as creating situation will be very difficult in normal scenario.

For tracking purpose, I have raised a separate bug for bad_mode() issue i.e. Bug 200637 - ARM64: bad_mode() handler may not result in panic always due to die() call in the beginning
Probable fix could be to remove die() from bad_mode() but better other confirms.

As stated, die() is called from other places also my feeling and logically restoration is required. Please
confirm whether to close this issue without any change or proceed with only one restore() call in die and discard at
other 3 places.

Quoted 10 lines · Hari Vyas

For tracking purpose, I have raised a separate bug for bad_mode() issue i.e. Bug 200637 - ARM64: bad_mode() handler may not result in panic always due to die() call in the beginning
Probable fix could be to remove die() from bad_mode() but better other confirms.

As stated, die() is called from other places also my feeling and logically restoration is required. Please
confirm whether to close this issue without any change or proceed with only one restore() call in die and discard at
other 3 places.

I think that if we've encountered a die(), leaving the loglevel up is fine. Something has gone very wrong, regardless of whether we try to continue.

Mark.

changes in v1:
Separate functions for changing, retrieving, storing and restoring console loglevel messages.
Restoring console loglevel message is done from arm64 trap handling code but can be extended on need basis by other user. Restoring is actually required in die() function only and other places kernel will panic but added considering code completion