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
