Pile o' entry/exit/sp0 changes

Xen PV is fundamentally incompatible with our fancy NMI code: it doesn't use IST at all, and Xen entries clobber two stack slots below the hardware frame.

Drop Xen PV support from our NMI code entirely.

Cc Juergen Gross <[email protected]>

Cc Boris Ostrovsky <[email protected]>

Signed-off-by Andy Lutomirski <[email protected]>

I'm removing thread_struct::sp0, and Xen's usage of it is slightly dubious and unnecessary. Use appropriate helpers instead.

While we're at at, reorder the code slightly to make it more obvious what's going on.

Cc Juergen Gross <[email protected]>

Cc Boris Ostrovsky <[email protected]>

Signed-off-by Andy Lutomirski <[email protected]>

All of the code paths that ended up doing IRET to usermode did SWAPGS immediately beforehand. Move the SWAPGS into the common code.

Signed-off-by Andy Lutomirski <[email protected]>

+GLOBAL(swapgs_restore_regs_and_return_to_usermode)

Quoted 4 lines · Andy Lutomirski

All of the code paths that ended up doing IRET to usermode did SWAPGS immediately beforehand. Move the SWAPGS into the common code.

Quoted 3 lines

Signed-off-by Andy Lutomirski <[email protected]>

+GLOBAL(swapgs_restore_regs_and_return_to_usermode)

Is adding swapgs to the label really necessary? It's redundant with the usermode part. I'm noticing a trend towards absurdly verbose labels lately.

I kinda appreciate the verbosity. When I see a jump to such a label, I

The only user was the 64-bit opportunistic SYSRET failure path, and that path didn't really need it. This change makes the opportunistic SYSRET code a bit more straightforward and gets rid of the label.

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/entry/entry_64.S | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

Here's the first "real" version of this series. It's based on Linus' tree from a couple days ago.

This causees the MSR_IA32_SYSENTER_CS write to move out of the paravirt hook. This shouldn't affect Xen PV: Xen already ignores MSR_IA32_SYSENTER_ESP writes. In any event, Xen doesn't support vm86() in a useful way.

Note to any potential backporters: This patch won't break lguest, as lguest didn't have any SYSENTER support at all.

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/include/asm/processor.h |  7 -------
 arch/x86/include/asm/switch_to.h | 11 +++++++++++
 arch/x86/kernel/process_32.c     |  1 +
 arch/x86/kernel/process_64.c     |  2 +-
 arch/x86/kernel/vm86_32.c        |  6 +++++-
 5 files changed, 18 insertions(+), 9 deletions(-)

arch/x86/include/asm/processor.h +0 -7

index b390ff76e58f..0167e3e35a57 100644
@@ -520,13 +520,6 @@ static inline void
 native_load_sp0(struct tss_struct *tss, struct thread_struct *thread)
 {
        tss->x86_tss.sp0 = thread->sp0;
-#ifdef CONFIG_X86_32
-       /* Only happens when SEP is enabled, no need to test "SEP"arately: */
-       if (unlikely(tss->x86_tss.ss1 != thread->sysenter_cs)) {
-               tss->x86_tss.ss1 = thread->sysenter_cs;
-               wrmsr(MSR_IA32_SYSENTER_CS, thread->sysenter_cs, 0);
-       }
-#endif
 }
 
 static inline void native_swapgs(void)

arch/x86/include/asm/switch_to.h +11 -0

index fcc5cd387fd1..f3fa19925ae1 100644
@@ -72,4 +72,15 @@ do {                                                                 \
        ((last) = __switch_to_asm((prev), (next)));                     \
 } while (0)
 
+#ifdef CONFIG_X86_32
+static inline void refresh_sysenter_cs(struct thread_struct *thread)
+{
+       /* Only happens when SEP is enabled, no need to test "SEP"arately: */
+       if (unlikely(this_cpu_read(cpu_tss.x86_tss.ss1) == thread->sysenter_cs))
+               return;
+
+       this_cpu_write(cpu_tss.x86_tss.ss1, thread->sysenter_cs);
+}
+#endif
+
 #endif /* _ASM_X86_SWITCH_TO_H */

