linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/8] x86: stack-related cleanups and fixes
@ 2016-08-18 15:59 Josh Poimboeuf
  2016-08-18 15:59 ` [PATCH 1/8] x86/dumpstack: remove show_trace() Josh Poimboeuf
                   ` (7 more replies)
  0 siblings, 8 replies; 14+ messages in thread
From: Josh Poimboeuf @ 2016-08-18 15:59 UTC (permalink / raw)
  To: Thomas Gleixner, Ingo Molnar, H . Peter Anvin
  Cc: x86, linux-kernel, Andy Lutomirski, Linus Torvalds,
	Steven Rostedt, Brian Gerst, Kees Cook, Peter Zijlstra,
	Frederic Weisbecker, Byungchul Park, Nilay Vaish

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 1/8] x86/dumpstack: remove show_trace()
  2016-08-18 15:59 [PATCH 0/8] x86: stack-related cleanups and fixes Josh Poimboeuf
@ 2016-08-18 15:59 ` Josh Poimboeuf
  2016-08-18 15:59 ` [PATCH 2/8] x86/asm/head: remove unused init_rsp variable extern Josh Poimboeuf
                   ` (6 subsequent siblings)
  7 siblings, 0 replies; 14+ messages in thread
From: Josh Poimboeuf @ 2016-08-18 15:59 UTC (permalink / raw)
  To: Thomas Gleixner, Ingo Molnar, H . Peter Anvin
  Cc: x86, linux-kernel, Andy Lutomirski, Linus Torvalds,
	Steven Rostedt, Brian Gerst, Kees Cook, Peter Zijlstra,
	Frederic Weisbecker, Byungchul Park, Nilay Vaish

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

Reviewed-by: Andy Lutomirski <luto@kernel.org>
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 arch/x86/include/asm/kdebug.h | 2 --
 arch/x86/kernel/dumpstack.c   | 6 ------
 2 files changed, 8 deletions(-)

diff --git a/arch/x86/include/asm/kdebug.h b/arch/x86/include/asm/kdebug.h
index 1ef9d58..d318811 100644
--- a/arch/x86/include/asm/kdebug.h
+++ b/arch/x86/include/asm/kdebug.h
@@ -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);
diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c
index 92e8f0a..5f49c04 100644
--- a/arch/x86/kernel/dumpstack.c
+++ b/arch/x86/kernel/dumpstack.c
@@ -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

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 2/8] x86/asm/head: remove unused init_rsp variable extern
  2016-08-18 15:59 [PATCH 0/8] x86: stack-related cleanups and fixes Josh Poimboeuf
  2016-08-18 15:59 ` [PATCH 1/8] x86/dumpstack: remove show_trace() Josh Poimboeuf
@ 2016-08-18 15:59 ` Josh Poimboeuf
  2016-08-18 15:59 ` [PATCH 3/8] x86/asm/head: rename 'stack_start' -> 'initial_stack' Josh Poimboeuf
                   ` (5 subsequent siblings)
  7 siblings, 0 replies; 14+ messages in thread
From: Josh Poimboeuf @ 2016-08-18 15:59 UTC (permalink / raw)
  To: Thomas Gleixner, Ingo Molnar, H . Peter Anvin
  Cc: x86, linux-kernel, Andy Lutomirski, Linus Torvalds,
	Steven Rostedt, Brian Gerst, Kees Cook, Peter Zijlstra,
	Frederic Weisbecker, Byungchul Park, Nilay Vaish

There is no init_rsp variable.  Remove its extern.

Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 arch/x86/include/asm/realmode.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/x86/include/asm/realmode.h b/arch/x86/include/asm/realmode.h
index b2988c0..3327ffb 100644
--- a/arch/x86/include/asm/realmode.h
+++ b/arch/x86/include/asm/realmode.h
@@ -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

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 3/8] x86/asm/head: rename 'stack_start' -> 'initial_stack'
  2016-08-18 15:59 [PATCH 0/8] x86: stack-related cleanups and fixes Josh Poimboeuf
  2016-08-18 15:59 ` [PATCH 1/8] x86/dumpstack: remove show_trace() Josh Poimboeuf
  2016-08-18 15:59 ` [PATCH 2/8] x86/asm/head: remove unused init_rsp variable extern Josh Poimboeuf
@ 2016-08-18 15:59 ` Josh Poimboeuf
  2016-08-18 15:59 ` [PATCH 4/8] x86/dumpstack: remove extra brackets around "<EOE>" Josh Poimboeuf
                   ` (4 subsequent siblings)
  7 siblings, 0 replies; 14+ messages in thread
