linux-kernel - Re: [PATCH 0/5] add support for relative references in special sections

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: <CAKv+Gu9MBz68SMpQFbw33qvOwbGYYVO=5HHuLESUtsa0qsBP5g@mail.gmail.com>
Date:   Fri, 18 Aug 2017 07:12:42 +0100
From:   Ard Biesheuvel <ard.biesheuvel@...aro.org>
To:     Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>
Cc:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-arch@...r.kernel.org" <linux-arch@...r.kernel.org>,
        "H. Peter Anvin" <hpa@...or.com>, Arnd Bergmann <arnd@...db.de>,
        Heiko Carstens <heiko.carstens@...ibm.com>,
        Kees Cook <keescook@...omium.org>,
        Will Deacon <will.deacon@....com>,
        Michael Ellerman <mpe@...erman.id.au>,
        Thomas Garnier <thgarnie@...gle.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        "Serge E. Hallyn" <serge@...lyn.com>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Paul Mackerras <paulus@...ba.org>,
        Catalin Marinas <catalin.marinas@....com>,
        Petr Mladek <pmladek@...e.com>, Ingo Molnar <mingo@...hat.com>,
        James Morris <james.l.morris@...cle.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Nicolas Pitre <nico@...aro.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Martin Schwidefsky <schwidefsky@...ibm.com>,
        Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
        Jessica Yu <jeyu@...nel.org>
Subject: Re: [PATCH 0/5] add support for relative references in special sections

Hi Sergey,

Thanks for taking a look

On 18 August 2017 at 06:56, Sergey Senozhatsky
<sergey.senozhatsky.work@...il.com> wrote:
> On (08/14/17 11:52), Ard Biesheuvel wrote:
>> This adds support for emitting special sections such as initcall arrays,
>> PCI fixups and tracepoints as relative references rather than absolute
>> references. This reduces the size by 50% on 64-bit architectures, but
>> more importantly, it removes the need for carrying relocation metadata
>> for these sections in relocatables kernels (e.g., for KASLR) that need
>> to fix up these absolute references at boot time. On arm64, this reduces
>> the vmlinux footprint of such a reference by 8x (8 byte absolute reference
>> + 24 byte RELA entry vs 4 byte relative reference)
> [..]
>
> a side note,
> checkpatch complaints quite a lot.
>

Yeah, fair point. Many of them are debatable or completely bogus, though:

ERROR: "foo * bar" should be "foo *bar"
#114: FILE: include/linux/compiler.h:600:
+ static void * __attribute__((section(".discard.text"), used)) \

I think it is rather common to keep whitespace between * and what
follows if it is not the identifier.

ERROR: Macros with complex values should be enclosed in parentheses
#147: FILE: include/linux/export.h:64:
+#define __KSYMTAB_ENTRY(sym, sec) \
+ __ADDRESSABLE(sym) \
+ asm(" .section \"___ksymtab" sec "+" #sym "\", \"a\" \n" \
+    " .balign 8 \n" \
+    VMLINUX_SYMBOL_STR(__ksymtab_##sym) ": \n" \
+    " .long " VMLINUX_SYMBOL_STR(sym) "- . \n" \
+    " .long " VMLINUX_SYMBOL_STR(__kstrtab_##sym) "- .\n" \
+    " .previous \n")

WARNING: do not add new typedefs
#29: FILE: include/linux/init.h:114:
+typedef signed int initcall_entry_t;

WARNING: do not add new typedefs
#36: FILE: include/linux/init.h:121:
+typedef initcall_t initcall_entry_t;

This is a typedef that accompanies the existing typedef for
initcall_t, and it is the only way to parameterise this code without a
ton of changes to duplicate all extern declarations of initcall
arrays.

ERROR: Macros with multiple statements should be enclosed in a do - while loop
#55: FILE: include/linux/init.h:181:
+#define ___define_initcall(fn, id, __sec) \
+ __ADDRESSABLE(fn) \
+ asm(".section \"" #__sec ".init\", \"a\" \n" \
+ "__initcall_" #fn #id ": \n" \
+    ".long " VMLINUX_SYMBOL_STR(fn) " - . \n" \
+    ".previous \n");

This one is bogus. This macro is only used in file scope.

WARNING: externs should be avoided in .c files
#109: FILE: init/main.c:837:
+extern initcall_entry_t __initcall0_start[];

This changes the type of the existing initcall array declarations, and
I don't think moving them to a .h file for no other reason than to
appease checkpatch is pointless.

WARNING: unnecessary whitespace before a quoted newline
#63: FILE: include/linux/pci.h:1759:
+ asm(".section " #sec ", \"a\" \n" \

WARNING: unnecessary whitespace before a quoted newline
#64: FILE: include/linux/pci.h:1760:
+    ".balign 16 \n" \

WARNING: unnecessary whitespace before a quoted newline
#65: FILE: include/linux/pci.h:1761:
+    ".short " #vendor ", " #device " \n" \

These are not newlines that end up as strings in the program, and I
think it makes little sense to make the inline asm less readable by
putting the \n right after the text.

WARNING: braces {} are not necessary for single statement blocks
#83: FILE: kernel/tracepoint.c:515:
+ for (iter = begin; iter < (signed int *)end; iter++) {
+ fct((struct tracepoint *)((unsigned long)iter + *iter), priv);
+ }

This simply mirrors the existing code in the other branch of the if ()

I will clean up the meaningful ones in v2, but please don't expect
this series to be checkpatch clean: it simply doesn't deal with inline
asm very well, and some of this code predates checkpatch by a decade,
and I'd rather not mix up rather tricky functional changes with
checkpatch cleanup duty.

-- 
Ard.