[PATCH] objtool: prefer memory clobber & %= to volatile & __COUNTER__

From: Nick Desaulniers <ndesaulniers@google.com>
To: Josh Poimboeuf <jpoimboe@redhat.com>, Borislav Petkov <bp@alien8.de>
Cc: Vasily Gorbik <gor@linux.ibm.com>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	 Ingo Molnar <mingo@kernel.org>,
	Dave Hansen <dave.hansen@linux.intel.com>,
	 Thomas Gleixner <tglx@linutronix.de>,
	Peter Zijlstra <peterz@infradead.org>,
	 Luc Van Oostenryck <luc.vanoostenryck@gmail.com>,
	x86@kernel.org, llvm@lists.linux.dev,
	 linux-sparse@vger.kernel.org, linux-kernel@vger.kernel.org,
	 Nick Desaulniers <ndesaulniers@google.com>,
	kernel test robot <lkp@intel.com>,
	 Nathan Chancellor <nathan@kernel.org>
Subject: [PATCH] objtool: prefer memory clobber & %= to volatile & __COUNTER__
Date: Thu, 13 Jan 2022 17:05:26 -0800	[thread overview]
Message-ID: <20220114010526.1776605-1-ndesaulniers@google.com> (raw)

commit dcce50e6cc4d ("compiler.h: Fix annotation macro misplacement with Clang")
mentions:

> 'volatile' is ignored for some reason and Clang feels free to move the
> reachable() annotation away from its intended location.

Indeed, volatile is not a compiler barrier. Particularly once `-march=`
flags are used under certain configs, LLVM's machine-scheduler can be
observed moving instructions across the asm statement meant to point to
known reachable or unreachable code, as reported by 0day bot.

Prefer a memory clobber which is a compiler barrier that prevents these
re-orderings and remove the volatile qualifier.

Looking closer, the use of __COUNTER__ seems to have been used to
prevent de-duplication of these asm statements. The GCC manual mentions:

> Under certain circumstances, GCC may duplicate (or remove duplicates
> of) your assembly code when optimizing. This can lead to unexpected
> duplicate symbol errors during compilation if your asm code defines
> symbols or labels. Using ‘%=’ (see AssemblerTemplate) may help resolve
> this problem.
>
> ‘%=’ Outputs a number that is unique to each instance of the asm
> statement in the entire compilation. This option is useful when
> creating local labels and referring to them multiple times in a single
> template that generates multiple assembler instructions.

commit 3d1e236022cc ("objtool: Prevent GCC from merging annotate_unreachable()")

Mentions that

> The inline asm ‘%=’ token could be used for that, but unfortunately
> older versions of GCC don't support it.

From testing all versions of GCC available on godbolt.org, GCC 4.1+
seems to support 4.1. Since the minimum supported version of GCC at the
moment is GCC 5.1, it sounds like this is no longer a concern.

Prefer the %= assembler template to having to stringify __COUNTER__.

This commit is effectively a revert of the following commits:
commit dcce50e6cc4d ("compiler.h: Fix annotation macro misplacement with Clang")
commit f1069a8756b9 ("compiler.h: Avoid using inline asm operand modifiers")
commit c199f64ff93c ("instrumentation.h: Avoid using inline asm operand modifiers")
commit d0c2e691d1cb ("objtool: Add a comment for the unreachable annotation macros")
commit ec1e1b610917 ("objtool: Prevent GCC from merging annotate_unreachable(), take 2")
commit 3d1e236022cc ("objtool: Prevent GCC from merging annotate_unreachable()")

Link: https://github.com/ClangBuiltLinux/linux/issues/1566
Link: https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html#Volatile
Link: https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html#AssemblerTemplate
Link: https://lore.kernel.org/llvm/202112080834.XFYU8b5Q-lkp@intel.com/
Link: https://lore.kernel.org/llvm/202111300857.IyINAyJk-lkp@intel.com/
Reported-by: kernel test robot <lkp@intel.com>
Debugged-by: Nathan Chancellor <nathan@kernel.org>
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
---
 include/linux/compiler.h        | 31 +++++++++++--------------------
 include/linux/instrumentation.h | 24 ++++++++++--------------
 2 files changed, 21 insertions(+), 34 deletions(-)

diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 429dcebe2b99..3ac21b888d20 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -108,30 +108,21 @@ void ftrace_likely_update(struct ftrace_likely_data *f, int val,
 # define barrier_before_unreachable() do { } while (0)
 #endif
 
-/* Unreachable code */
+/* These macros help objtool understand GCC code flow for unreachable code. */
 #ifdef CONFIG_STACK_VALIDATION
-/*
- * These macros help objtool understand GCC code flow for unreachable code.
- * The __COUNTER__ based labels are a hack to make each instance of the macros
- * unique, to convince GCC not to merge duplicate inline asm statements.
- */
-#define __stringify_label(n) #n
-
-#define __annotate_reachable(c) ({					\
-	asm volatile(__stringify_label(c) ":\n\t"			\
-		     ".pushsection .discard.reachable\n\t"		\
-		     ".long " __stringify_label(c) "b - .\n\t"		\
-		     ".popsection\n\t" : : "i" (c));			\
+#define annotate_reachable() ({			\
+	asm (".Lreachable%=:\n\t"			\
+	     ".pushsection .discard.reachable\n\t"	\
+	     ".long .Lreachable%= - .\n\t"		\
+	     ".popsection\n\t" ::: "memory");		\
 })
-#define annotate_reachable() __annotate_reachable(__COUNTER__)
 
-#define __annotate_unreachable(c) ({					\
-	asm volatile(__stringify_label(c) ":\n\t"			\
-		     ".pushsection .discard.unreachable\n\t"		\
-		     ".long " __stringify_label(c) "b - .\n\t"		\
-		     ".popsection\n\t" : : "i" (c));			\
+#define annotate_unreachable() ({			\
+	asm (".Lunreachable%=:\n\t"			\
+	     ".pushsection .discard.unreachable\n\t"	\
+	     ".long .Lunreachable%= - .\n\t"		\
+	     ".popsection\n\t" ::: "memory");		\
 })
-#define annotate_unreachable() __annotate_unreachable(__COUNTER__)
 
 #define ASM_UNREACHABLE							\
 	"999:\n\t"							\
diff --git a/include/linux/instrumentation.h b/include/linux/instrumentation.h
index 24359b4a9605..0dae9c08764f 100644
--- a/include/linux/instrumentation.h
+++ b/include/linux/instrumentation.h
@@ -4,16 +4,13 @@
 
 #if defined(CONFIG_DEBUG_ENTRY) && defined(CONFIG_STACK_VALIDATION)
 
-#include <linux/stringify.h>
-
 /* Begin/end of an instrumentation safe region */
-#define __instrumentation_begin(c) ({					\
-	asm volatile(__stringify(c) ": nop\n\t"				\
-		     ".pushsection .discard.instr_begin\n\t"		\
-		     ".long " __stringify(c) "b - .\n\t"		\
-		     ".popsection\n\t" : : "i" (c));			\
+#define instrumentation_begin() ({			\
+	asm (".Linstbegin%=: nop\n\t"			\
+	     ".pushsection .discard.instr_begin\n\t"	\
+	     ".long .Linstbegin%= - .\n\t"		\
+	     ".popsection\n\t" ::: "memory");		\
 })
-#define instrumentation_begin() __instrumentation_begin(__COUNTER__)
 
 /*
  * Because instrumentation_{begin,end}() can nest, objtool validation considers
@@ -46,13 +43,12 @@
  * 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(c) ({					\
-	asm volatile(__stringify(c) ": nop\n\t"				\
-		     ".pushsection .discard.instr_end\n\t"		\
-		     ".long " __stringify(c) "b - .\n\t"		\
-		     ".popsection\n\t" : : "i" (c));			\
+#define instrumentation_end() ({			\
+	asm (".Linstend%=: nop\n\t"			\
+	     ".pushsection .discard.instr_end\n\t"	\
+	     ".long .Linstend%= - .\n\t"		\
+	     ".popsection\n\t" ::: "memory");		\
 })
-#define instrumentation_end() __instrumentation_end(__COUNTER__)
 #else
 # define instrumentation_begin()	do { } while(0)
 # define instrumentation_end()		do { } while(0)

base-commit: dcce50e6cc4d86a63dc0a9a6ee7d4f948ccd53a1
-- 
2.34.1.703.g22d0c6ccf7-goog