From: Josh Poimboeuf @ 2016-08-18 15:59 UTC (permalink / raw)
  To: Thomas Gleixner, Ingo Molnar, H . Peter Anvin
  Cc: x86, linux-kernel, Andy Lutomirski, Linus Torvalds,
	Steven Rostedt, Brian Gerst, Kees Cook, Peter Zijlstra,
	Frederic Weisbecker, Byungchul Park, Nilay Vaish

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.

Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 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(-)

diff --git a/arch/x86/include/asm/realmode.h b/arch/x86/include/asm/realmode.h
index 3327ffb..230e190 100644
--- a/arch/x86/include/asm/realmode.h
+++ b/arch/x86/include/asm/realmode.h
@@ -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[];
diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h
index ebd0c16..19980b3 100644
--- a/arch/x86/include/asm/smp.h
+++ b/arch/x86/include/asm/smp.h
@@ -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 {
diff --git a/arch/x86/kernel/acpi/sleep.c b/arch/x86/kernel/acpi/sleep.c
index adb3eaf..4858733 100644
--- a/arch/x86/kernel/acpi/sleep.c
+++ b/arch/x86/kernel/acpi/sleep.c
@@ -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());
diff --git a/arch/x86/kernel/head_32.S b/arch/x86/kernel/head_32.S
index 6f8902b..5f40126 100644
--- a/arch/x86/kernel/head_32.S
+++ b/arch/x86/kernel/head_32.S
@@ -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
diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
index 9f8efc9..e048142 100644
--- a/arch/x86/kernel/head_64.S
+++ b/arch/x86/kernel/head_64.S
@@ -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
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 26b473d..3878725 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -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

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 4/8] x86/dumpstack: remove extra brackets around "<EOE>"
  2016-08-18 15:59 [PATCH 0/8] x86: stack-related cleanups and fixes Josh Poimboeuf
                   ` (2 preceding siblings ...)
  2016-08-18 15:59 ` [PATCH 3/8] x86/asm/head: rename 'stack_start' -> 'initial_stack' Josh Poimboeuf
