Messages in this thread | | | From | Andy Lutomirski <> | Date | Thu, 19 Dec 2019 17:49:58 -0800 | Subject | Re: [PATCH] x86: Remove force_iret() |
| |
On Thu, Dec 19, 2019 at 3:58 AM Brian Gerst <brgerst@gmail.com> wrote: > > force_iret() was originally intended to prevent the return to user mode with > the SYSRET or SYSEXIT instructions, in cases where the register state could > have been changed to be incompatible with those instructions.
It's more than that. Before the big syscall rework, we didn't restore the caller-saved regs. See:
commit 21d375b6b34ff511a507de27bf316b3dde6938d9 Author: Andy Lutomirski <luto@kernel.org> Date: Sun Jan 28 10:38:49 2018 -0800
x86/entry/64: Remove the SYSCALL64 fast path
So if you changed r12, for example, the change would get lost.
The entry code > has been significantly reworked since then, and register state is validated > before SYSRET or SYSEXIT are used. force_iret() no longer serves its original > purpose and can be eliminated. > > Signed-off-by: Brian Gerst <brgerst@gmail.com> > --- > arch/x86/ia32/ia32_signal.c | 2 -- > arch/x86/include/asm/ptrace.h | 16 ---------------- > arch/x86/include/asm/thread_info.h | 9 --------- > arch/x86/kernel/process_32.c | 1 - > arch/x86/kernel/process_64.c | 1 - > arch/x86/kernel/signal.c | 2 -- > arch/x86/kernel/vm86_32.c | 1 - > 7 files changed, 32 deletions(-) > > diff --git a/arch/x86/ia32/ia32_signal.c b/arch/x86/ia32/ia32_signal.c > index 30416d7f19d4..a3aefe9b9401 100644 > --- a/arch/x86/ia32/ia32_signal.c > +++ b/arch/x86/ia32/ia32_signal.c > @@ -114,8 +114,6 @@ static int ia32_restore_sigcontext(struct pt_regs *regs, > > err |= fpu__restore_sig(buf, 1); > > - force_iret(); > - > return err; > } > > diff --git a/arch/x86/include/asm/ptrace.h b/arch/x86/include/asm/ptrace.h > index 5057a8ed100b..78897a8da01f 100644 > --- a/arch/x86/include/asm/ptrace.h > +++ b/arch/x86/include/asm/ptrace.h > @@ -339,22 +339,6 @@ static inline unsigned long regs_get_kernel_argument(struct pt_regs *regs, > > #define ARCH_HAS_USER_SINGLE_STEP_REPORT > > -/* > - * When hitting ptrace_stop(), we cannot return using SYSRET because > - * that does not restore the full CPU state, only a minimal set. The > - * ptracer can change arbitrary register values, which is usually okay > - * because the usual ptrace stops run off the signal delivery path which > - * forces IRET; however, ptrace_event() stops happen in arbitrary places > - * in the kernel and don't force IRET path. > - * > - * So force IRET path after a ptrace stop. > - */ > -#define arch_ptrace_stop_needed(code, info) \ > -({ \ > - force_iret(); \ > - false; \ > -}) > - > struct user_desc; > extern int do_get_thread_area(struct task_struct *p, int idx, > struct user_desc __user *info); > diff --git a/arch/x86/include/asm/thread_info.h b/arch/x86/include/asm/thread_info.h > index d779366ce3f8..cf4327986e98 100644 > --- a/arch/x86/include/asm/thread_info.h > +++ b/arch/x86/include/asm/thread_info.h > @@ -239,15 +239,6 @@ static inline int arch_within_stack_frames(const void * const stack, > current_thread_info()->status & TS_COMPAT) > #endif > > -/* > - * Force syscall return via IRET by making it look as if there was > - * some work pending. IRET is our most capable (but slowest) syscall > - * return path, which is able to restore modified SS, CS and certain > - * EFLAGS values that other (fast) syscall return instructions > - * are not able to restore properly. > - */ > -#define force_iret() set_thread_flag(TIF_NOTIFY_RESUME) > - > extern void arch_task_cache_init(void); > extern int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src); > extern void arch_release_task_struct(struct task_struct *tsk); > diff --git a/arch/x86/kernel/process_32.c b/arch/x86/kernel/process_32.c > index 323499f48858..5052ced43373 100644 > --- a/arch/x86/kernel/process_32.c > +++ b/arch/x86/kernel/process_32.c > @@ -124,7 +124,6 @@ start_thread(struct pt_regs *regs, unsigned long new_ip, unsigned long new_sp) > regs->ip = new_ip; > regs->sp = new_sp; > regs->flags = X86_EFLAGS_IF; > - force_iret(); > } > EXPORT_SYMBOL_GPL(start_thread); > > diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c > index 506d66830d4d..ffd497804dbc 100644 > --- a/arch/x86/kernel/process_64.c > +++ b/arch/x86/kernel/process_64.c > @@ -394,7 +394,6 @@ start_thread_common(struct pt_regs *regs, unsigned long new_ip, > regs->cs = _cs; > regs->ss = _ss; > regs->flags = X86_EFLAGS_IF; > - force_iret(); > } > > void > diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c > index 8eb7193e158d..8a29573851a3 100644 > --- a/arch/x86/kernel/signal.c > +++ b/arch/x86/kernel/signal.c > @@ -151,8 +151,6 @@ static int restore_sigcontext(struct pt_regs *regs, > > err |= fpu__restore_sig(buf, IS_ENABLED(CONFIG_X86_32)); > > - force_iret(); > - > return err; > } > > diff --git a/arch/x86/kernel/vm86_32.c b/arch/x86/kernel/vm86_32.c > index a76c12b38e92..91d55454e702 100644 > --- a/arch/x86/kernel/vm86_32.c > +++ b/arch/x86/kernel/vm86_32.c > @@ -381,7 +381,6 @@ static long do_sys_vm86(struct vm86plus_struct __user *user_vm86, bool plus) > mark_screen_rdonly(tsk->mm); > > memcpy((struct kernel_vm86_regs *)regs, &vm86regs, sizeof(vm86regs)); > - force_iret(); > return regs->ax; > } > > -- > 2.23.0 >
| |