Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1425846 > unrolled thread

[PATCH v2 0/6] x86: Rewrite switch_to()

Back to article view | Back to linux.kernel

Contents

  [PATCH v2 0/6] x86: Rewrite switch_to() Brian Gerst <brgerst@gmail.com> - 2016-06-18 23:00 +0200
    [PATCH v2 6/6] x86: Fix thread_saved_pc() Brian Gerst <brgerst@gmail.com> - 2016-06-18 23:00 +0200
      Re: [PATCH v2 6/6] x86: Fix thread_saved_pc() Josh Poimboeuf <jpoimboe@redhat.com> - 2016-06-20 18:10 +0200
        Re: [PATCH v2 6/6] x86: Fix thread_saved_pc() Brian Gerst <brgerst@gmail.com> - 2016-06-22 06:30 +0200
    [PATCH v2 2/6] x86-64, kgdb: clear GDB_PS on 64-bit Brian Gerst <brgerst@gmail.com> - 2016-06-18 23:00 +0200
    [PATCH v2 5/6] x86: Pass kernel thread parameters in fork_frame Brian Gerst <brgerst@gmail.com> - 2016-06-18 23:00 +0200
      Re: [PATCH v2 5/6] x86: Pass kernel thread parameters in fork_frame Andy Lutomirski <luto@amacapital.net> - 2016-06-19 23:30 +0200
        Re: [PATCH v2 5/6] x86: Pass kernel thread parameters in fork_frame Brian Gerst <brgerst@gmail.com> - 2016-06-20 00:10 +0200
      Re: [PATCH v2 5/6] x86: Pass kernel thread parameters in fork_frame Borislav Petkov <bp@suse.de> - 2016-06-20 16:00 +0200
        Re: [PATCH v2 5/6] x86: Pass kernel thread parameters in fork_frame Brian Gerst <brgerst@gmail.com> - 2016-06-20 17:20 +0200
          Re: [PATCH v2 5/6] x86: Pass kernel thread parameters in fork_frame Borislav Petkov <bp@alien8.de> - 2016-06-21 13:50 +0200
            Re: [PATCH v2 5/6] x86: Pass kernel thread parameters in fork_frame Brian Gerst <brgerst@gmail.com> - 2016-06-22 10:30 +0200
    Re: [PATCH v2 0/6] x86: Rewrite switch_to() Brian Gerst <brgerst@gmail.com> - 2016-06-20 00:10 +0200

#1425846 — [PATCH v2 0/6] x86: Rewrite switch_to()

This patch set simplifies the switch_to() code, by moving the stack switch
code out of line into an asm stub before calling __switch_to().  This ends
up being more readable, and using the C calling convention instead of
clobbering all registers improves code generation.  It also allows newly
forked processes to construct a special stack frame to seamlessly flow
to ret_from_fork, instead of using a test and branch, or an unbalanced
call/ret.

Changes from v1:
- Added struct inactive_task_frame
- Added comments about kernel threads returning to userspace
- Cleaned up some incorrect uses of thread.sp

Brian Gerst (6):
      x86-32, kgdb: Don't use thread.ip in sleeping_thread_to_gdb_regs()
      x86-64, kgdb: clear GDB_PS on 64-bit
      x86: Add struct inactive_task_frame
      x86: Rewrite switch_to() code
      x86: Pass kernel thread parameters in fork_frame
      x86: Fix thread_saved_pc()

 arch/x86/entry/entry_32.S          |  68 +++++++++++++-----
 arch/x86/entry/entry_64.S          |  76 ++++++++++++++------
 arch/x86/include/asm/processor.h   |  13 +---
 arch/x86/include/asm/stacktrace.h  |   4 +-
 arch/x86/include/asm/switch_to.h   | 142 +++++++------------------------------
 arch/x86/include/asm/thread_info.h |   2 -
 arch/x86/kernel/asm-offsets.c      |   6 ++
 arch/x86/kernel/asm-offsets_32.c   |   5 ++
 arch/x86/kernel/asm-offsets_64.c   |   5 ++
 arch/x86/kernel/kgdb.c             |   8 +--
 arch/x86/kernel/process.c          |  13 +++-
 arch/x86/kernel/process_32.c       |  29 +++-----
 arch/x86/kernel/process_64.c       |  19 ++---
 arch/x86/kernel/smpboot.c          |   1 -
 14 files changed, 187 insertions(+), 204 deletions(-)

[toc] | [next] | [standalone]

#1425847 — [PATCH v2 6/6] x86: Fix thread_saved_pc()

