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

[Ard Biesheuvel <ardb@kernel.org>]

On Thu, 28 Apr 2022 at 11:51, Mark Rutland <mark.rutland@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@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