[RFC PATCH 1/2] arm64: jump_label: use more precise asm constraints
Mark Rutland
mark.rutland at arm.com
Thu Apr 28 02:51:28 PDT 2022
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 <ardb at 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.
>
> 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
>
More information about the linux-arm-kernel
mailing list