arm64: vmlinux.lds.S: keep .entry.tramp.text section

PATCH In review Arnd Bergmann 4 participants 9 messages Feb 26, 2021 – Jun 3, 2021 arm64

lore

Share & tools

lore

Arnd Bergmann

author

2021-02-26 14:06 UTC #0 lore raw

PATCH 1/1 arm64: vmlinux.lds.S: keep .entry.tramp.text section

From: Arnd Bergmann <[email protected]>

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 <[email protected]>

---
 arch/arm64/kernel/vmlinux.lds.S | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/arm64/kernel/vmlinux.lds.S +1 -1

index 926cdb597a45..c5ee9d5842db 100644
@@ -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

Signature · 7 lines

2.29.2


_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Kees Cook

Reviewed-by on v1 1/1

2021-02-26 21:00 UTC #1 lore raw

Quoted 6 lines · Arnd Bergmann

From: Arnd Bergmann <[email protected]>
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 )

Quoted 7 lines

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 <[email protected]>

As a work-around, it seems fine to me.

Reviewed-by Kees Cook <[email protected]>

-Kees

Quoted 20 lines

---
 arch/arm64/kernel/vmlinux.lds.S | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/arm64/kernel/vmlinux.lds.S +1 -1

index 926cdb597a45..c5ee9d5842db 100644
@@ -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

Signature · 1 line

2.29.2

Signature · 1 line

Kees Cook

Fangrui Song

Reviewed-by

2021-02-27 04:34 UTC #2 lore raw

Quoted 6 lines · Kees Cook

Quoted 5 lines · Arnd Bergmann
From: Arnd Bergmann <[email protected]>
When building with CONFIG_LD_DEAD_CODE_DATA_ELIMINATION, I sometimes see an assertion

Quoted 7 lines

Quoted 1 line
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/[email protected]/

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 <[email protected]>

Quoted 39 lines

Quoted 9 lines
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 <[email protected]>
As a work-around, it seems fine to me.
Reviewed-by Kees Cook <[email protected]>
-Kees
Quoted 20 lines
---
 arch/arm64/kernel/vmlinux.lds.S | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/arm64/kernel/vmlinux.lds.S +1 -2
index 926cdb597a45..c5ee9d5842db 100644
@@ -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
Signature · 1 line
Kees Cook

Catalin Marinas

maintainer

2021-03-16 10:46 UTC #3 lore raw

Quoted 11 lines · Fangrui Song

Quoted 10 lines · Kees Cook
Quoted 6 lines · Arnd Bergmann
From: Arnd Bergmann <[email protected]>
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. :)

Quoted 8 lines

Quoted 3 lines
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?

Signature · 1 line

Catalin

Catalin Marinas

maintainer

2021-03-16 16:28 UTC #4 lore raw

Quoted 14 lines · Catalin Marinas

Quoted 13 lines · Fangrui Song
Quoted 12 lines · Kees Cook
Quoted 6 lines · Arnd Bergmann
From: Arnd Bergmann <[email protected]>
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?

Quoted 8 lines

Quoted 6 lines
Quoted 1 line
( 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.

Signature · 1 line

Catalin

Arnd Bergmann

author

2021-03-16 16:40 UTC #5 lore raw

Quoted 23 lines · Catalin Marinas

Quoted 21 lines · Catalin Marinas
Quoted 19 lines · Fangrui Song
Quoted 6 lines · Kees Cook
Quoted 5 lines · Arnd Bergmann
From: Arnd Bergmann <[email protected]>
When building with CONFIG_LD_DEAD_CODE_DATA_ELIMINATION, I sometimes see an assertion
Quoted 7 lines
Quoted 1 line
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")

Quoted 7 lines

Quoted 1 line
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

Catalin Marinas

maintainer

2021-03-16 19:10 UTC #6 lore raw

Quoted 27 lines · Arnd Bergmann

Quoted 26 lines · Catalin Marinas
Quoted 14 lines · Catalin Marinas
Quoted 13 lines · Fangrui Song
Quoted 12 lines · Kees Cook
Quoted 6 lines · Arnd Bergmann
From: Arnd Bergmann <[email protected]>
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?
Quoted 8 lines
Quoted 6 lines
Quoted 1 line
( 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.

Quoted 8 lines

Quoted 3 lines
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).

Signature · 1 line

Catalin

Kees Cook

2021-06-02 19:33 UTC #7 lore raw

Quoted 43 lines · Catalin Marinas

Quoted 27 lines · Arnd Bergmann
Quoted 26 lines · Catalin Marinas
Quoted 14 lines · Catalin Marinas
Quoted 13 lines · Fangrui Song
Quoted 12 lines · Kees Cook
Quoted 6 lines · Arnd Bergmann
From: Arnd Bergmann <[email protected]>
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?
Quoted 8 lines
Quoted 6 lines
Quoted 1 line
( 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.
Quoted 8 lines
Quoted 3 lines
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.

Quoted 8 lines

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

Signature · 1 line

Kees Cook

Catalin Marinas

maintainer

2021-06-03 12:07 UTC #8 lore raw

Quoted 49 lines · Kees Cook

Quoted 48 lines · Catalin Marinas
Quoted 27 lines · Arnd Bergmann
Quoted 26 lines · Catalin Marinas
Quoted 14 lines · Catalin Marinas
Quoted 13 lines · Fangrui Song
Quoted 12 lines · Kees Cook
Quoted 6 lines · Arnd Bergmann
From: Arnd Bergmann <[email protected]>
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?
Quoted 8 lines
Quoted 6 lines
Quoted 1 line
( 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.
Quoted 8 lines
Quoted 3 lines
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).

Quoted 6 lines

Quoted 3 lines
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/[email protected]

So I decided against queuing that for now (and this patch on top was not necessary).

Signature · 1 line

Catalin