Re: [PATCH v3 45/51] x86: remove 64-byte gap at end of irq stack
On Sun, Aug 14, 2016 at 08:50:57AM -0400, Brian Gerst wrote: > On Sun, Aug 14, 2016 at 3:52 AM, Andy Lutomirskiwrote: > > On Fri, Aug 12, 2016 at 7:29 AM, Josh Poimboeuf wrote: > >> 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? > > > > I can't think of any reason this would matter. > > > > For that matter, do you have any idea why irq_stack_union is a union > > or why we insist on sticking it at %gs:0? Sure, the *canary* needs to > > live at a fixed offset (because GCC is daft, sigh), but I don't see > > what that has to do with the rest of the IRQ stack. > > > > --Andy > > Because the IRQ stack requires page alignment so it was convenient to > put it at the start of the per-cpu area. I don't think at the time I > wrote this there was specific support for page-aligned objects in > per-cpu memory. Since stacks grow down, it was tolerable to reserve a > few bytes at the bottom for the canary. Hm. Sounds like another good opportunity for a cleanup (though it's well outside the scope of this patch set). > What would be great is if we could leverage the new GCC plugin tools > to reimplement stack protector in a manner that is more compatible > with the kernel environment. It would make the stack canary a true > per-cpu variable instead of the hard-coded TLS-based location it is > now. That would make 64-bit be able to use normal delta per-cpu > offsets instead of zero-based, and would allow 32-bit to always do > lazy GS. > > -- > Brian Gerst -- Josh
Re: [PATCH v3 45/51] x86: remove 64-byte gap at end of irq stack
On Sun, Aug 14, 2016 at 08:50:57AM -0400, Brian Gerst wrote: > On Sun, Aug 14, 2016 at 3:52 AM, Andy Lutomirski wrote: > > On Fri, Aug 12, 2016 at 7:29 AM, Josh Poimboeuf wrote: > >> 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? > > > > I can't think of any reason this would matter. > > > > For that matter, do you have any idea why irq_stack_union is a union > > or why we insist on sticking it at %gs:0? Sure, the *canary* needs to > > live at a fixed offset (because GCC is daft, sigh), but I don't see > > what that has to do with the rest of the IRQ stack. > > > > --Andy > > Because the IRQ stack requires page alignment so it was convenient to > put it at the start of the per-cpu area. I don't think at the time I > wrote this there was specific support for page-aligned objects in > per-cpu memory. Since stacks grow down, it was tolerable to reserve a > few bytes at the bottom for the canary. Hm. Sounds like another good opportunity for a cleanup (though it's well outside the scope of this patch set). > What would be great is if we could leverage the new GCC plugin tools > to reimplement stack protector in a manner that is more compatible > with the kernel environment. It would make the stack canary a true > per-cpu variable instead of the hard-coded TLS-based location it is > now. That would make 64-bit be able to use normal delta per-cpu > offsets instead of zero-based, and would allow 32-bit to always do > lazy GS. > > -- > Brian Gerst -- Josh
Re: [PATCH v3 45/51] x86: remove 64-byte gap at end of irq stack
On Sun, Aug 14, 2016 at 12:52:40AM -0700, Andy Lutomirski wrote: > On Fri, Aug 12, 2016 at 7:29 AM, Josh Poimboeufwrote: > > 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? > > I can't think of any reason this would matter. > > For that matter, do you have any idea why irq_stack_union is a union > or why we insist on sticking it at %gs:0? Sure, the *canary* needs to > live at a fixed offset (because GCC is daft, sigh), but I don't see > what that has to do with the rest of the IRQ stack. Good question. I have no idea... -- Josh
Re: [PATCH v3 45/51] x86: remove 64-byte gap at end of irq stack
On Sun, Aug 14, 2016 at 12:52:40AM -0700, Andy Lutomirski wrote: > On Fri, Aug 12, 2016 at 7:29 AM, Josh Poimboeuf wrote: > > 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? > > I can't think of any reason this would matter. > > For that matter, do you have any idea why irq_stack_union is a union > or why we insist on sticking it at %gs:0? Sure, the *canary* needs to > live at a fixed offset (because GCC is daft, sigh), but I don't see > what that has to do with the rest of the IRQ stack. Good question. I have no idea... -- Josh
Re: [PATCH v3 45/51] x86: remove 64-byte gap at end of irq stack
On Sun, Aug 14, 2016 at 3:52 AM, Andy Lutomirskiwrote: > On Fri, Aug 12, 2016 at 7:29 AM, Josh Poimboeuf wrote: >> 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? > > I can't think of any reason this would matter. > > For that matter, do you have any idea why irq_stack_union is a union > or why we insist on sticking it at %gs:0? Sure, the *canary* needs to > live at a fixed offset (because GCC is daft, sigh), but I don't see > what that has to do with the rest of the IRQ stack. > > --Andy Because the IRQ stack requires page alignment so it was convenient to put it at the start of the per-cpu area. I don't think at the time I wrote this there was specific support for page-aligned objects in per-cpu memory. Since stacks grow down, it was tolerable to reserve a few bytes at the bottom for the canary. What would be great is if we could leverage the new GCC plugin tools to reimplement stack protector in a manner that is more compatible with the kernel environment. It would make the stack canary a true per-cpu variable instead of the hard-coded TLS-based location it is now. That would make 64-bit be able to use normal delta per-cpu offsets instead of zero-based, and would allow 32-bit to always do lazy GS. -- Brian Gerst
Re: [PATCH v3 45/51] x86: remove 64-byte gap at end of irq stack
On Sun, Aug 14, 2016 at 3:52 AM, Andy Lutomirski wrote: > On Fri, Aug 12, 2016 at 7:29 AM, Josh Poimboeuf wrote: >> 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? > > I can't think of any reason this would matter. > > For that matter, do you have any idea why irq_stack_union is a union > or why we insist on sticking it at %gs:0? Sure, the *canary* needs to > live at a fixed offset (because GCC is daft, sigh), but I don't see > what that has to do with the rest of the IRQ stack. > > --Andy Because the IRQ stack requires page alignment so it was convenient to put it at the start of the per-cpu area. I don't think at the time I wrote this there was specific support for page-aligned objects in per-cpu memory. Since stacks grow down, it was tolerable to reserve a few bytes at the bottom for the canary. What would be great is if we could leverage the new GCC plugin tools to reimplement stack protector in a manner that is more compatible with the kernel environment. It would make the stack canary a true per-cpu variable instead of the hard-coded TLS-based location it is now. That would make 64-bit be able to use normal delta per-cpu offsets instead of zero-based, and would allow 32-bit to always do lazy GS. -- Brian Gerst
Re: [PATCH v3 45/51] x86: remove 64-byte gap at end of irq stack
On Fri, Aug 12, 2016 at 7:29 AM, Josh Poimboeufwrote: > 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? I can't think of any reason this would matter. For that matter, do you have any idea why irq_stack_union is a union or why we insist on sticking it at %gs:0? Sure, the *canary* needs to live at a fixed offset (because GCC is daft, sigh), but I don't see what that has to do with the rest of the IRQ stack. --Andy
Re: [PATCH v3 45/51] x86: remove 64-byte gap at end of irq stack
On Fri, Aug 12, 2016 at 7:29 AM, Josh Poimboeuf wrote: > 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? I can't think of any reason this would matter. For that matter, do you have any idea why irq_stack_union is a union or why we insist on sticking it at %gs:0? Sure, the *canary* needs to live at a fixed offset (because GCC is daft, sigh), but I don't see what that has to do with the rest of the IRQ stack. --Andy
[PATCH v3 45/51] x86: remove 64-byte gap at end of irq stack
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--- arch/x86/include/asm/page_64_types.h | 3 --- arch/x86/kernel/cpu/common.c | 2 +- arch/x86/kernel/dumpstack_64.c | 4 ++-- arch/x86/kernel/setup_percpu.c | 2 +- 4 files changed, 4 insertions(+), 7 deletions(-) diff --git a/arch/x86/include/asm/page_64_types.h b/arch/x86/include/asm/page_64_types.h index 6256baf..3c0be3b 100644 --- a/arch/x86/include/asm/page_64_types.h +++ b/arch/x86/include/asm/page_64_types.h @@ -24,9 +24,6 @@ #define IRQ_STACK_ORDER(2 + KASAN_STACK_ORDER) #define IRQ_STACK_SIZE (PAGE_SIZE << IRQ_STACK_ORDER) -/* FIXME: why? */ -#define IRQ_USABLE_STACK_SIZE (IRQ_STACK_SIZE - 64) - #define DOUBLEFAULT_STACK 1 #define NMI_STACK 2 #define DEBUG_STACK 3 diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c index 55684b1..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_USABLE_STACK_SIZE; + 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 9f7a9f9..001a75d 100644 --- a/arch/x86/kernel/dumpstack_64.c +++ b/arch/x86/kernel/dumpstack_64.c @@ -78,7 +78,7 @@ static bool in_exception_stack(unsigned long *stack, struct stack_info *info) static bool in_irq_stack(unsigned long *stack, struct stack_info *info) { unsigned long *end = (unsigned long *)this_cpu_read(irq_stack_ptr); - unsigned long *begin = end - (IRQ_USABLE_STACK_SIZE / sizeof(long)); + unsigned long *begin = end - (IRQ_STACK_SIZE / sizeof(long)); if (stack < begin || stack >= end) return false; @@ -145,7 +145,7 @@ void show_stack_log_lvl(struct task_struct *task, struct pt_regs *regs, int i; irq_stack_end = (unsigned long *)this_cpu_read(irq_stack_ptr); - irq_stack = irq_stack_end - (IRQ_USABLE_STACK_SIZE / sizeof(long)); + irq_stack = irq_stack_end - (IRQ_STACK_SIZE / sizeof(long)); sp = sp ? : get_stack_pointer(task, regs); diff --git a/arch/x86/kernel/setup_percpu.c b/arch/x86/kernel/setup_percpu.c index a2a0eae..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_USABLE_STACK_SIZE; + IRQ_STACK_SIZE; #endif #ifdef CONFIG_NUMA per_cpu(x86_cpu_to_node_map, cpu) = -- 2.7.4
[PATCH v3 45/51] x86: remove 64-byte gap at end of irq stack
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 --- arch/x86/include/asm/page_64_types.h | 3 --- arch/x86/kernel/cpu/common.c | 2 +- arch/x86/kernel/dumpstack_64.c | 4 ++-- arch/x86/kernel/setup_percpu.c | 2 +- 4 files changed, 4 insertions(+), 7 deletions(-) diff --git a/arch/x86/include/asm/page_64_types.h b/arch/x86/include/asm/page_64_types.h index 6256baf..3c0be3b 100644 --- a/arch/x86/include/asm/page_64_types.h +++ b/arch/x86/include/asm/page_64_types.h @@ -24,9 +24,6 @@ #define IRQ_STACK_ORDER(2 + KASAN_STACK_ORDER) #define IRQ_STACK_SIZE (PAGE_SIZE << IRQ_STACK_ORDER) -/* FIXME: why? */ -#define IRQ_USABLE_STACK_SIZE (IRQ_STACK_SIZE - 64) - #define DOUBLEFAULT_STACK 1 #define NMI_STACK 2 #define DEBUG_STACK 3 diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c index 55684b1..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_USABLE_STACK_SIZE; + 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 9f7a9f9..001a75d 100644 --- a/arch/x86/kernel/dumpstack_64.c +++ b/arch/x86/kernel/dumpstack_64.c @@ -78,7 +78,7 @@ static bool in_exception_stack(unsigned long *stack, struct stack_info *info) static bool in_irq_stack(unsigned long *stack, struct stack_info *info) { unsigned long *end = (unsigned long *)this_cpu_read(irq_stack_ptr); - unsigned long *begin = end - (IRQ_USABLE_STACK_SIZE / sizeof(long)); + unsigned long *begin = end - (IRQ_STACK_SIZE / sizeof(long)); if (stack < begin || stack >= end) return false; @@ -145,7 +145,7 @@ void show_stack_log_lvl(struct task_struct *task, struct pt_regs *regs, int i; irq_stack_end = (unsigned long *)this_cpu_read(irq_stack_ptr); - irq_stack = irq_stack_end - (IRQ_USABLE_STACK_SIZE / sizeof(long)); + irq_stack = irq_stack_end - (IRQ_STACK_SIZE / sizeof(long)); sp = sp ? : get_stack_pointer(task, regs); diff --git a/arch/x86/kernel/setup_percpu.c b/arch/x86/kernel/setup_percpu.c index a2a0eae..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_USABLE_STACK_SIZE; + IRQ_STACK_SIZE; #endif #ifdef CONFIG_NUMA per_cpu(x86_cpu_to_node_map, cpu) = -- 2.7.4