linux-kernel - Re: [PATCH v3] export.h: reduce __ksymtab_strings string duplication by using "MS" section flags

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:   Thu, 12 Dec 2019 11:36:07 +0100
From:   Jessica Yu <jeyu@...nel.org>
To:     Masahiro Yamada <masahiroy@...nel.org>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Matthias Maennich <maennich@...gle.com>,
        Rasmus Villemoes <linux@...musvillemoes.dk>,
        Arnd Bergmann <arnd@...db.de>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Subject: Re: [PATCH v3] export.h: reduce __ksymtab_strings string duplication
 by using "MS" section flags

+++ Masahiro Yamada [12/12/19 15:22 +0900]:
>On Fri, Dec 6, 2019 at 9:41 PM Jessica Yu <jeyu@...nel.org> wrote:
>>
>> Commit c3a6cf19e695 ("export: avoid code duplication in
>> include/linux/export.h") refactors export.h quite nicely, but introduces
>> a slight increase in memory usage due to using the empty string ""
>> instead of NULL to indicate that an exported symbol has no namespace. As
>> mentioned in that commit, this meant an increase of 1 byte per exported
>> symbol without a namespace. For example, if a kernel configuration has
>> about 10k exported symbols, this would mean that the size of
>> __ksymtab_strings would increase by roughly 10kB.
>>
>> We can alleviate this situation by utilizing the SHF_MERGE and
>> SHF_STRING section flags. SHF_MERGE|SHF_STRING indicate to the linker
>> that the data in the section are null-terminated strings that can be
>> merged to eliminate duplication. More specifically, from the binutils
>> documentation - "for sections with both M and S, a string which is a
>> suffix of a larger string is considered a duplicate. Thus "def" will be
>> merged with "abcdef"; A reference to the first "def" will be changed to
>> a reference to "abcdef"+3". Thus, all the empty strings would be merged
>> as well as any strings that can be merged according to the cited method
>> above. For example, "memset" and "__memset" would be merged to just
>> "__memset" in __ksymtab_strings.
>>
>> As of v5.4-rc5, the following statistics were gathered with x86
>> defconfig with approximately 10.7k exported symbols.
>>
>> Size of __ksymtab_strings in vmlinux:
>> -------------------------------------
>> v5.4-rc5: 213834 bytes
>> v5.4-rc5 with commit c3a6cf19e695: 224455 bytes
>> v5.4-rc5 with this patch: 205759 bytes
>>
>> So, we already see memory savings of ~8kB compared to vanilla -rc5 and
>> savings of nearly 18.7kB compared to -rc5 with commit c3a6cf19e695 on top.
>>
>> Unfortunately, as of this writing, strings will not get deduplicated for
>> kernel modules, as ld does not do the deduplication for
>> SHF_MERGE|SHF_STRINGS sections for relocatable files (ld -r), which
>> kernel modules are. A patch for ld is currently being worked on to
>> hopefully allow for string deduplication in relocatable files in the
>> future.
>>
>> Suggested-by: Rasmus Villemoes <linux@...musvillemoes.dk>
>> Signed-off-by: Jessica Yu <jeyu@...nel.org>
>> ---
>> v3:
>>   - remove __KSTRTAB_ENTRY macros in favor of just putting the asm directly
>>     in ___EXPORT_SYMBOL
>>   - Document more clearly what the ___EXPORT_SYMBOL macro does
>>
>>  include/asm-generic/export.h |  8 +++++---
>>  include/linux/export.h       | 27 ++++++++++++++++++++-------
>>  2 files changed, 25 insertions(+), 10 deletions(-)
>>
>> diff --git a/include/asm-generic/export.h b/include/asm-generic/export.h
>> index fa577978fbbd..23bc98e97a66 100644
>> --- a/include/asm-generic/export.h
>> +++ b/include/asm-generic/export.h
>> @@ -26,9 +26,11 @@
>>  .endm
>>
>>  /*
>> - * note on .section use: @progbits vs %progbits nastiness doesn't matter,
>> - * since we immediately emit into those sections anyway.
>> + * note on .section use: we specify progbits since usage of the "M" (SHF_MERGE)
>> + * section flag requires it. Use '%progbits' instead of '@...gbits' since the
>> + * former apparently works on all arches according to the binutils source.
>>   */
>> +
>>  .macro ___EXPORT_SYMBOL name,val,sec
>>  #ifdef CONFIG_MODULES
>>         .globl __ksymtab_\name
>> @@ -37,7 +39,7 @@
>>  __ksymtab_\name:
>>         __put \val, __kstrtab_\name
>>         .previous
>> -       .section __ksymtab_strings,"a"
>> +       .section __ksymtab_strings,"aMS",%progbits,1
>>  __kstrtab_\name:
>>         .asciz "\name"
>>         .previous
>> diff --git a/include/linux/export.h b/include/linux/export.h
>> index 201262793369..18dcdcd118e7 100644
>> --- a/include/linux/export.h
>> +++ b/include/linux/export.h
>> @@ -81,16 +81,29 @@ struct kernel_symbol {
>>
>>  #else
>>
>> -/* For every exported symbol, place a struct in the __ksymtab section */
>> +/*
>> + * For every exported symbol, do the following:
>
>Just a nit.
>
>You mention "an entry" twice
>to talk about different classes of structures.
>
>It might be better to be explicit about "which entry".
>
>
>> + *
>> + * - If applicable, place an entry in the __kcrctab section.
>
>  "place a CRC entry in the __kcrctab section"
>might be clearer ?
>
>
>> + * - Put the name of the symbol and namespace (empty string "" for none) in
>> + *   __ksymtab_strings.
>> + * - Place an entry in the __ksymtab section.
>
> "Place a struct kernel_symbol entry in __ksymtab section"   ?
>might be clearer ?
>
>
>Other than that:
>Reviewed-by: Masahiro Yamada <masahiroy@...nel.org>

Yeah, that is much clearer, will resend. Thanks! 

Jessica