Re: [PATCH 2/4] arm64: implement support for static call trampolines

[Peter Zijlstra]

On Mon, Oct 25, 2021 at 04:08:37PM +0200, Ard Biesheuvel wrote:
> On Mon, 25 Oct 2021 at 15:57, Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:
> >
> > On Mon, Oct 25, 2021 at 02:21:00PM +0200, Frederic Weisbecker wrote:
> >
> > > +#define __ARCH_DEFINE_STATIC_CALL_TRAMP(name, insn)                      \
> > > +     asm("   .pushsection    .static_call.text, \"ax\"               \n" \
> > > +         "   .align          4                                       \n" \
> > > +         "   .globl          " STATIC_CALL_TRAMP_STR(name) "         \n" \
> > > +         "0: .quad   0x0                                             \n" \
> > > +         STATIC_CALL_TRAMP_STR(name) ":                              \n" \
> > > +         "   hint    34      /* BTI C */                             \n" \
> > > +             insn "                                                  \n" \
> > > +         "   ldr     x16, 0b                                         \n" \
> > > +         "   cbz     x16, 1f                                         \n" \
> > > +         "   br      x16                                             \n" \
> > > +         "1: ret                                                     \n" \
> > > +         "   .popsection                                             \n")
> >

> > OK, that's pretty magical...
> >
> > So you're writing the literal and the two instructions with 2 u64
> > stores. Relying on alignment to guarantee both are in a single page and
> > that copy_to_kernel_nofault() selects u64 writes.
> >
> 
> To be honest, it just seemed tidier and less likely to produce weird
> corner cases to put the literal and the patched insn in the smallest
> possible power-of-2 aligned window, as it ensures that the D-side view
> is always consistent.
> 
> However, the actual fetch of the instruction could still produce a
> stale value before the cache maintenance completes.
> 
> > By unconditionally writing the literal, you avoid there ever being an
> > stale value, which in turn avoids there being a race where you switch
> > from 'J @func' relative addressing to 'NOP; do-literal-thing' and cross
> > CPU execution gets the ordering inverted.
> >
> 
> Indeed.
> 
> > Ooohh, but what if you go from !func to NOP.
> >
> > assuming:
> >
> >         .literal = 0
> >         BTI C
> >         RET
> >
> > Then
> >
> >         CPU0                    CPU1
> >
> >         [S] literal = func      [I] NOP
> >         [S] insn[1] = NOP       [L] x16 = literal (NULL)
> >                                 b x16
> >                                 *BANG*
> >
> > Is that possible? (total lack of memory ordering etc..)
> >
> 
> The CBZ will branch to the RET instruction if x16 == 0x0, so this
> should not happen.

Oooh, I missed that :/ I was about to suggest writing the address of a
bare 'ret' trampoline instead of NULL into the literal.

> > On IRC you just alluded to the fact that this relies on it all being in
> > a single cacheline (i-fetch windows don't need to be cacheline sized,
> > but provided they're at least 16 bytes, this should still work given the
> > alignment).
> >
> > But is I$ and D$ coherent? One load is through I-fetch, the other is a
> > regular D-fetch.
> >
> > However, Will has previously expressed reluctance to rely on such
> > things.
> >
> 
> No they are not. That is why the CBZ is there. So the only issue we
> might see is where the branch instruction is out of sync with the
> literal, and so we may call the old function while switching to the
> new one and the I-cache maintenance hasn't completed yet.

OK, agreed. Perhaps put in a comment to explain some of this though. The
next poor sod trying to untangle this code is sure to have a question or
two :-)