[PATCH] x86/entry: re-enable interrupts before exiting

[PATCH] x86/entry: re-enable interrupts before exiting

[Tycho Andersen]

If the kernel oopses in an interrupt, nothing re-enables interrupts:

Aug 23 19:30:27 xpfo kernel: [   38.302714] BUG: sleeping function called from invalid context at
./include/linux/percpu-rwsem.h:33
Aug 23 19:30:27 xpfo kernel: [   38.303837] in_atomic(): 0, irqs_disabled(): 1, pid: 1970, name:
lkdtm_xpfo_test
Aug 23 19:30:27 xpfo kernel: [   38.304758] CPU: 3 PID: 1970 Comm: lkdtm_xpfo_test Tainted: G      D
4.13.0-rc5+ #228
Aug 23 19:30:27 xpfo kernel: [   38.305813] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
1.10.1-1ubuntu1 04/01/2014
Aug 23 19:30:27 xpfo kernel: [   38.306926] Call Trace:
Aug 23 19:30:27 xpfo kernel: [   38.307243]  dump_stack+0x63/0x8b
Aug 23 19:30:27 xpfo kernel: [   38.307665]  ___might_sleep+0xec/0x110
Aug 23 19:30:27 xpfo kernel: [   38.308139]  __might_sleep+0x45/0x80
Aug 23 19:30:27 xpfo kernel: [   38.308593]  exit_signals+0x21/0x1c0
Aug 23 19:30:27 xpfo kernel: [   38.309046]  ? blocking_notifier_call_chain+0x11/0x20
Aug 23 19:30:27 xpfo kernel: [   38.309677]  do_exit+0x98/0xbf0
Aug 23 19:30:27 xpfo kernel: [   38.310078]  ? smp_reader+0x27/0x40 [lkdtm]
Aug 23 19:30:27 xpfo kernel: [   38.310604]  ? kthread+0x10f/0x150
Aug 23 19:30:27 xpfo kernel: [   38.311045]  ? read_user_with_flags+0x60/0x60 [lkdtm]
Aug 23 19:30:27 xpfo kernel: [   38.311680]  rewind_stack_do_exit+0x17/0x20

do_exit() expects to be called in a well-defined environment, so let's
re-enable interrupts after unwinding the stack, in case they were disabled.

Note that if any spinlocks are held, etc. we'll also get the above warning,
so this isn't a silver bullet. So, let's add a C helper in case someone
wants to add fancier lock busting or if we've forgotten to unwind something
else.

I've had to add back in the hack that Josh removed in 8c1f75587a18
("x86/entry/64: Add unwind hint annotations") with the loop after the call,
because for whatever reason without that I get a warning:

  AS      arch/x86/entry/entry_64.o
arch/x86/entry/entry_64.o: warning: objtool: .entry.text: unexpected end of section

It seems to actually work fine for me though, since the new helper is also
__noreturn. Perhaps there's a better way to do this?

Signed-off-by: Tycho Andersen <tycho@tycho.ws>
CC: Josh Poimboeuf <jpoimboe@redhat.com>
---
I split this out from the XPFO series since it's mostly unrelated, and is
just a bug I found while working on that series.
---
 arch/x86/entry/common.c   | 10 ++++++++++
 arch/x86/entry/entry_32.S |  2 +-
 arch/x86/entry/entry_64.S |  3 ++-
 3 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/arch/x86/entry/common.c b/arch/x86/entry/common.c
index 7bc105f47d21..4e9c54e0495f 100644
--- a/arch/x86/entry/common.c
+++ b/arch/x86/entry/common.c
@@ -427,3 +427,13 @@ __visible long do_fast_syscall_32(struct pt_regs *regs)
 #endif
 }
 #endif
+
+void __noreturn __finish_rewind_stack_do_exit(long code)
+{
+	/*
+	 * If we oopsed in an interrupt handler, interrupts may be off. Let's turn
+	 * them back on before going back to "normal" code.
+	 */
+	 local_irq_enable();
+	 do_exit(code);
+}
diff --git a/arch/x86/entry/entry_32.S b/arch/x86/entry/entry_32.S
index d309f30cf7af..095b8770e3b0 100644
--- a/arch/x86/entry/entry_32.S
+++ b/arch/x86/entry/entry_32.S
@@ -1507,6 +1507,6 @@ ENTRY(rewind_stack_do_exit)
 	movl	PER_CPU_VAR(cpu_current_top_of_stack), %esi
 	leal	-TOP_OF_KERNEL_STACK_PADDING-PTREGS_SIZE(%esi), %esp
 
-	call	do_exit
+	call	__finish_rewind_stack_do_exit
 1:	jmp 1b
 END(rewind_stack_do_exit)
diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
index 1f0efdb7b629..c6166133e45d 100644
--- a/arch/x86/entry/entry_64.S
+++ b/arch/x86/entry/entry_64.S
@@ -1672,5 +1672,6 @@ ENTRY(rewind_stack_do_exit)
 	leaq	-PTREGS_SIZE(%rax), %rsp
 	UNWIND_HINT_FUNC sp_offset=PTREGS_SIZE
 
-	call	do_exit
+	call	__finish_rewind_stack_do_exit
+1:	jmp 1b
 END(rewind_stack_do_exit)
-- 
2.19.1

Re: [PATCH] x86/entry: re-enable interrupts before exiting

[Josh Poimboeuf]

On Fri, Apr 05, 2019 at 09:35:24AM -0600, Tycho Andersen wrote:
> If the kernel oopses in an interrupt, nothing re-enables interrupts:
> 
> Aug 23 19:30:27 xpfo kernel: [   38.302714] BUG: sleeping function called from invalid context at
> ./include/linux/percpu-rwsem.h:33
> Aug 23 19:30:27 xpfo kernel: [   38.303837] in_atomic(): 0, irqs_disabled(): 1, pid: 1970, name:
> lkdtm_xpfo_test
> Aug 23 19:30:27 xpfo kernel: [   38.304758] CPU: 3 PID: 1970 Comm: lkdtm_xpfo_test Tainted: G      D
> 4.13.0-rc5+ #228
> Aug 23 19:30:27 xpfo kernel: [   38.305813] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
> 1.10.1-1ubuntu1 04/01/2014
> Aug 23 19:30:27 xpfo kernel: [   38.306926] Call Trace:
> Aug 23 19:30:27 xpfo kernel: [   38.307243]  dump_stack+0x63/0x8b
> Aug 23 19:30:27 xpfo kernel: [   38.307665]  ___might_sleep+0xec/0x110
> Aug 23 19:30:27 xpfo kernel: [   38.308139]  __might_sleep+0x45/0x80
> Aug 23 19:30:27 xpfo kernel: [   38.308593]  exit_signals+0x21/0x1c0
> Aug 23 19:30:27 xpfo kernel: [   38.309046]  ? blocking_notifier_call_chain+0x11/0x20
> Aug 23 19:30:27 xpfo kernel: [   38.309677]  do_exit+0x98/0xbf0
> Aug 23 19:30:27 xpfo kernel: [   38.310078]  ? smp_reader+0x27/0x40 [lkdtm]
> Aug 23 19:30:27 xpfo kernel: [   38.310604]  ? kthread+0x10f/0x150
> Aug 23 19:30:27 xpfo kernel: [   38.311045]  ? read_user_with_flags+0x60/0x60 [lkdtm]
> Aug 23 19:30:27 xpfo kernel: [   38.311680]  rewind_stack_do_exit+0x17/0x20
> 
> do_exit() expects to be called in a well-defined environment, so let's
> re-enable interrupts after unwinding the stack, in case they were disabled.
> 
> Note that if any spinlocks are held, etc. we'll also get the above warning,
> so this isn't a silver bullet. So, let's add a C helper in case someone
> wants to add fancier lock busting or if we've forgotten to unwind something
> else.
> 
> I've had to add back in the hack that Josh removed in 8c1f75587a18
> ("x86/entry/64: Add unwind hint annotations") with the loop after the call,
> because for whatever reason without that I get a warning:
> 
>   AS      arch/x86/entry/entry_64.o
> arch/x86/entry/entry_64.o: warning: objtool: .entry.text: unexpected end of section
> 
> It seems to actually work fine for me though, since the new helper is also
> __noreturn. Perhaps there's a better way to do this?

