arm64: alternatives: Move length validation in alternative_{insn,endif}

PATCH Applied Nathan Chancellor 4 participants 9 messages Apr 14, 2021 – Apr 15, 2021

Subsystems (2)

Stable branch arm64

lore

Share & tools

lore

Nathan Chancellor

author

2021-04-14 00:09 UTC #0 lore raw

PATCH 1/1 arm64: alternatives: Move length validation in alternative_{insn,endif}

After commit 2decad92f473 ("arm64: mte: Ensure TIF_MTE_ASYNC_FAULT is set atomically"), LLVM's integrated assembler fails to build entry.S:

<instantiation>:5:7: error: expected assembly-time absolute expression
.org . - (664b-663b) + (662b-661b)

      ^
<instantiation>:6:7: error: expected assembly-time absolute expression
 .org . - (662b-661b) + (664b-663b)
      ^

The root cause is LLVM's assembler has a one-pass design, meaning it cannot figure out these instruction lengths when the .org directive is outside of the subsection that they are in, which was changed by the .arch_extension directive added in the above commit.

Apply the same fix from commit 966a0acce2fc ("arm64/alternatives: move length validation inside the subsection") to the alternative_endif macro, shuffling the .org directives so that the length validation happen will always happen in the same subsections. alternative_insn has not shown any issue yet but it appears that it could have the same issue in the future so just preemptively change it.

Cc [email protected]

Fixes f7b93d42945c ("arm64/alternatives: use subsections for replacement sequences")

Link https://github.com/ClangBuiltLinux/linux/issues/1347

Signed-off-by Nathan Chancellor <[email protected]>

---

Apologies if my explanation or terminology is off, I am only now getting more familiar with assembly.

 arch/arm64/include/asm/alternative-macros.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

arch/arm64/include/asm/alternative-macros.h +4 -4

index 5df500dcc627..8a078fc662ac 100644
@@ -97,9 +97,9 @@
 	.popsection
 	.subsection 1
 663:	\insn2
-664:	.previous
-	.org	. - (664b-663b) + (662b-661b)
+664:	.org	. - (664b-663b) + (662b-661b)
 	.org	. - (662b-661b) + (664b-663b)
+	.previous
 	.endif
 .endm

@@ -169,11 +169,11 @@
  */
 .macro alternative_endif
 664:
+	.org	. - (664b-663b) + (662b-661b)
+	.org	. - (662b-661b) + (664b-663b)
 	.if .Lasm_alt_mode==0
 	.previous
 	.endif
-	.org	. - (664b-663b) + (662b-661b)
-	.org	. - (662b-661b) + (664b-663b)
 .endm
 
 /*

base-commit: 738fa58ee1328481d1d7889e7c430b3401c571b9

Signature · 1 line

2.31.1.272.g89b43f80a5

Sami Tolvanen

Reviewed-by Tested-by on v1 1/1

2021-04-14 17:47 UTC #1 lore raw

Hi Nathan,

Quoted 55 lines · Nathan Chancellor

After commit 2decad92f473 ("arm64: mte: Ensure TIF_MTE_ASYNC_FAULT is set atomically"), LLVM's integrated assembler fails to build entry.S:
<instantiation>:5:7: error: expected assembly-time absolute expression
.org . - (664b-663b) + (662b-661b)
      ^
<instantiation>:6:7: error: expected assembly-time absolute expression
 .org . - (662b-661b) + (664b-663b)
      ^
The root cause is LLVM's assembler has a one-pass design, meaning it cannot figure out these instruction lengths when the .org directive is outside of the subsection that they are in, which was changed by the .arch_extension directive added in the above commit.
Apply the same fix from commit 966a0acce2fc ("arm64/alternatives: move length validation inside the subsection") to the alternative_endif macro, shuffling the .org directives so that the length validation happen will always happen in the same subsections. alternative_insn has not shown any issue yet but it appears that it could have the same issue in the future so just preemptively change it.
Cc [email protected]
Fixes f7b93d42945c ("arm64/alternatives: use subsections for replacement sequences")
Link https://github.com/ClangBuiltLinux/linux/issues/1347
Signed-off-by Nathan Chancellor <[email protected]>
---
Apologies if my explanation or terminology is off, I am only now getting more familiar with assembly.
 arch/arm64/include/asm/alternative-macros.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
arch/arm64/include/asm/alternative-macros.h +2 -2
index 5df500dcc627..8a078fc662ac 100644
@@ -97,9 +97,9 @@
        .popsection
        .subsection 1
 663:   \insn2
-664:   .previous
-       .org    . - (664b-663b) + (662b-661b)
+664:   .org    . - (664b-663b) + (662b-661b)
        .org    . - (662b-661b) + (664b-663b)
+       .previous
        .endif
 .endm
 
@@ -169,11 +169,11 @@
  */
 .macro alternative_endif
 664:

