Re: [PATCH] arm64: jump_label: use constraint "S" instead of "i"

[Fangrui Song]

On Thu, Feb 1, 2024 at 1:23 PM Ard Biesheuvel <ardb@xxxxxxxxxx> wrote:
>
> On Thu, 1 Feb 2024 at 21:56, Fangrui Song <maskray@xxxxxxxxxx> wrote:
> >
> > On 2024-02-01, Dave Martin wrote:
> > >On Thu, Feb 01, 2024 at 05:07:59PM +0100, Ard Biesheuvel wrote:
> > >> On Thu, 1 Feb 2024 at 12:50, Dave Martin <Dave.Martin@xxxxxxx> wrote:
> > >> >
> > >> > On Thu, Feb 01, 2024 at 01:11:20AM -0800, Fangrui Song wrote:
> > >> ...
> > >> > >
> > >> > > Hmm. Clang's constraint "S" implementation doesn't support a constant
> > >> > > offset, so
> > >> > > `static_key_false(&nf_hooks_needed[pf][hook])` in include/linux/netfilter.h:nf_hook
> > >> > > would not compile with Clang <= 18.
> > >> > >
> > >> > > I have a patch https://github.com/llvm/llvm-project/pull/80255 , but
> > >> > > even if it is accepted and cherry-picked into the 18.x release branch,
> > >> > > if we still support older Clang, we cannot use "S" unconditionally.
> > >> > >
> > >> > >
> > >> > > So we probably need the following to prepare for -fPIE support in the
> > >> > > future:
> > >> > >
> > >> > > diff --git a/arch/arm64/include/asm/jump_label.h b/arch/arm64/include/asm/jump_label.h
> > >> > > index 48ddc0f45d22..b8af2f8b0c99 100644
> > >> > > --- a/arch/arm64/include/asm/jump_label.h
> > >> > > +++ b/arch/arm64/include/asm/jump_label.h
> > >> > > @@ -15,6 +15,16 @@
> > >> > >  #define JUMP_LABEL_NOP_SIZE          AARCH64_INSN_SIZE
> > >> > > +/*
> > >> > > + * Prefer "S" to support PIC. However, use "i" for Clang 18 and earlier as "S"
> > >> > > + * on a symbol with a constant offset is not supported.
> > >> > > + */
> > >> > > +#if defined(CONFIG_CC_IS_CLANG) && __clang_major__ <= 18
> > >> >
> > >> > Is a GCC version check needed?  Or is the minimum GCC version specified
> > >> > for building the kernel new enough?
> > >> >
> > >> > > +#define JUMP_LABEL_STATIC_KEY_CONSTRAINT "i"
> > >> > > +#else
> > >> > > +#define JUMP_LABEL_STATIC_KEY_CONSTRAINT "S"
> > >> > > +#endif
> > >> > > +
> > >>
> > >> Can we use "Si" instead?
> > >
> > >I thought the point was to avoid "S" on compilers that would choke on
> > >it?  If so, those compilers would surely choke on "Si" too, no?
> >
> > "Si" is an invalid constraint. Unfortunately, GCC recognizes "S" as a
> > valid constraint and ignores trailing characters (asm_operand_ok). That
> > is, GCC would accept "Siiiii" as well...
> >
> > The GCC support for "S" is great. The initial aarch64 port in 2012 supports "S".
> >
>
> So it is not possible to combine the S and i constraint, and let the
> compiler figure out whether it meets either? We rely on this elsewhere
> by combining r and Z into rZ. x86 uses "rm" for inline asm parameters
> that could be either register or memory operands.

I did not realize that your "Si" suggestion meant this feature
https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html

    After the prefix, there must be one or more additional constraints
(see Constraints for asm Operands) that describe where the value
resides. Common constraints include ‘r’ for register and ‘m’ for
memory. When you list more than one possible location (for example,
"=rm"), the compiler chooses the most efficient one based on the
current context.

After some debugging of both GCC and Clang, I think you are right and
"Si" will work. Thanks for the suggestion!

