On Tue, Aug 31, 2021 at 11:27:20PM +1000, Michael Ellerman wrote:
> In commit 1e688dd2a3d6 ("powerpc/bug: Provide better flexibility to
> WARN_ON/__WARN_FLAGS() with asm goto") we changed WARN_ON(). Previously
> it would take the warning condition, x, and double negate it before
> converting the result to int, and passing that int to the underlying
> inline asm. ie:
>
> #define WARN_ON(x) ({
> int __ret_warn_on = !!(x);
> if (__builtin_constant_p(__ret_warn_on)) {
> ...
> } else {
> BUG_ENTRY(PPC_TLNEI " %4, 0",
> BUGFLAG_WARNING | BUGFLAG_TAINT(TAINT_WARN),
> "r" (__ret_warn_on));
>
> The asm then does a full register width comparison with zero and traps
> if it is non-zero (PPC_TLNEI).
>
> The new code instead passes the full expression, x, with some unknown
> type, to the inline asm:
>
> #define WARN_ON(x) ({
> ...
> do {
> if (__builtin_constant_p((x))) {
> ...
> } else {
> ...
> WARN_ENTRY(PPC_TLNEI " %4, 0",
> BUGFLAG_WARNING | BUGFLAG_TAINT(TAINT_WARN),
> __label_warn_on, "r" (x));
>
> This was not seen to cause any issues with GCC, however with clang in at
> least one case it leads to a WARN_ON() that fires incorrectly and
> repeatedly at boot, as reported[1] by Nathan:
>
> WARNING: CPU: 0 PID: 1 at lib/klist.c:62 .klist_add_tail+0x3c/0x110
> Modules linked in:
> CPU: 0 PID: 1 Comm: swapper/0 Tainted: G W
> 5.14.0-rc7-next-20210825 #1
> NIP: c0000000007ff81c LR: c00000000090a038 CTR: 0000000000000000
> REGS: c0000000073c32a0 TRAP: 0700 Tainted: G W
> (5.14.0-rc7-next-20210825)
> MSR: 8000000002029032 <SF,VEC,EE,ME,IR,DR,RI> CR: 22000a40 XER: 00000000
> CFAR: c00000000090a034 IRQMASK: 0
> GPR00: c00000000090a038 c0000000073c3540 c000000001be3200 0000000000000001
> GPR04: c0000000072d65c0 0000000000000000 c0000000091ba798 c0000000091bb0a0
> GPR08: 0000000000000001 0000000000000000 c000000008581918 fffffffffffffc00
> GPR12: 0000000044000240 c000000001dd0000 c000000000012300 0000000000000000
> GPR16: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> GPR20: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> GPR24: 0000000000000000 c0000000017e3200 0000000000000000 c000000001a0e778
> GPR28: c0000000072d65b0 c0000000072d65a8 c000000007de72c8 c0000000073c35d0
> NIP .klist_add_tail+0x3c/0x110
> LR .bus_add_driver+0x148/0x290
> Call Trace:
> 0xc0000000073c35d0 (unreliable)
> .bus_add_driver+0x148/0x290
> .driver_register+0xb8/0x190
> .__hid_register_driver+0x70/0xd0
> .redragon_driver_init+0x34/0x58
> .do_one_initcall+0x130/0x3b0
> .do_initcall_level+0xd8/0x188
> .do_initcalls+0x7c/0xdc
> .kernel_init_freeable+0x178/0x21c
> .kernel_init+0x34/0x220
> .ret_from_kernel_thread+0x58/0x60
> Instruction dump:
> fba10078 7c7d1b78 38600001 fb810070 3b9d0008 fbc10080 7c9e2378 389d0018
> fb9d0008 fb9d0010 90640000 fbdd0000 <0b1e0000> e87e0018 28230000 41820024
>
> The instruction dump shows that we are trapping because r30 is not zero:
> tdnei r30,0
>
> Where r30 = c000000007de72c8
>
> The WARN_ON() comes from:
>
> static void knode_set_klist(struct klist_node *knode, struct klist *klist)
> {
> knode->n_klist = klist;
> /* no knode deserves to start its life dead */
> WARN_ON(knode_dead(knode));
> ^^^^^^^^^^^^^^^^^
>
> Where:
>
> #define KNODE_DEAD 1LU
>
> static bool knode_dead(struct klist_node *knode)
> {
> return (unsigned long)knode->n_klist & KNODE_DEAD;
> }
>
> The full disassembly shows that the compiler has not generated any code
> to apply the "& KNODE_DEAD" to the n_klist pointer, which is surprising.
>
> Nathan filed an LLVM bug [2], in which Eli Friedman explained that "if
> you pass a value of a type that's narrower than a register to an inline
> asm, the high bits are undefined". In this case we are passing a bool
> to the inline asm, which is a single bit value, and so the compiler
> thinks it can leave the high bits of r30 unmasked, because only the
> value of bit 0 matters.
>
> Because the inline asm compares the full width of the register (32 or
> 64-bit) we need to ensure the value passed to the inline asm has all
> bits defined. So fix it by casting to long.
>
> We also already cast to long for the similar case in BUG_ENTRY(), which
> it turns out was added to fix a similar bug in 2005 in commit
> 32818c2eb6b8 ("[PATCH] ppc64: Fix issue with gcc 4.0 compiled kernels").
>
> [1]: http://lore.kernel.org/r/YSa1O4fcX1nNKqN/@Ryzen-9-3900X.localdomain
> [2]: https://bugs.llvm.org/show_bug.cgi?id=51634
>
> Fixes: 1e688dd2a3d6 ("powerpc/bug: Provide better flexibility to
> WARN_ON/__WARN_FLAGS() with asm goto")
> Reported-by: Nathan Chancellor <nat...@kernel.org>
> Signed-off-by: Michael Ellerman <m...@ellerman.id.au>
Reviewed-by: Nathan Chancellor <nat...@kernel.org>
Tested-by: Nathan Chancellor <nat...@kernel.org>
> ---
> arch/powerpc/include/asm/bug.h | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/include/asm/bug.h b/arch/powerpc/include/asm/bug.h
> index 1ee0f22313ee..02c08d1492f8 100644
> --- a/arch/powerpc/include/asm/bug.h
> +++ b/arch/powerpc/include/asm/bug.h
> @@ -119,7 +119,8 @@ __label_warn_on:
> \
> \
> WARN_ENTRY(PPC_TLNEI " %4, 0", \
> BUGFLAG_WARNING | BUGFLAG_TAINT(TAINT_WARN),
> \
> - __label_warn_on, "r" (x)); \
> + __label_warn_on, \
> + "r" ((__force long)(x))); \
> break; \
> __label_warn_on: \
> __ret_warn_on = true; \
>
> --
> 2.25.1
