x86: stack-related cleanups and fixes

Here's an assortment of stack-related cleanups and fixes I found when writing the new x86 unwinder.

Josh Poimboeuf (8):
x86/dumpstack: remove show_trace()
x86/asm/head: remove unused init_rsp variable extern x86/asm/head: rename 'stack_start' -> 'initial_stack' x86/dumpstack: remove extra brackets around "<EOE>" x86/head: remove useless zeroed word
x86/dumpstack: fix x86_32 kernel_stack_pointer() previous stack access
proc: fix return address printk conversion specifer in

    /proc/<pid>/stack

x86: remove 64-byte gap at end of irq stack

 arch/x86/include/asm/kdebug.h   |  2 --
 arch/x86/include/asm/realmode.h |  2 +-
 arch/x86/include/asm/smp.h      |  3 ---
 arch/x86/kernel/acpi/sleep.c    |  2 +-
 arch/x86/kernel/cpu/common.c    |  2 +-
 arch/x86/kernel/dumpstack.c     |  6 ------
 arch/x86/kernel/dumpstack_64.c  | 11 ++++-------
 arch/x86/kernel/head_32.S       |  8 ++++----
 arch/x86/kernel/head_64.S       | 12 +++++-------
 arch/x86/kernel/ptrace.c        |  4 ++--
 arch/x86/kernel/setup_percpu.c  |  2 +-
 arch/x86/kernel/smpboot.c       |  2 +-
 fs/proc/base.c                  |  2 +-
 13 files changed, 21 insertions(+), 37 deletions(-)

2.7.4

There are a bewildering array of options for dumping the stack. Simplify things a little by removing show_trace(), which is unused.

---
 arch/x86/include/asm/kdebug.h | 2 --
 arch/x86/kernel/dumpstack.c   | 6 ------
 2 files changed, 8 deletions(-)

