Entry stuff, in decent shape now

This will simplify future changes that want scratch variables early in the SYSENTER handler -- they'll be able to spill registers to the stack. It also lets us get rid of a SWAPGS_UNSAFE_STACK user.

This does not depend on CONFIG_IA32_EMULATION because we'll want the stack space even without IA32 emulation.

As far as I can tell, the reason that this wasn't done from day 1 is that we use IST for #DB and #BP, which is IMO rather nasty and causes a lot more problems than it solves. But, since #DB uses IST, we don't actually need a real stack for SYSENTER (because SYSENTER with TF set will invoke #DB on the IST stack rather than the SYSENTER stack). I want to remove IST usage from these vectors some day, and this patch is a prerequisite for that as well.

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

This will simplify future changes that want scratch variables early in the SYSENTER handler -- they'll be able to spill registers to the stack. It also lets us get rid of a SWAPGS_UNSAFE_STACK user.

This does not depend on CONFIG_IA32_EMULATION because we'll want the stack space even without IA32 emulation.

As far as I can tell, the reason that this wasn't done from day 1 is that we use IST for #DB and #BP, which is IMO rather nasty and causes a lot more problems than it solves. But, since #DB uses IST, we don't actually need a real stack for SYSENTER (because SYSENTER with TF set will invoke #DB on the IST stack rather than the SYSENTER stack). I want to remove IST usage from these vectors some day, and this patch is a prerequisite for that as well.

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

---
 arch/x86/entry/entry_64_compat.S | 2 +-
 arch/x86/include/asm/processor.h | 3 ---
 arch/x86/kernel/asm-offsets.c    | 5 +++++
 arch/x86/kernel/asm-offsets_32.c | 5 -----
 arch/x86/kernel/cpu/common.c     | 4 +++-
 arch/x86/kernel/process.c        | 2 --
 arch/x86/kernel/traps.c          | 3 +--
 7 files changed, 10 insertions(+), 14 deletions(-)

+bool in_SYSENTER_stack(unsigned long *stack, struct stack_info *info)

Quoted 1 line · Andy Lutomirski

+bool in_SYSENTER_stack(unsigned long *stack, struct stack_info *info)

Can you make it lowercase for consistency with the other in_*_stack() functions? For example, in_irq_stack() is all lowercase even though "IRQ" is normally written in uppercase.

But also, I'm wondering whether this get_stack_info() support is even really needed.

As currently written, the trampoline code doesn't have any ORC data associated with it. So the unwinder would never have the need to actually read the SYSENTER stack.

You _could_ add an UNWIND_HINT_IRET_REGS annotation after the simulated iret frame is written, which would allow the unwinder to dump those regs when unwinding from an NMI.

But there's only a tiny window where that would be possible: only a few instructions. I'm not sure that would be worth the effort, unless we got to the point where we expect to have 100% unwinder coverage. But that's currently unrealistic anyway because of generated code and runtime patching.

--
Josh

Quoted 14 lines · Josh Poimboeuf

Quoted 1 line · Andy Lutomirski

+bool in_SYSENTER_stack(unsigned long *stack, struct stack_info *info)

Can you make it lowercase for consistency with the other in_*_stack() functions? For example, in_irq_stack() is all lowercase even though "IRQ" is normally written in uppercase.

But also, I'm wondering whether this get_stack_info() support is even really needed.

As currently written, the trampoline code doesn't have any ORC data associated with it. So the unwinder would never have the need to actually read the SYSENTER stack.

Quoted 3 lines

You _could_ add an UNWIND_HINT_IRET_REGS annotation after the simulated iret frame is written, which would allow the unwinder to dump those regs when unwinding from an NMI.

There's some ORC data in the non-trampoline SYSENTER path

but, more importantly, the OOPS unwinder will just bail without this patch. With the patch, we get a valid unwind, except that everything has a ? in front.

Quoted 5 lines

But there's only a tiny window where that would be possible: only a few instructions. I'm not sure that would be worth the effort, unless we got to the point where we expect to have 100% unwinder coverage. But that's currently unrealistic anyway because of generated code and runtime patching.

I tripped it myself several times when debugging this code.

Quoted 15 lines · Andy Lutomirski

Quoted 14 lines · Josh Poimboeuf

Quoted 1 line · Andy Lutomirski

+bool in_SYSENTER_stack(unsigned long *stack, struct stack_info *info)

Can you make it lowercase for consistency with the other in_*_stack() functions? For example, in_irq_stack() is all lowercase even though "IRQ" is normally written in uppercase.

But also, I'm wondering whether this get_stack_info() support is even really needed.

As currently written, the trampoline code doesn't have any ORC data associated with it. So the unwinder would never have the need to actually read the SYSENTER stack.

Quoted 5 lines

Quoted 3 lines

You _could_ add an UNWIND_HINT_IRET_REGS annotation after the simulated iret frame is written, which would allow the unwinder to dump those regs when unwinding from an NMI.

There's some ORC data in the non-trampoline SYSENTER path

But that's *after* the stack switch to the real kernel stack, right?

Quoted 4 lines

but, more importantly, the OOPS unwinder will just bail without this patch. With the patch, we get a valid unwind, except that everything has a ? in front.

Hm. I can't even fathom how that's possible. Are you talking about the "unwind from NMI to SYSENTER stack" path? Or any unwind to a syscall? Either way I'm baffled... If the unwinder only encounters the SYSENTER stack at the end, how could that cause everything beforehand to have a question mark?

Quoted 8 lines

Quoted 6 lines

But there's only a tiny window where that would be possible: only a few instructions. I'm not sure that would be worth the effort, unless we got to the point where we expect to have 100% unwinder coverage. But that's currently unrealistic anyway because of generated code and runtime patching.

I tripped it myself several times when debugging this code.

Again I don't see how this patch would help if there's no ORC data for the code which uses the SYSENTER stack. I must be missing something.

Quoted 4 lines

Quoted 3 lines

but, more importantly, the OOPS unwinder will just bail without this patch. With the patch, we get a valid unwind, except that everything has a ? in front.

Quoted 5 lines

Hm. I can't even fathom how that's possible. Are you talking about the "unwind from NMI to SYSENTER stack" path? Or any unwind to a syscall? Either way I'm baffled... If the unwinder only encounters the SYSENTER stack at the end, how could that cause everything beforehand to have a question mark?

I mean that, if I put a ud2 or other bug in the code that runs on the SYSENTER stack, without this patch, I get a totally blank call trace.

Quoted 10 lines · Andy Lutomirski

Quoted 9 lines

Quoted 3 lines

but, more importantly, the OOPS unwinder will just bail without this patch. With the patch, we get a valid unwind, except that everything has a ? in front.

Hm. I can't even fathom how that's possible. Are you talking about the "unwind from NMI to SYSENTER stack" path? Or any unwind to a syscall? Either way I'm baffled... If the unwinder only encounters the SYSENTER stack at the end, how could that cause everything beforehand to have a question mark?

Quoted 2 lines

I mean that, if I put a ud2 or other bug in the code that runs on the SYSENTER stack, without this patch, I get a totally blank call trace.

I would expect a blank call trace either way...

In fact I just added a ud1 after your RDI save, and got a blank call trace.

Also the RIP printout isn't very helpful, I guess kallsyms doesn't know about the trampoline?

Quoted 11 lines · Josh Poimboeuf

Quoted 10 lines · Andy Lutomirski

Quoted 9 lines

Quoted 3 lines

but, more importantly, the OOPS unwinder will just bail without this patch. With the patch, we get a valid unwind, except that everything has a ? in front.

Hm. I can't even fathom how that's possible. Are you talking about the "unwind from NMI to SYSENTER stack" path? Or any unwind to a syscall? Either way I'm baffled... If the unwinder only encounters the SYSENTER stack at the end, how could that cause everything beforehand to have a question mark?

Quoted 4 lines

Quoted 2 lines

I mean that, if I put a ud2 or other bug in the code that runs on the SYSENTER stack, without this patch, I get a totally blank call trace.

I would expect a blank call trace either way...

Try making sync_regs use a few kB of stack space or, better yet, call a non-inlined function that uses too much stack.

Quoted 12 lines · Andy Lutomirski

Quoted 11 lines · Josh Poimboeuf

Quoted 10 lines · Andy Lutomirski

Quoted 9 lines

Quoted 3 lines

but, more importantly, the OOPS unwinder will just bail without this patch. With the patch, we get a valid unwind, except that everything has a ? in front.

Hm. I can't even fathom how that's possible. Are you talking about the "unwind from NMI to SYSENTER stack" path? Or any unwind to a syscall? Either way I'm baffled... If the unwinder only encounters the SYSENTER stack at the end, how could that cause everything beforehand to have a question mark?

Quoted 7 lines

Quoted 4 lines

Quoted 2 lines

I mean that, if I put a ud2 or other bug in the code that runs on the SYSENTER stack, without this patch, I get a totally blank call trace.

I would expect a blank call trace either way...

Try making sync_regs use a few kB of stack space or, better yet, call a non-inlined function that uses too much stack.

You mean overflow the exception stack? I still don't see how that would do it.

If you could show a specific example, with splats from before/after, that would be helpful. Because I still have no idea how this patch could possibly help.

--
Josh

Quoted 21 lines · Josh Poimboeuf

Quoted 12 lines · Andy Lutomirski

Quoted 11 lines · Josh Poimboeuf

Quoted 10 lines · Andy Lutomirski

Quoted 9 lines

Quoted 3 lines

but, more importantly, the OOPS unwinder will just bail without this patch. With the patch, we get a valid unwind, except that everything has a ? in front.

Hm. I can't even fathom how that's possible. Are you talking about the "unwind from NMI to SYSENTER stack" path? Or any unwind to a syscall? Either way I'm baffled... If the unwinder only encounters the SYSENTER stack at the end, how could that cause everything beforehand to have a question mark?

Quoted 7 lines

Quoted 4 lines

Quoted 2 lines

I mean that, if I put a ud2 or other bug in the code that runs on the SYSENTER stack, without this patch, I get a totally blank call trace.

I would expect a blank call trace either way...

Try making sync_regs use a few kB of stack space or, better yet, call a non-inlined function that uses too much stack.

Quoted 6 lines

You mean overflow the exception stack? I still don't see how that would do it.

If you could show a specific example, with splats from before/after, that would be helpful. Because I still have no idea how this patch could possibly help.

I added BUG() to sync_regs(). With the patch, I get:

[    4.211553] PANIC: double fault, error_code: 0x0
[    4.212113] CPU: 0 PID: 1 Comm: sh Not tainted 4.14.0+ #920
[    4.212741] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
BIOS 1.10.2-1.fc26 04/01/2014
[    4.213536] task: ffff88001aa18000 task.stack: ffff88001aa20000
[    4.214059] RIP: 0010:do_error_trap+0x33/0x1c0
[    4.214449] RSP: 0000:ffffffffff1b8f78 EFLAGS: 00010096
[    4.214934] RAX: dffffc0000000000 RBX: ffffffffff1b8f90 RCX: 0000000000000006
[    4.215554] RDX: ffffffff82048b20 RSI: 0000000000000000 RDI: ffffffffff1b9110
[    4.216176] RBP: ffffffffff1b9088 R08: 0000000000000004 R09: 0000000000000000
[    4.216793] R10: 0000000000000000 R11: fffffbffffe3723f R12: 0000000000000006
[    4.217419] R13: 0000000000000000 R14: 0000000000000004 R15: 0000000000000000
[    4.218046] FS:  0000000000000000(0000) GS:ffff88001ae00000(0000)
knlGS:0000000000000000
[    4.218775] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[    4.219280] CR2: ffffffffff1b8f68 CR3: 00000000193da002 CR4: 00000000003606f0
[    4.219931] Call Trace:
[    4.220156]  <SYSENTER>

[    4.220383]  ? async_page_fault+0x36/0x60
[    4.220768]  ? invalid_op+0x22/0x40
[    4.221087]  ? async_page_fault+0x36/0x60
[    4.221442]  ? sync_regs+0x3c/0x40
[    4.221745]  ? sync_regs+0x2e/0x40
[    4.222051]  ? error_entry+0x6c/0xd0
[    4.222395]  ? async_page_fault+0x36/0x60
[    4.222748]  </SYSENTER>

We currently have CPU 0's GDT at the top of the GDT range and higher-numbered CPUs at lower addreses. This happens because the fixmap is upside down (index 0 is the top of the fixmap).

Flip it so that GDTs are in ascending order by virtual address. This will simplify a future patch that will generalize the GDT remap to contain multiple pages.

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

We currently have CPU 0's GDT at the top of the GDT range and higher-numbered CPUs at lower addreses. This happens because the

fixmap is upside down (index 0 is the top of the fixmap).

Flip it so that GDTs are in ascending order by virtual address. This will simplify a future patch that will generalize the GDT remap to contain multiple pages.

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

---
 arch/x86/include/asm/desc.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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

index 4011cb03ef08..95cd95eb7285 100644

/* Provide the fixmap address of the remapped GDT */ static inline struct desc_struct *get_cpu_gdt_ro(int cpu) {

arch/x86/include/asm/desc.h +1 -2

-	unsigned int idx = get_cpu_gdt_ro_index(cpu);
-	return (struct desc_struct *)__fix_to_virt(idx);
+	return (struct desc_struct *)&get_cpu_entry_area(cpu)->gdt;
 }
 
 /* Provide the current read-only GDT */

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

index b0c505fe9a95..5ff8e251772a 100644

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

+ */
+struct cpu_entry_area
+{
+	char gdt[PAGE_SIZE];
+};
+
+#define CPU_ENTRY_AREA_PAGES (sizeof(struct cpu_entry_area) / PAGE_SIZE)
 
+static inline unsigned int __get_cpu_entry_area_page_index(int cpu, int page)
+{
+	BUILD_BUG_ON(sizeof(struct cpu_entry_area) % PAGE_SIZE != 0);
+

On Mon, 20 Nov 2017, Andy Lutomirski wrote:

Just a few nits.

Quoted 17 lines

/* Provide the fixmap address of the remapped GDT */ static inline struct desc_struct *get_cpu_gdt_ro(int cpu) {

arch/x86/include/asm/desc.h +1 -2

-     unsigned int idx = get_cpu_gdt_ro_index(cpu);
-     return (struct desc_struct *)__fix_to_virt(idx);
+     return (struct desc_struct *)&get_cpu_entry_area(cpu)->gdt;
 }
 
 /* Provide the current read-only GDT */

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

index b0c505fe9a95..5ff8e251772a 100644
@@ -44,6 +44,17 @@ extern unsigned long __FIXADDR_TOP;
                       PAGE_SIZE)
 #endif
 

arch/x86/include/asm/fixmap.h extern unsigned long __FIXADDR_TOP; · line 49

Quoted 3 lines

arch/x86/include/asm/fixmap.h +3 -0

+/*
+ * cpu_entry_area is a percpu region in the fixmap that contains things
+ * need by the CPU and early entry/exit code.  Real types aren't used here

s/need/needed/

arch/x86/include/asm/fixmap.h extern unsigned long __FIXADDR_TOP; · line 50

Quoted 2 lines

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

+ * to avoid circular header dependencies.

:(

Quoted 8 lines

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

+ */
+struct cpu_entry_area
+{
+     char gdt[PAGE_SIZE];
+};
+
+#define CPU_ENTRY_AREA_PAGES (sizeof(struct cpu_entry_area) / PAGE_SIZE)

Quoted 4 lines

arch/x86/include/asm/fixmap.h +4 -0

+static inline unsigned int __get_cpu_entry_area_page_index(int cpu, int page)
+{
+     BUILD_BUG_ON(sizeof(struct cpu_entry_area) % PAGE_SIZE != 0);
+

arch/x86/include/asm/fixmap.h void __init *early_memremap_decrypted_wp(resource_size_t phys_addr, · line 218

Quoted 10 lines

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

+     return FIX_CPU_ENTRY_AREA_BOTTOM - cpu*CPU_ENTRY_AREA_PAGES - page;
+}
+
+#define __get_cpu_entry_area_offset_index(cpu, offset) ({            \
+     BUILD_BUG_ON(offset % PAGE_SIZE != 0);                          \
+     __get_cpu_entry_area_page_index(cpu, offset / PAGE_SIZE);       \
+     })
+
+#define get_cpu_entry_area_index(cpu, field)                         \
+     __get_cpu_entry_area_offset_index((cpu), offsetof(struct cpu_entry_area, field))

Any reason why those need to be macros?

arch/x86/include/asm/fixmap.h void __init *early_memremap_decrypted_wp(resource_size_t phys_addr, · line 225

Quoted 7 lines

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

+static inline struct cpu_entry_area *get_cpu_entry_area(int cpu)
+{
+     return (struct cpu_entry_area *)
+             __fix_to_virt(__get_cpu_entry_area_page_index(cpu, 0));
+}
+

Thanks,

tglx

Quoted 4 lines · Thomas Gleixner

Quoted 1 line · Andy Lutomirski

+ * to avoid circular header dependencies.

:(

Hmm. I could probably fix this, but it involves (at least) moving a struct definition and adding several new includes, and I'm not sure it'll actually converge to something working.

Quoted 8 lines

Quoted 7 lines

+ */

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

+struct cpu_entry_area
+{
+     char gdt[PAGE_SIZE];
+};
+
+#define CPU_ENTRY_AREA_PAGES (sizeof(struct cpu_entry_area) / PAGE_SIZE)

Quoted 16 lines

Quoted 4 lines

arch/x86/include/asm/fixmap.h +4 -0

+static inline unsigned int __get_cpu_entry_area_page_index(int cpu, int page)
+{
+     BUILD_BUG_ON(sizeof(struct cpu_entry_area) % PAGE_SIZE != 0);
+

arch/x86/include/asm/fixmap.h void __init *early_memremap_decrypted_wp(resource_size_t phys_addr, · line 218

Quoted 10 lines

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

+     return FIX_CPU_ENTRY_AREA_BOTTOM - cpu*CPU_ENTRY_AREA_PAGES - page;
+}
+
+#define __get_cpu_entry_area_offset_index(cpu, offset) ({            \
+     BUILD_BUG_ON(offset % PAGE_SIZE != 0);                          \
+     __get_cpu_entry_area_page_index(cpu, offset / PAGE_SIZE);       \
+     })
+
+#define get_cpu_entry_area_index(cpu, field)                         \
+     __get_cpu_entry_area_offset_index((cpu), offsetof(struct cpu_entry_area, field))

Any reason why those need to be macros?

The former is a macro because I doubt that BUILD_BUG_ON is valid in that context in a function.

The latter is a macro because 'field' is a name, not a value.

I'm going to move SYSENTER_stack to the beginning of cpu_tss to help detect overflow. Before this can happen, I need to fix several code paths that hardcode assumptions about the old layout.

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

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

+	 */
+	unsigned long		SYSENTER_stack_canary;
+	unsigned long		SYSENTER_stack[64];
+
+	/*
+	 * The fixed hardware portion.  This must not cross a page boundary
+	 * at risk of violating the SDM's advice and potentially triggering
+	 * errata.
 	 */
 	struct x86_hw_tss	x86_tss;

arch/x86/include/asm/processor.h struct x86_hw_tss { · line 334

Quoted 8 lines · Andy Lutomirski

 struct tss_struct {
      /*

arch/x86/include/asm/processor.h +5 -1

-      * The hardware state:
+      * Space for the temporary SYSENTER stack.  Used for the entry
+      * trampoline as well.  Size it such that tss_struct ends up
+      * as a multiple of PAGE_SIZE.  This calculation assumes that
+      * io_bitmap is a multiple of PAGE_SIZE (8192 bytes) plus one
+      * long.

I don't see how sizeof(tss_struct) is a multiple of PAGE_SIZE

canary          =    8
stack           =  512
hw_tss          =  104
io bitmap       = 8200
-------------------------
                  8824

The alignment is what blows it up to 3 * PAGE_SIZE

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

index 1ea03027a4a9..e4a941be96cf 100644
 
-asmlinkage __visible notrace
+asmlinkage __visible notrace __no_sanitize_address
 struct bad_iret_stack *fixup_bad_iret(struct bad_iret_stack *s)
 {
 	/*
 	 * This is called from entry_64.S early in handling a fault
 	 * caused by a bad iret to user mode.  To handle the fault
-	 * correctly, we want move our stack frame to task_pt_regs
-	 * and we want to pretend that the exception came from the
-	 * iret target.
+	 * correctly, we want move our stack frame to where it would
+	 * be had we entered directly on the entry stack (rather than

Quoted 25 lines · Andy Lutomirski

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

index 1ea03027a4a9..e4a941be96cf 100644
 
-asmlinkage __visible notrace
+asmlinkage __visible notrace __no_sanitize_address
 struct bad_iret_stack *fixup_bad_iret(struct bad_iret_stack *s)
 {
      /*
       * This is called from entry_64.S early in handling a fault
       * caused by a bad iret to user mode.  To handle the fault
-      * correctly, we want move our stack frame to task_pt_regs
-      * and we want to pretend that the exception came from the
-      * iret target.
+      * correctly, we want move our stack frame to where it would
+      * be had we entered directly on the entry stack (rather than
+      * just below the IRET frame) and we want to pretend that the
+      * exception came from the iret target.
       */
      struct bad_iret_stack *new_stack =
-             container_of(task_pt_regs(current),
-                          struct bad_iret_stack, regs);
+             (struct bad_iret_stack *)this_cpu_read(cpu_tss.x86_tss.sp0) - 1;

Quoted 2 lines

      /* Copy the IRET target to the new stack. */
      memmove(&new_stack->regs.ip, (void *)s->regs.sp, 5*8);

So the addition of the __no_sanitize_address attribute to fixup_bad_iret() made the 64-bit allyesconfig/allmodconfig kernels fail the build:

arch/x86/entry/entry_64.S END(irq_entries_start) · line 569

Quoted 13 lines · Andy Lutomirski

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

@@ -560,6 +560,14 @@ END(irq_entries_start)
      .macro interrupt func
      cld
      ALLOC_PT_GPREGS_ON_STACK
+
+     testb   $3, CS(%rsp)
+     jz      1f
+     SWAPGS
+     call    switch_to_thread_stack
+     SWAPGS
+1:

This looks really weird to me. SWAPGS, switch stack, and SWAPGS again?

Is this so that we can use some per-cpu data in switch_to_thread_stack, and then put GS back so that the normal, non-trampoline entry code can do its normal SWAPGS voodoo?

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

+	/* Save RDI, since we need a scratch register. */
+	pushq	%rdi
+
+	/*
+	 * x86 lacks a near absolute jump, and we can't jump to the real
+	 * entry text with a relative jump, so we use a double trampoline.
+	 */
+	movq	$entry_SYSCALL_64_stage2, %rdi
+	jmp	*%rdi
+END(entry_SYSCALL_64_trampoline)
+
+	.popsection

Quoted 12 lines · Andy Lutomirski

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

+     /* Save RDI, since we need a scratch register. */
+     pushq   %rdi
+
+     /*
+      * x86 lacks a near absolute jump, and we can't jump to the real
+      * entry text with a relative jump, so we use a double trampoline.
+      */
+     movq    $entry_SYSCALL_64_stage2, %rdi
+     jmp     *%rdi
+END(entry_SYSCALL_64_trampoline)
+
+     .popsection

arch/x86/entry/entry_64.S END(native_usergs_sysret64) · line 198

Quoted 10 lines

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

+
+ENTRY(entry_SYSCALL_64_stage2)
+     /*
+      * Rather than polluting the normal SYSCALL path with stack switching
+      * nonsense, fix up our register state to match its expectations.
+      */
+     UNWIND_HINT_EMPTY
+     popq %rdi
+     jmp entry_SYSCALL_64_after_hwframe
+END(entry_SYSCALL_64_stage2)

Is there a reason why you couldn't just do the following?

pushq $entry_SYSCALL_64_after_hwframe
ret

Then you wouldn't need the 2nd trampoline and the %rdi clobber.

--
Josh

That race has been fixed and code cleaned up for a while now.

In case something goes wrong with unwind (not unlikely in case of overflow), print the offending IP where we detected the overflow.

Review void cpu_init(void) · line 1643

Quoted 12 lines · Andy Lutomirski

/* May not be marked __init: used by software suspend */ void syscall_init(void)
{

(quoted diff) +1 -1

@@ -1627,7 +1637,7 @@ void cpu_init(void)
 	 * set up and load the per-CPU TSS
 	 */
 	if (!oist->ist[0]) {
-		char *estacks = per_cpu(exception_stacks, cpu);
+		char *estacks = get_cpu_entry_area(cpu)->exception_stacks;
 
 		for (v = 0; v < N_EXCEPTION_STACKS; v++) {
 			estacks += exception_stack_sizes[v];

This generates a new build warning:

/home/mingo/tip/arch/x86/kernel/cpu/common.c: In function ‘syscall_init’:
/home/mingo/tip/arch/x86/kernel/cpu/common.c:1443:6: warning: unused variable ‘cpu’ [-Wunused-variable]
int cpu = smp_processor_id();

because 'cpu' is now unused in the !CONFIG_IA32_EMULATION part.

The naive fix is something like the patch below, untested.

Thanks,

Ingo

 arch/x86/kernel/cpu/common.c | 30 ++++++++++++++++--------------
 1 file changed, 16 insertions(+), 14 deletions(-)

arch/x86/kernel/cpu/common.c +0 -0

index c7f3a0a19dce..557bad9f4179 100644

arch/x86/kernel/cpu/common.c +16 -12

 #ifdef CONFIG_IA32_EMULATION
-	wrmsrl(MSR_CSTAR, (unsigned long)entry_SYSCALL_compat);
-	/*
-	 * This only works on Intel CPUs.
-	 * On AMD CPUs these MSRs are 32-bit, CPU truncates MSR_IA32_SYSENTER_EIP.
-	 * This does not cause SYSENTER to jump to the wrong location, because
-	 * AMD doesn't allow SYSENTER in long mode (either 32- or 64-bit).
-	 */
-	wrmsrl_safe(MSR_IA32_SYSENTER_CS, (u64)__KERNEL_CS);
-	wrmsrl_safe(MSR_IA32_SYSENTER_ESP,
-		    (unsigned long)&get_cpu_entry_area(cpu)->tss +
-		    offsetofend(struct tss_struct, SYSENTER_stack));
-	wrmsrl_safe(MSR_IA32_SYSENTER_EIP, (u64)entry_SYSENTER_compat);
+	{
+		int cpu = smp_processor_id();
+
+		wrmsrl(MSR_CSTAR, (unsigned long)entry_SYSCALL_compat);
+		/*
+		 * This only works on Intel CPUs.
+		 * On AMD CPUs these MSRs are 32-bit, CPU truncates MSR_IA32_SYSENTER_EIP.
+		 * This does not cause SYSENTER to jump to the wrong location, because
+		 * AMD doesn't allow SYSENTER in long mode (either 32- or 64-bit).
+		 */
+		wrmsrl_safe(MSR_IA32_SYSENTER_CS, (u64)__KERNEL_CS);
+		wrmsrl_safe(MSR_IA32_SYSENTER_ESP,
+			    (unsigned long)&get_cpu_entry_area(cpu)->tss +
+			    offsetofend(struct tss_struct, SYSENTER_stack));
+		wrmsrl_safe(MSR_IA32_SYSENTER_EIP, (u64)entry_SYSENTER_compat);
+	}
 #else
 	wrmsrl(MSR_CSTAR, (unsigned long)ignore_sysret);
 	wrmsrl_safe(MSR_IA32_SYSENTER_CS, (u64)GDT_ENTRY_INVALID_SEG);
 

Quoted 41 lines · Ingo Molnar

Review void cpu_init(void) · line 1643

Quoted 12 lines · Andy Lutomirski

/* May not be marked __init: used by software suspend */ void syscall_init(void)
{

(quoted diff) +1 -1

@@ -1627,7 +1637,7 @@ void cpu_init(void)
      * set up and load the per-CPU TSS
      */
     if (!oist->ist[0]) {
-            char *estacks = per_cpu(exception_stacks, cpu);
+            char *estacks = get_cpu_entry_area(cpu)->exception_stacks;
 
             for (v = 0; v < N_EXCEPTION_STACKS; v++) {
                     estacks += exception_stack_sizes[v];

This generates a new build warning:

/home/mingo/tip/arch/x86/kernel/cpu/common.c: In function ‘syscall_init’:
/home/mingo/tip/arch/x86/kernel/cpu/common.c:1443:6: warning: unused variable ‘cpu’ [-Wunused-variable]
int cpu = smp_processor_id();

because 'cpu' is now unused in the !CONFIG_IA32_EMULATION part.

The naive fix is something like the patch below, untested.

Thanks,

Ingo

 arch/x86/kernel/cpu/common.c | 30 ++++++++++++++++--------------
 1 file changed, 16 insertions(+), 14 deletions(-)

arch/x86/kernel/cpu/common.c +0 -0

index c7f3a0a19dce..557bad9f4179 100644
@@ -1440,24 +1440,26 @@ void syscall_init(void)
      extern char _entry_trampoline[];
      extern char entry_SYSCALL_64_trampoline[];
 

arch/x86/kernel/cpu/common.c void syscall_init(void) · line 1444

Quoted 4 lines

arch/x86/kernel/cpu/common.c +0 -2

-     int cpu = smp_processor_id();
-
      wrmsr(MSR_STAR, 0, (__USER32_CS << 16) | __KERNEL_CS);
      wrmsrl(MSR_LSTAR, (unsigned long)get_cpu_entry_area(smp_processor_id())->entry_trampoline + (entry_SYSCALL_64_trampoline - _entry_trampoline));

             It would be better to use 'cpu' variable here  ^^^^^^^^^^^^^^^^^

Review void cpu_init(void) · line 1643

Quoted 12 lines · Andy Lutomirski

/* May not be marked __init: used by software suspend */ void syscall_init(void)
{

(quoted diff) +1 -1

@@ -1627,7 +1637,7 @@ void cpu_init(void)
       * set up and load the per-CPU TSS
       */
      if (!oist->ist[0]) {
-             char *estacks = per_cpu(exception_stacks, cpu);
+             char *estacks = get_cpu_entry_area(cpu)->exception_stacks;
 
              for (v = 0; v < N_EXCEPTION_STACKS; v++) {
                      estacks += exception_stack_sizes[v];

This generates a new build warning:

/home/mingo/tip/arch/x86/kernel/cpu/common.c: In function ‘syscall_init’:
/home/mingo/tip/arch/x86/kernel/cpu/common.c:1443:6: warning: unused variable ‘cpu’ [-Wunused-variable]
int cpu = smp_processor_id();

because 'cpu' is now unused in the !CONFIG_IA32_EMULATION part.

The naive fix is something like the patch below, untested.

This sets up stack switching, including for SYSCALL. I think it's in decent shape.

- 32-bit kernels are failing the sigreturn_32 test. But they're also failing without the patches, so I'm not sure this is a bug in the series per se. Needs further investigation. (Off the top of my head, this could be further fallout from Thomas's IDT rework.)

This sets up stack switching, including for SYSCALL. I think it's in decent shape.

Known issues:
- KASAN is likely to be busted. This could be fixed either by teaching KASAN that cpu_entry_area contains valid stacks (I have no clue how to go about doing this) or by rigging up the IST entry code to switch RSP to point to the direct-mapped copy of the stacks before calling into non-KASAN-excluded C code.

- 32-bit kernels are failing the sigreturn_32 test. But they're also failing without the patches, so I'm not sure this is a bug in the series per se. Needs further investigation. (Off the top of my head, this could be further fallout from Thomas's IDT rework.)

- I think we're going to want a way to turn the stack switching on and off either at boot time or at runtime. It should be fairly straightforward to make it work.

- I think the ORC unwinder isn't so good at dealing with stack overflows. It bails too early (I think), resulting in lots of ? entries. This isn't a regression with this series -- it's just something that could be improved.

Quoted 15 lines · Andy Lutomirski

This sets up stack switching, including for SYSCALL. I think it's in decent shape.

Known issues:
- KASAN is likely to be busted. This could be fixed either by teaching KASAN that cpu_entry_area contains valid stacks (I have no clue how to go about doing this) or by rigging up the IST entry code to switch RSP to point to the direct-mapped copy of the stacks before calling into non-KASAN-excluded C code.

- 32-bit kernels are failing the sigreturn_32 test. But they're also failing without the patches, so I'm not sure this is a bug in the series per se. Needs further investigation. (Off the top of my head, this could be further fallout from Thomas's IDT rework.)

Quoted 8 lines

- I think we're going to want a way to turn the stack switching on and off either at boot time or at runtime. It should be fairly straightforward to make it work.

- I think the ORC unwinder isn't so good at dealing with stack overflows. It bails too early (I think), resulting in lots of ? entries. This isn't a regression with this series -- it's just something that could be improved.

Another problem I just found: IRQ tracing appears busted on 64-bit kernels - with lockdep enabled I get this boot warning:

 [    4.309026] WARNING: CPU: 10 PID: 222 at kernel/locking/lockdep.c:3924 check_flags.part.45+0x1a5/0x1b0
 ...
 [    4.309026] possible reason: unannotated irqs-off.

That's on a x86-64 defconfig-ish kernel with CONFIG_PROVE_LOCKING=y, running on an AMD system. Full splat below.

Thanks,

Ingo

[    4.272197] LVT offset 0 assigned for vector 0x400
[    4.278486] perf: AMD IBS detected (0x000000ff)
[    4.284786] kvm: Nested Virtualization enabled
[    4.289447] kvm: Nested Paging enabled
[    4.308496] DEBUG_LOCKS_WARN_ON(current->hardirqs_enabled)
[    4.308512] ------------[ cut here ]------------
[    4.309026] WARNING: CPU: 10 PID: 222 at kernel/locking/lockdep.c:3924 check_flags.part.45+0x1a5/0x1b0
[    4.309026] Modules linked in:
[    4.309026] CPU: 10 PID: 222 Comm: modprobe Not tainted 4.14.0-01345-g9490674-dirty #1
[    4.309026] Hardware name: Supermicro H8DG6/H8DGi/H8DG6/H8DGi, BIOS 2.0b       03/01/2012
[    4.309026] task: ffff880814b08000 task.stack: ffffc90007dcc000
[    4.309026] RIP: 0010:check_flags.part.45+0x1a5/0x1b0
[    4.309026] RSP: 0018:ffffc90007dcfeb0 EFLAGS: 00010082
[    4.309026] RAX: 000000000000002e RBX: ffff880814b08000 RCX: 0000000000000000
[    4.309026] RDX: 0000000000000000 RSI: 0000000000000001 RDI: ffffffff81152e76
[    4.309026] RBP: 0000000000000000 R08: 0000000000000001 R09: 0000000000000000
[    4.309026] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000
[    4.309026] R13: 0000000000000000 R14: 0000000000000001 R15: 0000000000000000
[    4.309026] FS:  0000000000000000(0000) GS:ffff880817c80000(0000) knlGS:0000000000000000
[    4.309026] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033

[    4.309026] CR2: 00007f8badc39218 CR3: 000000081584b000 CR4: 00000000000406e0
[    4.309026] Call Trace:
[    4.309026]  lock_acquire+0x11a/0x1d0
[    4.309026]  vtime_user_exit+0x3c/0xa0
[    4.309026]  ? __context_tracking_exit.part.4+0x45/0x130
[    4.309026]  __context_tracking_exit.part.4+0x45/0x130
[    4.309026]  do_syscall_64+0x13f/0x220
[    4.309026]  entry_SYSCALL64_slow_path+0x25/0x25

Quoted 61 lines · Ingo Molnar

Quoted 15 lines · Andy Lutomirski

This sets up stack switching, including for SYSCALL. I think it's in decent shape.

Known issues:
- KASAN is likely to be busted. This could be fixed either by teaching KASAN that cpu_entry_area contains valid stacks (I have no clue how to go about doing this) or by rigging up the IST entry code to switch RSP to point to the direct-mapped copy of the stacks before calling into non-KASAN-excluded C code.

- 32-bit kernels are failing the sigreturn_32 test. But they're also failing without the patches, so I'm not sure this is a bug in the series per se. Needs further investigation. (Off the top of my head, this could be further fallout from Thomas's IDT rework.)

Quoted 8 lines

- I think we're going to want a way to turn the stack switching on and off either at boot time or at runtime. It should be fairly straightforward to make it work.

- I think the ORC unwinder isn't so good at dealing with stack overflows. It bails too early (I think), resulting in lots of ? entries. This isn't a regression with this series -- it's just something that could be improved.

Another problem I just found: IRQ tracing appears busted on 64-bit kernels - with lockdep enabled I get this boot warning:

 [    4.309026] WARNING: CPU: 10 PID: 222 at kernel/locking/lockdep.c:3924 check_flags.part.45+0x1a5/0x1b0
 ...
 [    4.309026] possible reason: unannotated irqs-off.

That's on a x86-64 defconfig-ish kernel with CONFIG_PROVE_LOCKING=y, running on an AMD system. Full splat below.

Thanks,

Ingo

[    4.272197] LVT offset 0 assigned for vector 0x400
[    4.278486] perf: AMD IBS detected (0x000000ff)
[    4.284786] kvm: Nested Virtualization enabled
[    4.289447] kvm: Nested Paging enabled
[    4.308496] DEBUG_LOCKS_WARN_ON(current->hardirqs_enabled)
[    4.308512] ------------[ cut here ]------------
[    4.309026] WARNING: CPU: 10 PID: 222 at kernel/locking/lockdep.c:3924 check_flags.part.45+0x1a5/0x1b0
[    4.309026] Modules linked in:
[    4.309026] CPU: 10 PID: 222 Comm: modprobe Not tainted 4.14.0-01345-g9490674-dirty #1
[    4.309026] Hardware name: Supermicro H8DG6/H8DGi/H8DG6/H8DGi, BIOS 2.0b       03/01/2012
[    4.309026] task: ffff880814b08000 task.stack: ffffc90007dcc000
[    4.309026] RIP: 0010:check_flags.part.45+0x1a5/0x1b0
[    4.309026] RSP: 0018:ffffc90007dcfeb0 EFLAGS: 00010082
[    4.309026] RAX: 000000000000002e RBX: ffff880814b08000 RCX: 0000000000000000
[    4.309026] RDX: 0000000000000000 RSI: 0000000000000001 RDI: ffffffff81152e76
[    4.309026] RBP: 0000000000000000 R08: 0000000000000001 R09: 0000000000000000
[    4.309026] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000
[    4.309026] R13: 0000000000000000 R14: 0000000000000001 R15: 0000000000000000
[    4.309026] FS:  0000000000000000(0000) GS:ffff880817c80000(0000) knlGS:0000000000000000
[    4.309026] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033

Quoted 8 lines

[    4.309026] CR2: 00007f8badc39218 CR3: 000000081584b000 CR4: 00000000000406e0
[    4.309026] Call Trace:
[    4.309026]  lock_acquire+0x11a/0x1d0
[    4.309026]  vtime_user_exit+0x3c/0xa0
[    4.309026]  ? __context_tracking_exit.part.4+0x45/0x130
[    4.309026]  __context_tracking_exit.part.4+0x45/0x130
[    4.309026]  do_syscall_64+0x13f/0x220
[    4.309026]  entry_SYSCALL64_slow_path+0x25/0x25

I'm not reproducing this. On quick inspection, the only potential issue I see is that native_gs_load_index is missing IRQ tracing annotations, but I don't see why this series would have any particular effect on that.

Do you see it on my latest tree?

--Andy