[PATCH] arm64: vmlinux.lds.S: keep .entry.tramp.text section

[PATCH] arm64: vmlinux.lds.S: keep .entry.tramp.text section

[Arnd Bergmann]

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

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>
---
 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

Re: [PATCH] arm64: vmlinux.lds.S: keep .entry.tramp.text section

[Kees Cook]

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
> 

-- 
Kees Cook

Re: [PATCH] arm64: vmlinux.lds.S: keep .entry.tramp.text section

[Fangrui Song]

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

Re: [PATCH] arm64: vmlinux.lds.S: keep .entry.tramp.text section

[Catalin Marinas]

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?

-- 
Catalin

Re: [PATCH] arm64: vmlinux.lds.S: keep .entry.tramp.text section

[Catalin Marinas]

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.

-- 
Catalin

Re: [PATCH] arm64: vmlinux.lds.S: keep .entry.tramp.text section

[Arnd Bergmann]

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

Re: [PATCH] arm64: vmlinux.lds.S: keep .entry.tramp.text section

[Catalin Marinas]

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).

-- 
Catalin

Re: [PATCH] arm64: vmlinux.lds.S: keep .entry.tramp.text section

[Kees Cook]

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

-- 
Kees Cook

Re: [PATCH] arm64: vmlinux.lds.S: keep .entry.tramp.text section

[Catalin Marinas]

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).

-- 
Catalin