* Re: [PATCH] x86: bug.h: merge annotate_reachable into _BUG_FLAGS for __WARN_FLAGS
@ 2022-03-23 19:30 Charlemagne Lasse
2022-03-23 19:41 ` Nick Desaulniers
0 siblings, 1 reply; 4+ messages in thread
From: Charlemagne Lasse @ 2022-03-23 19:30 UTC (permalink / raw)
To: ndesaulniers
Cc: adobriyan, bp, dave.hansen, hpa, jpoimboe, linux-kernel,
linux-sparse, llvm, luc.vanoostenryck, mingo, nathan, peterz,
tglx, x86, Sasha Levin
> @@ -75,9 +77,9 @@ do { \
> */
> #define __WARN_FLAGS(flags) \
> do { \
> + __auto_type f = BUGFLAG_WARNING|(flags); \
> instrumentation_begin(); \
> - _BUG_FLAGS(ASM_UD2, BUGFLAG_WARNING|(flags)); \
> - annotate_reachable(); \
> + _BUG_FLAGS(ASM_UD2, f, ASM_REACHABLE); \
> instrumentation_end(); \
> } while (0)
This causes following sparse warning on x86:
make allnoconfig && touch init/version.c && make CHECK="sparse
-Wshadow" C=1 init/version.o
#
# No change to .config
#
CALL scripts/checksyscalls.sh
CALL scripts/atomic/check-atomics.sh
CHK include/generated/compile.h
CC init/version.o
CHECK init/version.c
init/version.c: note: in included file (through
include/linux/rculist.h, include/linux/pid.h, include/linux/sched.h,
include/linux/utsname.h):
./include/linux/rcupdate.h:1007:9: warning: symbol 'f' shadows an earlier one
./include/linux/rcupdate.h:1001:47: originally declared here
Affected versions (from the ones on kernel.org):
* 5.17 - bfb1a7c91fb7 ("x86/bug: Merge annotate_reachable() into
_BUG_FLAGS() asm")
* 5.16.17 - fe0c95903a68 ("x86/bug: Merge annotate_reachable() into
_BUG_FLAGS() asm")
Cannot be seen when changing the variable name:
diff --git a/arch/x86/include/asm/bug.h b/arch/x86/include/asm/bug.h
index bab883c0b6fe..cbd11e38252a 100644
--- a/arch/x86/include/asm/bug.h
+++ b/arch/x86/include/asm/bug.h
@@ -77,9 +77,9 @@ do { \
*/
#define __WARN_FLAGS(flags) \
do { \
- __auto_type f = BUGFLAG_WARNING|(flags); \
+ __auto_type __f = BUGFLAG_WARNING|(flags); \
instrumentation_begin(); \
- _BUG_FLAGS(ASM_UD2, f, ASM_REACHABLE); \
+ _BUG_FLAGS(ASM_UD2, __f, ASM_REACHABLE); \
instrumentation_end(); \
} while (0)
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] x86: bug.h: merge annotate_reachable into _BUG_FLAGS for __WARN_FLAGS
2022-03-23 19:30 [PATCH] x86: bug.h: merge annotate_reachable into _BUG_FLAGS for __WARN_FLAGS Charlemagne Lasse
@ 2022-03-23 19:41 ` Nick Desaulniers
2022-03-24 2:42 ` Vincent MAILHOL
0 siblings, 1 reply; 4+ messages in thread
From: Nick Desaulniers @ 2022-03-23 19:41 UTC (permalink / raw)
To: Charlemagne Lasse, Vincent Mailhol, jpoimboe
Cc: adobriyan, bp, dave.hansen, hpa, linux-kernel, linux-sparse,
llvm, luc.vanoostenryck, mingo, nathan, peterz, tglx, x86,
Sasha Levin
On Wed, Mar 23, 2022 at 12:30 PM Charlemagne Lasse
<charlemagnelasse@gmail.com> wrote:
>
> > @@ -75,9 +77,9 @@ do { \
> > */
> > #define __WARN_FLAGS(flags) \
> > do { \
> > + __auto_type f = BUGFLAG_WARNING|(flags); \
> > instrumentation_begin(); \
> > - _BUG_FLAGS(ASM_UD2, BUGFLAG_WARNING|(flags)); \
> > - annotate_reachable(); \
> > + _BUG_FLAGS(ASM_UD2, f, ASM_REACHABLE); \
> > instrumentation_end(); \
> > } while (0)
>
> This causes following sparse warning on x86:
>
> make allnoconfig && touch init/version.c && make CHECK="sparse
> -Wshadow" C=1 init/version.o
> #
> # No change to .config
> #
> CALL scripts/checksyscalls.sh
> CALL scripts/atomic/check-atomics.sh
> CHK include/generated/compile.h
> CC init/version.o
> CHECK init/version.c
> init/version.c: note: in included file (through
> include/linux/rculist.h, include/linux/pid.h, include/linux/sched.h,
> include/linux/utsname.h):
> ./include/linux/rcupdate.h:1007:9: warning: symbol 'f' shadows an earlier one
> ./include/linux/rcupdate.h:1001:47: originally declared here
Thanks for the report. There was already a fix sent for this:
https://lore.kernel.org/lkml/20220317065743.8467-1-mailhol.vincent@wanadoo.fr/
but it doesn't mention that sparse is warning about this, too.
I think if Vincent sent a v3 that mentioned that sparse is warning
about this, too, and cc'ed you, you could then supply
signed-off/tested-by tags (or just do so on v2, though it doesn't
mention sparse), and maybe Josh would be so kind as to pick that up?
>
>
> Affected versions (from the ones on kernel.org):
>
> * 5.17 - bfb1a7c91fb7 ("x86/bug: Merge annotate_reachable() into
> _BUG_FLAGS() asm")
> * 5.16.17 - fe0c95903a68 ("x86/bug: Merge annotate_reachable() into
> _BUG_FLAGS() asm")
>
> Cannot be seen when changing the variable name:
>
> diff --git a/arch/x86/include/asm/bug.h b/arch/x86/include/asm/bug.h
> index bab883c0b6fe..cbd11e38252a 100644
> --- a/arch/x86/include/asm/bug.h
> +++ b/arch/x86/include/asm/bug.h
> @@ -77,9 +77,9 @@ do { \
> */
> #define __WARN_FLAGS(flags) \
> do { \
> - __auto_type f = BUGFLAG_WARNING|(flags); \
> + __auto_type __f = BUGFLAG_WARNING|(flags); \
> instrumentation_begin(); \
> - _BUG_FLAGS(ASM_UD2, f, ASM_REACHABLE); \
> + _BUG_FLAGS(ASM_UD2, __f, ASM_REACHABLE); \
> instrumentation_end(); \
> } while (0)
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] x86: bug.h: merge annotate_reachable into _BUG_FLAGS for __WARN_FLAGS
2022-03-23 19:41 ` Nick Desaulniers
@ 2022-03-24 2:42 ` Vincent MAILHOL
0 siblings, 0 replies; 4+ messages in thread
From: Vincent MAILHOL @ 2022-03-24 2:42 UTC (permalink / raw)
To: Nick Desaulniers
Cc: Charlemagne Lasse, jpoimboe, adobriyan, bp, dave.hansen, hpa,
linux-kernel, linux-sparse, llvm, luc.vanoostenryck, mingo,
nathan, peterz, tglx, x86, Sasha Levin
On Thu. 24 Mar 2022 at 04:41, Nick Desaulniers <ndesaulniers@google.com> wrote:
> On Wed, Mar 23, 2022 at 12:30 PM Charlemagne Lasse
> <charlemagnelasse@gmail.com> wrote:
> >
> > > @@ -75,9 +77,9 @@ do { \
> > > */
> > > #define __WARN_FLAGS(flags) \
> > > do { \
> > > + __auto_type f = BUGFLAG_WARNING|(flags); \
> > > instrumentation_begin(); \
> > > - _BUG_FLAGS(ASM_UD2, BUGFLAG_WARNING|(flags)); \
> > > - annotate_reachable(); \
> > > + _BUG_FLAGS(ASM_UD2, f, ASM_REACHABLE); \
> > > instrumentation_end(); \
> > > } while (0)
> >
> > This causes following sparse warning on x86:
> >
> > make allnoconfig && touch init/version.c && make CHECK="sparse
> > -Wshadow" C=1 init/version.o
> > #
> > # No change to .config
> > #
> > CALL scripts/checksyscalls.sh
> > CALL scripts/atomic/check-atomics.sh
> > CHK include/generated/compile.h
> > CC init/version.o
> > CHECK init/version.c
> > init/version.c: note: in included file (through
> > include/linux/rculist.h, include/linux/pid.h, include/linux/sched.h,
> > include/linux/utsname.h):
> > ./include/linux/rcupdate.h:1007:9: warning: symbol 'f' shadows an earlier one
> > ./include/linux/rcupdate.h:1001:47: originally declared here
>
> Thanks for the report. There was already a fix sent for this:
> https://lore.kernel.org/lkml/20220317065743.8467-1-mailhol.vincent@wanadoo.fr/
> but it doesn't mention that sparse is warning about this, too.
>
> I think if Vincent sent a v3 that mentioned that sparse is warning
> about this, too, and cc'ed you, you could then supply
> signed-off/tested-by tags (or just do so on v2, though it doesn't
> mention sparse), and maybe Josh would be so kind as to pick that up?
Thank Nick, I did as you suggested. Here is the v3:
https://lore.kernel.org/all/20220324023742.106546-1-mailhol.vincent@wanadoo.fr/
Yours sincerely,
Vincent Mailhol
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH] x86: bug.h: merge annotate_reachable into _BUG_FLAGS for __WARN_FLAGS
@ 2022-02-02 20:55 Nick Desaulniers
0 siblings, 0 replies; 4+ messages in thread
From: Nick Desaulniers @ 2022-02-02 20:55 UTC (permalink / raw)
To: Borislav Petkov, Josh Poimboeuf
Cc: Peter Zijlstra, llvm, Nick Desaulniers, Thomas Gleixner,
Ingo Molnar, Dave Hansen, x86, H. Peter Anvin,
Luc Van Oostenryck, Nathan Chancellor, Alexey Dobriyan,
linux-kernel, linux-sparse
In __WARN_FLAGS, we had two asm statements (abbreviated):
asm volatile("ud2");
asm volatile(".pushsection .discard.reachable");
These pair of statements are used to trigger an exception, but then help
objtool understand that for warnings, control flow will be restored
immediately afterwards.
The problem is that volatile is not a compiler barrier. GCC explicitly
documents this.
> Note that the compiler can move even volatile asm instructions
> relative to other code, including across jump instructions.
Also, no clobbers are specified to prevent instructions from subsequent
statements from being scheduled by compiler before the second asm
statement. This can lead to instructions from subsequent statements
being emitted by the compiler before the second asm statement.
Providing a scheduling model such as via -march= options enables the
compiler to better schedule instructions with known latencies to hide
latencies from data hazards compared to inline asm statements in which
latencies are not estimated.
If an instruction gets scheduled by the compiler between the two asm
statements, then objtool will think that it is not reachable, producing
a warning.
To prevent instructions from being scheduled in between the two asm
statements, merge them.
This change also removes an unnecessary unreachable() asm annotation
from BUG() in favor of __builtin_unreachable(). objtool is able to track
that the ud2 from BUG() terminates control flow within the function.
Link: https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html#Volatile
Link: https://github.com/ClangBuiltLinux/linux/issues/1483
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
---
Note to reviewers:
This patch is related but somewhat orthogonal to
https://lore.kernel.org/llvm/20220114010526.1776605-1-ndesaulniers@google.com/.
We're still discussing in
https://lore.kernel.org/llvm/20220131225250.409564-1-ndesaulniers@google.com/
whether such a barrier exists to salvage instrumentation_{begin|end}. I
may have follow up patches based on the result of that discussion.
Regardless of the outcome, I think this patch stands on its own merit,
and any changes to instrumentation_{begin|end} would not undo the
merging of asm statements done in this patch. Finally, I tried adding a
diagnostic to clang to warn when volatile is being used in a way that's
suspicious (https://goto.google.com/llvm-cr/D118297), but I don't think
that will land. Thanks to Nathan for the relevant citiation of the GCC
docs.
arch/x86/include/asm/bug.h | 20 +++++++++++---------
include/linux/compiler.h | 21 +++++----------------
2 files changed, 16 insertions(+), 25 deletions(-)
diff --git a/arch/x86/include/asm/bug.h b/arch/x86/include/asm/bug.h
index 84b87538a15d..f98db09bffd4 100644
--- a/arch/x86/include/asm/bug.h
+++ b/arch/x86/include/asm/bug.h
@@ -22,7 +22,7 @@
#ifdef CONFIG_DEBUG_BUGVERBOSE
-#define _BUG_FLAGS(ins, flags) \
+#define _BUG_FLAGS(ins, flags, extra) \
do { \
asm_inline volatile("1:\t" ins "\n" \
".pushsection __bug_table,\"aw\"\n" \
@@ -31,7 +31,8 @@ do { \
"\t.word %c1" "\t# bug_entry::line\n" \
"\t.word %c2" "\t# bug_entry::flags\n" \
"\t.org 2b+%c3\n" \
- ".popsection" \
+ ".popsection\n" \
+ extra \
: : "i" (__FILE__), "i" (__LINE__), \
"i" (flags), \
"i" (sizeof(struct bug_entry))); \
@@ -39,14 +40,15 @@ do { \
#else /* !CONFIG_DEBUG_BUGVERBOSE */
-#define _BUG_FLAGS(ins, flags) \
+#define _BUG_FLAGS(ins, flags, extra) \
do { \
asm_inline volatile("1:\t" ins "\n" \
".pushsection __bug_table,\"aw\"\n" \
"2:\t" __BUG_REL(1b) "\t# bug_entry::bug_addr\n" \
"\t.word %c0" "\t# bug_entry::flags\n" \
"\t.org 2b+%c1\n" \
- ".popsection" \
+ ".popsection\n" \
+ extra \
: : "i" (flags), \
"i" (sizeof(struct bug_entry))); \
} while (0)
@@ -55,7 +57,7 @@ do { \
#else
-#define _BUG_FLAGS(ins, flags) asm volatile(ins)
+#define _BUG_FLAGS(ins, flags, extra) asm volatile(ins)
#endif /* CONFIG_GENERIC_BUG */
@@ -63,8 +65,8 @@ do { \
#define BUG() \
do { \
instrumentation_begin(); \
- _BUG_FLAGS(ASM_UD2, 0); \
- unreachable(); \
+ _BUG_FLAGS(ASM_UD2, 0, ""); \
+ __builtin_unreachable(); \
} while (0)
/*
@@ -75,9 +77,9 @@ do { \
*/
#define __WARN_FLAGS(flags) \
do { \
+ __auto_type f = BUGFLAG_WARNING|(flags); \
instrumentation_begin(); \
- _BUG_FLAGS(ASM_UD2, BUGFLAG_WARNING|(flags)); \
- annotate_reachable(); \
+ _BUG_FLAGS(ASM_UD2, f, ASM_REACHABLE); \
instrumentation_end(); \
} while (0)
diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 429dcebe2b99..0f7fd205ab7e 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -117,14 +117,6 @@ void ftrace_likely_update(struct ftrace_likely_data *f, int val,
*/
#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() __annotate_reachable(__COUNTER__)
-
#define __annotate_unreachable(c) ({ \
asm volatile(__stringify_label(c) ":\n\t" \
".pushsection .discard.unreachable\n\t" \
@@ -133,24 +125,21 @@ void ftrace_likely_update(struct ftrace_likely_data *f, int val,
})
#define annotate_unreachable() __annotate_unreachable(__COUNTER__)
-#define ASM_UNREACHABLE \
- "999:\n\t" \
- ".pushsection .discard.unreachable\n\t" \
- ".long 999b - .\n\t" \
+#define ASM_REACHABLE \
+ "998:\n\t" \
+ ".pushsection .discard.reachable\n\t" \
+ ".long 998b - .\n\t" \
".popsection\n\t"
/* Annotate a C jump table to allow objtool to follow the code flow */
#define __annotate_jump_table __section(".rodata..c_jump_table")
#else
-#define annotate_reachable()
#define annotate_unreachable()
+# define ASM_REACHABLE
#define __annotate_jump_table
#endif
-#ifndef ASM_UNREACHABLE
-# define ASM_UNREACHABLE
-#endif
#ifndef unreachable
# define unreachable() do { \
annotate_unreachable(); \
base-commit: 26291c54e111ff6ba87a164d85d4a4e134b7315c
--
2.35.0.rc2.247.g8bbb082509-goog
^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-03-24 2:42 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-23 19:30 [PATCH] x86: bug.h: merge annotate_reachable into _BUG_FLAGS for __WARN_FLAGS Charlemagne Lasse
2022-03-23 19:41 ` Nick Desaulniers
2022-03-24 2:42 ` Vincent MAILHOL
-- strict thread matches above, loose matches on Subject: below --
2022-02-02 20:55 Nick Desaulniers
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.