[RFC PATCH 0/2] arm64: use PIE code generation for KASLR kernel

[Ard Biesheuvel]

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.

Code generation with -fpie used to result in unnecessary overhead, as
all references to symbols with external visibility use emitted via
entries in the GOT, resulting in an additional load from memory for each
global variable access.

However, we can now manage this my using 'hidden' visibility (which is
already used in places such as the decompressor or the EFI stub), so we
can enable -fpie code generation without the overhead.

This series is RFC given that, beyond switching to a better supported
combination of compiler and linker options, I am not aware of any
advantages or disadvantages of making this change.

Cc: Sami Tolvanen <samito...@google.com>
Cc: Nick Desaulniers <ndesau...@google.com>

Ard Biesheuvel (2):
arm64: jump_label: use more precise asm constraints
arm64: kernel: switch to PIE code generation for relocatable kernels

arch/arm64/Makefile | 4 ++++
arch/arm64/include/asm/jump_label.h | 8 ++++----
arch/arm64/kernel/vmlinux.lds.S | 9 ++++-----
3 files changed, 12 insertions(+), 9 deletions(-)

--
2.30.2

[Ard Biesheuvel]

In order to set bit #0 of the struct static_key pointer in the the jump
label struct, we currently cast the pointer to char[], and take the
address of either the 0th or 1st array member, depending on the value of
'branch'. This works but creates problems with -fpie code generation:
GCC complains about the constraint being unsatisfiable, and Clang
miscompiles the code in a way that causes stability issues (immediate
panic on 'attempt to kill init')

So instead, pass the struct static_key reference and the 'branch'
immediate individually, in a way that satisfies both GCC and Clang (GCC
wants the 'S' constraint, whereas Clang wants the 'i' constraint for
argument %0)

Signed-off-by: Ard Biesheuvel <ar...@kernel.org>
---
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 cea441b6aa5d..f741bbacf175 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 *key,
" .pushsection __jump_table, \"aw\" \n\t"
" .align 3 \n\t"
" .long 1b - ., %l[l_yes] - . \n\t"
- " .quad %c0 - . \n\t"
+ " .quad %c0 - . + %1 \n\t"
" .popsection \n\t"
- : : "i"(&((char *)key)[branch]) : : l_yes);
+ : : "Si"(key), "i"(branch) : : l_yes);

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

return false;
l_yes:
--
2.30.2

[Ard Biesheuvel]

We currently use ordinary, position dependent code generation for the
core kernel, which happens to default to the 'small' code model on both
GCC and Clang. This is the code model that relies on ADRP/ADD or
ADRP/LDR pairs for symbol references, which are PC-relative with a range
of -/+ 4 GiB, and therefore happen to be position independent in
practice.

This means that the fact that we can link the relocatable KASLR kernel
using the -pie linker flag (which generates the runtime relocations and
inserts them into the binary) is somewhat of a coincidence, and not
something which is explicitly supported by the toolchains.

The reason we have not used -fpie for code generation so far (which is
the compiler flag that should be used to generate code that is to be
linked with -pie) is that by default, it generates code based on
assumptions that only hold for shared libraries and PIE executables,
i.e., that gathering all relocatable quantities into a Global Offset
Table (GOT) is desirable because it reduces the CoW footprint, and
because it permits ELF symbol preemption (which lets an executable
override symbols defined in a shared library, in a way that forces the
shared library to update all of its internal references as well).
Ironically, this means we end up with many more absolute references that
all need to be fixed up at boot.

Fortunately, we can convince the compiler to handle this in a way that
is a bit more suitable for freestanding binaries such as the kernel, by
setting the 'hidden' visibility #pragma, which informs the compiler that
symbol preemption or CoW footprint are of no concern to us, and so
PC-relative references that are resolved at link time are perfectly
fine.

So let's enable this #pragma and build with -fpie when building a
relocatable kernel. This also means that all constant data items that
carry statically initialized pointer variables are now emitted into the
.data.rel.ro* sections, so move these into .rodata where they belong.

Code size impact (GCC):

Before:

text data bss total filename
16712396 18659064 534556 35906016 vmlinux

After:

text data bss total filename
16804400 18612876 534556 35951832 vmlinux

Code size impact (Clang):

Before:

text data bss total filename
17194584 13335060 535268 31064912 vmlinux

After:

text data bss total filename
17194536 13310032 535268 31039836 vmlinux

Signed-off-by: Ard Biesheuvel <ar...@kernel.org>
---