arch/x86/kernel/process_32.c +1 -0

index 11966251cd42..84d6c9f554d0 100644
@@ -287,6 +287,7 @@ __switch_to(struct task_struct *prev_p, struct task_struct *next_p)
         * current_thread_info().
         */
        load_sp0(tss, next);
+       refresh_sysenter_cs(next);  /* in case prev or next is vm86 */
        this_cpu_write(cpu_current_top_of_stack,
                       (unsigned long)task_stack_page(next_p) +
                       THREAD_SIZE);

arch/x86/kernel/process_64.c +1 -1

index 302e7b2572d1..a6ff6d1a0110 100644
@@ -464,7 +464,7 @@ __switch_to(struct task_struct *prev_p, struct task_struct *next_p)
         */
        this_cpu_write(current_task, next_p);
 
-       /* Reload esp0 and ss1.  This changes current_thread_info(). */
+       /* Reload sp0. */
        load_sp0(tss, next);
 
        /*

arch/x86/kernel/vm86_32.c +2 -0

index 7924a5356c8a..5bc1c3ab6287 100644
@@ -54,6 +54,7 @@
 #include <asm/irq.h>
 #include <asm/traps.h>
 #include <asm/vm86.h>
+#include <asm/switch_to.h>
 
 /*
  * Known problems:

@@ -149,6 +150,7 @@ void save_v86_state(struct kernel_vm86_regs *regs, int retval)
        tsk->thread.sp0 = vm86->saved_sp0;
        tsk->thread.sysenter_cs = __KERNEL_CS;
        load_sp0(tss, &tsk->thread);
+       refresh_sysenter_cs(&tsk->thread);
        vm86->saved_sp0 = 0;
        put_cpu();
 

@@ -368,8 +370,10 @@ static long do_sys_vm86(struct vm86plus_struct __user *user_vm86, bool plus)
        /* make room for real-mode segments */

arch/x86/include/asm/processor.h +0 -7

index b390ff76e58f..0167e3e35a57 100644
@@ -520,13 +520,6 @@ static inline void
 native_load_sp0(struct tss_struct *tss, struct thread_struct *thread)
 {
 	tss->x86_tss.sp0 = thread->sp0;
-#ifdef CONFIG_X86_32
-	/* Only happens when SEP is enabled, no need to test "SEP"arately: */
-	if (unlikely(tss->x86_tss.ss1 != thread->sysenter_cs)) {
-		tss->x86_tss.ss1 = thread->sysenter_cs;
-		wrmsr(MSR_IA32_SYSENTER_CS, thread->sysenter_cs, 0);
-	}
-#endif
 }
 
 static inline void native_swapgs(void)

arch/x86/include/asm/switch_to.h +0 -0

