Re: [patch V6 00/37] x86/entry: Rework leftovers and merge plan

[Peter Zijlstra]

On Tue, May 19, 2020 at 10:38:26AM +0200, Peter Zijlstra wrote:
> $ make CC=gcc-9 O=defconfig-build/ vmlinux -j40 -s
> vmlinux.o: warning: objtool: exc_debug()+0x158: call to trace_hwlat_timestamp() leaves .noinstr.text section
> vmlinux.o: warning: objtool: exc_nmi()+0x190: call to trace_hwlat_timestamp() leaves .noinstr.text section
> vmlinux.o: warning: objtool: do_machine_check()+0x46: call to mce_rdmsrl() leaves .noinstr.text section
> $
> 
> (it really isn't defconfig, but your config-fail + DEBUG_ENTRY)
> 

With comment on, as requested.

---
diff --git a/arch/x86/include/asm/bug.h b/arch/x86/include/asm/bug.h
index f128e5c2ed42..fb34ff641e0a 100644
--- a/arch/x86/include/asm/bug.h
+++ b/arch/x86/include/asm/bug.h
@@ -79,8 +79,8 @@ do {								\
 do {								\
 	instrumentation_begin();				\
 	_BUG_FLAGS(ASM_UD2, BUGFLAG_WARNING|(flags));		\
-	instrumentation_end();					\
 	annotate_reachable();					\
+	instrumentation_end();					\
 } while (0)
 
 #include <asm-generic/bug.h>
diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 7db5902f8f6e..4b8fabed46ae 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -120,25 +120,61 @@ void ftrace_likely_update(struct ftrace_likely_data *f, int val,
 /* Annotate a C jump table to allow objtool to follow the code flow */
 #define __annotate_jump_table __section(.rodata..c_jump_table)
 
+#ifdef CONFIG_DEBUG_ENTRY
 /* Begin/end of an instrumentation safe region */
-#define instrumentation_begin() ({						\
+#define instrumentation_begin() ({					\
 	asm volatile("%c0:\n\t"						\
 		     ".pushsection .discard.instr_begin\n\t"		\
 		     ".long %c0b - .\n\t"				\
 		     ".popsection\n\t" : : "i" (__COUNTER__));		\
 })
 
-#define instrumentation_end() ({							\
-	asm volatile("%c0:\n\t"						\
+/*
+ * Because instrumentation_{begin,end}() can nest, objtool validation considers
+ * _begin() a +1 and _end() a -1 and computes a sum over the instructions.
+ * When the value is greater than 0, we consider instrumentation allowed.
+ *
+ * There is a problem with code like:
+ *
+ * noinstr void foo()
+ * {
+ *	instrumentation_begin();
+ *	...
+ *	if (cond) {
+ *		instrumentation_begin();
+ *		...
+ *		instrumentation_end();
+ *	}
+ *	bar();
+ *	instrumentation_end();
+ * }
+ *
+ * If instrumentation_end() would be an empty label, like all the other
+ * annotations, the inner _end(), which is at the end of a conditional block,
+ * would land on the instruction after the block.
+ *
+ * If we then consider the sum of the !cond path, we'll see that the call to
+ * bar() is with a 0-value, even though, we meant it to happen with a positive
+ * value.
+ *
+ * To avoid this, have _end() be a NOP instruction, this ensures it will be
+ * part of the condition block and does not escape.
+ */
+#define instrumentation_end() ({					\
+	asm volatile("%c0: nop\n\t"					\
 		     ".pushsection .discard.instr_end\n\t"		\
 		     ".long %c0b - .\n\t"				\
 		     ".popsection\n\t" : : "i" (__COUNTER__));		\
 })
+#endif /* CONFIG_DEBUG_ENTRY */
 
 #else
 #define annotate_reachable()
 #define annotate_unreachable()
 #define __annotate_jump_table
+#endif
+
+#ifndef instrumentation_begin
 #define instrumentation_begin()		do { } while(0)
 #define instrumentation_end()		do { } while(0)
 #endif