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

Groups > linux.kernel > #1404859 > unrolled thread

[PATCH 0/4] x86: Rewrite switch_to()

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/4] x86: Rewrite switch_to() Brian Gerst <brgerst@gmail.com> - 2016-05-21 18:10 +0200
    [PATCH 4/4] x86: Pass kernel thread parameters in fork_frame Brian Gerst <brgerst@gmail.com> - 2016-05-21 18:10 +0200
      Re: [PATCH 4/4] x86: Pass kernel thread parameters in fork_frame Andy Lutomirski <luto@amacapital.net> - 2016-05-22 20:10 +0200
        Re: [PATCH 4/4] x86: Pass kernel thread parameters in fork_frame Brian Gerst <brgerst@gmail.com> - 2016-05-22 21:30 +0200
      Re: [PATCH 4/4] x86: Pass kernel thread parameters in fork_frame Josh Poimboeuf <jpoimboe@redhat.com> - 2016-05-23 17:30 +0200
        Re: [PATCH 4/4] x86: Pass kernel thread parameters in fork_frame Andy Lutomirski <luto@amacapital.net> - 2016-05-23 17:40 +0200
          Re: [PATCH 4/4] x86: Pass kernel thread parameters in fork_frame Brian Gerst <brgerst@gmail.com> - 2016-05-23 23:10 +0200
    [PATCH 1/4] x86: Save return value from kernel_thread Brian Gerst <brgerst@gmail.com> - 2016-05-21 18:10 +0200
      Re: [PATCH 1/4] x86: Save return value from kernel_thread Andy Lutomirski <luto@amacapital.net> - 2016-05-22 03:50 +0200
        Re: [PATCH 1/4] x86: Save return value from kernel_thread Brian Gerst <brgerst@gmail.com> - 2016-05-22 04:20 +0200

#1404859 — [PATCH 0/4] 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.

[PATCH 1/4] x86: Save return value from kernel_thread
[PATCH 2/4] x86-32, kgdb: Don't use thread.ip in
[PATCH 3/4] x86: Rewrite switch_to() code
[PATCH 4/4] x86: Pass kernel thread parameters in fork_frame

 arch/x86/entry/entry_32.S          |  68 +++++++++++++-----
 arch/x86/entry/entry_64.S          |  72 +++++++++++++------
 arch/x86/include/asm/processor.h   |   3 -
 arch/x86/include/asm/switch_to.h   | 137 ++++++-------------------------------
 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             |   3 +-
 arch/x86/kernel/process_32.c       |  19 ++---
 arch/x86/kernel/process_64.c       |  17 +++--
 arch/x86/kernel/smpboot.c          |   1 -
 12 files changed, 153 insertions(+), 185 deletions(-)

[toc] | [next] | [standalone]

#1404860 — [PATCH 4/4] 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    | 28 +++++++++++-----------------
 arch/x86/entry/entry_64.S    | 30 +++++++++++-------------------
 arch/x86/kernel/process_32.c | 15 ++++-----------
 arch/x86/kernel/process_64.c | 10 +++-------
 4 files changed, 29 insertions(+), 54 deletions(-)

diff --git a/arch/x86/entry/entry_32.S b/arch/x86/entry/entry_32.S
index 05e5340..424e8d3 100644
--- a/arch/x86/entry/entry_32.S
+++ b/arch/x86/entry/entry_32.S
@@ -241,35 +241,29 @@ 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)
+	/* kernel thread */
+1:	movl	%edi, %eax
+	call	*%ebx
 	movl	%eax, PT_EAX(%esp)
-
-	/*
-	 * 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.
-	 */
-	movl    %esp, %eax
-	call    syscall_return_slowpath
-	jmp     restore_all
-ENDPROC(ret_from_kernel_thread)
+	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 0542ad1..cc8a0ba 100644
--- a/arch/x86/entry/entry_64.S
+++ b/arch/x86/entry/entry_64.S
@@ -405,37 +405,29 @@ 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)
-	movq	%rax, 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
+	movq	%rax, 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 0ba6fdf..bba563f 100644
--- a/arch/x86/kernel/process_32.c
+++ b/arch/x86/kernel/process_32.c
@@ -138,6 +138,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) frame;
 	p->thread.sp0 = (unsigned long) (childregs+1);
 	memset(p->thread.ptrace_bps, 0, sizeof(p->thread.ptrace_bps));
