linux-kernel - Re: [PATCH linux-next] power:pkeys: fix bugon.cocci warnings

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Sat, 7 May 2022 10:49:31 +0200
From:   Arnd Bergmann <arnd@...db.de>
To:     Christophe Leroy <christophe.leroy@...roup.eu>
Cc:     Michael Ellerman <mpe@...erman.id.au>, CGEL <cgel.zte@...il.com>,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Paul Mackerras <paulus@...ba.org>,
        Sandipan Das <sandipan@...ux.ibm.com>,
        "linuxppc-dev@...ts.ozlabs.org" <linuxppc-dev@...ts.ozlabs.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Zeal Robot <zealci@....com.cn>,
        Jing Yangyang <jing.yangyang@....com.cn>
Subject: Re: [PATCH linux-next] power:pkeys: fix bugon.cocci warnings

On Sat, May 7, 2022 at 9:04 AM Christophe Leroy
<christophe.leroy@...roup.eu> wrote:
> Le 02/05/2022 à 15:24, Michael Ellerman a écrit :
> > CGEL <cgel.zte@...il.com> writes:
> >> From: Jing Yangyang <jing.yangyang@....com.cn>
> >>
> >> Use BUG_ON instead of a if condition followed by BUG.
> >>
> >> ./arch/powerpc/include/asm/book3s/64/pkeys.h:21:2-5:WARNING
> >> Use BUG_ON instead of if condition followed by BUG.
> >> ./arch/powerpc/include/asm/book3s/64/pkeys.h:14:2-5:WARNING
> >> Use BUG_ON instead of if condition followed by BUG.
> >>
> >> Generated by: scripts/coccinelle/misc/bugon.cocci
> >>
> >> Reported-by: Zeal Robot <zealci@....com.cn>
> >> Signed-off-by: Jing Yangyang <jing.yangyang@....com.cn>
> >> ---
> >>   arch/powerpc/include/asm/book3s/64/pkeys.h | 6 ++----
> >>   1 file changed, 2 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/arch/powerpc/include/asm/book3s/64/pkeys.h b/arch/powerpc/include/asm/book3s/64/pkeys.h
> >> index 5b17813..5f74f0c 100644
> >> --- a/arch/powerpc/include/asm/book3s/64/pkeys.h
> >> +++ b/arch/powerpc/include/asm/book3s/64/pkeys.h
> >> @@ -10,15 +10,13 @@ static inline u64 vmflag_to_pte_pkey_bits(u64 vm_flags)
> >>      if (!mmu_has_feature(MMU_FTR_PKEY))
> >>              return 0x0UL;
> >>
> >> -    if (radix_enabled())
> >> -            BUG();
> >> +    BUG_ON(radix_enabled());
> >>      return hash__vmflag_to_pte_pkey_bits(vm_flags);
> >>   }
> >>
> >>   static inline u16 pte_to_pkey_bits(u64 pteflags)
> >>   {
> >> -    if (radix_enabled())
> >> -            BUG();
> >> +    BUG_ON(radix_enabled());
> >>      return hash__pte_to_pkey_bits(pteflags);
> >>   }
> >
> > Have you checked how this changes the generated code?
> >
> > radix_enabled() is a jump label, via mmu_feature().
> >
> > Possibly the compiler just works it all out and generates the same code,
> > but I'd want some evidence of that before merging this.
>
> Seems like the compiler is not that good, the generated code for test1()
> is much better than the one for test2(), see below.
>
> void test1(void)
> {
>         if (radix_enabled())
>                 BUG();
> }
>
> void test2(void)
> {
>         BUG_ON(radix_enabled());
> }
>
> 0000000000000900 <.test1>:
>   900:  60 00 00 00     nop
>   904:  0f e0 00 00     twui    r0,0
>   908:  60 00 00 00     nop
>   90c:  60 00 00 00     nop
>   910:  4e 80 00 20     blr
>   914:  60 00 00 00     nop
>   918:  60 00 00 00     nop
>   91c:  60 00 00 00     nop
>
> 0000000000000920 <.test2>:
>   920:  60 00 00 00     nop
>   924:  39 20 00 01     li      r9,1
>   928:  0b 09 00 00     tdnei   r9,0
>   92c:  4e 80 00 20     blr
>   930:  39 20 00 00     li      r9,0
>   934:  0b 09 00 00     tdnei   r9,0
>   938:  4e 80 00 20     blr
>
>
> We should keep things as they are and change the coccinelle script.

Maybe just drop the custom ppc64 BUG_ON() then if it creates worse
code? The default BUG_ON() should be equivalent to the open-coded
version.

        Arnd