From: Kees Cook <keescook@chromium.org> To: Arvind Sankar <nivedita@alum.mit.edu> Cc: "Thomas Gleixner" <tglx@linutronix.de>, "Will Deacon" <will@kernel.org>, "Nick Desaulniers" <ndesaulniers@google.com>, "Jian Cai" <jiancai@google.com>, "Fāng-ruì Sòng" <maskray@google.com>, "Luis Lozano" <llozano@google.com>, "Manoj Gupta" <manojgupta@google.com>, stable@vger.kernel.org, "Catalin Marinas" <catalin.marinas@arm.com>, "Mark Rutland" <mark.rutland@arm.com>, "Ard Biesheuvel" <ardb@kernel.org>, "Peter Collingbourne" <pcc@google.com>, "James Morse" <james.morse@arm.com>, "Borislav Petkov" <bp@suse.de>, "Ingo Molnar" <mingo@redhat.com>, "Russell King" <linux@armlinux.org.uk>, "Masahiro Yamada" <masahiroy@kernel.org>, "Nathan Chancellor" <natechancellor@gmail.com>, "Arnd Bergmann" <arnd@arndb.de>, x86@kernel.org, clang-built-linux@googlegroups.com, linux-arch@vger.kernel.org, linux-efi@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, "Andi Kleen" <ak@linux.intel.com>, "Michal Marek" <michal.lkml@markovi.net>, "Kristen Carlson Accardi" <kristen@linux.intel.com> Subject: Re: [PATCH v5 13/36] vmlinux.lds.h: add PGO and AutoFDO input sections Date: Fri, 31 Jul 2020 23:18:02 -0700 Message-ID: <202007312237.4F385EB3@keescook> (raw) In-Reply-To: <20200801035128.GB2800311@rani.riverdale.lan> On Fri, Jul 31, 2020 at 11:51:28PM -0400, Arvind Sankar wrote: > On Fri, Jul 31, 2020 at 04:07:57PM -0700, Kees Cook wrote: > > From: Nick Desaulniers <ndesaulniers@google.com> > > > > Basically, consider .text.{hot|unlikely|unknown}.* part of .text, too. > > > > When compiling with profiling information (collected via PGO > > instrumentations or AutoFDO sampling), Clang will separate code into > > .text.hot, .text.unlikely, or .text.unknown sections based on profiling > > information. After D79600 (clang-11), these sections will have a > > trailing `.` suffix, ie. .text.hot., .text.unlikely., .text.unknown.. > > > > When using -ffunction-sections together with profiling infomation, > > either explicitly (FGKASLR) or implicitly (LTO), code may be placed in > > sections following the convention: > > .text.hot.<foo>, .text.unlikely.<bar>, .text.unknown.<baz> > > where <foo>, <bar>, and <baz> are functions. (This produces one section > > per function; we generally try to merge these all back via linker script > > so that we don't have 50k sections). > > > > For the above cases, we need to teach our linker scripts that such > > sections might exist and that we'd explicitly like them grouped > > together, otherwise we can wind up with code outside of the > > _stext/_etext boundaries that might not be mapped properly for some > > architectures, resulting in boot failures. > > > > If the linker script is not told about possible input sections, then > > where the section is placed as output is a heuristic-laiden mess that's > > non-portable between linkers (ie. BFD and LLD), and has resulted in many > > hard to debug bugs. Kees Cook is working on cleaning this up by adding > > --orphan-handling=warn linker flag used in ARCH=powerpc to additional > > architectures. In the case of linker scripts, borrowing from the Zen of > > Python: explicit is better than implicit. > > > > Also, ld.bfd's internal linker script considers .text.hot AND > > .text.hot.* to be part of .text, as well as .text.unlikely and > > .text.unlikely.*. I didn't see support for .text.unknown.*, and didn't > > see Clang producing such code in our kernel builds, but I see code in > > LLVM that can produce such section names if profiling information is > > missing. That may point to a larger issue with generating or collecting > > profiles, but I would much rather be safe and explicit than have to > > debug yet another issue related to orphan section placement. > > > > Reported-by: Jian Cai <jiancai@google.com> > > Suggested-by: Fāng-ruì Sòng <maskray@google.com> > > Tested-by: Luis Lozano <llozano@google.com> > > Tested-by: Manoj Gupta <manojgupta@google.com> > > Acked-by: Kees Cook <keescook@chromium.org> > > Cc: stable@vger.kernel.org > > Link: https://sourceware.org/git/?p=binutils-gdb.git;a=commitdiff;h=add44f8d5c5c05e08b11e033127a744d61c26aee > > Link: https://sourceware.org/git/?p=binutils-gdb.git;a=commitdiff;h=1de778ed23ce7492c523d5850c6c6dbb34152655 > > Link: https://reviews.llvm.org/D79600 > > Link: https://bugs.chromium.org/p/chromium/issues/detail?id=1084760 > > Debugged-by: Luis Lozano <llozano@google.com> > > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> > > Signed-off-by: Kees Cook <keescook@chromium.org> > > --- > > include/asm-generic/vmlinux.lds.h | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h > > index 2593957f6e8b..af5211ca857c 100644 > > --- a/include/asm-generic/vmlinux.lds.h > > +++ b/include/asm-generic/vmlinux.lds.h > > @@ -561,7 +561,10 @@ > > */ > > #define TEXT_TEXT \ > > ALIGN_FUNCTION(); \ > > - *(.text.hot TEXT_MAIN .text.fixup .text.unlikely) \ > > + *(.text.hot .text.hot.*) \ > > + *(TEXT_MAIN .text.fixup) \ > > + *(.text.unlikely .text.unlikely.*) \ > > + *(.text.unknown .text.unknown.*) \ > > NOINSTR_TEXT \ > > *(.text..refcount) \ > > *(.ref.text) \ > > -- > > 2.25.1 > > > > This also changes the ordering to place all hot resp unlikely sections separate > from other text, while currently it places the hot/unlikely bits of each file > together with the rest of the code in that file. That seems like a reasonable Oh, hmm, yes, we aren't explicitly using SORT() here. Does that mean the input sections were entirely be ordered in compilation unit link order, even in the case of orphan sections? (And I think either way, the answer isn't the same between bfd and lld.) I actually thought the like-named input sections were collected together first with lld, but bfd strictly appended to the output section. I guess it's time for me to stare at -M output from ld... Regardless, this patch is attempting to fix the problem where bfd and lld lay out the orphans differently (as mentioned above, lld seems to sort them in a way that is not strictly appended, and bfd seems to sort them strictly appended). In the case of being appended to the .text output section, this would cause boot failures due to _etext not covering the resulting sections (which this[1] also encountered and fixed to be more robust for such appended collection -- that series actually _depends_ on orphan handling doing the appending, because there is no current way to map wildcard input sections to their own separate output sections). > change and should be mentioned in the commit message. > > However, the history of their being together comes from > > 9bebe9e5b0f3 ("kbuild: Fix .text.unlikely placement") > > which seems to indicate there was some problem with having them separated out, > although I don't quite understand what the issue was from the commit message. Looking at this again, I actually wonder if we have bigger issues here with dead code elimination: #ifdef CONFIG_LD_DEAD_CODE_DATA_ELIMINATION #define TEXT_MAIN .text .text.[0-9a-zA-Z_]* ... that would catch: .text.hot .text.fixup .text.unlikely and .text.unknown but not .text.hot.*, etc (i.e. the third dot isn't matched, which is, I assume, why Clang switched to adding a trailing dot). However, this patch lists .text.hot .text.hot.* first, so they'd get pulled to the front correctly, but the trailing ones (with 2 dots) would not, since they'd match the TEXT_MAIN wildcard first. (This problem actually existed before this patch too, and is not the fault of 9bebe9e5b0f3, but rather the addition of TEXT_MAIN, which could potentially match .text.unlikely and .text.fixup) Unless I'm totally wrong and the bfd docs don't match the behavior? e.g. if I have a link order of ".foo.before", ".foo.after", and ".foo.middle", and this rule: .foo : { *(.foo.before .foo.* .foo.after) } do I get this (first match): .foo.before .foo.after .foo.middle or (most specific match): .foo.before .foo.middle .foo.after ? As I said, now that I'm able to better articulate these questions, I'll go get answers from -M output. :) Perhaps we need to fix TEXT_MAIN not TEXT_TEXT? TEXT_TEXT is for collecting .text, .text.[^\.]* and *.text, where, effectively, .text and .text[^\.]* are defined by TEXT_MAIN. i.e. adding 3-dot "text" input sections needs to likely be included in TEXT_MAIN Anyway, I'll keep looking at this... (In the meantime, perhaps we can take Arvind's series, and the earlier portions of the orphan series where asm-generic/vmlinux.lds.h and other things are cleaned up...) -Kees [1] https://lore.kernel.org/lkml/20200717170008.5949-6-kristen@linux.intel.com/ -- Kees Cook
next prev parent reply index Thread overview: 70+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-07-31 23:07 [PATCH v5 00/36] Warn on orphan section placement Kees Cook 2020-07-31 23:07 ` [PATCH v5 01/36] x86/boot/compressed: Move .got.plt entries out of the .got section Kees Cook 2020-08-14 15:23 ` [tip: x86/boot] " tip-bot2 for Ard Biesheuvel 2020-07-31 23:07 ` [PATCH v5 02/36] x86/boot/compressed: Force hidden visibility for all symbol references Kees Cook 2020-08-14 15:23 ` [tip: x86/boot] " tip-bot2 for Ard Biesheuvel 2020-07-31 23:07 ` [PATCH v5 03/36] x86/boot/compressed: Get rid of GOT fixup code Kees Cook 2020-08-14 15:23 ` [tip: x86/boot] " tip-bot2 for Ard Biesheuvel 2020-07-31 23:07 ` [PATCH v5 04/36] x86/boot: Add .text.* to setup.ld Kees Cook 2020-08-14 15:23 ` [tip: x86/boot] " tip-bot2 for Arvind Sankar 2020-07-31 23:07 ` [PATCH v5 05/36] x86/boot: Remove run-time relocations from .head.text code Kees Cook 2020-07-31 23:42 ` Nick Desaulniers 2020-08-14 15:23 ` [tip: x86/boot] " tip-bot2 for Arvind Sankar 2020-07-31 23:07 ` [PATCH v5 06/36] x86/boot: Remove run-time relocations from head_{32,64}.S Kees Cook 2020-08-07 18:12 ` Nick Desaulniers 2020-08-07 20:20 ` Arvind Sankar 2020-08-14 15:23 ` [tip: x86/boot] " tip-bot2 for Arvind Sankar 2020-07-31 23:07 ` [PATCH v5 07/36] x86/boot: Check that there are no run-time relocations Kees Cook 2020-08-14 15:23 ` [tip: x86/boot] " tip-bot2 for Arvind Sankar 2020-07-31 23:07 ` [PATCH v5 08/36] vmlinux.lds.h: Create COMMON_DISCARDS Kees Cook 2020-07-31 23:07 ` [PATCH v5 09/36] vmlinux.lds.h: Add .gnu.version* to COMMON_DISCARDS Kees Cook 2020-07-31 23:07 ` [PATCH v5 10/36] vmlinux.lds.h: Avoid KASAN and KCSAN's unwanted sections Kees Cook 2020-07-31 23:07 ` [PATCH v5 11/36] vmlinux.lds.h: Split ELF_DETAILS from STABS_DEBUG Kees Cook 2020-07-31 23:07 ` [PATCH v5 12/36] vmlinux.lds.h: Add .symtab, .strtab, and .shstrtab to ELF_DETAILS Kees Cook 2020-07-31 23:07 ` [PATCH v5 13/36] vmlinux.lds.h: add PGO and AutoFDO input sections Kees Cook 2020-08-01 3:51 ` Arvind Sankar 2020-08-01 6:18 ` Kees Cook [this message] 2020-08-01 17:27 ` Arvind Sankar 2020-08-03 19:05 ` Andi Kleen 2020-08-03 20:15 ` Arvind Sankar 2020-08-04 1:19 ` Fāng-ruì Sòng 2020-08-04 4:45 ` Andi Kleen 2020-08-04 5:32 ` Fāng-ruì Sòng 2020-08-04 16:06 ` Arvind Sankar 2020-08-21 19:18 ` Kees Cook 2020-07-31 23:07 ` [PATCH v5 14/36] efi/libstub: Disable -mbranch-protection Kees Cook 2020-07-31 23:07 ` [PATCH v5 15/36] arm64/mm: Remove needless section quotes Kees Cook 2020-07-31 23:08 ` [PATCH v5 16/36] arm64/kernel: Remove needless Call Frame Information annotations Kees Cook 2020-07-31 23:08 ` [PATCH v5 17/36] arm64/build: Remove .eh_frame* sections due to unwind tables Kees Cook 2020-07-31 23:08 ` [PATCH v5 18/36] arm64/build: Use common DISCARDS in linker script Kees Cook 2020-07-31 23:08 ` [PATCH v5 19/36] arm64/build: Add missing DWARF sections Kees Cook 2020-07-31 23:08 ` [PATCH v5 20/36] arm64/build: Assert for unwanted sections Kees Cook 2020-07-31 23:08 ` [PATCH v5 21/36] arm64/build: Warn on orphan section placement Kees Cook 2020-07-31 23:08 ` [PATCH v5 22/36] arm/build: Refactor linker script headers Kees Cook 2020-07-31 23:08 ` [PATCH v5 23/36] arm/build: Explicitly keep .ARM.attributes sections Kees Cook 2020-08-03 19:02 ` Nick Desaulniers 2020-08-17 22:06 ` Fangrui Song 2020-07-31 23:08 ` [PATCH v5 24/36] arm/build: Add missing sections Kees Cook 2020-07-31 23:08 ` [PATCH v5 25/36] arm/build: Warn on orphan section placement Kees Cook 2020-07-31 23:08 ` [PATCH v5 26/36] arm/boot: Handle all sections explicitly Kees Cook 2020-07-31 23:08 ` [PATCH v5 27/36] arm/boot: Warn on orphan section placement Kees Cook 2020-07-31 23:08 ` [PATCH v5 28/36] x86/asm: Avoid generating unused kprobe sections Kees Cook 2020-07-31 23:08 ` [PATCH v5 29/36] x86/build: Enforce an empty .got.plt section Kees Cook 2020-08-01 2:12 ` Arvind Sankar 2020-08-01 5:32 ` Kees Cook 2020-08-21 17:49 ` Kees Cook 2020-07-31 23:08 ` [PATCH v5 30/36] x86/build: Assert for unwanted sections Kees Cook 2020-07-31 23:08 ` [PATCH v5 31/36] x86/build: Warn on orphan section placement Kees Cook 2020-07-31 23:08 ` [PATCH v5 32/36] x86/boot/compressed: Reorganize zero-size section asserts Kees Cook 2020-08-01 1:47 ` Arvind Sankar 2020-08-01 2:53 ` Arvind Sankar 2020-08-01 5:36 ` Kees Cook 2020-08-01 17:12 ` Arvind Sankar 2020-08-21 18:24 ` Kees Cook 2020-08-01 5:35 ` Kees Cook 2020-08-01 17:00 ` Arvind Sankar 2020-08-21 18:19 ` Kees Cook 2020-07-31 23:08 ` [PATCH v5 33/36] x86/boot/compressed: Remove, discard, or assert for unwanted sections Kees Cook 2020-07-31 23:08 ` [PATCH v5 34/36] x86/boot/compressed: Add missing debugging sections to output Kees Cook 2020-07-31 23:08 ` [PATCH v5 35/36] x86/boot/compressed: Warn on orphan section placement Kees Cook 2020-07-31 23:08 ` [PATCH v5 36/36] arm/build: Assert for unwanted sections Kees Cook
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=202007312237.4F385EB3@keescook \ --to=keescook@chromium.org \ --cc=ak@linux.intel.com \ --cc=ardb@kernel.org \ --cc=arnd@arndb.de \ --cc=bp@suse.de \ --cc=catalin.marinas@arm.com \ --cc=clang-built-linux@googlegroups.com \ --cc=james.morse@arm.com \ --cc=jiancai@google.com \ --cc=kristen@linux.intel.com \ --cc=linux-arch@vger.kernel.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-efi@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux@armlinux.org.uk \ --cc=llozano@google.com \ --cc=manojgupta@google.com \ --cc=mark.rutland@arm.com \ --cc=masahiroy@kernel.org \ --cc=maskray@google.com \ --cc=michal.lkml@markovi.net \ --cc=mingo@redhat.com \ --cc=natechancellor@gmail.com \ --cc=ndesaulniers@google.com \ --cc=nivedita@alum.mit.edu \ --cc=pcc@google.com \ --cc=stable@vger.kernel.org \ --cc=tglx@linutronix.de \ --cc=will@kernel.org \ --cc=x86@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
LKML Archive on lore.kernel.org Archives are clonable: git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git git clone --mirror https://lore.kernel.org/lkml/8 lkml/git/8.git git clone --mirror https://lore.kernel.org/lkml/9 lkml/git/9.git # If you have public-inbox 1.1+ installed, you may # initialize and index your mirror using the following commands: public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \ linux-kernel@vger.kernel.org public-inbox-index lkml Example config snippet for mirrors Newsgroup available over NNTP: nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel AGPL code for this site: git clone https://public-inbox.org/public-inbox.git