@@ -145,25 +146,17 @@ 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)
 		childregs->sp = sp;
 
-	frame->ret_addr = (unsigned long) ret_from_fork;
 	task_user_gs(p) = get_user_gs(current_pt_regs());
 
 	p->thread.io_bitmap_ptr = NULL;
diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
index 9fab915..421646a 100644
--- a/arch/x86/kernel/process_64.c
+++ b/arch/x86/kernel/process_64.c
@@ -163,15 +163,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]

#1405030 — Re: [PATCH 4/4] x86: Pass kernel thread parameters in fork_frame

On Sat, May 21, 2016 at 9:04 AM, 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.
>
> Signed-off-by: Brian Gerst <brgerst@gmail.com>

Seems reasonable.  Can you make sure you explicitly test with
something that uses the usermodehelper mechanism?

--Andy

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

#1405035 — Re: [PATCH 4/4] x86: Pass kernel thread parameters in fork_frame

On Sun, May 22, 2016 at 2:01 PM, Andy Lutomirski <luto@amacapital.net> wrote:
> On Sat, May 21, 2016 at 9:04 AM, 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.
>>
>> Signed-off-by: Brian Gerst <brgerst@gmail.com>
>
> Seems reasonable.  Can you make sure you explicitly test with
> something that uses the usermodehelper mechanism?
>
> --Andy

kernel_init() does the same thing to spawn init, so it wouldn't boot otherwise.

--
Brian Gerst

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

#1405452 — Re: [PATCH 4/4] x86: Pass kernel thread parameters in fork_frame

On Sat, May 21, 2016 at 12:04:51PM -0400, Brian Gerst wrote:
> --- a/arch/x86/entry/entry_64.S
> +++ b/arch/x86/entry/entry_64.S
> @@ -405,37 +405,29 @@ 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)
> -	movq	%rax, 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
> +	movq	%rax, RAX(%rsp)
> +	jmp	2b
>  END(ret_from_fork)

It seems really surprising that a kernel thread would be returning to
user space.  It would probably be a good idea to preserve the existing
comments about that.

-- 
Josh

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

#1405461 — Re: [PATCH 4/4] x86: Pass kernel thread parameters in fork_frame

On Mon, May 23, 2016 at 8:23 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> On Sat, May 21, 2016 at 12:04:51PM -0400, Brian Gerst wrote:
>> --- a/arch/x86/entry/entry_64.S
>> +++ b/arch/x86/entry/entry_64.S
>> @@ -405,37 +405,29 @@ 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)
>> -     movq    %rax, 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
>> +     movq    %rax, RAX(%rsp)
>> +     jmp     2b
>>  END(ret_from_fork)
>
> It seems really surprising that a kernel thread would be returning to
> user space.  It would probably be a good idea to preserve the existing
> comments about that.
>

Agreed.

Which reminds me: at some point, on top of this series, we should
consider either having multiple variants of ret_from_fork or otherwise
generalizing the code.  If and when we implement CPL3 for *kernel*
code (SGX and UEFI come to mind as possible use cases), we probably
won't want to go through syscall_return_slowpath.

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

#1405646 — Re: [PATCH 4/4] x86: Pass kernel thread parameters in fork_frame

