From: Ard Biesheuvel <ardb@kernel.org>
To: Peter Zijlstra <peterz@infradead.org>
Cc: Linux ARM <linux-arm-kernel@lists.infradead.org>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
Mark Rutland <mark.rutland@arm.com>,
Quentin Perret <qperret@google.com>,
Catalin Marinas <catalin.marinas@arm.com>,
James Morse <james.morse@arm.com>, Will Deacon <will@kernel.org>,
Frederic Weisbecker <frederic@kernel.org>,
Kees Cook <keescook@chromium.org>,
Sami Tolvanen <samitolvanen@google.com>,
Andy Lutomirski <luto@kernel.org>,
Josh Poimboeuf <jpoimboe@redhat.com>,
Steven Rostedt <rostedt@goodmis.org>
Subject: Re: [PATCH v6 2/2] arm64: implement support for static call trampolines
Date: Mon, 8 Nov 2021 12:29:04 +0100 [thread overview]
Message-ID: <CAMj1kXHEBc1XkTuHPXYV8rp5++HA9ruROUP-UApzEnVzvgFvTw@mail.gmail.com> (raw)
In-Reply-To: <YYj6ib6Mrp9rAjVJ@hirez.programming.kicks-ass.net>
On Mon, 8 Nov 2021 at 11:23, Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Fri, Nov 05, 2021 at 03:59:17PM +0100, Ard Biesheuvel wrote:
> > diff --git a/arch/arm64/include/asm/static_call.h b/arch/arm64/include/asm/static_call.h
> > new file mode 100644
> > index 000000000000..6ee918991510
> > --- /dev/null
> > +++ b/arch/arm64/include/asm/static_call.h
> > @@ -0,0 +1,40 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef _ASM_STATIC_CALL_H
> > +#define _ASM_STATIC_CALL_H
> > +
> > +/*
> > + * The sequence below is laid out in a way that guarantees that the literal and
> > + * the instruction are always covered by the same cacheline, and can be updated
> > + * using a single store-pair instruction (provided that we rewrite the BTI C
> > + * instruction as well). This means the literal and the instruction are always
> > + * in sync when observed via the D-side.
> > + *
> > + * However, this does not guarantee that the I-side will catch up immediately
> > + * as well: until the I-cache maintenance completes, CPUs may branch to the old
> > + * target, or execute a stale NOP or RET. We deal with this by writing the
> > + * literal unconditionally, even if it is 0x0 or the branch is in range. That
> > + * way, a stale NOP will fall through and call the new target via an indirect
> > + * call. Stale RETs or Bs will be taken as before, and branch to the old
> > + * target.
> > + */
>
> Thanks for the comment!
>
>
> > diff --git a/arch/arm64/kernel/patching.c b/arch/arm64/kernel/patching.c
> > index 771f543464e0..a265a87d4d9e 100644
> > --- a/arch/arm64/kernel/patching.c
> > +++ b/arch/arm64/kernel/patching.c
>
>
> > +static void *strip_cfi_jt(void *addr)
> > +{
> > + if (IS_ENABLED(CONFIG_CFI_CLANG)) {
> > + void *p = addr;
> > + u32 insn;
> > +
> > + /*
> > + * Taking the address of a function produces the address of the
> > + * jump table entry when Clang CFI is enabled. Such entries are
> > + * ordinary jump instructions, preceded by a BTI C instruction
> > + * if BTI is enabled for the kernel.
> > + */
> > + if (IS_ENABLED(CONFIG_ARM64_BTI_KERNEL))
> > + p += 4;
>
> Perhaps:
> if (aarch64_insn_is_bti(le32_to_cpup(p)))
That instruction does not exist yet, and it begs the question which
type of BTI instruction we want to detect.
> p += 4;
>
> Perhapser still, add:
> else
> WARN_ON(IS_ENABLED(CONFIG_ARM64_BTI_KERNEL));
>
There's already a WARN() below that will trigger and return the
original address if the entry did not have the expected layout, which
means a direct branch at offset 0x0 or 0x4 depending on whether BTI is
on.
So I could add a WARN() here as well, but I'd prefer to keep the one
at the bottom, which makes the one here slightly redundant.
> > +
> > + insn = le32_to_cpup(p);
> > + if (aarch64_insn_is_b(insn))
> > + return p + aarch64_get_branch_offset(insn);
> > +
> > + WARN_ON(1);
> > + }
> > + return addr;
> > +}
>
> Also, can this please have a comment decrying the lack of built-in for
> this?
Sure.
next prev parent reply other threads:[~2021-11-08 11:29 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-11-05 14:59 [PATCH v6 0/2] static call support for arm64 Ard Biesheuvel
2021-11-05 14:59 ` [PATCH v6 1/2] static_call: use non-function types to refer to the trampolines Ard Biesheuvel
2021-11-08 10:08 ` Peter Zijlstra
2021-11-05 14:59 ` [PATCH v6 2/2] arm64: implement support for static call trampolines Ard Biesheuvel
2021-11-08 10:23 ` Peter Zijlstra
2021-11-08 11:29 ` Ard Biesheuvel [this message]
2021-11-08 11:52 ` Peter Zijlstra
2021-11-09 17:55 ` Mark Rutland
2021-11-09 18:09 ` Ard Biesheuvel
2021-11-09 19:02 ` Quentin Perret
2021-11-10 11:09 ` Mark Rutland
2021-11-10 12:05 ` Quentin Perret
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAMj1kXHEBc1XkTuHPXYV8rp5++HA9ruROUP-UApzEnVzvgFvTw@mail.gmail.com \
--to=ardb@kernel.org \
--cc=catalin.marinas@arm.com \
--cc=frederic@kernel.org \
--cc=james.morse@arm.com \
--cc=jpoimboe@redhat.com \
--cc=keescook@chromium.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=luto@kernel.org \
--cc=mark.rutland@arm.com \
--cc=peterz@infradead.org \
--cc=qperret@google.com \
--cc=rostedt@goodmis.org \
--cc=samitolvanen@google.com \
--cc=will@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).