@ 2016-08-18 15:59 ` Josh Poimboeuf
  2016-08-18 15:59 ` [PATCH 5/8] x86/head: remove useless zeroed word Josh Poimboeuf
                   ` (3 subsequent siblings)
  7 siblings, 0 replies; 14+ messages in thread
From: Josh Poimboeuf @ 2016-08-18 15:59 UTC (permalink / raw)
  To: Thomas Gleixner, Ingo Molnar, H . Peter Anvin
  Cc: x86, linux-kernel, Andy Lutomirski, Linus Torvalds,
	Steven Rostedt, Brian Gerst, Kees Cook, Peter Zijlstra,
	Frederic Weisbecker, Byungchul Park, Nilay Vaish

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.

Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 arch/x86/kernel/dumpstack_64.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/dumpstack_64.c b/arch/x86/kernel/dumpstack_64.c
index 9ee4520..daf9f63 100644
--- a/arch/x86/kernel/dumpstack_64.c
+++ b/arch/x86/kernel/dumpstack_64.c
@@ -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

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 5/8] x86/head: remove useless zeroed word
  2016-08-18 15:59 [PATCH 0/8] x86: stack-related cleanups and fixes Josh Poimboeuf
                   ` (3 preceding siblings ...)
  2016-08-18 15:59 ` [PATCH 4/8] x86/dumpstack: remove extra brackets around "<EOE>" Josh Poimboeuf
@ 2016-08-18 15:59 ` Josh Poimboeuf
  2016-08-22 21:48   ` Andi Kleen
  2016-08-18 15:59 ` [PATCH 6/8] x86/dumpstack: fix x86_32 kernel_stack_pointer() previous stack access Josh Poimboeuf
                   ` (2 subsequent siblings)
  7 siblings, 1 reply; 14+ messages in thread
From: Josh Poimboeuf @ 2016-08-18 15:59 UTC (permalink / raw)
  To: Thomas Gleixner, Ingo Molnar, H . Peter Anvin
  Cc: x86, linux-kernel, Andy Lutomirski, Linus Torvalds,
	Steven Rostedt, Brian Gerst, Kees Cook, Peter Zijlstra,
	Frederic Weisbecker, Byungchul Park, Nilay Vaish

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

Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 arch/x86/kernel/head_64.S | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
index e048142..c98a559 100644
--- a/arch/x86/kernel/head_64.S
+++ b/arch/x86/kernel/head_64.S
@@ -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

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 6/8] x86/dumpstack: fix x86_32 kernel_stack_pointer() previous stack access
  2016-08-18 15:59 [PATCH 0/8] x86: stack-related cleanups and fixes Josh Poimboeuf
                   ` (4 preceding siblings ...)
  2016-08-18 15:59 ` [PATCH 5/8] x86/head: remove useless zeroed word Josh Poimboeuf
@ 2016-08-18 15:59 ` Josh Poimboeuf
  2016-08-18 15:59 ` [PATCH 7/8] proc: fix return address printk conversion specifer in /proc/<pid>/stack Josh Poimboeuf
  2016-08-18 15:59 ` [PATCH 8/8] x86: remove 64-byte gap at end of irq stack Josh Poimboeuf
  7 siblings, 0 replies; 14+ messages in thread
From: Josh Poimboeuf @ 2016-08-18 15:59 UTC (permalink / raw)
  To: Thomas Gleixner, Ingo Molnar, H . Peter Anvin
  Cc: x86, linux-kernel, Andy Lutomirski, Linus Torvalds,
	Steven Rostedt, Brian Gerst, Kees Cook, Peter Zijlstra,
	Frederic Weisbecker, Byungchul Park, Nilay Vaish

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.

Fixes: 0788aa6a23cb ("x86: Prepare removal of previous_esp from i386 thread_info structure")
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 arch/x86/kernel/ptrace.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c
index 2537cfb..5b88a1b 100644
--- a/arch/x86/kernel/ptrace.c
+++ b/arch/x86/kernel/ptrace.c
@@ -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

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 7/8] proc: fix return address printk conversion specifer in /proc/<pid>/stack
  2016-08-18 15:59 [PATCH 0/8] x86: stack-related cleanups and fixes Josh Poimboeuf
                   ` (5 preceding siblings ...)
  2016-08-18 15:59 ` [PATCH 6/8] x86/dumpstack: fix x86_32 kernel_stack_pointer() previous stack access Josh Poimboeuf
@ 2016-08-18 15:59 ` Josh Poimboeuf
  2016-08-18 15:59 ` [PATCH 8/8] x86: remove 64-byte gap at end of irq stack Josh Poimboeuf
  7 siblings, 0 replies; 14+ messages in thread
From: Josh Poimboeuf @ 2016-08-18 15:59 UTC (permalink / raw)
  To: Thomas Gleixner, Ingo Molnar, H . Peter Anvin
  Cc: x86, linux-kernel, Andy Lutomirski, Linus Torvalds,
	Steven Rostedt, Brian Gerst, Kees Cook, Peter Zijlstra,
	Frederic Weisbecker, Byungchul Park, Nilay Vaish

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.

Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 fs/proc/base.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 54e2702..e9ff186 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -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

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 8/8] x86: remove 64-byte gap at end of irq stack
  2016-08-18 15:59 [PATCH 0/8] x86: stack-related cleanups and fixes Josh Poimboeuf
                   ` (6 preceding siblings ...)
  2016-08-18 15:59 ` [PATCH 7/8] proc: fix return address printk conversion specifer in /proc/<pid>/stack Josh Poimboeuf
