[PATCH 2/4] x86: entry_64.S: remove stub_iopl

[PATCH 2/4] x86: entry_64.S: remove stub_iopl

[Denys Vlasenko]

stub_iopl is no longer needed: pt_regs->flags needs no fixing up
after previous change. Removing it.

Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
CC: Linus Torvalds <torvalds@linux-foundation.org>
CC: Steven Rostedt <rostedt@goodmis.org>
CC: Ingo Molnar <mingo@kernel.org>
CC: Borislav Petkov <bp@alien8.de>
CC: "H. Peter Anvin" <hpa@zytor.com>
CC: Oleg Nesterov <oleg@redhat.com>
CC: Frederic Weisbecker <fweisbec@gmail.com>
CC: Alexei Starovoitov <ast@plumgrid.com>
CC: Will Drewry <wad@chromium.org>
CC: Kees Cook <keescook@chromium.org>
CC: x86@kernel.org
CC: linux-kernel@vger.kernel.org
---
 arch/x86/kernel/entry_64.S       | 13 -------------
 arch/x86/syscalls/syscall_64.tbl |  2 +-
 arch/x86/um/sys_call_table_64.c  |  2 +-
 3 files changed, 2 insertions(+), 15 deletions(-)

diff --git a/arch/x86/kernel/entry_64.S b/arch/x86/kernel/entry_64.S
index 324200a..703ced0 100644
--- a/arch/x86/kernel/entry_64.S
+++ b/arch/x86/kernel/entry_64.S
@@ -421,22 +421,9 @@ ENTRY(stub_\func)
 END(stub_\func)
 	.endm
 
-	.macro FIXED_FRAME label,func
-ENTRY(\label)
-	CFI_STARTPROC
-	DEFAULT_FRAME 0, 8		/* offset 8: return address */
-	FIXUP_TOP_OF_STACK %r11, 8
-	call \func
-	RESTORE_TOP_OF_STACK %r11, 8
-	ret
-	CFI_ENDPROC
-END(\label)
-	.endm
-
 	FORK_LIKE  clone
 	FORK_LIKE  fork
 	FORK_LIKE  vfork
-	FIXED_FRAME stub_iopl, sys_iopl
 
 ENTRY(stub_execve)
 	CFI_STARTPROC
diff --git a/arch/x86/syscalls/syscall_64.tbl b/arch/x86/syscalls/syscall_64.tbl
index 8d656fb..9ef32d5 100644
--- a/arch/x86/syscalls/syscall_64.tbl
+++ b/arch/x86/syscalls/syscall_64.tbl
@@ -178,7 +178,7 @@
 169	common	reboot			sys_reboot
 170	common	sethostname		sys_sethostname
 171	common	setdomainname		sys_setdomainname
-172	common	iopl			stub_iopl
+172	common	iopl			sys_iopl
 173	common	ioperm			sys_ioperm
 174	64	create_module
 175	common	init_module		sys_init_module
diff --git a/arch/x86/um/sys_call_table_64.c b/arch/x86/um/sys_call_table_64.c
index 5cdfa9d..a75d8700 100644
--- a/arch/x86/um/sys_call_table_64.c
+++ b/arch/x86/um/sys_call_table_64.c
@@ -16,7 +16,7 @@
  */
 
 /* Not going to be implemented by UML, since we have no hardware. */
-#define stub_iopl sys_ni_syscall
+#define sys_iopl sys_ni_syscall
 #define sys_ioperm sys_ni_syscall
 
 /*
-- 
1.8.1.4

[PATCH 4/4] x86: entry_64.S: remove unused thread_struct::usersp

[Denys Vlasenko]

All manipulations of PER_CPU(old_rsp) in C code are removed:
it is not used on SYSRET return, storing anything there is pointless.

This also allows to get rid of thread_struct::usersp,
which was needed only to set PER_CPU(old_rsp) for
correct return from fork/clone.

Tweak a few comments (we no longer have "partial stack frame", ever).

Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
CC: Linus Torvalds <torvalds@linux-foundation.org>
CC: Steven Rostedt <rostedt@goodmis.org>
CC: Ingo Molnar <mingo@kernel.org>
CC: Borislav Petkov <bp@alien8.de>
CC: "H. Peter Anvin" <hpa@zytor.com>
CC: Oleg Nesterov <oleg@redhat.com>
CC: Frederic Weisbecker <fweisbec@gmail.com>
CC: Alexei Starovoitov <ast@plumgrid.com>
CC: Will Drewry <wad@chromium.org>
CC: Kees Cook <keescook@chromium.org>
CC: x86@kernel.org
CC: linux-kernel@vger.kernel.org
---
 arch/x86/include/asm/processor.h | 6 ------
 arch/x86/kernel/entry_64.S       | 4 ++--
 arch/x86/kernel/process_64.c     | 5 -----
 3 files changed, 2 insertions(+), 13 deletions(-)

diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index 48a61c1..c77605d 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -478,7 +478,6 @@ struct thread_struct {
 #ifdef CONFIG_X86_32
 	unsigned long		sysenter_cs;
 #else
-	unsigned long		usersp;	/* Copy from PDA */
 	unsigned short		es;
 	unsigned short		ds;
 	unsigned short		fsindex;
@@ -894,11 +893,6 @@ extern unsigned long thread_saved_pc(struct task_struct *tsk);
 #define task_pt_regs(tsk)	((struct pt_regs *)(tsk)->thread.sp0 - 1)
 extern unsigned long KSTK_ESP(struct task_struct *task);
 
-/*
- * User space RSP while inside the SYSCALL fast path
- */
-DECLARE_PER_CPU(unsigned long, old_rsp);
-
 #endif /* CONFIG_X86_64 */
 
 extern void start_thread(struct pt_regs *regs, unsigned long new_ip,
diff --git a/arch/x86/kernel/entry_64.S b/arch/x86/kernel/entry_64.S
index d86788c..9e37b36 100644
--- a/arch/x86/kernel/entry_64.S
+++ b/arch/x86/kernel/entry_64.S
@@ -303,7 +303,7 @@ int_ret_from_sys_call_fixup:
 	FIXUP_TOP_OF_STACK %r11
 	jmp int_ret_from_sys_call
 
-	/* Do syscall tracing */
+	/* Do syscall entry tracing */
 tracesys:
 	movq %rsp, %rdi
 	movq $AUDIT_ARCH_X86_64, %rsi
@@ -343,7 +343,7 @@ tracesys_phase2:
 
 /*
  * Syscall return path ending with IRET.
- * Has correct top of stack, but partial stack frame.
+ * Has correct iret frame.
  */
 GLOBAL(int_ret_from_sys_call)
 	DISABLE_INTERRUPTS(CLBR_NONE)
diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
index e8c124a..14df2be 100644
--- a/arch/x86/kernel/process_64.c
+++ b/arch/x86/kernel/process_64.c
@@ -161,7 +161,6 @@ int copy_thread(unsigned long clone_flags, unsigned long sp,
 	p->thread.sp0 = (unsigned long)task_stack_page(p) + THREAD_SIZE;
 	childregs = task_pt_regs(p);
 	p->thread.sp = (unsigned long) childregs;
-	p->thread.usersp = me->thread.usersp;
 	set_tsk_thread_flag(p, TIF_FORK);
 	p->thread.io_bitmap_ptr = NULL;
 
@@ -235,10 +234,8 @@ start_thread_common(struct pt_regs *regs, unsigned long new_ip,
 	loadsegment(es, _ds);
 	loadsegment(ds, _ds);
 	load_gs_index(0);
-	current->thread.usersp	= new_sp;
 	regs->ip		= new_ip;
 	regs->sp		= new_sp;
-	this_cpu_write(old_rsp, new_sp);
 	regs->cs		= _cs;
 	regs->ss		= _ss;
 	regs->flags		= X86_EFLAGS_IF;
@@ -398,8 +395,6 @@ __switch_to(struct task_struct *prev_p, struct task_struct *next_p)
 	/*
 	 * Switch the PDA and FPU contexts.
 	 */
-	prev->usersp = this_cpu_read(old_rsp);
-	this_cpu_write(old_rsp, next->usersp);
 	this_cpu_write(current_task, next_p);
 
 	/*
-- 
1.8.1.4

Re: [PATCH 2/4] x86: entry_64.S: remove stub_iopl

[Borislav Petkov]

On Tue, Mar 10, 2015 at 11:45:06AM +0100, Denys Vlasenko wrote:
> stub_iopl is no longer needed: pt_regs->flags needs no fixing up
> after previous change. Removing it.

Can we flesh out "previous change" a bit more detailed here please?

When looking at this patch months if not years from now, people would be
scratching head about the "previous change".

Thanks.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH 4/4] x86: entry_64.S: remove unused thread_struct::usersp

[Borislav Petkov]

On Tue, Mar 10, 2015 at 11:45:07AM +0100, Denys Vlasenko wrote:
> @@ -894,11 +893,6 @@ extern unsigned long thread_saved_pc(struct task_struct *tsk);
>  #define task_pt_regs(tsk)	((struct pt_regs *)(tsk)->thread.sp0 - 1)
>  extern unsigned long KSTK_ESP(struct task_struct *task);
>  
> -/*
> - * User space RSP while inside the SYSCALL fast path
> - */
> -DECLARE_PER_CPU(unsigned long, old_rsp);

Please grep the whole arch/x86/ tree for old_rsp as there are more
leftovers.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH 4/4] x86: entry_64.S: remove unused thread_struct::usersp

[Denys Vlasenko]

On 03/11/2015 01:55 PM, Borislav Petkov wrote:
> On Tue, Mar 10, 2015 at 11:45:07AM +0100, Denys Vlasenko wrote:
>> @@ -894,11 +893,6 @@ extern unsigned long thread_saved_pc(struct task_struct *tsk);
>>  #define task_pt_regs(tsk)	((struct pt_regs *)(tsk)->thread.sp0 - 1)
>>  extern unsigned long KSTK_ESP(struct task_struct *task);
>>  
>> -/*
>> - * User space RSP while inside the SYSCALL fast path
>> - */
>> -DECLARE_PER_CPU(unsigned long, old_rsp);
> 
> Please grep the whole arch/x86/ tree for old_rsp as there are more
> leftovers.

I think I did:

$ grep -r old_rsp arch/x86
arch/x86/xen/xen-asm_64.S:	movq %rsp, PER_CPU_VAR(old_rsp)
arch/x86/xen/xen-asm_64.S:	pushq PER_CPU_VAR(old_rsp)
arch/x86/xen/xen-asm_64.S:	movq %rsp, PER_CPU_VAR(old_rsp)
arch/x86/xen/xen-asm_64.S:	pushq PER_CPU_VAR(old_rsp)
arch/x86/kernel/process_64.c:__visible DEFINE_PER_CPU(unsigned long, old_rsp);
arch/x86/kernel/entry_64.S:	movq	%rsp,PER_CPU_VAR(old_rsp)
arch/x86/kernel/entry_64.S:	movq	PER_CPU_VAR(old_rsp),%rcx
arch/x86/kernel/entry_64.S:	/* 0(%rsp): old_rsp */

The only remaining use in C code is the definition in process_64.c
It is necessary for assembly (.S) files.

[tip:x86/asm] x86/asm/entry/64: Remove stub_iopl

[tip-bot for Denys Vlasenko]

Commit-ID:  616ab249f1e42f6135642183529f910fcedc2642
Gitweb:     http://git.kernel.org/tip/616ab249f1e42f6135642183529f910fcedc2642
Author:     Denys Vlasenko <dvlasenk@redhat.com>
AuthorDate: Tue, 10 Mar 2015 11:45:06 +0100
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Tue, 10 Mar 2015 13:56:10 +0100

x86/asm/entry/64: Remove stub_iopl

stub_iopl is no longer needed: pt_regs->flags needs no fixing up
after previous change. Remove it.

Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
Cc: Alexei Starovoitov <ast@plumgrid.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Will Drewry <wad@chromium.org>
Link: http://lkml.kernel.org/r/1425984307-2143-1-git-send-email-dvlasenk@redhat.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/kernel/entry_64.S       | 13 -------------
 arch/x86/syscalls/syscall_64.tbl |  2 +-
 arch/x86/um/sys_call_table_64.c  |  2 +-
 3 files changed, 2 insertions(+), 15 deletions(-)

diff --git a/arch/x86/kernel/entry_64.S b/arch/x86/kernel/entry_64.S
index 324200a..703ced0 100644
--- a/arch/x86/kernel/entry_64.S
+++ b/arch/x86/kernel/entry_64.S
@@ -421,22 +421,9 @@ ENTRY(stub_\func)
 END(stub_\func)
 	.endm
 
-	.macro FIXED_FRAME label,func
-ENTRY(\label)
-	CFI_STARTPROC
-	DEFAULT_FRAME 0, 8		/* offset 8: return address */
-	FIXUP_TOP_OF_STACK %r11, 8
-	call \func
-	RESTORE_TOP_OF_STACK %r11, 8
-	ret
-	CFI_ENDPROC
-END(\label)
-	.endm
-
 	FORK_LIKE  clone
 	FORK_LIKE  fork
 	FORK_LIKE  vfork
-	FIXED_FRAME stub_iopl, sys_iopl
 
 ENTRY(stub_execve)
 	CFI_STARTPROC
diff --git a/arch/x86/syscalls/syscall_64.tbl b/arch/x86/syscalls/syscall_64.tbl
index 8d656fb..9ef32d5 100644
--- a/arch/x86/syscalls/syscall_64.tbl
+++ b/arch/x86/syscalls/syscall_64.tbl
@@ -178,7 +178,7 @@
 169	common	reboot			sys_reboot
 170	common	sethostname		sys_sethostname
 171	common	setdomainname		sys_setdomainname
-172	common	iopl			stub_iopl
+172	common	iopl			sys_iopl
 173	common	ioperm			sys_ioperm
 174	64	create_module
 175	common	init_module		sys_init_module
diff --git a/arch/x86/um/sys_call_table_64.c b/arch/x86/um/sys_call_table_64.c
index 5cdfa9d..a75d8700 100644
--- a/arch/x86/um/sys_call_table_64.c
+++ b/arch/x86/um/sys_call_table_64.c
@@ -16,7 +16,7 @@
  */
 
 /* Not going to be implemented by UML, since we have no hardware. */
-#define stub_iopl sys_ni_syscall
+#define sys_iopl sys_ni_syscall
 #define sys_ioperm sys_ni_syscall
 
 /*

[tip:x86/asm] x86/asm/entry/64: Remove unused thread_struct:: usersp

[tip-bot for Denys Vlasenko]

Commit-ID:  245214a155c711764b3853189441c9f8aeb058b3
Gitweb:     http://git.kernel.org/tip/245214a155c711764b3853189441c9f8aeb058b3
Author:     Denys Vlasenko <dvlasenk@redhat.com>
AuthorDate: Tue, 10 Mar 2015 11:45:07 +0100
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Tue, 10 Mar 2015 13:56:11 +0100

x86/asm/entry/64: Remove unused thread_struct::usersp

All manipulations of PER_CPU(old_rsp) in C code are removed:
it is not used on SYSRET return, so storing anything there is
pointless.

This also allows us to get rid of thread_struct::usersp,
which was needed only to set PER_CPU(old_rsp) for correct
return from fork/clone.

Tweak a few comments as well: we no longer have "partial stack frame",
ever.

Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
Cc: Alexei Starovoitov <ast@plumgrid.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Will Drewry <wad@chromium.org>
Link: http://lkml.kernel.org/r/1425984307-2143-2-git-send-email-dvlasenk@redhat.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/include/asm/processor.h | 6 ------
 arch/x86/kernel/entry_64.S       | 4 ++--
 arch/x86/kernel/process_64.c     | 5 -----
 3 files changed, 2 insertions(+), 13 deletions(-)

diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index 48a61c1..c77605d 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -478,7 +478,6 @@ struct thread_struct {
 #ifdef CONFIG_X86_32
 	unsigned long		sysenter_cs;
 #else
-	unsigned long		usersp;	/* Copy from PDA */
 	unsigned short		es;
 	unsigned short		ds;
 	unsigned short		fsindex;
@@ -894,11 +893,6 @@ extern unsigned long thread_saved_pc(struct task_struct *tsk);
 #define task_pt_regs(tsk)	((struct pt_regs *)(tsk)->thread.sp0 - 1)
 extern unsigned long KSTK_ESP(struct task_struct *task);
 
-/*
- * User space RSP while inside the SYSCALL fast path
- */
-DECLARE_PER_CPU(unsigned long, old_rsp);
-
 #endif /* CONFIG_X86_64 */
 
 extern void start_thread(struct pt_regs *regs, unsigned long new_ip,
diff --git a/arch/x86/kernel/entry_64.S b/arch/x86/kernel/entry_64.S
index d86788c..9e37b36 100644
--- a/arch/x86/kernel/entry_64.S
+++ b/arch/x86/kernel/entry_64.S
@@ -303,7 +303,7 @@ int_ret_from_sys_call_fixup:
 	FIXUP_TOP_OF_STACK %r11
 	jmp int_ret_from_sys_call
 
-	/* Do syscall tracing */
+	/* Do syscall entry tracing */
 tracesys:
 	movq %rsp, %rdi
 	movq $AUDIT_ARCH_X86_64, %rsi
@@ -343,7 +343,7 @@ tracesys_phase2:
 
 /*
  * Syscall return path ending with IRET.
- * Has correct top of stack, but partial stack frame.
+ * Has correct iret frame.
  */
 GLOBAL(int_ret_from_sys_call)
 	DISABLE_INTERRUPTS(CLBR_NONE)
diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
index e8c124a..14df2be 100644
--- a/arch/x86/kernel/process_64.c
+++ b/arch/x86/kernel/process_64.c
@@ -161,7 +161,6 @@ int copy_thread(unsigned long clone_flags, unsigned long sp,
 	p->thread.sp0 = (unsigned long)task_stack_page(p) + THREAD_SIZE;
 	childregs = task_pt_regs(p);
 	p->thread.sp = (unsigned long) childregs;
-	p->thread.usersp = me->thread.usersp;
 	set_tsk_thread_flag(p, TIF_FORK);
 	p->thread.io_bitmap_ptr = NULL;
 
@@ -235,10 +234,8 @@ start_thread_common(struct pt_regs *regs, unsigned long new_ip,
 	loadsegment(es, _ds);
 	loadsegment(ds, _ds);
 	load_gs_index(0);
-	current->thread.usersp	= new_sp;
 	regs->ip		= new_ip;
 	regs->sp		= new_sp;
-	this_cpu_write(old_rsp, new_sp);
 	regs->cs		= _cs;
 	regs->ss		= _ss;
 	regs->flags		= X86_EFLAGS_IF;
@@ -398,8 +395,6 @@ __switch_to(struct task_struct *prev_p, struct task_struct *next_p)
 	/*
 	 * Switch the PDA and FPU contexts.
 	 */
-	prev->usersp = this_cpu_read(old_rsp);
-	this_cpu_write(old_rsp, next->usersp);
 	this_cpu_write(current_task, next_p);
 
 	/*

Re: [tip:x86/asm] x86/asm/entry/64: Remove unused thread_struct:: usersp

[Borislav Petkov]

On Mon, Mar 16, 2015 at 05:05:53AM -0700, tip-bot for Denys Vlasenko wrote:
> Commit-ID:  245214a155c711764b3853189441c9f8aeb058b3
> Gitweb:     http://git.kernel.org/tip/245214a155c711764b3853189441c9f8aeb058b3
> Author:     Denys Vlasenko <dvlasenk@redhat.com>
> AuthorDate: Tue, 10 Mar 2015 11:45:07 +0100
> Committer:  Ingo Molnar <mingo@kernel.org>
> CommitDate: Tue, 10 Mar 2015 13:56:11 +0100
> 
> x86/asm/entry/64: Remove unused thread_struct::usersp
> 
> All manipulations of PER_CPU(old_rsp) in C code are removed:
> it is not used on SYSRET return, so storing anything there is
> pointless.
> 
> This also allows us to get rid of thread_struct::usersp,
> which was needed only to set PER_CPU(old_rsp) for correct
> return from fork/clone.
> 
> Tweak a few comments as well: we no longer have "partial stack frame",
> ever.
> 
> Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
> Cc: Alexei Starovoitov <ast@plumgrid.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Frederic Weisbecker <fweisbec@gmail.com>
> Cc: H. Peter Anvin <hpa@zytor.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Oleg Nesterov <oleg@redhat.com>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Will Drewry <wad@chromium.org>
> Link: http://lkml.kernel.org/r/1425984307-2143-2-git-send-email-dvlasenk@redhat.com
> Signed-off-by: Ingo Molnar <mingo@kernel.org>

So this patch is causing all kinds of segfaults when booting my kvm
guest here, see below.

Reverting it makes the segfaults go away but from looking at the patch,
I have no idea why it would even cause those segfaults.

[    5.285547] kmod[1316]: segfault at 738c08 ip 0000000000738c08 sp 00007ffdb6079c68 error 15
[    9.537606] tput[2716]: segfault at 0 ip           (null) sp 00007fffffffdbd0 error 14 in tput[400000+3000]
					  ^^^^^^^^^^^^^^^^^

Looks like rIP has went off somewhere in the weeds.

Hmmm...

[    4.593374] grep[998]: segfault at 7ffc3a9f4378 ip 00007fb8409fe1df sp 00007ffc3a9f4378 error 4 in ld-2.13.so[7fb8409e8000+20000]
[    4.593374] grep[998]: segfault at 7ffc3a9f4378 ip 00007fb8409fe1df sp 00007ffc3a9f4378 error 4 in ld-2.13.so[7fb8409e8000+20000]

[    7.160423] sed[1999]: segfault at 7ffe9998f778 ip 00007f37deef0b52 sp 00007ffe9998f778 error 4 in libc-2.13.so[7f37dee18000+182000]

[    4.593374] grep[998]: segfault at 7ffc3a9f4378 ip 00007fb8409fe1df sp 00007ffc3a9f4378 error 4 in ld-2.13.so[7fb8409e8000+20000]
[    7.160423] sed[1999]: segfault at 7ffe9998f778 ip 00007f37deef0b52 sp 00007ffe9998f778 error 4 in libc-2.13.so[7f37dee18000+182000]

[    4.593374] grep[998]: segfault at 7ffc3a9f4378 ip 00007fb8409fe1df sp 00007ffc3a9f4378 error 4 in ld-2.13.so[7fb8409e8000+20000]
[    7.160423] sed[1999]: segfault at 7ffe9998f778 ip 00007f37deef0b52 sp 00007ffe9998f778 error 4 in libc-2.13.so[7f37dee18000+182000]
[    5.607611] sed[1350]: segfault at 7ffddd4a4bf0 ip 00007ff24a11fafc sp 00007ffddd4a4bf0 error 4 in libc-2.13.so[7ff24a050000+182000]
[    9.820987] update-exim4.co[2755]: segfault at 7ffff79ab000 ip 00007ffff79ab000 sp 00007fffffffe278 error 15
[   10.580362] tput[3060]: segfault at 7ffff6376cb0 ip 00007ffff7df3422 sp 00007ffff6376cb0 error 4 in ld-2.13.so[7ffff7ddd000+20000]

[    5.607611] sed[1350]: segfault at 7ffddd4a4bf0 ip 00007ff24a11fafc sp 00007ffddd4a4bf0 error 4 in libc-2.13.so[7ff24a050000+182000]
[    9.820987] update-exim4.co[2755]: segfault at 7ffff79ab000 ip 00007ffff79ab000 sp 00007fffffffe278 error 15
[   10.580362] tput[3060]: segfault at 7ffff6376cb0 ip 00007ffff7df3422 sp 00007ffff6376cb0 error 4 in ld-2.13.so[7ffff7ddd000+20000]

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [tip:x86/asm] x86/asm/entry/64: Remove unused thread_struct::usersp

[Denys Vlasenko]

On 03/16/2015 05:47 PM, Borislav Petkov wrote:
> On Mon, Mar 16, 2015 at 05:05:53AM -0700, tip-bot for Denys Vlasenko wrote:
>> Commit-ID:  245214a155c711764b3853189441c9f8aeb058b3
>> Gitweb:     http://git.kernel.org/tip/245214a155c711764b3853189441c9f8aeb058b3
>> Author:     Denys Vlasenko <dvlasenk@redhat.com>
>> AuthorDate: Tue, 10 Mar 2015 11:45:07 +0100
>> Committer:  Ingo Molnar <mingo@kernel.org>
>> CommitDate: Tue, 10 Mar 2015 13:56:11 +0100
>>
>> x86/asm/entry/64: Remove unused thread_struct::usersp
>>
>> All manipulations of PER_CPU(old_rsp) in C code are removed:
>> it is not used on SYSRET return, so storing anything there is
>> pointless.
>>
>> This also allows us to get rid of thread_struct::usersp,
>> which was needed only to set PER_CPU(old_rsp) for correct
>> return from fork/clone.
>>
>> Tweak a few comments as well: we no longer have "partial stack frame",
>> ever.
>>
>> Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
>> Cc: Alexei Starovoitov <ast@plumgrid.com>
>> Cc: Borislav Petkov <bp@alien8.de>
>> Cc: Frederic Weisbecker <fweisbec@gmail.com>
>> Cc: H. Peter Anvin <hpa@zytor.com>
>> Cc: Kees Cook <keescook@chromium.org>
>> Cc: Linus Torvalds <torvalds@linux-foundation.org>
>> Cc: Oleg Nesterov <oleg@redhat.com>
>> Cc: Steven Rostedt <rostedt@goodmis.org>
>> Cc: Will Drewry <wad@chromium.org>
>> Link: http://lkml.kernel.org/r/1425984307-2143-2-git-send-email-dvlasenk@redhat.com
>> Signed-off-by: Ingo Molnar <mingo@kernel.org>
> 
> So this patch is causing all kinds of segfaults when booting my kvm
> guest here, see below.

I built defconfig kernel from tip, and tested it again under qemu-kvm.
Works for me with and without this change.

What's your config? What distro do you run in the guest?

> Reverting it makes the segfaults go away but from looking at the patch,
> I have no idea why it would even cause those segfaults.

Yep. This is one of those cases where "it must be completely safe"...

> [    5.285547] kmod[1316]: segfault at 738c08 ip 0000000000738c08 sp 00007ffdb6079c68 error 15
> [    9.537606] tput[2716]: segfault at 0 ip           (null) sp 00007fffffffdbd0 error 14 in tput[400000+3000]
> 					  ^^^^^^^^^^^^^^^^^
> 
> Looks like rIP has went off somewhere in the weeds.
> Hmmm...
> 
> [    4.593374] grep[998]: segfault at 7ffc3a9f4378 ip 00007fb8409fe1df sp 00007ffc3a9f4378 error 4 in ld-2.13.so[7fb8409e8000+20000]
> [    4.593374] grep[998]: segfault at 7ffc3a9f4378 ip 00007fb8409fe1df sp 00007ffc3a9f4378 error 4 in ld-2.13.so[7fb8409e8000+20000]
> 
> [    7.160423] sed[1999]: segfault at 7ffe9998f778 ip 00007f37deef0b52 sp 00007ffe9998f778 error 4 in libc-2.13.so[7f37dee18000+182000]
> 
> [    4.593374] grep[998]: segfault at 7ffc3a9f4378 ip 00007fb8409fe1df sp 00007ffc3a9f4378 error 4 in ld-2.13.so[7fb8409e8000+20000]
> [    7.160423] sed[1999]: segfault at 7ffe9998f778 ip 00007f37deef0b52 sp 00007ffe9998f778 error 4 in libc-2.13.so[7f37dee18000+182000]
> 
> [    4.593374] grep[998]: segfault at 7ffc3a9f4378 ip 00007fb8409fe1df sp 00007ffc3a9f4378 error 4 in ld-2.13.so[7fb8409e8000+20000]
> [    7.160423] sed[1999]: segfault at 7ffe9998f778 ip 00007f37deef0b52 sp 00007ffe9998f778 error 4 in libc-2.13.so[7f37dee18000+182000]
> [    5.607611] sed[1350]: segfault at 7ffddd4a4bf0 ip 00007ff24a11fafc sp 00007ffddd4a4bf0 error 4 in libc-2.13.so[7ff24a050000+182000]

This does not look entirely random.
Can you take a look what's at those locations in ld-2.13.so and libc-2.13.so?

Re: [tip:x86/asm] x86/asm/entry/64: Remove unused thread_struct::usersp

[Borislav Petkov]

[-- Attachment #1: Type: text/plain, Size: 4790 bytes --]

On Mon, Mar 16, 2015 at 11:20:38PM +0100, Denys Vlasenko wrote:
> What's your config?

Attached.

> What distro do you run in the guest?

Some old debian. Here's the full qemu command line:

$ qemu-system-x86_64
-enable-kvm
-gdb tcp::1234
-cpu Opteron_G5
-m 2048
-hda /home/boris/kvm/debian/sid-x86_64.img
-boot menu=off,order=c
-localtime
-net nic,model=rtl8139
-net user,hostfwd=tcp::1235-:22
-usbdevice tablet
-kernel /home/boris/kernel/linux-2.6/arch/x86/boot/bzImage
-append "root=/dev/sda1 debug ignore_loglevel log_buf_len=16M earlyprintk=ttyS0,115200 console=ttyS0,115200 console=tty0 "
-monitor pty
-virtfs local,path=/tmp,mount_tag=tmp,security_model=none
-serial file:/home/boris/kvm/test-x86_64-1235.log
-snapshot
-name "Debian x86_64:1235"
-smp 2

> Yep. This is one of those cases where "it must be completely safe"...

Yap.

> > [    5.285547] kmod[1316]: segfault at 738c08 ip 0000000000738c08 sp 00007ffdb6079c68 error 15
> > [    9.537606] tput[2716]: segfault at 0 ip           (null) sp 00007fffffffdbd0 error 14 in tput[400000+3000]
> > 					  ^^^^^^^^^^^^^^^^^
> > 
> > Looks like rIP has went off somewhere in the weeds.
> > Hmmm...
> > 
> > [    4.593374] grep[998]: segfault at 7ffc3a9f4378 ip 00007fb8409fe1df sp 00007ffc3a9f4378 error 4 in ld-2.13.so[7fb8409e8000+20000]
> > [    4.593374] grep[998]: segfault at 7ffc3a9f4378 ip 00007fb8409fe1df sp 00007ffc3a9f4378 error 4 in ld-2.13.so[7fb8409e8000+20000]
> > 
> > [    7.160423] sed[1999]: segfault at 7ffe9998f778 ip 00007f37deef0b52 sp 00007ffe9998f778 error 4 in libc-2.13.so[7f37dee18000+182000]
> > 
> > [    4.593374] grep[998]: segfault at 7ffc3a9f4378 ip 00007fb8409fe1df sp 00007ffc3a9f4378 error 4 in ld-2.13.so[7fb8409e8000+20000]
> > [    7.160423] sed[1999]: segfault at 7ffe9998f778 ip 00007f37deef0b52 sp 00007ffe9998f778 error 4 in libc-2.13.so[7f37dee18000+182000]
> > 
> > [    4.593374] grep[998]: segfault at 7ffc3a9f4378 ip 00007fb8409fe1df sp 00007ffc3a9f4378 error 4 in ld-2.13.so[7fb8409e8000+20000]
> > [    7.160423] sed[1999]: segfault at 7ffe9998f778 ip 00007f37deef0b52 sp 00007ffe9998f778 error 4 in libc-2.13.so[7f37dee18000+182000]
> > [    5.607611] sed[1350]: segfault at 7ffddd4a4bf0 ip 00007ff24a11fafc sp 00007ffddd4a4bf0 error 4 in libc-2.13.so[7ff24a050000+182000]
> 
> This does not look entirely random.
> Can you take a look what's at those locations in ld-2.13.so and libc-2.13.so?

The interesting thing is that the segfaulting address is the stack
pointer.

Let me see if I'd get the math right:

[    4.593374] grep[998]: segfault at 7ffc3a9f4378 ip 00007fb8409fe1df sp 00007ffc3a9f4378 error 4 in ld-2.13.so[7fb8409e8000+20000]

0x7fb8409fe1df - 0x7fb8409e8000 = 0x161df

   161cf:       90                      nop
   161d0:       b8 02 00 00 00          mov    $0x2,%eax
   161d5:       0f 05                   syscall
   161d7:       48 3d 01 f0 ff ff       cmp    $0xfffffffffffff001,%rax
   161dd:       73 01                   jae    161e0 <calloc+0xb60>
   161df:       c3                      retq
   161e0:       48 8d 0d 9d af 20 00    lea    0x20af9d(%rip),%rcx        # 221184 <_rtld_global+0x1144>
   161e7:       31 d2                   xor    %edx,%edx
   161e9:       48 29 c2                sub    %rax,%rdx
   161ec:       89 11                   mov    %edx,(%rcx)
   161ee:       48 83 c8 ff             or     $0xffffffffffffffff,%rax
   161f2:       eb eb                   jmp    161df <calloc+0xb5f>

Interesting, RETQ. So this pops RIP from the stack and we segfault at
the stack address. syscall 2 looks like open().

The next segfault line above is the same.




[    7.160423] sed[1999]: segfault at 7ffe9998f778 ip 00007f37deef0b52 sp 00007ffe9998f778 error 4 in libc-2.13.so[7f37dee18000+182000]

0x7f37deef0b52 - 0x7f37dee18000 = 0xd8b52

Whoops, RETQ again:

00000000000d8b40 <mmap>:
   d8b40:       49 89 ca                mov    %rcx,%r10
   d8b43:       b8 09 00 00 00          mov    $0x9,%eax
   d8b48:       0f 05                   syscall
   d8b4a:       48 3d 01 f0 ff ff       cmp    $0xfffffffffffff001,%rax
   d8b50:       73 01                   jae    d8b53 <mmap+0x13>
   d8b52:       c3                      retq
   d8b53:       48 8b 0d be c2 2a 00    mov    0x2ac2be(%rip),%rcx        # 384e18 <_IO_file_jumps+0x918>
   d8b5a:       31 d2                   xor    %edx,%edx
   d8b5c:       48 29 c2                sub    %rax,%rdx
   d8b5f:       64 89 11                mov    %edx,%fs:(%rcx)
   d8b62:       48 83 c8 ff             or     $0xffffffffffffffff,%rax
   d8b66:       eb ea                   jmp    d8b52 <mmap+0x12>

This time syscall 9, mmap.

So for some reason we manage to corrupt the stack after some syscalls...

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 20769 bytes --]

Re: [tip:x86/asm] x86/asm/entry/64: Remove unused thread_struct::usersp

[Ingo Molnar]

* Borislav Petkov <bp@alien8.de> wrote:

> On Mon, Mar 16, 2015 at 11:20:38PM +0100, Denys Vlasenko wrote:
> > What's your config?
> 
> Attached.

Could you try just this patch (on top of broken tip:master):

diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index 66a1954439ea..997e6a1c288f 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -496,6 +496,7 @@ struct thread_struct {
 #ifdef CONFIG_X86_32
 	unsigned long		sysenter_cs;
 #else
+	unsigned long		usersp;	/* Copy from PDA */
 	unsigned short		es;
 	unsigned short		ds;
 	unsigned short		fsindex;

to see whether it's the changed values of old_rsp, or the change in 
sizeof(thread_struct), that causes the regression?

Thanks,

	Ingo

Re: [tip:x86/asm] x86/asm/entry/64: Remove unused thread_struct::usersp

[Ingo Molnar]

* Ingo Molnar <mingo@kernel.org> wrote:

> Could you try just this patch (on top of broken tip:master):
> 
> diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
> index 66a1954439ea..997e6a1c288f 100644
> --- a/arch/x86/include/asm/processor.h
> +++ b/arch/x86/include/asm/processor.h
> @@ -496,6 +496,7 @@ struct thread_struct {
>  #ifdef CONFIG_X86_32
>  	unsigned long		sysenter_cs;
>  #else
> +	unsigned long		usersp;	/* Copy from PDA */
>  	unsigned short		es;
>  	unsigned short		ds;
>  	unsigned short		fsindex;
> 
> to see whether it's the changed values of old_rsp, or the change in 
> sizeof(thread_struct), that causes the regression?

Assuming this does not fix the regression, could you apply the minimal 
patch below - which reverts the old_rsp handling change.

(The rest of the commit are in a third patch, but those are only 
comment changes.)

So my theory is that this change is what will revert the regression.

Thanks,

	Ingo

======================>

diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index 66a1954439ea..997e6a1c288f 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -907,6 +908,11 @@ extern unsigned long thread_saved_pc(struct task_struct *tsk);
 #define task_pt_regs(tsk)	((struct pt_regs *)(tsk)->thread.sp0 - 1)
 extern unsigned long KSTK_ESP(struct task_struct *task);
 
+/*
+ * User space RSP while inside the SYSCALL fast path
+ */
+DECLARE_PER_CPU(unsigned long, old_rsp);
+
 #endif /* CONFIG_X86_64 */
 
 extern void start_thread(struct pt_regs *regs, unsigned long new_ip,
diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
index 14df2be4711f..e8c124a1f885 100644
--- a/arch/x86/kernel/process_64.c
+++ b/arch/x86/kernel/process_64.c
@@ -161,6 +161,7 @@ int copy_thread(unsigned long clone_flags, unsigned long sp,
 	p->thread.sp0 = (unsigned long)task_stack_page(p) + THREAD_SIZE;
 	childregs = task_pt_regs(p);
 	p->thread.sp = (unsigned long) childregs;
+	p->thread.usersp = me->thread.usersp;
 	set_tsk_thread_flag(p, TIF_FORK);
 	p->thread.io_bitmap_ptr = NULL;
 
@@ -234,8 +235,10 @@ start_thread_common(struct pt_regs *regs, unsigned long new_ip,
 	loadsegment(es, _ds);
 	loadsegment(ds, _ds);
 	load_gs_index(0);
+	current->thread.usersp	= new_sp;
 	regs->ip		= new_ip;
 	regs->sp		= new_sp;
+	this_cpu_write(old_rsp, new_sp);
 	regs->cs		= _cs;
 	regs->ss		= _ss;
 	regs->flags		= X86_EFLAGS_IF;
@@ -395,6 +398,8 @@ __switch_to(struct task_struct *prev_p, struct task_struct *next_p)
 	/*
 	 * Switch the PDA and FPU contexts.
 	 */
+	prev->usersp = this_cpu_read(old_rsp);
+	this_cpu_write(old_rsp, next->usersp);
 	this_cpu_write(current_task, next_p);
 
 	/*

Re: [tip:x86/asm] x86/asm/entry/64: Remove unused thread_struct::usersp

[Borislav Petkov]

On Tue, Mar 17, 2015 at 08:21:18AM +0100, Ingo Molnar wrote:
> Assuming this does not fix the regression, could you apply the minimal 
> patch below - which reverts the old_rsp handling change.
> 
> (The rest of the commit are in a third patch, but those are only 
> comment changes.)
> 
> So my theory is that this change is what will revert the regression.

Yep, it does. Below is the diff that works (it is the rough revert
without the comments :-)):

---
diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index 66a1954439ea..e39bf83cb55b 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -496,6 +496,7 @@ struct thread_struct {
 #ifdef CONFIG_X86_32
 	unsigned long		sysenter_cs;
 #else
+	unsigned long           usersp; /* Copy from PDA */
 	unsigned short		es;
 	unsigned short		ds;
 	unsigned short		fsindex;
@@ -907,6 +908,11 @@ extern unsigned long thread_saved_pc(struct task_struct *tsk);
 #define task_pt_regs(tsk)	((struct pt_regs *)(tsk)->thread.sp0 - 1)
 extern unsigned long KSTK_ESP(struct task_struct *task);
 
+/*
+ * User space RSP while inside the SYSCALL fast path
+ */
+DECLARE_PER_CPU(unsigned long, old_rsp);
+
 #endif /* CONFIG_X86_64 */
 
 extern void start_thread(struct pt_regs *regs, unsigned long new_ip,
diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
index 14df2be4711f..e8c124a1f885 100644
--- a/arch/x86/kernel/process_64.c
+++ b/arch/x86/kernel/process_64.c
@@ -161,6 +161,7 @@ int copy_thread(unsigned long clone_flags, unsigned long sp,
 	p->thread.sp0 = (unsigned long)task_stack_page(p) + THREAD_SIZE;
 	childregs = task_pt_regs(p);
 	p->thread.sp = (unsigned long) childregs;
+	p->thread.usersp = me->thread.usersp;
 	set_tsk_thread_flag(p, TIF_FORK);
 	p->thread.io_bitmap_ptr = NULL;
 
@@ -234,8 +235,10 @@ start_thread_common(struct pt_regs *regs, unsigned long new_ip,
 	loadsegment(es, _ds);
 	loadsegment(ds, _ds);
 	load_gs_index(0);
+	current->thread.usersp	= new_sp;
 	regs->ip		= new_ip;
 	regs->sp		= new_sp;
+	this_cpu_write(old_rsp, new_sp);
 	regs->cs		= _cs;
 	regs->ss		= _ss;
 	regs->flags		= X86_EFLAGS_IF;
@@ -395,6 +398,8 @@ __switch_to(struct task_struct *prev_p, struct task_struct *next_p)
 	/*
 	 * Switch the PDA and FPU contexts.
 	 */
+	prev->usersp = this_cpu_read(old_rsp);
+	this_cpu_write(old_rsp, next->usersp);
 	this_cpu_write(current_task, next_p);
 
 	/*

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [tip:x86/asm] x86/asm/entry/64: Remove unused thread_struct::usersp

[Ingo Molnar]

* Ingo Molnar <mingo@kernel.org> wrote:

> Assuming this does not fix the regression, could you apply the 
> minimal patch below - which reverts the old_rsp handling change.

Assuming this solves the regression (it really should, it's now 
equivalent to a full revert minus comments):

> @@ -395,6 +398,8 @@ __switch_to(struct task_struct *prev_p, struct task_struct *next_p)
>  	/*
>  	 * Switch the PDA and FPU contexts.
>  	 */
> +	prev->usersp = this_cpu_read(old_rsp);
> +	this_cpu_write(old_rsp, next->usersp);
>  	this_cpu_write(current_task, next_p);
>  
>  	/*

can you confirm that your guest (sometimes) uses SYSENTER to do 
syscalls?

If yes then my theory is that we broke SYSENTER (or SYSEXIT) support - 
and that this would not be visible in our normal tests of KVM because 
SYSCALL is used most of the time.

Thanks,

	Ingo

Re: [tip:x86/asm] x86/asm/entry/64: Remove unused thread_struct::usersp

[Borislav Petkov]

On Tue, Mar 17, 2015 at 08:51:39AM +0100, Ingo Molnar wrote:
> can you confirm that your guest (sometimes) uses SYSENTER to do 
> syscalls?

I don't think so - in both dumps of libc.so and ld.so, we have

SYSCALL... RET -> <segfault>

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [tip:x86/asm] x86/asm/entry/64: Remove unused thread_struct::usersp

[Ingo Molnar]

* Borislav Petkov <bp@alien8.de> wrote:

> On Tue, Mar 17, 2015 at 08:51:39AM +0100, Ingo Molnar wrote:
> > can you confirm that your guest (sometimes) uses SYSENTER to do 
> > syscalls?
> 
> I don't think so - in both dumps of libc.so and ld.so, we have
> 
> SYSCALL... RET -> <segfault>

Ok, in any case I'm doing a rebase of the affected commits in 
tip:x86/asm. That's a tree where we don't want to break bisectability, 
and this breakage looks sufficiently mysterious.

Thanks,

	Ingo

Re: [tip:x86/asm] x86/asm/entry/64: Remove unused thread_struct::usersp

[Borislav Petkov]

On Tue, Mar 17, 2015 at 09:27:36AM +0100, Ingo Molnar wrote:
> Ok, in any case I'm doing a rebase of the affected commits in
> tip:x86/asm. That's a tree where we don't want to break bisectability,
> and this breakage looks sufficiently mysterious.

Yeah.

And it keeps getting better and better. I added some debug output to the
PF-path to see why exactly we segfault and the guest booted fine! No
segfaults. So add timing to that failure :-(

---
diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
index ede025fb46f1..54ca8539f345 100644
--- a/arch/x86/mm/fault.c
+++ b/arch/x86/mm/fault.c
@@ -746,6 +746,9 @@ show_signal_msg(struct pt_regs *regs, unsigned long error_code,
 	print_vma_addr(KERN_CONT " in ", regs->ip);
 
 	printk(KERN_CONT "\n");
+
+	dump_stack();
+
 }
 
 static void
@@ -827,8 +830,10 @@ __bad_area(struct pt_regs *regs, unsigned long error_code,
 }
 
 static noinline void
-bad_area(struct pt_regs *regs, unsigned long error_code, unsigned long address)
+bad_area(const char *where, struct pt_regs *regs, unsigned long error_code,
+	 unsigned long address)
 {
+	pr_emerg("%s: %s\n", __func__, where);
 	__bad_area(regs, error_code, address, SEGV_MAPERR);
 }
 
@@ -1189,13 +1194,13 @@ retry:
 
 	vma = find_vma(mm, address);
 	if (unlikely(!vma)) {
-		bad_area(regs, error_code, address);
+		bad_area("!vma", regs, error_code, address);
 		return;
 	}
 	if (likely(vma->vm_start <= address))
 		goto good_area;
 	if (unlikely(!(vma->vm_flags & VM_GROWSDOWN))) {
-		bad_area(regs, error_code, address);
+		bad_area("!VM_GROWSDOWN", regs, error_code, address);
 		return;
 	}
 	if (error_code & PF_USER) {
@@ -1206,12 +1211,12 @@ retry:
 		 * 32 pointers and then decrements %sp by 65535.)
 		 */
 		if (unlikely(address + 65536 + 32 * sizeof(unsigned long) < regs->sp)) {
-			bad_area(regs, error_code, address);
+			bad_area("65536", regs, error_code, address);
 			return;
 		}
 	}
 	if (unlikely(expand_stack(vma, address))) {
-		bad_area(regs, error_code, address);
+		bad_area("expand_stack", regs, error_code, address);
 		return;
 	}
 
diff --git a/mm/mmap.c b/mm/mmap.c
index da9990acc08b..47c4321f0d18 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -2225,13 +2225,17 @@ int expand_downwards(struct vm_area_struct *vma,
 	 * We must make sure the anon_vma is allocated
 	 * so that the anon_vma locking is not a noop.
 	 */
-	if (unlikely(anon_vma_prepare(vma)))
+	if (unlikely(anon_vma_prepare(vma))) {
+		pr_err("anon_vma_prepare\n");
 		return -ENOMEM;
+	}
 
 	address &= PAGE_MASK;
 	error = security_mmap_addr(address);
-	if (error)
+	if (error) {
+		pr_err("security_mmap_addr\n");
 		return error;
+	}
 
 	vma_lock_anon_vma(vma);
 
@@ -2278,6 +2282,7 @@ int expand_downwards(struct vm_area_struct *vma,
 	vma_unlock_anon_vma(vma);
 	khugepaged_enter_vma_merge(vma, vma->vm_flags);
 	validate_mm(vma->vm_mm);
+	pr_err("validate_mm: %d\n", error);
 	return error;
 }
 
@@ -2326,11 +2331,15 @@ int expand_stack(struct vm_area_struct *vma, unsigned long address)
 {
 	struct vm_area_struct *prev;
 
+	pr_err("%s: address: 0x%lx\n", __func__, address);
+
 	address &= PAGE_MASK;
 	prev = vma->vm_prev;
 	if (prev && prev->vm_end == address) {
-		if (!(prev->vm_flags & VM_GROWSDOWN))
+		if (!(prev->vm_flags & VM_GROWSDOWN)) {
+			pr_err("!(prev->vm_flags & VM_GROWSDOWN)\n");
 			return -ENOMEM;
+		}
 	}
 	return expand_downwards(vma, address);
 }

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [tip:x86/asm] x86/asm/entry/64: Remove unused thread_struct::usersp

[Denys Vlasenko]

On 03/17/2015 08:39 AM, Borislav Petkov wrote:
> On Tue, Mar 17, 2015 at 08:21:18AM +0100, Ingo Molnar wrote:
>> Assuming this does not fix the regression, could you apply the minimal 
>> patch below - which reverts the old_rsp handling change.
>>
>> (The rest of the commit are in a third patch, but those are only 
>> comment changes.)
>>
>> So my theory is that this change is what will revert the regression.
> 
> Yep, it does. Below is the diff that works (it is the rough revert
> without the comments :-)):
> 
...
> @@ -395,6 +398,8 @@ __switch_to(struct task_struct *prev_p, struct task_struct *next_p)
>  	/*
>  	 * Switch the PDA and FPU contexts.
>  	 */
> +	prev->usersp = this_cpu_read(old_rsp);
> +	this_cpu_write(old_rsp, next->usersp);

I have a theory. There is a time window when user's sp
is in PER_CPU_VAR(old_rsp) but not yet in pt_regs->sp,
and *interrupts are enabled*:

ENTRY(system_call)
        SWAPGS_UNSAFE_STACK
        movq    %rsp,PER_CPU_VAR(old_rsp)
        movq    PER_CPU_VAR(kernel_stack),%rsp
        ENABLE_INTERRUPTS(CLBR_NONE)
        ALLOC_PT_GPREGS_ON_STACK 8              /* +8: space for orig_ax */
        movq    %rcx,RIP(%rsp)
        movq    PER_CPU_VAR(old_rsp),%rcx
      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
        movq    %r11,EFLAGS(%rsp)
        movq    %rcx,RSP(%rsp)

Before indicated insn, interrupts are already enabled.
If preempt would hit now, next task can clobber PER_CPU_VAR(old_rsp).
Then, when we return to this task, a bogus user's sp will be stored
in pt_regs, restores on exit to userspace, and next attempt
to, say, execute RETQ will try to pop a bogus, likely noncanonical
address into RIP -> #GP -> SEGV!

The theory can be tested by just moving interrupt enable a bit down:

ENTRY(system_call)
        SWAPGS_UNSAFE_STACK
        movq    %rsp,PER_CPU_VAR(old_rsp)
        movq    PER_CPU_VAR(kernel_stack),%rsp
-       ENABLE_INTERRUPTS(CLBR_NONE)
        ALLOC_PT_GPREGS_ON_STACK 8              /* +8: space for orig_ax */
        movq    %rcx,RIP(%rsp)
        movq    PER_CPU_VAR(old_rsp),%rcx
        movq    %r11,EFLAGS(%rsp)
        movq    %rcx,RSP(%rsp)
+       ENABLE_INTERRUPTS(CLBR_NONE)

If I'm right, segfaults should be gone.
Borislav, can you try this?

Re: [tip:x86/asm] x86/asm/entry/64: Remove unused thread_struct::usersp

[Denys Vlasenko]

On 03/17/2015 01:22 PM, Denys Vlasenko wrote:
> On 03/17/2015 08:39 AM, Borislav Petkov wrote:
>> On Tue, Mar 17, 2015 at 08:21:18AM +0100, Ingo Molnar wrote:
>>> Assuming this does not fix the regression, could you apply the minimal 
>>> patch below - which reverts the old_rsp handling change.
>>>
>>> (The rest of the commit are in a third patch, but those are only 
>>> comment changes.)
>>>
>>> So my theory is that this change is what will revert the regression.
>>
>> Yep, it does. Below is the diff that works (it is the rough revert
>> without the comments :-)):
>>
> ...
>> @@ -395,6 +398,8 @@ __switch_to(struct task_struct *prev_p, struct task_struct *next_p)
>>  	/*
>>  	 * Switch the PDA and FPU contexts.
>>  	 */
>> +	prev->usersp = this_cpu_read(old_rsp);
>> +	this_cpu_write(old_rsp, next->usersp);
> 
> I have a theory. There is a time window when user's sp
> is in PER_CPU_VAR(old_rsp) but not yet in pt_regs->sp,
> and *interrupts are enabled*:
> 
> ENTRY(system_call)
>         SWAPGS_UNSAFE_STACK
>         movq    %rsp,PER_CPU_VAR(old_rsp)
>         movq    PER_CPU_VAR(kernel_stack),%rsp
>         ENABLE_INTERRUPTS(CLBR_NONE)
>         ALLOC_PT_GPREGS_ON_STACK 8              /* +8: space for orig_ax */
>         movq    %rcx,RIP(%rsp)
>         movq    PER_CPU_VAR(old_rsp),%rcx
>       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>         movq    %r11,EFLAGS(%rsp)
>         movq    %rcx,RSP(%rsp)
> 
> Before indicated insn, interrupts are already enabled.
> If preempt would hit now, next task can clobber PER_CPU_VAR(old_rsp).
> Then, when we return to this task, a bogus user's sp will be stored
> in pt_regs, restores on exit to userspace, and next attempt
> to, say, execute RETQ will try to pop a bogus, likely noncanonical
> address into RIP -> #GP -> SEGV!
> 
> The theory can be tested by just moving interrupt enable a bit down:
> 
> ENTRY(system_call)
>         SWAPGS_UNSAFE_STACK
>         movq    %rsp,PER_CPU_VAR(old_rsp)
>         movq    PER_CPU_VAR(kernel_stack),%rsp
> -       ENABLE_INTERRUPTS(CLBR_NONE)
>         ALLOC_PT_GPREGS_ON_STACK 8              /* +8: space for orig_ax */
>         movq    %rcx,RIP(%rsp)
>         movq    PER_CPU_VAR(old_rsp),%rcx
>         movq    %r11,EFLAGS(%rsp)
>         movq    %rcx,RSP(%rsp)
> +       ENABLE_INTERRUPTS(CLBR_NONE)
> 
> If I'm right, segfaults should be gone.
> Borislav, can you try this?

I managed to reproduce the segfault, and the fix shown above works.

I see that Ingo removed the failing commit from his tree.

I'll send two patches: one which moves ENABLE_INTERRUPTS(CLBR_NONE) down,
and another which tries to remove thread_struct::usersp again.