thread_saved_pc() was using a completely bogus method to get the return
address.  Since switch_to() was previously inlined, there was no sane way
to know where on the stack the return address was stored.  Now with the
frame of a sleeping thread well defined, this can be implemented correctly.

Signed-off-by: Brian Gerst <brgerst@gmail.com>
---
 arch/x86/include/asm/processor.h | 10 ++--------
 arch/x86/kernel/process.c        | 10 ++++++++++
 arch/x86/kernel/process_32.c     |  8 --------
 3 files changed, 12 insertions(+), 16 deletions(-)

diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index 1e7d634..413f4f1 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -716,8 +716,6 @@ static inline void spin_lock_prefetch(const void *x)
 	.io_bitmap_ptr		= NULL,					  \
 }
 
-extern unsigned long thread_saved_pc(struct task_struct *tsk);
-
 /*
  * TOP_OF_KERNEL_STACK_PADDING reserves 8 bytes on top of the ring0 stack.
  * This is necessary to guarantee that the entire "struct pt_regs"
@@ -767,17 +765,13 @@ extern unsigned long thread_saved_pc(struct task_struct *tsk);
 	.sp0 = TOP_OF_INIT_STACK \
 }
 
-/*
- * Return saved PC of a blocked thread.
- * What is this good for? it will be always the scheduler or ret_from_fork.
- */
-#define thread_saved_pc(t)	READ_ONCE_NOCHECK(*(unsigned long *)((t)->thread.sp - 8))
-
 #define task_pt_regs(tsk)	((struct pt_regs *)(tsk)->thread.sp0 - 1)
 extern unsigned long KSTK_ESP(struct task_struct *task);
 
 #endif /* CONFIG_X86_64 */
 
+extern unsigned long thread_saved_pc(struct task_struct *tsk);
+
 extern void start_thread(struct pt_regs *regs, unsigned long new_ip,
 					       unsigned long new_sp);
 
diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index 00ebab0..db458c4 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -513,6 +513,16 @@ unsigned long arch_randomize_brk(struct mm_struct *mm)
 }
 
 /*
+ * Return saved PC of a blocked thread.
+ */
+unsigned long thread_saved_pc(struct task_struct *tsk)
+{
+	struct inactive_task_frame *frame =
+		(struct inactive_task_frame *) READ_ONCE(tsk->thread.sp);
+	return READ_ONCE_NOCHECK(frame->ret_addr);
+}
+
+/*
  * Called from fs/proc with a reference on @p to find the function
  * which called into schedule(). This needs to be done carefully
  * because the task might wake up and we might look at a stack
diff --git a/arch/x86/kernel/process_32.c b/arch/x86/kernel/process_32.c
index acee6ca..bcf0a38 100644
--- a/arch/x86/kernel/process_32.c
+++ b/arch/x86/kernel/process_32.c
@@ -58,14 +58,6 @@
 asmlinkage void ret_from_fork(void) __asm__("ret_from_fork");
 asmlinkage void ret_from_kernel_thread(void) __asm__("ret_from_kernel_thread");
 
-/*
- * Return saved PC of a blocked thread.
- */
-unsigned long thread_saved_pc(struct task_struct *tsk)
-{
-	return ((unsigned long *)tsk->thread.sp)[3];
-}
-
 void __show_regs(struct pt_regs *regs, int all)
 {
 	unsigned long cr0 = 0L, cr2 = 0L, cr3 = 0L, cr4 = 0L;
-- 
2.5.5

[toc] | [prev] | [next] | [standalone]

#1426757 — Re: [PATCH v2 6/6] x86: Fix thread_saved_pc()

On Sat, Jun 18, 2016 at 04:56:18PM -0400, Brian Gerst wrote:
> thread_saved_pc() was using a completely bogus method to get the return
> address.  Since switch_to() was previously inlined, there was no sane way
> to know where on the stack the return address was stored.  Now with the
> frame of a sleeping thread well defined, this can be implemented correctly.
> 
> Signed-off-by: Brian Gerst <brgerst@gmail.com>
> ---
>  arch/x86/include/asm/processor.h | 10 ++--------
>  arch/x86/kernel/process.c        | 10 ++++++++++
>  arch/x86/kernel/process_32.c     |  8 --------
>  3 files changed, 12 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
> index 1e7d634..413f4f1 100644
> --- a/arch/x86/include/asm/processor.h
> +++ b/arch/x86/include/asm/processor.h
> @@ -716,8 +716,6 @@ static inline void spin_lock_prefetch(const void *x)
>  	.io_bitmap_ptr		= NULL,					  \
>  }
>  
> -extern unsigned long thread_saved_pc(struct task_struct *tsk);
> -
>  /*
>   * TOP_OF_KERNEL_STACK_PADDING reserves 8 bytes on top of the ring0 stack.
>   * This is necessary to guarantee that the entire "struct pt_regs"
> @@ -767,17 +765,13 @@ extern unsigned long thread_saved_pc(struct task_struct *tsk);
>  	.sp0 = TOP_OF_INIT_STACK \
>  }
>  
> -/*
> - * Return saved PC of a blocked thread.
> - * What is this good for? it will be always the scheduler or ret_from_fork.
> - */
> -#define thread_saved_pc(t)	READ_ONCE_NOCHECK(*(unsigned long *)((t)->thread.sp - 8))
> -
>  #define task_pt_regs(tsk)	((struct pt_regs *)(tsk)->thread.sp0 - 1)
>  extern unsigned long KSTK_ESP(struct task_struct *task);
>  
>  #endif /* CONFIG_X86_64 */
>  
> +extern unsigned long thread_saved_pc(struct task_struct *tsk);
> +
>  extern void start_thread(struct pt_regs *regs, unsigned long new_ip,
>  					       unsigned long new_sp);
>  
> diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
> index 00ebab0..db458c4 100644
> --- a/arch/x86/kernel/process.c
> +++ b/arch/x86/kernel/process.c
> @@ -513,6 +513,16 @@ unsigned long arch_randomize_brk(struct mm_struct *mm)
>  }
>  
>  /*
> + * Return saved PC of a blocked thread.
> + */
> +unsigned long thread_saved_pc(struct task_struct *tsk)
> +{
> +	struct inactive_task_frame *frame =
> +		(struct inactive_task_frame *) READ_ONCE(tsk->thread.sp);
> +	return READ_ONCE_NOCHECK(frame->ret_addr);
> +}
> +
> +/*

I would agree with the above (removed) comment:

  "What is this good for?  it will be always the scheduler or ret_from_fork."

And I'd guess the same is true for all the arches which have to
implement it.  Maybe this function (and its single call site in
sched_show_task()) should just be removed altogether?

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1428403 — Re: [PATCH v2 6/6] x86: Fix thread_saved_pc()

On Mon, Jun 20, 2016 at 12:01 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> On Sat, Jun 18, 2016 at 04:56:18PM -0400, Brian Gerst wrote:
>> thread_saved_pc() was using a completely bogus method to get the return
>> address.  Since switch_to() was previously inlined, there was no sane way
>> to know where on the stack the return address was stored.  Now with the
>> frame of a sleeping thread well defined, this can be implemented correctly.
>>
>> Signed-off-by: Brian Gerst <brgerst@gmail.com>
>> ---
>>  arch/x86/include/asm/processor.h | 10 ++--------
>>  arch/x86/kernel/process.c        | 10 ++++++++++
>>  arch/x86/kernel/process_32.c     |  8 --------
>>  3 files changed, 12 insertions(+), 16 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
>> index 1e7d634..413f4f1 100644
>> --- a/arch/x86/include/asm/processor.h
>> +++ b/arch/x86/include/asm/processor.h
>> @@ -716,8 +716,6 @@ static inline void spin_lock_prefetch(const void *x)
>>       .io_bitmap_ptr          = NULL,                                   \
>>  }
>>
>> -extern unsigned long thread_saved_pc(struct task_struct *tsk);
>> -
>>  /*
>>   * TOP_OF_KERNEL_STACK_PADDING reserves 8 bytes on top of the ring0 stack.
>>   * This is necessary to guarantee that the entire "struct pt_regs"
>> @@ -767,17 +765,13 @@ extern unsigned long thread_saved_pc(struct task_struct *tsk);
>>       .sp0 = TOP_OF_INIT_STACK \
>>  }
>>
>> -/*
>> - * Return saved PC of a blocked thread.
>> - * What is this good for? it will be always the scheduler or ret_from_fork.
>> - */
>> -#define thread_saved_pc(t)   READ_ONCE_NOCHECK(*(unsigned long *)((t)->thread.sp - 8))
>> -
>>  #define task_pt_regs(tsk)    ((struct pt_regs *)(tsk)->thread.sp0 - 1)
>>  extern unsigned long KSTK_ESP(struct task_struct *task);
>>
>>  #endif /* CONFIG_X86_64 */
>>
>> +extern unsigned long thread_saved_pc(struct task_struct *tsk);
>> +
>>  extern void start_thread(struct pt_regs *regs, unsigned long new_ip,
>>                                              unsigned long new_sp);
>>
>> diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
>> index 00ebab0..db458c4 100644
>> --- a/arch/x86/kernel/process.c
>> +++ b/arch/x86/kernel/process.c
>> @@ -513,6 +513,16 @@ unsigned long arch_randomize_brk(struct mm_struct *mm)
>>  }
>>
>>  /*
>> + * Return saved PC of a blocked thread.
>> + */
>> +unsigned long thread_saved_pc(struct task_struct *tsk)
>> +{
>> +     struct inactive_task_frame *frame =
>> +             (struct inactive_task_frame *) READ_ONCE(tsk->thread.sp);
>> +     return READ_ONCE_NOCHECK(frame->ret_addr);
>> +}
>> +
>> +/*
>
> I would agree with the above (removed) comment:
>
>   "What is this good for?  it will be always the scheduler or ret_from_fork."
>
> And I'd guess the same is true for all the arches which have to
> implement it.  Maybe this function (and its single call site in
> sched_show_task()) should just be removed altogether?

I didn't really want to stray down that path with this series.  This
just makes it functional again.  the usefulness is still open for
debate.

--
Brian Gerst

[toc] | [prev] | [next] | [standalone]

#1425848 — [PATCH v2 2/6] x86-64, kgdb: clear GDB_PS on 64-bit

switch_to() no longer saves EFLAGS, so it's bogus to look for it on the
stack.  Set it to zero like 32-bit.

Signed-off-by: Brian Gerst <brgerst@gmail.com>
---
 arch/x86/kernel/kgdb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/kgdb.c b/arch/x86/kernel/kgdb.c
index fe649a5..5e3f294 100644
--- a/arch/x86/kernel/kgdb.c
+++ b/arch/x86/kernel/kgdb.c
@@ -176,7 +176,7 @@ void sleeping_thread_to_gdb_regs(unsigned long *gdb_regs, struct task_struct *p)
 	gdb_regs[GDB_FS]	= 0xFFFF;
 	gdb_regs[GDB_GS]	= 0xFFFF;
 #else
-	gdb_regs32[GDB_PS]	= *(unsigned long *)(p->thread.sp + 8);
+	gdb_regs32[GDB_PS]	= 0;
 	gdb_regs32[GDB_CS]	= __KERNEL_CS;
 	gdb_regs32[GDB_SS]	= __KERNEL_DS;
 	gdb_regs[GDB_R8]	= 0;
-- 
2.5.5

[toc] | [prev] | [next] | [standalone]

#1425849 — [PATCH v2 5/6] x86: Pass kernel thread parameters in fork_frame

Instead of setting up a fake pt_regs context, put the kernel thread
function pointer and arg into the unused callee-restored registers
of struct fork_frame.

Signed-off-by: Brian Gerst <brgerst@gmail.com>
---
 arch/x86/entry/entry_32.S    | 31 +++++++++++++++----------------
 arch/x86/entry/entry_64.S    | 35 ++++++++++++++++-------------------
 arch/x86/kernel/process_32.c | 14 ++++----------
 arch/x86/kernel/process_64.c | 10 +++-------
 4 files changed, 38 insertions(+), 52 deletions(-)

diff --git a/arch/x86/entry/entry_32.S b/arch/x86/entry/entry_32.S
index e8302b5..e8a460d 100644
--- a/arch/x86/entry/entry_32.S
+++ b/arch/x86/entry/entry_32.S
@@ -240,35 +240,34 @@ END(__switch_to_asm)
  * A newly forked process directly context switches into this address.
  *
  * eax: prev task we switched from
+ * ebx: kernel thread func
+ * edi: kernel thread arg
  */
 ENTRY(ret_from_fork)
 	pushl	%eax
 	call	schedule_tail
 	popl	%eax
 