index 1ef9d58..d318811 100644
@@ -24,8 +24,6 @@ enum die_val {
 extern void printk_address(unsigned long address);
 extern void die(const char *, struct pt_regs *,long);
 extern int __must_check __die(const char *, struct pt_regs *, long);
-extern void show_trace(struct task_struct *t, struct pt_regs *regs,
-		       unsigned long *sp, unsigned long bp);
 extern void show_stack_regs(struct pt_regs *regs);
 extern void __show_regs(struct pt_regs *regs, int all);
 extern unsigned long oops_begin(void);

index 92e8f0a..5f49c04 100644
@@ -182,12 +182,6 @@ show_trace_log_lvl(struct task_struct *task, struct pt_regs *regs,
 	dump_trace(task, regs, stack, bp, &print_trace_ops, log_lvl);
 }
 
-void show_trace(struct task_struct *task, struct pt_regs *regs,
-		unsigned long *stack, unsigned long bp)
-{
-	show_trace_log_lvl(task, regs, stack, bp, "");
-}
-
 void show_stack(struct task_struct *task, unsigned long *sp)
 {
 	unsigned long bp = 0;

2.7.4

There is no init_rsp variable. Remove its extern.

---
 arch/x86/include/asm/realmode.h | 1 -
 1 file changed, 1 deletion(-)

index b2988c0..3327ffb 100644
@@ -44,7 +44,6 @@ struct trampoline_header {
 extern struct real_mode_header *real_mode_header;
 extern unsigned char real_mode_blob_end[];
 
-extern unsigned long init_rsp;
 extern unsigned long initial_code;
 extern unsigned long initial_gs;
 

2.7.4

The 'stack_start' variable is similar in usage to 'initial_code' and 'initial_gs': they're all stored in head_64.S and they're all updated by SMP and ACPI suspend before starting a CPU.

Rename it to 'initial_stack' to be consistent with the others.

---
 arch/x86/include/asm/realmode.h |  1 +
 arch/x86/include/asm/smp.h      |  3 ---
 arch/x86/kernel/acpi/sleep.c    |  2 +-
 arch/x86/kernel/head_32.S       |  8 ++++----
 arch/x86/kernel/head_64.S       | 11 +++++------
 arch/x86/kernel/smpboot.c       |  2 +-
 6 files changed, 12 insertions(+), 15 deletions(-)

index 3327ffb..230e190 100644
@@ -46,6 +46,7 @@ extern unsigned char real_mode_blob_end[];
 
 extern unsigned long initial_code;
 extern unsigned long initial_gs;
+extern unsigned long initial_stack;
 
 extern unsigned char real_mode_blob[];
 extern unsigned char real_mode_relocs[];

index ebd0c16..19980b3 100644
@@ -39,9 +39,6 @@ DECLARE_EARLY_PER_CPU_READ_MOSTLY(u16, x86_bios_cpu_apicid);
 DECLARE_EARLY_PER_CPU_READ_MOSTLY(int, x86_cpu_to_logical_apicid);
 #endif
 
-/* Static state in head.S used to set up a CPU */
-extern unsigned long stack_start; /* Initial stack pointer address */
-
 struct task_struct;
 
 struct smp_ops {

index adb3eaf..4858733 100644
@@ -99,7 +99,7 @@ int x86_acpi_suspend_lowlevel(void)
 	saved_magic = 0x12345678;
 #else /* CONFIG_64BIT */
 #ifdef CONFIG_SMP
-	stack_start = (unsigned long)temp_stack + sizeof(temp_stack);
+	initial_stack = (unsigned long)temp_stack + sizeof(temp_stack);
 	early_gdt_descr.address =
 			(unsigned long)get_cpu_gdt_table(smp_processor_id());
 	initial_gs = per_cpu_offset(smp_processor_id());

index 6f8902b..5f40126 100644
@@ -94,7 +94,7 @@ RESERVE_BRK(pagetables, INIT_MAP_SIZE)
  */
 __HEAD
 ENTRY(startup_32)
-	movl pa(stack_start),%ecx
+	movl pa(initial_stack),%ecx
 	
 	/* test KEEP_SEGMENTS flag to see if the bootloader is asking
 		us to not reload segments */

@@ -286,7 +286,7 @@ num_subarch_entries = (. - subarch_entries) / 4
  * start_secondary().
  */
 ENTRY(start_cpu0)
-	movl stack_start, %ecx
+	movl initial_stack, %ecx
 	movl %ecx, %esp
 	jmp  *(initial_code)
 ENDPROC(start_cpu0)

@@ -307,7 +307,7 @@ ENTRY(startup_32_smp)
 	movl %eax,%es
 	movl %eax,%fs
 	movl %eax,%gs
-	movl pa(stack_start),%ecx
+	movl pa(initial_stack),%ecx
 	movl %eax,%ss
 	leal -__PAGE_OFFSET(%ecx),%esp
 

@@ -703,7 +703,7 @@ ENTRY(initial_page_table)
 
 .data
 .balign 4
-ENTRY(stack_start)
+ENTRY(initial_stack)
 	.long init_thread_union+THREAD_SIZE
 
 __INITRODATA

index 9f8efc9..e048142 100644
@@ -66,7 +66,7 @@ startup_64:
 	 */
 
 	/*
-	 * Setup stack for verify_cpu(). "-8" because stack_start is defined
+	 * Setup stack for verify_cpu(). "-8" because initial_stack is defined
 	 * this way, see below. Our best guess is a NULL ptr for stack
 	 * termination heuristics and we don't want to break anything which
 	 * might depend on it (kgdb, ...).

@@ -226,7 +226,7 @@ ENTRY(secondary_startup_64)
 	movq	%rax, %cr0
 
 	/* Setup a boot time stack */
-	movq stack_start(%rip), %rsp
+	movq initial_stack(%rip), %rsp
 
 	/* zero EFLAGS after setting rsp */
 	pushq $0

@@ -310,7 +310,7 @@ ENDPROC(secondary_startup_64)
  * start_secondary().
  */
 ENTRY(start_cpu0)
-	movq stack_start(%rip),%rsp
+	movq initial_stack(%rip),%rsp
 	movq	initial_code(%rip),%rax
 	pushq	$0		# fake return address to stop unwinder
 	pushq	$__KERNEL_CS	# set correct cs

@@ -319,15 +319,14 @@ ENTRY(start_cpu0)
 ENDPROC(start_cpu0)
 #endif
 
-	/* SMP bootup changes these two */
+	/* Both SMP bootup and ACPI suspend change these variables */
 	__REFDATA
 	.balign	8
 	GLOBAL(initial_code)
 	.quad	x86_64_start_kernel
 	GLOBAL(initial_gs)
 	.quad	INIT_PER_CPU_VAR(irq_stack_union)
-
-	GLOBAL(stack_start)
+	GLOBAL(initial_stack)
 	.quad  init_thread_union+THREAD_SIZE-8
 	.word  0
 	__FINITDATA

index 26b473d..3878725 100644
@@ -969,7 +969,7 @@ static int do_boot_cpu(int apicid, int cpu, struct task_struct *idle)
 
 	early_gdt_descr.address = (unsigned long)get_cpu_gdt_table(cpu);
 	initial_code = (unsigned long)start_secondary;
-	stack_start  = idle->thread.sp;
+	initial_stack  = idle->thread.sp;
 
 	/*
 	 * Enable the espfix hack for this CPU

2.7.4

When starting the dump of an exception stack, it shows "<<EOE>>" instead of "<EOE>". print_trace_stack() already adds brackets, no need to add them again.

---
 arch/x86/kernel/dumpstack_64.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 9ee4520..daf9f63 100644
@@ -202,7 +202,7 @@ void dump_trace(struct task_struct *task, struct pt_regs *regs,
 
 			bp = ops->walk_stack(task, stack, bp, ops,
 					     data, stack_end, &graph);
-			ops->stack(data, "<EOE>");
+			ops->stack(data, "EOE");
 			/*
 			 * We link to the next stack via the
 			 * second-to-last pointer (index -2 to end) in the

2.7.4

This zeroed word has no apparent purpose, so remove it.

Brian Gerst says:

"FYI the word used to be the SS segment selector for the LSS instruction, which isn't needed in 64-bit mode."

---
 arch/x86/kernel/head_64.S | 1 -
 1 file changed, 1 deletion(-)

index e048142..c98a559 100644
@@ -328,7 +328,6 @@ ENDPROC(start_cpu0)
 	.quad	INIT_PER_CPU_VAR(irq_stack_union)
 	GLOBAL(initial_stack)
 	.quad  init_thread_union+THREAD_SIZE-8
-	.word  0
 	__FINITDATA
 
 bad_address:

2.7.4

Seems dangerous. It wouldn't surprise me if some CPUs or x86 emulations load it anyways and trigger page faults if there is really nothing there.

-Andi

Yes, seems pointlessly risky.

Sent from my Android device with K-9 Mail. Please excuse brevity and formatting.

It was added in commit 9cf4f298e29abba25c16679fe7be70898223167e, copied from 32-bit but never used. It doesn't look like we ever used LSS on 64-bit.

--
Brian Gerst

Ah, ok. Wasn't in front of a computer.

Sent from my Android device with K-9 Mail. Please excuse brevity and formatting.

Don't be silly, Andi and Peter.

We don't actually *use* lss any more. Not even on 32-bit. The zero is never accessed. And on x86-64 we never did, obviously.

On 32-bit that zero doesn't even exist any more. On x86-64, it was never even used at all. On x86-32, it was removed in commit 11d4c3f9b671 ("x86-32: Make sure the stack is set up before we use it"), when the code stopped doing lss.

On x86-64, it has never made sense. It was added in commit 9cf4f298e29a ("x86: use stack_start in x86_64") to match x86-32 at the time, but it didn't actually make sense even then, because x86-64 didn't use lss. 32-bit did, but 64-but just did a simple

movq stack_start(%rip),%rsp

like a good user should.

Linus

On x86_32, when an interrupt happens from kernel space, SS and SP aren't pushed and the existing stack is used. So pt_regs is effectively two words shorter, and the previous stack pointer is normally the memory after the shortened pt_regs, aka '&regs->sp'.

But in the rare case where the interrupt hits right after the stack pointer has been changed to point to an empty stack, like for example when call_on_stack() is used, the address immediately after the shortened pt_regs is no longer on the stack. In that case, instead of '&regs->sp', the previous stack pointer should be retrieved from the beginning of the current stack page.

kernel_stack_pointer() wants to do that, but it forgets to dereference the pointer. So instead of returning a pointer to the previous stack, it returns a pointer to the beginning of the current stack.

Note that it's probably outside of kernel_stack_pointer()'s scope to be switching stacks at all. The x86_64 version of this function doesn't do it, and it would be better for the caller to do it if necessary. But that's a patch for another day. This just fixes the original intent.

---
 arch/x86/kernel/ptrace.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index 2537cfb..5b88a1b 100644
@@ -173,8 +173,8 @@ unsigned long kernel_stack_pointer(struct pt_regs *regs)
 		return sp;
 
 	prev_esp = (u32 *)(context);
-	if (prev_esp)
-		return (unsigned long)prev_esp;
+	if (*prev_esp)
+		return (unsigned long)*prev_esp;
 
 	return (unsigned long)regs;
 }

2.7.4

When printing call return addresses found on a stack, /proc/<pid>/stack can sometimes give a confusing result. If the call instruction was the last instruction in the function (which can happen when calling a noreturn function), '%pS' will incorrectly display the name of the function which happens to be next in the object code, rather than the name of the actual calling function.

Use '%pB' instead, which was created for this exact purpose.

---
 fs/proc/base.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 54e2702..e9ff186 100644
@@ -483,7 +483,7 @@ static int proc_pid_stack(struct seq_file *m, struct pid_namespace *ns,
 		save_stack_trace_tsk(task, &trace);
 
 		for (i = 0; i < trace.nr_entries; i++) {
-			seq_printf(m, "[<%pK>] %pS\n",
+			seq_printf(m, "[<%pK>] %pB\n",
 				   (void *)entries[i], (void *)entries[i]);
 		}
 		unlock_trace(task);

2.7.4

There has been a 64-byte gap at the end of the irq stack for at least 12 years. It predates git history, and I can't find any good reason for it. Remove it. What's the worst that could happen?

---
 arch/x86/kernel/cpu/common.c   | 2 +-
 arch/x86/kernel/dumpstack_64.c | 9 +++------
 arch/x86/kernel/setup_percpu.c | 2 +-
 3 files changed, 5 insertions(+), 8 deletions(-)

index d3b91be..ce7a4c1 100644
@@ -1286,7 +1286,7 @@ DEFINE_PER_CPU(struct task_struct *, current_task) ____cacheline_aligned =
 EXPORT_PER_CPU_SYMBOL(current_task);
 
 DEFINE_PER_CPU(char *, irq_stack_ptr) =
-	init_per_cpu_var(irq_stack_union.irq_stack) + IRQ_STACK_SIZE - 64;
+	init_per_cpu_var(irq_stack_union.irq_stack) + IRQ_STACK_SIZE;
 
 DEFINE_PER_CPU(unsigned int, irq_count) __visible = -1;
 

index daf9f63..066eb5c7 100644
@@ -103,9 +103,6 @@ in_irq_stack(unsigned long *stack, unsigned long *irq_stack,
 	return (stack >= irq_stack && stack < irq_stack_end);
 }
 
-static const unsigned long irq_stack_size =
-	(IRQ_STACK_SIZE - 64) / sizeof(unsigned long);
-
 enum stack_type {
 	STACK_IS_UNKNOWN,
 	STACK_IS_NORMAL,

@@ -133,7 +130,7 @@ analyze_stack(int cpu, struct task_struct *task, unsigned long *stack,
 		return STACK_IS_NORMAL;
 
 	*stack_end = irq_stack;
-	irq_stack = irq_stack - irq_stack_size;
+	irq_stack -= (IRQ_STACK_SIZE / sizeof(long));
 
 	if (in_irq_stack(stack, irq_stack, *stack_end))
 		return STACK_IS_IRQ;

@@ -256,8 +253,8 @@ show_stack_log_lvl(struct task_struct *task, struct pt_regs *regs,
 	preempt_disable();
 	cpu = smp_processor_id();
 
-	irq_stack_end	= (unsigned long *)(per_cpu(irq_stack_ptr, cpu));
-	irq_stack	= (unsigned long *)(per_cpu(irq_stack_ptr, cpu) - IRQ_STACK_SIZE);
+	irq_stack_end = (unsigned long *)(per_cpu(irq_stack_ptr, cpu));
+	irq_stack     = irq_stack_end - (IRQ_STACK_SIZE / sizeof(long));
 
 	/*
 	 * Debugging aid: "show_stack(NULL, NULL);" prints the

index 1d5c794..2bbd27f 100644
@@ -246,7 +246,7 @@ void __init setup_per_cpu_areas(void)
 #ifdef CONFIG_X86_64
 		per_cpu(irq_stack_ptr, cpu) =
 			per_cpu(irq_stack_union.irq_stack, cpu) +
-			IRQ_STACK_SIZE - 64;
+			IRQ_STACK_SIZE;
 #endif
 #ifdef CONFIG_NUMA
 		per_cpu(x86_cpu_to_node_map, cpu) =

2.7.4