* [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 '®s->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
'®s->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).