linux-kernel - Re: [PATCH 01/16] powerpc: Replace unreachable() with it's builtin variant in WARN

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

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 18 Aug 2022 16:16:32 +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 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.

- Naveen