Re: [PATCH] arm64: jump_label: use constraint "S" instead of "i"
From: Fangrui Song <hidden>
Date: 2024-02-01 09:11:25
Also in:
lkml, llvm
Subsystem:
arm64 port (aarch64 architecture), static branch/call, the rest · Maintainers:
Catalin Marinas, Will Deacon, Peter Zijlstra, Josh Poimboeuf, Jason Baron, Alice Ryhl, Linus Torvalds
On 2024-02-01, Ard Biesheuvel wrote:
On Thu, 1 Feb 2024 at 05:55, Fangrui Song [off-list ref] wrote:quoted
On 2024-01-31, Dave Martin wrote:quoted
On Wed, Jan 31, 2024 at 08:16:04AM +0100, Ard Biesheuvel wrote:quoted
Hello Fangrui, On Wed, 31 Jan 2024 at 07:53, Fangrui Song [off-list ref] wrote:quoted
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.)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
quoted
quoted
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 <redacted> 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(-)diff --git a/arch/arm64/include/asm/jump_label.h b/arch/arm64/include/asm/jump_label.h index 48ddc0f45d22..31862b3bb33d 100644 --- a/arch/arm64/include/asm/jump_label.h +++ b/arch/arm64/include/asm/jump_label.h@@ -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.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).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:
diff --git a/arch/arm64/include/asm/jump_label.h b/arch/arm64/include/asm/jump_label.h
index 48ddc0f45d22..b8af2f8b0c99 100644
--- a/arch/arm64/include/asm/jump_label.h
+++ b/arch/arm64/include/asm/jump_label.h@@ -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 +#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) {
@@ -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); 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
quoted
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.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
quoted
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
quoted
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/20220427171241.2426592-1-ardb@kernel.org/ (local) "[RFC PATCH 0/2] arm64: use PIE code generation for KASLR kernel" and see some recent PIE codegen patches.quoted
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.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 ... _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel