x86/entry: Entry/exception code rework, syscall and KVM changes

All ASM code which is not part of the entry functionality can move out into the .text section. No reason to keep it in the non-instrumentable entry section.

All ASM code which is not part of the entry functionality can move out into the .text section. No reason to keep it in the non-instrumentable entry section.

Quoted 4 lines · Thomas Gleixner

All ASM code which is not part of the entry functionality can move out into the .text section. No reason to keep it in the non-instrumentable entry section.

Ick. How about just moving that code into another file altogether?

arch/x86/entry/entry_64.S SYM_FUNC_START(native_load_gs_index) · line 1066

Quoted 9 lines

+.pushsection .text, "ax"
SYM_FUNC_START(native_load_gs_index)
FRAME_BEGIN
pushfq

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

@@ -1058,6 +1063,7 @@ SYM_FUNC_START(native_load_gs_index)
        ret
 SYM_FUNC_END(native_load_gs_index)
 EXPORT_SYMBOL(native_load_gs_index)
+.popsection

native_load_gs_index is toast if it gets instrumented in the wrong way.

All ASM code which is not part of the entry functionality can move out into the .text section. No reason to keep it in the non-instrumentable entry section.

Signed-off-by Thomas Gleixner <[email protected]>

---
  arch/x86/entry/entry_32.S |   11 ++++++++++-
  1 file changed, 10 insertions(+), 1 deletion(-)

arch/x86/entry/entry_32.S +9 -0

+.pushsection .text, "ax"
  SYM_CODE_START(__switch_to_asm)
  	/*
  	 * Save callee-saved registers

@@ -776,6 +777,7 @@ SYM_CODE_START(__switch_to_asm)
 
  	jmp	__switch_to
  SYM_CODE_END(__switch_to_asm)
+.popsection
 
  /*
   * The unwinder expects the last frame on the stack to always be at the same

@@ -784,6 +786,7 @@ SYM_CODE_END(__switch_to_asm)
   * 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
 

@@ -794,6 +797,8 @@ SYM_FUNC_START(schedule_tail_wrapper)
  	FRAME_END
  	ret
  SYM_FUNC_END(schedule_tail_wrapper)
+.popsection
+
  /*
   * A newly forked process directly context switches into this address.
   *

@@ -801,6 +806,7 @@ SYM_FUNC_END(schedule_tail_wrapper)
   * ebx: kernel thread func (NULL for user thread)
   * edi: kernel thread arg
   */
+.pushsection .text, "ax"
  SYM_CODE_START(ret_from_fork)
  	call	schedule_tail_wrapper
 

@@ -825,6 +831,7 @@ SYM_CODE_START(ret_from_fork)
  	movl	$0, PT_EAX(%esp)
  	jmp	2b
  SYM_CODE_END(ret_from_fork)
+.popsection
 
  /*
   * Return to user mode is not as complex as all this looks,

@@ -1693,6 +1700,7 @@ SYM_CODE_START(general_protection)
  	jmp	common_exception
  SYM_CODE_END(general_protection)
 
+.pushsection .text, "ax"
  SYM_CODE_START(rewind_stack_do_exit)
  	/* Prevent any naive code from trying to unwind to our caller. */
  	xorl	%ebp, %ebp

@@ -1703,3 +1711,4 @@ SYM_CODE_START(rewind_stack_do_exit)
  	call	do_exit
  1:	jmp 1b
  SYM_CODE_END(rewind_stack_do_exit)
+.popsection

Quoted 5 lines · Thomas Gleixner

/*
* %eax: prev task
* %edx: next task
- */
+*/

Misaligned comment end, this line shouldn't change.

Both the callers in the low level ASM code and __context_tracking_exit() which is invoked from enter_from_user_mode() via user_exit_irqoff() are marked NOKPROBE. Allowing enter_from_user_mode() to be probed is inconsistent at best.

Aside of that while function tracing per se is safe the function trace entry/exit points can be used via BPF as well which is not safe to use before context tracking has reached CONTEXT_KERNEL and adjusted RCU.

Mark it noinstr which moves it into the instrumentation protected text section and includes notrace.

Note, this needs further fixups in context tracking to ensure that the full call chain is protected. Will be addressed in follow up changes.

Signed-off-by Thomas Gleixner <[email protected]>

---
 arch/x86/entry/common.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/x86/entry/common.c +1 -1

@@ -41,7 +41,7 @@
 
 #ifdef CONFIG_CONTEXT_TRACKING
 /* Called on entry from user mode with IRQs off. */
-__visible inline void enter_from_user_mode(void)
+__visible inline noinstr void enter_from_user_mode(void)
 {
 	CT_WARN_ON(ct_state() != CONTEXT_USER);
 	user_exit_irqoff();

Mark the various syscall entries with noinstr to protect them against instrumentation and add the noinstr_begin()/end() annotations to mark the parts of the functions which are safe to call out into instrumentable code.

Signed-off-by Thomas Gleixner <[email protected]>

---
  arch/x86/entry/common.c |  135 ++++++++++++++++++++++++++++++++----------------
  1 file changed, 90 insertions(+), 45 deletions(-)

arch/x86/entry/common.c +41 -15

@@ -41,15 +41,26 @@
 
  #ifdef CONFIG_CONTEXT_TRACKING
  /* Called on entry from user mode with IRQs off. */
-__visible inline noinstr void enter_from_user_mode(void)
+__visible noinstr void enter_from_user_mode(void)
  {
-	CT_WARN_ON(ct_state() != CONTEXT_USER);
+	enum ctx_state state = ct_state();
+
  	user_exit_irqoff();
+
+	instr_begin();
+	CT_WARN_ON(state != CONTEXT_USER);
+	instr_end();
  }
  #else
  static inline void enter_from_user_mode(void) {}
  #endif
 
+static noinstr void exit_to_user_mode(void)
+{
+	user_enter_irqoff();
+	mds_user_clear_cpu_buffers();
+}
+
  static void do_audit_syscall_entry(struct pt_regs *regs, u32 arch)
  {
  #ifdef CONFIG_X86_64

@@ -179,8 +190,7 @@ static void exit_to_usermode_loop(struct
  	}
  }
 
-/* Called with IRQs disabled. */
-__visible inline void prepare_exit_to_usermode(struct pt_regs *regs)
+static void __prepare_exit_to_usermode(struct pt_regs *regs)
  {
  	struct thread_info *ti = current_thread_info();
  	u32 cached_flags;

@@ -219,10 +229,14 @@ static void exit_to_usermode_loop(struct
  	 */
  	ti->status &= ~(TS_COMPAT|TS_I386_REGS_POKED);
  #endif
+}
 
-	user_enter_irqoff();
-
-	mds_user_clear_cpu_buffers();
+__visible noinstr void prepare_exit_to_usermode(struct pt_regs *regs)
+{
+	instr_begin();
+	__prepare_exit_to_usermode(regs);
+	instr_end();
+	exit_to_user_mode();
  }
 
  #define SYSCALL_EXIT_WORK_FLAGS				\

@@ -251,11 +265,7 @@ static void syscall_slow_exit_work(struc
  		tracehook_report_syscall_exit(regs, step);
  }
 
-/*
- * Called with IRQs on and fully valid regs.  Returns with IRQs off in a
- * state such that we can immediately switch to user mode.
- */
-__visible inline void syscall_return_slowpath(struct pt_regs *regs)
+static void __syscall_return_slowpath(struct pt_regs *regs)
  {
  	struct thread_info *ti = current_thread_info();
  	u32 cached_flags = READ_ONCE(ti->flags);

@@ -276,15 +286,29 @@ static void syscall_slow_exit_work(struc
  		syscall_slow_exit_work(regs, cached_flags);
 
  	local_irq_disable();
-	prepare_exit_to_usermode(regs);
+	__prepare_exit_to_usermode(regs);
+}
+
+/*
+ * Called with IRQs on and fully valid regs.  Returns with IRQs off in a
+ * state such that we can immediately switch to user mode.
+ */
+__visible noinstr void syscall_return_slowpath(struct pt_regs *regs)
+{
+	instr_begin();
+	__syscall_return_slowpath(regs);
+	instr_end();
+	exit_to_user_mode();
  }
 
  #ifdef CONFIG_X86_64
-__visible void do_syscall_64(unsigned long nr, struct pt_regs *regs)
+__visible noinstr void do_syscall_64(unsigned long nr, struct pt_regs *regs)
  {
  	struct thread_info *ti;
 
  	enter_from_user_mode();
+	instr_begin();
+
  	local_irq_enable();
  	ti = current_thread_info();
  	if (READ_ONCE(ti->flags) & _TIF_WORK_SYSCALL_ENTRY)

@@ -301,8 +325,10 @@ static void syscall_slow_exit_work(struc
  		regs->ax = x32_sys_call_table[nr](regs);
  #endif
  	}
+	__syscall_return_slowpath(regs);
 
-	syscall_return_slowpath(regs);
+	instr_end();
+	exit_to_user_mode();
  }
  #endif
 

arch/x86/entry/common.c +48 -29

   * do_fast_syscall_32, so forcibly inline it to improve performance.
   */
-static __always_inline void do_syscall_32_irqs_on(struct pt_regs *regs)
+static void do_syscall_32_irqs_on(struct pt_regs *regs)
  {
  	struct thread_info *ti = current_thread_info();
  	unsigned int nr = (unsigned int)regs->orig_ax;

@@ -337,27 +363,62 @@ static __always_inline void do_syscall_3
  		regs->ax = ia32_sys_call_table[nr](regs);
  	}
 
-	syscall_return_slowpath(regs);
+	__syscall_return_slowpath(regs);
  }
 
  /* Handles int $0x80 */
-__visible void do_int80_syscall_32(struct pt_regs *regs)
+__visible noinstr void do_int80_syscall_32(struct pt_regs *regs)
  {
  	enter_from_user_mode();
+	instr_begin();
+
  	local_irq_enable();
  	do_syscall_32_irqs_on(regs);
+
+	instr_end();
+	exit_to_user_mode();
+}
+
+static bool __do_fast_syscall_32(struct pt_regs *regs)
+{
+	int res;
+
+	/* Fetch EBP from where the vDSO stashed it. */
+	if (IS_ENABLED(CONFIG_X86_64)) {
+		/*
+		 * Micro-optimization: the pointer we're following is
+		 * explicitly 32 bits, so it can't be out of range.
+		 */
+		res = __get_user(*(u32 *)&regs->bp,
+			 (u32 __user __force *)(unsigned long)(u32)regs->sp);
+	} else {
+		res = get_user(*(u32 *)&regs->bp,
+		       (u32 __user __force *)(unsigned long)(u32)regs->sp);
+	}
+
+	if (res) {
+		/* User code screwed up. */
+		regs->ax = -EFAULT;
+		local_irq_disable();
+		__prepare_exit_to_usermode(regs);
+		return false;
+	}
+
+	/* Now this is just like a normal syscall. */
+	do_syscall_32_irqs_on(regs);
+	return true;
  }
 
  /* Returns 0 to return using IRET or 1 to return using SYSEXIT/SYSRETL. */
-__visible long do_fast_syscall_32(struct pt_regs *regs)
+__visible noinstr long do_fast_syscall_32(struct pt_regs *regs)
  {
  	/*
  	 * Called using the internal vDSO SYSENTER/SYSCALL32 calling
  	 * convention.  Adjust regs so it looks like we entered using int80.
  	 */
-
  	unsigned long landing_pad = (unsigned long)current->mm->context.vdso +
-		vdso_image_32.sym_int80_landing_pad;
+					vdso_image_32.sym_int80_landing_pad;
+	bool success;
 
  	/*
  	 * SYSENTER loses EIP, and even SYSCALL32 needs us to skip forward

@@ -367,33 +428,17 @@ static __always_inline void do_syscall_3
  	regs->ip = landing_pad;
 
  	enter_from_user_mode();
+	instr_begin();
 
  	local_irq_enable();
+	success = __do_fast_syscall_32(regs);
 
-	/* Fetch EBP from where the vDSO stashed it. */
-	if (
-#ifdef CONFIG_X86_64
-		/*
-		 * Micro-optimization: the pointer we're following is explicitly
-		 * 32 bits, so it can't be out of range.
-		 */
-		__get_user(*(u32 *)&regs->bp,
-			    (u32 __user __force *)(unsigned long)(u32)regs->sp)
-#else
-		get_user(*(u32 *)&regs->bp,
-			 (u32 __user __force *)(unsigned long)(u32)regs->sp)
-#endif
-		) {
-
-		/* User code screwed up. */
-		local_irq_disable();
-		regs->ax = -EFAULT;
-		prepare_exit_to_usermode(regs);
-		return 0;	/* Keep it simple: use IRET. */
-	}
+	instr_end();
+	exit_to_user_mode();
 
-	/* Now this is just like a normal syscall. */
-	do_syscall_32_irqs_on(regs);
+	/* If it failed, keep it simple: use IRET. */
+	if (!success)
+		return 0;
 
  #ifdef CONFIG_X86_64
  	/*

arch/x86/entry/common.c static void syscall_slow_exit_work(struc · line 339

Quoted 6 lines

arch/x86/entry/common.c +1 -1

@@ -310,10 +336,10 @@ static void syscall_slow_exit_work(struc
  /*
   * Does a 32-bit syscall.  Called with IRQs on in CONTEXT_KERNEL.  Does
   * all entry and exit work and returns with IRQs off.  This function is
- * extremely hot in workloads that use it, and it's usually called from
+ * ex2tremely hot in workloads that use it, and it's usually called from

typo: "ex2tremely"

Now that the C entry points are safe, move the irq flags tracing code into the entry helper:

- Invoke lockdep before calling into context tracking

- Use the safe trace_hardirqs_on_prepare() trace function after context tracking established state and RCU is watching.

Signed-off-by Thomas Gleixner <[email protected]>

---
  arch/x86/entry/common.c          |   21 +++++++++++++++++++--
  arch/x86/entry/entry_32.S        |   12 ------------
  arch/x86/entry/entry_64.S        |    2 --
  arch/x86/entry/entry_64_compat.S |   18 ------------------
  4 files changed, 19 insertions(+), 34 deletions(-)

arch/x86/entry/common.c +19 -2

@@ -40,19 +40,36 @@
  #include <trace/events/syscalls.h>
 
  #ifdef CONFIG_CONTEXT_TRACKING
-/* Called on entry from user mode with IRQs off. */
+/**
+ * enter_from_user_mode - Establish state when coming from user mode
+ *
+ * Syscall entry disables interrupts, but user mode is traced as interrupts
+ * enabled. Also with NO_HZ_FULL RCU might be idle.
+ *
+ * 1) Tell lockdep that interrupts are disabled
+ * 2) Invoke context tracking if enabled to reactivate RCU
+ * 3) Trace interrupts off state
+ */
  __visible noinstr void enter_from_user_mode(void)
  {
  	enum ctx_state state = ct_state();
 
+	lockdep_hardirqs_off(CALLER_ADDR0);
  	user_exit_irqoff();
 
  	instr_begin();
  	CT_WARN_ON(state != CONTEXT_USER);
+	trace_hardirqs_off_prepare();
  	instr_end();
  }
  #else
-static inline void enter_from_user_mode(void) {}
+static __always_inline void enter_from_user_mode(void)
+{
+	lockdep_hardirqs_off(CALLER_ADDR0);
+	instr_begin();
+	trace_hardirqs_off_prepare();
+	instr_end();
+}
  #endif
 
  static noinstr void exit_to_user_mode(void)

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

@@ -967,12 +967,6 @@ SYM_FUNC_START(entry_SYSENTER_32)
  	jnz	.Lsysenter_fix_flags
  .Lsysenter_flags_fixed:
 
-	/*
-	 * User mode is traced as though IRQs are on, and SYSENTER
-	 * turned them off.
-	 */
-	TRACE_IRQS_OFF
-
  	movl	%esp, %eax
  	call	do_fast_syscall_32
  	/* XEN PV guests always use IRET path */

@@ -1082,12 +1076,6 @@ SYM_FUNC_START(entry_INT80_32)
 
  	SAVE_ALL pt_regs_ax=$-ENOSYS switch_stacks=1	/* save rest */
 
-	/*
-	 * User mode is traced as though IRQs are on, and the interrupt gate
-	 * turned them off.
-	 */
-	TRACE_IRQS_OFF
-
  	movl	%esp, %eax
  	call	do_int80_syscall_32
  .Lsyscall_32_done:

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

@@ -167,8 +167,6 @@ SYM_INNER_LABEL(entry_SYSCALL_64_after_h
 
  	PUSH_AND_CLEAR_REGS rax=$-ENOSYS
 
-	TRACE_IRQS_OFF
-
  	/* IRQs are off. */
  	movq	%rax, %rdi
  	movq	%rsp, %rsi

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

@@ -129,12 +129,6 @@ SYM_FUNC_START(entry_SYSENTER_compat)
  	jnz	.Lsysenter_fix_flags
  .Lsysenter_flags_fixed:
 
-	/*
-	 * User mode is traced as though IRQs are on, and SYSENTER
-	 * turned them off.
-	 */
-	TRACE_IRQS_OFF
-
  	movq	%rsp, %rdi
  	call	do_fast_syscall_32
  	/* XEN PV guests always use IRET path */

@@ -247,12 +241,6 @@ SYM_INNER_LABEL(entry_SYSCALL_compat_aft
  	pushq   $0			/* pt_regs->r15 = 0 */
  	xorl	%r15d, %r15d		/* nospec   r15 */
 
-	/*
-	 * User mode is traced as though IRQs are on, and SYSENTER
-	 * turned them off.
-	 */
-	TRACE_IRQS_OFF
-
  	movq	%rsp, %rdi
  	call	do_fast_syscall_32
  	/* XEN PV guests always use IRET path */

@@ -403,12 +391,6 @@ SYM_CODE_START(entry_INT80_compat)
  	xorl	%r15d, %r15d		/* nospec   r15 */
  	cld

arch/x86/entry/entry_64_compat.S SYM_CODE_START(entry_INT80_compat) · line 396

Quoted 10 lines · Thomas Gleixner

arch/x86/entry/entry_64_compat.S +0 -6

-	/*
-	 * User mode is traced as though IRQs are on, and the interrupt
-	 * gate turned them off.
-	 */
-	TRACE_IRQS_OFF
-
  	movq	%rsp, %rdi
  	call	do_int80_syscall_32
  .Lsyscall_32_done:

enter_from_user_mode() is also called with the CALL_enter_from_user_mode macro, which is used in interrupt_entry() and identry. Don't you need to also remove the TRACE_IRQS_OFF there now?

Quoted 15 lines · Alexandre Chartre

arch/x86/entry/entry_64_compat.S SYM_CODE_START(entry_INT80_compat) · line 396

Quoted 10 lines · Thomas Gleixner

arch/x86/entry/entry_64_compat.S +0 -6

-	/*
-	 * User mode is traced as though IRQs are on, and the interrupt
-	 * gate turned them off.
-	 */
-	TRACE_IRQS_OFF
-
  	movq	%rsp, %rdi
  	call	do_int80_syscall_32
  .Lsyscall_32_done:

enter_from_user_mode() is also called with the CALL_enter_from_user_mode macro, which is used in interrupt_entry() and identry. Don't you need to also remove the TRACE_IRQS_OFF there now?

Hrm. right. OTOH, it's just redundant and should be no harm, but let me have a look at that again.

Quoted 17 lines · Thomas Gleixner

Quoted 15 lines · Alexandre Chartre

arch/x86/entry/entry_64_compat.S SYM_CODE_START(entry_INT80_compat) · line 396

Quoted 10 lines · Thomas Gleixner

arch/x86/entry/entry_64_compat.S +0 -6

-	/*
-	 * User mode is traced as though IRQs are on, and the interrupt
-	 * gate turned them off.
-	 */
-	TRACE_IRQS_OFF
-
  	movq	%rsp, %rdi
  	call	do_int80_syscall_32
  .Lsyscall_32_done:

enter_from_user_mode() is also called with the CALL_enter_from_user_mode macro, which is used in interrupt_entry() and identry. Don't you need to also remove the TRACE_IRQS_OFF there now?

Quoted 2 lines

Hrm. right. OTOH, it's just redundant and should be no harm, but let me have a look at that again.

Grr, no. It'll trigger the warnon when context tracking is enabled. /me scratches head and goes to fix.

This is another step towards more C-code and less convoluted ASM.

Similar to the entry path, invoke the tracer before context tracking which might turn off RCU and invoke lockdep as the last step before going back to user space. Annotate the code sections in exit_to_user_mode() accordingly so objtool won't complain about the tracer invocation.

Quoted 7 lines · Thomas Gleixner

This is another step towards more C-code and less convoluted ASM.

Similar to the entry path, invoke the tracer before context tracking which might turn off RCU and invoke lockdep as the last step before going back to user space. Annotate the code sections in exit_to_user_mode() accordingly so objtool won't complain about the tracer invocation.

Acked-by Andy Lutomirski <[email protected]>

Note to self: the nmi code needs to be reworked to go through prepare_exit_to_usermode(), too. I'll do this once this whole pile lands.

context tracking lacks a few protection mechanisms against instrumentation:

- While the core functions are marked NOKPROBE they lack protection against function tracing which is required as the function entry/exit points can be utilized by BPF.

- static functions invoked from the protected functions need to be marked as well as they can be instrumented otherwise.

- using plain inline allows the compiler to emit traceable and probable functions.

Fix this by marking the functions noinstr and converting the plain inlines to __always_inline.

The NOKPROBE_SYMBOL() annotations are removed as the .noinstr.text section is already excluded from being probed.

Cures the following objtool warnings:

vmlinux.o: warning: objtool: enter_from_user_mode()+0x34: call to __context_tracking_exit() leaves .noinstr.text section vmlinux.o: warning: objtool: prepare_exit_to_usermode()+0x29: call to __context_tracking_enter() leaves .noinstr.text section vmlinux.o: warning: objtool: syscall_return_slowpath()+0x29: call to __context_tracking_enter() leaves .noinstr.text section vmlinux.o: warning: objtool: do_syscall_64()+0x7f: call to __context_tracking_enter() leaves .noinstr.text section vmlinux.o: warning: objtool: do_int80_syscall_32()+0x3d: call to __context_tracking_enter() leaves .noinstr.text section vmlinux.o: warning: objtool: do_fast_syscall_32()+0x9c: call to __context_tracking_enter() leaves .noinstr.text section

and generates new ones...

Signed-off-by Thomas Gleixner <[email protected]>

---
 include/linux/context_tracking.h       |    6 +++---
 include/linux/context_tracking_state.h |    6 +++---
 kernel/context_tracking.c              |   14 ++++++++------
 3 files changed, 14 insertions(+), 12 deletions(-)

include/linux/context_tracking.h +3 -3

@@ -33,13 +33,13 @@ static inline void user_exit(void)
 }
 
 /* Called with interrupts disabled.  */
-static inline void user_enter_irqoff(void)
+static __always_inline void user_enter_irqoff(void)
 {
 	if (context_tracking_enabled())
 		__context_tracking_enter(CONTEXT_USER);
 
 }
-static inline void user_exit_irqoff(void)
+static __always_inline void user_exit_irqoff(void)
 {
 	if (context_tracking_enabled())
 		__context_tracking_exit(CONTEXT_USER);

@@ -75,7 +75,7 @@ static inline void exception_exit(enum c
  * is enabled.  If context tracking is disabled, returns
  * CONTEXT_DISABLED.  This should be used primarily for debugging.
  */
-static inline enum ctx_state ct_state(void)
+static __always_inline enum ctx_state ct_state(void)
 {
 	return context_tracking_enabled() ?
 		this_cpu_read(context_tracking.state) : CONTEXT_DISABLED;

include/linux/context_tracking_state.h +3 -3

@@ -26,12 +26,12 @@ struct context_tracking {
 extern struct static_key_false context_tracking_key;
 DECLARE_PER_CPU(struct context_tracking, context_tracking);
 
-static inline bool context_tracking_enabled(void)
+static __always_inline bool context_tracking_enabled(void)
 {
 	return static_branch_unlikely(&context_tracking_key);
 }
 
-static inline bool context_tracking_enabled_cpu(int cpu)
+static __always_inline bool context_tracking_enabled_cpu(int cpu)
 {
 	return context_tracking_enabled() && per_cpu(context_tracking.active, cpu);
 }

@@ -41,7 +41,7 @@ static inline bool context_tracking_enab
 	return context_tracking_enabled() && __this_cpu_read(context_tracking.active);
 }
 
-static inline bool context_tracking_in_user(void)
+static __always_inline bool context_tracking_in_user(void)
 {
 	return __this_cpu_read(context_tracking.state) == CONTEXT_USER;
 }

kernel/context_tracking.c +8 -6

@@ -31,7 +31,7 @@ EXPORT_SYMBOL_GPL(context_tracking_key);
 DEFINE_PER_CPU(struct context_tracking, context_tracking);
 EXPORT_SYMBOL_GPL(context_tracking);
 
-static bool context_tracking_recursion_enter(void)
+static noinstr bool context_tracking_recursion_enter(void)
 {
 	int recursion;
 

@@ -45,7 +45,7 @@ static bool context_tracking_recursion_e
 	return false;
 }
 
-static void context_tracking_recursion_exit(void)
+static __always_inline void context_tracking_recursion_exit(void)
 {
 	__this_cpu_dec(context_tracking.recursion);
 }

@@ -59,7 +59,7 @@ static void context_tracking_recursion_e
  * instructions to execute won't use any RCU read side critical section
  * because this function sets RCU in extended quiescent state.
  */
-void __context_tracking_enter(enum ctx_state state)
+void noinstr __context_tracking_enter(enum ctx_state state)
 {
 	/* Kernel threads aren't supposed to go to userspace */
 	WARN_ON_ONCE(!current->mm);

@@ -77,8 +77,10 @@ void __context_tracking_enter(enum ctx_s
 			 * on the tick.
 			 */
 			if (state == CONTEXT_USER) {
+				instr_begin();
 				trace_user_enter(0);
 				vtime_user_enter(current);
+				instr_end();
 			}
 			rcu_user_enter();
 		}

@@ -99,7 +101,6 @@ void __context_tracking_enter(enum ctx_s
 	}
 	context_tracking_recursion_exit();
 }
-NOKPROBE_SYMBOL(__context_tracking_enter);
 EXPORT_SYMBOL_GPL(__context_tracking_enter);
 
 void context_tracking_enter(enum ctx_state state)

@@ -142,7 +143,7 @@ NOKPROBE_SYMBOL(context_tracking_user_en
  * This call supports re-entrancy. This way it can be called from any exception
  * handler without needing to know if we came from userspace or not.
  */
-void __context_tracking_exit(enum ctx_state state)
+void noinstr __context_tracking_exit(enum ctx_state state)
 {
 	if (!context_tracking_recursion_enter())
 		return;

@@ -155,15 +156,16 @@ void __context_tracking_exit(enum ctx_st
 			 */
 			rcu_user_exit();
 			if (state == CONTEXT_USER) {
+				instr_begin();
 				vtime_user_exit(current);
 				trace_user_exit(0);
+				instr_end();
 			}
 		}
 		__this_cpu_write(context_tracking.state, CONTEXT_KERNEL);
 	}
 	context_tracking_recursion_exit();
 }
-NOKPROBE_SYMBOL(__context_tracking_exit);
 EXPORT_SYMBOL_GPL(__context_tracking_exit);
 
 void context_tracking_exit(enum ctx_state state)

The preempt_enable_notrace() ASM thunk is called from tracing, entry code RCU and other places which are already in or going to be in the noinstr section which protects sensitve code from being instrumented.

Calls out of these sections happen with interrupts disabled, which is handled in C code, but the push regs, call, pop regs sequence can be completely avoided in this case.

This is also a preparatory step for annotating the call from the thunk to preempt_enable_notrace() safe from a noinstr section.

Signed-off-by Thomas Gleixner <[email protected]>

---
  arch/x86/entry/thunk_64.S       |   27 +++++++++++++++++++++++----
  arch/x86/include/asm/irqflags.h |    3 +--
  arch/x86/include/asm/paravirt.h |    3 +--
  3 files changed, 25 insertions(+), 8 deletions(-)

arch/x86/entry/thunk_64.S +23 -4

@@ -9,10 +9,28 @@
  #include "calling.h"
  #include <asm/asm.h>
  #include <asm/export.h>
+#include <asm/irqflags.h>
+
+.code64
 
  	/* rdi:	arg1 ... normal C conventions. rax is saved/restored. */
-	.macro THUNK name, func, put_ret_addr_in_rdi=0
+	.macro THUNK name, func, put_ret_addr_in_rdi=0, check_if=0
  SYM_FUNC_START_NOALIGN(\name)
+
+	.if \check_if
+	/*
+	 * Check for interrupts disabled right here. No point in
+	 * going all the way down
+	 */
+	pushq	%rax
+	SAVE_FLAGS(CLBR_RAX)
+	testl	$X86_EFLAGS_IF, %eax
+	popq	%rax
+	jnz	1f
+	ret
+1:
+	.endif
+
  	pushq %rbp
  	movq %rsp, %rbp
 

@@ -38,14 +56,15 @@ SYM_FUNC_END(\name)
  	.endm
 
  #ifdef CONFIG_TRACE_IRQFLAGS
-	THUNK trace_hardirqs_on_thunk,trace_hardirqs_on_caller,1
-	THUNK trace_hardirqs_off_thunk,trace_hardirqs_off_caller,1
+	THUNK trace_hardirqs_on_thunk,trace_hardirqs_on_caller, put_ret_addr_in_rdi=1
+	THUNK trace_hardirqs_off_thunk,trace_hardirqs_off_caller, put_ret_addr_in_rdi=1
  #endif
 
  #ifdef CONFIG_PREEMPTION
  	THUNK preempt_schedule_thunk, preempt_schedule
-	THUNK preempt_schedule_notrace_thunk, preempt_schedule_notrace
  	EXPORT_SYMBOL(preempt_schedule_thunk)
+
+	THUNK preempt_schedule_notrace_thunk, preempt_schedule_notrace, check_if=1
  	EXPORT_SYMBOL(preempt_schedule_notrace_thunk)
  #endif
 

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

@@ -127,9 +127,8 @@ static inline notrace unsigned long arch
  #define DISABLE_INTERRUPTS(x)	cli
 
  #ifdef CONFIG_X86_64
-#ifdef CONFIG_DEBUG_ENTRY
+
  #define SAVE_FLAGS(x)		pushfq; popq %rax
-#endif
 
  #define SWAPGS	swapgs
  /*

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

@@ -907,14 +907,13 @@ extern void default_banner(void);
  		  ANNOTATE_RETPOLINE_SAFE;				\
  		  jmp PARA_INDIRECT(pv_ops+PV_CPU_usergs_sysret64);)
 
-#ifdef CONFIG_DEBUG_ENTRY
  #define SAVE_FLAGS(clobbers)                                        \
  	PARA_SITE(PARA_PATCH(PV_IRQ_save_fl),			    \
  		  PV_SAVE_REGS(clobbers | CLBR_CALLEE_SAVE);        \
  		  ANNOTATE_RETPOLINE_SAFE;			    \
  		  call PARA_INDIRECT(pv_ops+PV_IRQ_save_fl);	    \
  		  PV_RESTORE_REGS(clobbers | CLBR_CALLEE_SAVE);)
-#endif
+
  #endif /* CONFIG_PARAVIRT_XXL */
  #endif	/* CONFIG_X86_64 */
 

The preempt_enable_notrace() ASM thunk is called from tracing, entry code RCU and other places which are already in or going to be in the noinstr section which protects sensitve code from being instrumented.

+ THUNK preempt_schedule_notrace_thunk, preempt_schedule_notrace, check_if=1

Quoted 4 lines · Thomas Gleixner

The preempt_enable_notrace() ASM thunk is called from tracing, entry code RCU and other places which are already in or going to be in the noinstr section which protects sensitve code from being instrumented.

This text and $SUBJECT agree that you're talking about preempt_enable_notrace(), but:

Quoted 1 line

+ THUNK preempt_schedule_notrace_thunk, preempt_schedule_notrace, check_if=1

You actually seem to be changing preempt_schedule_notrace().

The actual code in question has this comment:

/**
* preempt_schedule_notrace - preempt_schedule called by tracing *
* The tracing infrastructure uses preempt_enable_notrace to prevent
* recursion and tracing preempt enabling caused by the tracing
* infrastructure itself. But as tracing can happen in areas coming
* from userspace or just about to enter userspace, a preempt enable
* can occur before user_exit() is called. This will cause the scheduler
* to be called when the system is still in usermode.
*
* To prevent this, the preempt_enable_notrace will use this function
* instead of preempt_schedule() to exit user context if needed before
* calling the scheduler.
*/

Which is no longer really applicable to x86 -- in the state that this comment nonsensically refers to as "userspace", x86 *always* has IRQs off, which means that preempt_enable() will not schedule.

So I'm guessing that the issue you're solving is that we have redundant preempt disable/enable pairs somewhere in the bowels of tracing code that is called with IRQs off, and objtool is now complaining. Could the actual code in question be fixed to assert that IRQs are off instead of disabling preemption? If not, can you fix the $SUBJECT and changelog and perhaps add a comment to the code as to *why* you're checking IF? Otherwise some intrepid programmer is going to notice it down the road, wonder if it's optimizing anything useful at all, and get rid of it.

Quoted 16 lines · Andy Lutomirski

On Tue, May 5, 2020 at 7:14 AM Thomas Gleixner <[email protected]> wrote:
/**
* preempt_schedule_notrace - preempt_schedule called by tracing *
* The tracing infrastructure uses preempt_enable_notrace to prevent
* recursion and tracing preempt enabling caused by the tracing
* infrastructure itself. But as tracing can happen in areas coming
* from userspace or just about to enter userspace, a preempt enable
* can occur before user_exit() is called. This will cause the scheduler
* to be called when the system is still in usermode.
*
* To prevent this, the preempt_enable_notrace will use this function
* instead of preempt_schedule() to exit user context if needed before
* calling the scheduler.
*/

Quoted 3 lines

Which is no longer really applicable to x86 -- in the state that this comment nonsensically refers to as "userspace", x86 *always* has IRQs off, which means that preempt_enable() will not schedule.

Quoted 9 lines

So I'm guessing that the issue you're solving is that we have redundant preempt disable/enable pairs somewhere in the bowels of tracing code that is called with IRQs off, and objtool is now complaining. Could the actual code in question be fixed to assert that IRQs are off instead of disabling preemption? If not, can you fix the $SUBJECT and changelog and perhaps add a comment to the code as to *why* you're checking IF? Otherwise some intrepid programmer is going to notice it down the road, wonder if it's optimizing anything useful at all, and get rid of it.

Let me stare into that again.

Quoted 1 line · Thomas Gleixner

Let me stare into that again.

There are a few preempt_disable/enable() pairs in some of the helper functions which are called in various places. That means we would have to chase all of them and provide 'naked' helpers for these particular call chains. I'll fix the changelog and add a comment to make clear what this is about.

Quoted 4 lines · Thomas Gleixner

The preempt_enable_notrace() ASM thunk is called from tracing, entry code RCU and other places which are already in or going to be in the noinstr section which protects sensitve code from being instrumented.

This text and $SUBJECT agree that you're talking about preempt_enable_notrace(), but:

Quoted 1 line

+ THUNK preempt_schedule_notrace_thunk, preempt_schedule_notrace, check_if=1

You actually seem to be changing preempt_schedule_notrace().

The actual code in question has this comment:

/**
* preempt_schedule_notrace - preempt_schedule called by tracing *
* The tracing infrastructure uses preempt_enable_notrace to prevent
* recursion and tracing preempt enabling caused by the tracing
* infrastructure itself. But as tracing can happen in areas coming
* from userspace or just about to enter userspace, a preempt enable
* can occur before user_exit() is called. This will cause the scheduler
* to be called when the system is still in usermode.
*
* To prevent this, the preempt_enable_notrace will use this function
* instead of preempt_schedule() to exit user context if needed before
* calling the scheduler.
*/

Which is no longer really applicable to x86 -- in the state that this comment nonsensically refers to as "userspace", x86 *always* has IRQs off, which means that preempt_enable() will not schedule.

So I'm guessing that the issue you're solving is that we have redundant preempt disable/enable pairs somewhere in the bowels of tracing code that is called with IRQs off, and objtool is now complaining. Could the actual code in question be fixed to assert that IRQs are off instead of disabling preemption? If not, can you fix the $SUBJECT and changelog and perhaps add a comment to the code as to *why* you're checking IF? Otherwise some intrepid programmer is going to notice it down the road, wonder if it's optimizing anything useful at all, and get rid of it.

The preempt_enable_notrace() ASM thunk is called from tracing, entry code RCU and other places which are already in or going to be in the noinstr section which protects sensitve code from being instrumented.

Calls out of these sections happen with interrupts disabled, which is handled in C code, but the push regs, call, pop regs sequence can be completely avoided in this case.

This is also a preparatory step for annotating the call from the thunk to preempt_enable_notrace() safe from a noinstr section.

Quoted 11 lines · On Tue, 05 May 2020 15:41:22 +0200 Thomas Gleixner <[email protected]> wrote:

The preempt_enable_notrace() ASM thunk is called from tracing, entry code RCU and other places which are already in or going to be in the noinstr section which protects sensitve code from being instrumented.

Calls out of these sections happen with interrupts disabled, which is handled in C code, but the push regs, call, pop regs sequence can be completely avoided in this case.

This is also a preparatory step for annotating the call from the thunk to preempt_enable_notrace() safe from a noinstr section.

BTW, after applying this patch, I get the following error:

/work/git/linux-test.git/arch/x86/entry/thunk_64.S: Assembler messages:
/work/git/linux-test.git/arch/x86/entry/thunk_64.S:67: Error: invalid operands (*UND* and *UND* sections) for +' /work/git/linux-test.git/arch/x86/entry/thunk_64.S:67: Error: invalid operands (*UND* and *ABS* sections) for /' make[3]: *** [/work/git/linux-test.git/scripts/Makefile.build:349: arch/x86/entry/thunk_64.o] Error 1 make[3]: *** Waiting for unfinished jobs....

Currently entry_64_compat is exempt from objtool, but with vmlinux mode there is no hiding it.

Make the following changes to make it pass:

- change entry_SYSENTER_compat to STT_NOTYPE; it's not a function and doesn't have function type stack setup.

- mark all STT_NOTYPE symbols with UNWIND_HINT_EMPTY; so we do validate them and don't treat them as unreachable.

- don't abuse RSP as a temp register, this confuses objtool mightily as it (rightfully) thinks we're doing unspeakable things to the stack.

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

Quoted 9 lines · Thomas Gleixner

Currently entry_64_compat is exempt from objtool, but with vmlinux mode there is no hiding it.

Make the following changes to make it pass:

- change entry_SYSENTER_compat to STT_NOTYPE; it's not a function and doesn't have function type stack setup.

Quoted 7 lines

- mark all STT_NOTYPE symbols with UNWIND_HINT_EMPTY; so we do validate them and don't treat them as unreachable.

- don't abuse RSP as a temp register, this confuses objtool mightily as it (rightfully) thinks we're doing unspeakable things to the stack.

Acked-by Andy Lutomirski <[email protected]>

Quoted 1 line

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

Did a From line get eaten?

Context tracking for KVM happens way too early in the vcpu_run() code. Anything after guest_enter_irqoff() and before guest_exit_irqoff() cannot use RCU and should also be not instrumented.

The current way of doing this covers way too much code. Move it closer to the actual vmenter/exit code.

Signed-off-by Thomas Gleixner <[email protected]>

Cc Paolo Bonzini <[email protected]>

Cc Sean Christopherson <[email protected]>

---
 arch/x86/kvm/svm/svm.c |   16 ++++++++++++++++
 arch/x86/kvm/vmx/vmx.c |   10 ++++++++++
 arch/x86/kvm/x86.c     |    2 --
 3 files changed, 26 insertions(+), 2 deletions(-)

arch/x86/kvm/svm/svm.c +16 -0

@@ -3330,6 +3330,14 @@ static void svm_vcpu_run(struct kvm_vcpu
 	 */
 	x86_spec_ctrl_set_guest(svm->spec_ctrl, svm->virt_spec_ctrl);
 
+	/*
+	 * Tell context tracking that this CPU is about to enter guest
+	 * mode. This has to be after x86_spec_ctrl_set_guest() because
+	 * that can take locks (lockdep needs RCU) and calls into world and
+	 * some more.
+	 */
+	guest_enter_irqoff();
+
 	__svm_vcpu_run(svm->vmcb_pa, (unsigned long *)&svm->vcpu.arch.regs);
 
 #ifdef CONFIG_X86_64

@@ -3340,6 +3348,14 @@ static void svm_vcpu_run(struct kvm_vcpu
 	loadsegment(gs, svm->host.gs);
 #endif
 #endif
+	/*
+	 * Tell context tracking that this CPU is back.
+	 *
+	 * This needs to be done before the below as native_read_msr()
+	 * contains a tracepoint and x86_spec_ctrl_restore_host() calls
+	 * into world and some more.
+	 */
+	guest_exit_irqoff();
 
 	/*
 	 * We do not use IBRS in the kernel. If this vCPU has used the

arch/x86/kvm/vmx/vmx.c +10 -0

@@ -6603,6 +6603,11 @@ static void vmx_vcpu_run(struct kvm_vcpu
 	 */
 	x86_spec_ctrl_set_guest(vmx->spec_ctrl, 0);
 
+	/*
+	 * Tell context tracking that this CPU is about to enter guest mode.
+	 */
+	guest_enter_irqoff();
+
 	/* L1D Flush includes CPU buffer clear to mitigate MDS */
 	if (static_branch_unlikely(&vmx_l1d_should_flush))
 		vmx_l1d_flush(vcpu);

@@ -6618,6 +6623,11 @@ static void vmx_vcpu_run(struct kvm_vcpu
 	vcpu->arch.cr2 = read_cr2();
 
 	/*
+	 * Tell context tracking that this CPU is back.
+	 */
+	guest_exit_irqoff();
+
+	/*
 	 * We do not use IBRS in the kernel. If this vCPU has used the
 	 * SPEC_CTRL MSR it may have left it on; save the value and
 	 * turn it off. This is much more efficient than blindly adding

arch/x86/kvm/x86.c +0 -1

@@ -8404,7 +8404,6 @@ static int vcpu_enter_guest(struct kvm_v
 	}
 
 	trace_kvm_entry(vcpu->vcpu_id);
-	guest_enter_irqoff();
 
 	fpregs_assert_state_consistent();
 	if (test_thread_flag(TIF_NEED_FPU_LOAD))

Context tracking for KVM happens way too early in the vcpu_run() code. Anything after guest_enter_irqoff() and before guest_exit_irqoff() cannot use RCU and should also be not instrumented.

The current way of doing this covers way too much code. Move it closer to the actual vmenter/exit code.

Quoted 7 lines · Thomas Gleixner

Context tracking for KVM happens way too early in the vcpu_run() code. Anything after guest_enter_irqoff() and before guest_exit_irqoff() cannot use RCU and should also be not instrumented.

The current way of doing this covers way too much code. Move it closer to the actual vmenter/exit code.

Now you've made me wonder what happens if someone traces vmx_vcpu_run(). I'm not sure I really want to think about this.

Add hardirq tracing to guest enter/exit functions in the same way as it is done in the user mode enter/exit code.

Signed-off-by Thomas Gleixner <[email protected]>

Cc Paolo Bonzini <[email protected]>

Cc Sean Christopherson <[email protected]>

---
 arch/x86/kvm/vmx/vmx.c |   25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

arch/x86/kvm/vmx/vmx.c +18 -2

@@ -6604,9 +6604,19 @@ static void vmx_vcpu_run(struct kvm_vcpu
 	x86_spec_ctrl_set_guest(vmx->spec_ctrl, 0);
 
 	/*
-	 * Tell context tracking that this CPU is about to enter guest mode.
+	 * VMENTER enables interrupts (host state), but the kernel state is
+	 * interrupts disabled when this is invoked. Also tell RCU about
+	 * it. This is the same logic as for exit_to_user_mode().
+	 *
+	 * 1) Trace interrupts on state
+	 * 2) Prepare lockdep with RCU on
+	 * 3) Invoke context tracking if enabled to adjust RCU state
+	 * 4) Tell lockdep that interrupts are enabled
 	 */
+	trace_hardirqs_on_prepare();
+	lockdep_hardirqs_on_prepare(CALLER_ADDR0);
 	guest_enter_irqoff();
+	lockdep_hardirqs_on(CALLER_ADDR0);
 
 	/* L1D Flush includes CPU buffer clear to mitigate MDS */
 	if (static_branch_unlikely(&vmx_l1d_should_flush))

@@ -6623,9 +6633,20 @@ static void vmx_vcpu_run(struct kvm_vcpu
 	vcpu->arch.cr2 = read_cr2();
 
 	/*
-	 * Tell context tracking that this CPU is back.
+	 * VMEXIT disables interrupts (host state), but tracing and lockdep
+	 * have them in state 'on'. Same as enter_from_user_mode().
+	 *
+	 * 1) Tell lockdep that interrupts are disabled
+	 * 2) Invoke context tracking if enabled to reactivate RCU
+	 * 3) Trace interrupts off state
+	 *

arch/x86/kvm/svm/svm.c +15 -0

+	 * VMENTER enables interrupts (host state), but the kernel state is
+	 * interrupts disabled when this is invoked. Also tell RCU about
+	 * it. This is the same logic as for exit_to_user_mode().
+	 *
+	 * 1) Trace interrupts on state
+	 * 2) Prepare lockdep with RCU on
+	 * 3) Invoke context tracking if enabled to adjust RCU state
+	 * 4) Tell lockdep that interrupts are enabled
+	 *
+	 * This has to be after x86_spec_ctrl_set_guest() because that can
+	 * take locks (lockdep needs RCU) and calls into world and some
+	 * more.
 	 */
+	trace_hardirqs_on_prepare();
+	lockdep_hardirqs_on_prepare(CALLER_ADDR0);
 	guest_enter_irqoff();
+	lockdep_hardirqs_on(CALLER_ADDR0);
 
 	__svm_vcpu_run(svm->vmcb_pa, (unsigned long *)&svm->vcpu.arch.regs);

arch/x86/kvm/svm/svm.c +8 -0

+      but tracing and lockdep have them in state 'on'. Same as
+	 * enter_from_user_mode().
+	 *
+	 * 1) Tell lockdep that interrupts are disabled
+	 * 2) Invoke context tracking if enabled to reactivate RCU
+	 * 3) Trace interrupts off state
 	 *
 	 * This needs to be done before the below as native_read_msr()
 	 * contains a tracepoint and x86_spec_ctrl_restore_host() calls
 	 * into world and some more.
 	 */
+	lockdep_hardirqs_off(CALLER_ADDR0);
 	guest_exit_irqoff();
+	trace_hardirqs_off_prepare();
 

arch/x86/kvm/svm/svm.c static void svm_vcpu_run(struct kvm_vcpu

Quoted 4 lines · Thomas Gleixner

arch/x86/kvm/svm/svm.c +2 -1

 	/*
-	 * Tell context tracking that this CPU is back.
+	 * VMEXIT disables interrupts (host state, see the CLI in the ASM
+	 * above),

Apart from the small inaccuracy in that CLI has moved to vmenter.S, the

comments and commit message don't really help my understanding of why this is needed.

It's true that interrupts cause a vmexit, and therefore from the processor point of view it's as if they are enabled. However, the interrupt remains latched until local_irq_enable() in vcpu_enter_guest, so from the point of view of the kernel interrupts are still disabled. I don't understand why it's necessary to inform tracing and lockdep about a processor-internal state that doesn't percolate up to the kernel.

For VMX indeed some care is necessary, because we the interrupt is eaten rather than latched. Therefore, we call the interrupt handler from handle_external_interrupt_irqoff while EFLAGS.IF is still clear. However, if informing trace and lockdep turns out to be unnecessary after all for SVM, it should be okay (and clearer) to place the code in handle_external_interrupt_irqoff (also in arch/x86/kvm/vmx/vmx.c) .

Instead, if I'm wrong, the four steps above are the same in code and comment, and same for the three steps in the comment below. Can you replace them with the "why" of this change?