linux-kernel - Re: [PATCH 3/3] powerpc: use __builtin

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]

Message-ID: <44a19633-f2a9-79f9-da7c-16ba64a66600@c-s.fr>
Date:   Mon, 19 Aug 2019 17:05:46 +0200
From:   Christophe Leroy <christophe.leroy@....fr>
To:     Segher Boessenkool <segher@...nel.crashing.org>
Cc:     Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Paul Mackerras <paulus@...ba.org>,
        Michael Ellerman <mpe@...erman.id.au>,
        linux-kernel@...r.kernel.org, linuxppc-dev@...ts.ozlabs.org
Subject: Re: [PATCH 3/3] powerpc: use __builtin_trap() in BUG/WARN macros.



Le 19/08/2019 à 16:37, Segher Boessenkool a écrit :
> On Mon, Aug 19, 2019 at 04:08:43PM +0200, Christophe Leroy wrote:
>> Le 19/08/2019 à 15:23, Segher Boessenkool a écrit :
>>> On Mon, Aug 19, 2019 at 01:06:31PM +0000, Christophe Leroy wrote:
>>>> Note that we keep using an assembly text using "twi 31, 0, 0" for
>>>> inconditional traps because GCC drops all code after
>>>> __builtin_trap() when the condition is always true at build time.
>>>
>>> As I said, it can also do this for conditional traps, if it can prove
>>> the condition is always true.
>>
>> But we have another branch for 'always true' and 'always false' using
>> __builtin_constant_p(), which don't use __builtin_trap(). Is there
>> anything wrong with that ?:
> 
> The compiler might not realise it is constant when it evaluates the
> __builtin_constant_p, but only realises it later.  As the documentation
> for the builtin says:
>    A return of 0 does not indicate that the
>    value is _not_ a constant, but merely that GCC cannot prove it is a
>    constant with the specified value of the '-O' option.

So you mean GCC would not be able to prove that 
__builtin_constant_p(cond) is always true but it would be able to prove 
that if (cond)  is always true ?

And isn't there a away to tell GCC that '__builtin_trap()' is 
recoverable in our case ?

> 
> (and there should be many more and more serious warnings here).
> 
>> #define BUG_ON(x) do {						\
>> 	if (__builtin_constant_p(x)) {				\
>> 		if (x)						\
>> 			BUG();					\
>> 	} else {						\
>> 		if (x)						\
>> 			__builtin_trap();			\
>> 		BUG_ENTRY("", 0);				\
>> 	}							\
>> } while (0)
> 
> I think it may work if you do
> 
> #define BUG_ON(x) do {						\
> 	if (__builtin_constant_p(x)) {				\
> 		if (x)						\
> 			BUG();					\
> 	} else {						\
> 		BUG_ENTRY("", 0);				\
> 		if (x)						\
> 			__builtin_trap();			\
> 	}							\
> } while (0)

It doesn't work:

void test_bug1(unsigned long long a)
{
	BUG_ON(a);
}

00000090 <test_bug1>:
   90:	7c 63 23 78 	or      r3,r3,r4
   94:	0f 03 00 00 	twnei   r3,0
   98:	4e 80 00 20 	blr

RELOCATION RECORDS FOR [__bug_table]:
OFFSET   TYPE              VALUE
00000084 R_PPC_ADDR32      .text+0x00000090

As you see, the relocation in __bug_table points to the 'or' and not to 
the 'twnei'.

> 
> or even just
> 
> #define BUG_ON(x) do {						\
> 	BUG_ENTRY("", 0);					\
> 	if (x)							\
> 		__builtin_trap();				\
> 	}							\
> } while (0)
> 
> if BUG_ENTRY can work for the trap insn *after* it.
> 
>>> Can you put the bug table asm *before* the __builtin_trap maybe?  That
>>> should make it all work fine...  If you somehow can tell what machine
>>> instruction is that trap, anyway.
>>
>> And how can I tell that ?
> 
> I don't know how BUG_ENTRY works exactly.

It's basic, maybe too basic: it adds an inline asm with a label, and 
adds a .long in the __bug_table section with the address of that label.

When putting it after the __builtin_trap(), I changed it to using the 
address before the one of the label which is always the twxx instruction 
as far as I can see.

#define BUG_ENTRY(insn, flags, ...)			\
	__asm__ __volatile__(				\
		"1:	" insn "\n"			\
		".section __bug_table,\"aw\"\n"		\
		"2:\t" PPC_LONG "1b, %0\n"		\
		"\t.short %1, %2\n"			\
		".org 2b+%3\n"				\
		".previous\n"				\
		: : "i" (__FILE__), "i" (__LINE__),	\
		  "i" (flags),				\
		  "i" (sizeof(struct bug_entry)),	\
		  ##__VA_ARGS__)

Christophe