linux-kernel - Re: [PATCH v6] mm: Uninline copy

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: <3fb6ecb8-5c40-7ff4-4a55-7ea5ffad9962@csgroup.eu>
Date:   Mon, 14 Feb 2022 15:31:56 +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 à 16:10, David Laight a écrit :
> From: Christophe Leroy
>> Sent: 14 February 2022 14:58
> ...
>>> 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.
> 
> I wondered what that did :-)
> Although you really want the -- cut here -- to contain the pr_warn().
> Doesn't WARN() do that for you?

I remember some discussion about that in the past. Will dig into it 
tomorrow.

> 
> I was looking at that last week because the 'scheduling while atomic'
> trace is "BUG: xxxx" but doesn't have the '--- cut here --" marker.
> 
>>> 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.
> 
> Didn't the linker merge the format strings?
> They ought to end up in strdata.ro.1 (or whatever it is called)
> and the linker merge the references.
> 
>> 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.
> 
> I suspect #define __inline __always_inline is the way to go.

That was the case until 889b3c1245de ("compiler: remove 
CONFIG_OPTIMIZE_INLINING entirely")


> Probable along with -Winline.
> 
> The kernel shouldn't have inline sprinkled where it isn't needed.
> 
>>> 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().
> 
> You could use a differently named function so they can be changed in stages.
> 
> 	David
> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)