Rewrite ret_from_fork() in C
Subsystems (3)
v2 cover Rewrite ret_from_fork() in C
When kCFI is enabled, special handling is needed for the indirect call to the kernel thread function. Handling this in pure assembly is not simple, so moving it to C is more appropriate. Instead of moving just the indirect call to C as Peter Zijlstra has proposed, this patchset rewrites the whole ret_from_fork() function in C (other than some necessary asm glue remaning).
V2:
- Fixed wrong address of pt_regs being passed
- Simplified 64-bit asm stub to not use a tail-call
Brian Gerst (2):
x86/32: Remove schedule_tail_wrapper()
x86: Rewrite ret_from_fork() in C
arch/x86/entry/entry_32.S | 53 ++++++++------------------------ arch/x86/entry/entry_64.S | 33 +++++--------------- arch/x86/include/asm/switch_to.h | 4 ++- arch/x86/kernel/process.c | 22 ++++++++++++- 4 files changed, 45 insertions(+), 67 deletions(-)
Signature · 1 line
2.41.0
PATCH v2 1/2 x86/32: Remove schedule_tail_wrapper()
The unwinder expects a return address at the very top of the kernel stack just below pt_regs and before any stack frame is created. Instead of calling a wrapper, set up a return address as if ret_from_fork() was called from the syscall entry code.
--- arch/x86/entry/entry_32.S | 33 ++++++++++----------------------- 1 file changed, 10 insertions(+), 23 deletions(-)
arch/x86/entry/entry_32.S +10 -23
@@ -719,26 +719,6 @@ SYM_CODE_START(__switch_to_asm) SYM_CODE_END(__switch_to_asm) .popsection -/* - * The unwinder expects the last frame on the stack to always be at the same - * offset from the end of the page, which allows it to validate the stack. - * Calling schedule_tail() directly would break that convention because its an - * asmlinkage function so its argument has to be pushed on the stack. This - * wrapper creates a proper "end of stack" frame header before the call. - */ -.pushsection .text, "ax" -SYM_FUNC_START(schedule_tail_wrapper) - FRAME_BEGIN - - pushl %eax - call schedule_tail - popl %eax - - FRAME_END - RET -SYM_FUNC_END(schedule_tail_wrapper) -.popsection - /* * A newly forked process directly context switches into this address. *
@@ -748,16 +728,23 @@ SYM_FUNC_END(schedule_tail_wrapper) */ .pushsection .text, "ax" SYM_CODE_START(ret_from_fork) - call schedule_tail_wrapper + /* return address for the stack unwinder */ + pushl $.Lsyscall_32_done + + FRAME_BEGIN + pushl %eax + call schedule_tail + addl $4, %esp + FRAME_END testl %ebx, %ebx jnz 1f /* kernel threads are uncommon */ 2: /* When we fork, we trace the syscall return in the child, too. */ - movl %esp, %eax + leal 4(%esp), %eax call syscall_exit_to_user_mode - jmp .Lsyscall_32_done + RET /* kernel thread */ 1: movl %edi, %eax
Signature · 1 line
2.41.0
[tip: x86/urgent] x86/32: Remove schedule_tail_wrapper()
The following commit has been merged into the x86/urgent branch of tip:
The following commit has been merged into the x86/urgent branch of tip:
Commit-ID: 81f755d561f365f544795fad92f05a085ea4f292 Gitweb: https://git.kernel.org/tip/81f755d561f365f544795fad92f05a085ea4f292 Author: Brian Gerst <[email protected]> AuthorDate: Fri, 23 Jun 2023 18:55:28 -04:00 Committer: Peter Zijlstra <[email protected]> CommitterDate: Mon, 10 Jul 2023 09:52:24 +02:00
x86/32: Remove schedule_tail_wrapper()
The unwinder expects a return address at the very top of the kernel stack just below pt_regs and before any stack frame is created. Instead of calling a wrapper, set up a return address as if ret_from_fork() was called from the syscall entry code.
--- arch/x86/entry/entry_32.S | 33 ++++++++++----------------------- 1 file changed, 10 insertions(+), 23 deletions(-)
arch/x86/entry/entry_32.S +10 -23
@@ -720,26 +720,6 @@ SYM_CODE_END(__switch_to_asm) .popsection /* - * The unwinder expects the last frame on the stack to always be at the same - * offset from the end of the page, which allows it to validate the stack. - * Calling schedule_tail() directly would break that convention because its an - * asmlinkage function so its argument has to be pushed on the stack. This - * wrapper creates a proper "end of stack" frame header before the call. - */ -.pushsection .text, "ax" -SYM_FUNC_START(schedule_tail_wrapper) - FRAME_BEGIN - - pushl %eax - call schedule_tail - popl %eax - - FRAME_END - RET -SYM_FUNC_END(schedule_tail_wrapper) -.popsection - -/* * A newly forked process directly context switches into this address. * * eax: prev task we switched from
@@ -748,16 +728,23 @@ SYM_FUNC_END(schedule_tail_wrapper) */ .pushsection .text, "ax" SYM_CODE_START(ret_from_fork) - call schedule_tail_wrapper + /* return address for the stack unwinder */ + pushl $.Lsyscall_32_done + + FRAME_BEGIN + pushl %eax + call schedule_tail + addl $4, %esp + FRAME_END testl %ebx, %ebx jnz 1f /* kernel threads are uncommon */ 2: /* When we fork, we trace the syscall return in the child, too. */ - movl %esp, %eax + leal 4(%esp), %eax call syscall_exit_to_user_mode - jmp .Lsyscall_32_done + RET /* kernel thread */ 1: movl %edi, %eax
PATCH v2 2/2 x86: Rewrite ret_from_fork() in C
In reply to Brian GerstWhen kCFI is enabled, special handling is needed for the indirect call to the kernel thread function. Rewrite the ret_from_fork() function in C so that the compiler can properly handle the indirect call.
--- arch/x86/entry/entry_32.S | 30 ++++++++--------------------- arch/x86/entry/entry_64.S | 33 ++++++++------------------------ arch/x86/include/asm/switch_to.h | 4 +++- arch/x86/kernel/process.c | 22 ++++++++++++++++++++- 4 files changed, 40 insertions(+), 49 deletions(-)
arch/x86/entry/entry_32.S +8 -22
@@ -727,36 +727,22 @@ SYM_CODE_END(__switch_to_asm) * edi: kernel thread arg */ .pushsection .text, "ax" -SYM_CODE_START(ret_from_fork) +SYM_CODE_START(ret_from_fork_asm) + movl %esp, %edx /* regs */ + /* return address for the stack unwinder */ pushl $.Lsyscall_32_done FRAME_BEGIN - pushl %eax - call schedule_tail + /* prev already in EAX */ + movl %ebx, %ecx /* fn */ + pushl %edi /* fn_arg */ + call ret_from_fork addl $4, %esp FRAME_END - testl %ebx, %ebx - jnz 1f /* kernel threads are uncommon */ - -2: - /* When we fork, we trace the syscall return in the child, too. */ - leal 4(%esp), %eax - call syscall_exit_to_user_mode RET - - /* kernel thread */ -1: movl %edi, %eax - CALL_NOSPEC ebx - /* - * A kernel thread is allowed to return here after successfully - * calling kernel_execve(). Exit to userspace to complete the execve() - * syscall. - */ - movl $0, PT_EAX(%esp) - jmp 2b -SYM_CODE_END(ret_from_fork) +SYM_CODE_END(ret_from_fork_asm) .popsection SYM_ENTRY(__begin_SYSENTER_singlestep_region, SYM_L_GLOBAL, SYM_A_NONE)
arch/x86/entry/entry_64.S +8 -25
@@ -284,36 +284,19 @@ SYM_FUNC_END(__switch_to_asm) * r12: kernel thread arg */ .pushsection .text, "ax" - __FUNC_ALIGN -SYM_CODE_START_NOALIGN(ret_from_fork) - UNWIND_HINT_END_OF_STACK +SYM_CODE_START(ret_from_fork_asm) + UNWIND_HINT_REGS ANNOTATE_NOENDBR // copy_thread CALL_DEPTH_ACCOUNT - movq %rax, %rdi - call schedule_tail /* rdi: 'prev' task parameter */ - testq %rbx, %rbx /* from kernel_thread? */ - jnz 1f /* kernel threads are uncommon */ + movq %rax, %rdi /* prev */ + movq %rsp, %rsi /* regs */ + movq %rbx, %rdx /* fn */ + movq %r12, %rcx /* fn_arg */ + call ret_from_fork -2: - UNWIND_HINT_REGS - movq %rsp, %rdi - call syscall_exit_to_user_mode /* returns with IRQs disabled */ jmp swapgs_restore_regs_and_return_to_usermode - -1: - /* kernel thread */ - UNWIND_HINT_END_OF_STACK - movq %r12, %rdi - CALL_NOSPEC rbx - /* - * A kernel thread is allowed to return here after successfully - * calling kernel_execve(). Exit to userspace to complete the execve() - * syscall. - */ - movq $0, RAX(%rsp) - jmp 2b -SYM_CODE_END(ret_from_fork) +SYM_CODE_END(ret_from_fork_asm) .popsection .macro DEBUG_ENTRY_ASSERT_IRQS_OFF
arch/x86/include/asm/switch_to.h +3 -1
@@ -12,7 +12,9 @@ struct task_struct *__switch_to_asm(struct task_struct *prev, __visible struct task_struct *__switch_to(struct task_struct *prev,struct task_struct *next); -asmlinkage void ret_from_fork(void); +asmlinkage void ret_from_fork_asm(void); +__visible void ret_from_fork(struct task_struct *prev, struct pt_regs *regs, +int (*fn)(void *),void *fn_arg);/* * This is the structure pointed to by thread.sp for an inactive task. The
arch/x86/kernel/process.c +21 -1
@@ -28,6 +28,7 @@#include <linux/static_call.h> #include <trace/events/power.h> #include <linux/hw_breakpoint.h> +#include <linux/entry-common.h> #include <asm/cpu.h> #include <asm/apic.h> #include <linux/uaccess.h>
@@ -136,6 +137,25 @@ static int set_new_tls(struct task_struct *p, unsigned long tls)return do_set_thread_area_64 (p,ARCH_SET_FS , tls); } +__visible noinstr void ret_from_fork(struct task_struct *prev, struct pt_regs *regs, +int (*fn)(void *),void *fn_arg) +{ +schedule_tail (prev); + +/* Is this a kernel thread? */ +if (unlikely (fn)) { +fn (fn_arg); +/* +* A kernel thread is allowed to return here after successfully +* calling kernel_execve(). Exit to userspace to complete the +* execve() syscall. +*/ + regs->ax =0 ; + } + +syscall_exit_to_user_mode (regs); +} +int copy_thread (struct task_struct *p,const struct kernel_clone_args *args) {unsigned long clone_flags = args->flags;
@@ -152,7 +172,7 @@ int copy_thread(struct task_struct *p, const struct kernel_clone_args *args) frame = &fork_frame->frame; frame->bp =encode_frame_pointer (childregs); - frame->ret_addr = (unsigned long ) ret_from_fork; + frame->ret_addr = (unsigned long ) ret_from_fork_asm; p->thread.sp = (unsigned long ) fork_frame; p->thread.io_bitmap =NULL ; p->thread.iopl_warn =0 ;
Signature · 1 line
2.41.0
[tip: x86/urgent] x86: Rewrite ret_from_fork() in C
The following commit has been merged into the x86/urgent branch of tip:
The following commit has been merged into the x86/urgent branch of tip:
Commit-ID: 3aec4ecb3d1f313a8ab985df7cab07c4af81f478 Gitweb: https://git.kernel.org/tip/3aec4ecb3d1f313a8ab985df7cab07c4af81f478 Author: Brian Gerst <[email protected]> AuthorDate: Fri, 23 Jun 2023 18:55:29 -04:00 Committer: Peter Zijlstra <[email protected]> CommitterDate: Mon, 10 Jul 2023 09:52:25 +02:00
x86: Rewrite ret_from_fork() in C
When kCFI is enabled, special handling is needed for the indirect call to the kernel thread function. Rewrite the ret_from_fork() function in C so that the compiler can properly handle the indirect call.
--- arch/x86/entry/entry_32.S | 30 +++++++--------------------- arch/x86/entry/entry_64.S | 33 +++++++------------------------ arch/x86/include/asm/switch_to.h | 4 +++- arch/x86/kernel/process.c | 22 ++++++++++++++++++++- 4 files changed, 40 insertions(+), 49 deletions(-)
arch/x86/entry/entry_32.S +8 -22
@@ -727,36 +727,22 @@ SYM_CODE_END(__switch_to_asm) * edi: kernel thread arg */ .pushsection .text, "ax" -SYM_CODE_START(ret_from_fork) +SYM_CODE_START(ret_from_fork_asm) + movl %esp, %edx /* regs */ + /* return address for the stack unwinder */ pushl $.Lsyscall_32_done FRAME_BEGIN - pushl %eax - call schedule_tail + /* prev already in EAX */ + movl %ebx, %ecx /* fn */ + pushl %edi /* fn_arg */ + call ret_from_fork addl $4, %esp FRAME_END - testl %ebx, %ebx - jnz 1f /* kernel threads are uncommon */ - -2: - /* When we fork, we trace the syscall return in the child, too. */ - leal 4(%esp), %eax - call syscall_exit_to_user_mode RET - - /* kernel thread */ -1: movl %edi, %eax - CALL_NOSPEC ebx - /* - * A kernel thread is allowed to return here after successfully - * calling kernel_execve(). Exit to userspace to complete the execve() - * syscall. - */ - movl $0, PT_EAX(%esp) - jmp 2b -SYM_CODE_END(ret_from_fork) +SYM_CODE_END(ret_from_fork_asm) .popsection SYM_ENTRY(__begin_SYSENTER_singlestep_region, SYM_L_GLOBAL, SYM_A_NONE)
arch/x86/entry/entry_64.S +8 -25
@@ -284,36 +284,19 @@ SYM_FUNC_END(__switch_to_asm) * r12: kernel thread arg */ .pushsection .text, "ax" - __FUNC_ALIGN -SYM_CODE_START_NOALIGN(ret_from_fork) - UNWIND_HINT_END_OF_STACK +SYM_CODE_START(ret_from_fork_asm) + UNWIND_HINT_REGS ANNOTATE_NOENDBR // copy_thread CALL_DEPTH_ACCOUNT - movq %rax, %rdi - call schedule_tail /* rdi: 'prev' task parameter */ - testq %rbx, %rbx /* from kernel_thread? */ - jnz 1f /* kernel threads are uncommon */ + movq %rax, %rdi /* prev */ + movq %rsp, %rsi /* regs */ + movq %rbx, %rdx /* fn */ + movq %r12, %rcx /* fn_arg */ + call ret_from_fork -2: - UNWIND_HINT_REGS - movq %rsp, %rdi - call syscall_exit_to_user_mode /* returns with IRQs disabled */ jmp swapgs_restore_regs_and_return_to_usermode - -1: - /* kernel thread */ - UNWIND_HINT_END_OF_STACK - movq %r12, %rdi - CALL_NOSPEC rbx - /* - * A kernel thread is allowed to return here after successfully - * calling kernel_execve(). Exit to userspace to complete the execve() - * syscall. - */ - movq $0, RAX(%rsp) - jmp 2b -SYM_CODE_END(ret_from_fork) +SYM_CODE_END(ret_from_fork_asm) .popsection .macro DEBUG_ENTRY_ASSERT_IRQS_OFF
arch/x86/include/asm/switch_to.h +3 -1
@@ -12,7 +12,9 @@ struct task_struct *__switch_to_asm(struct task_struct *prev, __visible struct task_struct *__switch_to(struct task_struct *prev,struct task_struct *next); -asmlinkage void ret_from_fork(void); +asmlinkage void ret_from_fork_asm(void); +__visible void ret_from_fork(struct task_struct *prev, struct pt_regs *regs, +int (*fn)(void *),void *fn_arg);/* * This is the structure pointed to by thread.sp for an inactive task. The
arch/x86/kernel/process.c +21 -1
@@ -28,6 +28,7 @@#include <linux/static_call.h> #include <trace/events/power.h> #include <linux/hw_breakpoint.h> +#include <linux/entry-common.h> #include <asm/cpu.h> #include <asm/apic.h> #include <linux/uaccess.h>
@@ -134,6 +135,25 @@ static int set_new_tls(struct task_struct *p, unsigned long tls)return do_set_thread_area_64 (p,ARCH_SET_FS , tls); } +__visible void ret_from_fork(struct task_struct *prev, struct pt_regs *regs, +int (*fn)(void *),void *fn_arg) +{ +schedule_tail (prev); + +/* Is this a kernel thread? */ +if (unlikely (fn)) { +fn (fn_arg); +/* +* A kernel thread is allowed to return here after successfully +* calling kernel_execve(). Exit to userspace to complete the +* execve() syscall. +*/ + regs->ax =0 ; + } + +syscall_exit_to_user_mode (regs); +} +int copy_thread (struct task_struct *p,const struct kernel_clone_args *args) {unsigned long clone_flags = args->flags;
@@ -149,7 +169,7 @@ int copy_thread(struct task_struct *p, const struct kernel_clone_args *args) frame = &fork_frame->frame; frame->bp =encode_frame_pointer (childregs); - frame->ret_addr = (unsigned long ) ret_from_fork; + frame->ret_addr = (unsigned long ) ret_from_fork_asm; p->thread.sp = (unsigned long ) fork_frame; p->thread.io_bitmap =NULL ; p->thread.iopl_warn =0 ;
Petr Mladek
maintainerRe: [PATCH v2 2/2] x86: Rewrite ret_from_fork() in C
In reply to Brian GerstQuoted 3 lines · Brian Gerst
When kCFI is enabled, special handling is needed for the indirect call to the kernel thread function. Rewrite the
ret_from_fork()function in C so that the compiler can properly handle the indirect call.
This patch broke livepatching. Kthreads never have a reliable stack. It works when I revert it.
See also below.
Quoted 23 lines
arch/x86/entry/entry_64.S +7
-7@@ -284,36 +284,19 @@ SYM_FUNC_END(__switch_to_asm) * r12: kernel thread arg */ .pushsection .text, "ax" - __FUNC_ALIGN -SYM_CODE_START_NOALIGN(ret_from_fork) - UNWIND_HINT_END_OF_STACK +SYM_CODE_START(ret_from_fork_asm) + UNWIND_HINT_REGS ANNOTATE_NOENDBR // copy_thread CALL_DEPTH_ACCOUNT - movq %rax, %rdi - call schedule_tail /* rdi: 'prev' task parameter */ - testq %rbx, %rbx /* from kernel_thread? */ - jnz 1f /* kernel threads are uncommon */ + movq %rax, %rdi /* prev */ + movq %rsp, %rsi /* regs */ + movq %rbx, %rdx /* fn */ + movq %r12, %rcx /* fn_arg */ + call ret_from_fork
arch/x86/entry/entry_64.S SYM_FUNC_END(__switch_to_asm) · line 306
Quoted 10 lines
arch/x86/entry/entry_64.S +0
-8-2: - UNWIND_HINT_REGS - movq %rsp, %rdi - call syscall_exit_to_user_mode /* returns with IRQs disabled */ jmp swapgs_restore_regs_and_return_to_usermode - -1: - /* kernel thread */ - UNWIND_HINT_END_OF_STACK
I think that it might be related to removal of this line. The following intructions are going to call fn(fn_arg). See below.
Quoted 35 lines
arch/x86/entry/entry_64.S +1
-10- movq %r12, %rdi - CALL_NOSPEC rbx - /* - * A kernel thread is allowed to return here after successfully - * calling kernel_execve(). Exit to userspace to complete the execve() - * syscall. - */ - movq $0, RAX(%rsp) - jmp 2b -SYM_CODE_END(ret_from_fork) +SYM_CODE_END(ret_from_fork_asm) .popsection .macro DEBUG_ENTRY_ASSERT_IRQS_OFFarch/x86/include/asm/switch_to.h +3
-1@@ -12,7 +12,9 @@ struct task_struct *__switch_to_asm(struct task_struct *prev, __visible struct task_struct *__switch_to(struct task_struct *prev,struct task_struct *next); -asmlinkage void ret_from_fork(void); +asmlinkage void ret_from_fork_asm(void); +__visible void ret_from_fork(struct task_struct *prev, struct pt_regs *regs, +int (*fn)(void *),void *fn_arg);/* * This is the structure pointed to by thread.sp for an inactive task. The arch/x86/kernel/process.c +0
-0@@ -136,6 +137,25 @@ static int set_new_tls(struct task_struct *p, unsigned long tls)return do_set_thread_area_64 (p,ARCH_SET_FS , tls);
arch/x86/kernel/process.c static int set_new_tls(struct task_struct *p, unsigned long tls) · line 147
Quoted 10 lines
arch/x86/kernel/process.c +8
-0} +__visible noinstr void ret_from_fork(struct task_struct *prev, struct pt_regs *regs, +int (*fn)(void *),void *fn_arg) +{ +schedule_tail (prev); + +/* Is this a kernel thread? */ +if (unlikely (fn)) { +fn (fn_arg);
This is the related code but it does not include the annotation about the end of the stack.
Honestly, I am not familiar with the stack unwinder and how this is supposed to work.
I hope that Josh or anyone else might know better.
Quoted 4 lines
arch/x86/kernel/process.c +4
-0+/* +* A kernel thread is allowed to return here after successfully +* calling kernel_execve(). Exit to userspace to complete the +* execve() syscall.
arch/x86/kernel/process.c static int set_new_tls(struct task_struct *p, unsigned long tls) · line 161
Quoted 10 lines
arch/x86/kernel/process.c +7
-0+*/ + regs->ax =0 ; + } + +syscall_exit_to_user_mode (regs); +} +int copy_thread (struct task_struct *p,const struct kernel_clone_args *args) {unsigned long clone_flags = args->flags;
Best Regards,
Petr
Re: [PATCH v2 2/2] x86: Rewrite ret_from_fork() in C
Quoted 5 lines · Petr Mladek
This patch broke livepatching. Kthreads never have a reliable stack. It works when I revert it.
arch/x86/entry/entry_64.S SYM_FUNC_END(__switch_to_asm) · line 288
Quoted 2 lines
arch/x86/entry/entry_64.S +2
-0+SYM_CODE_START(ret_from_fork_asm) + UNWIND_HINT_REGS
It works again when I change the above hint to UNWIND_HINT_END_OF_STACK, so yeah. Doing this makes objtool unhappy with something else though, so I'll go prod at things with something sharp...
Thanks!
Quoted 10 lines
Quoted 10 lines
arch/x86/entry/entry_64.S +6
-0ANNOTATE_NOENDBR // copy_thread CALL_DEPTH_ACCOUNT + movq %rax, %rdi /* prev */ + movq %rsp, %rsi /* regs */ + movq %rbx, %rdx /* fn */ + movq %r12, %rcx /* fn_arg */ + call ret_from_fork +SYM_CODE_END(ret_from_fork_asm)
Re: [PATCH v2 2/2] x86: Rewrite ret_from_fork() in C
Quoted 11 lines · Peter Zijlstra
Quoted 5 lines · Petr Mladek
This patch broke livepatching. Kthreads never have a reliable stack. It works when I revert it.
Review
Quoted 2 lines
(quoted diff) +2
-0+SYM_CODE_START(ret_from_fork_asm) + UNWIND_HINT_REGSIt works again when I change the above hint to
UNWIND_HINT_END_OF_STACK, so yeah. Doing this makes objtool unhappy with something else though, so I'll go prod at things with something sharp...
The below cures things; Josh, did I miss anything?
--- arch/x86/entry/entry_64.S | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)
arch/x86/entry/entry_64.S +13 -1
@@ -285,7 +285,14 @@ SYM_FUNC_END(__switch_to_asm) */ .pushsection .text, "ax" SYM_CODE_START(ret_from_fork_asm) - UNWIND_HINT_REGS + /* + * This is the start of the kernel stack; even through there's a regs + * set at the top, there is no real exception frame and one cannot + * unwind further. This is the end. + * + * This ensures stack unwinds of kernel threads hit a known good state. + */ + UNWIND_HINT_END_OF_STACK ANNOTATE_NOENDBR // copy_thread CALL_DEPTH_ACCOUNT
@@ -295,6 +302,11 @@ SYM_CODE_START(ret_from_fork_asm) movq %r12, %rcx /* fn_arg */ call ret_from_fork + /* + * Set the stack state to what is expected for the target function + * -- also it is not wrong. + */ + UNWIND_HINT_REGS jmp swapgs_restore_regs_and_return_to_usermode SYM_CODE_END(ret_from_fork_asm) .popsection
Re: [PATCH v2 2/2] x86: Rewrite ret_from_fork() in C
Quoted 27 lines · Peter Zijlstra
Quoted 11 lines · Peter Zijlstra
Quoted 5 lines · Petr Mladek
This patch broke livepatching. Kthreads never have a reliable stack. It works when I revert it.
Review
Quoted 2 lines
(quoted diff) +2
-0+SYM_CODE_START(ret_from_fork_asm) + UNWIND_HINT_REGSIt works again when I change the above hint to
UNWIND_HINT_END_OF_STACK, so yeah. Doing this makes objtool unhappy with something else though, so I'll go prod at things with something sharp...The below cures things; Josh, did I miss anything?
--- arch/x86/entry/entry_64.S | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)arch/x86/entry/entry_64.S +0
-0@@ -285,7 +285,14 @@ SYM_FUNC_END(__switch_to_asm) */ .pushsection .text, "ax"
arch/x86/entry/entry_64.S SYM_FUNC_END(__switch_to_asm) · line 295
Quoted 10 lines
arch/x86/entry/entry_64.S +8
-1SYM_CODE_START(ret_from_fork_asm) - UNWIND_HINT_REGS + /* + * This is the start of the kernel stack; even through there's a regs + * set at the top, there is no real exception frame and one cannot + * unwind further. This is the end. + * + * This ensures stack unwinds of kernel threads hit a known good state. + */ + UNWIND_HINT_END_OF_STACK
So unwind_orc.c:unwind_next_frame() will terminate on this hint *or* on user_mode(state->regs).
AFAICT way things are set up in copy_thread(), user_mode() will not be true -- after all there is no usermode, the kthread would first have to exec() something to create a usermode.
Yet I'm wondering if perhaps we should spoof the regs to make user_mode() true and auto-terminate without this explicit hint.
Josh, do you remember the rationale for all this?
Josh Poimboeuf
maintainerRe: [PATCH v2 2/2] x86: Rewrite ret_from_fork() in C
Quoted 15 lines · Peter Zijlstra
Quoted 15 lines
The below cures things; Josh, did I miss anything?
--- arch/x86/entry/entry_64.S | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)arch/x86/entry/entry_64.S +0
-1@@ -285,7 +285,14 @@ SYM_FUNC_END(__switch_to_asm) */ .pushsection .text, "ax" SYM_CODE_START(ret_from_fork_asm) - UNWIND_HINT_REGS
Quoted 8 lines
arch/x86/entry/entry_64.S SYM_FUNC_END(__switch_to_asm) · line 295
Quoted 8 lines
arch/x86/entry/entry_64.S +8
-0+ /* + * This is the start of the kernel stack; even through there's a regs + * set at the top, there is no real exception frame and one cannot + * unwind further. This is the end. + * + * This ensures stack unwinds of kernel threads hit a known good state. + */ + UNWIND_HINT_END_OF_STACK
The comments may be a bit superfluous (to me at least) but the patch looks fine.
Quoted 9 lines
So unwind_orc.c:unwind_next_frame() will terminate on this hint *or* on
user_mode(state->regs).AFAICT way things are set up in
copy_thread(),user_mode()will not be true -- after all there is no usermode, the kthread would first have toexec()something to create a usermode.Yet I'm wondering if perhaps we should spoof the regs to make
user_mode()true and auto-terminate without this explicit hint.
I'm not sure that would be worth the trouble / cleverness. The hint is straightforward IMO.
Quoted 1 line
Josh, do you remember the rationale for all this?
For what exactly :-)
Signature · 1 line
Josh
Re: [PATCH v2 2/2] x86: Rewrite ret_from_fork() in C
Quoted 19 lines · Josh Poimboeuf
Quoted 18 lines · Peter Zijlstra
Quoted 18 lines
The below cures things; Josh, did I miss anything?
--- arch/x86/entry/entry_64.S | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)arch/x86/entry/entry_64.S +3
-1@@ -285,7 +285,14 @@ SYM_FUNC_END(__switch_to_asm) */ .pushsection .text, "ax" SYM_CODE_START(ret_from_fork_asm) - UNWIND_HINT_REGS + /* + * This is the start of the kernel stack; even through there's a regs + * set at the top, there is no real exception frame and one cannot
Quoted 8 lines
Quoted 5 lines
arch/x86/entry/entry_64.S SYM_FUNC_END(__switch_to_asm) · line 295
Quoted 5 lines
arch/x86/entry/entry_64.S +5
-0+ * unwind further. This is the end. + * + * This ensures stack unwinds of kernel threads hit a known good state. + */ + UNWIND_HINT_END_OF_STACKThe comments may be a bit superfluous (to me at least) but the patch looks fine.
Right, well, it took me a minute to figure out how it was all supposed to work, I figured I'd stick a comment on it.
The bit I missed is that if you reach the return-to-user part, you will actually have user_mode() true on the regset.
Quoted 7 lines
Quoted 7 lines
So unwind_orc.c:unwind_next_frame() will terminate on this hint *or* on
user_mode(state->regs).AFAICT way things are set up in
copy_thread(),user_mode()will not be true -- after all there is no usermode, the kthread would first have toexec()something to create a usermode.
Quoted 5 lines
Quoted 2 lines
Yet I'm wondering if perhaps we should spoof the regs to make
user_mode()true and auto-terminate without this explicit hint.I'm not sure that would be worth the trouble / cleverness. The hint is straightforward IMO.
I tried, it doesn't work, clearly I missed something.
Re: [PATCH v2 2/2] x86: Rewrite ret_from_fork() in C
Quoted 4 lines · Peter Zijlstra
Quoted 2 lines
I'm not sure that would be worth the trouble / cleverness. The hint is straightforward IMO.
I tried, it doesn't work, clearly I missed something.
FWIW, I tried the below. That should make user_mode() true for the kernel thread regset, and while the kernel did boot, it still fails the livepatch self-test.
The difference seems to be that END_OF_STACK terminates it right there, while REGS thinks its a valid frame and only terminates on user_mode() when unwinding one more frame. The frame at REGS clearly isn't very sane.
arch/x86/kernel/process.c +1 -0
@@ -232,6 +232,7 @@ int copy_thread(struct task_struct *p, const struct kernel_clone_args *args)* It does the same kernel frame setup to return to a kernel * function that a kernel thread does. */ + childregs->cs =3 ; childregs->sp =0 ; childregs->ip =0 ;kthread_frame_init (frame, args->fn, args->fn_arg);
Re: [PATCH v2 2/2] x86: Rewrite ret_from_fork() in C
In reply to Peter ZijlstraQuoted 14 lines · Peter Zijlstra
Quoted 11 lines · Peter Zijlstra
Quoted 5 lines · Petr Mladek
This patch broke livepatching. Kthreads never have a reliable stack. It works when I revert it.
Review
Quoted 2 lines
(quoted diff) +2
-0+SYM_CODE_START(ret_from_fork_asm) + UNWIND_HINT_REGSIt works again when I change the above hint to
UNWIND_HINT_END_OF_STACK, so yeah. Doing this makes objtool unhappy with something else though, so I'll go prod at things with something sharp...
Quoted 1 line
The below cures things; Josh, did I miss anything?
I can confirm that it solved the problem. Feel free to use:
Thanks a lot for the quick fix.
Best Regards,
Petr
Quoted 36 lines
--- arch/x86/entry/entry_64.S | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)arch/x86/entry/entry_64.S +13
-1@@ -285,7 +285,14 @@ SYM_FUNC_END(__switch_to_asm) */ .pushsection .text, "ax" SYM_CODE_START(ret_from_fork_asm) - UNWIND_HINT_REGS + /* + * This is the start of the kernel stack; even through there's a regs + * set at the top, there is no real exception frame and one cannot + * unwind further. This is the end. + * + * This ensures stack unwinds of kernel threads hit a known good state. + */ + UNWIND_HINT_END_OF_STACK ANNOTATE_NOENDBR // copy_thread CALL_DEPTH_ACCOUNT@@ -295,6 +302,11 @@ SYM_CODE_START(ret_from_fork_asm) movq %r12, %rcx /* fn_arg */ call ret_from_fork + /* + * Set the stack state to what is expected for the target function + * -- also it is not wrong. + */ + UNWIND_HINT_REGS jmp swapgs_restore_regs_and_return_to_usermode SYM_CODE_END(ret_from_fork_asm) .popsection
[tip: x86/urgent] x86: Fix kthread unwind
In reply to Peter ZijlstraThe following commit has been merged into the x86/urgent branch of tip:
The following commit has been merged into the x86/urgent branch of tip:
Commit-ID: 2e7e5bbb1c3c8d502edeb5c0670eac4995134b6f Gitweb: https://git.kernel.org/tip/2e7e5bbb1c3c8d502edeb5c0670eac4995134b6f Author: Peter Zijlstra <[email protected]> AuthorDate: Wed, 19 Jul 2023 22:15:38 +02:00 Committer: Peter Zijlstra <[email protected]> CommitterDate: Thu, 20 Jul 2023 23:03:50 +02:00
x86: Fix kthread unwind
The rewrite of ret_from_form() misplaced an unwind hint which caused all kthread stack unwinds to be marked unreliable, breaking livepatching.
Restore the annotation and add a comment to explain the how and why of things.
--- arch/x86/entry/entry_64.S | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-)
arch/x86/entry/entry_64.S +15 -1
@@ -285,7 +285,15 @@ SYM_FUNC_END(__switch_to_asm) */ .pushsection .text, "ax" SYM_CODE_START(ret_from_fork_asm) - UNWIND_HINT_REGS + /* + * This is the start of the kernel stack; even through there's a + * register set at the top, the regset isn't necessarily coherent + * (consider kthreads) and one cannot unwind further. + * + * This ensures stack unwinds of kernel threads terminate in a known + * good state. + */ + UNWIND_HINT_END_OF_STACK ANNOTATE_NOENDBR // copy_thread CALL_DEPTH_ACCOUNT
@@ -295,6 +303,12 @@ SYM_CODE_START(ret_from_fork_asm) movq %r12, %rcx /* fn_arg */ call ret_from_fork + /* + * Set the stack state to what is expected for the target function + * -- at this point the register set should be a valid user set + * and unwind should work normally. + */ + UNWIND_HINT_REGS jmp swapgs_restore_regs_and_return_to_usermode SYM_CODE_END(ret_from_fork_asm) .popsection
Joe Lawrence
maintainerRe: [PATCH v2 2/2] x86: Rewrite ret_from_fork() in C
In reply to Petr MladekQuoted 8 lines · Petr Mladek
Quoted 3 lines · Brian Gerst
When kCFI is enabled, special handling is needed for the indirect call to the kernel thread function. Rewrite the
ret_from_fork()function in C so that the compiler can properly handle the indirect call.This patch broke livepatching. Kthreads never have a reliable stack. It works when I revert it.
Just curious -- did the selftests catch this anywhere? I'm not 100% clear on what trees / frequency they all run, so maybe Petr you found this by code inspection or other means?
Signature · 1 line
Joe
Re: [PATCH v2 2/2] x86: Rewrite ret_from_fork() in C
Quoted 6 lines · Joe Lawrence
Quoted 5 lines · Petr Mladek
Quoted 3 lines · Brian Gerst
When kCFI is enabled, special handling is needed for the indirect call to the kernel thread function. Rewrite the
ret_from_fork()function in C so that the compiler can properly handle the indirect call.
Quoted 7 lines
Quoted 3 lines
This patch broke livepatching. Kthreads never have a reliable stack. It works when I revert it.
Just curious -- did the selftests catch this anywhere? I'm not 100% clear on what trees / frequency they all run, so maybe Petr you found this by code inspection or other means?
I suspect Petr ran the selftests himself, they're fairly easy to run (once you figure out the magic incantation) and insta fail.
I'm not sure the robots consistently run this stuff -- I've had these patches exposed to 0day for weeks...