index fcc5cd387fd1..f3fa19925ae1 100644
@@ -72,4 +72,15 @@ do {									\
 	((last) = __switch_to_asm((prev), (next)));			\

Quoted 1 line · Dave Hansen

I kinda appreciate the verbosity. When I see a jump to such a label, I

Sorry but this and other labels' names are just ridiculously long. swapgs_restore_regs_return or swapgs_restore or somesuch are perfectly fine.

Quoted 26 lines · Andy Lutomirski

arch/x86/include/asm/processor.h +0 -7

index b390ff76e58f..0167e3e35a57 100644
@@ -520,13 +520,6 @@ static inline void
 native_load_sp0(struct tss_struct *tss, struct thread_struct *thread)
 {
      tss->x86_tss.sp0 = thread->sp0;
-#ifdef CONFIG_X86_32
-     /* Only happens when SEP is enabled, no need to test "SEP"arately: */
-     if (unlikely(tss->x86_tss.ss1 != thread->sysenter_cs)) {
-             tss->x86_tss.ss1 = thread->sysenter_cs;
-             wrmsr(MSR_IA32_SYSENTER_CS, thread->sysenter_cs, 0);
-     }
-#endif
 }
 
 static inline void native_swapgs(void)

arch/x86/include/asm/switch_to.h +0 -0

index fcc5cd387fd1..f3fa19925ae1 100644
@@ -72,4 +72,15 @@ do {                                                                       \
      ((last) = __switch_to_asm((prev), (next)));                     \
 } while (0)
 

arch/x86/include/asm/switch_to.h do { \ · line 82

Quoted 8 lines

arch/x86/include/asm/switch_to.h +8 -0

+#ifdef CONFIG_X86_32
+static inline void refresh_sysenter_cs(struct thread_struct *thread)
+{
+     /* Only happens when SEP is enabled, no need to test "SEP"arately: */
+     if (unlikely(this_cpu_read(cpu_tss.x86_tss.ss1) == thread->sysenter_cs))
+             return;
+
+     this_cpu_write(cpu_tss.x86_tss.ss1, thread->sysenter_cs);

You lost the wrmsr() on the way... Ideally you move the code unmodified first and then do the this_cpu_ change on top.

Thanks,

tglx

These code paths will diverge soon.

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/entry/entry_64.S        | 32 +++++++++++++++++++++++---------
 arch/x86/entry/entry_64_compat.S |  2 +-
 arch/x86/kernel/head_64.S        |  2 +-
 3 files changed, 25 insertions(+), 11 deletions(-)

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

index afe1f403fa0e..493e5e234d36 100644

arch/x86/entry/entry_64.S +1 -1

 END(entry_SYSCALL_64)
 
 ENTRY(stub_ptregs_64)

@@ -423,7 +423,7 @@ ENTRY(ret_from_fork)
 	call	syscall_return_slowpath	/* returns with IRQs disabled */
 	TRACE_IRQS_ON			/* user mode is traced as IRQS on */
 	SWAPGS
-	jmp	restore_regs_and_iret
+	jmp	restore_regs_and_return_to_usermode
 
 1:
 	/* kernel thread */

arch/x86/entry/entry_64.S +5 -2

+	RESTORE_EXTRA_REGS
+	RESTORE_C_REGS
+	REMOVE_PT_GPREGS_FROM_STACK 8
+	INTERRUPT_RETURN
+
 
 /* Returning to kernel space */
 retint_kernel:

@@ -632,11 +644,13 @@ retint_kernel:
 	 */
 	TRACE_IRQS_IRETQ
 
-/*
- * At this label, code paths which return to kernel and to user,

paranoid_exit_restore was a copy of restore_regs_and_return_to_kernel. Merge them and make the paranoid_exit internal labels local.

Keeping .Lparanoid_exit makes the code a bit shorter because it allows a 2-byte jnz instead of a 5-byte jnz.

Saves 96 bytes of text.

(This is still a bit suboptimal in a non-CONFIG_TRACE_IRQFLAGS
kernel, but fixing that would make the code rather messy.)

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/entry/entry_64.S | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

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

index 4ad40067162a..d6404a613df4 100644

Saves 64 bytes.

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/entry/entry_64.S | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

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

index d6404a613df4..9dafafa3e0ec 100644
@@ -313,10 +313,18 @@ return_from_SYSCALL_64:
 	 */
 syscall_return_via_sysret:
 	/* rcx and r11 are already restored (see code above) */
-	RESTORE_EXTRA_REGS
-	RESTORE_C_REGS_EXCEPT_RCX_R11
-	movq	RSP(%rsp), %rsp
 	UNWIND_HINT_EMPTY
+	POP_EXTRA_REGS
+	popq	%rsi	/* skip r11 */
+	popq	%r10
+	popq	%r9

They did almost the same thing. Remove a bunch of pointless instructions (mostly hidden in macros) and reduce cognitive load by merging them.

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/entry/entry_64.S | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

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

index 9dafafa3e0ec..c855ee91a3a5 100644
@@ -220,10 +220,9 @@ entry_SYSCALL_64_fastpath:
 	TRACE_IRQS_ON		/* user mode is traced as IRQs on */
 	movq	RIP(%rsp), %rcx
 	movq	EFLAGS(%rsp), %r11
-	RESTORE_C_REGS_EXCEPT_RCX_R11
-	movq	RSP(%rsp), %rsp
+	addq	$6*8, %rsp	/* skip extra regs -- they were preserved */
 	UNWIND_HINT_EMPTY
-	USERGS_SYSRET64
+	jmp	.Lpop_c_regs_except_rcx_r11_and_sysret
 
 1:
 	/*

Xen PV is fundamentally incompatible with our fancy NMI code: it doesn't use IST at all, and Xen entries clobber two stack slots below the hardware frame.

Drop Xen PV support from our NMI code entirely.

Cc Juergen Gross <[email protected]>

Cc Boris Ostrovsky <[email protected]>

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/entry/entry_64.S | 30 ++++++++++++++++++------------
 1 file changed, 18 insertions(+), 12 deletions(-)

This causees the MSR_IA32_SYSENTER_CS write to move out of the

paravirt hook. This shouldn't affect Xen PV: Xen already ignores MSR_IA32_SYSENTER_ESP writes. In any event, Xen doesn't support vm86() in a useful way.

Note to any potential backporters: This patch won't break lguest, as lguest didn't have any SYSENTER support at all.

load_sp0() had an odd signature:

void load_sp0(struct tss_struct *tss, struct thread_struct *thread);

Simplify it to:

void load_sp0(unsigned long sp0);

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/include/asm/paravirt.h       |  5 ++---
 arch/x86/include/asm/paravirt_types.h |  2 +-
 arch/x86/include/asm/processor.h      |  9 ++++-----
 arch/x86/kernel/cpu/common.c          |  4 ++--
 arch/x86/kernel/process_32.c          |  2 +-
 arch/x86/kernel/process_64.c          |  2 +-
 arch/x86/kernel/vm86_32.c             | 14 ++++++--------
 arch/x86/xen/enlighten_pv.c           |  7 +++----
 8 files changed, 20 insertions(+), 25 deletions(-)

On x86_64, we can easily calculate sp0 when needed instead of storing it in thread_struct.

On x86_32, a similar cleanup would be possible, but it would require cleaning up the vm86 code first, and that can wait for a later cleanup series.

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/include/asm/compat.h    |  1 +
 arch/x86/include/asm/processor.h | 33 +++++++++++++++------------------
 arch/x86/include/asm/switch_to.h |  6 ++++++
 arch/x86/kernel/process_64.c     |  1 -
 4 files changed, 22 insertions(+), 19 deletions(-)

arch/x86/include/asm/compat.h +1 -0

index 5343c19814b3..948b6d8ec46f 100644
@@ -6,6 +6,7 @@
  */
 #include <linux/types.h>
 #include <linux/sched.h>
+#include <linux/sched/task_stack.h>
 #include <asm/processor.h>
 #include <asm/user32.h>
 #include <asm/unistd.h>

arch/x86/include/asm/processor.h +9 -17

index ad59cec14239..562c575d8bc3 100644
@@ -430,7 +430,9 @@ typedef struct {
 struct thread_struct {
        /* Cached TLS descriptors: */
        struct desc_struct      tls_array[GDT_ENTRY_TLS_ENTRIES];
+#ifdef CONFIG_X86_32
        unsigned long           sp0;
+#endif
        unsigned long           sp;
 #ifdef CONFIG_X86_32
        unsigned long           sysenter_cs;

@@ -797,6 +799,13 @@ static inline void spin_lock_prefetch(const void *x)
 
 #define task_top_of_stack(task) ((unsigned long)(task_pt_regs(task) + 1))
 
+#define task_pt_regs(task) \
+({                                                                     \
+       unsigned long __ptr = (unsigned long)task_stack_page(task);     \
+       __ptr += THREAD_SIZE - TOP_OF_KERNEL_STACK_PADDING;             \
+       ((struct pt_regs *)__ptr) - 1;                                  \
+})
+
 #ifdef CONFIG_X86_32
 /*
  * User space process size: 3GB (default).

@@ -816,23 +825,6 @@ static inline void spin_lock_prefetch(const void *x)
        .addr_limit             = KERNEL_DS,                              \
 }
 
-/*
- * TOP_OF_KERNEL_STACK_PADDING reserves 8 bytes on top of the ring0 stack.
- * This is necessary to guarantee that the entire "struct pt_regs"
- * is accessible even if the CPU haven't stored the SS/ESP registers
- * on the stack (interrupt gate does not save these registers
- * when switching to the same priv ring).
- * Therefore beware: accessing the ss/esp fields of the
- * "struct pt_regs" is possible, but they may contain the
- * completely wrong values.
- */
-#define task_pt_regs(task) \
-({                                                                     \
-       unsigned long __ptr = (unsigned long)task_stack_page(task);     \
-       __ptr += THREAD_SIZE - TOP_OF_KERNEL_STACK_PADDING;             \
-       ((struct pt_regs *)__ptr) - 1;                                  \
-})
-
 #define KSTK_ESP(task)         (task_pt_regs(task)->sp)
 
 #else

@@ -865,12 +857,17 @@ static inline void spin_lock_prefetch(const void *x)
 #define STACK_TOP              TASK_SIZE_LOW
 #define STACK_TOP_MAX          TASK_SIZE_MAX
 

Quoted 3 lines · Andy Lutomirski

Here's the first "real" version of this series. It's based on Linus' tree from a couple days ago.

Not seeing any problems in this series. I was hoping it would remove more lines of code than it adds, and it's the reverse, but that seems to be mostly the new push/pop stuff. I'm taking it we couldn't just replace the 'mov' instructions in the save/restore macros..

I *do* ask that when we have code like this:

POP_EXTRA_REGS
POP_C_REGS
addq $8, %rsp

that "addq $8" would have a comment about what stack entry we're skipping.

I *think* it's 'orig_eax', but wouldn't it be nice to say so?

Linus

arch/x86/kernel/process.c line 52

Quoted 9 lines · Andy Lutomirski

arch/x86/kernel/process.c +2 -1

@@ -48,7 +48,8 @@
  */
 __visible DEFINE_PER_CPU_SHARED_ALIGNED(struct tss_struct, cpu_tss) = {
      .x86_tss = {
-             .sp0 = TOP_OF_INIT_STACK,
+             /* Initialize sp0 to a value that is definitely invalid. */
+             .sp0 = (1UL << (BITS_PER_LONG-1)) + 1,

This confused me at first: How does this not poison the init task's stack?

Should the comment maybe say something like:

The hardware only uses .sp0 (or sp1 or sp2 for that matter) when doing ring transitions. Since the init task never runs anything other than ring 0, it has no need for a valid value here. Poison it.

to clarify what's going on?

The only remaining readers in context switch code or vm86(), and they all just want to update TSS.sp0 to match the current task. Replace them all with a new helper update_sp0().

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/include/asm/switch_to.h | 6 ++++++
 arch/x86/kernel/process_32.c     | 2 +-
 arch/x86/kernel/process_64.c     | 2 +-
 arch/x86/kernel/vm86_32.c        | 4 ++--
 4 files changed, 10 insertions(+), 4 deletions(-)

cpu_current_top_of_stack's initialization forgot about TOP_OF_KERNEL_STACK_PADDING. This bug didn't matter because the idle threads never enter user mode.

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/kernel/smpboot.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

arch/x86/kernel/smpboot.c +0 -0

index ad59edd84de7..06c18fe1c09e 100644

On x86_64, we can easily calculate sp0 when needed instead of storing it in thread_struct.

On x86_32, a similar cleanup would be possible, but it would require cleaning up the vm86 code first, and that can wait for a later cleanup series.

Signed-off-by Andy Lutomirski <[email protected]>

arch/x86/include/asm/processor.h +0 -0

@@ -816,23 +825,6 @@ static inline void spin_lock_prefetch(const void *x)
 	.addr_limit		= KERNEL_DS,				  \
 }
 

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/include/asm/processor.h | 6 ++++++
 arch/x86/kernel/traps.c          | 3 +--
 2 files changed, 7 insertions(+), 2 deletions(-)

arch/x86/include/asm/processor.h +6 -0

index 562c575d8bc3..26fc33231cbd 100644
@@ -541,6 +541,12 @@ static inline unsigned long current_top_of_stack(void)
 #endif
 }
 
+static inline bool on_thread_stack(void)
+{
+	return (unsigned long)(current_top_of_stack() -
+			       current_stack_pointer) < THREAD_SIZE;
+}
+
 #ifdef CONFIG_PARAVIRT
 #include <asm/paravirt.h>
 #else

arch/x86/kernel/traps.c +0 -0

index 67db4f43309e..42a9c4458f5d 100644

Quoted 8 lines · Andy Lutomirski

On x86_64, we can easily calculate sp0 when needed instead of storing it in thread_struct.

On x86_32, a similar cleanup would be possible, but it would require cleaning up the vm86 code first, and that can wait for a later cleanup series.

Signed-off-by Andy Lutomirski <[email protected]>

...

Quoted 4 lines

arch/x86/include/asm/processor.h +0 -0

@@ -816,23 +825,6 @@ static inline void spin_lock_prefetch(const void *x)
      .addr_limit             = KERNEL_DS,                              \
 }
 

arch/x86/include/asm/processor.h static inline void spin_lock_prefetch(const void *x) · line 828

Quoted 10 lines

arch/x86/include/asm/processor.h +0 -10

-/*
- * TOP_OF_KERNEL_STACK_PADDING reserves 8 bytes on top of the ring0 stack.
- * This is necessary to guarantee that the entire "struct pt_regs"
- * is accessible even if the CPU haven't stored the SS/ESP registers
- * on the stack (interrupt gate does not save these registers
- * when switching to the same priv ring).
- * Therefore beware: accessing the ss/esp fields of the
- * "struct pt_regs" is possible, but they may contain the
- * completely wrong values.
- */

Why are we removing the explaination of TOP_OF_KERNEL_STACK_PADDING? Is that going to change later?

Quoted 28 lines · Andy Lutomirski

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/include/asm/processor.h | 6 ++++++
 arch/x86/kernel/traps.c          | 3 +--
 2 files changed, 7 insertions(+), 2 deletions(-)

arch/x86/include/asm/processor.h +6 -0

index 562c575d8bc3..26fc33231cbd 100644
@@ -541,6 +541,12 @@ static inline unsigned long current_top_of_stack(void)
 #endif
 }
 
+static inline bool on_thread_stack(void)
+{
+     return (unsigned long)(current_top_of_stack() -
+                            current_stack_pointer) < THREAD_SIZE;
+}
+
 #ifdef CONFIG_PARAVIRT
 #include <asm/paravirt.h>
 #else

arch/x86/kernel/traps.c +0 -0

index 67db4f43309e..42a9c4458f5d 100644

arch/x86/kernel/traps.c void ist_begin_non_atomic(struct pt_regs *regs)

Quoted 10 lines

arch/x86/kernel/traps.c +1 -3

@@ -141,8 +141,7 @@ void ist_begin_non_atomic(struct pt_regs *regs)
       * will catch asm bugs and any attempt to use ist_preempt_enable
       * from double_fault.
       */
-     BUG_ON((unsigned long)(current_top_of_stack() -
-                            current_stack_pointer) >= THREAD_SIZE);
+     BUG_ON(!on_thread_stack());
 
      preempt_enable_no_resched();
 }
--

Having a commit message is always better. Other than that:

Reviewed-by Borislav Petkov <[email protected]>

--
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg) --

Quoted 24 lines · Andy Lutomirski

They did almost the same thing. Remove a bunch of pointless instructions (mostly hidden in macros) and reduce cognitive load by merging them.

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/entry/entry_64.S | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

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

index 9dafafa3e0ec..c855ee91a3a5 100644
@@ -220,10 +220,9 @@ entry_SYSCALL_64_fastpath:
      TRACE_IRQS_ON           /* user mode is traced as IRQs on */
      movq    RIP(%rsp), %rcx
      movq    EFLAGS(%rsp), %r11
-     RESTORE_C_REGS_EXCEPT_RCX_R11
-     movq    RSP(%rsp), %rsp
+     addq    $6*8, %rsp      /* skip extra regs -- they were preserved */
      UNWIND_HINT_EMPTY
-     USERGS_SYSRET64
+     jmp     .Lpop_c_regs_except_rcx_r11_and_sysret
 

arch/x86/entry/entry_64.S syscall_return_via_sysret: · line 317

Quoted 7 lines

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

 1:
      /*

@@ -315,6 +314,7 @@ syscall_return_via_sysret:
      /* rcx and r11 are already restored (see code above) */
      UNWIND_HINT_EMPTY
      POP_EXTRA_REGS
+.Lpop_c_regs_except_rcx_r11_and_sysret:

.Lpop_regs_sysret I guess.

Quoted 14 lines · Andy Lutomirski

These code paths will diverge soon.

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/entry/entry_64.S        | 32 +++++++++++++++++++++++---------
 arch/x86/entry/entry_64_compat.S |  2 +-
 arch/x86/kernel/head_64.S        |  2 +-
 3 files changed, 25 insertions(+), 11 deletions(-)

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

index afe1f403fa0e..493e5e234d36 100644

arch/x86/entry/entry_64.S syscall_return_via_sysret: · line 324

Quoted 5 lines

arch/x86/entry/entry_64.S +1 -1

@@ -321,7 +321,7 @@ syscall_return_via_sysret:
 
 opportunistic_sysret_failed:
      SWAPGS
-     jmp     restore_regs_and_iret
+     jmp     restore_regs_and_return_to_usermode

rstor_regs_ret_user

still sounds pretty ok to me and it is shorter and it is using the *RSTOR spelling the respective FPU insns use, so should be recognizable. :-)

