| 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
| ||
|
Date: Mon, 14 Feb 2022 14:57:51 +0000
From: Christophe Leroy <christophe.leroy@...roup.eu>
To: David Laight <David.Laight@...LAB.COM>,
'Anshuman Khandual' <anshuman.khandual@....com>,
Andrew Morton <akpm@...ux-foundation.org>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-mm@...ck.org" <linux-mm@...ck.org>
Subject: Re: [PATCH v6] mm: Uninline copy_overflow()
Le 14/02/2022 à 15:00, David Laight a écrit :
> From: Christophe Leroy
>> Sent: 14 February 2022 13:21
>>
>> Le 14/02/2022 à 12:31, David Laight a écrit :
>>> From: Anshuman Khandual
>>>> Sent: 14 February 2022 09:54
>>> ...
>>>>> With -Winline, GCC tells:
>>>>>
>>>>> /include/linux/thread_info.h:212:20: warning: inlining failed in call to 'copy_overflow': call
>>>> is unlikely and code size would grow [-Winline]
>>>>>
>>>>> copy_overflow() is a non conditional warning called by
>>>>> check_copy_size() on an error path.
>>>>>
>>>>> check_copy_size() have to remain inlined in order to benefit
>>>>> from constant folding, but copy_overflow() is not worth inlining.
>>>>>
>>>>> Uninline the warning when CONFIG_BUG is selected.
>>>>>
>>>>> When CONFIG_BUG is not selected, WARN() does nothing so skip it.
>>>>>
>>>>> This reduces the size of vmlinux by almost 4kbytes.
>>>>
>>>
>>>>> +void __copy_overflow(int size, unsigned long count);
>>>>> +
>>>>> static inline void copy_overflow(int size, unsigned long count)
>>>>> {
>>>>> - WARN(1, "Buffer overflow detected (%d < %lu)!\n", size, count);
>>>>> + if (IS_ENABLED(CONFIG_BUG))
>>>>> + __copy_overflow(size, count);
>>>>> }
>>>
>>>> Just wondering, is this the only such scenario which results in
>>>> an avoidable bloated vmlinux image ?
>>>
>>> The more interesting question is whether the call to __copy_overflow()
>>> is actually significantly smaller than the one to WARN()?
>>> And if so why.
>>>
>> unsigned long tst_copy_to_user(void __user *to, unsigned long n)
>> {
>> return copy_to_user(to, &jiffies_64, n);
>> }
>>
>> With the patch:
>>
>> 00003c78 <tst_copy_to_user>:
>> 3c78: 28 04 00 08 cmplwi r4,8
>> 3c7c: 7c 85 23 78 mr r5,r4
>> 3c80: 41 81 00 10 bgt 3c90 <tst_copy_to_user+0x18>
>> 3c84: 3c 80 00 00 lis r4,0
>> 3c86: R_PPC_ADDR16_HA jiffies_64
>> 3c88: 38 84 00 00 addi r4,r4,0
>> 3c8a: R_PPC_ADDR16_LO jiffies_64
>> 3c8c: 48 00 00 00 b 3c8c <tst_copy_to_user+0x14>
>> 3c8c: R_PPC_REL24 _copy_to_user
>>
>> 3c90: 94 21 ff f0 stwu r1,-16(r1)
>> 3c94: 7c 08 02 a6 mflr r0
>> 3c98: 38 60 00 08 li r3,8
>> 3c9c: 90 01 00 14 stw r0,20(r1)
>> 3ca0: 90 81 00 08 stw r4,8(r1)
>> 3ca4: 48 00 00 01 bl 3ca4 <tst_copy_to_user+0x2c>
>> 3ca4: R_PPC_REL24 __copy_overflow
>> 3ca8: 80 a1 00 08 lwz r5,8(r1)
>> 3cac: 80 01 00 14 lwz r0,20(r1)
>> 3cb0: 7c a3 2b 78 mr r3,r5
>> 3cb4: 7c 08 03 a6 mtlr r0
>> 3cb8: 38 21 00 10 addi r1,r1,16
>> 3cbc: 4e 80 00 20 blr
>>
>>
>> Without the patch:
>>
>> 00003c88 <tst_copy_to_user>:
>> 3c88: 28 04 00 08 cmplwi r4,8
>> 3c8c: 7c 85 23 78 mr r5,r4
>> 3c90: 41 81 00 10 bgt 3ca0 <tst_copy_to_user+0x18>
>> 3c94: 3c 80 00 00 lis r4,0
>> 3c96: R_PPC_ADDR16_HA jiffies_64
>> 3c98: 38 84 00 00 addi r4,r4,0
>> 3c9a: R_PPC_ADDR16_LO jiffies_64
>> 3c9c: 48 00 00 00 b 3c9c <tst_copy_to_user+0x14>
>> 3c9c: R_PPC_REL24 _copy_to_user
>>
>> 3ca0: 94 21 ff f0 stwu r1,-16(r1)
>> 3ca4: 3c 60 00 00 lis r3,0
>> 3ca6: R_PPC_ADDR16_HA .rodata.str1.4+0x30
>> 3ca8: 90 81 00 08 stw r4,8(r1)
>> 3cac: 7c 08 02 a6 mflr r0
>> 3cb0: 38 63 00 00 addi r3,r3,0
>> 3cb2: R_PPC_ADDR16_LO .rodata.str1.4+0x30
>> 3cb4: 38 80 00 08 li r4,8
>> 3cb8: 90 01 00 14 stw r0,20(r1)
>> 3cbc: 48 00 00 01 bl 3cbc <tst_copy_to_user+0x34>
>> 3cbc: R_PPC_REL24 __warn_printk
>> 3cc0: 80 a1 00 08 lwz r5,8(r1)
>> 3cc4: 0f e0 00 00 twui r0,0
>> 3cc8: 80 01 00 14 lwz r0,20(r1)
>> 3ccc: 7c a3 2b 78 mr r3,r5
>> 3cd0: 7c 08 03 a6 mtlr r0
>> 3cd4: 38 21 00 10 addi r1,r1,16
>> 3cd8: 4e 80 00 20 blr
>
> I make that 3 extra instructions.
> Two are needed to load the format string.
> Not sure why the third gets added.
Third instruction is 'twui', to 'trap' and get the warning oops.
>
> Not really significant in the 12-15 the error call actually takes.
> Although a lot of those are just generating the stack frame
> in order to call the error function - and wouldn't be there in
> a less trivial example.
Yes, after looking once more, maybe making it __always_inline would be
enough.
The starting point was that I got almost 50 times copy_overflow() in my
vmlinux, each having its own format string as well.
So my patch reduced vmlinux size by 3908 bytes.
But with __always_inline I get a reduction by 3560 which is almost the same.
So if you prefer, I can just make copy_overflow() __always_inline and voila.
>
> More interesting would be changing copy_overflow() to return the size.
> So copy_to_user() becomes:
>
> if (size_valid())
> return _copy_to_user();
> return copy_overflow()
Yes that's something to try, allthough it means changing all callers of
check_copy_size()
Christophe
Powered by blists - more mailing lists