Virtually mapped stacks with guard pages (x86, core)

On Wed, Jun 15, 2016 at 05:28:26PM -0700, Andy Lutomirski wrote:
...

Quoted 4 lines

kernel/fork.c +0 -0

@@ -225,7 +225,8 @@ static void account_kernel_stack(struct thread_info *ti, int account)
 {
      struct zone *zone = page_zone(virt_to_page(ti));
 

kernel/fork.c static void account_kernel_stack(struct thread_info *ti, int account) · line 229

Quoted 3 lines

kernel/fork.c +2 -1

-     mod_zone_page_state(zone, NR_KERNEL_STACK, account);
+     mod_zone_page_state(zone, NR_KERNEL_STACK,
+                         THREAD_SIZE / PAGE_SIZE * account);

It won't work if THREAD_SIZE < PAGE_SIZE. Is there an arch with such a thread size, anyway? If no, we should probably drop thread_info_cache.

Quoted 6 lines · Vladimir Davydov

On Wed, Jun 15, 2016 at 05:28:26PM -0700, Andy Lutomirski wrote:
...

Quoted 4 lines

kernel/fork.c +0 -0

@@ -225,7 +225,8 @@ static void account_kernel_stack(struct thread_info *ti, int account)
 {
      struct zone *zone = page_zone(virt_to_page(ti));
 

Quoted 6 lines

kernel/fork.c static void account_kernel_stack(struct thread_info *ti, int account) · line 229

Quoted 3 lines

kernel/fork.c +2 -1

-     mod_zone_page_state(zone, NR_KERNEL_STACK, account);
+     mod_zone_page_state(zone, NR_KERNEL_STACK,
+                         THREAD_SIZE / PAGE_SIZE * account);

It won't work if THREAD_SIZE < PAGE_SIZE. Is there an arch with such a thread size, anyway? If no, we should probably drop thread_info_cache.

On a quick grep, I can't find any. I'll add a BUILD_BUG_ON for now.

--Andy

Currently, NR_KERNEL_STACK tracks the number of kernel stacks in a zone. This only makes sense if each kernel stack exists entirely in one zone, and allowing vmapped stacks could break this assumption.

It turns out that the code for tracking kernel stack allocations in units of pages is slightly simpler, so just switch to counting pages.

Cc Vladimir Davydov <[email protected]>

Cc Johannes Weiner <[email protected]>

Cc Michal Hocko <[email protected]>

Cc [email protected]

Signed-off-by Andy Lutomirski <[email protected]>

---
 fs/proc/meminfo.c | 2 +-
 kernel/fork.c     | 3 ++-
 mm/page_alloc.c   | 3 +--
 3 files changed, 4 insertions(+), 4 deletions(-)

Quoted 13 lines · Andy Lutomirski

Currently, NR_KERNEL_STACK tracks the number of kernel stacks in a zone. This only makes sense if each kernel stack exists entirely in one zone, and allowing vmapped stacks could break this assumption.

It turns out that the code for tracking kernel stack allocations in units of pages is slightly simpler, so just switch to counting pages.

Cc Vladimir Davydov <[email protected]>

Cc Johannes Weiner <[email protected]>

Cc Michal Hocko <[email protected]>

Cc [email protected]

Signed-off-by Andy Lutomirski <[email protected]>

Quoted 5 lines

---
 fs/proc/meminfo.c | 2 +-
 kernel/fork.c     | 3 ++-
 mm/page_alloc.c   | 3 +--
 3 files changed, 4 insertions(+), 4 deletions(-)

You missed another usage of NR_KERNEL_STACK in drivers/base/node.c.

Quoted 20 lines · Josh Poimboeuf

Quoted 13 lines · Andy Lutomirski

Currently, NR_KERNEL_STACK tracks the number of kernel stacks in a zone. This only makes sense if each kernel stack exists entirely in one zone, and allowing vmapped stacks could break this assumption.

It turns out that the code for tracking kernel stack allocations in units of pages is slightly simpler, so just switch to counting pages.

Cc Vladimir Davydov <[email protected]>

Cc Johannes Weiner <[email protected]>

Cc Michal Hocko <[email protected]>

Cc [email protected]

Signed-off-by Andy Lutomirski <[email protected]>

Quoted 5 lines

---
 fs/proc/meminfo.c | 2 +-
 kernel/fork.c     | 3 ++-
 mm/page_alloc.c   | 3 +--
 3 files changed, 4 insertions(+), 4 deletions(-)

Quoted 1 line

You missed another usage of NR_KERNEL_STACK in drivers/base/node.c.

Thanks.

The real reason I cc'd you was so you could look at rewind_stack_do_exit and the sneaky trick I did in no_context in the last patch, though. :) Both survive objtool, but I figured I'd check with objtool's author as well. If there was a taint bit I could set saying "kernel is hosed -- don't try to apply live patches any more", I'd have extra confidence.

If CONFIG_VMAP_STACK is selected, kernel stacks are allocated with vmalloc_node.

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/Kconfig  | 12 ++++++++++++
 kernel/fork.c | 45 +++++++++++++++++++++++++++++++++++++--------
 2 files changed, 49 insertions(+), 8 deletions(-)

arch/Kconfig +0 -0

index d794384a0404..1acd262036b0 100644

Quoted 17 lines · Andy Lutomirski

If CONFIG_VMAP_STACK is selected, kernel stacks are allocated with vmalloc_node.

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/Kconfig  | 12 ++++++++++++
 kernel/fork.c | 45 +++++++++++++++++++++++++++++++++++++--------
 2 files changed, 49 insertions(+), 8 deletions(-)

arch/Kconfig +0 -0

index d794384a0404..1acd262036b0 100644
@@ -658,4 +658,16 @@ config ARCH_NO_COHERENT_DMA_MMAP
 config CPU_NO_EFFICIENT_FFS
        def_bool n
 

arch/Kconfig config ARCH_NO_COHERENT_DMA_MMAP · line 662

Quoted 2 lines

arch/Kconfig +2 -0

+config HAVE_ARCH_VMAP_STACK
+       def_bool n

In the style of HAVE_ARCH_SECCOMP_FILTER, can you detail all the things an architecture needs to do to correctly support a vmap stack? This will help with arch porting.

If we call do_exit with a clean stack, we greatly reduce the risk of recursive oopses due to stack overflow in do_exit, and we allow do_exit to work even if we OOPS from an IST stack. The latter gives us a much better chance of surviving long enough after we detect a stack overflow to write out our logs.

I intentionally separated this from the preceding patch that disables do_exit-on-OOPS on IST stacks. This way, if we need to revert this patch, we still end up in an acceptable state wrt stack overflow handling.

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/entry/entry_32.S   | 11 +++++++++++
 arch/x86/entry/entry_64.S   | 11 +++++++++++
 arch/x86/kernel/dumpstack.c | 13 +++++++++----
 3 files changed, 31 insertions(+), 4 deletions(-)

arch/x86/entry/entry_32.S +11 -0

index 983e5d3a0d27..1499db695a88 100644
@@ -1153,3 +1153,14 @@ ENTRY(async_page_fault)
 	jmp	error_code
 END(async_page_fault)
 #endif
+
+ENTRY(rewind_stack_do_exit)
+	/* Prevent any naive code from trying to unwind to our caller. */
+	xorl	%ebp, %ebp
+
+	movl	PER_CPU_VAR(cpu_current_top_of_stack), %esi
+	leal	-TOP_OF_KERNEL_STACK_PADDING-PT_OLDSS(%esi), %esp
+
+	call	do_exit
+1:	jmp 1b
+END(rewind_stack_do_exit)

arch/x86/entry/entry_64.S +0 -0

index 9ee0da1807ed..394cad73e890 100644

Quoted 42 lines · Andy Lutomirski

If we call do_exit with a clean stack, we greatly reduce the risk of recursive oopses due to stack overflow in do_exit, and we allow do_exit to work even if we OOPS from an IST stack. The latter gives us a much better chance of surviving long enough after we detect a stack overflow to write out our logs.

I intentionally separated this from the preceding patch that disables do_exit-on-OOPS on IST stacks. This way, if we need to revert this patch, we still end up in an acceptable state wrt stack overflow handling.

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/entry/entry_32.S   | 11 +++++++++++
 arch/x86/entry/entry_64.S   | 11 +++++++++++
 arch/x86/kernel/dumpstack.c | 13 +++++++++----
 3 files changed, 31 insertions(+), 4 deletions(-)

arch/x86/entry/entry_32.S +11 -0

index 983e5d3a0d27..1499db695a88 100644
@@ -1153,3 +1153,14 @@ ENTRY(async_page_fault)
      jmp     error_code
 END(async_page_fault)
 #endif
+
+ENTRY(rewind_stack_do_exit)
+     /* Prevent any naive code from trying to unwind to our caller. */
+     xorl    %ebp, %ebp
+
+     movl    PER_CPU_VAR(cpu_current_top_of_stack), %esi
+     leal    -TOP_OF_KERNEL_STACK_PADDING-PT_OLDSS(%esi), %esp
+
+     call    do_exit
+1:   jmp 1b
+END(rewind_stack_do_exit)

arch/x86/entry/entry_64.S +0 -0

index 9ee0da1807ed..394cad73e890 100644

arch/x86/entry/entry_64.S ENTRY(ignore_sysret) · line 1432

Quoted 10 lines

arch/x86/entry/entry_64.S +7 -0

@@ -1423,3 +1423,14 @@ ENTRY(ignore_sysret)
      mov     $-ENOSYS, %eax
      sysret
 END(ignore_sysret)
+
+ENTRY(rewind_stack_do_exit)
+     /* Prevent any naive code from trying to unwind to our caller. */
+     xorl    %ebp, %ebp
+
+     movq    PER_CPU_VAR(cpu_current_top_of_stack), %rax
+     leaq    -TOP_OF_KERNEL_STACK_PADDING-SS(%rax), %rsp

I think this should be:

leaq -TOP_OF_KERNEL_STACK_PADDING-SIZEOF_PTREGS, %rsp

That way when it calls do_exit(), the stack frame will be placed at the conventional spot where a smart unwinder would expect to find it.

The comment suggests that show_stack(NULL, NULL) should backtrace the current context, but the code doesn't match the comment. If regs are given, start the "Stack:" hexdump at regs->sp.

Signed-off-by Andy Lutomirski <[email protected]>

If we overflow the stack, print_context_stack will abort. Detect this case and rewind back into the valid part of the stack so that we can trace it.

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/kernel/dumpstack.c | 7 +++++++
 1 file changed, 7 insertions(+)

arch/x86/kernel/dumpstack.c +0 -0

index d4d085e27d04..400a2e17c1d1 100644

arch/x86/kernel/dumpstack.c +1 -0

+
 	while (valid_stack_ptr(tinfo, stack, sizeof(*stack), end)) {
 		unsigned long addr;

Quoted 14 lines · Andy Lutomirski

If we overflow the stack, print_context_stack will abort. Detect this case and rewind back into the valid part of the stack so that we can trace it.

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/kernel/dumpstack.c | 7 +++++++
 1 file changed, 7 insertions(+)

arch/x86/kernel/dumpstack.c +0 -0

index d4d085e27d04..400a2e17c1d1 100644

arch/x86/kernel/dumpstack.c print_context_stack(struct thread_info *tinfo, · line 108

Quoted 9 lines

arch/x86/kernel/dumpstack.c +6 -0

@@ -100,6 +100,13 @@ print_context_stack(struct thread_info *tinfo,
 {
      struct stack_frame *frame = (struct stack_frame *)bp;
 
+     /*
+      * If we overflowed the stack into a guard page, jump back to the
+      * bottom of the usable stack.
+      */
+     if ((unsigned long)tinfo - (unsigned long)stack < PAGE_SIZE)
+             stack = (unsigned long *)tinfo + 1;

That will start walking the stack in the middle of the thread_info struct.

I think you meant:

stack = (unsigned long *)(tinfo + 1)

However, thread_info will have been overwritten anyway. So maybe it should just be:

                stack = tinfo;

(Though that still wouldn't quite work because the valid_stack_ptr() check would fail...)

Quoted 35 lines · Josh Poimboeuf

Quoted 14 lines · Andy Lutomirski

If we overflow the stack, print_context_stack will abort. Detect this case and rewind back into the valid part of the stack so that we can trace it.

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/kernel/dumpstack.c | 7 +++++++
 1 file changed, 7 insertions(+)

arch/x86/kernel/dumpstack.c +0 -0

index d4d085e27d04..400a2e17c1d1 100644

arch/x86/kernel/dumpstack.c print_context_stack(struct thread_info *tinfo, · line 108

Quoted 9 lines

arch/x86/kernel/dumpstack.c +6 -0

@@ -100,6 +100,13 @@ print_context_stack(struct thread_info *tinfo,
 {
      struct stack_frame *frame = (struct stack_frame *)bp;
 
+     /*
+      * If we overflowed the stack into a guard page, jump back to the
+      * bottom of the usable stack.
+      */
+     if ((unsigned long)tinfo - (unsigned long)stack < PAGE_SIZE)
+             stack = (unsigned long *)tinfo + 1;

That will start walking the stack in the middle of the thread_info struct.

I think you meant:

stack = (unsigned long *)(tinfo + 1)

However, thread_info will have been overwritten anyway. So maybe it should just be:

Quoted 4 lines

                stack = tinfo;

(Though that still wouldn't quite work because the valid_stack_ptr() check would fail...)

I did mean what I wrote, because I wanted to start at the bottom of the validly allocated area. IOW I wanted to do the minimum possible backward jump to make the code display something.

Quoted 41 lines · Andy Lutomirski

Quoted 39 lines · Josh Poimboeuf

Quoted 14 lines · Andy Lutomirski

If we overflow the stack, print_context_stack will abort. Detect this case and rewind back into the valid part of the stack so that we can trace it.

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/kernel/dumpstack.c | 7 +++++++
 1 file changed, 7 insertions(+)

arch/x86/kernel/dumpstack.c +0 -0

index d4d085e27d04..400a2e17c1d1 100644

arch/x86/kernel/dumpstack.c print_context_stack(struct thread_info *tinfo, · line 108

Quoted 9 lines

arch/x86/kernel/dumpstack.c +6 -0

@@ -100,6 +100,13 @@ print_context_stack(struct thread_info *tinfo,
 {
      struct stack_frame *frame = (struct stack_frame *)bp;
 
+     /*
+      * If we overflowed the stack into a guard page, jump back to the
+      * bottom of the usable stack.
+      */
+     if ((unsigned long)tinfo - (unsigned long)stack < PAGE_SIZE)
+             stack = (unsigned long *)tinfo + 1;

That will start walking the stack in the middle of the thread_info struct.

I think you meant:

stack = (unsigned long *)(tinfo + 1)

However, thread_info will have been overwritten anyway. So maybe it should just be:

                stack = tinfo;

(Though that still wouldn't quite work because the valid_stack_ptr() check would fail...)

Quoted 3 lines

I did mean what I wrote, because I wanted to start at the bottom of the validly allocated area. IOW I wanted to do the minimum possible backward jump to make the code display something.

But why the "+ 1"? Is that a hack to make it pass the valid_stack_ptr() check?

Quoted 42 lines · Josh Poimboeuf

Quoted 41 lines · Andy Lutomirski

Quoted 39 lines · Josh Poimboeuf

Quoted 14 lines · Andy Lutomirski

If we overflow the stack, print_context_stack will abort. Detect this case and rewind back into the valid part of the stack so that we can trace it.

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/kernel/dumpstack.c | 7 +++++++
 1 file changed, 7 insertions(+)

arch/x86/kernel/dumpstack.c +0 -0

index d4d085e27d04..400a2e17c1d1 100644

arch/x86/kernel/dumpstack.c print_context_stack(struct thread_info *tinfo, · line 108

Quoted 9 lines

arch/x86/kernel/dumpstack.c +6 -0

@@ -100,6 +100,13 @@ print_context_stack(struct thread_info *tinfo,
 {
      struct stack_frame *frame = (struct stack_frame *)bp;
 
+     /*
+      * If we overflowed the stack into a guard page, jump back to the
+      * bottom of the usable stack.
+      */
+     if ((unsigned long)tinfo - (unsigned long)stack < PAGE_SIZE)
+             stack = (unsigned long *)tinfo + 1;

That will start walking the stack in the middle of the thread_info struct.

I think you meant:

stack = (unsigned long *)(tinfo + 1)

However, thread_info will have been overwritten anyway. So maybe it should just be:

                stack = tinfo;

(Though that still wouldn't quite work because the valid_stack_ptr() check would fail...)

Quoted 6 lines

Quoted 3 lines

I did mean what I wrote, because I wanted to start at the bottom of the validly allocated area. IOW I wanted to do the minimum possible backward jump to make the code display something.

But why the "+ 1"? Is that a hack to make it pass the valid_stack_ptr() check?

Yes.

But hmm. Maybe the right fix is to drop the + 1 and to change the last line of valid_stck_ptr from:

    return p > t && p < t + THREAD_SIZE - size;

to:

    return p >= t && p < t + THREAD_SIZE - size;

This allows x86_64 kernels to enable vmapped stacks. There are a couple of interesting bits.

First, x86 lazily faults in top-level paging entries for the vmalloc area. This won't work if we get a page fault while trying to access the stack: the CPU will promote it to a double-fault and we'll die. To avoid this problem, probe the new stack when switching stacks and forcibly populate the pgd entry for the stack when switching mms.

Second, once we have guard pages around the stack, we'll want to detect and handle stack overflow.

I didn't enable it on x86_32. We'd need to rework the double-fault code a bit and I'm concerned about running out of vmalloc virtual addresses under some workloads.

This patch, by itself, will behave somewhat erratically when the stack overflows while RSP is still more than a few tens of bytes above the bottom of the stack. Specifically, we'll get #PF and make it to no_context and an oops without triggering a double-fault, and no_context doesn't know about stack overflows. The next patch will improve that case.

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/Kconfig                 |  1 +
 arch/x86/include/asm/switch_to.h | 28 +++++++++++++++++++++++++++-
 arch/x86/kernel/traps.c          | 32 ++++++++++++++++++++++++++++++++
 arch/x86/mm/tlb.c                | 15 +++++++++++++++
 4 files changed, 75 insertions(+), 1 deletion(-)

arch/x86/Kconfig +1 -0

index 0a7b885964ba..b624b24d1dc1 100644
@@ -92,6 +92,7 @@ config X86
 	select HAVE_ARCH_TRACEHOOK
 	select HAVE_ARCH_TRANSPARENT_HUGEPAGE
 	select HAVE_EBPF_JIT			if X86_64
+	select HAVE_ARCH_VMAP_STACK		if X86_64
 	select HAVE_CC_STACKPROTECTOR
 	select HAVE_CMPXCHG_DOUBLE
 	select HAVE_CMPXCHG_LOCAL

arch/x86/include/asm/switch_to.h +27 -1

index 8f321a1b03a1..14e4b20f0aaf 100644
@@ -8,6 +8,28 @@ struct tss_struct;
 void __switch_to_xtra(struct task_struct *prev_p, struct task_struct *next_p,
 		      struct tss_struct *tss);
 
+/* This runs runs on the previous thread's stack. */
+static inline void prepare_switch_to(struct task_struct *prev,
+				     struct task_struct *next)
+{
+#ifdef CONFIG_VMAP_STACK
+	/*
+	 * If we switch to a stack that has a top-level paging entry
+	 * that is not present in the current mm, the resulting #PF will
+	 * will be promoted to a double-fault and we'll panic.  Probe
+	 * the new stack now so that vmalloc_fault can fix up the page
+	 * tables if needed.  This can only happen if we use a stack
+	 * in vmap space.
+	 *
+	 * We assume that the stack is aligned so that it never spans
+	 * more than one top-level paging entry.
+	 *
+	 * To minimize cache pollution, just follow the stack pointer.
+	 */
+	READ_ONCE(*(unsigned char *)next->thread.sp);
+#endif
+}
+
 #ifdef CONFIG_X86_32
 
 #ifdef CONFIG_CC_STACKPROTECTOR

@@ -39,6 +61,8 @@ do {									\
 	 */								\
 	unsigned long ebx, ecx, edx, esi, edi;				\
 									\
+	prepare_switch_to(prev, next);					\
+									\
 	asm volatile("pushl %%ebp\n\t"		/* save    EBP   */	\
 		     "movl %%esp,%[prev_sp]\n\t"	/* save    ESP   */ \
 		     "movl %[next_sp],%%esp\n\t"	/* restore ESP   */ \

@@ -103,7 +127,9 @@ do {									\
  * clean in kernel mode, with the possible exception of IOPL.  Kernel IOPL
  * has no effect.
  */
-#define switch_to(prev, next, last) \
+#define switch_to(prev, next, last)					  \
+	prepare_switch_to(prev, next);					  \
+									  \
 	asm volatile(SAVE_CONTEXT					  \
 	     "movq %%rsp,%P[threadrsp](%[prev])\n\t" /* save RSP */	  \
 	     "movq %P[threadrsp](%[next]),%%rsp\n\t" /* restore RSP */	  \

arch/x86/kernel/traps.c +32 -0

index 00f03d82e69a..9cb7ea781176 100644
@@ -292,12 +292,30 @@ DO_ERROR(X86_TRAP_NP,     SIGBUS,  "segment not present",	segment_not_present)
 DO_ERROR(X86_TRAP_SS,     SIGBUS,  "stack segment",		stack_segment)
 DO_ERROR(X86_TRAP_AC,     SIGBUS,  "alignment check",		alignment_check)
 
+#ifdef CONFIG_VMAP_STACK
+static void __noreturn handle_stack_overflow(const char *message,
+					     struct pt_regs *regs,
+					     unsigned long fault_address)
+{
+	printk(KERN_EMERG "BUG: stack guard page was hit at %p (stack is %p..%p)\n",
+		 (void *)fault_address, current->stack,
+		 (char *)current->stack + THREAD_SIZE - 1);
+	die(message, regs, 0);
+
+	/* Be absolutely certain we don't return. */
+	panic(message);
+}
+#endif
+
 #ifdef CONFIG_X86_64
 /* Runs on IST stack */
 dotraplinkage void do_double_fault(struct pt_regs *regs, long error_code)
 {
 	static const char str[] = "double fault";
 	struct task_struct *tsk = current;
+#ifdef CONFIG_VMAP_STACK
+	unsigned long cr2;
+#endif
 
 #ifdef CONFIG_X86_ESPFIX64
 	extern unsigned char native_irq_return_iret[];

@@ -332,6 +350,20 @@ dotraplinkage void do_double_fault(struct pt_regs *regs, long error_code)
 	tsk->thread.error_code = error_code;
 	tsk->thread.trap_nr = X86_TRAP_DF;
 
+#ifdef CONFIG_VMAP_STACK
+	/*
+	 * If we overflow the stack into a guard page, the CPU will fail
+	 * to deliver #PF and will send #DF instead.  CR2 will contain
+	 * the linear address of the second fault, which will be in the
+	 * guard page below the bottom of the stack.
+	 */
+	cr2 = read_cr2();
+	if ((unsigned long)tsk->stack - 1 - cr2 < PAGE_SIZE)
+		handle_stack_overflow(
+			"kernel stack overflow (double-fault)",
+			regs, cr2);
+#endif
+
 #ifdef CONFIG_DOUBLEFAULT
 	df_debug(regs, error_code);
 #endif

arch/x86/mm/tlb.c +0 -0

index 5643fd0b1a7d..fbf036ae72ac 100644
@@ -77,10 +77,25 @@ void switch_mm_irqs_off(struct mm_struct *prev, struct mm_struct *next,
 	unsigned cpu = smp_processor_id();

arch/x86/mm/tlb.c +4 -0

+			pgd_t *pgd = next->pgd + stack_pgd_index;
+
+			if (unlikely(pgd_none(*pgd)))
+				set_pgd(pgd, init_mm.pgd[stack_pgd_index]);

Hi,

Quoted 7 lines

arch/x86/mm/tlb.c +0 -0

index 5643fd0b1a7d..fbf036ae72ac 100644
@@ -77,10 +77,25 @@ void switch_mm_irqs_off(struct mm_struct *prev, struct mm_struct *next,
      unsigned cpu = smp_processor_id();
 

arch/x86/mm/tlb.c void switch_mm_irqs_off(struct mm_struct *prev, struct mm_struct *next, · line 87

Quoted 9 lines

arch/x86/mm/tlb.c +8 -0

      if (likely(prev != next)) {
+             if (IS_ENABLED(CONFIG_VMAP_STACK)) {
+                     /*
+                      * If our current stack is in vmalloc space and isn't
+                      * mapped in the new pgd, we'll double-fault.  Forcibly
+                      * map it.
+                      */
+                     unsigned int stack_pgd_index =
+                             pgd_index(current_stack_pointer());

stack pointer is still the previous task's, current_stack_pointer() returns that, not next task's which was intention I guess. Things may happen to work if on same pgd, but at least the boot cpu init_task_struct is special.

Quoted 14 lines

Quoted 14 lines

+++ b/arch/x86/mm/tlb.c

arch/x86/mm/tlb.c +3 -0

@@ -77,10 +77,25 @@ void switch_mm_irqs_off(struct mm_struct
 *prev, struct mm_struct *next,
       unsigned cpu = smp_processor_id();
 
       if (likely(prev != next)) {
+             if (IS_ENABLED(CONFIG_VMAP_STACK)) {
+                     /*
+                      * If our current stack is in vmalloc space

and isn't
+                      * mapped in the new pgd, we'll double- fault.  Forcibly
+                      * map it.
+                      */

Quoted 8 lines

Quoted 2 lines

+                     unsigned int stack_pgd_index =
+                             pgd_index(current_stack_pointer());

stack pointer is still the previous task's, current_stack_pointer() returns that, not
next task's which was intention I guess. Things may happen to work if on same pgd, but at least the
boot cpu init_task_struct is special.

This is intentional.  When switching processes, we first switch the mm
and then switch the task.  We need to make sure that the prev stack is
mapped in the new mm or we'll double-fault and die after switching the
mm which still trying to execute on the old stack.

The change to switch_to makes sure that the new stack is mapped.

Since the dawn of time, a kernel stack overflow has been a real PITA to debug, has caused nondeterministic crashes some time after the actual overflow, and has generally been easy to exploit for root.

With this series, arches can enable HAVE_ARCH_VMAP_STACK. Arches that enable it (just x86 for now) get virtually mapped stacks with guard pages. This causes reliable faults when the stack overflows.

If the arch implements it well, we get a nice OOPS on stack overflow (as opposed to panicing directly or otherwise exploding badly). On x86, the OOPS is nice, has a usable call trace, and the overflowing task is killed cleanly.

Quoted 8 lines · Andy Lutomirski

Since the dawn of time, a kernel stack overflow has been a real PITA to debug, has caused nondeterministic crashes some time after the actual overflow, and has generally been easy to exploit for root.

With this series, arches can enable HAVE_ARCH_VMAP_STACK. Arches that enable it (just x86 for now) get virtually mapped stacks with guard pages. This causes reliable faults when the stack overflows.

Quoted 4 lines

If the arch implements it well, we get a nice OOPS on stack overflow (as opposed to panicing directly or otherwise exploding badly). On x86, the OOPS is nice, has a usable call trace, and the overflowing task is killed cleanly.

Do you have numbers which reflect the performance impact of this change?

Quoted 14 lines · Heiko Carstens

Quoted 12 lines · Andy Lutomirski

Since the dawn of time, a kernel stack overflow has been a real PITA to debug, has caused nondeterministic crashes some time after the actual overflow, and has generally been easy to exploit for root.

With this series, arches can enable HAVE_ARCH_VMAP_STACK. Arches that enable it (just x86 for now) get virtually mapped stacks with guard pages. This causes reliable faults when the stack overflows.

If the arch implements it well, we get a nice OOPS on stack overflow (as opposed to panicing directly or otherwise exploding badly). On x86, the OOPS is nice, has a usable call trace, and the overflowing task is killed cleanly.

Quoted 2 lines

Do you have numbers which reflect the performance impact of this change?

Hmm. My attempt to benchmark it caused some of the vmalloc core code to hang. I'll dig around.

Adding Paul, because RCU blew up.

Quoted 16 lines · Andy Lutomirski

Quoted 14 lines · Heiko Carstens

Quoted 12 lines · Andy Lutomirski

Since the dawn of time, a kernel stack overflow has been a real PITA to debug, has caused nondeterministic crashes some time after the actual overflow, and has generally been easy to exploit for root.

With this series, arches can enable HAVE_ARCH_VMAP_STACK. Arches that enable it (just x86 for now) get virtually mapped stacks with guard pages. This causes reliable faults when the stack overflows.

If the arch implements it well, we get a nice OOPS on stack overflow (as opposed to panicing directly or otherwise exploding badly). On x86, the OOPS is nice, has a usable call trace, and the overflowing task is killed cleanly.

Quoted 5 lines

Quoted 2 lines

Do you have numbers which reflect the performance impact of this change?

Hmm. My attempt to benchmark it caused some of the vmalloc core code to hang. I'll dig around.

[  488.482010] Call Trace:
[  488.482389]  <IRQ>  [<ffffffff810da5f6>] sched_show_task+0xb6/0x110
[  488.483341]  [<ffffffff81108c7a>] rcu_check_callbacks+0x83a/0x840
[  488.484226]  [<ffffffff810dd48a>] ? account_system_time+0x7a/0x110
[  488.485157]  [<ffffffff8111c0f0>] ? tick_sched_do_timer+0x30/0x30
[  488.486133]  [<ffffffff8110d314>] update_process_times+0x34/0x60
[  488.487050]  [<ffffffff8111bb51>] tick_sched_handle.isra.13+0x31/0x40
[  488.488018]  [<ffffffff8111c128>] tick_sched_timer+0x38/0x70
[  488.488853]  [<ffffffff8110db2a>] __hrtimer_run_queues+0xda/0x250
[  488.489739]  [<ffffffff8110e263>] hrtimer_interrupt+0xa3/0x190
[  488.490630]  [<ffffffff810952f3>] local_apic_timer_interrupt+0x33/0x50
[  488.491660]  [<ffffffff81095d58>] smp_apic_timer_interrupt+0x38/0x50
[  488.492644]  [<ffffffff8194d022>] apic_timer_interrupt+0x82/0x90
[  488.493502]  [<ffffffff810ee1c0>] ? queued_spin_lock_slowpath+0x20/0x190
[  488.494550]  [<ffffffff8194c21b>] _raw_spin_lock+0x1b/0x20
[  488.495321]  [<ffffffff811b7a54>] find_vmap_area+0x14/0x60
[  488.496197]  [<ffffffff811b8f69>] find_vm_area+0x9/0x20
[  488.496922]  [<ffffffff810afb19>] account_kernel_stack+0x89/0x100
[  488.497885]  [<ffffffff810aff76>] free_task+0x16/0x50
[  488.498599]  [<ffffffff810b0042>] __put_task_struct+0x92/0x120
[  488.499525]  [<ffffffff810b4a66>] delayed_put_task_struct+0x76/0x80
[  488.500348]  [<ffffffff81107969>] rcu_process_callbacks+0x1f9/0x5e0
[  488.501208]  [<ffffffff810b7ca1>] __do_softirq+0xf1/0x280
[  488.501932]  [<ffffffff810b7f7e>] irq_exit+0x9e/0xa0
[  488.502955]  [<ffffffff81095d5d>] smp_apic_timer_interrupt+0x3d/0x50
[  488.503943]  [<ffffffff8194d022>] apic_timer_interrupt+0x82/0x90
[  488.504886]  <EOI>  [<ffffffff8194c20b>] ? _raw_spin_lock+0xb/0x20
[  488.505877]  [<ffffffff811b7c33>] ? __get_vm_area_node+0xc3/0x160
[  488.506812]  [<ffffffff810e013e>] ? task_move_group_fair+0x7e/0x90
[  488.507730]  [<ffffffff811b9360>] __vmalloc_node_range+0x70/0x280
[  488.508689]  [<ffffffff810b1ce5>] ? _do_fork+0xc5/0x370
[  488.509512]  [<ffffffff811ceb9b>] ? kmem_cache_alloc_node+0x7b/0x170
[  488.510502]  [<ffffffff81327418>] ? current_has_perm+0x38/0x40
[  488.511430]  [<ffffffff810b0501>] copy_process.part.46+0x141/0x1760
[  488.512449]  [<ffffffff810b1ce5>] ? _do_fork+0xc5/0x370
[  488.513285]  [<ffffffff8111faf3>] ? do_futex+0x293/0xad0
[  488.514093]  [<ffffffff810df14a>] ? check_preempt_wakeup+0x10a/0x240
[  488.515108]  [<ffffffff810d92c2>] ? wake_up_new_task+0xf2/0x180
[  488.516043]  [<ffffffff810b1ce5>] _do_fork+0xc5/0x370
[  488.516786]  [<ffffffff8112039d>] ? SyS_futex+0x6d/0x150
[  488.517615]  [<ffffffff810b2014>] SyS_clone+0x14/0x20
[  488.518385]  [<ffffffff81002b72>] do_syscall_64+0x52/0xb0
[  488.519239]  [<ffffffff8194c4e1>] entry_SYSCALL64_slow_path+0x25/0x25

The bug seems straightforward: vmap_area_lock is held, the RCU softirq fires, and vmap_area_lock recurses and deadlocks. Lockdep agrees with my assessment and catches the bug immediately on boot.

What's the right fix? Change all spin_lock calls on vmap_area_lock to spin_lock_bh? Somehow ask RCU not to call delayed_put_task_struct from bh context? I would naively have expected RCU to only call its callbacks from thread context, but I was clearly wrong.

Quoted 8 lines · Andy Lutomirski

Since the dawn of time, a kernel stack overflow has been a real PITA to debug, has caused nondeterministic crashes some time after the actual overflow, and has generally been easy to exploit for root.

With this series, arches can enable HAVE_ARCH_VMAP_STACK. Arches that enable it (just x86 for now) get virtually mapped stacks with guard pages. This causes reliable faults when the stack overflows.

Quoted 4 lines

If the arch implements it well, we get a nice OOPS on stack overflow (as opposed to panicing directly or otherwise exploding badly). On x86, the OOPS is nice, has a usable call trace, and the overflowing task is killed cleanly.

Do you have numbers which reflect the performance impact of this change?

Quoted 9 lines · Heiko Carstens

Quoted 8 lines · Andy Lutomirski

Since the dawn of time, a kernel stack overflow has been a real PITA to debug, has caused nondeterministic crashes some time after the actual overflow, and has generally been easy to exploit for root.

With this series, arches can enable HAVE_ARCH_VMAP_STACK. Arches that enable it (just x86 for now) get virtually mapped stacks with guard pages. This causes reliable faults when the stack overflows.

Quoted 7 lines

Quoted 4 lines

If the arch implements it well, we get a nice OOPS on stack overflow (as opposed to panicing directly or otherwise exploding badly). On x86, the OOPS is nice, has a usable call trace, and the overflowing task is killed cleanly.

Do you have numbers which reflect the performance impact of this change?

It seems to add ~1.5µs per thread creation/join pair, which is around 15% overhead. I *think* the major cost is that vmalloc calls alloc_kmem_pages_node once per page rather than using a higher-order block if available.

Anyway, if anyone wants this to become faster, I think the way to do it would be to ask some friendly mm folks to see if they can speed up vmalloc. I don't really want to dig in to the guts of the page allocator. My instinct would be to add a new interface (GFP_SMALLER_OK?) to ask the page allocator for a high-order allocation such that, if a high-order block is not immediately available (on the freelist) then it should fall back to a smaller allocation rather than working hard to get a high-order allocation. Then vmalloc could use this, and vfree could free pages in blocks corresponding to whatever orders it got the pages in, thus avoiding the need to merge all the pages back together.

There's another speedup available: actually reuse allocations. We could keep a very small freelist of vmap_areas with their associated pages so we could reuse them. (We can't efficiently reuse a vmap_area without its backing pages because we need to flush the TLB in the middle if we do that.)

Quoted 36 lines · Andy Lutomirski

Quoted 9 lines · Heiko Carstens

Quoted 8 lines · Andy Lutomirski

Since the dawn of time, a kernel stack overflow has been a real PITA to debug, has caused nondeterministic crashes some time after the actual overflow, and has generally been easy to exploit for root.

With this series, arches can enable HAVE_ARCH_VMAP_STACK. Arches that enable it (just x86 for now) get virtually mapped stacks with guard pages. This causes reliable faults when the stack overflows.

Quoted 7 lines

Quoted 4 lines

If the arch implements it well, we get a nice OOPS on stack overflow (as opposed to panicing directly or otherwise exploding badly). On x86, the OOPS is nice, has a usable call trace, and the overflowing task is killed cleanly.

Do you have numbers which reflect the performance impact of this change?

It seems to add ~1.5盜 per thread creation/join pair, which is around 15% overhead. I *think* the major cost is that vmalloc calls alloc_kmem_pages_node once per page rather than using a higher-order block if available.

Anyway, if anyone wants this to become faster, I think the way to do it would be to ask some friendly mm folks to see if they can speed up vmalloc. I don't really want to dig in to the guts of the page allocator. My instinct would be to add a new interface (GFP_SMALLER_OK?) to ask the page allocator for a high-order allocation such that, if a high-order block is not immediately available (on the freelist) then it should fall back to a smaller allocation rather than working hard to get a high-order allocation. Then vmalloc could use this, and vfree could free pages in blocks corresponding to whatever orders it got the pages in, thus avoiding the need to merge all the pages back together.

Quoted 5 lines

There's another speedup available: actually reuse allocations. We could keep a very small freelist of vmap_areas with their associated pages so we could reuse them. (We can't efficiently reuse a vmap_area without its backing pages because we need to flush the TLB in the middle if we do that.)

That's rather expensive. Just for the records: on s390 we use gcc's architecture specific compile options (kernel: CONFIG_STACK_GUARD)

-mstack-guard=stack-guard
-mstack-size=stack-size

These generate two additional instructions at the beginning of each function prologue and verify that the stack size left won't be below a specified number of bytes. If so it would execute an illegal instruction.

A disassembly looks like this (r15 is the stackpointer):

0000000000000670 <setup_arch>:

     670:       eb 6f f0 48 00 24       stmg    %r6,%r15,72(%r15)
     676:       c0 d0 00 00 00 00       larl    %r13,676 <setup_arch+0x6>
     67c:       a7 f1 3f 80             tmll    %r15,16256  <--- test if enough space left
     680:       b9 04 00 ef             lgr     %r14,%r15
     684:       a7 84 00 01             je      686 <setup_arch+0x16> <--- branch to illegal op
     688:       e3 f0 ff 90 ff 71       lay     %r15,-112(%r15)

The branch jumps actually into the branch instruction itself since the 0001 part of the "je" instruction is an illegal instruction.

This catches at least wild stack overflows because of two many functions being called.

Of course it doesn't catch wild accesses outside the stack because e.g. the index into an array on the stack is wrong.

The runtime overhead is within noise ratio, therefore we have this always enabled.

Quoted 13 lines

A disassembly looks like this (r15 is the stackpointer):

0000000000000670 <setup_arch>:

     670:       eb 6f f0 48 00 24       stmg    %r6,%r15,72(%r15)
     676:       c0 d0 00 00 00 00       larl    %r13,676 <setup_arch+0x6>
     67c:       a7 f1 3f 80             tmll    %r15,16256  <--- test if enough space left
     680:       b9 04 00 ef             lgr     %r14,%r15
     684:       a7 84 00 01             je      686 <setup_arch+0x16> <--- branch to illegal op
     688:       e3 f0 ff 90 ff 71       lay     %r15,-112(%r15)

The branch jumps actually into the branch instruction itself since the 0001 part of the "je" instruction is an illegal instruction.

Quoted 9 lines

This catches at least wild stack overflows because of two many functions being called.

Of course it doesn't catch wild accesses outside the stack because e.g. the index into an array on the stack is wrong.

The runtime overhead is within noise ratio, therefore we have this always enabled.

Neat! What exactly does tmll do? I assume this works by checking the low bits of the stack pointer.

x86_64 would have to do:

movl %esp, %r11d
shll %r11d, $18
cmpl %r11d, <threshold>
jg error

Or similar. I think the cmpl could be eliminated if the threshold were a power of two by simply testing the low bits of the stack pointer.

Quoted 28 lines · Andy Lutomirski

Quoted 13 lines

A disassembly looks like this (r15 is the stackpointer):

0000000000000670 <setup_arch>:

     670:       eb 6f f0 48 00 24       stmg    %r6,%r15,72(%r15)
     676:       c0 d0 00 00 00 00       larl    %r13,676 <setup_arch+0x6>
     67c:       a7 f1 3f 80             tmll    %r15,16256  <--- test if enough space left
     680:       b9 04 00 ef             lgr     %r14,%r15
     684:       a7 84 00 01             je      686 <setup_arch+0x16> <--- branch to illegal op
     688:       e3 f0 ff 90 ff 71       lay     %r15,-112(%r15)

The branch jumps actually into the branch instruction itself since the 0001 part of the "je" instruction is an illegal instruction.

Quoted 9 lines

This catches at least wild stack overflows because of two many functions being called.

Of course it doesn't catch wild accesses outside the stack because e.g. the index into an array on the stack is wrong.

The runtime overhead is within noise ratio, therefore we have this always enabled.

Neat! What exactly does tmll do? I assume this works by checking the low bits of the stack pointer.

x86_64 would have to do:

Quoted 8 lines

movl %esp, %r11d
shll %r11d, $18
cmpl %r11d, <threshold>
jg error

Or similar. I think the cmpl could be eliminated if the threshold were a power of two by simply testing the low bits of the stack pointer.

The tmll instruction tests if any of the higher bits within the 16k stackframe address are set. In this specific case that would be bits 7-15 (mask 0x3f80). If no bit would be set we know that only up to 128 bytes would be left on the stack, and thus trigger an exception.

This check does of course only work if a 16k stack is also 16k aligned, which is always the case.

Quoted 8 lines

The tmll instruction tests if any of the higher bits within the 16k stackframe address are set. In this specific case that would be bits 7-15 (mask 0x3f80). If no bit would be set we know that only up to 128 bytes would be left on the stack, and thus trigger an exception.

This check does of course only work if a 16k stack is also 16k aligned, which is always the case.

Oh, interesting. How do you handle the case of a single function that uses more than 128 bytes of stack?

Since the dawn of time, a kernel stack overflow has been a real PITA to debug, has caused nondeterministic crashes some time after the actual overflow, and has generally been easy to exploit for root.

With this series, arches can enable HAVE_ARCH_VMAP_STACK. Arches that enable it (just x86 for now) get virtually mapped stacks with guard pages. This causes reliable faults when the stack overflows.

If the arch implements it well, we get a nice OOPS on stack overflow (as opposed to panicing directly or otherwise exploding badly). On x86, the OOPS is nice, has a usable call trace, and the overflowing task is killed cleanly.

This does not address interrupt stacks.

Andy Lutomirski (12):
x86/cpa: In populate_pgd, don't set the pgd entry until it's populated x86/cpa: Warn if kernel_unmap_pages_in_pgd is used inappropriately
mm: Track NR_KERNEL_STACK in pages instead of number of stacks
mm: Move memcg stack accounting to account_kernel_stack
fork: Add generic vmalloced stack support
x86/die: Don't try to recover from an OOPS on a non-default stack x86/dumpstack: When OOPSing, rewind the stack before do_exit x86/dumpstack: When dumping stack bytes due to OOPS, start with

    regs->sp

x86/dumpstack: Try harder to get a call trace on stack overflow x86/dumpstack/64: Handle faults when printing the "Stack:" part of an
OOPS
x86/mm/64: Enable vmapped stacks
x86/mm: Improve stack-overflow #PF handling

Ingo Molnar (1):
x86/mm/hotplug: Don't remove PGD entries in remove_pagetable()

 arch/Kconfig                     | 12 ++++++++++
 arch/x86/Kconfig                 |  1 +
 arch/x86/entry/entry_32.S        | 11 +++++++++
 arch/x86/entry/entry_64.S        | 11 +++++++++
 arch/x86/include/asm/switch_to.h | 28 +++++++++++++++++++++-
 arch/x86/include/asm/traps.h     |  6 +++++
 arch/x86/kernel/dumpstack.c      | 17 +++++++++++++-
 arch/x86/kernel/dumpstack_32.c   |  4 +++-
 arch/x86/kernel/dumpstack_64.c   | 16 ++++++++++---
 arch/x86/kernel/traps.c          | 32 +++++++++++++++++++++++++

 arch/x86/mm/fault.c              | 39 ++++++++++++++++++++++++++++++
 arch/x86/mm/init_64.c            | 27 ---------------------
 arch/x86/mm/pageattr.c           |  7 +++++-
 arch/x86/mm/tlb.c                | 15 ++++++++++++
 fs/proc/meminfo.c                |  2 +-
 kernel/fork.c                    | 51 ++++++++++++++++++++++++++++++----------
 mm/page_alloc.c                  |  3 +--
 17 files changed, 233 insertions(+), 49 deletions(-)

printk: Make the printk*once() variants return a value

include/linux/printk.h +0 -3

@@ -108,11 +108,14 @@ struct va_format {
   * Dummy printk for disabled debugging statements to use whilst maintaining
   * gcc's format checking.
   */
-#define no_printk(fmt, ...)			\
-do {						\
-	if (0)					\

This change isn't described in the commit message and there doesn't seem to be a need to change this.

Quoted 2 lines · Joe Perches

This change isn't described in the commit message and there doesn't seem to be a need to change this.

How do *you* know? Did *you* actually sit down and build a kernel with your proposed change before sending a reply?
I'm pretty sure you didn't.

Well, there is a very good reason why I made that change but I'm not going to tell you.

defconfigs both with and without CONFIG_PRINTK build properly with the proposed change to this specific patch.

Borislav, your delightful personality always impresses. Never change.

Quoted 3 lines · Joe Perches

defconfigs both with and without CONFIG_PRINTK build properly with the proposed change to this specific patch.

Did you try latest tip/master?

Assuming tip is included in linux-next as of july 8, yes.

Quoted 2 lines · Joe Perches

Assuming tip is included in linux-next as of july 8, yes.

Try one which has http://git.kernel.org/tip/81c2949f7fdcf8ff681326669a
fde24962232670

From: Borislav Petkov <[email protected]>

Hi all,

here's v2 with review feedback integrated.

Quoted 5 lines · Borislav Petkov

From: Borislav Petkov <[email protected]>

Hi all,

here's v2 with review feedback integrated.

Just a silly overall thought: WARN_ON will be noticed by the 0day bot and similar tools. This series changes the message text a bit -- is it still noticed?