linux-kernel - WARN_ON(1) generates ugly code since commit 6b15f678fb7d

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

Message-ID: <1d4fcfcf-e0b1-5af7-a54d-a5a3bbcedb89@c-s.fr>
Date:   Mon, 19 Aug 2019 10:18:57 +0200
From:   Christophe Leroy <christophe.leroy@....fr>
To:     Drew Davenport <ddavenport@...omium.org>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        "linuxppc-dev@...ts.ozlabs.org" <linuxppc-dev@...ts.ozlabs.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Kees Cook <keescook@...omium.org>
Subject: WARN_ON(1) generates ugly code since commit 6b15f678fb7d

Hi Drew,

I recently noticed gcc suddenly generating ugly code for WARN_ON(1).

It looks like commit 6b15f678fb7d ("include/asm-generic/bug.h: fix "cut 
here" for WARN_ON for __WARN_TAINT architectures") is the culprit.

unsigned long test_mul1(unsigned long a, unsigned long b)
{
     unsigned long long r = (unsigned long long)a * (unsigned long long)b;

     if (r > 0xffffffff)
         WARN_ON(1);

     return r;
}

Before that patch, I was getting the following code:

00000008 <test_mul1>:
    8:    7d 23 20 16     mulhwu  r9,r3,r4
    c:    7c 63 21 d6     mullw   r3,r3,r4
   10:    2f 89 00 00     cmpwi   cr7,r9,0
   14:    4d 9e 00 20     beqlr   cr7
   18:    0f e0 00 00     twui    r0,0
   1c:    4e 80 00 20     blr

Now I get:

0000002c <test_mul1>:
   2c:    7d 23 20 16     mulhwu  r9,r3,r4
   30:    94 21 ff f0     stwu    r1,-16(r1)
   34:    7c 08 02 a6     mflr    r0
   38:    93 e1 00 0c     stw     r31,12(r1)
   3c:    90 01 00 14     stw     r0,20(r1)
   40:    7f e3 21 d6     mullw   r31,r3,r4
   44:    2f 89 00 00     cmpwi   cr7,r9,0
   48:    40 9e 00 1c     bne     cr7,64 <test_mul1+0x38>
   4c:    80 01 00 14     lwz     r0,20(r1)
   50:    7f e3 fb 78     mr      r3,r31
   54:    83 e1 00 0c     lwz     r31,12(r1)
   58:    7c 08 03 a6     mtlr    r0
   5c:    38 21 00 10     addi    r1,r1,16
   60:    4e 80 00 20     blr
   64:    3c 60 00 00     lis     r3,0
             66: R_PPC_ADDR16_HA    .rodata.str1.4
   68:    38 63 00 00     addi    r3,r3,0
             6a: R_PPC_ADDR16_LO    .rodata.str1.4
   6c:    48 00 00 01     bl      6c <test_mul1+0x40>
             6c: R_PPC_REL24    printk
   70:    0f e0 00 00     twui    r0,0
   74:    4b ff ff d8     b       4c <test_mul1+0x20>

As you can see, a call to printk() is added, which means setting up a 
stack frame, saving volatile registers, etc ...
That's all the things we want to avoid when using WARN_ON().

And digging a bit more, I see that you are only adding this 'cut here' 
to calls like WARN_ON(1), ie where the condition is a constant.
For calls where the condition is not a constant, there is no change and 
no 'cut here' line added:

unsigned long test_mul2(unsigned long a, unsigned long b)
{
     unsigned long long r = (unsigned long long)a * (unsigned long long)b;

     WARN_ON(r > 0xffffffff);

     return r;
}

Before and after your patch, the code is clean and no call to add any 
'cut here' line.
00000078 <test_mul2>:
   78:    7d 43 20 16     mulhwu  r10,r3,r4
   7c:    7c 63 21 d6     mullw   r3,r3,r4
   80:    31 2a ff ff     addic   r9,r10,-1
   84:    7d 29 51 10     subfe   r9,r9,r10
   88:    0f 09 00 00     twnei   r9,0
   8c:    4e 80 00 20     blr

Was it your intention to modify the behaviour and kill the lightweight 
implementations of WARN_ON() ?

Looking into arch/powerpc/include/bug.h, I see that when the condition 
is constant, WARN_ON() uses __WARN(), which itself calls __WARN_FLAGS() 
with relevant flags.

In the old days, __WARN() was implemented in arch/powerpc/include/bug.h
Commit b2be05273a17 ("panic: Allow warnings to set different taint 
flags") replaced __WARN() by __WARN_TAINT() and added a generic 
definition of __WARN()
In the begining I thought the __WARN() call in 
arch/powerpc/include/bug.h was forgotten, but looking into the commit in 
full, it looks like it was intentional to make __WARN() generic and have 
arches use it.

Then commit 19d436268dde ("debug: Add _ONCE() logic to report_bug()") 
replaced __WARN_TAINT() by __WARN_FLAGS().

So by changing the generic __WARN() you are impacting all users include 
those using 'trap' like instruction in order to avoid function calls.

What is to be done for getting back a clean code which doesn't call 
printk() on the hot path ?

Thanks,
Christophe