And the other one could be

rstor_regs_ret_kernel

of course.

Quoted 13 lines

arch/x86/entry/entry_64.S +1 -1

 END(entry_SYSCALL_64)
 
 ENTRY(stub_ptregs_64)

@@ -423,7 +423,7 @@ ENTRY(ret_from_fork)
      call    syscall_return_slowpath /* returns with IRQs disabled */
      TRACE_IRQS_ON                   /* user mode is traced as IRQS on */
      SWAPGS
-     jmp     restore_regs_and_iret
+     jmp     restore_regs_and_return_to_usermode
 
 1:
      /* kernel thread */

arch/x86/entry/entry_64.S GLOBAL(retint_user) · line 618

Quoted 8 lines

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

@@ -612,7 +612,19 @@ GLOBAL(retint_user)
      call    prepare_exit_to_usermode
      TRACE_IRQS_IRETQ
      SWAPGS
-     jmp     restore_regs_and_iret
+
+GLOBAL(restore_regs_and_return_to_usermode)
+#ifdef CONFIG_DEBUG_ENTRY
+     testl   $3, CS(%rsp)

testb

ditto for the other spot.

arch/x86/entry/entry_64.S GLOBAL(retint_user) · line 622

Quoted 4 lines

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

+     jnz     1f
+     ud2
+1:
+#endif

