arm64: jump_label: use constraint "S" instead of "i"

The constraint "i" seems to be copied from x86 (and with a redundant modifier "c"). It works with -fno-PIE but not with -fPIE/-fPIC in GCC's aarch64 port.

The constraint "S", which denotes a symbol reference (e.g. function, global variable) or label reference, is more appropriate, and has been available in GCC since 2012 and in Clang since 7.0.

Signed-off-by Fangrui Song <[email protected]>

Link https://maskray.me/blog/2024-01-30-raw-symbol-names-in-inline-assembly

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

arch/arm64/include/asm/jump_label.h +0 -0

index 48ddc0f45d22..31862b3bb33d 100644

arch/arm64/include/asm/jump_label.h +2 -3

        return false;
 l_yes:

@@ -40,9 +40,9 @@ static __always_inline bool arch_static_branch_jump(struct static_key * const ke
                 "      .pushsection    __jump_table, \"aw\"    \n\t"
                 "      .align          3                       \n\t"
                 "      .long           1b - ., %l[l_yes] - .   \n\t"
-                "      .quad           %c0 - .                 \n\t"
+                "      .quad           %0 - .                  \n\t"
                 "      .popsection                             \n\t"
-                :  :  "i"(&((char *)key)[branch]) :  : l_yes);
+                :  :  "S"(&((char *)key)[branch]) :  : l_yes);
 
        return false;
 l_yes:
--

2.43.0.429.g432eaa2c6b-goog

Hello Fangrui,

Quoted 4 lines · Fangrui Song

The constraint "i" seems to be copied from x86 (and with a redundant modifier "c"). It works with -fno-PIE but not with -fPIE/-fPIC in GCC's aarch64 port.

Quoted 15 lines

The constraint "S", which denotes a symbol reference (e.g. function, global variable) or label reference, is more appropriate, and has been available in GCC since 2012 and in Clang since 7.0.

Signed-off-by Fangrui Song <[email protected]>

Link https://maskray.me/blog/2024-01-30-raw-symbol-names-in-inline-assembly

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

arch/arm64/include/asm/jump_label.h +0 -0

index 48ddc0f45d22..31862b3bb33d 100644

arch/arm64/include/asm/jump_label.h static __always_inline bool arch_static_branch(struct static_key * const key, · line 28

Quoted 8 lines

arch/arm64/include/asm/jump_label.h +2 -2

@@ -23,9 +23,9 @@ static __always_inline bool arch_static_branch(struct static_key * const key,
                 "      .pushsection    __jump_table, \"aw\"    \n\t"
                 "      .align          3                       \n\t"
                 "      .long           1b - ., %l[l_yes] - .   \n\t"
-                "      .quad           %c0 - .                 \n\t"
+                "      .quad           %0 - .                  \n\t"
                 "      .popsection                             \n\t"
-                :  :  "i"(&((char *)key)[branch]) :  : l_yes);
+                :  :  "S"(&((char *)key)[branch]) :  : l_yes);

'key' is not used as a raw symbol name. We should make this

" .quad %0 + %1 - ."

and

:: "S"(key), "i"(branch) :: l_yes);

if we want to really clean this up.

Quoted 7 lines · Ard Biesheuvel

Hello Fangrui,

Quoted 4 lines · Fangrui Song

The constraint "i" seems to be copied from x86 (and with a redundant modifier "c"). It works with -fno-PIE but not with -fPIE/-fPIC in GCC's aarch64 port.

(I'm not sure of the exact history, but the "c" may be inherited from arm, where an output modifier was needed to suppress the "#" that prefixes immediates in the traditional asm syntax. This does not actually seem to be required for AArch64: rather while a # is allowed and still considered good style in handwritten asm code, the syntax doesn't require it, and the compiler doesn't emit it for "i" arguments, AFAICT.)

Quoted 30 lines

Quoted 15 lines

The constraint "S", which denotes a symbol reference (e.g. function, global variable) or label reference, is more appropriate, and has been available in GCC since 2012 and in Clang since 7.0.

Signed-off-by Fangrui Song <[email protected]>

Link https://maskray.me/blog/2024-01-30-raw-symbol-names-in-inline-assembly

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

arch/arm64/include/asm/jump_label.h +0 -0

index 48ddc0f45d22..31862b3bb33d 100644

arch/arm64/include/asm/jump_label.h static __always_inline bool arch_static_branch(struct static_key * const key, · line 28

Quoted 8 lines

arch/arm64/include/asm/jump_label.h +2 -2

@@ -23,9 +23,9 @@ static __always_inline bool arch_static_branch(struct static_key * const key,
                 "      .pushsection    __jump_table, \"aw\"    \n\t"
                 "      .align          3                       \n\t"
                 "      .long           1b - ., %l[l_yes] - .   \n\t"
-                "      .quad           %c0 - .                 \n\t"
+                "      .quad           %0 - .                  \n\t"
                 "      .popsection                             \n\t"
-                :  :  "i"(&((char *)key)[branch]) :  : l_yes);
+                :  :  "S"(&((char *)key)[branch]) :  : l_yes);

'key' is not used as a raw symbol name. We should make this

" .quad %0 + %1 - ."

and

Quoted 3 lines

:: "S"(key), "i"(branch) :: l_yes);

if we want to really clean this up.

This hides more logic in the asm so it's arguably more cryptic (although the code is fairly cryptic to begin with -- I don't really see why the argument wasn't written as the equivalent (char *)key + branch...)

Anyway, I don't think the "i" versys "S" distinction makes any difference without -fpic or equivalent, so it is not really relevant for the kernel (except that "S" breaks compatibility with older compilers...)

I think the main advantage of "S" is that it stops you accidentally emitting undesirable relocations from asm code that is not written for the -fpic case.

But just changing "i" to "S" is not sufficient to port asms to -fpic:
the asms still need to be reviewed.

So unless the asm has been reviewed for position-independence, it may anyway be better to stick with "i" so that the compiler actually chokes if someone tries to build the code with -fpic.

Since we are not trying to run arbitraily many running kernels in a common address space (and not likely to do that), I'm not sure that we would ever build the kernel with -fpic except for a few special-case bits like the EFI stub and vDSO... unless I've missed something?

If there's another reason why "S" is advantageous though, I'm happy to be corrected.

Building the KASLR kernel without -fpie but linking it with -pie works in practice, but it is not something that is explicitly supported by the toolchains - it happens to work because the default 'small' code model used by both GCC and Clang relies mostly on ADRP+ADD/LDR to generate symbol references.

Quoted 9 lines · Dave Martin

Quoted 7 lines · Ard Biesheuvel

Hello Fangrui,

Quoted 4 lines · Fangrui Song

The constraint "i" seems to be copied from x86 (and with a redundant modifier "c"). It works with -fno-PIE but not with -fPIE/-fPIC in GCC's aarch64 port.

Quoted 7 lines

(I'm not sure of the exact history, but the "c" may be inherited from arm, where an output modifier was needed to suppress the "#" that prefixes immediates in the traditional asm syntax. This does not actually seem to be required for AArch64: rather while a # is allowed and still considered good style in handwritten asm code, the syntax doesn't require it, and the compiler doesn't emit it for "i" arguments, AFAICT.)

The aarch64 one could be inherited from arch/arm/include/asm/jump_label.h (2012), which could in turn be inherited from x86 (2010).
Both the constraint "i" and the modifier "c" are generic.. For -fno-pic this combination can be used for every arch.

Quoted 30 lines

Quoted 30 lines

Quoted 15 lines

The constraint "S", which denotes a symbol reference (e.g. function, global variable) or label reference, is more appropriate, and has been available in GCC since 2012 and in Clang since 7.0.

Signed-off-by Fangrui Song <[email protected]>

Link https://maskray.me/blog/2024-01-30-raw-symbol-names-in-inline-assembly

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

arch/arm64/include/asm/jump_label.h +0 -0

index 48ddc0f45d22..31862b3bb33d 100644

arch/arm64/include/asm/jump_label.h static __always_inline bool arch_static_branch(struct static_key * const key, · line 28

Quoted 8 lines

arch/arm64/include/asm/jump_label.h +2 -2

@@ -23,9 +23,9 @@ static __always_inline bool arch_static_branch(struct static_key * const key,
                 "      .pushsection    __jump_table, \"aw\"    \n\t"
                 "      .align          3                       \n\t"
                 "      .long           1b - ., %l[l_yes] - .   \n\t"
-                "      .quad           %c0 - .                 \n\t"
+                "      .quad           %0 - .                  \n\t"
                 "      .popsection                             \n\t"
-                :  :  "i"(&((char *)key)[branch]) :  : l_yes);
+                :  :  "S"(&((char *)key)[branch]) :  : l_yes);

'key' is not used as a raw symbol name. We should make this

" .quad %0 + %1 - ."

and

Quoted 8 lines

Quoted 3 lines

:: "S"(key), "i"(branch) :: l_yes);

if we want to really clean this up.

