| 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: <1660824799.vnjff6w3m0.naveen@linux.ibm.com>
Date: Thu, 18 Aug 2022 17:55:04 +0530
From: "Naveen N. Rao" <naveen.n.rao@...ux.vnet.ibm.com>
To: Christophe Leroy <christophe.leroy@...roup.eu>,
"linuxppc-dev@...ts.ozlabs.org" <linuxppc-dev@...ts.ozlabs.org>,
Sathvika Vasireddy <sv@...ux.ibm.com>
Cc: "aik@...abs.ru" <aik@...abs.ru>,
"chenzhongjin@...wei.com" <chenzhongjin@...wei.com>,
"jpoimboe@...hat.com" <jpoimboe@...hat.com>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"mbenes@...e.cz" <mbenes@...e.cz>,
"mingo@...hat.com" <mingo@...hat.com>,
"mpe@...erman.id.au" <mpe@...erman.id.au>,
"npiggin@...il.com" <npiggin@...il.com>,
"peterz@...radead.org" <peterz@...radead.org>,
"rostedt@...dmis.org" <rostedt@...dmis.org>
Subject: Re: [PATCH 01/16] powerpc: Replace unreachable() with it's builtin
variant in WARN_ON()
Christophe Leroy wrote:
>
>
> Le 18/08/2022 à 12:46, Naveen N. Rao a écrit :
>> Christophe Leroy wrote:
>>>
>>>
>>> Le 08/08/2022 à 13:48, Sathvika Vasireddy a écrit :
>>>> objtool is throwing *unannotated intra-function call*
>>>> warnings with a few instructions that are marked
>>>> unreachable. Replace unreachable() with __builtin_unreachable()
>>>> to fix these warnings, as the codegen remains same
>>>> with unreachable() and __builtin_unreachable().
>>>
>>> I think it is necessary to explain why using unreachable() is not
>>> necessary for powerpc, or even why using unreachable() is wrong.
>>>
>>> Allthough we are getting rid of the problem here by replacing
>>> unreachable() by __builtin_unreachable(), it might still be a problem
>>> in core parts of kernel which still use unreachable.
>>
>> I did a kernel build with this series applied, with a variant of
>> ppc64le_defconfig. I then did another build with the same config, but
>> with the below hunk to disable objtool:
>>
>> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
>> index 6be2e68fa9eb64..4c466acdc70d4c 100644
>> --- a/arch/powerpc/Kconfig
>> +++ b/arch/powerpc/Kconfig
>> @@ -237,8 +237,6 @@ config PPC
>> select HAVE_MOD_ARCH_SPECIFIC
>> select HAVE_NMI if PERF_EVENTS || (PPC64
>> && PPC_BOOK3S)
>> select HAVE_OPTPROBES
>> - select HAVE_OBJTOOL if PPC32 || MPROFILE_KERNEL
>> - select HAVE_OBJTOOL_MCOUNT if HAVE_OBJTOOL
>> select HAVE_PERF_EVENTS
>> select HAVE_PERF_EVENTS_NMI if PPC64
>> select HAVE_PERF_REGS
>>
>> This has the effect of disabling annotations for unreachable().
>>
>> When I compared the resulting object files, I did not see changes in
>> codegen relating to the annotation, like we do with using unreachable()
>> in __WARN_FLAGS().
>>
>> More specifically, arch/powerpc/kvm/book3s.o:kvmppc_h_logical_ci_load()
>> uses BUG(), and the generated code remains the same with/without the
>> unreachable() annotation.
>>
>> This suggests that the bad codegen we are seeing with the annotation in
>> unreachable() is limited to its use in __WARN_FLAGS(), which I suspect
>> is due to an interaction with the use of asm_volatile_goto() for
>> WARN_ENTRY().
>>
>> If I revert this patch (patch 01/16), gcc seems to add a label 8 bytes
>> before _some_ function in this object file, which happens to hold a
>> relocation against .TOC., and emits a bl to that symbol. Otherwise, gcc
>> either emits no new instruction for the annotation, or a 'nop' in some
>> cases.
>>
>> If I add a 'nop' between WARN_ENTRY() and unreachable() in
>> __WARN_FLAGS(), or convert WARN_ENTRY to BUG_ENTRY thereby removing use
>> of asm_volatile_goto(), the problem goes away and no bl is emitted:
>>
>> diff --git a/arch/powerpc/include/asm/bug.h
>> b/arch/powerpc/include/asm/bug.h
>> index 61a4736355c244..88e0027c20ba5c 100644
>> --- a/arch/powerpc/include/asm/bug.h
>> +++ b/arch/powerpc/include/asm/bug.h
>> @@ -99,6 +99,7 @@
>> __label__ __label_warn_on; \
>> \
>> WARN_ENTRY("twi 31, 0, 0", BUGFLAG_WARNING | (flags),
>> __label_warn_on); \
>> + __asm__ __volatile__("nop"); \
>> unreachable(); \
>> \
>> __label_warn_on:
>>
>>
>> In summary, I think the annotation itself is fine and we are only seeing
>> an issue with its usage after WARN_ENTRY() due to use of
>> asm_volatile_goto. Other uses of unreachable() don't seem to exhibit
>> this problem.
>>
>> As such, I think this patch is appropriate for this series, though I
>> think we should capture some of this information in the changelog.
>>
>> Note also that if and when we start utlizing the annotation, if we
>> classify twui as INSN_BUG, this change will continue to be appropriate.
>>
>
> INSN_TRAP instead of INSN_BUG ?
INSN_BUG, in line with your suggestion here:
http://lkml.kernel.org/r/ff623097-9f18-3914-5eae-bc6e4cd1510f@csgroup.eu
Peter was of the opinion that INSN_TRAP may not be what we want:
http://lkml.kernel.org/r/YsLSU6idNME/BtwH@hirez.programming.kicks-ass.net
If we classify twui as INSN_BUG, then objtool will know to stop control
flow here without the need for an annotation. Parsing extable will
then show that control flow continues with the label subsequently.
- Naveen
Powered by blists - more mailing lists