On Mon, May 23, 2016 at 11:36 AM, Andy Lutomirski <luto@amacapital.net> wrote:
> On Mon, May 23, 2016 at 8:23 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>> On Sat, May 21, 2016 at 12:04:51PM -0400, Brian Gerst wrote:
>>> --- a/arch/x86/entry/entry_64.S
>>> +++ b/arch/x86/entry/entry_64.S
>>> @@ -405,37 +405,29 @@ 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)
>>> -     movq    %rax, 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
>>> +     movq    %rax, RAX(%rsp)
>>> +     jmp     2b
>>>  END(ret_from_fork)
>>
>> It seems really surprising that a kernel thread would be returning to
>> user space.  It would probably be a good idea to preserve the existing
>> comments about that.
>>
>
> Agreed.
>
> Which reminds me: at some point, on top of this series, we should
> consider either having multiple variants of ret_from_fork or otherwise
> generalizing the code.  If and when we implement CPL3 for *kernel*
> code (SGX and UEFI come to mind as possible use cases), we probably
> won't want to go through syscall_return_slowpath.

I don't understand what you mean by CPL3 kernel code.  Do you mean
something like the VDSO where the kernel maps the code into userspace?
 Why would you want to do this?

--
Brian Gerst

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

#1404861 — [PATCH 1/4] x86: Save return value from kernel_thread

Kernel threads should always return zero on success after calling do_execve().  The
two existing cases in the kernel (kernel_init() and call_usermodehelper_exec_async())
correctly do this.  Save a few bytes by storing EAX/RAX instead of an immediate zero.
Also fix the 64-bit case which should save the full 64-bits.

Signed-off-by: Brian Gerst <brgerst@gmail.com>
---
 arch/x86/entry/entry_32.S | 2 +-
 arch/x86/entry/entry_64.S | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/entry/entry_32.S b/arch/x86/entry/entry_32.S
index 983e5d3..ee6fea0 100644
--- a/arch/x86/entry/entry_32.S
+++ b/arch/x86/entry/entry_32.S
@@ -221,7 +221,7 @@ ENTRY(ret_from_kernel_thread)
 	popl	%eax
 	movl	PT_EBP(%esp), %eax
 	call	*PT_EBX(%esp)
-	movl	$0, PT_EAX(%esp)
+	movl	%eax, PT_EAX(%esp)
 
 	/*
 	 * Kernel threads return to userspace as if returning from a syscall.
diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
index 9ee0da1..ab9f8c8 100644
--- a/arch/x86/entry/entry_64.S
+++ b/arch/x86/entry/entry_64.S
@@ -387,7 +387,7 @@ ENTRY(ret_from_fork)
 	 */
 	movq	RBP(%rsp), %rdi
 	call	*RBX(%rsp)
-	movl	$0, RAX(%rsp)
+	movq	%rax, RAX(%rsp)
 
 	/*
 	 * Fall through as though we're exiting a syscall.  This makes a
-- 
2.5.5

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

#1404906 — Re: [PATCH 1/4] x86: Save return value from kernel_thread

On Sat, May 21, 2016 at 9:04 AM, Brian Gerst <brgerst@gmail.com> wrote:
> Kernel threads should always return zero on success after calling do_execve().  The
> two existing cases in the kernel (kernel_init() and call_usermodehelper_exec_async())
> correctly do this.  Save a few bytes by storing EAX/RAX instead of an immediate zero.
> Also fix the 64-bit case which should save the full 64-bits.

Does this have any additional motivation beyond cleanup and fixing an
inconsequential bug?  I.e. does the rest of your series need this?

--Andy

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

#1404907 — Re: [PATCH 1/4] x86: Save return value from kernel_thread

On Sat, May 21, 2016 at 9:44 PM, Andy Lutomirski <luto@amacapital.net> wrote:
> On Sat, May 21, 2016 at 9:04 AM, Brian Gerst <brgerst@gmail.com> wrote:
>> Kernel threads should always return zero on success after calling do_execve().  The
>> two existing cases in the kernel (kernel_init() and call_usermodehelper_exec_async())
>> correctly do this.  Save a few bytes by storing EAX/RAX instead of an immediate zero.
>> Also fix the 64-bit case which should save the full 64-bits.
>
> Does this have any additional motivation beyond cleanup and fixing an
> inconsequential bug?  I.e. does the rest of your series need this?
>
> --Andy

It's just a minor cleanup, not necessary.

--
Brian Gerst

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web