@ 2016-08-18 15:59 ` Josh Poimboeuf
  7 siblings, 0 replies; 14+ messages in thread
From: Josh Poimboeuf @ 2016-08-18 15:59 UTC (permalink / raw)
  To: Thomas Gleixner, Ingo Molnar, H . Peter Anvin
  Cc: x86, linux-kernel, Andy Lutomirski, Linus Torvalds,
	Steven Rostedt, Brian Gerst, Kees Cook, Peter Zijlstra,
	Frederic Weisbecker, Byungchul Park, Nilay Vaish

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?

Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 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(-)

diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index d3b91be..ce7a4c1 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -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;
 
diff --git a/arch/x86/kernel/dumpstack_64.c b/arch/x86/kernel/dumpstack_64.c
index daf9f63..066eb5c7 100644
--- a/arch/x86/kernel/dumpstack_64.c
+++ b/arch/x86/kernel/dumpstack_64.c
@@ -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
diff --git a/arch/x86/kernel/setup_percpu.c b/arch/x86/kernel/setup_percpu.c
index 1d5c794..2bbd27f 100644
--- a/arch/x86/kernel/setup_percpu.c
+++ b/arch/x86/kernel/setup_percpu.c
@@ -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

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH 5/8] x86/head: remove useless zeroed word
  2016-08-18 15:59 ` [PATCH 5/8] x86/head: remove useless zeroed word Josh Poimboeuf
@ 2016-08-22 21:48   ` Andi Kleen
  2016-08-22 22:23     ` H. Peter Anvin
  2016-08-22 22:47     ` Linus Torvalds
  0 siblings, 2 replies; 14+ messages in thread
From: Andi Kleen @ 2016-08-22 21:48 UTC (permalink / raw)
  To: Josh Poimboeuf
  Cc: Thomas Gleixner, Ingo Molnar, H . Peter Anvin, x86, linux-kernel,
	Andy Lutomirski, Linus Torvalds, Steven Rostedt, Brian Gerst,
	Kees Cook, Peter Zijlstra, Frederic Weisbecker, Byungchul Park,
	Nilay Vaish

Josh Poimboeuf <jpoimboe@redhat.com> writes:

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

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 5/8] x86/head: remove useless zeroed word
  2016-08-22 21:48   ` Andi Kleen
@ 2016-08-22 22:23     ` H. Peter Anvin
  2016-08-22 22:47       ` Brian Gerst
  2016-08-22 22:47     ` Linus Torvalds
  1 sibling, 1 reply; 14+ messages in thread
From: H. Peter Anvin @ 2016-08-22 22:23 UTC (permalink / raw)
  To: Andi Kleen, Josh Poimboeuf
  Cc: Thomas Gleixner, Ingo Molnar, x86, linux-kernel, Andy Lutomirski,
	Linus Torvalds, Steven Rostedt, Brian Gerst, Kees Cook,
	Peter Zijlstra, Frederic Weisbecker, Byungchul Park, Nilay Vaish

On August 22, 2016 2:48:46 PM PDT, Andi Kleen <andi@firstfloor.org> wrote:
>Josh Poimboeuf <jpoimboe@redhat.com> writes:
>
>> 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."
>>
>
>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.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 5/8] x86/head: remove useless zeroed word
  2016-08-22 21:48   ` Andi Kleen
  2016-08-22 22:23     ` H. Peter Anvin
@ 2016-08-22 22:47     ` Linus Torvalds
  1 sibling, 0 replies; 14+ messages in thread
From: Linus Torvalds @ 2016-08-22 22:47 UTC (permalink / raw)
  To: Andi Kleen
  Cc: Josh Poimboeuf, Thomas Gleixner, Ingo Molnar, H . Peter Anvin,
	the arch/x86 maintainers, Linux Kernel Mailing List,
	Andy Lutomirski, Steven Rostedt, Brian Gerst, Kees Cook,
	Peter Zijlstra, Frederic Weisbecker, Byungchul Park, Nilay Vaish