arch/arm64/Makefile | 4 ++++
arch/arm64/kernel/vmlinux.lds.S | 9 ++++-----
2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/Makefile b/arch/arm64/Makefile
index 2f1de88651e6..94b6c51f5de6 100644
--- a/arch/arm64/Makefile
+++ b/arch/arm64/Makefile
@@ -18,6 +18,10 @@ ifeq ($(CONFIG_RELOCATABLE), y)
# with the relocation offsets always being zero.
LDFLAGS_vmlinux += -shared -Bsymbolic -z notext \
$(call ld-option, --no-apply-dynamic-relocs)
+
+# Generate position independent code without relying on a Global Offset Table
+KBUILD_CFLAGS_KERNEL += -fpie -include $(srctree)/include/linux/hidden.h
+
endif

ifeq ($(CONFIG_ARM64_ERRATUM_843419),y)
diff --git a/arch/arm64/kernel/vmlinux.lds.S b/arch/arm64/kernel/vmlinux.lds.S
index edaf0faf766f..b1e071ac1acf 100644
--- a/arch/arm64/kernel/vmlinux.lds.S
+++ b/arch/arm64/kernel/vmlinux.lds.S
@@ -174,8 +174,6 @@ SECTIONS
KEXEC_TEXT
TRAMP_TEXT
*(.gnu.warning)
- . = ALIGN(16);
- *(.got) /* Global offset table */
}

/*
@@ -192,6 +190,8 @@ SECTIONS
/* everything from this point to __init_begin will be marked RO NX */
RO_DATA(PAGE_SIZE)

+ .data.rel.ro : ALIGN(8) { *(.got) *(.data.rel.ro*) }
+
HYPERVISOR_DATA_SECTIONS

idmap_pg_dir = .;
@@ -273,6 +273,8 @@ SECTIONS
_sdata = .;
RW_DATA(L1_CACHE_BYTES, PAGE_SIZE, THREAD_ALIGN)