+	testl	%ebx, %ebx
+	jnz	1f
+
+2:
 	/* When we fork, we trace the syscall return in the child, too. */
 	movl    %esp, %eax
 	call    syscall_return_slowpath
 	jmp     restore_all
-END(ret_from_fork)
-
-ENTRY(ret_from_kernel_thread)
-	pushl	%eax
-	call	schedule_tail
-	popl	%eax
-	movl	PT_EBP(%esp), %eax
-	call	*PT_EBX(%esp)
-	movl	$0, PT_EAX(%esp)
 
+	/* kernel thread */
+1:	movl	%edi, %eax
+	call	*%ebx
 	/*
-	 * Kernel threads return to userspace as if returning from a syscall.
-	 * We should check whether anything actually uses this path and, if so,
-	 * consider switching it over to ret_from_fork.
+	 * A kernel thread is allowed to return here after successfully
+	 * calling do_execve().  Exit to userspace to complete the execve()
+	 * syscall.
 	 */
-	movl    %esp, %eax
-	call    syscall_return_slowpath
-	jmp     restore_all
-ENDPROC(ret_from_kernel_thread)
+	movl	$0, PT_EAX(%esp)
+	jmp	2b
+END(ret_from_fork)
 
 /*
  * Return to user mode is not as complex as all this looks,
diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
index 7574528..23e764c 100644
--- a/arch/x86/entry/entry_64.S
+++ b/arch/x86/entry/entry_64.S
@@ -404,37 +404,34 @@ END(__switch_to_asm)
  * A newly forked process directly context switches into this address.
  *
  * rax: prev task we switched from
+ * rbx: kernel thread func
+ * r12: kernel thread arg
  */
 ENTRY(ret_from_fork)
 	movq	%rax, %rdi
 	call	schedule_tail			/* rdi: 'prev' task parameter */
 
