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

From: Ard Biesheuvel <ardb@kernel.org>
To: Mark Rutland <mark.rutland@arm.com>
Cc: Peter Zijlstra <peterz@infradead.org>,
	Frederic Weisbecker <frederic@kernel.org>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will@kernel.org>,
	LKML <linux-kernel@vger.kernel.org>,
	James Morse <james.morse@arm.com>,
	Quentin Perret <qperret@google.com>,
	Christophe Leroy <christophe.leroy@csgroup.eu>
Subject: Re: [PATCH 2/4] arm64: implement support for static call trampolines
Date: Tue, 21 Sep 2021 17:55:11 +0200	[thread overview]
Message-ID: <CAMj1kXHQM9WOQutZg6P63=zQDE67jjfGv1tub1+W44LoZrON+g@mail.gmail.com> (raw)
In-Reply-To: <20210921153352.GC35846@C02TD0UTHF1T.local>

On Tue, 21 Sept 2021 at 17:33, Mark Rutland <mark.rutland@arm.com> wrote:
>
> On Tue, Sep 21, 2021 at 04:44:56PM +0200, Ard Biesheuvel wrote:
> > On Tue, 21 Sept 2021 at 09:10, Peter Zijlstra <peterz@infradead.org> wrote:
...
> > >
> > > So I like what Christophe did for PPC32:
> > >
> > >   https://lkml.kernel.org/r/6ec2a7865ed6a5ec54ab46d026785bafe1d837ea.1630484892.git.christophe.leroy@csgroup.eu
> > >
> > > Where he starts with an unconditional jmp and uses that IFF the offset
> > > fits and only does the data load when it doesn't. Ard, woulnd't that
> > > also make sense on ARM64? I'm thinking most in-kernel function pointers
> > > would actually fit, it's just the module muck that gets to have too
> > > large pointers, no?
> > >
> >
> > Yeah, I'd have to page that back in. But it seems like the following
> >
> >   bti c
> >   <branch>
> >   adrp x16, <literal>
> >   ldr x16, [x16, ...]
> >   br x16
> >
> > with <branch> either set to 'b target' for the near targets, 'ret' for
> > the NULL target, and 'nop' for the far targets should work, and the
> > architecture permits patching branches into NOPs and vice versa
> > without special synchronization.
>
> I think so, yes. We can do sligntly better with an inline literal pool
> and a PC-relative LDR to fold the ADRP+LDR, e.g.
>
>         .align 3
> tramp:
>         BTI     C
>         {B <func> | RET | NOP}
>         LDR     X16, 1f
>         BR      X16
> 1:      .quad   <literal>
>
> Since that's in the .text, it's RO for regular accesses anyway.
>

I tried to keep the literal in .rodata to avoid inadvertent gadgets
and/or anticipate exec-only mappings of .text, but that may be a bit
overzealous.

> > But I must be missing something here, or why did we have that long
> > discussion before?
>
> I think the long discussion was because v2 had some more complex options
> (mostly due to trying to use ADRP+ADD) and atomicity/preemption issues
> meant we could only transition between some of those one-way, and it was
> subtle/complex:
>
> https://lore.kernel.org/linux-arm-kernel/20201028184114.6834-1-ardb@kernel.org/
>

Ah yes, I was trying to use ADRP/ADD to avoid the load, and this is
what created all the complexity.

> For v3, that was all gone, but we didn't have a user.
>
> Since the common case *should* be handled by {B <func> | RET | NOP }, I
> reckon it's fine to have just that and the literal pool fallback (which
> I'll definitely need for the sorts of kernel I run when fuzzing, where
> the kernel Image itself can be 100s of MiBs).

Ack. So I'll respin this along these lines. Do we care deeply about
the branch and the literal being transiently out of sync?