Why aren't we exploding here unconditionally? I mean, we don't want to return with RPL != 3 CS for whatever reason...

arch/x86/entry/entry_64.S GLOBAL(retint_user) · line 620

Quoted 5 lines · Andy Lutomirski

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

+GLOBAL(restore_regs_and_return_to_usermode)
+#ifdef CONFIG_DEBUG_ENTRY
+     testl   $3, CS(%rsp)
+     jnz     1f
+     ud2

A nit from the mere mortals in the audience: Could we start commenting or make a constant for the user segment bits in CS?

Also, it would be nice to explain what's going on here. Maybe:

/*
* We think we are returning to the kernel. Check the
* registers we are about to restore and if we appear to
* be returning to userspace, do something that will cause
* a fault and hopefully an oops report.
*/

Otherwise, I really like this change. It's really hard to figure out what the context is in the entry assembly in a lot of cases. It's a place where code reuse actually makes things harder to follow.

I'm inclined to leave it. I first wrote it without the long name and then I had to re-read the code to make sure I got the register state right. The long name serves as documentation that we need rcx and r11 to have special contents. There's only one place that jumps there and it's in the same function.

They did almost the same thing. Remove a bunch of pointless instructions (mostly hidden in macros) and reduce cognitive load by merging them.

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/entry/entry_64.S | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

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

index 9dafafa3e0ec..c855ee91a3a5 100644
@@ -220,10 +220,9 @@ entry_SYSCALL_64_fastpath:
        TRACE_IRQS_ON           /* user mode is traced as IRQs on */
        movq    RIP(%rsp), %rcx
        movq    EFLAGS(%rsp), %r11
-       RESTORE_C_REGS_EXCEPT_RCX_R11
-       movq    RSP(%rsp), %rsp
+       addq    $6*8, %rsp      /* skip extra regs -- they were preserved */
        UNWIND_HINT_EMPTY
-       USERGS_SYSRET64
+       jmp     .Lpop_c_regs_except_rcx_r11_and_sysret
 
 1:
        /*

Quoted 5 lines · Andy Lutomirski

I'm inclined to leave it. I first wrote it without the long name and then I had to re-read the code to make sure I got the register state right. The long name serves as documentation that we need rcx and r11 to have special contents. There's only one place that jumps there and it's in the same function.

Frankly, I'd prefer a sane label name and a good comment over it instead of insanely long label name which is not as explanatory as a normal english sentence or two.

Quoted 24 lines · Andy Lutomirski

They did almost the same thing. Remove a bunch of pointless instructions (mostly hidden in macros) and reduce cognitive load by merging them.

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/entry/entry_64.S | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

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

index 9dafafa3e0ec..c855ee91a3a5 100644
@@ -220,10 +220,9 @@ entry_SYSCALL_64_fastpath:
        TRACE_IRQS_ON           /* user mode is traced as IRQs on */
        movq    RIP(%rsp), %rcx
        movq    EFLAGS(%rsp), %r11
