| Message ID | 20210226140352.3477860-1-arnd@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: vmlinux.lds.S: keep .entry.tramp.text section | expand |
On Fri, Feb 26, 2021 at 03:03:39PM +0100, Arnd Bergmann wrote: > From: Arnd Bergmann <arnd@arndb.de> > > When building with CONFIG_LD_DEAD_CODE_DATA_ELIMINATION, > I sometimes see an assertion > > ld.lld: error: Entry trampoline text too big Heh, "too big" seems a weird report for having it discarded. :) Any idea on this Fangrui? ( I see this is https://github.com/ClangBuiltLinux/linux/issues/1311 ) > > This happens when any reference to the trampoline is discarded at link > time. Marking the section as KEEP() avoids the assertion, but I have > not figured out whether this is the correct solution for the underlying > problem. > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> As a work-around, it seems fine to me. Reviewed-by: Kees Cook <keescook@chromium.org> -Kees > --- > arch/arm64/kernel/vmlinux.lds.S | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/arm64/kernel/vmlinux.lds.S b/arch/arm64/kernel/vmlinux.lds.S > index 926cdb597a45..c5ee9d5842db 100644 > --- a/arch/arm64/kernel/vmlinux.lds.S > +++ b/arch/arm64/kernel/vmlinux.lds.S > @@ -96,7 +96,7 @@ jiffies = jiffies_64; > #define TRAMP_TEXT \ > . = ALIGN(PAGE_SIZE); \ > __entry_tramp_text_start = .; \ > - *(.entry.tramp.text) \ > + KEEP(*(.entry.tramp.text)) \ > . = ALIGN(PAGE_SIZE); \ > __entry_tramp_text_end = .; > #else > -- > 2.29.2 >
On 2021-02-26, Kees Cook wrote: >On Fri, Feb 26, 2021 at 03:03:39PM +0100, Arnd Bergmann wrote: >> From: Arnd Bergmann <arnd@arndb.de> >> >> When building with CONFIG_LD_DEAD_CODE_DATA_ELIMINATION, >> I sometimes see an assertion >> >> ld.lld: error: Entry trampoline text too big > >Heh, "too big" seems a weird report for having it discarded. :) > >Any idea on this Fangrui? > >( I see this is https://github.com/ClangBuiltLinux/linux/issues/1311 ) This diagnostic is from an ASSERT in arch/arm64/kernel/vmlinux.lds ASSERT((__entry_tramp_text_end - __entry_tramp_text_start) == (1 << 16), "Entry trampoline text too big") In our case (aarch64-linux-gnu-ld or LLD, --gc-sections), all the input sections with this name are discarded, so the output section is either absent (GNU ld) or empty (LLD). KEEP makes the sections GC roots, and it is appropriate to use here. However, I worry that many other KEEP keywords in vmlinux.lds are unnecessary: https://lore.kernel.org/linux-arm-kernel/20210226211323.arkvjnr4hifxapqu@google.com/ git log -S KEEP -- include/asm-generic/vmlinux.lds.h => there is quite a bit unjustified usage. Sure, adding KEEP (GC root) is easy and works around problems, but it not good for CONFIG_LD_DEAD_CODE_DATA_ELIMINATION. Reviewed-by: Fangrui Song <maskray@google.com> > >> >> This happens when any reference to the trampoline is discarded at link >> time. Marking the section as KEEP() avoids the assertion, but I have >> not figured out whether this is the correct solution for the underlying >> problem. >> >> Signed-off-by: Arnd Bergmann <arnd@arndb.de> > >As a work-around, it seems fine to me. > >Reviewed-by: Kees Cook <keescook@chromium.org> > >-Kees > >> --- >> arch/arm64/kernel/vmlinux.lds.S | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/arch/arm64/kernel/vmlinux.lds.S b/arch/arm64/kernel/vmlinux.lds.S >> index 926cdb597a45..c5ee9d5842db 100644 >> --- a/arch/arm64/kernel/vmlinux.lds.S >> +++ b/arch/arm64/kernel/vmlinux.lds.S >> @@ -96,7 +96,7 @@ jiffies = jiffies_64; >> #define TRAMP_TEXT \ >> . = ALIGN(PAGE_SIZE); \ >> __entry_tramp_text_start = .; \ >> - *(.entry.tramp.text) \ >> + KEEP(*(.entry.tramp.text)) \ >> . = ALIGN(PAGE_SIZE); \ >> __entry_tramp_text_end = .; >> #else >> -- >> 2.29.2 >> > >-- >Kees Cook
On Fri, Feb 26, 2021 at 08:32:57PM -0800, Fangrui Song wrote: > On 2021-02-26, Kees Cook wrote: > > On Fri, Feb 26, 2021 at 03:03:39PM +0100, Arnd Bergmann wrote: > > > From: Arnd Bergmann <arnd@arndb.de> > > > > > > When building with CONFIG_LD_DEAD_CODE_DATA_ELIMINATION, > > > I sometimes see an assertion > > > > > > ld.lld: error: Entry trampoline text too big > > > > Heh, "too big" seems a weird report for having it discarded. :) > > > > Any idea on this Fangrui? > > > > ( I see this is https://github.com/ClangBuiltLinux/linux/issues/1311 ) > > This diagnostic is from an ASSERT in arch/arm64/kernel/vmlinux.lds > > ASSERT((__entry_tramp_text_end - __entry_tramp_text_start) == (1 << 16), > "Entry trampoline text too big") Can we not change the ASSERT to be <= PAGE_SIZE instead?
On Tue, Mar 16, 2021 at 10:45:32AM +0000, Catalin Marinas wrote: > On Fri, Feb 26, 2021 at 08:32:57PM -0800, Fangrui Song wrote: > > On 2021-02-26, Kees Cook wrote: > > > On Fri, Feb 26, 2021 at 03:03:39PM +0100, Arnd Bergmann wrote: > > > > From: Arnd Bergmann <arnd@arndb.de> > > > > > > > > When building with CONFIG_LD_DEAD_CODE_DATA_ELIMINATION, > > > > I sometimes see an assertion > > > > > > > > ld.lld: error: Entry trampoline text too big > > > > > > Heh, "too big" seems a weird report for having it discarded. :) > > > > > > Any idea on this Fangrui? > > > > > > ( I see this is https://github.com/ClangBuiltLinux/linux/issues/1311 ) > > > > This diagnostic is from an ASSERT in arch/arm64/kernel/vmlinux.lds > > > > ASSERT((__entry_tramp_text_end - __entry_tramp_text_start) == (1 << 16), > > "Entry trampoline text too big") > > Can we not change the ASSERT to be <= PAGE_SIZE instead? Ah, that won't work as I suspect we still need the trampoline section. Arnd, do you know why this section disappears? I did a simple test with defconfig + LD_DEAD_CODE_DATA_ELIMINATION and the trampoline section is still around.
On Tue, Mar 16, 2021 at 5:27 PM Catalin Marinas <catalin.marinas@arm.com> wrote: > > On Tue, Mar 16, 2021 at 10:45:32AM +0000, Catalin Marinas wrote: > > On Fri, Feb 26, 2021 at 08:32:57PM -0800, Fangrui Song wrote: > > > On 2021-02-26, Kees Cook wrote: > > > > On Fri, Feb 26, 2021 at 03:03:39PM +0100, Arnd Bergmann wrote: > > > > > From: Arnd Bergmann <arnd@arndb.de> > > > > > > > > > > When building with CONFIG_LD_DEAD_CODE_DATA_ELIMINATION, > > > > > I sometimes see an assertion > > > > > > > > > > ld.lld: error: Entry trampoline text too big > > > > > > > > Heh, "too big" seems a weird report for having it discarded. :) > > > > > > > > Any idea on this Fangrui? > > > > > > > > ( I see this is https://github.com/ClangBuiltLinux/linux/issues/1311 ) > > > > > > This diagnostic is from an ASSERT in arch/arm64/kernel/vmlinux.lds > > > > > > ASSERT((__entry_tramp_text_end - __entry_tramp_text_start) == (1 << 16), > > > "Entry trampoline text too big") > > > > Can we not change the ASSERT to be <= PAGE_SIZE instead? > > Ah, that won't work as I suspect we still need the trampoline section. > > Arnd, do you know why this section disappears? I did a simple test with > defconfig + LD_DEAD_CODE_DATA_ELIMINATION and the trampoline section is > still around. If I remember correctly, this showed up when CONFIG_ARM_SDE_INTERFACE is disabled, which dropped the only reference into this section. If that doesn't make sense, I can try digging through the old build logs to reproduce the problem. Arnd
On Tue, Mar 16, 2021 at 05:39:27PM +0100, Arnd Bergmann wrote: > On Tue, Mar 16, 2021 at 5:27 PM Catalin Marinas <catalin.marinas@arm.com> wrote: > > On Tue, Mar 16, 2021 at 10:45:32AM +0000, Catalin Marinas wrote: > > > On Fri, Feb 26, 2021 at 08:32:57PM -0800, Fangrui Song wrote: > > > > On 2021-02-26, Kees Cook wrote: > > > > > On Fri, Feb 26, 2021 at 03:03:39PM +0100, Arnd Bergmann wrote: > > > > > > From: Arnd Bergmann <arnd@arndb.de> > > > > > > > > > > > > When building with CONFIG_LD_DEAD_CODE_DATA_ELIMINATION, > > > > > > I sometimes see an assertion > > > > > > > > > > > > ld.lld: error: Entry trampoline text too big > > > > > > > > > > Heh, "too big" seems a weird report for having it discarded. :) > > > > > > > > > > Any idea on this Fangrui? > > > > > > > > > > ( I see this is https://github.com/ClangBuiltLinux/linux/issues/1311 ) > > > > > > > > This diagnostic is from an ASSERT in arch/arm64/kernel/vmlinux.lds > > > > > > > > ASSERT((__entry_tramp_text_end - __entry_tramp_text_start) == (1 << 16), > > > > "Entry trampoline text too big") > > > > > > Can we not change the ASSERT to be <= PAGE_SIZE instead? > > > > Ah, that won't work as I suspect we still need the trampoline section. > > > > Arnd, do you know why this section disappears? I did a simple test with > > defconfig + LD_DEAD_CODE_DATA_ELIMINATION and the trampoline section is > > still around. > > If I remember correctly, this showed up when CONFIG_ARM_SDE_INTERFACE > is disabled, which dropped the only reference into this section. > If that doesn't make sense, I can try digging through the old build logs to > reproduce the problem. I suspected this as well but still worked for me when disabling it. Anyway, I don't think identifying the exact option is necessary. With CONFIG_UNMAP_KERNEL_AT_EL0=y we need this section around even if only __entry_tramp_text_start/end are referenced. In this case we happened to detect this issue because of the ASSERT in vmlinux.lds.S but I wonder what else the linker drops with this dead code elimination that we may not notice (it seems to remove about 500KB from the resulting image in my test). I'll push these two patches to -next for wider coverage before deciding on mainline (though the option may not get much testing as it's hidden behind EXPERT and default n).
On Tue, Mar 16, 2021 at 07:09:23PM +0000, Catalin Marinas wrote: > On Tue, Mar 16, 2021 at 05:39:27PM +0100, Arnd Bergmann wrote: > > On Tue, Mar 16, 2021 at 5:27 PM Catalin Marinas <catalin.marinas@arm.com> wrote: > > > On Tue, Mar 16, 2021 at 10:45:32AM +0000, Catalin Marinas wrote: > > > > On Fri, Feb 26, 2021 at 08:32:57PM -0800, Fangrui Song wrote: > > > > > On 2021-02-26, Kees Cook wrote: > > > > > > On Fri, Feb 26, 2021 at 03:03:39PM +0100, Arnd Bergmann wrote: > > > > > > > From: Arnd Bergmann <arnd@arndb.de> > > > > > > > > > > > > > > When building with CONFIG_LD_DEAD_CODE_DATA_ELIMINATION, > > > > > > > I sometimes see an assertion > > > > > > > > > > > > > > ld.lld: error: Entry trampoline text too big > > > > > > > > > > > > Heh, "too big" seems a weird report for having it discarded. :) > > > > > > > > > > > > Any idea on this Fangrui? > > > > > > > > > > > > ( I see this is https://github.com/ClangBuiltLinux/linux/issues/1311 ) > > > > > > > > > > This diagnostic is from an ASSERT in arch/arm64/kernel/vmlinux.lds > > > > > > > > > > ASSERT((__entry_tramp_text_end - __entry_tramp_text_start) == (1 << 16), > > > > > "Entry trampoline text too big") > > > > > > > > Can we not change the ASSERT to be <= PAGE_SIZE instead? > > > > > > Ah, that won't work as I suspect we still need the trampoline section. > > > > > > Arnd, do you know why this section disappears? I did a simple test with > > > defconfig + LD_DEAD_CODE_DATA_ELIMINATION and the trampoline section is > > > still around. > > > > If I remember correctly, this showed up when CONFIG_ARM_SDE_INTERFACE > > is disabled, which dropped the only reference into this section. > > If that doesn't make sense, I can try digging through the old build logs to > > reproduce the problem. > > I suspected this as well but still worked for me when disabling it. > > Anyway, I don't think identifying the exact option is necessary. With > CONFIG_UNMAP_KERNEL_AT_EL0=y we need this section around even if only > __entry_tramp_text_start/end are referenced. > > In this case we happened to detect this issue because of the ASSERT in > vmlinux.lds.S but I wonder what else the linker drops with this dead > code elimination that we may not notice (it seems to remove about 500KB > from the resulting image in my test). > > I'll push these two patches to -next for wider coverage before deciding > on mainline (though the option may not get much testing as it's hidden > behind EXPERT and default n). I don't see this in -next? Catalin, do you want me to pick it up as part of my collecting various linker fixes? -Kees
On Wed, Jun 02, 2021 at 12:32:40PM -0700, Kees Cook wrote: > On Tue, Mar 16, 2021 at 07:09:23PM +0000, Catalin Marinas wrote: > > On Tue, Mar 16, 2021 at 05:39:27PM +0100, Arnd Bergmann wrote: > > > On Tue, Mar 16, 2021 at 5:27 PM Catalin Marinas <catalin.marinas@arm.com> wrote: > > > > On Tue, Mar 16, 2021 at 10:45:32AM +0000, Catalin Marinas wrote: > > > > > On Fri, Feb 26, 2021 at 08:32:57PM -0800, Fangrui Song wrote: > > > > > > On 2021-02-26, Kees Cook wrote: > > > > > > > On Fri, Feb 26, 2021 at 03:03:39PM +0100, Arnd Bergmann wrote: > > > > > > > > From: Arnd Bergmann <arnd@arndb.de> > > > > > > > > > > > > > > > > When building with CONFIG_LD_DEAD_CODE_DATA_ELIMINATION, > > > > > > > > I sometimes see an assertion > > > > > > > > > > > > > > > > ld.lld: error: Entry trampoline text too big > > > > > > > > > > > > > > Heh, "too big" seems a weird report for having it discarded. :) > > > > > > > > > > > > > > Any idea on this Fangrui? > > > > > > > > > > > > > > ( I see this is https://github.com/ClangBuiltLinux/linux/issues/1311 ) > > > > > > > > > > > > This diagnostic is from an ASSERT in arch/arm64/kernel/vmlinux.lds > > > > > > > > > > > > ASSERT((__entry_tramp_text_end - __entry_tramp_text_start) == (1 << 16), > > > > > > "Entry trampoline text too big") > > > > > > > > > > Can we not change the ASSERT to be <= PAGE_SIZE instead? > > > > > > > > Ah, that won't work as I suspect we still need the trampoline section. > > > > > > > > Arnd, do you know why this section disappears? I did a simple test with > > > > defconfig + LD_DEAD_CODE_DATA_ELIMINATION and the trampoline section is > > > > still around. > > > > > > If I remember correctly, this showed up when CONFIG_ARM_SDE_INTERFACE > > > is disabled, which dropped the only reference into this section. > > > If that doesn't make sense, I can try digging through the old build logs to > > > reproduce the problem. > > > > I suspected this as well but still worked for me when disabling it. > > > > Anyway, I don't think identifying the exact option is necessary. With > > CONFIG_UNMAP_KERNEL_AT_EL0=y we need this section around even if only > > __entry_tramp_text_start/end are referenced. > > > > In this case we happened to detect this issue because of the ASSERT in > > vmlinux.lds.S but I wonder what else the linker drops with this dead > > code elimination that we may not notice (it seems to remove about 500KB > > from the resulting image in my test). > > > > I'll push these two patches to -next for wider coverage before deciding > > on mainline (though the option may not get much testing as it's hidden > > behind EXPERT and default n). > > I don't see this in -next? Catalin, do you want me to pick it up as part > of my collecting various linker fixes? IIRC this patch only makes sense if we also enable HAVE_LD_DEAD_CODE_DATA_ELIMINATION on arm64. Last time I looked at Arnd's RFC it still had some issues: https://lore.kernel.org/r/20210319122506.GA6832@arm.com So I decided against queuing that for now (and this patch on top was not necessary).
diff --git a/arch/arm64/kernel/vmlinux.lds.S b/arch/arm64/kernel/vmlinux.lds.S index 926cdb597a45..c5ee9d5842db 100644 --- a/arch/arm64/kernel/vmlinux.lds.S +++ b/arch/arm64/kernel/vmlinux.lds.S @@ -96,7 +96,7 @@ jiffies = jiffies_64; #define TRAMP_TEXT \ . = ALIGN(PAGE_SIZE); \ __entry_tramp_text_start = .; \ - *(.entry.tramp.text) \ + KEEP(*(.entry.tramp.text)) \ . = ALIGN(PAGE_SIZE); \ __entry_tramp_text_end = .; #else