This hides more logic in the asm so it's arguably more cryptic (although the code is fairly cryptic to begin with -- I don't really see why the argument wasn't written as the equivalent (char *)key + branch...)

I agree that using "S" and "i" would introduce complexity. Using just "S" as this patch does should be clear.

All of "i" "s" "S" support a symbol or label reference and a constant offset (can be zero), (in object file, a symbol and an addend; in GCC's term, the sum of a SYMBOL_REF and a CONST_INT).

Quoted 10 lines

Anyway, I don't think the "i" versys "S" distinction makes any difference without -fpic or equivalent, so it is not really relevant for the kernel (except that "S" breaks compatibility with older compilers...)

I think the main advantage of "S" is that it stops you accidentally emitting undesirable relocations from asm code that is not written for the -fpic case.

Quoted 7 lines

But just changing "i" to "S" is not sufficient to port asms to -fpic:
the asms still need to be reviewed.

So unless the asm has been reviewed for position-independence, it may anyway be better to stick with "i" so that the compiler actually chokes if someone tries to build the code with -fpic.

Quoted 5 lines

Since we are not trying to run arbitraily many running kernels in a common address space (and not likely to do that), I'm not sure that we would ever build the kernel with -fpic except for a few special-case bits like the EFI stub and vDSO... unless I've missed something?

Quoted 2 lines

If there's another reason why "S" is advantageous though, I'm happy to be corrected.

I remember that Ard has an RFC
https://lore.kernel.org/linux-arm-kernel/[email protected]/
"[RFC PATCH 0/2] arm64: use PIE code generation for KASLR kernel" and see some recent PIE codegen patches.

Quoted 5 lines

Building the KASLR kernel without -fpie but linking it with -pie works in practice, but it is not something that is explicitly supported by the toolchains - it happens to work because the default 'small' code model used by both GCC and Clang relies mostly on ADRP+ADD/LDR to generate symbol references.

I agree that current -fno-PIE with -shared -Bsymbolic linking is a hack that works as a conincidence, not guaranteed by the toolchain. This jump_label improvement (with no object file difference) fixes an obstacle.

Quoted 67 lines · Fangrui Song

Quoted 9 lines · Dave Martin

Quoted 7 lines · Ard Biesheuvel

Hello Fangrui,

Quoted 4 lines · Fangrui Song

The constraint "i" seems to be copied from x86 (and with a redundant modifier "c"). It works with -fno-PIE but not with -fPIE/-fPIC in GCC's aarch64 port.

Quoted 7 lines

(I'm not sure of the exact history, but the "c" may be inherited from arm, where an output modifier was needed to suppress the "#" that prefixes immediates in the traditional asm syntax. This does not actually seem to be required for AArch64: rather while a # is allowed and still considered good style in handwritten asm code, the syntax doesn't require it, and the compiler doesn't emit it for "i" arguments, AFAICT.)

The aarch64 one could be inherited from arch/arm/include/asm/jump_label.h (2012), which could in turn be inherited from x86 (2010).
Both the constraint "i" and the modifier "c" are generic.. For -fno-pic this combination can be used for every arch.

Quoted 30 lines

Quoted 30 lines

Quoted 15 lines

The constraint "S", which denotes a symbol reference (e.g. function, global variable) or label reference, is more appropriate, and has been available in GCC since 2012 and in Clang since 7.0.

Signed-off-by Fangrui Song <[email protected]>

Link https://maskray.me/blog/2024-01-30-raw-symbol-names-in-inline-assembly

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

arch/arm64/include/asm/jump_label.h +0 -0

index 48ddc0f45d22..31862b3bb33d 100644

arch/arm64/include/asm/jump_label.h static __always_inline bool arch_static_branch(struct static_key * const key, · line 28

Quoted 8 lines

arch/arm64/include/asm/jump_label.h +2 -2

@@ -23,9 +23,9 @@ static __always_inline bool arch_static_branch(struct static_key * const key,
                 "      .pushsection    __jump_table, \"aw\"    \n\t"
                 "      .align          3                       \n\t"
                 "      .long           1b - ., %l[l_yes] - .   \n\t"
-                "      .quad           %c0 - .                 \n\t"
+                "      .quad           %0 - .                  \n\t"
                 "      .popsection                             \n\t"
-                :  :  "i"(&((char *)key)[branch]) :  : l_yes);
+                :  :  "S"(&((char *)key)[branch]) :  : l_yes);

'key' is not used as a raw symbol name. We should make this

" .quad %0 + %1 - ."

and

Quoted 8 lines

Quoted 3 lines

:: "S"(key), "i"(branch) :: l_yes);

if we want to really clean this up.

This hides more logic in the asm so it's arguably more cryptic (although the code is fairly cryptic to begin with -- I don't really see why the argument wasn't written as the equivalent (char *)key + branch...)

I agree that using "S" and "i" would introduce complexity. Using just "S" as this patch does should be clear.

Quoted 3 lines

All of "i" "s" "S" support a symbol or label reference and a constant offset (can be zero), (in object file, a symbol and an addend; in GCC's term, the sum of a SYMBOL_REF and a CONST_INT).

Taken the address of a struct, cast it to char[] and then index it using a boolean is rather disgusting, no?

Quoted 10 lines

Quoted 10 lines

Anyway, I don't think the "i" versys "S" distinction makes any difference without -fpic or equivalent, so it is not really relevant for the kernel (except that "S" breaks compatibility with older compilers...)

I think the main advantage of "S" is that it stops you accidentally emitting undesirable relocations from asm code that is not written for the -fpic case.

Quoted 8 lines

Quoted 7 lines

But just changing "i" to "S" is not sufficient to port asms to -fpic:
the asms still need to be reviewed.

So unless the asm has been reviewed for position-independence, it may anyway be better to stick with "i" so that the compiler actually chokes if someone tries to build the code with -fpic.

The virtual address of the kernel is randomized by KASLR, which relies on PIE linking, and this puts constraints on the permitted types of relocations.

IOW, we basically already build the kernel as PIC code, but without relying on -fPIC, because that triggers some behaviors that only make sense for shared objects in user space.

Quoted 5 lines

Quoted 5 lines

Since we are not trying to run arbitraily many running kernels in a common address space (and not likely to do that), I'm not sure that we would ever build the kernel with -fpic except for a few special-case bits like the EFI stub and vDSO... unless I've missed something?

Yes, KASLR. The number of kernels is not the point, the point is that the virtual load address of the kernel is usually decided at boot, and so the code needs to be generated to accommodate that.

Quoted 14 lines

Quoted 2 lines

If there's another reason why "S" is advantageous though, I'm happy to be corrected.

I remember that Ard has an RFC
https://lore.kernel.org/linux-arm-kernel/[email protected]/
"[RFC PATCH 0/2] arm64: use PIE code generation for KASLR kernel" and see some recent PIE codegen patches.

Quoted 5 lines

Building the KASLR kernel without -fpie but linking it with -pie works in practice, but it is not something that is explicitly supported by the toolchains - it happens to work because the default 'small' code model used by both GCC and Clang relies mostly on ADRP+ADD/LDR to generate symbol references.

Quoted 4 lines

I agree that current -fno-PIE with -shared -Bsymbolic linking is a hack that works as a conincidence, not guaranteed by the toolchain. This jump_label improvement (with no object file difference) fixes an obstacle.

If we can get the guaranteed behavior of #pragma GCC visibility push(hidden) from a command line option, we should build the core kernel with -fpie instead. (Modules are partially linked objects, so they can be built non-PIC as before)

Quoted 20 lines · Ard Biesheuvel

Quoted 19 lines · Fangrui Song

Quoted 16 lines · Dave Martin

Quoted 7 lines · Ard Biesheuvel

Hello Fangrui,

Quoted 4 lines · Fangrui Song

The constraint "i" seems to be copied from x86 (and with a redundant modifier "c"). It works with -fno-PIE but not with -fPIE/-fPIC in GCC's aarch64 port.

(I'm not sure of the exact history, but the "c" may be inherited from arm, where an output modifier was needed to suppress the "#" that prefixes immediates in the traditional asm syntax. This does not actually seem to be required for AArch64: rather while a # is allowed and still considered good style in handwritten asm code, the syntax doesn't require it, and the compiler doesn't emit it for "i" arguments, AFAICT.)

Quoted 5 lines

Quoted 5 lines

The aarch64 one could be inherited from arch/arm/include/asm/jump_label.h (2012), which could in turn be inherited from x86 (2010).
Both the constraint "i" and the modifier "c" are generic.. For -fno-pic this combination can be used for every arch.

Quoted 43 lines

Quoted 43 lines

Quoted 31 lines

Quoted 31 lines

Quoted 16 lines

The constraint "S", which denotes a symbol reference (e.g. function, global variable) or label reference, is more appropriate, and has been available in GCC since 2012 and in Clang since 7.0.

Signed-off-by Fangrui Song <[email protected]>

Link https://maskray.me/blog/2024-01-30-raw-symbol-names-in-inline-assembly

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

arch/arm64/include/asm/jump_label.h +0 -0

index 48ddc0f45d22..31862b3bb33d 100644

arch/arm64/include/asm/jump_label.h static __always_inline bool arch_static_branch(struct static_key * const key, · line 28

Quoted 8 lines

arch/arm64/include/asm/jump_label.h +2 -2

@@ -23,9 +23,9 @@ static __always_inline bool arch_static_branch(struct static_key * const key,
                 "      .pushsection    __jump_table, \"aw\"    \n\t"
                 "      .align          3                       \n\t"
                 "      .long           1b - ., %l[l_yes] - .   \n\t"
-                "      .quad           %c0 - .                 \n\t"
+                "      .quad           %0 - .                  \n\t"
                 "      .popsection                             \n\t"
-                :  :  "i"(&((char *)key)[branch]) :  : l_yes);
+                :  :  "S"(&((char *)key)[branch]) :  : l_yes);

'key' is not used as a raw symbol name. We should make this

" .quad %0 + %1 - ."

and

Quoted 8 lines

Quoted 3 lines

:: "S"(key), "i"(branch) :: l_yes);

if we want to really clean this up.

This hides more logic in the asm so it's arguably more cryptic (although the code is fairly cryptic to begin with -- I don't really see why the argument wasn't written as the equivalent (char *)key + branch...)

I agree that using "S" and "i" would introduce complexity. Using just "S" as this patch does should be clear.

Quoted 6 lines

Quoted 3 lines

All of "i" "s" "S" support a symbol or label reference and a constant offset (can be zero), (in object file, a symbol and an addend; in GCC's term, the sum of a SYMBOL_REF and a CONST_INT).

Taken the address of a struct, cast it to char[] and then index it using a boolean is rather disgusting, no?

I agree with you.

Hmm. Clang's constraint "S" implementation doesn't support a constant offset, so
static_key_false(&nf_hooks_needed[pf][hook]) in include/linux/netfilter.h:nf_hook would not compile with Clang <= 18.

I have a patch https://github.com/llvm/llvm-project/pull/80255 , but even if it is accepted and cherry-picked into the 18.x release branch, if we still support older Clang, we cannot use "S" unconditionally.

So we probably need the following to prepare for -fPIE support in the future:

arch/arm64/include/asm/jump_label.h +0 -0

index 48ddc0f45d22..b8af2f8b0c99 100644

arch/arm64/include/asm/jump_label.h +5 -0

+#define JUMP_LABEL_STATIC_KEY_CONSTRAINT "i"
+#else
+#define JUMP_LABEL_STATIC_KEY_CONSTRAINT "S"
+#endif
+
 static __always_inline bool arch_static_branch(struct static_key * const key,
 					       const bool branch)
 {

arch/arm64/include/asm/jump_label.h +2 -2

 	return false;
 l_yes:

@@ -40,9 +50,9 @@ static __always_inline bool arch_static_branch_jump(struct static_key * const ke
 		 "	.pushsection	__jump_table, \"aw\"	\n\t"
 		 "	.align		3			\n\t"
 		 "	.long		1b - ., %l[l_yes] - .	\n\t"
-		 "	.quad		%c0 - .			\n\t"
+		 "	.quad		%0 + %1 - .		\n\t"
 		 "	.popsection				\n\t"
-		 :  :  "i"(&((char *)key)[branch]) :  : l_yes);
+		 :  :  JUMP_LABEL_STATIC_KEY_CONSTRAINT(key), "i"(branch) :  : l_yes);
 	return false;
 l_yes:
 

Quoted 46 lines

Quoted 11 lines

Quoted 11 lines

Anyway, I don't think the "i" versys "S" distinction makes any difference without -fpic or equivalent, so it is not really relevant for the kernel (except that "S" breaks compatibility with older compilers...)

I think the main advantage of "S" is that it stops you accidentally emitting undesirable relocations from asm code that is not written for the -fpic case.

Quoted 8 lines

Quoted 7 lines

But just changing "i" to "S" is not sufficient to port asms to -fpic:
the asms still need to be reviewed.

So unless the asm has been reviewed for position-independence, it may anyway be better to stick with "i" so that the compiler actually chokes if someone tries to build the code with -fpic.

The virtual address of the kernel is randomized by KASLR, which relies on PIE linking, and this puts constraints on the permitted types of relocations.

IOW, we basically already build the kernel as PIC code, but without relying on -fPIC, because that triggers some behaviors that only make sense for shared objects in user space.

Quoted 5 lines

Quoted 5 lines

Since we are not trying to run arbitraily many running kernels in a common address space (and not likely to do that), I'm not sure that we would ever build the kernel with -fpic except for a few special-case bits like the EFI stub and vDSO... unless I've missed something?

Yes, KASLR. The number of kernels is not the point, the point is that the virtual load address of the kernel is usually decided at boot, and so the code needs to be generated to accommodate that.

Quoted 8 lines

Quoted 2 lines

If there's another reason why "S" is advantageous though, I'm happy to be corrected.

I remember that Ard has an RFC
https://lore.kernel.org/linux-arm-kernel/[email protected]/
"[RFC PATCH 0/2] arm64: use PIE code generation for KASLR kernel" and see some recent PIE codegen patches.

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Building the KASLR kernel without -fpie but linking it with -pie works in practice, but it is not something that is explicitly supported by the toolchains - it happens to work because the default 'small' code model used by both GCC and Clang relies mostly on ADRP+ADD/LDR to generate symbol references.

Quoted 6 lines

Quoted 5 lines

I agree that current -fno-PIE with -shared -Bsymbolic linking is a hack that works as a conincidence, not guaranteed by the toolchain. This jump_label improvement (with no object file difference) fixes an obstacle.

Quoted 5 lines

If we can get the guaranteed behavior of #pragma GCC visibility push(hidden) from a command line option, we should build the core kernel with -fpie instead. (Modules are partially linked objects, so they can be built non-PIC as before)

I believe we don't have such a GCC option, but the effect can be simulated by -include hidden.h ...

On Thu, Feb 01, 2024 at 01:11:20AM -0800, Fangrui Song wrote:

Quoted 19 lines

Hmm. Clang's constraint "S" implementation doesn't support a constant offset, so
static_key_false(&nf_hooks_needed[pf][hook]) in include/linux/netfilter.h:nf_hook would not compile with Clang <= 18.

I have a patch https://github.com/llvm/llvm-project/pull/80255 , but even if it is accepted and cherry-picked into the 18.x release branch, if we still support older Clang, we cannot use "S" unconditionally.

So we probably need the following to prepare for -fPIE support in the future:

arch/arm64/include/asm/jump_label.h +0 -0

index 48ddc0f45d22..b8af2f8b0c99 100644

arch/arm64/include/asm/jump_label.h line 20

Quoted 6 lines

arch/arm64/include/asm/jump_label.h +5 -0

@@ -15,6 +15,16 @@
 #define JUMP_LABEL_NOP_SIZE          AARCH64_INSN_SIZE
+/*
+ * Prefer "S" to support PIC. However, use "i" for Clang 18 and earlier as "S"
+ * on a symbol with a constant offset is not supported.
+ */
+#if defined(CONFIG_CC_IS_CLANG) && __clang_major__ <= 18

Is a GCC version check needed? Or is the minimum GCC version specified for building the kernel new enough?

arch/arm64/include/asm/jump_label.h line 27

Quoted 5 lines

arch/arm64/include/asm/jump_label.h +5 -0

+#define JUMP_LABEL_STATIC_KEY_CONSTRAINT "i"
+#else
+#define JUMP_LABEL_STATIC_KEY_CONSTRAINT "S"
+#endif
+

arch/arm64/include/asm/jump_label.h static __always_inline bool arch_static_branch(struct static_key * const key, · line 38

Quoted 12 lines

arch/arm64/include/asm/jump_label.h +2 -2

 static __always_inline bool arch_static_branch(struct static_key * const key,
                                             const bool branch)
 {

@@ -23,9 +33,9 @@ static __always_inline bool arch_static_branch(struct static_key * const key,
               "      .pushsection    __jump_table, \"aw\"    \n\t"
               "      .align          3                       \n\t"
               "      .long           1b - ., %l[l_yes] - .   \n\t"
-              "      .quad           %c0 - .                 \n\t"
+              "      .quad           %0 + %1 - .             \n\t"
               "      .popsection                             \n\t"
-              :  :  "i"(&((char *)key)[branch]) :  : l_yes);
+              :  :  JUMP_LABEL_STATIC_KEY_CONSTRAINT(key), "i"(branch) :  : l_yes);

While this looks reasonable, I'm still not clear now why the asm must do the addition.

Can we roll the branch argument into the macro?

"S"(symbol + constant) seems to do the right thing for AArch64, at least in GCC.

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 2 lines

If there's another reason why "S" is advantageous though, I'm happy to be corrected.

I remember that Ard has an RFC
https://lore.kernel.org/linux-arm-kernel/[email protected]/
"[RFC PATCH 0/2] arm64: use PIE code generation for KASLR kernel" and see some recent PIE codegen patches.

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Building the KASLR kernel without -fpie but linking it with -pie works in practice, but it is not something that is explicitly supported by the toolchains - it happens to work because the default 'small' code model used by both GCC and Clang relies mostly on ADRP+ADD/LDR to generate symbol references.

Does this mean that doing boot-time relocation on a fully statically linked kernel doesn't bring much benefit?

It would tend to be more
painful to do the relocation work, and mean that the kernel text has to be temporarily writeable, but static linking should have the best runtime performance.

Maybe it doesn't make as much difference as I thought (certainly ADRP based addressing should make a fair amount of the PIC/PIE overhead go away).

Quoted 2 lines · Dave Martin

On Thu, Feb 01, 2024 at 01:11:20AM -0800, Fangrui Song wrote:

...

Quoted 29 lines

Quoted 19 lines

Hmm. Clang's constraint "S" implementation doesn't support a constant offset, so
static_key_false(&nf_hooks_needed[pf][hook]) in include/linux/netfilter.h:nf_hook would not compile with Clang <= 18.

I have a patch https://github.com/llvm/llvm-project/pull/80255 , but even if it is accepted and cherry-picked into the 18.x release branch, if we still support older Clang, we cannot use "S" unconditionally.

So we probably need the following to prepare for -fPIE support in the future:

arch/arm64/include/asm/jump_label.h +0 -0

index 48ddc0f45d22..b8af2f8b0c99 100644

arch/arm64/include/asm/jump_label.h line 20

Quoted 6 lines

arch/arm64/include/asm/jump_label.h +5 -0

@@ -15,6 +15,16 @@
 #define JUMP_LABEL_NOP_SIZE          AARCH64_INSN_SIZE
+/*
+ * Prefer "S" to support PIC. However, use "i" for Clang 18 and earlier as "S"
+ * on a symbol with a constant offset is not supported.
+ */
+#if defined(CONFIG_CC_IS_CLANG) && __clang_major__ <= 18

Is a GCC version check needed? Or is the minimum GCC version specified for building the kernel new enough?

Quoted 5 lines

arch/arm64/include/asm/jump_label.h line 27

Quoted 5 lines

arch/arm64/include/asm/jump_label.h +5 -0

+#define JUMP_LABEL_STATIC_KEY_CONSTRAINT "i"
+#else
+#define JUMP_LABEL_STATIC_KEY_CONSTRAINT "S"
+#endif
+

Can we use "Si" instead?

Quoted 16 lines

arch/arm64/include/asm/jump_label.h static __always_inline bool arch_static_branch(struct static_key * const key, · line 38

Quoted 12 lines

arch/arm64/include/asm/jump_label.h +2 -2

 static __always_inline bool arch_static_branch(struct static_key * const key,
                                             const bool branch)
 {

@@ -23,9 +33,9 @@ static __always_inline bool arch_static_branch(struct static_key * const key,
               "      .pushsection    __jump_table, \"aw\"    \n\t"
               "      .align          3                       \n\t"
               "      .long           1b - ., %l[l_yes] - .   \n\t"
-              "      .quad           %c0 - .                 \n\t"
+              "      .quad           %0 + %1 - .             \n\t"
               "      .popsection                             \n\t"
-              :  :  "i"(&((char *)key)[branch]) :  : l_yes);
+              :  :  JUMP_LABEL_STATIC_KEY_CONSTRAINT(key), "i"(branch) :  : l_yes);

While this looks reasonable, I'm still not clear now why the asm must do the addition.

Quoted 5 lines

Can we roll the branch argument into the macro?

"S"(symbol + constant) seems to do the right thing for AArch64, at least in GCC.

'symbol' is a struct static_key pointer, so adding '1' to it will not produce the needed result.

...

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 2 lines

If there's another reason why "S" is advantageous though, I'm happy to be corrected.

I remember that Ard has an RFC
https://lore.kernel.org/linux-arm-kernel/[email protected]/
"[RFC PATCH 0/2] arm64: use PIE code generation for KASLR kernel" and see some recent PIE codegen patches.

Quoted 8 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Building the KASLR kernel without -fpie but linking it with -pie works in practice, but it is not something that is explicitly supported by the toolchains - it happens to work because the default 'small' code model used by both GCC and Clang relies mostly on ADRP+ADD/LDR to generate symbol references.

Does this mean that doing boot-time relocation on a fully statically linked kernel doesn't bring much benefit?

Not sure I follow you here. The boot-time relocation is necessary in any case, to fix up statically initialized pointer variables all over the kernel.

Quoted 5 lines

It would tend to be more
painful to do the relocation work, and mean that the kernel text has to be temporarily writeable, but static linking should have the best runtime performance.

Not sure what you mean by 'static linking' here.

The only thing PIE linking does in the case of the kernel is
a) complain if static relocations are used that cannot be fixed up at runtime (e.g., movk/movz sequences)
b) emit dynamic relocations that the early startup code can use to fix up all statically initialized pointers

From a codegen point-of-view, there is no difference because we don't use -fpic code. The problem with -fpic codegen is that it makes some assumptions that only hold for shared ELF objects, e.g., avoiding dynamic relocations in .text, using GOT entries even for variables defined in the same compilation so that they can be preempted, keeping all runtime relocatable quantities together so the CoW footprint of the shared library is minimized.

None of this matters for the kernel, and as it turns out, the non-PIC small C model produces code that the PIE linker is happy with.

TL;DR our code (including inline and out-of-line asm) is already PIC, and this property is relied upon by KASLR.

Quoted 4 lines

Maybe it doesn't make as much difference as I thought (certainly ADRP based addressing should make a fair amount of the PIC/PIE overhead go away).

The PIC/PIE overhead is in the indirections via the GOT. However, recent linkers will actually perform some relaxations too: if a symbol is defined in the same executable and is not preemptible, a GOT load

ADRP
LDR

can be transparently converted to

ADRP
ADD

referring to the symbol directly. This is also where hidden.h comes
in: making all symbol declarations and definitions 'hidden' will allow the compiler to assume that a GOT entry is never needed.

So building with -fPIC is currently not need in practice, and creates some complications, which is why we have been avoiding it. But PIE linking non-PIC objects is not guaranteed to remain supported going forward, so it would be better to have a plan B, i.e., being able to turn on -fpic without massive regressions due to GOT overhead, or codegen problems with our asm hacks.

Quoted 33 lines · Ard Biesheuvel

Quoted 2 lines · Dave Martin

On Thu, Feb 01, 2024 at 01:11:20AM -0800, Fangrui Song wrote:

...

Quoted 29 lines

Quoted 19 lines

Hmm. Clang's constraint "S" implementation doesn't support a constant offset, so
static_key_false(&nf_hooks_needed[pf][hook]) in include/linux/netfilter.h:nf_hook would not compile with Clang <= 18.

I have a patch https://github.com/llvm/llvm-project/pull/80255 , but even if it is accepted and cherry-picked into the 18.x release branch, if we still support older Clang, we cannot use "S" unconditionally.

So we probably need the following to prepare for -fPIE support in the future:

arch/arm64/include/asm/jump_label.h +0 -0

index 48ddc0f45d22..b8af2f8b0c99 100644

arch/arm64/include/asm/jump_label.h line 20

Quoted 6 lines

arch/arm64/include/asm/jump_label.h +5 -0

@@ -15,6 +15,16 @@
 #define JUMP_LABEL_NOP_SIZE          AARCH64_INSN_SIZE
+/*
+ * Prefer "S" to support PIC. However, use "i" for Clang 18 and earlier as "S"
+ * on a symbol with a constant offset is not supported.
+ */
+#if defined(CONFIG_CC_IS_CLANG) && __clang_major__ <= 18

Is a GCC version check needed? Or is the minimum GCC version specified for building the kernel new enough?

Quoted 7 lines

Quoted 5 lines

arch/arm64/include/asm/jump_label.h line 27

Quoted 5 lines

arch/arm64/include/asm/jump_label.h +5 -0

+#define JUMP_LABEL_STATIC_KEY_CONSTRAINT "i"
+#else
+#define JUMP_LABEL_STATIC_KEY_CONSTRAINT "S"
+#endif
+

Can we use "Si" instead?

I thought the point was to avoid "S" on compilers that would choke on it? If so, those compilers would surely choke on "Si" too, no?

Quoted 16 lines

Quoted 16 lines

arch/arm64/include/asm/jump_label.h static __always_inline bool arch_static_branch(struct static_key * const key, · line 38

Quoted 12 lines

arch/arm64/include/asm/jump_label.h +2 -2

 static __always_inline bool arch_static_branch(struct static_key * const key,
                                             const bool branch)
 {

@@ -23,9 +33,9 @@ static __always_inline bool arch_static_branch(struct static_key * const key,
               "      .pushsection    __jump_table, \"aw\"    \n\t"
               "      .align          3                       \n\t"
               "      .long           1b - ., %l[l_yes] - .   \n\t"
-              "      .quad           %c0 - .                 \n\t"
+              "      .quad           %0 + %1 - .             \n\t"
               "      .popsection                             \n\t"
-              :  :  "i"(&((char *)key)[branch]) :  : l_yes);
+              :  :  JUMP_LABEL_STATIC_KEY_CONSTRAINT(key), "i"(branch) :  : l_yes);

While this looks reasonable, I'm still not clear now why the asm must do the addition.

Quoted 8 lines

Quoted 5 lines

Can we roll the branch argument into the macro?

"S"(symbol + constant) seems to do the right thing for AArch64, at least in GCC.

'symbol' is a struct static_key pointer, so adding '1' to it will not produce the needed result.

I mean loosely things that resolve to something of the form "symbol + constant" in the compiler output.

"S"(&((char *)key)[branch]) does indeed seem to do the right thing, at least with GCC.

I probably didn't help by bikeshedding the way that expression was written, apologies for that. It's orthogonal to what this patch is doing.

Quoted 68 lines

...

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 2 lines

If there's another reason why "S" is advantageous though, I'm happy to be corrected.

I remember that Ard has an RFC
https://lore.kernel.org/linux-arm-kernel/[email protected]/
"[RFC PATCH 0/2] arm64: use PIE code generation for KASLR kernel" and see some recent PIE codegen patches.

Quoted 8 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Building the KASLR kernel without -fpie but linking it with -pie works in practice, but it is not something that is explicitly supported by the toolchains - it happens to work because the default 'small' code model used by both GCC and Clang relies mostly on ADRP+ADD/LDR to generate symbol references.

Does this mean that doing boot-time relocation on a fully statically linked kernel doesn't bring much benefit?

Not sure I follow you here. The boot-time relocation is necessary in any case, to fix up statically initialized pointer variables all over the kernel.

Quoted 5 lines

It would tend to be more
painful to do the relocation work, and mean that the kernel text has to be temporarily writeable, but static linking should have the best runtime performance.

Not sure what you mean by 'static linking' here.

The only thing PIE linking does in the case of the kernel is
a) complain if static relocations are used that cannot be fixed up at runtime (e.g., movk/movz sequences)
b) emit dynamic relocations that the early startup code can use to fix up all statically initialized pointers

From a codegen point-of-view, there is no difference because we don't use -fpic code. The problem with -fpic codegen is that it makes some assumptions that only hold for shared ELF objects, e.g., avoiding dynamic relocations in .text, using GOT entries even for variables defined in the same compilation so that they can be preempted, keeping all runtime relocatable quantities together so the CoW footprint of the shared library is minimized.

None of this matters for the kernel, and as it turns out, the non-PIC small C model produces code that the PIE linker is happy with.

TL;DR our code (including inline and out-of-line asm) is already PIC, and this property is relied upon by KASLR.

Quoted 4 lines

Maybe it doesn't make as much difference as I thought (certainly ADRP based addressing should make a fair amount of the PIC/PIE overhead go away).

The PIC/PIE overhead is in the indirections via the GOT. However, recent linkers will actually perform some relaxations too: if a symbol is defined in the same executable and is not preemptible, a GOT load

ADRP
LDR

can be transparently converted to

ADRP
ADD

Quoted 3 lines

referring to the symbol directly. This is also where hidden.h comes
in: making all symbol declarations and definitions 'hidden' will allow the compiler to assume that a GOT entry is never needed.

Is there a reason why we don't just build the whole kernel with -fvisibility=hidden today?

Quoted 6 lines

So building with -fPIC is currently not need in practice, and creates some complications, which is why we have been avoiding it. But PIE linking non-PIC objects is not guaranteed to remain supported going forward, so it would be better to have a plan B, i.e., being able to turn on -fpic without massive regressions due to GOT overhead, or codegen problems with our asm hacks.

Summarising all of this is it right that:

1) ld -pie is how we get the reloc info into the kernel for KASLR today.

2) We use gcc -fno-pic today, but this might break with ld -pie in the future, although it works for now.

3) gcc -fno-pic and gcc -fPIC (or -fPIE) generate almost the same code, assuming we tweak symbol visibility and use a memory model that ADR+ADD/LDR can span. So, moving to -fPIE is likely to be do-able.

My point is that an alternative option would be to move to ld -no-pie. We would need another way to get relocs into the kernel, such as an intermediate step with ld --emit-relocs. I have definitely seen this done before somewhere, but it would be extra work and not necessarily worth it, based on what you say about code gen.

This may all have been discussed to death already -- I didn't mean to hijack this patch, so I'll stop digging here, but if I've misunderstood, please shout!

Cheers
---Dave

Quoted 42 lines · Dave Martin

Quoted 40 lines · Ard Biesheuvel

Quoted 2 lines · Dave Martin

On Thu, Feb 01, 2024 at 01:11:20AM -0800, Fangrui Song wrote:

...

Quoted 29 lines

Quoted 19 lines

Hmm. Clang's constraint "S" implementation doesn't support a constant offset, so
static_key_false(&nf_hooks_needed[pf][hook]) in include/linux/netfilter.h:nf_hook would not compile with Clang <= 18.

I have a patch https://github.com/llvm/llvm-project/pull/80255 , but even if it is accepted and cherry-picked into the 18.x release branch, if we still support older Clang, we cannot use "S" unconditionally.

So we probably need the following to prepare for -fPIE support in the future:

arch/arm64/include/asm/jump_label.h +0 -0

index 48ddc0f45d22..b8af2f8b0c99 100644

arch/arm64/include/asm/jump_label.h line 20

Quoted 6 lines

arch/arm64/include/asm/jump_label.h +5 -0

@@ -15,6 +15,16 @@
 #define JUMP_LABEL_NOP_SIZE          AARCH64_INSN_SIZE
+/*
+ * Prefer "S" to support PIC. However, use "i" for Clang 18 and earlier as "S"
+ * on a symbol with a constant offset is not supported.
+ */
+#if defined(CONFIG_CC_IS_CLANG) && __clang_major__ <= 18

Is a GCC version check needed? Or is the minimum GCC version specified for building the kernel new enough?

Quoted 5 lines

arch/arm64/include/asm/jump_label.h line 27

Quoted 5 lines

arch/arm64/include/asm/jump_label.h +5 -0

+#define JUMP_LABEL_STATIC_KEY_CONSTRAINT "i"
+#else
+#define JUMP_LABEL_STATIC_KEY_CONSTRAINT "S"
+#endif
+

Can we use "Si" instead?

Quoted 2 lines

I thought the point was to avoid "S" on compilers that would choke on it? If so, those compilers would surely choke on "Si" too, no?