On Mon, Aug 22, 2016 at 2:48 PM, Andi Kleen <andi@firstfloor.org> wrote:
>
> 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.

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 5/8] x86/head: remove useless zeroed word
  2016-08-22 22:23     ` H. Peter Anvin
@ 2016-08-22 22:47       ` Brian Gerst
  2016-08-22 23:10         ` H. Peter Anvin
  0 siblings, 1 reply; 14+ messages in thread
From: Brian Gerst @ 2016-08-22 22:47 UTC (permalink / raw)
  To: H. Peter Anvin
  Cc: Andi Kleen, Josh Poimboeuf, Thomas Gleixner, Ingo Molnar,
	the arch/x86 maintainers, Linux Kernel Mailing List,
	Andy Lutomirski, Linus Torvalds, Steven Rostedt, Kees Cook,
	Peter Zijlstra, Frederic Weisbecker, Byungchul Park, Nilay Vaish

On Mon, Aug 22, 2016 at 6:23 PM, H. Peter Anvin <hpa@zytor.com> wrote:
> On August 22, 2016 2:48:46 PM PDT, Andi Kleen <andi@firstfloor.org> wrote:
>>Josh Poimboeuf <jpoimboe@redhat.com> writes:
>>
>>> 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."
>>>
>>
>>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.

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 5/8] x86/head: remove useless zeroed word
  2016-08-22 22:47       ` Brian Gerst
@ 2016-08-22 23:10         ` H. Peter Anvin
  0 siblings, 0 replies; 14+ messages in thread
From: H. Peter Anvin @ 2016-08-22 23:10 UTC (permalink / raw)
  To: Brian Gerst
  Cc: Andi Kleen, Josh Poimboeuf, Thomas Gleixner, Ingo Molnar,
	the arch/x86 maintainers, Linux Kernel Mailing List,
	Andy Lutomirski, Linus Torvalds, Steven Rostedt, Kees Cook,
	Peter Zijlstra, Frederic Weisbecker, Byungchul Park, Nilay Vaish

On August 22, 2016 3:47:48 PM PDT, Brian Gerst <brgerst@gmail.com> wrote:
>On Mon, Aug 22, 2016 at 6:23 PM, H. Peter Anvin <hpa@zytor.com> wrote:
>> On August 22, 2016 2:48:46 PM PDT, Andi Kleen <andi@firstfloor.org>
>wrote:
>>>Josh Poimboeuf <jpoimboe@redhat.com> writes:
>>>
>>>> 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."
>>>>
>>>
>>>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.
>
>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.

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2016-08-22 23:13 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-08-18 15:59 [PATCH 0/8] x86: stack-related cleanups and fixes Josh Poimboeuf
2016-08-18 15:59 ` [PATCH 1/8] x86/dumpstack: remove show_trace() Josh Poimboeuf
2016-08-18 15:59 ` [PATCH 2/8] x86/asm/head: remove unused init_rsp variable extern Josh Poimboeuf
2016-08-18 15:59 ` [PATCH 3/8] x86/asm/head: rename 'stack_start' -> 'initial_stack' Josh Poimboeuf
2016-08-18 15:59 ` [PATCH 4/8] x86/dumpstack: remove extra brackets around "<EOE>" Josh Poimboeuf
2016-08-18 15:59 ` [PATCH 5/8] x86/head: remove useless zeroed word Josh Poimboeuf
2016-08-22 21:48   ` Andi Kleen
2016-08-22 22:23     ` H. Peter Anvin
2016-08-22 22:47       ` Brian Gerst
2016-08-22 23:10         ` H. Peter Anvin
2016-08-22 22:47     ` Linus Torvalds
2016-08-18 15:59 ` [PATCH 6/8] x86/dumpstack: fix x86_32 kernel_stack_pointer() previous stack access Josh Poimboeuf
2016-08-18 15:59 ` [PATCH 7/8] proc: fix return address printk conversion specifer in /proc/<pid>/stack Josh Poimboeuf
2016-08-18 15:59 ` [PATCH 8/8] x86: remove 64-byte gap at end of irq stack Josh Poimboeuf

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).