arch/arm64/include/asm/alternative-macros.h line 179

Quoted 10 lines

arch/arm64/include/asm/alternative-macros.h +2 -2

+       .org    . - (664b-663b) + (662b-661b)
+       .org    . - (662b-661b) + (664b-663b)
        .if .Lasm_alt_mode==0
        .previous
        .endif
-       .org    . - (664b-663b) + (662b-661b)
-       .org    . - (662b-661b) + (664b-663b)
 .endm
 
 /*

Thank you for fixing these!

The patch looks correct to me, next-20210413 builds with LLVM_IAS=1 after I applied it, and defconfig built with both Clang and gcc boots normally. Please feel free to add:

Reviewed-by Sami Tolvanen <[email protected]>

Tested-by Sami Tolvanen <[email protected]>

Sami

Nick Desaulniers

Reviewed-by Tested-by on v1 1/1

2021-04-14 19:22 UTC #2 lore raw

In reply to Nathan Chancellor

Quoted 16 lines · Nathan Chancellor

After commit 2decad92f473 ("arm64: mte: Ensure TIF_MTE_ASYNC_FAULT is set atomically"), LLVM's integrated assembler fails to build entry.S:
<instantiation>:5:7: error: expected assembly-time absolute expression
.org . - (664b-663b) + (662b-661b)
      ^
<instantiation>:6:7: error: expected assembly-time absolute expression
 .org . - (662b-661b) + (664b-663b)
      ^
The root cause is LLVM's assembler has a one-pass design, meaning it cannot figure out these instruction lengths when the .org directive is outside of the subsection that they are in, which was changed by the .arch_extension directive added in the above commit.

Quoted 6 lines

Apply the same fix from commit 966a0acce2fc ("arm64/alternatives: move length validation inside the subsection") to the alternative_endif macro, shuffling the .org directives so that the length validation happen will always happen in the same subsections. alternative_insn has not shown any issue yet but it appears that it could have the same issue in the future so just preemptively change it.

Thanks Nathan.

Reviewed-by Nick Desaulniers <[email protected]>

Tested-by Nick Desaulniers <[email protected]>

I did some additional disassembly comparison. In case we ever need it again, I'll copy it below for posterity.

$ ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu make LLVM=1 LLVM_IAS=1
-j72 O=/tmp/a defconfig all
$ b4 am https://lore.kernel.org/lkml/[email protected]/
-o - | git am -3
$ ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu make LLVM=1 LLVM_IAS=1
-j72 O=/tmp/b defconfig all
$ for f in $(find /tmp/a/arch/arm64 -name \*.o); do llvm-objdump -dr
$f > $f.txt; done
$ for f in $(find /tmp/b/arch/arm64 -name \*.o); do llvm-objdump -dr
$f > $f.txt; done
$ for f in $(find /tmp/a/arch/arm64 -name \*.o); do diff -u $f.txt
$(echo $f.txt|sed 's/a/b/'); done | less

For no difference. You can check more sections than .text by changing -d to -D for llvm-objdump, though you're going to get a lot of noise related to changes in .strtab and relocations referring to debug info (.debug_str). But if I drop your patch, rebuild, and recompare, I see the same differences.

Quoted 48 lines

Cc [email protected]
Fixes f7b93d42945c ("arm64/alternatives: use subsections for replacement sequences")
Link https://github.com/ClangBuiltLinux/linux/issues/1347
Signed-off-by Nathan Chancellor <[email protected]>
---
Apologies if my explanation or terminology is off, I am only now getting more familiar with assembly.
 arch/arm64/include/asm/alternative-macros.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
arch/arm64/include/asm/alternative-macros.h +4 -4
index 5df500dcc627..8a078fc662ac 100644
@@ -97,9 +97,9 @@
        .popsection
        .subsection 1
 663:   \insn2
-664:   .previous
-       .org    . - (664b-663b) + (662b-661b)
+664:   .org    . - (664b-663b) + (662b-661b)
        .org    . - (662b-661b) + (664b-663b)
+       .previous
        .endif
 .endm
 
@@ -169,11 +169,11 @@
  */
 .macro alternative_endif
 664:
+       .org    . - (664b-663b) + (662b-661b)
+       .org    . - (662b-661b) + (664b-663b)
        .if .Lasm_alt_mode==0
        .previous
        .endif
-       .org    . - (664b-663b) + (662b-661b)
-       .org    . - (662b-661b) + (664b-663b)
 .endm
 
 /*
base-commit: 738fa58ee1328481d1d7889e7c430b3401c571b9 --
2.31.1.272.g89b43f80a5

Signature · 2 lines

Thanks,
~Nick Desaulniers

Catalin Marinas

maintainer

2021-04-15 09:17 UTC #3 lore raw

In reply to Nathan Chancellor

Hi Nathan,

Quoted 9 lines · Nathan Chancellor

After commit 2decad92f473 ("arm64: mte: Ensure TIF_MTE_ASYNC_FAULT is set atomically"), LLVM's integrated assembler fails to build entry.S:
<instantiation>:5:7: error: expected assembly-time absolute expression
.org . - (664b-663b) + (662b-661b)
      ^
<instantiation>:6:7: error: expected assembly-time absolute expression
 .org . - (662b-661b) + (664b-663b)
      ^

I tried the latest Linus' tree and linux-next (defconfig) with this commit in and I can't get your build error. I used both clang-10 from Debian stable and clang-11 from Debian sid. So, which clang version did you use or which kernel config options?

Signature · 1 line

Catalin

Nathan Chancellor

author

2021-04-15 13:26 UTC #4 lore raw

Quoted 13 lines · Catalin Marinas

Hi Nathan,
Quoted 9 lines · Nathan Chancellor
After commit 2decad92f473 ("arm64: mte: Ensure TIF_MTE_ASYNC_FAULT is set atomically"), LLVM's integrated assembler fails to build entry.S:
<instantiation>:5:7: error: expected assembly-time absolute expression
.org . - (664b-663b) + (662b-661b)
      ^
<instantiation>:6:7: error: expected assembly-time absolute expression
 .org . - (662b-661b) + (664b-663b)
      ^

Quoted 8 lines

I tried the latest Linus' tree and linux-next (defconfig) with this commit in and I can't get your build error. I used both clang-10 from Debian stable and clang-11 from Debian sid. So, which clang version did you use or which kernel config options?
Signature · 1 line
Catalin

Hi Catalin,

Interesting, this reproduces for me with LLVM 12 or newer with just defconfig.

$ make -j"$(nproc)" ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- LLVM=1 LLVM_IAS=1 defconfig arch/arm64/kernel/entry.o

https://github.com/ClangBuiltLinux/continuous-integration2/runs/2350258778?check_suite_focus=true

Cheers,
Nathan

Catalin Marinas

maintainer

2021-04-15 14:02 UTC #5 lore raw

Quoted 12 lines · Nathan Chancellor

Quoted 11 lines · Catalin Marinas
Quoted 9 lines · Nathan Chancellor
After commit 2decad92f473 ("arm64: mte: Ensure TIF_MTE_ASYNC_FAULT is set atomically"), LLVM's integrated assembler fails to build entry.S:
<instantiation>:5:7: error: expected assembly-time absolute expression
.org . - (664b-663b) + (662b-661b)
      ^
<instantiation>:6:7: error: expected assembly-time absolute expression
 .org . - (662b-661b) + (664b-663b)
      ^

Quoted 7 lines

Quoted 4 lines
I tried the latest Linus' tree and linux-next (defconfig) with this commit in and I can't get your build error. I used both clang-10 from Debian stable and clang-11 from Debian sid. So, which clang version did you use or which kernel config options?
Interesting, this reproduces for me with LLVM 12 or newer with just defconfig.

It fails for me as well with clang-12. Do you happen to know why it works fine with previous clang versions?

Signature · 1 line

Catalin

Sami Tolvanen

2021-04-15 15:50 UTC #6 lore raw

Quoted 19 lines · Catalin Marinas

Quoted 17 lines · Nathan Chancellor
Quoted 15 lines · Catalin Marinas
Quoted 9 lines · Nathan Chancellor
After commit 2decad92f473 ("arm64: mte: Ensure TIF_MTE_ASYNC_FAULT is set atomically"), LLVM's integrated assembler fails to build entry.S:
<instantiation>:5:7: error: expected assembly-time absolute expression
.org . - (664b-663b) + (662b-661b)
      ^
<instantiation>:6:7: error: expected assembly-time absolute expression
 .org . - (662b-661b) + (664b-663b)
      ^
I tried the latest Linus' tree and linux-next (defconfig) with this commit in and I can't get your build error. I used both clang-10 from Debian stable and clang-11 from Debian sid. So, which clang version did you use or which kernel config options?

Quoted 5 lines

Quoted 2 lines
Interesting, this reproduces for me with LLVM 12 or newer with just defconfig.
It fails for me as well with clang-12. Do you happen to know why it works fine with previous clang versions?

It looks like CONFIG_ARM64_AS_HAS_MTE is not set when we use the integrated assembler with LLVM 11, and the code that breaks later versions is gated behind CONFIG_ARM64_MTE.

Sami

Catalin Marinas

maintainer

2021-04-15 16:57 UTC #7 lore raw

Quoted 23 lines · Sami Tolvanen

Quoted 22 lines · Catalin Marinas
Quoted 19 lines · Nathan Chancellor
Quoted 11 lines · Catalin Marinas
Quoted 9 lines · Nathan Chancellor
After commit 2decad92f473 ("arm64: mte: Ensure TIF_MTE_ASYNC_FAULT is set atomically"), LLVM's integrated assembler fails to build entry.S:
<instantiation>:5:7: error: expected assembly-time absolute expression
.org . - (664b-663b) + (662b-661b)
      ^
<instantiation>:6:7: error: expected assembly-time absolute expression
 .org . - (662b-661b) + (664b-663b)
      ^
Quoted 4 lines
I tried the latest Linus' tree and linux-next (defconfig) with this commit in and I can't get your build error. I used both clang-10 from Debian stable and clang-11 from Debian sid. So, which clang version did you use or which kernel config options?
Interesting, this reproduces for me with LLVM 12 or newer with just defconfig.

Quoted 6 lines

Quoted 2 lines
It fails for me as well with clang-12. Do you happen to know why it works fine with previous clang versions?
It looks like CONFIG_ARM64_AS_HAS_MTE is not set when we use the integrated assembler with LLVM 11, and the code that breaks later versions is gated behind CONFIG_ARM64_MTE.

That explains it, thanks.

Signature · 1 line

Catalin

Catalin Marinas

maintainer Applied

2021-04-15 17:48 UTC #8 lore raw

Re: [PATCH] arm64: alternatives: Move length validation in alternative_{insn, endif}

In reply to Nathan Chancellor

Quoted 11 lines · Nathan Chancellor

After commit 2decad92f473 ("arm64: mte: Ensure TIF_MTE_ASYNC_FAULT is set atomically"), LLVM's integrated assembler fails to build entry.S:
<instantiation>:5:7: error: expected assembly-time absolute expression
.org . - (664b-663b) + (662b-661b)
      ^
<instantiation>:6:7: error: expected assembly-time absolute expression
 .org . - (662b-661b) + (664b-663b)
      ^
[...]

Applied to arm64 (for-next/fixes), thanks!

[1/1] arm64: alternatives: Move length validation in alternative_{insn, endif}

      https://git.kernel.org/arm64/c/22315a2296f4

Signature · 1 line

Catalin