x86: Rewrite switch_to()

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.

Quoted 4 lines · Brian Gerst

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

Match 64-bit and set gdb_regs[GDB_PC] to zero. thread.ip is always the same point in the scheduler (except for newly forked processes), and will be removed in a future patch.

Move the low-level context switch code to an out-of-line asm stub instead of using complex inline asm. This allows constructing a new stack frame for the child process to make it seamlessly flow to ret_from_fork without an extra test and branch in __switch_to(). It also improves code generation for __schedule() by using the C calling convention instead of clobbering all registers.

Signed-off-by Brian Gerst <[email protected]>

---
 arch/x86/entry/entry_32.S          |  38 ++++++++++
 arch/x86/entry/entry_64.S          |  42 +++++++++++-
 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/process_32.c       |   8 ++-
 arch/x86/kernel/process_64.c       |   7 +-
 arch/x86/kernel/smpboot.c          |   1 -
 11 files changed, 124 insertions(+), 130 deletions(-)

arch/x86/entry/entry_32.S +19 -0

index ee6fea0..05e5340 100644
@@ -204,6 +204,44 @@
        POP_GS_EX
 .endm
 
+/*
+ * %eax: prev task
+ * %edx: next task
+ */
+ENTRY(__switch_to_asm)
+       /*
+        * Save callee-saved registers
+        * This must match the order in struct fork_frame
+        * Frame pointer must be last for get_wchan
+        */
+       pushl   %ebx
+       pushl   %edi
+       pushl   %esi
+       pushl   %ebp
+
+       /* switch stack */
+       movl    %esp, TASK_threadsp(%eax)
+       movl    TASK_threadsp(%edx), %esp
+

arch/x86/entry/entry_32.S +25 -0

+END(__switch_to_asm)
 
 /*
+ * %rdi: prev task
+ * %rsi: next task
+ */
+ENTRY(__switch_to_asm)
+       /*
+        * Save callee-saved registers
+        * This must match the order in struct fork_frame
+        * Frame pointer must be last for get_wchan
+        */
+       pushq   %rbx
+       pushq   %r12
+       pushq   %r13
+       pushq   %r14
+       pushq   %r15
+       pushq   %rbp
+
+       /* switch stack */
+       movq    %rsp, TASK_threadsp(%rdi)
+       movq    TASK_threadsp(%rsi), %rsp
+
+#ifdef CONFIG_CC_STACKPROTECTOR
+       movq    TASK_stack_canary(%rsi), %rbx
+       movq    %rbx, PER_CPU_VAR(irq_stack_union)+stack_canary_offset
+#endif

arch/x86/entry/entry_32.S +5 -0