+ .data.rel : ALIGN(8) { *(.data.rel*) }
+
/*
* Data written with the MMU off but read with the MMU on requires
* cache lines to be invalidated, discarding up to a Cache Writeback
@@ -320,9 +322,6 @@ SECTIONS
*(.plt) *(.plt.*) *(.iplt) *(.igot .igot.plt)
}
ASSERT(SIZEOF(.plt) == 0, "Unexpected run-time procedure linkages detected!")
-
- .data.rel.ro : { *(.data.rel.ro) }
- ASSERT(SIZEOF(.data.rel.ro) == 0, "Unexpected RELRO detected!")
}

#include "image-vars.h"
--
2.30.2

[Nick Desaulniers]

On Wed, Apr 27, 2022 at 10:13 AM Ard Biesheuvel <ar...@kernel.org> wrote:
>
> In order to set bit #0 of the struct static_key pointer in the the jump
> label struct, we currently cast the pointer to char[], and take the
> address of either the 0th or 1st array member, depending on the value of
> 'branch'. This works but creates problems with -fpie code generation:
> GCC complains about the constraint being unsatisfiable, and Clang
> miscompiles the code in a way that causes stability issues (immediate
> panic on 'attempt to kill init')

Any more info on the "miscompile?" Perhaps worth an upstream bug report?

>
> So instead, pass the struct static_key reference and the 'branch'
> immediate individually, in a way that satisfies both GCC and Clang (GCC
> wants the 'S' constraint, whereas Clang wants the 'i' constraint for
> argument %0)

Anything we can do to improve Clang's handling of S constraints?
https://gcc.gnu.org/onlinedocs/gcc/Machine-Constraints.html#Machine-Constraints
>> An absolute symbolic address or a label reference

>
> Signed-off-by: Ard Biesheuvel <ar...@kernel.org>
> ---
> 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 cea441b6aa5d..f741bbacf175 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 *key,
> " .pushsection __jump_table, \"aw\" \n\t"
> " .align 3 \n\t"
> " .long 1b - ., %l[l_yes] - . \n\t"
> - " .quad %c0 - . \n\t"
> + " .quad %c0 - . + %1 \n\t"

If %1 is "i" constrained, then I think we can use the %c output
template for it as well?
https://gcc.gnu.org/onlinedocs/gccint/Output-Template.html

Is the expression clearer if we keep the `- .` at the end of each expression?

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

--
Thanks,
~Nick Desaulniers

[Ard Biesheuvel]

Hi Nick,

On Wed, 27 Apr 2022 at 20:59, Nick Desaulniers <ndesau...@google.com> wrote:
>
> On Wed, Apr 27, 2022 at 10:13 AM Ard Biesheuvel <ar...@kernel.org> wrote:
> >
> > In order to set bit #0 of the struct static_key pointer in the the jump
> > label struct, we currently cast the pointer to char[], and take the
> > address of either the 0th or 1st array member, depending on the value of
> > 'branch'. This works but creates problems with -fpie code generation:
> > GCC complains about the constraint being unsatisfiable, and Clang
> > miscompiles the code in a way that causes stability issues (immediate
> > panic on 'attempt to kill init')
>
> Any more info on the "miscompile?" Perhaps worth an upstream bug report?
>

I made very little progress trying to narrow it down: a segfault in
user space caused by who knows what. I'd file a bug if I knew how to
reproduce it.

> >
> > So instead, pass the struct static_key reference and the 'branch'
> > immediate individually, in a way that satisfies both GCC and Clang (GCC
> > wants the 'S' constraint, whereas Clang wants the 'i' constraint for
> > argument %0)
>
> Anything we can do to improve Clang's handling of S constraints?
> https://gcc.gnu.org/onlinedocs/gcc/Machine-Constraints.html#Machine-Constraints
> >> An absolute symbolic address or a label reference
>

"S" seems more appropriate than "i" for a symbol reference, and GCC
rejects "i" when using -fpie. But the actual literal being emitted is
a relative reference, not an absolute one. This likely needs more
discussion, but using "i" in the way we do now is definitely dodgy.

> >
> > Signed-off-by: Ard Biesheuvel <ar...@kernel.org>
> > ---
> > 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 cea441b6aa5d..f741bbacf175 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 *key,
> > " .pushsection __jump_table, \"aw\" \n\t"
> > " .align 3 \n\t"
> > " .long 1b - ., %l[l_yes] - . \n\t"
> > - " .quad %c0 - . \n\t"
> > + " .quad %c0 - . + %1 \n\t"
>
> If %1 is "i" constrained, then I think we can use the %c output
> template for it as well?
> https://gcc.gnu.org/onlinedocs/gccint/Output-Template.html
>

Is %c what prevents the leading # from being emitted? I'm not sure
whether that matters here, as AArch64 asm does not require it (and the
code builds fine with either compiler). But if it reflects the use
more precisely, I agree we should add it.

> Is the expression clearer if we keep the `- .` at the end of each expression?
>

I don't think so. The add sets the enabled bit, so I'd even be
inclined to write this as (%c0 - .) + %c1 to emphasize that this is a
relative reference with bit #0 set separately.

[Fangrui Song]

On 2022-04-27, Ard Biesheuvel wrote:
>We currently use ordinary, position dependent code generation for the
>core kernel, which happens to default to the 'small' code model on both
>GCC and Clang. This is the code model that relies on ADRP/ADD or
>ADRP/LDR pairs for symbol references, which are PC-relative with a range
>of -/+ 4 GiB, and therefore happen to be position independent in
>practice.
>
>This means that the fact that we can link the relocatable KASLR kernel
>using the -pie linker flag (which generates the runtime relocations and
>inserts them into the binary) is somewhat of a coincidence, and not
>something which is explicitly supported by the toolchains.

Agree. The current -fno-PIE + -shared -Bsymbolic combo works as a
conincidence, not guaranteed by the toolchain.

-shared needs -fpic object files. -shared -Bsymbolic is very similar to
-pie and therefore works with -fpie object files, but the usage is not
recommended from the toolchain perspective.

>The reason we have not used -fpie for code generation so far (which is
>the compiler flag that should be used to generate code that is to be
>linked with -pie) is that by default, it generates code based on
>assumptions that only hold for shared libraries and PIE executables,
>i.e., that gathering all relocatable quantities into a Global Offset
>Table (GOT) is desirable because it reduces the CoW footprint, and
>because it permits ELF symbol preemption (which lets an executable
>override symbols defined in a shared library, in a way that forces the
>shared library to update all of its internal references as well).
>Ironically, this means we end up with many more absolute references that
>all need to be fixed up at boot.

This is not about symbol preemption (when the executable and a shared
objectdefine the same symbol, which one wins). An executable using a GOT
which will be resolved to a shared object => this is regular relocation
resolving and there is no preemption.

It is that the compiler prefers code generation which can avoid text
relocations / copy relocations / canonical PLT entries
(https://maskray.me/blog/2021-01-09-copy-relocations-canonical-plt-entries-and-protected#summary).

>Fortunately, we can convince the compiler to handle this in a way that
>is a bit more suitable for freestanding binaries such as the kernel, by
>setting the 'hidden' visibility #pragma, which informs the compiler that
>symbol preemption or CoW footprint are of no concern to us, and so
>PC-relative references that are resolved at link time are perfectly
>fine.

Agree

>So let's enable this #pragma and build with -fpie when building a
>relocatable kernel. This also means that all constant data items that
>carry statically initialized pointer variables are now emitted into the
>.data.rel.ro* sections, so move these into .rodata where they belong.

LGTM, except: is ".rodata" a typo? The patch doesn't reference .rodata

>--
>You received this message because you are subscribed to the Google Groups "Clang Built Linux" group.
>To unsubscribe from this group and stop receiving emails from it, send an email to clang-built-li...@googlegroups.com.
>To view this discussion on the web visit https://groups.google.com/d/msgid/clang-built-linux/20220427171241.2426592-3-ardb%40kernel.org.

[Ard Biesheuvel]

On Thu, 28 Apr 2022 at 04:40, Fangrui Song <mas...@google.com> wrote:
>
> On 2022-04-27, Ard Biesheuvel wrote:
> >We currently use ordinary, position dependent code generation for the
> >core kernel, which happens to default to the 'small' code model on both
> >GCC and Clang. This is the code model that relies on ADRP/ADD or
> >ADRP/LDR pairs for symbol references, which are PC-relative with a range
> >of -/+ 4 GiB, and therefore happen to be position independent in
> >practice.
> >
> >This means that the fact that we can link the relocatable KASLR kernel
> >using the -pie linker flag (which generates the runtime relocations and
> >inserts them into the binary) is somewhat of a coincidence, and not
> >something which is explicitly supported by the toolchains.
>
> Agree. The current -fno-PIE + -shared -Bsymbolic combo works as a
> conincidence, not guaranteed by the toolchain.
>
> -shared needs -fpic object files. -shared -Bsymbolic is very similar to
> -pie and therefore works with -fpie object files, but the usage is not
> recommended from the toolchain perspective.
>

So are you suggesting we should also switch from -shared to -Bsymbol
to -pie if we can? I don't remember the details, but IIRC ld.bfd
didn't set the ELF binary type correctly, but perhaps this has now
been fixed.

> >The reason we have not used -fpie for code generation so far (which is
> >the compiler flag that should be used to generate code that is to be
> >linked with -pie) is that by default, it generates code based on
> >assumptions that only hold for shared libraries and PIE executables,
> >i.e., that gathering all relocatable quantities into a Global Offset
> >Table (GOT) is desirable because it reduces the CoW footprint, and
> >because it permits ELF symbol preemption (which lets an executable
> >override symbols defined in a shared library, in a way that forces the
> >shared library to update all of its internal references as well).
> >Ironically, this means we end up with many more absolute references that
> >all need to be fixed up at boot.
>
> This is not about symbol preemption (when the executable and a shared
> objectdefine the same symbol, which one wins). An executable using a GOT
> which will be resolved to a shared object => this is regular relocation
> resolving and there is no preemption.
>
> It is that the compiler prefers code generation which can avoid text
> relocations / copy relocations / canonical PLT entries
> (https://maskray.me/blog/2021-01-09-copy-relocations-canonical-plt-entries-and-protected#summary).
>

Fair enough. So the compiler cannot generate relative references to
undefined external symbols since it doesn't know at codegen time
whether the symbol reference will be satisfied by the executable
itself or by a shared library, and in the latter case, the relative
distance to the symbol is not known at build time, and so a runtime
relocation is required. But how about references to symbols with
external visibility that are defined in the same compilation unit? I
don't quite understand why those references need to go via the GOT as
well.

> >Fortunately, we can convince the compiler to handle this in a way that
> >is a bit more suitable for freestanding binaries such as the kernel, by
> >setting the 'hidden' visibility #pragma, which informs the compiler that
> >symbol preemption or CoW footprint are of no concern to us, and so
> >PC-relative references that are resolved at link time are perfectly
> >fine.
>
> Agree
>

The only unfortunate thing is that -fvisibility=hidden does not give
us the behavior we want, and we are forced to use the #pragma instead.

> >So let's enable this #pragma and build with -fpie when building a
> >relocatable kernel. This also means that all constant data items that
> >carry statically initialized pointer variables are now emitted into the
> >.data.rel.ro* sections, so move these into .rodata where they belong.
>
> LGTM, except: is ".rodata" a typo? The patch doesn't reference .rodata
>

I am referring to the .rodata pseudo-segment that we have in the
kernel, which runs from _etext to __inittext_begin.

[Fangrui Song]

On 2022-04-28, Ard Biesheuvel wrote:
>On Thu, 28 Apr 2022 at 04:40, Fangrui Song <mas...@google.com> wrote:
>>
>> On 2022-04-27, Ard Biesheuvel wrote:
>> >We currently use ordinary, position dependent code generation for the
>> >core kernel, which happens to default to the 'small' code model on both
>> >GCC and Clang. This is the code model that relies on ADRP/ADD or
>> >ADRP/LDR pairs for symbol references, which are PC-relative with a range
>> >of -/+ 4 GiB, and therefore happen to be position independent in
>> >practice.
>> >
>> >This means that the fact that we can link the relocatable KASLR kernel
>> >using the -pie linker flag (which generates the runtime relocations and
>> >inserts them into the binary) is somewhat of a coincidence, and not
>> >something which is explicitly supported by the toolchains.
>>
>> Agree. The current -fno-PIE + -shared -Bsymbolic combo works as a
>> conincidence, not guaranteed by the toolchain.
>>
>> -shared needs -fpic object files. -shared -Bsymbolic is very similar to
>> -pie and therefore works with -fpie object files, but the usage is not
>> recommended from the toolchain perspective.
>>
>
>So are you suggesting we should also switch from -shared to -Bsymbol
>to -pie if we can? I don't remember the details, but IIRC ld.bfd
>didn't set the ELF binary type correctly, but perhaps this has now
>been fixed.

Yes, -shared -Bsymbolic => -pie, but that can be done later.

For e_type: ET_DYN, I think unlikely there was a bug.
-pie was added by binutils in 2003: it's close to -shared but doesn't
allow its definitions to be preempted/interposed. Code earlier than that
might use -shared -Bsymbolic before -pie was available.

>> >The reason we have not used -fpie for code generation so far (which is
>> >the compiler flag that should be used to generate code that is to be
>> >linked with -pie) is that by default, it generates code based on
>> >assumptions that only hold for shared libraries and PIE executables,
>> >i.e., that gathering all relocatable quantities into a Global Offset
>> >Table (GOT) is desirable because it reduces the CoW footprint, and
>> >because it permits ELF symbol preemption (which lets an executable
>> >override symbols defined in a shared library, in a way that forces the
>> >shared library to update all of its internal references as well).
>> >Ironically, this means we end up with many more absolute references that
>> >all need to be fixed up at boot.
>>
>> This is not about symbol preemption (when the executable and a shared
>> objectdefine the same symbol, which one wins). An executable using a GOT
>> which will be resolved to a shared object => this is regular relocation
>> resolving and there is no preemption.
>>
>> It is that the compiler prefers code generation which can avoid text
>> relocations / copy relocations / canonical PLT entries
>> (https://maskray.me/blog/2021-01-09-copy-relocations-canonical-plt-entries-and-protected#summary).
>>
>
>Fair enough. So the compiler cannot generate relative references to
>undefined external symbols since it doesn't know at codegen time
>whether the symbol reference will be satisfied by the executable
>itself or by a shared library, and in the latter case, the relative
>distance to the symbol is not known at build time, and so a runtime
>relocation is required.

Right.

>But how about references to symbols with
>external visibility that are defined in the same compilation unit? I
>don't quite understand why those references need to go via the GOT as
>well.

If you mean references to a non-local STV_DEFAULT (default visibility) definition =>

* -fpic: use GOT because the definition may be replaced by another at run time.
Conservatively use a GOT-generating code sequence to allow potential symbol
preemption(interposition). The linker may optimize out the GOT (x86-64
GOTPCRELX, recent ld.lld for aarch64, powerpc64 TOC-indirect to
TOC-relative optimization).
* -fpie or -fno-pie: the definition cannot be replaced. GOT is unneeded.

-fpie is an optimization on top of -fpic: (a) non-local STV_DEFAULT
definitions can be assumed non-interposable (b) (irrelevant to the
kernel) TLS can use more optimized models.

>> >Fortunately, we can convince the compiler to handle this in a way that
>> >is a bit more suitable for freestanding binaries such as the kernel, by
>> >setting the 'hidden' visibility #pragma, which informs the compiler that
>> >symbol preemption or CoW footprint are of no concern to us, and so
>> >PC-relative references that are resolved at link time are perfectly
>> >fine.
>>
>> Agree
>>
>
>The only unfortunate thing is that -fvisibility=hidden does not give
>us the behavior we want, and we are forced to use the #pragma instead.

Right. For a very long time there had been no option controlling the
access mode for undefined symbols (-fvisibility= is for defined
symbols).

I added -fdirect-access-external-data to Clang which supports
many architectures (x86, aarch64, arm, riscv, ...).
GCC's x86 port added -mdirect-extern-access in 2022-02 (not available on aarch64).

The use of `#pragma GCC visibility push(hidden)` looks good as a
portable solution.

>> >So let's enable this #pragma and build with -fpie when building a
>> >relocatable kernel. This also means that all constant data items that
>> >carry statically initialized pointer variables are now emitted into the
>> >.data.rel.ro* sections, so move these into .rodata where they belong.
>>
>> LGTM, except: is ".rodata" a typo? The patch doesn't reference .rodata
>>
>
>I am referring to the .rodata pseudo-segment that we have in the
>kernel, which runs from _etext to __inittext_begin.

OK

>> >Code size impact (GCC):
>> >
>> >Before:
>> >
>> > text data bss total filename
>> > 16712396 18659064 534556 35906016 vmlinux
>> >
>> >After:
>> >
>> > text data bss total filename
>> > 16804400 18612876 534556 35951832 vmlinux
>> >
>> >Code size impact (Clang):
>> >
>> >Before:
>> >
>> > text data bss total filename
>> > 17194584 13335060 535268 31064912 vmlinux
>> >
>> >After:
>> >
>> > text data bss total filename
>> > 17194536 13310032 535268 31039836 vmlinux

The size difference for Clang matches my expecation:)
I am somewhat surprised that data is smaller, though...

I wonder how GCC makes code bloated so much...

[Ard Biesheuvel]

On Wed, 27 Apr 2022 at 23:50, Ard Biesheuvel <ar...@kernel.org> wrote:
>
> Hi Nick,
>
> On Wed, 27 Apr 2022 at 20:59, Nick Desaulniers <ndesau...@google.com> wrote:
> >
> > On Wed, Apr 27, 2022 at 10:13 AM Ard Biesheuvel <ar...@kernel.org> wrote:
> > >
> > > In order to set bit #0 of the struct static_key pointer in the the jump
> > > label struct, we currently cast the pointer to char[], and take the
> > > address of either the 0th or 1st array member, depending on the value of
> > > 'branch'. This works but creates problems with -fpie code generation:
> > > GCC complains about the constraint being unsatisfiable, and Clang
> > > miscompiles the code in a way that causes stability issues (immediate
> > > panic on 'attempt to kill init')
> >
> > Any more info on the "miscompile?" Perhaps worth an upstream bug report?
> >
>
> I made very little progress trying to narrow it down: a segfault in
> user space caused by who knows what. I'd file a bug if I knew how to
> reproduce it.
>

... and now, I cannot even reproduce it anymore, so I'll drop this
mention altogether.

> > >
> > > So instead, pass the struct static_key reference and the 'branch'
> > > immediate individually, in a way that satisfies both GCC and Clang (GCC
> > > wants the 'S' constraint, whereas Clang wants the 'i' constraint for
> > > argument %0)
> >
> > Anything we can do to improve Clang's handling of S constraints?
> > https://gcc.gnu.org/onlinedocs/gcc/Machine-Constraints.html#Machine-Constraints
> > >> An absolute symbolic address or a label reference
> >
>
> "S" seems more appropriate than "i" for a symbol reference, and GCC
> rejects "i" when using -fpie. But the actual literal being emitted is
> a relative reference, not an absolute one. This likely needs more
> discussion, but using "i" in the way we do now is definitely dodgy.
>

I have tried to create a reproducer for this issue, but the code
below, which does essentially the same thing as jump_label.h, works
fine with Clang and GCC with or without -fpie.

extern struct s {
struct { } m;
} ss;

static inline __attribute__((always_inline)) int inner(struct s *s) {
asm goto("adrp xzr, %c0" : : "S"(&s->m) : : l);

return 0;
l:return 1;
}

int outer(void) {
return inner(&ss);
}

So what's tricky here is that arch_static_branch() [like inner()
above] does not refer to the global directly, which is either struct
static_key_true or struct static_key_false, but to its 'key' member,
which is the first member of either former type. However, Clang does
not seem to care in case of this example, but when building the
kernel, it errors out with

/home/ard/linux/arch/arm64/include/asm/jump_label.h:22:3: error:
invalid operand for inline asm constraint 'S'

Another thing I hate about this code is that it needs optimizations
enabled, or it won't compile.

[Mark Rutland]

Hi Ard,

On Wed, Apr 27, 2022 at 07:12:40PM +0200, Ard Biesheuvel wrote:
> In order to set bit #0 of the struct static_key pointer in the the jump
> label struct

I think you mean jump_entry::key here?

> , we currently cast the pointer to char[], and take the
> address of either the 0th or 1st array member, depending on the value of
> 'branch'. This works but creates problems with -fpie code generation:
> GCC complains about the constraint being unsatisfiable, and Clang
> miscompiles the code in a way that causes stability issues (immediate
> panic on 'attempt to kill init')

I couldn't reproduce that stability issue locally playing with Clang 12.0.0 and
14.0.0 (and just applying patch 2 of this series atop v5.18-rc1). I built
defconfig and booted that under a QEMU HVF VM on an M1 Mac.

FWIW, I used the binaries from llvm.org and built with:

// magic script to add the toolchains to my PATH
usellvm 12.0.0 make LLVM=1 ARCH=arm64 defconfig
usellvm 12.0.0 make LLVM=1 ARCH=arm64 -j50 Image

... and QEMU isn't providing entropy to the guest, so it's possible that:

* This only goes wrong when randomizing VAs (maybe we get a redundant
relocation, and corrupt the key offset?).

* This is specific to the LLVM binaries you're using.

* This is down to a combination of LLVM + binutils, if you're not building with
LLVM=1?

I had a go with Clang 12.0.0 and the kernel.org crosstool GCC 11.1.0
release's binutils. I made the constraint "Si" but left the indexing logic,
and that still worked fine.

> So instead, pass the struct static_key reference and the 'branch'
> immediate individually, in a way that satisfies both GCC and Clang (GCC
> wants the 'S' constraint, whereas Clang wants the 'i' constraint for
> argument %0)
>
> Signed-off-by: Ard Biesheuvel <ar...@kernel.org>
> ---
> 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 cea441b6aa5d..f741bbacf175 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 *key,
> " .pushsection __jump_table, \"aw\" \n\t"
> " .align 3 \n\t"
> " .long 1b - ., %l[l_yes] - . \n\t"
> - " .quad %c0 - . \n\t"
> + " .quad %c0 - . + %1 \n\t"
> " .popsection \n\t"
> - : : "i"(&((char *)key)[branch]) : : l_yes);
> + : : "Si"(key), "i"(branch) : : l_yes);

Nice! I like that this clearly separate the "set bit 0" portion out, and IMO
that's much clearer than the array indexing.

Thanks,
Mark.

[Ard Biesheuvel]

OK

This is caused by the use of RELA format instead of RELR.

[Ard Biesheuvel]

On Thu, 28 Apr 2022 at 11:51, Mark Rutland <mark.r...@arm.com> wrote:
>
> Hi Ard,
>
> On Wed, Apr 27, 2022 at 07:12:40PM +0200, Ard Biesheuvel wrote:
> > In order to set bit #0 of the struct static_key pointer in the the jump
> > label struct
>
> I think you mean jump_entry::key here?
>

Yes, which points to a struct static_key - I'll clarify this in v2.

> > , we currently cast the pointer to char[], and take the
> > address of either the 0th or 1st array member, depending on the value of
> > 'branch'. This works but creates problems with -fpie code generation:
> > GCC complains about the constraint being unsatisfiable, and Clang
> > miscompiles the code in a way that causes stability issues (immediate
> > panic on 'attempt to kill init')
>
> I couldn't reproduce that stability issue locally playing with Clang 12.0.0 and
> 14.0.0 (and just applying patch 2 of this series atop v5.18-rc1). I built
> defconfig and booted that under a QEMU HVF VM on an M1 Mac.
>
> FWIW, I used the binaries from llvm.org and built with:
>
> // magic script to add the toolchains to my PATH
> usellvm 12.0.0 make LLVM=1 ARCH=arm64 defconfig
> usellvm 12.0.0 make LLVM=1 ARCH=arm64 -j50 Image
>
> ... and QEMU isn't providing entropy to the guest, so it's possible that:
>
> * This only goes wrong when randomizing VAs (maybe we get a redundant
> relocation, and corrupt the key offset?).
>
> * This is specific to the LLVM binaries you're using.
>
> * This is down to a combination of LLVM + binutils, if you're not building with
> LLVM=1?
>
> I had a go with Clang 12.0.0 and the kernel.org crosstool GCC 11.1.0
> release's binutils. I made the constraint "Si" but left the indexing logic,
> and that still worked fine.
>

Yeah, as I reported in another email, I failed to reproduce this, and
I experienced some other issues yesterday due to the fact that llvm-nm
and clang/lld on my system were out of sync. So I think this was a
false positive.

[Nick Desaulniers]

On Wed, Apr 27, 2022 at 11:57 PM Fangrui Song <mas...@google.com> wrote:
>
> On 2022-04-28, Ard Biesheuvel wrote:
> >On Thu, 28 Apr 2022 at 04:40, Fangrui Song <mas...@google.com> wrote:
> >>
> >> On 2022-04-27, Ard Biesheuvel wrote:
> >> >Fortunately, we can convince the compiler to handle this in a way that
> >> >is a bit more suitable for freestanding binaries such as the kernel, by
> >> >setting the 'hidden' visibility #pragma, which informs the compiler that
> >> >symbol preemption or CoW footprint are of no concern to us, and so
> >> >PC-relative references that are resolved at link time are perfectly
> >> >fine.
> >>
> >> Agree
> >>
> >
> >The only unfortunate thing is that -fvisibility=hidden does not give
> >us the behavior we want, and we are forced to use the #pragma instead.
>
> Right. For a very long time there had been no option controlling the
> access mode for undefined symbols (-fvisibility= is for defined
> symbols).
>
> I added -fdirect-access-external-data to Clang which supports
> many architectures (x86, aarch64, arm, riscv, ...).
> GCC's x86 port added -mdirect-extern-access in 2022-02 (not available on aarch64).
>
> The use of `#pragma GCC visibility push(hidden)` looks good as a
> portable solution.

Portable, sure, which is fine for now.

But there's just something about injecting a header into ever TU via
-include in order to set a pragma and that there's such pragmas
effecting codegen that makes my skin crawl.

Perhaps we can come up with a formal feature request for toolchain
vendors for an actual command line flag?

Does the pragma have the same effect as
`-fdirect-access-external-data`/`-mdirect-extern-access`, or would
this feature request look like yet another distinct flag?
--
Thanks,
~Nick Desaulniers

[Ard Biesheuvel]

> `-fdirect-access-external-data`/`-mdirect-extern-access`, or wvisould

> this feature request look like yet another distinct flag?

I agree that this is rather nasty. What I don't understand is why
-fvisibility=hidden gives different behavior to begin with, or why
-ffreestanding -fpie builds don't default to hidden visibility for
symbol declarations as well as definitions.

[Fangrui Song]

`#pragma GCC visibility push(hidden)` is very similar to
-fvisibility=hidden -fdirect-access-external-data with Clang.
In Clang there are only two differences:

// TLS initial-exec model with -fdirect-access-external-data;
// TLS local-exec model with `#pragma GCC visibility push(hidden)`
extern __thread int var;
int foo() { return var; }

// hidden visibility suppresses -fno-plt.
// -fdirect-access-external-data / GCC -mdirect-extern-access doesn't suppress -fno-plt.
extern int bar();
int foo() { return bar() + 2; }


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

>I agree that this is rather nasty. What I don't understand is why
>-fvisibility=hidden gives different behavior to begin with, or why
>-ffreestanding -fpie builds don't default to hidden visibility for
>symbol declarations as well as definitions.

-ffreestanding doesn't mean there is no DSO. A libc implementation (e.g.
musl) may use -ffreestanding to avoid libc dependencies from the host
environment. It may ship several shared objects and export multiple symbols.
Implied -fvisibility=hidden will get in the way.

There is a merit to make options orthogonal.

[Ard Biesheuvel]

OK. But you mentioned that GCC does not implement
-mdirect-extern-access for AArch64, right? So for now, the pragma is
the only portable option we have.

> >I agree that this is rather nasty. What I don't understand is why
> >-fvisibility=hidden gives different behavior to begin with, or why
> >-ffreestanding -fpie builds don't default to hidden visibility for
> >symbol declarations as well as definitions.
>
> -ffreestanding doesn't mean there is no DSO. A libc implementation (e.g.
> musl) may use -ffreestanding to avoid libc dependencies from the host
> environment. It may ship several shared objects and export multiple symbols.
> Implied -fvisibility=hidden will get in the way.
>
> There is a merit to make options orthogonal.

Fair enough.

[Fangrui Song]

Right.