Unfortunately, objtool doesn't have a way to detect noreturn functions
in other objects, so they're hard-coded in a list.  You can add
__finish_rewind_stack_do_exit to the global_noreturns array in
tools/objtool/check.c.

-- 
Josh

Re: [PATCH] x86/entry: re-enable interrupts before exiting

[Tycho Andersen]

On Fri, Apr 05, 2019 at 10:58:33AM -0500, Josh Poimboeuf wrote:
> On Fri, Apr 05, 2019 at 09:35:24AM -0600, Tycho Andersen wrote:
> > If the kernel oopses in an interrupt, nothing re-enables interrupts:
> > 
> > Aug 23 19:30:27 xpfo kernel: [   38.302714] BUG: sleeping function called from invalid context at
> > ./include/linux/percpu-rwsem.h:33
> > Aug 23 19:30:27 xpfo kernel: [   38.303837] in_atomic(): 0, irqs_disabled(): 1, pid: 1970, name:
> > lkdtm_xpfo_test
> > Aug 23 19:30:27 xpfo kernel: [   38.304758] CPU: 3 PID: 1970 Comm: lkdtm_xpfo_test Tainted: G      D
> > 4.13.0-rc5+ #228
> > Aug 23 19:30:27 xpfo kernel: [   38.305813] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
> > 1.10.1-1ubuntu1 04/01/2014
> > Aug 23 19:30:27 xpfo kernel: [   38.306926] Call Trace:
> > Aug 23 19:30:27 xpfo kernel: [   38.307243]  dump_stack+0x63/0x8b
> > Aug 23 19:30:27 xpfo kernel: [   38.307665]  ___might_sleep+0xec/0x110
> > Aug 23 19:30:27 xpfo kernel: [   38.308139]  __might_sleep+0x45/0x80
> > Aug 23 19:30:27 xpfo kernel: [   38.308593]  exit_signals+0x21/0x1c0
> > Aug 23 19:30:27 xpfo kernel: [   38.309046]  ? blocking_notifier_call_chain+0x11/0x20
> > Aug 23 19:30:27 xpfo kernel: [   38.309677]  do_exit+0x98/0xbf0
> > Aug 23 19:30:27 xpfo kernel: [   38.310078]  ? smp_reader+0x27/0x40 [lkdtm]
> > Aug 23 19:30:27 xpfo kernel: [   38.310604]  ? kthread+0x10f/0x150
> > Aug 23 19:30:27 xpfo kernel: [   38.311045]  ? read_user_with_flags+0x60/0x60 [lkdtm]
> > Aug 23 19:30:27 xpfo kernel: [   38.311680]  rewind_stack_do_exit+0x17/0x20
> > 
> > do_exit() expects to be called in a well-defined environment, so let's
> > re-enable interrupts after unwinding the stack, in case they were disabled.
> > 
> > Note that if any spinlocks are held, etc. we'll also get the above warning,
> > so this isn't a silver bullet. So, let's add a C helper in case someone
> > wants to add fancier lock busting or if we've forgotten to unwind something
> > else.
> > 
> > I've had to add back in the hack that Josh removed in 8c1f75587a18
> > ("x86/entry/64: Add unwind hint annotations") with the loop after the call,
> > because for whatever reason without that I get a warning:
> > 
> >   AS      arch/x86/entry/entry_64.o
> > arch/x86/entry/entry_64.o: warning: objtool: .entry.text: unexpected end of section
> > 
> > It seems to actually work fine for me though, since the new helper is also
> > __noreturn. Perhaps there's a better way to do this?
> 
> Unfortunately, objtool doesn't have a way to detect noreturn functions
> in other objects, so they're hard-coded in a list.  You can add
> __finish_rewind_stack_do_exit to the global_noreturns array in
> tools/objtool/check.c.

Awesome, this is the magic I was missing. Thanks!

Tycho