Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SRSO] mitigations cause unable to move location counter backward for: .text for LTO #1909

Closed
nickdesaulniers opened this issue Aug 9, 2023 · 12 comments
Labels
[FEATURE] LTO Related to building the kernel with LLVM Link Time Optimization [FIXED][LINUX] development cycle This bug was only present and fixed in a -next or -rc cycle [TOOL] lld The issue is relevant to LLD linker

Comments

@nickdesaulniers
Copy link
Member

As reported by @nathanchance here

ld.lld: error: ./arch/x86/kernel/vmlinux.lds:35: unable to move location counter backward for: .text
@nickdesaulniers
Copy link
Member Author

@samitolvanen and I had a conversation about this on internal chat:

me:
I'm getting some odd results where the linker script has:.text {
  *(.text.__x86.rethunk_untrain)
  __entry_text_start = .
  __entry_text_end = .
  *(.text.__x86.rethunk_safe)
}
but with LTO somehow the symbols from the two retunk sections have addresses both less than the two __entry symbols
I wonder if TEXT_MAIN is matching those rethunks.  include/asm-generic/vmlinux.lds.h L99
sami:
Maybe .text.__x86* gets merged into .text
Exactly
Needs double dots
me:
#define TEXT_MAIN .text .text.[0-9a-zA-Z_]*would .text.__x86.rethunk_untrain match that though?
shouldn't the third . in .text.__x86.rethunk_untrain fail to match?
sami:
Good point, the second dot shouldn't match
Then again, the regex doesn't specify the end, so depends on how it's used
It matches the .text.__x86 part
Not sure how the linker handles it

on a hunch, I tried simply renaming the section and that seems to fix this.

diff --git a/arch/x86/kernel/vmlinux.lds.S b/arch/x86/kernel/vmlinux.lds.S
index eb1326602e8a..e5bde2e64a62 100644
--- a/arch/x86/kernel/vmlinux.lds.S
+++ b/arch/x86/kernel/vmlinux.lds.S
@@ -142,7 +142,7 @@ SECTIONS
 
                ALIGN_ENTRY_TEXT_BEGIN
 #ifdef CONFIG_CPU_SRSO
-               *(.text.__x86.rethunk_untrain)
+               *(.__x86.rethunk_untrain)
 #endif
 
                ENTRY_TEXT
@@ -155,7 +155,7 @@ SECTIONS
                ASSERT((srso_untrain_ret_alias & (PMD_SIZE - 1)) == 0,
                        "srso_untrain_ret_alias is not 2MiB aligned");
                . = srso_untrain_ret_alias | (1 << 2) | (1 << 8) | (1 << 14) | (1 << 20);
-               *(.text.__x86.rethunk_safe)
+               *(.__x86.rethunk_safe)
 #endif
                ALIGN_ENTRY_TEXT_END
                *(.gnu.warning)
diff --git a/arch/x86/lib/retpoline.S b/arch/x86/lib/retpoline.S
index 2cff585f22f2..df29da82993c 100644
--- a/arch/x86/lib/retpoline.S
+++ b/arch/x86/lib/retpoline.S
@@ -148,7 +148,7 @@ SYM_CODE_END(__x86_indirect_jump_thunk_array)
  * As a result, srso_safe_ret_alias() becomes a safe return.
  */
 #ifdef CONFIG_CPU_SRSO
-       .section .text.__x86.rethunk_untrain
+       .section .__x86.rethunk_untrain, "ax"
 
 SYM_START(srso_untrain_ret_alias, SYM_L_GLOBAL, SYM_A_NONE)
        ANNOTATE_NOENDBR
@@ -158,7 +158,7 @@ SYM_START(srso_untrain_ret_alias, SYM_L_GLOBAL, SYM_A_NONE)
 SYM_FUNC_END(srso_untrain_ret_alias)
 __EXPORT_THUNK(srso_untrain_ret_alias)
 
-       .section .text.__x86.rethunk_safe
+       .section .__x86.rethunk_safe, "ax"
 #endif
 
 /* Needs a definition for the __x86_return_thunk alternative below. */

And that works:

$ grep -rn LTO_CLANG_THIN .config
719:CONFIG_ARCH_SUPPORTS_LTO_CLANG_THIN=y
723:CONFIG_LTO_CLANG_THIN=y
$ llvm-readelf -s vmlinux | grep -e ' srso_safe_ret_alias' -e ' srso_untrain_ret_alias' -e '__entry_text_'
159108: ffffffff82104104     6 FUNC    GLOBAL DEFAULT     1 srso_safe_ret_alias
159475: ffffffff82000000    10 FUNC    GLOBAL DEFAULT     1 srso_untrain_ret_alias
163671: ffffffff82000010     0 NOTYPE  GLOBAL DEFAULT     1 __entry_text_start
163672: ffffffff82001fdb     0 NOTYPE  GLOBAL DEFAULT     1 __entry_text_end

I'm trying to remember if globbing rules for input sections in linker scripts came up before.

Perhaps this should be fixed in LLD? It's surprising to me that .text.[0-9a-zA-Z_]* would match .text.__x86.rethunk_untrain. Otherwise perhaps we need to be more specific with our regex here

@nathanchance
Copy link
Member

nathanchance commented Aug 9, 2023

Ha, I think this series might need to be fast tracked and be updated for SRSO...

https://lore.kernel.org/20230711091952.27944-1-petr.pavlu@suse.com/

EDIT: Or at least the first patch.

@nathanchance
Copy link
Member

Yeah, that series is in -next and I can see that Stephen fixed up the merge conflict with respect to SRSO:

https://lore.kernel.org/20230809121511.683222bc@canb.auug.org.au/

If I apply the proposed fix for #1907 on top of next-20230809, I don't see an error with ARCH=x86_64 defconfig + CONFIG_LTO_CLANG_THIN=y any more.

@nickdesaulniers
Copy link
Member Author

nickdesaulniers commented Aug 9, 2023

FWICT, it seems like LLD matches the behavior of BFD for globbing.

__attribute__((section(".text.10.hot")))
int bar2 = -1;

__attribute__((section(".text.10")))
int foo2 = 42;

int main() {
  return foo2 + bar2;
}
# foo.lds
PHDRS {
  text PT_LOAD ;
}
SECTIONS {
  .text : {
    *(.text .text.[0-9]*)
  } : text
}
$ clang y.c -Wl,-T,foo.lds -O0 -fuse-ld=bfd -Wl,--orphan-handling=warn 2>&1 | grep text
$
$ llvm-nm a.out | grep -e foo -e bar
00000000000003dc T bar2
00000000000003e0 T foo2

So bar2 was defined first in section .text.10.hot and foo2 in .text.10. Without the linker script, I'd have expected all linkers to lay out bar2 then foo2 (so that bar2 has a lower address than foo2).

With the addition of the linker script and my knowledge of how regexes work (apparently everywhere BUT linkerscripts), I'd have expected:

  1. .text.10 to be matched by .text.[0-9]* (thus moving foo2 first)
  2. -Wl-,--orphan-handling=warn to warn that .text.10.hot was considered an orphan, since it wasn't explicitly placed and did not match .text.[0-9]* due to the third dot in .text.10.hot.

Instead, we get the unexpected order:

$ clang y.c -Wl,-T,foo.lds -O0 -fuse-ld=bfd -Wl,--orphan-handling=warn 2>&1 | grep text
$ llvm-nm a.out | grep -e foo -e bar
00000000000003dc T bar2
00000000000003e0 T foo2

Where bar2 comes //after// foo2, and no orphan section warning. This happens even if I:

  1. change -fuse-ld=bfd to -fuse-ld=lld
  2. change -fuse-ld=bfd to -fuse-ld=lld -flto=thin

I swear @MaskRay explained this to us once before somewhere...but those experiments make it look to me less like any kind of bug in LLD and more like a general hazard when using globbing rules on input sections in linker scripts.

I'm not sure this surprising behavior is documented

Ha, I think this series might need to be fast tracked and be updated for SRSO...
https://lore.kernel.org/20230711091952.27944-1-petr.pavlu@suse.com/
EDIT: Or at least the first patch.
Yeah, that series is in -next and I can see that Stephen fixed up the merge conflict with respect to SRSO:
https://lore.kernel.org/20230809121511.683222bc@canb.auug.org.au/

Ah, yeah! That looks like the same issue and sfr has fixed it in the new places, too. That fixed up version resolves the issue for me.

Boris also mentioned on IRC:

those series by Petr are already in tip and I had to add the second "." to the section names while merging but more tomorrow. Gnight

Though what's in tip is the old/broken version
https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/commit/?id=973ab2d61f33dc85212c486e624af348c4eeb5c9
that needs sfr's fixes
https://lore.kernel.org/all/20230809121511.683222bc@canb.auug.org.au/

@nickdesaulniers nickdesaulniers added the [PATCH] Exists There is a patch that fixes this issue label Aug 9, 2023
@nathanchance
Copy link
Member

I have pushed https://github.com/ClangBuiltLinux/linux/commits/srso-fixes for testing.

@nickdesaulniers nickdesaulniers changed the title SRSO mitigations cause unable to move location counter backward for: .text for LTO [SRSO] mitigations cause unable to move location counter backward for: .text for LTO Aug 10, 2023
@nickdesaulniers
Copy link
Member Author

nickdesaulniers commented Aug 10, 2023

I've left this message to Boris on IRC:

bpetkov: so this patch in tip needs 2 fixes: https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/commit/?id=973ab2d61f33dc85212c486e624af348c4eeb5c9 . 1 is the strncmp size adjustment pointed out by andyhhp on list. 2 is the update for the newly added sections in your SRSO patch. How would you like to proceed?
nathanchance has pushed an example of the fixed up patch: 150c424
that will resolve our LTO build failure

krasCGQ added a commit to archlinux-kud/linux-tkg that referenced this issue Aug 11, 2023
Two patches from https://github.com/ClangBuiltLinux/tree/srso-fixes have to
be brought up due to linking issues with LLD and Clang LTO.

These are tracked as two separate issues: ClangBuiltLinux/linux#1907 and ClangBuiltLinux/linux#1909

Signed-off-by: Albert I <kras@raphielgang.org>
krasCGQ added a commit to archlinux-kud/linux-tkg that referenced this issue Aug 11, 2023
Two patches from https://github.com/ClangBuiltLinux/linux/tree/srso-fixes have
to be brought up due to linking issues with LLD and Clang LTO.

These are tracked as two separate issues: ClangBuiltLinux/linux#1907 and ClangBuiltLinux/linux#1909

Signed-off-by: Albert I <kras@raphielgang.org>
@nathanchance
Copy link
Member

Patch has been picked up in x86/urgent: https://git.kernel.org/tip/79cd2a11224eab86d6673fe8a11d2046ae9d2757

@nathanchance nathanchance added [PATCH] Accepted A submitted patch has been accepted upstream and removed [PATCH] Exists There is a patch that fixes this issue labels Aug 14, 2023
Locietta added a commit to Locietta/xanmod-kernel-WSL2 that referenced this issue Aug 18, 2023
* fix: remove flags not available in clang 15

* ci: move toolchain to llvm-git, drop patch for clang15 workaround

* ci: use wildcard for safe directory workaround

* ci: install llvm and upgrade all packages

LLVM is already trimmed from upstream container

* patch: fix LTO build with SRSO

patch to fix this is from https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/diff/?id=79cd2a11224eab86d6673fe8a11d2046ae9d2757

Ref: ClangBuiltLinux/linux#1909

* patch: adapt srso lto fix to 6.1

* misc: fix typo, rename `build-lts` to `build`
@nathanchance
Copy link
Member

The fix is now in mainline: https://git.kernel.org/linus/79cd2a11224eab86d6673fe8a11d2046ae9d2757

It has been backported to 6.4 so far but it should go back to 6.1 and 5.15, it appears the conflicts/issues with all the SRSO fixes have delayed these backports past 6.4 for now.

@nathanchance nathanchance added [FIXED][LINUX] development cycle This bug was only present and fixed in a -next or -rc cycle and removed [PATCH] Accepted A submitted patch has been accepted upstream labels Aug 23, 2023
@nickdesaulniers
Copy link
Member Author

Do we want to keep this open to track backports?

@nathanchance
Copy link
Member

Sure.

@nathanchance nathanchance reopened this Aug 23, 2023
@nathanchance nathanchance added the Needs Backport Should be backported to either linux-stable tree or latest llvm release branch. label Aug 23, 2023
@nathanchance
Copy link
Member

@nathanchance
Copy link
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[FEATURE] LTO Related to building the kernel with LLVM Link Time Optimization [FIXED][LINUX] development cycle This bug was only present and fixed in a -next or -rc cycle [TOOL] lld The issue is relevant to LLD linker
Projects
None yet
Development

No branches or pull requests

2 participants