Re: [PATCH v2 2/2] powerpc/bug: Provide better flexibility to WARN_ON/__WARN_FLAGS() with asm goto

From: Nathan Chancellor
Date: Thu Aug 26 2021 - 14:54:24 EST

Next message: Stephen Boyd: "Re: [PATCH RESEND 1/2] dt-bindings: clock: add Qualcomm MSM8953 GCC driver bindings"
Previous message: pr-tracker-bot: "Re: [GIT PULL] arm64 fix for 5.14"
In reply to: Segher Boessenkool: "Re: [PATCH v2 2/2] powerpc/bug: Provide better flexibility to WARN_ON/__WARN_FLAGS() with asm goto"
Next in thread: Nathan Chancellor: "Re: [PATCH v2 2/2] powerpc/bug: Provide better flexibility to WARN_ON/__WARN_FLAGS() with asm goto"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Thu, Aug 26, 2021 at 01:21:39PM +1000, Michael Ellerman wrote:
> Nathan Chancellor <nathan@xxxxxxxxxx> writes:
> > On Tue, Apr 13, 2021 at 04:38:10PM +0000, Christophe Leroy wrote:
> >> Using asm goto in __WARN_FLAGS() and WARN_ON() allows more
> >> flexibility to GCC.
> ...
> >
> > This patch as commit 1e688dd2a3d6 ("powerpc/bug: Provide better
> > flexibility to WARN_ON/__WARN_FLAGS() with asm goto") cause a WARN_ON in
> > klist_add_tail to trigger over and over on boot when compiling with
> > clang:
> >
> > [ 2.177416][ T1] WARNING: CPU: 0 PID: 1 at lib/klist.c:62 .klist_add_tail+0x3c/0x110
> > [ 2.177456][ T1] Modules linked in:
> > [ 2.177481][ T1] CPU: 0 PID: 1 Comm: swapper/0 Tainted: G W 5.14.0-rc7-next-20210825 #1
> > [ 2.177520][ T1] NIP: c0000000007ff81c LR: c00000000090a038 CTR: 0000000000000000
> > [ 2.177557][ T1] REGS: c0000000073c32a0 TRAP: 0700 Tainted: G W (5.14.0-rc7-next-20210825)
> > [ 2.177593][ T1] MSR: 8000000002029032 <SF,VEC,EE,ME,IR,DR,RI> CR: 22000a40 XER: 00000000
> > [ 2.177667][ T1] CFAR: c00000000090a034 IRQMASK: 0
> > [ 2.177667][ T1] GPR00: c00000000090a038 c0000000073c3540 c000000001be3200 0000000000000001
> > [ 2.177667][ T1] GPR04: c0000000072d65c0 0000000000000000 c0000000091ba798 c0000000091bb0a0
> > [ 2.177667][ T1] GPR08: 0000000000000001 0000000000000000 c000000008581918 fffffffffffffc00
> > [ 2.177667][ T1] GPR12: 0000000044000240 c000000001dd0000 c000000000012300 0000000000000000
> > [ 2.177667][ T1] GPR16: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> > [ 2.177667][ T1] GPR20: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> > [ 2.177667][ T1] GPR24: 0000000000000000 c0000000017e3200 0000000000000000 c000000001a0e778
> > [ 2.177667][ T1] GPR28: c0000000072d65b0 c0000000072d65a8 c000000007de72c8 c0000000073c35d0
> > [ 2.178019][ T1] NIP [c0000000007ff81c] .klist_add_tail+0x3c/0x110
> > [ 2.178058][ T1] LR [c00000000090a038] .bus_add_driver+0x148/0x290
> > [ 2.178088][ T1] Call Trace:
> > [ 2.178105][ T1] [c0000000073c3540] [c0000000073c35d0] 0xc0000000073c35d0 (unreliable)
> > [ 2.178150][ T1] [c0000000073c35d0] [c00000000090a038] .bus_add_driver+0x148/0x290
> > [ 2.178190][ T1] [c0000000073c3670] [c00000000090fae8] .driver_register+0xb8/0x190
> > [ 2.178234][ T1] [c0000000073c3700] [c000000000be55c0] .__hid_register_driver+0x70/0xd0
> > [ 2.178275][ T1] [c0000000073c37a0] [c00000000116955c] .redragon_driver_init+0x34/0x58
> > [ 2.178314][ T1] [c0000000073c3820] [c000000000011ae0] .do_one_initcall+0x130/0x3b0
> > [ 2.178357][ T1] [c0000000073c3bb0] [c0000000011065e0] .do_initcall_level+0xd8/0x188
> > [ 2.178403][ T1] [c0000000073c3c50] [c0000000011064a8] .do_initcalls+0x7c/0xdc
> > [ 2.178445][ T1] [c0000000073c3ce0] [c000000001106238] .kernel_init_freeable+0x178/0x21c
> > [ 2.178491][ T1] [c0000000073c3d90] [c000000000012334] .kernel_init+0x34/0x220
> > [ 2.178530][ T1] [c0000000073c3e10] [c00000000000cf50] .ret_from_kernel_thread+0x58/0x60
> > [ 2.178569][ T1] Instruction dump:
> > [ 2.178592][ T1] fba10078 7c7d1b78 38600001 fb810070 3b9d0008 fbc10080 7c9e2378 389d0018
> > [ 2.178662][ T1] fb9d0008 fb9d0010 90640000 fbdd0000 <0b1e0000> e87e0018 28230000 41820024
> > [ 2.178728][ T1] ---[ end trace 52ed3431f58f1847 ]---
> >
> > Is this a bug with clang or is there something wrong with the patch? The
> > vmlinux image is available at [1] if you want to inspect it and our QEMU
> > command and the warning at boot can be viewed at [2]. If there is any
> > other information I can provide, please let me know.
> >
> > [1] https://builds.tuxbuild.com/1xDcmp3Tvno0TTGxDVPedRKIKM2/
> > [2] https://github.com/ClangBuiltLinux/continuous-integration2/commit/cee159b66a58eb57fa2359e7888074b9da24126c/checks/3422232736/logs
>
> Thanks.
>
> This is the generated assembly:
>
> c0000000007ff600 <.klist_add_tail>:
> c0000000007ff600: 7c 08 02 a6 mflr r0
> c0000000007ff604: f8 01 00 10 std r0,16(r1)
> c0000000007ff608: f8 21 ff 71 stdu r1,-144(r1) ^ prolog
> c0000000007ff60c: fb a1 00 78 std r29,120(r1) save r29 to stack
> c0000000007ff610: 7c 7d 1b 78 mr r29,r3 r29 = struct klist_node *n
> c0000000007ff614: 38 60 00 01 li r3,1 r3 = 1
> c0000000007ff618: fb 81 00 70 std r28,112(r1) save r28 to stack
> c0000000007ff61c: 3b 9d 00 08 addi r28,r29,8 r28 = &n->n_node
> c0000000007ff620: fb c1 00 80 std r30,128(r1) save r30 to stack
> c0000000007ff624: 7c 9e 23 78 mr r30,r4 r30 = struct klist *k
> c0000000007ff628: 38 9d 00 18 addi r4,r29,24 r4 = &n->n_ref
> c0000000007ff62c: fb 9d 00 08 std r28,8(r29) n->n_node.next = &n->n_node INIT_LIST_HEAD
> c0000000007ff630: fb 9d 00 10 std r28,16(r29) n->n_node.prev = &n->n_node
> c0000000007ff634: 90 64 00 00 stw r3,0(r4) kref_init(&n->n_ref)
> c0000000007ff638: fb dd 00 00 std r30,0(r29) n->n_klist = k
> c0000000007ff63c: 0b 1e 00 00 tdnei r30,0 trap if r30 (k) is not zero
>
>
> 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));
> ^^^^^^^^^^^^^^^^^
> }
>
> Which expands to:
>
> static void knode_set_klist(struct klist_node *knode, struct klist *klist)
> {
> knode->n_klist = klist;
>
> ({
> bool __ret_warn_on = false;
> do {
> ...
> } else {
> __label__ __label_warn_on;
> do {
> asm goto(
> "1: "
> "tdnei"
> "
> " " % 4,
> 0 " "\n " ".section __ex_table,\"a\";"
> " "
> ".balign 4;"
> " "
> ".long (1b) - . ;"
> " "
> ".long (%l[__label_warn_on]) - . ;"
> " "
> ".previous"
> " "
> ".section __bug_table,\"aw\"\n"
> "2:\t.4byte 1b - 2b, %0 - 2b\n"
> "\t.short %1, %2\n"
> ".org 2b+%3\n"
> ".previous\n"
> :
> : "i"("lib/klist.c"), "i"(62),
> "i"((1 << 0) | ((9) << 8)),
> "i"(sizeof(struct bug_entry)),
> "r"(knode_dead(knode))
> ^^^^^^^^^^^^^^^^^^^^^
>
> :
> : __label_warn_on);
> asm("");
> } while (0);
> break;
> __label_warn_on:
> __ret_warn_on = true;
> }
> } while (0);
> __builtin_expect(!!(__ret_warn_on), 0);
> });
> }
>
> And knode_dead is:
>
> #define KNODE_DEAD 1LU
>
> static bool knode_dead(struct klist_node *knode)
> {
> return (unsigned long)knode->n_klist & KNODE_DEAD;
> }
>
>
> So it's meant to warn if (n_klist & KNODE_DEAD) is not equal to zero.
>
> But in the asm:
>
> c0000000007ff600 <.klist_add_tail>:
> ...
> c0000000007ff624: 7c 9e 23 78 mr r30,r4 r30 = struct klist *k
> ...
> c0000000007ff638: fb dd 00 00 std r30,0(r29) n->n_klist = k
> c0000000007ff63c: 0b 1e 00 00 tdnei r30,0 trap if r30 (k) is not zero
>
>
> It's just warning if n_klist is not equal to zero. ie. we lost the "& KNODE_DEAD".
>
> In the GCC output you can see it:
>
> c0000000008c8a30 <.klist_node_init>:
> c0000000008c8a30: 39 24 00 08 addi r9,r4,8
> c0000000008c8a34: 39 40 00 01 li r10,1
> c0000000008c8a38: f9 24 00 08 std r9,8(r4)
> c0000000008c8a3c: f9 24 00 10 std r9,16(r4)
> c0000000008c8a40: 91 44 00 18 stw r10,24(r4)
> c0000000008c8a44: f8 64 00 00 std r3,0(r4)
> c0000000008c8a48: 54 69 07 fe clrlwi r9,r3,31
> c0000000008c8a4c: 0b 09 00 00 tdnei r9,0
>
> ie. the clrlwi is "clear left (word) immediate", and zeroes everything
> except bit 0, which is equivalent to "& KNODE_DEAD".
>
>
> So there seems to be some misunderstanding with clang, it doesn't like
> us passing an expression to the inline asm.
>
> AFAIK it is legal to pass expressions as inputs to inline asm, ie. it
> doesn't have to just be a variable name.
>
> This patch seems to fix it. Not sure if that's just papering over it though.
>
> diff --git a/arch/powerpc/include/asm/bug.h b/arch/powerpc/include/asm/bug.h
> index 1ee0f22313ee..75fcb4370d96 100644
> --- a/arch/powerpc/include/asm/bug.h
> +++ b/arch/powerpc/include/asm/bug.h
> @@ -119,7 +119,7 @@ __label_warn_on: \
> \
> WARN_ENTRY(PPC_TLNEI " %4, 0", \
> BUGFLAG_WARNING | BUGFLAG_TAINT(TAINT_WARN), \
> - __label_warn_on, "r" (x)); \
> + __label_warn_on, "r" (!!(x))); \
> break; \
> __label_warn_on: \
> __ret_warn_on = true; \
>
>
> Generates:
>
> c0000000008e2ac0 <.klist_add_tail>:
> c0000000008e2ac0: 7c 08 02 a6 mflr r0
> c0000000008e2ac4: f8 01 00 10 std r0,16(r1)
> c0000000008e2ac8: f8 21 ff 71 stdu r1,-144(r1)
> c0000000008e2acc: fb a1 00 78 std r29,120(r1)
> c0000000008e2ad0: 7c 7d 1b 78 mr r29,r3
> c0000000008e2ad4: 38 60 00 01 li r3,1
> c0000000008e2ad8: fb c1 00 80 std r30,128(r1)
> c0000000008e2adc: 7c 9e 23 78 mr r30,r4
> c0000000008e2ae0: 38 9d 00 18 addi r4,r29,24
> c0000000008e2ae4: 57 c5 07 fe clrlwi r5,r30,31 <-
> c0000000008e2ae8: fb 81 00 70 std r28,112(r1)
> c0000000008e2aec: 3b 9d 00 08 addi r28,r29,8
> c0000000008e2af0: fb 9d 00 08 std r28,8(r29)
> c0000000008e2af4: fb 9d 00 10 std r28,16(r29)
> c0000000008e2af8: 90 64 00 00 stw r3,0(r4)
> c0000000008e2afc: fb dd 00 00 std r30,0(r29)
> c0000000008e2b00: 0b 05 00 00 tdnei r5,0 <-

Thank you for the in-depth explanation and triage! I have gone ahead and
created a standalone reproducer that shows this based on the
preprocessed file and opened https://bugs.llvm.org/show_bug.cgi?id=51634
so the LLVM developers can take a look.

Cheers,
Nathan

Next message: Stephen Boyd: "Re: [PATCH RESEND 1/2] dt-bindings: clock: add Qualcomm MSM8953 GCC driver bindings"
Previous message: pr-tracker-bot: "Re: [GIT PULL] arm64 fix for 5.14"
In reply to: Segher Boessenkool: "Re: [PATCH v2 2/2] powerpc/bug: Provide better flexibility to WARN_ON/__WARN_FLAGS() with asm goto"
Next in thread: Nathan Chancellor: "Re: [PATCH v2 2/2] powerpc/bug: Provide better flexibility to WARN_ON/__WARN_FLAGS() with asm goto"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]