Re: [RFC PATCH 1/2] arm64: jump_label: use more precise asm constraints

[Mark Rutland <mark.rutland@arm.com>]

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@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
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel