[PATCH] arm64: fix for restoring console loglevel after handling traps

[PATCH] arm64: fix for restoring console loglevel after handling traps

[Hari Vyas]

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

Hari Vyas (1):
  arm64: fix for restoring console loglevel after handling traps

 arch/arm64/kernel/traps.c |  6 +++++-
 include/linux/printk.h    | 38 +++++++++++++++++++++++++++++++++++---
 kernel/printk/printk.c    |  3 +++
 3 files changed, 43 insertions(+), 4 deletions(-)

-- 
1.9.1

[PATCH] arm64: fix for restoring console loglevel after handling traps

[Hari Vyas]

As part of handling traps, console loglevel is set to max i.e.
motormouth to provide maximum log information but this is not
restored after graceful handling of culprit user level process.

As console loglevel is set to maximum, this causes unnecessary
debug messages from modules which supports dynamic debugging.
One such example is pci module which prints lot of unnecessary
debug message while rescanning pci device.

Issue is fixed by providing separate functions for changing
console loglevel message, saving previous console loglevel
and restoring loglevel.

Currently loglevel is restored from arm64 traps handling where
issue was observed but same can be adapted from other (platform)
specific code on need basis.

Signed-off-by: Hari Vyas <hari.vyas@broadcom.com>
---
 arch/arm64/kernel/traps.c |  6 +++++-
 include/linux/printk.h    | 38 +++++++++++++++++++++++++++++++++++---
 kernel/printk/printk.c    |  3 +++
 3 files changed, 43 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
index d399d45..6d37995 100644
--- a/arch/arm64/kernel/traps.c
+++ b/arch/arm64/kernel/traps.c
@@ -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();
 
 	if (in_interrupt())
@@ -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();
 }
 
 /*
@@ -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();
 	cpu_park_loop();
 }
 #endif
@@ -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();
 	cpu_park_loop();
 	unreachable();
 }
diff --git a/include/linux/printk.h b/include/linux/printk.h
index 6d7e800..7c261f8 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -60,22 +60,54 @@ static inline const char *printk_skip_headers(const char *buffer)
  */
 #define CONSOLE_LOGLEVEL_DEFAULT CONFIG_CONSOLE_LOGLEVEL_DEFAULT
 
+#define LOGLEVEL_STACK_MAX 4
+#define LOGLEVEL_STACK_EMPTY -1
 extern int console_printk[];
+extern int stack_pos;
+extern int old_console_loglevel[];
 
 #define console_loglevel (console_printk[0])
 #define default_message_loglevel (console_printk[1])
 #define minimum_console_loglevel (console_printk[2])
 #define default_console_loglevel (console_printk[3])
 
+static inline void set_console_loglevel(int loglevel)
+{
+	console_loglevel = loglevel;
+}
+
+static inline int get_console_loglevel(void)
+{
+	return console_loglevel;
+}
+
+/* Sets new console log level and saves previous one */
+static inline void break_console_loglevel(int loglevel)
+{
+	int current_loglevel = get_console_loglevel();
+
+	if (loglevel != current_loglevel) {
+		if (stack_pos < LOGLEVEL_STACK_MAX)
+			old_console_loglevel[++stack_pos] = current_loglevel;
+		set_console_loglevel(loglevel);
+	}
+}
+
+/* Restores previous console log level */
+static inline void restore_console_loglevel(void)
+{
+	if (stack_pos > LOGLEVEL_STACK_EMPTY)
+		console_loglevel = old_console_loglevel[stack_pos--];
+}
+
 static inline void console_silent(void)
 {
-	console_loglevel = CONSOLE_LOGLEVEL_SILENT;
+	break_console_loglevel(CONSOLE_LOGLEVEL_SILENT);
 }
 
 static inline void console_verbose(void)
 {
-	if (console_loglevel)
-		console_loglevel = CONSOLE_LOGLEVEL_MOTORMOUTH;
+	break_console_loglevel(CONSOLE_LOGLEVEL_MOTORMOUTH);
 }
 
 /* strlen("ratelimit") + 1 */
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 2478083..6548b52 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -66,6 +66,9 @@
 	CONSOLE_LOGLEVEL_DEFAULT,	/* default_console_loglevel */
 };
 
+int stack_pos = -1;
+int old_console_loglevel[LOGLEVEL_STACK_MAX];
+
 /*
  * Low level drivers may need that to know if they can schedule in
  * their unblank() callback or not. So let's export it.
-- 
1.9.1

[PATCH] arm64: fix for restoring console loglevel after handling traps

[Sergey Senozhatsky]

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

Not so sure about the patch, sorry.

> @@ -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.

> @@ -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()

> @@ -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()

> @@ -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

[PATCH] arm64: fix for restoring console loglevel after handling traps

[Hari Vyas]

On Thu, Jul 19, 2018 at 3:31 PM, Sergey Senozhatsky
<sergey.senozhatsky.work@gmail.com> wrote:
> 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.
>> @@ -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.
>> @@ -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()
>
>> @@ -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()
>
>> @@ -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
Except die(), other places restore call can be avoided but just added
for code completeness.
Same mentioned in cover letter.
More from ARM maintainers

[PATCH] arm64: fix for restoring console loglevel after handling traps

[Mark Rutland]

On Thu, Jul 19, 2018 at 03:47:46PM +0530, Hari Vyas wrote:
> On Thu, Jul 19, 2018 at 3:31 PM, Sergey Senozhatsky
> <sergey.senozhatsky.work@gmail.com> wrote:
> > 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.
> >> @@ -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.

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.

> >> @@ -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()
> >
> >> @@ -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()
> >
> >> @@ -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
> 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.

[PATCH] arm64: fix for restoring console loglevel after handling traps

[Hari Vyas]

On Thu, Jul 19, 2018 at 4:09 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Thu, Jul 19, 2018 at 03:47:46PM +0530, Hari Vyas wrote:
>> On Thu, Jul 19, 2018 at 3:31 PM, Sergey Senozhatsky
>> <sergey.senozhatsky.work@gmail.com> wrote:
>> > 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.
>> >> @@ -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.
>
> 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.
>
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. Now
after entry.S change,  scenario may not be easy but code perspective, it is
very much possible as arm64_notify_die(), force_signal_inject(), bug_handler()
too calls.
>> >> @@ -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()
>> >
>> >> @@ -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()
>> >
>> >> @@ -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
>> 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.
> Thanks,
> Mark.

[PATCH] arm64: fix for restoring console loglevel after handling traps

[Mark Rutland]

On Thu, Jul 19, 2018 at 07:42:50PM +0530, Hari Vyas wrote:
> On Thu, Jul 19, 2018 at 4:09 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> > On Thu, Jul 19, 2018 at 03:47:46PM +0530, Hari Vyas wrote:
> >> On Thu, Jul 19, 2018 at 3:31 PM, Sergey Senozhatsky
> >> <sergey.senozhatsky.work@gmail.com> wrote:
> >> > 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.
> >> >> @@ -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.
> >
> > 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.
> >
> 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.

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

Under what circumstances do you see die() being called?

Can you provide a backtrace?

Thanks,
Mark.

[PATCH] arm64: fix for restoring console loglevel after handling traps

[Sergey Senozhatsky]

On (07/20/18 07:26), Mark Rutland wrote:
> > >
> > 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.

[..]

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

[PATCH] arm64: fix for restoring console loglevel after handling traps

[Hari Vyas]

On Fri, Jul 20, 2018 at 12:10 PM, Sergey Senozhatsky
<sergey.senozhatsky.work@gmail.com> wrote:
> On (07/20/18 07:26), Mark Rutland wrote:
>> > >
>> > 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.
>
> [..]
>
> 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
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@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

[   46.179194] Modules linked in: bnxt_re(O) rdma_ucm ib_uverbs
rdma_cm iw_cm ib_cm ib_core bnxt_en(O) nfsd ipv6

[   46.189435] CPU: 1 PID: 4862 Comm: devmem Tainted: G           O
4.14.49+g8f1356a #1

[   46.197692] Hardware name: Stingray PS225xx (BCM958802A8021) (DT)

[   46.203978] task: ffff800b67619900 task.stack: ffff000013a00000

[   46.210084] PC is at 0x406530

[   46.213144] LR is@0x40cdec

[   46.216203] pc : [<0000000000406530>] lr : [<000000000040cdec>]
pstate: 60000000

[   46.223833] sp : 0000ffffc6560a50

[   46.227250] x29: 0000ffffc6560b70 x28: 0000000000000000

[   46.232731] x27: 0000000000000000 x26: 0000000000000000

[   46.238210] x25: 0000ffffc6560ca0 x24: 0000000000000003

[   46.243688] x23: 0000000000001000 x22: 0000ffffc6560ca8

[   46.249167] x21: 0000000000000000 x20: 0000ffffb0a05000

[   46.254645] x19: 0000000000000020 x18: 00000000000004ca

[   46.260124] x17: 00000000004b79a8 x16: 0000ffffb0896ad0

[   46.265603] x15: 0000ffffb07cdde0 x14: 0000ffffb07db2c8

[   46.271081] x13: 0000ffffb091cac8 x12: 0000000000000000

[   46.276561] x11: 0000000000000000 x10: 0101010101010101

[   46.282040] x9 : 0000ffffb08dc300 x8 : 00000000000000de

[   46.287519] x7 : 0000ffffb08dcc00 x6 : 0000000000000000

[   46.292998] x5 : 0000000000000000 x4 : 0000000000000003

[   46.298477] x3 : 0000000000000001 x2 : 0000000000000000

[   46.303955] x1 : 0000000000000008 x0 : 000000000048c4e8

[   46.309434] Process devmem (pid: 4862, stack limit = 0xffff000013a00000)

[   46.316347] ---[ end trace f57fc7b196a41802 ]---



Message from syslogd at bcm958802a8021 at Jul  9 22:08:54 ...

kernel:[   46.173805] Internal error: Oops - bad mode: 0 [#1] SMP



Message from syslogd at bcm958802a8021 at Jul  9 22:08:54 ...

kernel:[   46.309434] Process devmem (pid: 4862, stack limit =
0xffff000013a00000)

Segmentation fault

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

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

root@bcm958802a8021:~# echo 1 > /sys/bus/pci/rescan <=== UNNECESSARY
DEBUG MESSAGES

[   53.861448] pci_bus 0008:00: scanning bus

[   53.865599] pci 0008:00:00.0: scanning [bus 01-01] behind bridge, pass 0

[   53.872521] pci_bus 0008:00: 2-byte config write to 0008:00:00.0
offset 0x3e may corrupt adjacent RW1C bits

[   53.882579] pci_bus 0008:01: scanning bus

[   53.886742] pci_bus 0008:01: bus scan returning with max=01

[   53.892496] pci_bus 0008:00: 2-byte config write to 0008:00:00.0
offset 0x3e may corrupt adjacent RW1C bits

[   53.902552] pci 0008:00:00.0: scanning [bus 01-01] behind bridge, pass 1

[   53.909469] pci_bus 0008:00: 2-byte config write to 0008:00:00.0
offset 0x3e may corrupt adjacent RW1C bits

[   53.919523] pci_bus 0008:00: 2-byte config write to 0008:00:00.0
offset 0x3e may corrupt adjacent RW1C bits

[   53.929578] pci_bus 0008:00: bus scan returning with max=01

[   53.935331] pci_bus 0008:00: 2-byte config write to 0008:00:00.0
offset 0x1c may corrupt adjacent RW1C bits

[   53.945385] pci_bus 0008:00: 2-byte config write to 0008:00:00.0
offset 0x1c may corrupt adjacent RW1C bits

root@bcm958802a8021:~#

[PATCH] arm64: fix for restoring console loglevel after handling traps

[Florian Fainelli]

On 07/19/2018 02:48 AM, Hari Vyas wrote:
> 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

Hari, there is no need for a cover letter for single patch submissions,
if you want to put a changelog, you can put it after the --- below your
Signed-off-by tag, thank you.
-- 
Florian

[PATCH] arm64: fix for restoring console loglevel after handling traps

[Mark Rutland]

On Fri, Jul 20, 2018 at 01:01:39PM +0530, Hari Vyas wrote:
> On Fri, Jul 20, 2018 at 12:10 PM, Sergey Senozhatsky
> <sergey.senozhatsky.work@gmail.com> wrote:
> > On (07/20/18 07:26), Mark Rutland wrote:
> >> > >
> >> > 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.
> >
> > [..]
> >
> > 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
> 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

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.

[PATCH] arm64: fix for restoring console loglevel after handling traps

[Hari Vyas]

On Fri, Jul 20, 2018 at 3:14 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Fri, Jul 20, 2018 at 01:01:39PM +0530, Hari Vyas wrote:
>> On Fri, Jul 20, 2018 at 12:10 PM, Sergey Senozhatsky
>> <sergey.senozhatsky.work@gmail.com> wrote:
>> > On (07/20/18 07:26), Mark Rutland wrote:
>> >> > >
>> >> > 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.
>> >
>> > [..]
>> >
>> > 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
>> 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
>
> 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.

[PATCH] arm64: fix for restoring console loglevel after handling traps

[Mark Rutland]

On Fri, Jul 20, 2018 at 04:27:50PM +0530, Hari Vyas wrote:
> 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.

The intent is that bad_mode() should always be fatal, so we should fix
bad_mode() to ensure that this is always the case.

>From a quick look, it seems that will be rather messy; this code could
do with a cleanup. :/

Thanks,
Mark.

[PATCH] arm64: fix for restoring console loglevel after handling traps

[Petr Mladek]

On Thu 2018-07-19 19:42:50, Hari Vyas wrote:
> On Thu, Jul 19, 2018 at 4:09 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> > On Thu, Jul 19, 2018 at 03:47:46PM +0530, Hari Vyas wrote:
> >> On Thu, Jul 19, 2018 at 3:31 PM, Sergey Senozhatsky
> >> <sergey.senozhatsky.work@gmail.com> wrote:
> >> >> @@ -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
> >> 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

[PATCH] arm64: fix for restoring console loglevel after handling traps

[Hari Vyas]

On Fri, Jul 20, 2018 at 5:43 PM, Petr Mladek <pmladek@suse.com> wrote:
> On Thu 2018-07-19 19:42:50, Hari Vyas wrote:
>> On Thu, Jul 19, 2018 at 4:09 PM, Mark Rutland <mark.rutland@arm.com> wrote:
>> > On Thu, Jul 19, 2018 at 03:47:46PM +0530, Hari Vyas wrote:
>> >> On Thu, Jul 19, 2018 at 3:31 PM, Sergey Senozhatsky
>> >> <sergey.senozhatsky.work@gmail.com> wrote:
>> >> >> @@ -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
>> >> 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
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.

[PATCH] arm64: fix for restoring console loglevel after handling traps

[Hari Vyas]

Hi all,

On Fri, Jul 20, 2018 at 7:04 PM, Hari Vyas <hari.vyas@broadcom.com> wrote:
> On Fri, Jul 20, 2018 at 5:43 PM, Petr Mladek <pmladek@suse.com> wrote:
>> On Thu 2018-07-19 19:42:50, Hari Vyas wrote:
>>> On Thu, Jul 19, 2018 at 4:09 PM, Mark Rutland <mark.rutland@arm.com> wrote:
>>> > On Thu, Jul 19, 2018 at 03:47:46PM +0530, Hari Vyas wrote:
>>> >> On Thu, Jul 19, 2018 at 3:31 PM, Sergey Senozhatsky
>>> >> <sergey.senozhatsky.work@gmail.com> wrote:
>>> >> >> @@ -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
>>> >> 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
> 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.

[PATCH] arm64: fix for restoring console loglevel after handling traps

[Mark Rutland]

On Tue, Jul 24, 2018 at 02:44:48PM +0530, Hari Vyas wrote:
> 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.

[PATCH] arm64: fix for restoring console loglevel after handling traps

[Hari Vyas]

On Tue, Jul 24, 2018 at 3:18 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Tue, Jul 24, 2018 at 02:44:48PM +0530, Hari Vyas wrote:
>> 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.
>
Traps/exception will not happen in normal scenario any how. Just
re-checked code, die() is not a static function currently and
it is being called from do_page_fault() and do_kernel_fault() defined
in mm/fault.c Probably those calls also shouldn't happen in normal
scenario.
In that case, better to reject issue then.
> Mark.