"Si" is an invalid constraint. Unfortunately, GCC recognizes "S" as a valid constraint and ignores trailing characters (asm_operand_ok). That is, GCC would accept "Siiiii" as well...

The GCC support for "S" is great. The initial aarch64 port in 2012 supports "S".

Quoted 31 lines

Quoted 16 lines

Quoted 16 lines

arch/arm64/include/asm/jump_label.h static __always_inline bool arch_static_branch(struct static_key * const key, · line 38

Quoted 12 lines

arch/arm64/include/asm/jump_label.h +2 -2

 static __always_inline bool arch_static_branch(struct static_key * const key,
                                             const bool branch)
 {

@@ -23,9 +33,9 @@ static __always_inline bool arch_static_branch(struct static_key * const key,
               "      .pushsection    __jump_table, \"aw\"    \n\t"
               "      .align          3                       \n\t"
               "      .long           1b - ., %l[l_yes] - .   \n\t"
-              "      .quad           %c0 - .                 \n\t"
+              "      .quad           %0 + %1 - .             \n\t"
               "      .popsection                             \n\t"
-              :  :  "i"(&((char *)key)[branch]) :  : l_yes);
+              :  :  JUMP_LABEL_STATIC_KEY_CONSTRAINT(key), "i"(branch) :  : l_yes);

While this looks reasonable, I'm still not clear now why the asm must do the addition.

Quoted 8 lines

Quoted 5 lines

Can we roll the branch argument into the macro?

"S"(symbol + constant) seems to do the right thing for AArch64, at least in GCC.

'symbol' is a struct static_key pointer, so adding '1' to it will not produce the needed result.

I mean loosely things that resolve to something of the form "symbol + constant" in the compiler output.

"S"(&((char *)key)[branch]) does indeed seem to do the right thing, at least with GCC.

Quoted 3 lines

I probably didn't help by bikeshedding the way that expression was written, apologies for that. It's orthogonal to what this patch is doing.

Yes, "S"(&((char *)key)[branch]) would do the right thing.
I have compared assembly output. It's a matter of "s" vs "s + 0" and "s+1" vs "s + 1".

Quoted 38 lines

Quoted 38 lines

The only thing PIE linking does in the case of the kernel is
a) complain if static relocations are used that cannot be fixed up at runtime (e.g., movk/movz sequences)
b) emit dynamic relocations that the early startup code can use to fix up all statically initialized pointers

