[RFC PATCH 1/2] arm64: jump_label: use more precise asm constraints
Ard Biesheuvel
ardb at kernel.org
Thu Apr 28 09:05:54 PDT 2022
On Thu, 28 Apr 2022 at 11:51, Mark Rutland <mark.rutland at 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.
> > 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