-	testb	$3, CS(%rsp)			/* from kernel_thread? */
+	testq	%rbx, %rbx			/* from kernel_thread? */
 	jnz	1f
 
-	/*
-	 * We came from kernel_thread.  This code path is quite twisted, and
-	 * someone should clean it up.
-	 *
-	 * copy_thread_tls stashes the function pointer in RBX and the
-	 * parameter to be passed in RBP.  The called function is permitted
-	 * to call do_execve and thereby jump to user mode.
-	 */
-	movq	RBP(%rsp), %rdi
-	call	*RBX(%rsp)
-	movl	$0, RAX(%rsp)
-
-	/*
-	 * Fall through as though we're exiting a syscall.  This makes a
-	 * twisted sort of sense if we just called do_execve.
-	 */
-
-1:
+2:
 	movq	%rsp, %rdi
 	call	syscall_return_slowpath	/* returns with IRQs disabled */
 	TRACE_IRQS_ON			/* user mode is traced as IRQS on */
 	SWAPGS
 	jmp	restore_regs_and_iret
+
+1:
+	/* kernel thread */
+	movq	%r12, %rdi
+	call	*%rbx
+	/*
+	 * A kernel thread is allowed to return here after successfully
+	 * calling do_execve().  Exit to userspace to complete the execve()
+	 * syscall.
+	 */
+	movq	$0, RAX(%rsp)
+	jmp	2b
 END(ret_from_fork)
 
 /*
diff --git a/arch/x86/kernel/process_32.c b/arch/x86/kernel/process_32.c
index 81a82f5..acee6ca 100644
--- a/arch/x86/kernel/process_32.c
+++ b/arch/x86/kernel/process_32.c
@@ -139,6 +139,7 @@ int copy_thread_tls(unsigned long clone_flags, unsigned long sp,
 	int err;
 
 	frame->bp = 0;
+	frame->ret_addr = (unsigned long) ret_from_fork;
 	p->thread.sp = (unsigned long) fork_frame;
 	p->thread.sp0 = (unsigned long) (childregs+1);
 	memset(p->thread.ptrace_bps, 0, sizeof(p->thread.ptrace_bps));
@@ -146,19 +147,12 @@ int copy_thread_tls(unsigned long clone_flags, unsigned long sp,
 	if (unlikely(p->flags & PF_KTHREAD)) {
 		/* kernel thread */
 		memset(childregs, 0, sizeof(struct pt_regs));
-		frame->ret_addr = (unsigned long) ret_from_kernel_thread;
-		task_user_gs(p) = __KERNEL_STACK_CANARY;
-		childregs->ds = __USER_DS;
-		childregs->es = __USER_DS;
-		childregs->fs = __KERNEL_PERCPU;
-		childregs->bx = sp;	/* function */
-		childregs->bp = arg;
-		childregs->orig_ax = -1;
-		childregs->cs = __KERNEL_CS | get_kernel_rpl();
-		childregs->flags = X86_EFLAGS_IF | X86_EFLAGS_FIXED;
+		frame->bx = sp;		/* function */
+		frame->di = arg;
 		p->thread.io_bitmap_ptr = NULL;
 		return 0;
 	}
+	frame->bx = 0;
 	*childregs = *current_pt_regs();
 	childregs->ax = 0;
 	if (sp)
diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
index 3ac9522..479cae5f 100644
--- a/arch/x86/kernel/process_64.c
+++ b/arch/x86/kernel/process_64.c
@@ -165,15 +165,11 @@ int copy_thread_tls(unsigned long clone_flags, unsigned long sp,
 	if (unlikely(p->flags & PF_KTHREAD)) {
 		/* kernel thread */
 		memset(childregs, 0, sizeof(struct pt_regs));
-		childregs->sp = (unsigned long)childregs;
-		childregs->ss = __KERNEL_DS;
-		childregs->bx = sp; /* function */
-		childregs->bp = arg;
-		childregs->orig_ax = -1;
-		childregs->cs = __KERNEL_CS | get_kernel_rpl();
-		childregs->flags = X86_EFLAGS_IF | X86_EFLAGS_FIXED;
+		frame->bx = sp;		/* function */
+		frame->r12 = arg;
 		return 0;
 	}
+	frame->bx = 0;
 	*childregs = *current_pt_regs();
 
 	childregs->ax = 0;
-- 
2.5.5

[toc] | [prev] | [next] | [standalone]

#1426043 — Re: [PATCH v2 5/6] x86: Pass kernel thread parameters in fork_frame

On Sat, Jun 18, 2016 at 1:56 PM, Brian Gerst <brgerst@gmail.com> wrote:
> Instead of setting up a fake pt_regs context, put the kernel thread
> function pointer and arg into the unused callee-restored registers
> of struct fork_frame.

This seems generally okay.

>
> Signed-off-by: Brian Gerst <brgerst@gmail.com>

> @@ -146,19 +147,12 @@ int copy_thread_tls(unsigned long clone_flags, unsigned long sp,


>         if (unlikely(p->flags & PF_KTHREAD)) {
>                 /* kernel thread */
>                 memset(childregs, 0, sizeof(struct pt_regs));
> -               frame->ret_addr = (unsigned long) ret_from_kernel_thread;
> -               task_user_gs(p) = __KERNEL_STACK_CANARY;
> -               childregs->ds = __USER_DS;
> -               childregs->es = __USER_DS;
> -               childregs->fs = __KERNEL_PERCPU;

Is the idea that do_execve promises to initialize all these fields to
something sensible if the kernel thread in question tries to return to
user mode?

--Andy

[toc] | [prev] | [next] | [standalone]

#1426047 — Re: [PATCH v2 5/6] x86: Pass kernel thread parameters in fork_frame

On Sun, Jun 19, 2016 at 5:28 PM, Andy Lutomirski <luto@amacapital.net> wrote:
> On Sat, Jun 18, 2016 at 1:56 PM, Brian Gerst <brgerst@gmail.com> wrote:
>> Instead of setting up a fake pt_regs context, put the kernel thread
>> function pointer and arg into the unused callee-restored registers
>> of struct fork_frame.
>
> This seems generally okay.
>
>>
>> Signed-off-by: Brian Gerst <brgerst@gmail.com>
>
>> @@ -146,19 +147,12 @@ int copy_thread_tls(unsigned long clone_flags, unsigned long sp,
>
>
>>         if (unlikely(p->flags & PF_KTHREAD)) {
>>                 /* kernel thread */
>>                 memset(childregs, 0, sizeof(struct pt_regs));
>> -               frame->ret_addr = (unsigned long) ret_from_kernel_thread;
>> -               task_user_gs(p) = __KERNEL_STACK_CANARY;
>> -               childregs->ds = __USER_DS;
>> -               childregs->es = __USER_DS;
>> -               childregs->fs = __KERNEL_PERCPU;
>
> Is the idea that do_execve promises to initialize all these fields to
> something sensible if the kernel thread in question tries to return to
> user mode?
>
> --Andy

Yes, do_execve() should be setting the full pt_regs.

--
Brian Gerst

[toc] | [prev] | [next] | [standalone]

#1426625 — Re: [PATCH v2 5/6] x86: Pass kernel thread parameters in fork_frame

On Sat, Jun 18, 2016 at 04:56:17PM -0400, Brian Gerst wrote:
> Instead of setting up a fake pt_regs context, put the kernel thread
> function pointer and arg into the unused callee-restored registers
> of struct fork_frame.
> 
> Signed-off-by: Brian Gerst <brgerst@gmail.com>

> ---
>  arch/x86/entry/entry_32.S    | 31 +++++++++++++++----------------
>  arch/x86/entry/entry_64.S    | 35 ++++++++++++++++-------------------
>  arch/x86/kernel/process_32.c | 14 ++++----------
>  arch/x86/kernel/process_64.c | 10 +++-------
>  4 files changed, 38 insertions(+), 52 deletions(-)

...

> diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
> index 7574528..23e764c 100644
> --- a/arch/x86/entry/entry_64.S
> +++ b/arch/x86/entry/entry_64.S
> @@ -404,37 +404,34 @@ END(__switch_to_asm)
>   * A newly forked process directly context switches into this address.
>   *
>   * rax: prev task we switched from
> + * rbx: kernel thread func
> + * r12: kernel thread arg
>   */
>  ENTRY(ret_from_fork)
>  	movq	%rax, %rdi
>  	call	schedule_tail			/* rdi: 'prev' task parameter */
>  
> -	testb	$3, CS(%rsp)			/* from kernel_thread? */
> +	testq	%rbx, %rbx			/* from kernel_thread? */
>  	jnz	1f
>  
> -	/*
> -	 * We came from kernel_thread.  This code path is quite twisted, and
> -	 * someone should clean it up.
> -	 *
> -	 * copy_thread_tls stashes the function pointer in RBX and the
> -	 * parameter to be passed in RBP.  The called function is permitted
> -	 * to call do_execve and thereby jump to user mode.
> -	 */
> -	movq	RBP(%rsp), %rdi
> -	call	*RBX(%rsp)
> -	movl	$0, RAX(%rsp)
> -
> -	/*
> -	 * Fall through as though we're exiting a syscall.  This makes a
> -	 * twisted sort of sense if we just called do_execve.
> -	 */
> -
> -1:
> +2:
>  	movq	%rsp, %rdi
>  	call	syscall_return_slowpath	/* returns with IRQs disabled */
>  	TRACE_IRQS_ON			/* user mode is traced as IRQS on */
>  	SWAPGS
>  	jmp	restore_regs_and_iret
> +
> +1:
> +	/* kernel thread */
> +	movq	%r12, %rdi
> +	call	*%rbx
> +	/*
> +	 * A kernel thread is allowed to return here after successfully
> +	 * calling do_execve().  Exit to userspace to complete the execve()
> +	 * syscall.
> +	 */
> +	movq	$0, RAX(%rsp)
> +	jmp	2b
>  END(ret_from_fork)

Wouldn't it be simpler to reverse the JNZ after the TESTB above and thus
have a single label and simpler code flow (pasting the whole thing here
because a diff is unreadable):

ENTRY(ret_from_fork)
        movq    %rax, %rdi
        call    schedule_tail                   /* rdi: 'prev' task parameter */

        testq   %rbx, %rbx                      /* from kernel_thread? */
        jz      1f

        /* kernel thread */
        movq    %r12, %rdi
        call    *%rbx
        /*
         * A kernel thread is allowed to return here after successfully
         * calling do_execve().  Exit to userspace to complete the execve()
         * syscall.
         */
        movq    $0, RAX(%rsp)

1:
        movq    %rsp, %rdi
        call    syscall_return_slowpath /* returns with IRQs disabled */
        TRACE_IRQS_ON                   /* user mode is traced as IRQS on */
        SWAPGS
        jmp     restore_regs_and_iret
END(ret_from_fork)

It boots fine in my guest this way too.

Ditto for 32-bit.

-- 
Regards/Gruss,
    Boris.

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

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--

[toc] | [prev] | [next] | [standalone]

#1426683 — Re: [PATCH v2 5/6] x86: Pass kernel thread parameters in fork_frame

On Mon, Jun 20, 2016 at 9:51 AM, Borislav Petkov <bp@suse.de> wrote:
> On Sat, Jun 18, 2016 at 04:56:17PM -0400, Brian Gerst wrote:
>> Instead of setting up a fake pt_regs context, put the kernel thread
>> function pointer and arg into the unused callee-restored registers
>> of struct fork_frame.
>>
>> Signed-off-by: Brian Gerst <brgerst@gmail.com>
>
>> ---
>>  arch/x86/entry/entry_32.S    | 31 +++++++++++++++----------------
>>  arch/x86/entry/entry_64.S    | 35 ++++++++++++++++-------------------
>>  arch/x86/kernel/process_32.c | 14 ++++----------
>>  arch/x86/kernel/process_64.c | 10 +++-------
>>  4 files changed, 38 insertions(+), 52 deletions(-)
>
> ...
>
>> diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
>> index 7574528..23e764c 100644
>> --- a/arch/x86/entry/entry_64.S
>> +++ b/arch/x86/entry/entry_64.S
>> @@ -404,37 +404,34 @@ END(__switch_to_asm)
>>   * A newly forked process directly context switches into this address.
>>   *
>>   * rax: prev task we switched from
>> + * rbx: kernel thread func
>> + * r12: kernel thread arg
>>   */
>>  ENTRY(ret_from_fork)
>>       movq    %rax, %rdi
>>       call    schedule_tail                   /* rdi: 'prev' task parameter */
>>
>> -     testb   $3, CS(%rsp)                    /* from kernel_thread? */
>> +     testq   %rbx, %rbx                      /* from kernel_thread? */
>>       jnz     1f
>>
>> -     /*
>> -      * We came from kernel_thread.  This code path is quite twisted, and
>> -      * someone should clean it up.
>> -      *
>> -      * copy_thread_tls stashes the function pointer in RBX and the
>> -      * parameter to be passed in RBP.  The called function is permitted
>> -      * to call do_execve and thereby jump to user mode.
>> -      */
>> -     movq    RBP(%rsp), %rdi
>> -     call    *RBX(%rsp)
>> -     movl    $0, RAX(%rsp)
>> -
>> -     /*
>> -      * Fall through as though we're exiting a syscall.  This makes a
>> -      * twisted sort of sense if we just called do_execve.
>> -      */
>> -
>> -1:
>> +2:
>>       movq    %rsp, %rdi
>>       call    syscall_return_slowpath /* returns with IRQs disabled */
>>       TRACE_IRQS_ON                   /* user mode is traced as IRQS on */
>>       SWAPGS
>>       jmp     restore_regs_and_iret
>> +
>> +1:
>> +     /* kernel thread */
>> +     movq    %r12, %rdi
>> +     call    *%rbx
>> +     /*
>> +      * A kernel thread is allowed to return here after successfully
>> +      * calling do_execve().  Exit to userspace to complete the execve()
>> +      * syscall.
>> +      */
>> +     movq    $0, RAX(%rsp)
>> +     jmp     2b
>>  END(ret_from_fork)
>
> Wouldn't it be simpler to reverse the JNZ after the TESTB above and thus
> have a single label and simpler code flow (pasting the whole thing here
> because a diff is unreadable):
>
> ENTRY(ret_from_fork)
>         movq    %rax, %rdi
>         call    schedule_tail                   /* rdi: 'prev' task parameter */
>
>         testq   %rbx, %rbx                      /* from kernel_thread? */
>         jz      1f
>
>         /* kernel thread */
>         movq    %r12, %rdi
>         call    *%rbx
>         /*
>          * A kernel thread is allowed to return here after successfully
>          * calling do_execve().  Exit to userspace to complete the execve()
>          * syscall.
>          */
>         movq    $0, RAX(%rsp)
>
> 1:
>         movq    %rsp, %rdi
>         call    syscall_return_slowpath /* returns with IRQs disabled */
>         TRACE_IRQS_ON                   /* user mode is traced as IRQS on */
>         SWAPGS
>         jmp     restore_regs_and_iret
> END(ret_from_fork)
>
> It boots fine in my guest this way too.

The idea was to put the uncommon case (kernel thread) out of line for
performance reasons.

--
Brian Gerst

[toc] | [prev] | [next] | [standalone]

#1427661 — Re: [PATCH v2 5/6] x86: Pass kernel thread parameters in fork_frame

On Mon, Jun 20, 2016 at 11:01:02AM -0400, Brian Gerst wrote:
> The idea was to put the uncommon case (kernel thread) out of line for
> performance reasons.

A comment saying so wouldn't hurt...

-- 
Regards/Gruss,
    Boris.

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

[toc] | [prev] | [next] | [standalone]

#1428543 — Re: [PATCH v2 5/6] x86: Pass kernel thread parameters in fork_frame

On Mon, Jun 20, 2016 at 11:14 AM, Borislav Petkov <bp@alien8.de> wrote:
> On Mon, Jun 20, 2016 at 11:01:02AM -0400, Brian Gerst wrote:
>> The idea was to put the uncommon case (kernel thread) out of line for
>> performance reasons.
>
> A comment saying so wouldn't hurt...

This is a fairly common pattern.  Do we have to document every case of it?

--
Brian Gerst

[toc] | [prev] | [next] | [standalone]

#1426046

On Sat, Jun 18, 2016 at 4:56 PM, Brian Gerst <brgerst@gmail.com> wrote:
> This patch set simplifies the switch_to() code, by moving the stack switch
> code out of line into an asm stub before calling __switch_to().  This ends
> up being more readable, and using the C calling convention instead of
> clobbering all registers improves code generation.  It also allows newly
> forked processes to construct a special stack frame to seamlessly flow
> to ret_from_fork, instead of using a test and branch, or an unbalanced
> call/ret.
>
> Changes from v1:
> - Added struct inactive_task_frame
> - Added comments about kernel threads returning to userspace
> - Cleaned up some incorrect uses of thread.sp

I forgot to also add:
- Rearranged inactive stack frame so that BP (frame pointer) is in the
natural position right below the return address.  This should take
care of unwinding issues Josh raised.

--
Brian Gerst

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web