GCC accepts "Si". Sorry for my incorrectly stating that GCC ignores
the trailing constraint. GCC does ignore an unknown constraint, as
long as one accepted constraint.

Clang accepts "Si" as well. In the absence of
https://github.com/llvm/llvm-project/pull/80255 (constant offset
support for 'S'), in TargetLowering::ComputeConstraintToUse, 'S' is
skipped and 'i' will be picked. This is unrelated to the "rm" weakness
in Clang (https://github.com/llvm/llvm-project/issues/20571).

I'll post a PATCH v2 removing JUMP_LABEL_STATIC_KEY_CONSTRAINT and
using "Si" unconditionally.

>
> > >> > >  static __always_inline bool arch_static_branch(struct static_key * const key,
> > >> > >                                              const bool branch)
> > >> > >  {
> > >> > > @@ -23,9 +33,9 @@ static __always_inline bool arch_static_branch(struct static_key * const key,
> > >> > >                "      .pushsection    __jump_table, \"aw\"    \n\t"
> > >> > >                "      .align          3                       \n\t"
> > >> > >                "      .long           1b - ., %l[l_yes] - .   \n\t"
> > >> > > -              "      .quad           %c0 - .                 \n\t"
> > >> > > +              "      .quad           %0 + %1 - .             \n\t"
> > >> > >                "      .popsection                             \n\t"
> > >> > > -              :  :  "i"(&((char *)key)[branch]) :  : l_yes);
> > >> > > +              :  :  JUMP_LABEL_STATIC_KEY_CONSTRAINT(key), "i"(branch) :  : l_yes);
> > >> >
> > >> > While this looks reasonable, I'm still not clear now why the asm must do
> > >> > the addition.
> > >> >
> > >> > Can we roll the branch argument into the macro?
> > >> >
> > >> > "S"(symbol + constant) seems to do the right thing for AArch64, at least
> > >> > in GCC.
> > >> >
> > >>
> > >> 'symbol' is a struct static_key pointer, so adding '1' to it will not
> > >> produce the needed result.
> > >
> > >I mean loosely things that resolve to something of the form
> > >"symbol + constant" in the compiler output.
> > >
> > >"S"(&((char *)key)[branch]) does indeed seem to do the right thing,
> > >at least with GCC.
> > >
> > >I probably didn't help by bikeshedding the way that expression was
> > >written, apologies for that.  It's orthogonal to what this patch is
> > >doing.
> >
> > Yes, "S"(&((char *)key)[branch])  would do the right thing.
> > I have compared assembly output. It's a matter of "s" vs "s + 0" and "s+1" vs "s + 1".
> >
>
> But if S does not work with constant offsets with older Clang, what is
> the harm in splitting it into "S" for the symbol and "i" for the
> branch?
> ...
> > >>
> > >> The only thing PIE linking does in the case of the kernel is
> > >> a) complain if static relocations are used that cannot be fixed up at
> > >> runtime (e.g., movk/movz sequences)
> > >> b) emit dynamic relocations that the early startup code can use to fix
> > >> up all statically initialized pointers
> > >>
> > >> From a codegen point-of-view, there is no difference because we don't
> > >> use -fpic code. The problem with -fpic codegen is that it makes some
> > >> assumptions that only hold for shared ELF objects, e.g., avoiding
> > >> dynamic relocations in .text, using GOT entries even for variables
> > >> defined in the same compilation so that they can be preempted, keeping
> > >> all runtime relocatable quantities together so the CoW footprint of
> > >> the shared library is minimized.
> > >>
> > >> None of this matters for the kernel, and as it turns out, the non-PIC
> > >> small C model produces code that the PIE linker is happy with.
> > >>
> > >> TL;DR our code (including inline and out-of-line asm) is already PIC,
> > >> and this property is relied upon by KASLR.
> > >>
> > >> > Maybe it doesn't make as much difference as I thought (certainly ADRP
> > >> > based addressing should make a fair amount of the PIC/PIE overhead go
> > >> > away).
> > >> >
> > >>
> > >> The PIC/PIE overhead is in the indirections via the GOT. However,
> > >> recent linkers will actually perform some relaxations too: if a symbol
> > >> is defined in the same executable and is not preemptible, a GOT load
> > >>
> > >> ADRP
> > >> LDR
> > >>
> > >> can be transparently converted to
> > >>
> > >> ADRP
> > >> ADD
> > >>
> > >> referring to the symbol directly. This is also where hidden.h comes
> > >> in: making all symbol declarations and definitions 'hidden' will allow
> > >> the compiler to assume that a GOT entry is never needed.
> > >
> > >Is there a reason why we don't just build the whole kernel with
> > >-fvisibility=hidden today?
> >
> > This topic, loosely related to this patch, is about switching to PIC
> > compiles. I am not familiar with the Linux kernel, so I'll mostly leave
> > the discussion to you and Ard :)
> >
> > That said, I have done a lot of Clang work in visibility/symbol
> > preemptibility and linkers, so I am happy to add what I know.
> >
> > -fvisibility=hidden only applies to definitions, not non-definition
> > declarations.
> >
> > I've mentioned this at
> > https://lore.kernel.org/all/20220429070318.iwj3j5lpfkw4t7g2@xxxxxxxxxx/
> >
> >      `#pragma GCC visibility push(hidden)` is very similar to -fvisibility=hidden -fdirect-access-external-data with Clang.
> >      ...
> >      The kernel uses neither TLS nor -fno-plt, so -fvisibility=hidden
> >      -fdirect-access-external-data can replace `#pragma GCC visibility
> >      push(hidden)`.
> >
> > >> So building with -fPIC is currently not need in practice, and creates
> > >> some complications, which is why we have been avoiding it. But PIE
> > >> linking non-PIC objects is not guaranteed to remain supported going
> > >> forward, so it would be better to have a plan B, i.e., being able to
> > >> turn on -fpic without massive regressions due to GOT overhead, or
> > >> codegen problems with our asm hacks.
> > >
> > >Summarising all of this is it right that:
> > >
> > >1) ld -pie is how we get the reloc info into the kernel for KASLR
> > >today.
> > >
> > >2) We use gcc -fno-pic today, but this might break with ld -pie in the
> > >future, although it works for now.
> > >
> > >3) gcc -fno-pic and gcc -fPIC (or -fPIE) generate almost the same code,
> > >assuming we tweak symbol visibility and use a memory model that
> > >ADR+ADD/LDR can span.  So, moving to -fPIE is likely to be do-able.
> > >
> > >
> > >My point is that an alternative option would be to move to ld -no-pie.
>
> Why? What would that achieve?
>
> > >We would need another way to get relocs into the kernel, such as an
> > >intermediate step with ld --emit-relocs.  I have definitely seen this
> > >done before somewhere, but it would be extra work and not necessarily
> > >worth it, based on what you say about code gen.
>
> Relying on --emit-relocs is a terrible hack. It contains mostly
> relocations that we don't need at runtime, so we need to postprocess
> them. And linkers may not update these relocations in some cases after
> relaxation or other transformations (and sometimes, it is not even
> possible to do so, if the post-transformation situation cannot be
> described by a relocation).

Agreed. I've filed
https://sourceware.org/bugzilla/show_bug.cgi?id=30844
("ld riscv: --emit-relocs does not retain the original relocation
type") for a RISC-V version.

--emit-relocs is under-specified and the linker behaviors are less tested.

> And with Fangrui's RELR optimization, the PIE relocation data is
> extremely dense.
>
> There is nothing to fix here, really.
>
> > >
> > >This may all have been discussed to death already -- I didn't mean to
> > >hijack this patch, so I'll stop digging here, but if I've misunderstood,
> > >please shout!
>
> No worries. Ramana and I spent some time 5+ years ago to document how
> PIC codegen for the kernel (or any freestanding executable, really)
> deviates from the typical PIC codegen for shared libraries and PIE
> executables, with the intention of adding a GCC compiler switch for
> it, but that never really materialized, mostly because things just
> work fine today.
>