From a codegen point-of-view, there is no difference because we don't use -fpic code. The problem with -fpic codegen is that it makes some assumptions that only hold for shared ELF objects, e.g., avoiding dynamic relocations in .text, using GOT entries even for variables defined in the same compilation so that they can be preempted, keeping all runtime relocatable quantities together so the CoW footprint of the shared library is minimized.

None of this matters for the kernel, and as it turns out, the non-PIC small C model produces code that the PIE linker is happy with.

TL;DR our code (including inline and out-of-line asm) is already PIC, and this property is relied upon by KASLR.

Quoted 4 lines

Maybe it doesn't make as much difference as I thought (certainly ADRP based addressing should make a fair amount of the PIC/PIE overhead go away).

The PIC/PIE overhead is in the indirections via the GOT. However, recent linkers will actually perform some relaxations too: if a symbol is defined in the same executable and is not preemptible, a GOT load

ADRP
LDR

can be transparently converted to

ADRP
ADD

Quoted 6 lines

Quoted 3 lines

referring to the symbol directly. This is also where hidden.h comes
in: making all symbol declarations and definitions 'hidden' will allow the compiler to assume that a GOT entry is never needed.

Is there a reason why we don't just build the whole kernel with -fvisibility=hidden today?

This topic, loosely related to this patch, is about switching to PIC compiles. I am not familiar with the Linux kernel, so I'll mostly leave the discussion to you and Ard :)

That said, I have done a lot of Clang work in visibility/symbol preemptibility and linkers, so I am happy to add what I know.

-fvisibility=hidden only applies to definitions, not non-definition declarations.

I've mentioned this at
https://lore.kernel.org/all/[email protected]/

#pragma GCC visibility push(hidden) is very similar to -fvisibility=hidden -fdirect-access-external-data with Clang.
...
The kernel uses neither TLS nor -fno-plt, so -fvisibility=hidden
-fdirect-access-external-data can replace `#pragma GCC visibility
push(hidden)`.

Quoted 21 lines

Quoted 6 lines

So building with -fPIC is currently not need in practice, and creates some complications, which is why we have been avoiding it. But PIE linking non-PIC objects is not guaranteed to remain supported going forward, so it would be better to have a plan B, i.e., being able to turn on -fpic without massive regressions due to GOT overhead, or codegen problems with our asm hacks.

Summarising all of this is it right that:

1) ld -pie is how we get the reloc info into the kernel for KASLR today.

2) We use gcc -fno-pic today, but this might break with ld -pie in the future, although it works for now.

3) gcc -fno-pic and gcc -fPIC (or -fPIE) generate almost the same code, assuming we tweak symbol visibility and use a memory model that ADR+ADD/LDR can span. So, moving to -fPIE is likely to be do-able.

My point is that an alternative option would be to move to ld -no-pie.

Quoted 4 lines

We would need another way to get relocs into the kernel, such as an intermediate step with ld --emit-relocs. I have definitely seen this done before somewhere, but it would be extra work and not necessarily worth it, based on what you say about code gen.

Quoted 4 lines

This may all have been discussed to death already -- I didn't mean to hijack this patch, so I'll stop digging here, but if I've misunderstood, please shout!

Quoted 51 lines · Fangrui Song

Quoted 42 lines · Dave Martin

Quoted 33 lines · Ard Biesheuvel

Quoted 2 lines · Dave Martin

On Thu, Feb 01, 2024 at 01:11:20AM -0800, Fangrui Song wrote:

...

Quoted 29 lines

Quoted 19 lines

Hmm. Clang's constraint "S" implementation doesn't support a constant offset, so
static_key_false(&nf_hooks_needed[pf][hook]) in include/linux/netfilter.h:nf_hook would not compile with Clang <= 18.

I have a patch https://github.com/llvm/llvm-project/pull/80255 , but even if it is accepted and cherry-picked into the 18.x release branch, if we still support older Clang, we cannot use "S" unconditionally.

So we probably need the following to prepare for -fPIE support in the future:

arch/arm64/include/asm/jump_label.h +0 -0

index 48ddc0f45d22..b8af2f8b0c99 100644

arch/arm64/include/asm/jump_label.h line 20

Quoted 6 lines

arch/arm64/include/asm/jump_label.h +5 -0

@@ -15,6 +15,16 @@
 #define JUMP_LABEL_NOP_SIZE          AARCH64_INSN_SIZE
+/*
+ * Prefer "S" to support PIC. However, use "i" for Clang 18 and earlier as "S"
+ * on a symbol with a constant offset is not supported.
+ */
+#if defined(CONFIG_CC_IS_CLANG) && __clang_major__ <= 18

Is a GCC version check needed? Or is the minimum GCC version specified for building the kernel new enough?

Quoted 7 lines

Quoted 5 lines

arch/arm64/include/asm/jump_label.h line 27

Quoted 5 lines

arch/arm64/include/asm/jump_label.h +5 -0

+#define JUMP_LABEL_STATIC_KEY_CONSTRAINT "i"
+#else
+#define JUMP_LABEL_STATIC_KEY_CONSTRAINT "S"
+#endif
+

Can we use "Si" instead?

Quoted 2 lines

I thought the point was to avoid "S" on compilers that would choke on it? If so, those compilers would surely choke on "Si" too, no?

"Si" is an invalid constraint. Unfortunately, GCC recognizes "S" as a valid constraint and ignores trailing characters (asm_operand_ok). That is, GCC would accept "Siiiii" as well...

Quoted 2 lines

The GCC support for "S" is great. The initial aarch64 port in 2012 supports "S".

So it is not possible to combine the S and i constraint, and let the compiler figure out whether it meets either? We rely on this elsewhere by combining r and Z into rZ. x86 uses "rm" for inline asm parameters that could be either register or memory operands.

Quoted 32 lines

Quoted 32 lines

Quoted 17 lines

Quoted 17 lines

arch/arm64/include/asm/jump_label.h static __always_inline bool arch_static_branch(struct static_key * const key, · line 38

Quoted 13 lines

arch/arm64/include/asm/jump_label.h +2 -2

 static __always_inline bool arch_static_branch(struct static_key * const key,
                                             const bool branch)
 {

@@ -23,9 +33,9 @@ static __always_inline bool arch_static_branch(struct static_key * const key,
               "      .pushsection    __jump_table, \"aw\"    \n\t"
               "      .align          3                       \n\t"
               "      .long           1b - ., %l[l_yes] - .   \n\t"
-              "      .quad           %c0 - .                 \n\t"
+              "      .quad           %0 + %1 - .             \n\t"
               "      .popsection                             \n\t"
-              :  :  "i"(&((char *)key)[branch]) :  : l_yes);
+              :  :  JUMP_LABEL_STATIC_KEY_CONSTRAINT(key), "i"(branch) :  : l_yes);

While this looks reasonable, I'm still not clear now why the asm must do the addition.

Quoted 8 lines

Quoted 5 lines

Can we roll the branch argument into the macro?

"S"(symbol + constant) seems to do the right thing for AArch64, at least in GCC.

'symbol' is a struct static_key pointer, so adding '1' to it will not produce the needed result.

I mean loosely things that resolve to something of the form "symbol + constant" in the compiler output.

"S"(&((char *)key)[branch]) does indeed seem to do the right thing, at least with GCC.

Quoted 7 lines

Quoted 3 lines

I probably didn't help by bikeshedding the way that expression was written, apologies for that. It's orthogonal to what this patch is doing.

Yes, "S"(&((char *)key)[branch]) would do the right thing.
I have compared assembly output. It's a matter of "s" vs "s + 0" and "s+1" vs "s + 1".

But if S does not work with constant offsets with older Clang, what is the harm in splitting it into "S" for the symbol and "i" for the branch?
...

Quoted 55 lines

Quoted 38 lines

Quoted 38 lines

The only thing PIE linking does in the case of the kernel is
a) complain if static relocations are used that cannot be fixed up at runtime (e.g., movk/movz sequences)
b) emit dynamic relocations that the early startup code can use to fix up all statically initialized pointers

From a codegen point-of-view, there is no difference because we don't use -fpic code. The problem with -fpic codegen is that it makes some assumptions that only hold for shared ELF objects, e.g., avoiding dynamic relocations in .text, using GOT entries even for variables defined in the same compilation so that they can be preempted, keeping all runtime relocatable quantities together so the CoW footprint of the shared library is minimized.

None of this matters for the kernel, and as it turns out, the non-PIC small C model produces code that the PIE linker is happy with.

TL;DR our code (including inline and out-of-line asm) is already PIC, and this property is relied upon by KASLR.

Quoted 4 lines

Maybe it doesn't make as much difference as I thought (certainly ADRP based addressing should make a fair amount of the PIC/PIE overhead go away).

The PIC/PIE overhead is in the indirections via the GOT. However, recent linkers will actually perform some relaxations too: if a symbol is defined in the same executable and is not preemptible, a GOT load

ADRP
LDR

can be transparently converted to

ADRP
ADD

Quoted 6 lines

Quoted 3 lines

referring to the symbol directly. This is also where hidden.h comes
in: making all symbol declarations and definitions 'hidden' will allow the compiler to assume that a GOT entry is never needed.

Is there a reason why we don't just build the whole kernel with -fvisibility=hidden today?

This topic, loosely related to this patch, is about switching to PIC compiles. I am not familiar with the Linux kernel, so I'll mostly leave the discussion to you and Ard :)

That said, I have done a lot of Clang work in visibility/symbol preemptibility and linkers, so I am happy to add what I know.

-fvisibility=hidden only applies to definitions, not non-definition declarations.

Quoted 30 lines

I've mentioned this at
https://lore.kernel.org/all/[email protected]/

#pragma GCC visibility push(hidden) is very similar to -fvisibility=hidden -fdirect-access-external-data with Clang.
...
The kernel uses neither TLS nor -fno-plt, so -fvisibility=hidden
-fdirect-access-external-data can replace `#pragma GCC visibility
push(hidden)`.

Quoted 21 lines

Quoted 6 lines

So building with -fPIC is currently not need in practice, and creates some complications, which is why we have been avoiding it. But PIE linking non-PIC objects is not guaranteed to remain supported going forward, so it would be better to have a plan B, i.e., being able to turn on -fpic without massive regressions due to GOT overhead, or codegen problems with our asm hacks.

Summarising all of this is it right that:

1) ld -pie is how we get the reloc info into the kernel for KASLR today.

2) We use gcc -fno-pic today, but this might break with ld -pie in the future, although it works for now.

3) gcc -fno-pic and gcc -fPIC (or -fPIE) generate almost the same code, assuming we tweak symbol visibility and use a memory model that ADR+ADD/LDR can span. So, moving to -fPIE is likely to be do-able.

My point is that an alternative option would be to move to ld -no-pie.

Why? What would that achieve?

Quoted 4 lines

Quoted 4 lines

We would need another way to get relocs into the kernel, such as an intermediate step with ld --emit-relocs. I have definitely seen this done before somewhere, but it would be extra work and not necessarily worth it, based on what you say about code gen.

Relying on --emit-relocs is a terrible hack. It contains mostly relocations that we don't need at runtime, so we need to postprocess them. And linkers may not update these relocations in some cases after relaxation or other transformations (and sometimes, it is not even possible to do so, if the post-transformation situation cannot be described by a relocation).

And with Fangrui's RELR optimization, the PIE relocation data is extremely dense.

There is nothing to fix here, really.

Quoted 4 lines

Quoted 4 lines

This may all have been discussed to death already -- I didn't mean to hijack this patch, so I'll stop digging here, but if I've misunderstood, please shout!

No worries. Ramana and I spent some time 5+ years ago to document how PIC codegen for the kernel (or any freestanding executable, really) deviates from the typical PIC codegen for shared libraries and PIE executables, with the intention of adding a GCC compiler switch for it, but that never really materialized, mostly because things just work fine today.

Quoted 8 lines · Ard Biesheuvel

Quoted 6 lines · Fangrui Song

Quoted 4 lines · Dave Martin

Quoted 3 lines · Ard Biesheuvel

Quoted 2 lines · Dave Martin

On Thu, Feb 01, 2024 at 01:11:20AM -0800, Fangrui Song wrote:

Quoted 11 lines

Quoted 11 lines

Quoted 11 lines

Quoted 10 lines

Quoted 8 lines

Is a GCC version check needed? Or is the minimum GCC version specified for building the kernel new enough?

Review

Quoted 5 lines

(quoted diff) +5 -0

+#define JUMP_LABEL_STATIC_KEY_CONSTRAINT "i"
+#else
+#define JUMP_LABEL_STATIC_KEY_CONSTRAINT "S"
+#endif
+

Can we use "Si" instead?

Quoted 8 lines

Quoted 8 lines

Quoted 2 lines

I thought the point was to avoid "S" on compilers that would choke on it? If so, those compilers would surely choke on "Si" too, no?

"Si" is an invalid constraint. Unfortunately, GCC recognizes "S" as a valid constraint and ignores trailing characters (asm_operand_ok). That is, GCC would accept "Siiiii" as well...

The GCC support for "S" is great. The initial aarch64 port in 2012 supports "S".

Quoted 4 lines

So it is not possible to combine the S and i constraint, and let the compiler figure out whether it meets either? We rely on this elsewhere by combining r and Z into rZ. x86 uses "rm" for inline asm parameters that could be either register or memory operands.

I did not realize that your "Si" suggestion meant this feature
https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html

After the prefix, there must be one or more additional constraints
(see Constraints for asm Operands) that describe where the value resides. Common constraints include ‘r’ for register and ‘m’ for memory. When you list more than one possible location (for example, "=rm"), the compiler chooses the most efficient one based on the current context.

After some debugging of both GCC and Clang, I think you are right and "Si" will work. Thanks for the suggestion!

GCC accepts "Si". Sorry for my incorrectly stating that GCC ignores the trailing constraint. GCC does ignore an unknown constraint, as long as one accepted constraint.

Clang accepts "Si" as well. In the absence of
https://github.com/llvm/llvm-project/pull/80255 (constant offset support for 'S'), in TargetLowering::ComputeConstraintToUse, 'S' is skipped and 'i' will be picked. This is unrelated to the "rm" weakness in Clang (https://github.com/llvm/llvm-project/issues/20571).

I'll post a PATCH v2 removing JUMP_LABEL_STATIC_KEY_CONSTRAINT and using "Si" unconditionally.

Quoted 12 lines

Quoted 12 lines

Quoted 12 lines

Quoted 12 lines

Quoted 12 lines

Quoted 12 lines

static __always_inline bool arch_static_branch(struct static_key * const key,
const bool branch)
{

(quoted diff) +2 -2

@@ -23,9 +33,9 @@ static __always_inline bool arch_static_branch(struct static_key * const key,
               "      .pushsection    __jump_table, \"aw\"    \n\t"
               "      .align          3                       \n\t"
               "      .long           1b - ., %l[l_yes] - .   \n\t"
-              "      .quad           %c0 - .                 \n\t"
+              "      .quad           %0 + %1 - .             \n\t"
               "      .popsection                             \n\t"
-              :  :  "i"(&((char *)key)[branch]) :  : l_yes);
+              :  :  JUMP_LABEL_STATIC_KEY_CONSTRAINT(key), "i"(branch) :  : l_yes);

Quoted 9 lines

Quoted 9 lines

Quoted 6 lines

"S"(&((char *)key)[branch]) does indeed seem to do the right thing, at least with GCC.

I probably didn't help by bikeshedding the way that expression was written, apologies for that. It's orthogonal to what this patch is doing.

Yes, "S"(&((char *)key)[branch]) would do the right thing.
I have compared assembly output. It's a matter of "s" vs "s + 0" and "s+1" vs "s + 1".

Quoted 51 lines

Quoted 13 lines

Quoted 2 lines

Is there a reason why we don't just build the whole kernel with -fvisibility=hidden today?

This topic, loosely related to this patch, is about switching to PIC compiles. I am not familiar with the Linux kernel, so I'll mostly leave the discussion to you and Ard :)

That said, I have done a lot of Clang work in visibility/symbol preemptibility and linkers, so I am happy to add what I know.

-fvisibility=hidden only applies to definitions, not non-definition declarations.

Quoted 30 lines

I've mentioned this at
https://lore.kernel.org/all/[email protected]/

#pragma GCC visibility push(hidden) is very similar to -fvisibility=hidden -fdirect-access-external-data with Clang.
...
The kernel uses neither TLS nor -fno-plt, so -fvisibility=hidden
-fdirect-access-external-data can replace `#pragma GCC visibility
push(hidden)`.

Quoted 21 lines

Quoted 6 lines

So building with -fPIC is currently not need in practice, and creates some complications, which is why we have been avoiding it. But PIE linking non-PIC objects is not guaranteed to remain supported going forward, so it would be better to have a plan B, i.e., being able to turn on -fpic without massive regressions due to GOT overhead, or codegen problems with our asm hacks.

Summarising all of this is it right that:

1) ld -pie is how we get the reloc info into the kernel for KASLR today.

2) We use gcc -fno-pic today, but this might break with ld -pie in the future, although it works for now.

3) gcc -fno-pic and gcc -fPIC (or -fPIE) generate almost the same code, assuming we tweak symbol visibility and use a memory model that ADR+ADD/LDR can span. So, moving to -fPIE is likely to be do-able.

My point is that an alternative option would be to move to ld -no-pie.

Why? What would that achieve?

Quoted 4 lines

Quoted 4 lines

We would need another way to get relocs into the kernel, such as an intermediate step with ld --emit-relocs. I have definitely seen this done before somewhere, but it would be extra work and not necessarily worth it, based on what you say about code gen.

Quoted 6 lines

Relying on --emit-relocs is a terrible hack. It contains mostly relocations that we don't need at runtime, so we need to postprocess them. And linkers may not update these relocations in some cases after relaxation or other transformations (and sometimes, it is not even possible to do so, if the post-transformation situation cannot be described by a relocation).

Agreed. I've filed
https://sourceware.org/bugzilla/show_bug.cgi?id=30844
("ld riscv: --emit-relocs does not retain the original relocation type") for a RISC-V version.

--emit-relocs is under-specified and the linker behaviors are less tested.

Quoted 10 lines

And with Fangrui's RELR optimization, the PIE relocation data is extremely dense.

There is nothing to fix here, really.

Quoted 4 lines

Quoted 4 lines

This may all have been discussed to death already -- I didn't mean to hijack this patch, so I'll stop digging here, but if I've misunderstood, please shout!

Quoted 7 lines

No worries. Ramana and I spent some time 5+ years ago to document how PIC codegen for the kernel (or any freestanding executable, really) deviates from the typical PIC codegen for shared libraries and PIE executables, with the intention of adding a GCC compiler switch for it, but that never really materialized, mostly because things just work fine today.

Quoted 9 lines · Fangrui Song

Quoted 8 lines · Ard Biesheuvel

Quoted 6 lines · Fangrui Song

Quoted 4 lines · Dave Martin

Quoted 3 lines · Ard Biesheuvel

Quoted 2 lines · Dave Martin

On Thu, Feb 01, 2024 at 01:11:20AM -0800, Fangrui Song wrote:

[...]

Quoted 11 lines

Quoted 11 lines

Quoted 11 lines

Quoted 11 lines

Quoted 10 lines

Quoted 8 lines

Is a GCC version check needed? Or is the minimum GCC version specified for building the kernel new enough?

Review

Quoted 5 lines

(quoted diff) +5 -0

+#define JUMP_LABEL_STATIC_KEY_CONSTRAINT "i"
+#else
+#define JUMP_LABEL_STATIC_KEY_CONSTRAINT "S"
+#endif
+

Can we use "Si" instead?

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 2 lines

I thought the point was to avoid "S" on compilers that would choke on it? If so, those compilers would surely choke on "Si" too, no?

"Si" is an invalid constraint. Unfortunately, GCC recognizes "S" as a valid constraint and ignores trailing characters (asm_operand_ok). That is, GCC would accept "Siiiii" as well...

The GCC support for "S" is great. The initial aarch64 port in 2012 supports "S".

Ah, no problem.

Quoted 22 lines

Quoted 4 lines

So it is not possible to combine the S and i constraint, and let the compiler figure out whether it meets either? We rely on this elsewhere by combining r and Z into rZ. x86 uses "rm" for inline asm parameters that could be either register or memory operands.

I did not realize that your "Si" suggestion meant this feature
https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html

After the prefix, there must be one or more additional constraints
(see Constraints for asm Operands) that describe where the value resides. Common constraints include ‘r’ for register and ‘m’ for memory. When you list more than one possible location (for example, "=rm"), the compiler chooses the most efficient one based on the current context.

After some debugging of both GCC and Clang, I think you are right and "Si" will work. Thanks for the suggestion!

GCC accepts "Si". Sorry for my incorrectly stating that GCC ignores the trailing constraint. GCC does ignore an unknown constraint, as long as one accepted constraint.

Quoted 8 lines

Clang accepts "Si" as well. In the absence of
https://github.com/llvm/llvm-project/pull/80255 (constant offset support for 'S'), in TargetLowering::ComputeConstraintToUse, 'S' is skipped and 'i' will be picked. This is unrelated to the "rm" weakness in Clang (https://github.com/llvm/llvm-project/issues/20571).

I'll post a PATCH v2 removing JUMP_LABEL_STATIC_KEY_CONSTRAINT and using "Si" unconditionally.

If older Clang just ignores the "S" in "Si" as an unsatisfiable alternative and falls back on "i", that seems ideal.

Quoted 12 lines

Quoted 12 lines

Quoted 12 lines

Quoted 12 lines

Quoted 12 lines

Quoted 12 lines

Quoted 12 lines

static __always_inline bool arch_static_branch(struct static_key * const key,
const bool branch)
{

(quoted diff) +2 -2

@@ -23,9 +33,9 @@ static __always_inline bool arch_static_branch(struct static_key * const key,
               "      .pushsection    __jump_table, \"aw\"    \n\t"
               "      .align          3                       \n\t"
               "      .long           1b - ., %l[l_yes] - .   \n\t"
-              "      .quad           %c0 - .                 \n\t"
+              "      .quad           %0 + %1 - .             \n\t"
               "      .popsection                             \n\t"
-              :  :  "i"(&((char *)key)[branch]) :  : l_yes);
+              :  :  JUMP_LABEL_STATIC_KEY_CONSTRAINT(key), "i"(branch) :  : l_yes);

[...]

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

Quoted 6 lines

"S"(&((char *)key)[branch]) does indeed seem to do the right thing, at least with GCC.

I probably didn't help by bikeshedding the way that expression was written, apologies for that. It's orthogonal to what this patch is doing.

Yes, "S"(&((char *)key)[branch]) would do the right thing.
I have compared assembly output. It's a matter of "s" vs "s + 0" and "s+1" vs "s + 1".

[...]

Quoted 58 lines

Quoted 51 lines

Quoted 13 lines

Quoted 2 lines

Is there a reason why we don't just build the whole kernel with -fvisibility=hidden today?

This topic, loosely related to this patch, is about switching to PIC compiles. I am not familiar with the Linux kernel, so I'll mostly leave the discussion to you and Ard :)

That said, I have done a lot of Clang work in visibility/symbol preemptibility and linkers, so I am happy to add what I know.

-fvisibility=hidden only applies to definitions, not non-definition declarations.

Quoted 30 lines

I've mentioned this at
https://lore.kernel.org/all/[email protected]/

#pragma GCC visibility push(hidden) is very similar to -fvisibility=hidden -fdirect-access-external-data with Clang.
...
The kernel uses neither TLS nor -fno-plt, so -fvisibility=hidden
-fdirect-access-external-data can replace `#pragma GCC visibility
push(hidden)`.

Quoted 21 lines

Quoted 6 lines

So building with -fPIC is currently not need in practice, and creates some complications, which is why we have been avoiding it. But PIE linking non-PIC objects is not guaranteed to remain supported going forward, so it would be better to have a plan B, i.e., being able to turn on -fpic without massive regressions due to GOT overhead, or codegen problems with our asm hacks.

Summarising all of this is it right that:

1) ld -pie is how we get the reloc info into the kernel for KASLR today.

2) We use gcc -fno-pic today, but this might break with ld -pie in the future, although it works for now.

3) gcc -fno-pic and gcc -fPIC (or -fPIE) generate almost the same code, assuming we tweak symbol visibility and use a memory model that ADR+ADD/LDR can span. So, moving to -fPIE is likely to be do-able.

My point is that an alternative option would be to move to ld -no-pie.

Why? What would that achieve?

Quoted 4 lines

Quoted 4 lines

We would need another way to get relocs into the kernel, such as an intermediate step with ld --emit-relocs. I have definitely seen this done before somewhere, but it would be extra work and not necessarily worth it, based on what you say about code gen.

Quoted 6 lines

Relying on --emit-relocs is a terrible hack. It contains mostly relocations that we don't need at runtime, so we need to postprocess them. And linkers may not update these relocations in some cases after relaxation or other transformations (and sometimes, it is not even possible to do so, if the post-transformation situation cannot be described by a relocation).

Quoted 6 lines

Agreed. I've filed
https://sourceware.org/bugzilla/show_bug.cgi?id=30844
("ld riscv: --emit-relocs does not retain the original relocation type") for a RISC-V version.

--emit-relocs is under-specified and the linker behaviors are less tested.

(Aside: This sounds like an interesting hole in the toolchain world.

I guess if you want post-link-time relocatability, it sounds like all the focus from the compiler folks is on PIC code, and non-PIC linker output is not considered relocatable at all (baremetal software being an inconvenient truth).

"Relocatable" and "PIC" are not really the same requirement, but the importance of the difference is arch-dependent, and I can see how we got here. End aside.)

Quoted 10 lines

Quoted 10 lines

And with Fangrui's RELR optimization, the PIE relocation data is extremely dense.

There is nothing to fix here, really.

Quoted 4 lines

Quoted 4 lines

This may all have been discussed to death already -- I didn't mean to hijack this patch, so I'll stop digging here, but if I've misunderstood, please shout!

Quoted 7 lines

Quoted 7 lines

No worries. Ramana and I spent some time 5+ years ago to document how PIC codegen for the kernel (or any freestanding executable, really) deviates from the typical PIC codegen for shared libraries and PIE executables, with the intention of adding a GCC compiler switch for it, but that never really materialized, mostly because things just work fine today.

Right. I was concerned that we would pay a significant runtime performance penalty when migrating to -fPIC, in which case it would be worth considering alernatives. But from the discussion it sounds like this concern is misguided (or at least, you have found robust workarounds which mean that the real penalty is negligible).

There's no underlying reason why a toolchain couldn't emit full static relocation information in a robust way, but since this info is hard to consume and would be used rarely and only for very specific use cases I can see why this support is rot-prone or even not considered a goal at all by many people...

Cheers
---Dave