| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ff623097-9f18-3914-5eae-bc6e4cd1510f@csgroup.eu>
Date: Thu, 30 Jun 2022 12:33:35 +0200
From: Christophe Leroy <christophe.leroy@...roup.eu>
To: "Naveen N. Rao" <naveen.n.rao@...ux.vnet.ibm.com>,
"linuxppc-dev@...ts.ozlabs.org" <linuxppc-dev@...ts.ozlabs.org>,
Sathvika Vasireddy <sv@...ux.ibm.com>,
Sathvika Vasireddy <sv@...ux.vnet.ibm.com>
Cc: "aik@...abs.ru" <aik@...abs.ru>,
"benh@...nel.crashing.org" <benh@...nel.crashing.org>,
Chen Zhongjin <chenzhongjin@...wei.com>,
"jpoimboe@...hat.com" <jpoimboe@...hat.com>,
Linux ARM <linux-arm-kernel@...ts.infradead.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Marc Zyngier <maz@...nel.org>,
"mbenes@...e.cz" <mbenes@...e.cz>,
"mingo@...hat.com" <mingo@...hat.com>,
"mpe@...erman.id.au" <mpe@...erman.id.au>,
"paulus@...ba.org" <paulus@...ba.org>,
"peterz@...radead.org" <peterz@...radead.org>,
"rostedt@...dmis.org" <rostedt@...dmis.org>
Subject: Re: [RFC PATCH v3 11/12] powerpc: Remove unreachable() from WARN_ON()
Le 30/06/2022 à 11:58, Christophe Leroy a écrit :
>
>
> Le 30/06/2022 à 10:05, Naveen N. Rao a écrit :
>> Christophe Leroy wrote:
>>> Hi Sathvika,
>>>
>>> Adding ARM people as they seem to face the same kind of problem (see
>>> https://patchwork.kernel.org/project/linux-kbuild/patch/20220623014917.199563-33-chenzhongjin@huawei.com/)
>>>
>>>
>>> Le 27/06/2022 à 17:35, Sathvika Vasireddy a écrit :
>>>>
>>>> On 25/06/22 12:16, Christophe Leroy wrote:
>>>>>
>>>>> Le 24/06/2022 à 20:32, Sathvika Vasireddy a écrit :
>>>>>> objtool is throwing *unannotated intra-function call*
>>>>>> warnings with a few instructions that are marked
>>>>>> unreachable. Remove unreachable() from WARN_ON()
>>>>>> to fix these warnings, as the codegen remains same
>>>>>> with and without unreachable() in WARN_ON().
>>>>> Did you try the two exemples described in commit 1e688dd2a3d6
>>>>> ("powerpc/bug: Provide better flexibility to WARN_ON/__WARN_FLAGS()
>>>>> with
>>>>> asm goto") ?
>>>>>
>>>>> Without your patch:
>>>>>
>>>>> 00000640 <test>:
>>>>> 640: 81 23 00 84 lwz r9,132(r3)
>>>>> 644: 71 29 40 00 andi. r9,r9,16384
>>>>> 648: 40 82 00 0c bne 654 <test+0x14>
>>>>> 64c: 80 63 00 0c lwz r3,12(r3)
>>>>> 650: 4e 80 00 20 blr
>>>>> 654: 0f e0 00 00 twui r0,0
>>>>>
>>>>> 00000658 <test9w>:
>>>>> 658: 2c 04 00 00 cmpwi r4,0
>>>>> 65c: 41 82 00 0c beq 668 <test9w+0x10>
>>>>> 660: 7c 63 23 96 divwu r3,r3,r4
>>>>> 664: 4e 80 00 20 blr
>>>>> 668: 0f e0 00 00 twui r0,0
>>>>> 66c: 38 60 00 00 li r3,0
>>>>> 670: 4e 80 00 20 blr
>>>>>
>>>>>
>>>>> With your patch:
>>>>>
>>>>> 00000640 <test>:
>>>>> 640: 81 23 00 84 lwz r9,132(r3)
>>>>> 644: 71 29 40 00 andi. r9,r9,16384
>>>>> 648: 40 82 00 0c bne 654 <test+0x14>
>>>>> 64c: 80 63 00 0c lwz r3,12(r3)
>>>>> 650: 4e 80 00 20 blr
>>>>> 654: 0f e0 00 00 twui r0,0
>>>>> 658: 4b ff ff f4 b 64c <test+0xc> <==
>>>>>
>>>>> 0000065c <test9w>:
>>>>> 65c: 2c 04 00 00 cmpwi r4,0
>>>>> 660: 41 82 00 0c beq 66c <test9w+0x10>
>>>>> 664: 7c 63 23 96 divwu r3,r3,r4
>>>>> 668: 4e 80 00 20 blr
>>>>> 66c: 0f e0 00 00 twui r0,0
>>>>> 670: 38 60 00 00 li r3,0 <==
>>>>> 674: 4e 80 00 20 blr <==
>>>>> 678: 38 60 00 00 li r3,0
>>>>> 67c: 4e 80 00 20 blr
>>>>>
>>>> The builtin variant of unreachable (__builtin_unreachable()) works.
>>>>
>>>> How about using that instead of unreachable() ?
>>>>
>>>>
>>>
>>> In fact the problem comes from the macro annotate_unreachable() which
>>> is called by unreachable() before calling __build_unreachable().
>>>
>>> Seems like this macro adds (after the unconditional trap twui) a call
>>> to an empty function whose address is listed in section
>>> .discard.unreachable
>>>
>>> 1c78: 00 00 e0 0f twui r0,0
>>> 1c7c: 55 e7 ff 4b bl 3d0
>>> <qdisc_root_sleeping_lock.part.0>
>>>
>>>
>>> RELOCATION RECORDS FOR [.discard.unreachable]:
>>> OFFSET TYPE VALUE
>>> 0000000000000000 R_PPC64_REL32 .text+0x00000000000003d0
>>>
>>> The problem is that that function has size 0:
>>>
>>> 00000000000003d0 l F .text 0000000000000000
>>> qdisc_root_sleeping_lock.part.0
>>>
>>>
>>> And objtool is not prepared for a function with size 0.
>>
>> annotate_unreachable() seems to have been introduced in commit
>> 649ea4d5a624f0 ("objtool: Assume unannotated UD2 instructions are dead
>> ends").
>>
>> Objtool considers 'ud2' instruction to be fatal, so BUG() has
>> __builtin_unreachable(), rather than unreachable(). See commit
>> bfb1a7c91fb775 ("x86/bug: Merge annotate_reachable() into _BUG_FLAGS()
>> asm"). For the same reason, __WARN_FLAGS() is annotated with
>> _ASM_REACHABLE so that objtool can differentiate warnings from a BUG().
>>
>> On powerpc, we use trap variants for both and don't have a special
>> instruction for a BUG(). As such, for _WARN_FLAGS(), using
>> __builtin_unreachable() suffices to achieve optimal code generation
>> from the compiler. Objtool would consider subsequent instructions to
>> be reachable. For BUG(), we can continue to use unreachable() so that
>> objtool can differentiate these from traps used in warnings.
>
> Not sure I understand what you mean.
>
> __WARN_FLAGS() and BUG() both use 'twui' which is unconditionnal trap,
> as such both are the same.
>
> On the other side, WARN_ON() and BUG_ON() use tlbnei which is a
> conditionnel trap.
>
>>
>>>
>>> The following changes to objtool seem to fix the problem, most
>>> warning are gone with that change.
>>>
>>> diff --git a/tools/objtool/elf.c b/tools/objtool/elf.c
>>> index 63218f5799c2..37c0a268b7ea 100644
>>> --- a/tools/objtool/elf.c
>>> +++ b/tools/objtool/elf.c
>>> @@ -77,6 +77,8 @@ static int symbol_by_offset(const void *key, const
>>> struct rb_node *node)
>>>
>>> if (*o < s->offset)
>>> return -1;
>>> + if (*o == s->offset && !s->len)
>>> + return 0;
>>> if (*o >= s->offset + s->len)
>>> return 1;
>>>
>>> @@ -400,7 +402,7 @@ static void elf_add_symbol(struct elf *elf,
>>> struct symbol *sym)
>>> * Don't store empty STT_NOTYPE symbols in the rbtree. They
>>> * can exist within a function, confusing the sorting.
>>> */
>>> - if (!sym->len)
>>> + if (sym->type == STT_NOTYPE && !sym->len)
>>> rb_erase(&sym->node, &sym->sec->symbol_tree);
>>> }
>>
>> Is there a reason to do this, rather than change __WARN_FLAGS() to use
>> __builtin_unreachable()? Or, are you seeing an issue with
>> unreachable() elsewhere in the kernel?
>>
>
> At the moment I'm trying to understand what the issue is, and explore
> possible fixes. I guess if we tell objtool that after 'twui' subsequent
> instructions are unreachable, then __builtin_unreachable() is enough.
I get a nice result with the following changes (on top of Sathvika's
series):
diff --git a/arch/powerpc/include/asm/bug.h b/arch/powerpc/include/asm/bug.h
index df6c11e008b9..73f5650f98df 100644
--- a/arch/powerpc/include/asm/bug.h
+++ b/arch/powerpc/include/asm/bug.h
@@ -89,7 +89,7 @@
#define BUG() do { \
BUG_ENTRY("twi 31, 0, 0", 0); \
- unreachable(); \
+ __builtin_unreachable(); \
} while (0)
#define HAVE_ARCH_BUG
@@ -97,6 +97,7 @@
__label__ __label_warn_on; \
\
WARN_ENTRY("twi 31, 0, 0", BUGFLAG_WARNING | (flags), __label_warn_on); \
+ __builtin_unreachable(); \
\
__label_warn_on: \
break; \
diff --git a/tools/objtool/arch/powerpc/decode.c
b/tools/objtool/arch/powerpc/decode.c
index 06fc0206bf8e..9a0303304923 100644
--- a/tools/objtool/arch/powerpc/decode.c
+++ b/tools/objtool/arch/powerpc/decode.c
@@ -68,6 +68,8 @@ int arch_decode_instruction(struct objtool_file *file,
const struct section *sec
}
break;
}
+ if (insn == 0x0fe00000) /* twui */
+ *type = INSN_BUG;
return 0;
}
---
Christophe
Powered by blists - more mailing lists