-       RESTORE_C_REGS_EXCEPT_RCX_R11
-       movq    RSP(%rsp), %rsp
+       addq    $6*8, %rsp      /* skip extra regs -- they were preserved */
        UNWIND_HINT_EMPTY
-       USERGS_SYSRET64
+       jmp     .Lpop_c_regs_except_rcx_r11_and_sysret
 

arch/x86/entry/entry_64.S syscall_return_via_sysret: · line 320

Quoted 10 lines

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

 1:
        /*

@@ -315,6 +314,7 @@ syscall_return_via_sysret:
        /* rcx and r11 are already restored (see code above) */
        UNWIND_HINT_EMPTY
        POP_EXTRA_REGS
+.Lpop_c_regs_except_rcx_r11_and_sysret:
        popq    %rsi    /* skip r11 */
        popq    %r10
        popq    %r9

Wouldn't it be more logical to keep the SYSRET path at the end of the fast path (reverse of what you are doing here)? That way the fast path falls through without jumping.

The only user was the 64-bit opportunistic SYSRET failure path, and that path didn't really need it. This change makes the opportunistic SYSRET code a bit more straightforward and gets rid of the label.

Signed-off-by Andy Lutomirski <[email protected]>

Reviewed-by Borislav Petkov <[email protected]>

---
 arch/x86/entry/entry_64.S | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

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

