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] [thread-next>] [day] [month] [year] [list]

Date:   Sat, 7 May 2022 07:04:01 +0000
From:   Christophe Leroy <christophe.leroy@...roup.eu>
To:     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>
CC:     Zeal Robot <zealci@....com.cn>,
        Jing Yangyang <jing.yangyang@....com.cn>
Subject: Re: [PATCH linux-next] power:pkeys: fix bugon.cocci warnings



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.

Christophe