+struct fork_frame {
+       unsigned long bp;
+#ifdef CONFIG_X86_64
+       unsigned long r15;
+       unsigned long r14;

cc: Josh Poimboeuf: do you care about the exact stack layout of the bottom of the stack of an inactive task?

Quoted 7 lines · Brian Gerst

Move the low-level context switch code to an out-of-line asm stub instead of using complex inline asm. This allows constructing a new stack frame for the child process to make it seamlessly flow to ret_from_fork without an extra test and branch in __switch_to(). It also improves code generation for __schedule() by using the C calling convention instead of clobbering all registers.

I like the concept a lot.

Quoted 47 lines

Signed-off-by Brian Gerst <[email protected]>

---
 arch/x86/entry/entry_32.S          |  38 ++++++++++
 arch/x86/entry/entry_64.S          |  42 +++++++++++-
 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/process_32.c       |   8 ++-
 arch/x86/kernel/process_64.c       |   7 +-
 arch/x86/kernel/smpboot.c          |   1 -
 11 files changed, 124 insertions(+), 130 deletions(-)

arch/x86/entry/entry_32.S +23 -0

index ee6fea0..05e5340 100644
@@ -204,6 +204,44 @@
        POP_GS_EX
 .endm
 
+/*
+ * %eax: prev task
+ * %edx: next task
+ */
+ENTRY(__switch_to_asm)
+       /*
+        * Save callee-saved registers
+        * This must match the order in struct fork_frame
+        * Frame pointer must be last for get_wchan
+        */
+       pushl   %ebx
+       pushl   %edi
+       pushl   %esi
+       pushl   %ebp
+
+       /* switch stack */
+       movl    %esp, TASK_threadsp(%eax)
+       movl    TASK_threadsp(%edx), %esp
+
+#ifdef CONFIG_CC_STACKPROTECTOR
+       movl    TASK_stack_canary(%edx), %ebx
+       movl    %ebx, PER_CPU_VAR(stack_canary)+stack_canary_offset
+#endif

arch/x86/entry/entry_32.S line 235

Quoted 6 lines

arch/x86/entry/entry_32.S +6 -0

+
+       /* restore callee-saved registers */
+       popl    %ebp
+       popl    %esi
+       popl    %edi
+       popl    %ebx

This is highly, highly magical. eax and edx are prev and next, and:

arch/x86/entry/entry_32.S line 237

Quoted 2 lines

arch/x86/entry/entry_32.S +2 -0

+
+       jmp     __switch_to

leaves prev in eax. This works, but it might be worth a comment.

Quoted 27 lines

arch/x86/entry/entry_32.S +25 -0

+END(__switch_to_asm)
 
 /*
+ * %rdi: prev task
+ * %rsi: next task
+ */
+ENTRY(__switch_to_asm)
+       /*
+        * Save callee-saved registers
+        * This must match the order in struct fork_frame
+        * Frame pointer must be last for get_wchan
+        */
+       pushq   %rbx
+       pushq   %r12
+       pushq   %r13
+       pushq   %r14
+       pushq   %r15
+       pushq   %rbp
+
+       /* switch stack */
+       movq    %rsp, TASK_threadsp(%rdi)
+       movq    TASK_threadsp(%rsi), %rsp
+
+#ifdef CONFIG_CC_STACKPROTECTOR
+       movq    TASK_stack_canary(%rsi), %rbx
+       movq    %rbx, PER_CPU_VAR(irq_stack_union)+stack_canary_offset
+#endif

arch/x86/entry/entry_32.S

Quoted 10 lines

arch/x86/entry/entry_32.S +10 -0

+
+       /* restore callee-saved registers */
+       popq    %rbp
+       popq    %r15
+       popq    %r14
+       popq    %r13
+       popq    %r12
+       popq    %rbx
+
+       jmp     __switch_to

Ditto with the magic here.

Quoted 5 lines

arch/x86/entry/entry_32.S +5 -0

+struct fork_frame {
+       unsigned long bp;
+#ifdef CONFIG_X86_64
+       unsigned long r15;
+       unsigned long r14;

arch/x86/entry/entry_32.S

Quoted 10 lines

arch/x86/entry/entry_32.S +10 -0

+       unsigned long r13;
+       unsigned long r12;
+#else
+       unsigned long si;
+       unsigned long di;
+#endif
+       unsigned long bx;
+       unsigned long ret_addr;
+       struct pt_regs regs;
+};

This, like the old implementation, is very much geared to the current implementation of fork. Can you split it up:

struct inactive_task_frame {
    unsigned long bp;
    ...
    unsigned long ret_addr;
};

/* fork works by setting up the child stack so that switch_to will land at ret_from_fork with sp pointing at pt_regs */

struct fork_frame {
    struct inactive_task_frame switch_frame;
    struct pt_regs regs;
};

Then, if and when someone wants to fork into a different type of context, they can reuse this. Also, a future improved unwinder can use inactive_task_frame directly to kick off its unwind.

Quoted 61 lines · Andy Lutomirski

cc: Josh Poimboeuf: do you care about the exact stack layout of the bottom of the stack of an inactive task?

Quoted 7 lines · Brian Gerst

Move the low-level context switch code to an out-of-line asm stub instead of using complex inline asm. This allows constructing a new stack frame for the child process to make it seamlessly flow to ret_from_fork without an extra test and branch in __switch_to(). It also improves code generation for __schedule() by using the C calling convention instead of clobbering all registers.

I like the concept a lot.

Quoted 47 lines

Signed-off-by Brian Gerst <[email protected]>

---
 arch/x86/entry/entry_32.S          |  38 ++++++++++
 arch/x86/entry/entry_64.S          |  42 +++++++++++-
 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/process_32.c       |   8 ++-
 arch/x86/kernel/process_64.c       |   7 +-
 arch/x86/kernel/smpboot.c          |   1 -
 11 files changed, 124 insertions(+), 130 deletions(-)

arch/x86/entry/entry_32.S +23 -0

index ee6fea0..05e5340 100644
@@ -204,6 +204,44 @@
        POP_GS_EX
 .endm
 
+/*
+ * %eax: prev task
+ * %edx: next task
+ */
+ENTRY(__switch_to_asm)
+       /*
+        * Save callee-saved registers
+        * This must match the order in struct fork_frame
+        * Frame pointer must be last for get_wchan
+        */
+       pushl   %ebx
+       pushl   %edi
+       pushl   %esi
+       pushl   %ebp
+
+       /* switch stack */
+       movl    %esp, TASK_threadsp(%eax)
+       movl    TASK_threadsp(%edx), %esp
+
+#ifdef CONFIG_CC_STACKPROTECTOR
+       movl    TASK_stack_canary(%edx), %ebx
+       movl    %ebx, PER_CPU_VAR(stack_canary)+stack_canary_offset
+#endif

Quoted 8 lines

arch/x86/entry/entry_32.S line 235

Quoted 6 lines

arch/x86/entry/entry_32.S +6 -0

+
+       /* restore callee-saved registers */
+       popl    %ebp
+       popl    %esi
+       popl    %edi
+       popl    %ebx

This is highly, highly magical. eax and edx are prev and next, and:

What is so magical about the standard C calling convention (regarm(3) in the 32-bit case)? This just passes them right though to __switch_to().

Quoted 4 lines

arch/x86/entry/entry_32.S line 237

Quoted 2 lines

arch/x86/entry/entry_32.S +2 -0

+
+       jmp     __switch_to

leaves prev in eax. This works, but it might be worth a comment.

Not quite, __switch_to() returns 'last', not 'prev'. The previous task when this is called is not the same task when the thread wakes up.

cc: Josh Poimboeuf: do you care about the exact stack layout of the bottom of the stack of an inactive task?

Quoted 2 lines · Andy Lutomirski

cc: Josh Poimboeuf: do you care about the exact stack layout of the bottom of the stack of an inactive task?

So there's one minor issue with this patch, relating to unwinding the stack of a newly forked task. For detecting reliable stacks, the unwinder needs to unwind all the way to the syscall pt_regs to make sure the stack is sane. But for newly forked tasks, that won't be possible here because the unwinding will stop at the fork_frame instead.

So from an unwinder standpoint it might be nice for copy_thread_tls() to place a frame pointer on the stack next to the ret_from_fork return address, so that it would resemble an actual stack frame. The frame pointer could probably just be hard-coded to zero. And then the first bp in fork_frame would need to be a pointer to it instead of zero. That would make it nicely resemble the stack of any other task.

Alternatively I could teach the unwinder that if the unwinding starts at the fork_frame offset from the end of the stack page, and the saved rbp is zero, it can assume that it's a newly forked task. But that seems a little more brittle to me, as it requires the unwinder to understand more of the internal workings of the fork code.

But overall I think this patch is a really nice cleanup, and other than the above minor issue it should be fine with my reliable unwinder, since rbp is still at the top of the stack.

--
Josh

Quoted 23 lines · Josh Poimboeuf

Quoted 2 lines · Andy Lutomirski

cc: Josh Poimboeuf: do you care about the exact stack layout of the bottom of the stack of an inactive task?

So there's one minor issue with this patch, relating to unwinding the stack of a newly forked task. For detecting reliable stacks, the unwinder needs to unwind all the way to the syscall pt_regs to make sure the stack is sane. But for newly forked tasks, that won't be possible here because the unwinding will stop at the fork_frame instead.

So from an unwinder standpoint it might be nice for copy_thread_tls() to place a frame pointer on the stack next to the ret_from_fork return address, so that it would resemble an actual stack frame. The frame pointer could probably just be hard-coded to zero. And then the first bp in fork_frame would need to be a pointer to it instead of zero. That would make it nicely resemble the stack of any other task.

Alternatively I could teach the unwinder that if the unwinding starts at the fork_frame offset from the end of the stack page, and the saved rbp is zero, it can assume that it's a newly forked task. But that seems a little more brittle to me, as it requires the unwinder to understand more of the internal workings of the fork code.

Quoted 3 lines

But overall I think this patch is a really nice cleanup, and other than the above minor issue it should be fine with my reliable unwinder, since rbp is still at the top of the stack.

Is this a regression or is there some reason that it works right without the patch?

In any event, whatever we settle on for general pt_regs unwinding should work for this, too.

Quoted 24 lines · Andy Lutomirski

Quoted 23 lines · Josh Poimboeuf

Quoted 2 lines · Andy Lutomirski

cc: Josh Poimboeuf: do you care about the exact stack layout of the bottom of the stack of an inactive task?

So there's one minor issue with this patch, relating to unwinding the stack of a newly forked task. For detecting reliable stacks, the unwinder needs to unwind all the way to the syscall pt_regs to make sure the stack is sane. But for newly forked tasks, that won't be possible here because the unwinding will stop at the fork_frame instead.

So from an unwinder standpoint it might be nice for copy_thread_tls() to place a frame pointer on the stack next to the ret_from_fork return address, so that it would resemble an actual stack frame. The frame pointer could probably just be hard-coded to zero. And then the first bp in fork_frame would need to be a pointer to it instead of zero. That would make it nicely resemble the stack of any other task.

Alternatively I could teach the unwinder that if the unwinding starts at the fork_frame offset from the end of the stack page, and the saved rbp is zero, it can assume that it's a newly forked task. But that seems a little more brittle to me, as it requires the unwinder to understand more of the internal workings of the fork code.

Quoted 6 lines

Quoted 3 lines

But overall I think this patch is a really nice cleanup, and other than the above minor issue it should be fine with my reliable unwinder, since rbp is still at the top of the stack.

Is this a regression or is there some reason that it works right without the patch?

Without the patch, it uses TIF_FORK to determine the stack is empty.

Quoted 29 lines · Josh Poimboeuf

Quoted 28 lines · Andy Lutomirski

Quoted 26 lines · Josh Poimboeuf

Quoted 2 lines · Andy Lutomirski

cc: Josh Poimboeuf: do you care about the exact stack layout of the bottom of the stack of an inactive task?

So there's one minor issue with this patch, relating to unwinding the stack of a newly forked task. For detecting reliable stacks, the unwinder needs to unwind all the way to the syscall pt_regs to make sure the stack is sane. But for newly forked tasks, that won't be possible here because the unwinding will stop at the fork_frame instead.

So from an unwinder standpoint it might be nice for copy_thread_tls() to place a frame pointer on the stack next to the ret_from_fork return address, so that it would resemble an actual stack frame. The frame pointer could probably just be hard-coded to zero. And then the first bp in fork_frame would need to be a pointer to it instead of zero. That would make it nicely resemble the stack of any other task.

Alternatively I could teach the unwinder that if the unwinding starts at the fork_frame offset from the end of the stack page, and the saved rbp is zero, it can assume that it's a newly forked task. But that seems a little more brittle to me, as it requires the unwinder to understand more of the internal workings of the fork code.

But overall I think this patch is a really nice cleanup, and other than the above minor issue it should be fine with my reliable unwinder, since rbp is still at the top of the stack.

Quoted 4 lines

Quoted 2 lines

Is this a regression or is there some reason that it works right without the patch?

Without the patch, it uses TIF_FORK to determine the stack is empty.

Where is this code? I don't see it in the mainline kernel.

Quoted 2 lines · Andy Lutomirski

cc: Josh Poimboeuf: do you care about the exact stack layout of the bottom of the stack of an inactive task?

So there's one minor issue with this patch, relating to unwinding the stack of a newly forked task. For detecting reliable stacks, the unwinder needs to unwind all the way to the syscall pt_regs to make sure the stack is sane. But for newly forked tasks, that won't be possible here because the unwinding will stop at the fork_frame instead.

So from an unwinder standpoint it might be nice for copy_thread_tls() to place a frame pointer on the stack next to the ret_from_fork return address, so that it would resemble an actual stack frame. The frame pointer could probably just be hard-coded to zero. And then the first bp in fork_frame would need to be a pointer to it instead of zero. That would make it nicely resemble the stack of any other task.

Alternatively I could teach the unwinder that if the unwinding starts at the fork_frame offset from the end of the stack page, and the saved rbp is zero, it can assume that it's a newly forked task. But that seems a little more brittle to me, as it requires the unwinder to understand more of the internal workings of the fork code.

But overall I think this patch is a really nice cleanup, and other than the above minor issue it should be fine with my reliable unwinder, since rbp is still at the top of the stack.

Quoted 23 lines · Josh Poimboeuf

Quoted 2 lines · Andy Lutomirski

cc: Josh Poimboeuf: do you care about the exact stack layout of the bottom of the stack of an inactive task?

So there's one minor issue with this patch, relating to unwinding the stack of a newly forked task. For detecting reliable stacks, the unwinder needs to unwind all the way to the syscall pt_regs to make sure the stack is sane. But for newly forked tasks, that won't be possible here because the unwinding will stop at the fork_frame instead.

So from an unwinder standpoint it might be nice for copy_thread_tls() to place a frame pointer on the stack next to the ret_from_fork return address, so that it would resemble an actual stack frame. The frame pointer could probably just be hard-coded to zero. And then the first bp in fork_frame would need to be a pointer to it instead of zero. That would make it nicely resemble the stack of any other task.

Alternatively I could teach the unwinder that if the unwinding starts at the fork_frame offset from the end of the stack page, and the saved rbp is zero, it can assume that it's a newly forked task. But that seems a little more brittle to me, as it requires the unwinder to understand more of the internal workings of the fork code.

Quoted 3 lines

But overall I think this patch is a really nice cleanup, and other than the above minor issue it should be fine with my reliable unwinder, since rbp is still at the top of the stack.

Ok, how about if it pushed RBP first, then we teach get_wchan() to add the fixed offset from thread.sp to get bp? that way it don't have to push it twice.

Quoted 24 lines · Brian Gerst

Quoted 23 lines · Josh Poimboeuf

Quoted 2 lines · Andy Lutomirski

cc: Josh Poimboeuf: do you care about the exact stack layout of the bottom of the stack of an inactive task?

So there's one minor issue with this patch, relating to unwinding the stack of a newly forked task. For detecting reliable stacks, the unwinder needs to unwind all the way to the syscall pt_regs to make sure the stack is sane. But for newly forked tasks, that won't be possible here because the unwinding will stop at the fork_frame instead.

So from an unwinder standpoint it might be nice for copy_thread_tls() to place a frame pointer on the stack next to the ret_from_fork return address, so that it would resemble an actual stack frame. The frame pointer could probably just be hard-coded to zero. And then the first bp in fork_frame would need to be a pointer to it instead of zero. That would make it nicely resemble the stack of any other task.

Alternatively I could teach the unwinder that if the unwinding starts at the fork_frame offset from the end of the stack page, and the saved rbp is zero, it can assume that it's a newly forked task. But that seems a little more brittle to me, as it requires the unwinder to understand more of the internal workings of the fork code.

Quoted 7 lines

Quoted 3 lines

But overall I think this patch is a really nice cleanup, and other than the above minor issue it should be fine with my reliable unwinder, since rbp is still at the top of the stack.

Ok, how about if it pushed RBP first, then we teach get_wchan() to add the fixed offset from thread.sp to get bp? that way it don't have to push it twice.

In theory I like the idea, and it would work: the unwinder could just use the inactive_task_frame struct (as Andy suggested) to find the frame pointer.

But I suspect it would break all existing unwinders, both in-tree and out-of-tree. The only out-of-tree one I know of is crash, not sure if there are more out there.

Quoted 37 lines · Josh Poimboeuf

Quoted 24 lines · Brian Gerst

Quoted 23 lines · Josh Poimboeuf

Quoted 2 lines · Andy Lutomirski

cc: Josh Poimboeuf: do you care about the exact stack layout of the bottom of the stack of an inactive task?

So there's one minor issue with this patch, relating to unwinding the stack of a newly forked task. For detecting reliable stacks, the unwinder needs to unwind all the way to the syscall pt_regs to make sure the stack is sane. But for newly forked tasks, that won't be possible here because the unwinding will stop at the fork_frame instead.

So from an unwinder standpoint it might be nice for copy_thread_tls() to place a frame pointer on the stack next to the ret_from_fork return address, so that it would resemble an actual stack frame. The frame pointer could probably just be hard-coded to zero. And then the first bp in fork_frame would need to be a pointer to it instead of zero. That would make it nicely resemble the stack of any other task.

Alternatively I could teach the unwinder that if the unwinding starts at the fork_frame offset from the end of the stack page, and the saved rbp is zero, it can assume that it's a newly forked task. But that seems a little more brittle to me, as it requires the unwinder to understand more of the internal workings of the fork code.

Quoted 7 lines

Quoted 3 lines

But overall I think this patch is a really nice cleanup, and other than the above minor issue it should be fine with my reliable unwinder, since rbp is still at the top of the stack.

Ok, how about if it pushed RBP first, then we teach get_wchan() to add the fixed offset from thread.sp to get bp? that way it don't have to push it twice.

In theory I like the idea, and it would work: the unwinder could just use the inactive_task_frame struct (as Andy suggested) to find the frame pointer.

Quoted 3 lines

But I suspect it would break all existing unwinders, both in-tree and out-of-tree. The only out-of-tree one I know of is crash, not sure if there are more out there.

I should mention it would only affect those unwinders which know how to do sleeping kernel tasks. So generic tools like gdb wouldn't be affected.

Quoted 38 lines · Josh Poimboeuf

Quoted 37 lines · Josh Poimboeuf

Quoted 24 lines · Brian Gerst

Quoted 23 lines · Josh Poimboeuf

Quoted 2 lines · Andy Lutomirski

cc: Josh Poimboeuf: do you care about the exact stack layout of the bottom of the stack of an inactive task?

So there's one minor issue with this patch, relating to unwinding the stack of a newly forked task. For detecting reliable stacks, the unwinder needs to unwind all the way to the syscall pt_regs to make sure the stack is sane. But for newly forked tasks, that won't be possible here because the unwinding will stop at the fork_frame instead.

So from an unwinder standpoint it might be nice for copy_thread_tls() to place a frame pointer on the stack next to the ret_from_fork return address, so that it would resemble an actual stack frame. The frame pointer could probably just be hard-coded to zero. And then the first bp in fork_frame would need to be a pointer to it instead of zero. That would make it nicely resemble the stack of any other task.

Alternatively I could teach the unwinder that if the unwinding starts at the fork_frame offset from the end of the stack page, and the saved rbp is zero, it can assume that it's a newly forked task. But that seems a little more brittle to me, as it requires the unwinder to understand more of the internal workings of the fork code.

Quoted 7 lines

Quoted 3 lines

But overall I think this patch is a really nice cleanup, and other than the above minor issue it should be fine with my reliable unwinder, since rbp is still at the top of the stack.

Ok, how about if it pushed RBP first, then we teach get_wchan() to add the fixed offset from thread.sp to get bp? that way it don't have to push it twice.

In theory I like the idea, and it would work: the unwinder could just use the inactive_task_frame struct (as Andy suggested) to find the frame pointer.

Quoted 7 lines

Quoted 3 lines

But I suspect it would break all existing unwinders, both in-tree and out-of-tree. The only out-of-tree one I know of is crash, not sure if there are more out there.

I should mention it would only affect those unwinders which know how to do sleeping kernel tasks. So generic tools like gdb wouldn't be affected.

[continuing my conversation with myself...]

To clarify, I still think we should do it. The stack format of a sleeping task isn't exactly an ABI, and I wouldn't expect many tools to rely on it. I can help with the fixing of in-tree unwinders if needed.

Or I could even do the moving of the frame pointer as a separate patch on top of this one, since it might cause breakage elsewhere.

Quoted 53 lines · Josh Poimboeuf

Quoted 38 lines · Josh Poimboeuf

Quoted 37 lines · Josh Poimboeuf

Quoted 24 lines · Brian Gerst

Quoted 23 lines · Josh Poimboeuf

Quoted 2 lines · Andy Lutomirski

cc: Josh Poimboeuf: do you care about the exact stack layout of the bottom of the stack of an inactive task?

So there's one minor issue with this patch, relating to unwinding the stack of a newly forked task. For detecting reliable stacks, the unwinder needs to unwind all the way to the syscall pt_regs to make sure the stack is sane. But for newly forked tasks, that won't be possible here because the unwinding will stop at the fork_frame instead.

So from an unwinder standpoint it might be nice for copy_thread_tls() to place a frame pointer on the stack next to the ret_from_fork return address, so that it would resemble an actual stack frame. The frame pointer could probably just be hard-coded to zero. And then the first bp in fork_frame would need to be a pointer to it instead of zero. That would make it nicely resemble the stack of any other task.

Alternatively I could teach the unwinder that if the unwinding starts at the fork_frame offset from the end of the stack page, and the saved rbp is zero, it can assume that it's a newly forked task. But that seems a little more brittle to me, as it requires the unwinder to understand more of the internal workings of the fork code.

Quoted 7 lines

Quoted 3 lines

But overall I think this patch is a really nice cleanup, and other than the above minor issue it should be fine with my reliable unwinder, since rbp is still at the top of the stack.

Ok, how about if it pushed RBP first, then we teach get_wchan() to add the fixed offset from thread.sp to get bp? that way it don't have to push it twice.

In theory I like the idea, and it would work: the unwinder could just use the inactive_task_frame struct (as Andy suggested) to find the frame pointer.

Quoted 7 lines

Quoted 3 lines

But I suspect it would break all existing unwinders, both in-tree and out-of-tree. The only out-of-tree one I know of is crash, not sure if there are more out there.

I should mention it would only affect those unwinders which know how to do sleeping kernel tasks. So generic tools like gdb wouldn't be affected.

[continuing my conversation with myself...]

To clarify, I still think we should do it. The stack format of a sleeping task isn't exactly an ABI, and I wouldn't expect many tools to rely on it. I can help with the fixing of in-tree unwinders if needed.

Quoted 2 lines

Or I could even do the moving of the frame pointer as a separate patch on top of this one, since it might cause breakage elsewhere.

Do you have any understanding of why there are so many unwinder implementations? Your reliable unwinder seems to be yet another copy of more or less the same code.

I'd like to see a single, high-quality unwinder implemented as a state machine, along the lines of:

struct unwind_state state;
unwind_start_inactive_task(&state, ...); or
unwind_start_pt_regs(&state, regs); or whatever.
unwind_next_frame(&state);

where, after unwind_next_frame, state encodes whatever registers are known (at least bp and ip, but all the GPRs would be nice and are probably mandatory for DWARF) and an indication of whether this is a real frame or a guessed frame (the things that currently show up as '?').

Do you have any understanding of why there are so many unwinder implementations? Your reliable unwinder seems to be yet another copy of more or less the same code.

I'd like to see a single, high-quality unwinder implemented as a state machine, along the lines of:

struct unwind_state state;
unwind_start_inactive_task(&state, ...); or
unwind_start_pt_regs(&state, regs); or whatever.
unwind_next_frame(&state);

where, after unwind_next_frame, state encodes whatever registers are known (at least bp and ip, but all the GPRs would be nice and are probably mandatory for DWARF) and an indication of whether this is a real frame or a guessed frame (the things that currently show up as '?').

Move the low-level context switch code to an out-of-line asm stub instead of using complex inline asm. This allows constructing a new stack frame for the child process to make it seamlessly flow to ret_from_fork without an extra test and branch in __switch_to(). It also improves code generation for __schedule() by using the C calling convention instead of clobbering all registers.

Quoted 6 lines · Brian Gerst

Move the low-level context switch code to an out-of-line asm stub instead of using complex inline asm. This allows constructing a new stack frame for the child process to make it seamlessly flow to ret_from_fork without an extra test and branch in __switch_to(). It also improves code generation for __schedule() by using the C calling convention instead of clobbering all registers.

Just a heads up: I'm writing some code that conflicts with this patch. The conflict will be easy to resolve, and, if this patch beats mine to -tip, I'll rebase.

Quoted 9 lines · Andy Lutomirski

Quoted 6 lines · Brian Gerst

Move the low-level context switch code to an out-of-line asm stub instead of using complex inline asm. This allows constructing a new stack frame for the child process to make it seamlessly flow to ret_from_fork without an extra test and branch in __switch_to(). It also improves code generation for __schedule() by using the C calling convention instead of clobbering all registers.

Quoted 3 lines

Just a heads up: I'm writing some code that conflicts with this patch. The conflict will be easy to resolve, and, if this patch beats mine to -tip, I'll rebase.

So I was expecting another iteration of this switch_to() series, but had no fundamental objections to the concept.

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 <[email protected]>

Quoted 5 lines · Brian Gerst

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 <[email protected]>

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

--Andy

arch/x86/entry/entry_64.S +4 -19

@@ -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 */

Quoted 45 lines · Brian Gerst

arch/x86/entry/entry_64.S +8 -19

@@ -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

arch/x86/entry/entry_64.S END(__switch_to_asm) · line 431

Quoted 4 lines

arch/x86/entry/entry_64.S +3 -0

+     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.

Quoted 46 lines · Josh Poimboeuf

Quoted 45 lines · Brian Gerst

arch/x86/entry/entry_64.S +8 -19

@@ -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

Quoted 9 lines

arch/x86/entry/entry_64.S END(__switch_to_asm) · line 431

Quoted 4 lines

arch/x86/entry/entry_64.S +3 -0

+     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.