Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20191219115812.102620-1-brgerst@gmail.com>
In-Reply-To: <20191219115812.102620-1-brgerst@gmail.com>
From:   Andy Lutomirski <luto@kernel.org>
Date:   Thu, 19 Dec 2019 17:49:58 -0800
Message-ID: <CALCETrW1zE0Uufrg_UG4JNQKMy3UFxnd+XmZye2gdTV36C-yTw@mail.gmail.com>
Subject: Re: [PATCH] x86: Remove force_iret()
To:     Brian Gerst <brgerst@gmail.com>
Cc:     X86 ML <x86@kernel.org>, LKML <linux-kernel@vger.kernel.org>,
        Ingo Molnar <mingo@kernel.org>,
        "H . Peter Anvin" <hpa@zytor.com>,
        Boris Ostrovsky <boris.ostrovsky@oracle.com>,
        Oleg Nesterov <oleg@redhat.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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
>