index 49167258d587..afe1f403fa0e 100644
@@ -245,7 +245,6 @@ entry_SYSCALL64_slow_path:
 	call	do_syscall_64		/* returns with IRQs disabled */
 
 return_from_SYSCALL_64:
-	RESTORE_EXTRA_REGS
 	TRACE_IRQS_IRETQ		/* we're about to change IF */
 
 	/*

@@ -314,6 +313,7 @@ return_from_SYSCALL_64:
 	 */
 syscall_return_via_sysret:
 	/* rcx and r11 are already restored (see code above) */
+	RESTORE_EXTRA_REGS
 	RESTORE_C_REGS_EXCEPT_RCX_R11
 	movq	RSP(%rsp), %rsp
 	UNWIND_HINT_EMPTY

@@ -321,7 +321,7 @@ syscall_return_via_sysret:
 
 opportunistic_sysret_failed:
 	SWAPGS
-	jmp	restore_c_regs_and_iret
+	jmp	restore_regs_and_iret
 END(entry_SYSCALL_64)
 
 ENTRY(stub_ptregs_64)

@@ -638,7 +638,6 @@ retint_kernel:
  */
 GLOBAL(restore_regs_and_iret)
 	RESTORE_EXTRA_REGS
-restore_c_regs_and_iret:
 	RESTORE_C_REGS
 	REMOVE_PT_GPREGS_FROM_STACK 8
 	INTERRUPT_RETURN