On Thu, 6 Feb 2025 13:33:07 +0100
Peter Zijlstra <pet...@infradead.org> wrote:
> On Thu, Feb 06, 2025 at 01:13:28PM +0100, Peter Zijlstra wrote:
>
> > No, it just cleans up this utter trainwreck. I also noticed this is a
> > second (new) copy of this garbage. Clearly I didn't yell loud enough
> > last time and people didn't think to vomit when doing the copy/paste :/
> >
> > Function will look something like:
> >
> > unsigned long arch_ftrace_get_symaddr(unsigned long fentry_ip)
> > {
> > if (is_endbr(fentry_ip - ENDBR_INSN_SIZE))
> > fentry_op -= ENDBR_INSN_SIZE;
> > return fentry_ip;
> > }
> >
> > Let me finish local build test before I push out.
>
> Bah, still waiting for a LLVM build, but patch should be here:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git x86/ibt
>
> I was supposed to merge this last cycle, but then akpm shat all over
> arch/x86/kernel/alternative.c and we had to clean that up :/
>
> Notably, this patch, not sure it applies out of order.
>
Ah, this looks good to me except for the removing redundant asm/ibt.h
which Haiyue pointed.
Acked-by: Masami Hiramatsu (Google) <mhira...@kernel.org>
Thanks,
> ---
>
> Subject: x86/ibt: Clean up is_endbr()
> From: Peter Zijlstra <pet...@infradead.org>
> Date: Mon Nov 27 09:58:06 CET 2023
>
> Pretty much every caller of is_endbr() actually wants to test something at an
> address and ends up doing get_kernel_nofault(). Fold the lot into a more
> convenient helper.
>
> Signed-off-by: Peter Zijlstra (Intel) <pet...@infradead.org>
> Acked-by: Alexei Starovoitov <a...@kernel.org>
> Acked-by: Andrii Nakryiko <and...@kernel.org>
> ---
> arch/x86/events/core.c | 2 +-
> arch/x86/include/asm/ftrace.h | 17 +++--------------
> arch/x86/include/asm/ibt.h | 5 +++--
> arch/x86/kernel/alternative.c | 20 ++++++++++++++------
> arch/x86/kernel/kprobes/core.c | 11 +----------
> arch/x86/net/bpf_jit_comp.c | 4 ++--
> kernel/trace/bpf_trace.c | 20 +++-----------------
> 7 files changed, 27 insertions(+), 52 deletions(-)
>
> --- a/arch/x86/events/core.c
> +++ b/arch/x86/events/core.c
> @@ -2857,7 +2857,7 @@ static bool is_uprobe_at_func_entry(stru
> return true;
>
> /* endbr64 (64-bit only) */
> - if (user_64bit_mode(regs) && is_endbr(*(u32 *)auprobe->insn))
> + if (user_64bit_mode(regs) && is_endbr((u32 *)auprobe->insn))
> return true;
>
> return false;
> --- a/arch/x86/include/asm/ftrace.h
> +++ b/arch/x86/include/asm/ftrace.h
> @@ -2,6 +2,7 @@
> #ifndef _ASM_X86_FTRACE_H
> #define _ASM_X86_FTRACE_H
>
> +#include "asm/ibt.h"
> #include <asm/ptrace.h>
>
> #ifdef CONFIG_FUNCTION_TRACER
> @@ -36,21 +37,9 @@ static inline unsigned long ftrace_call_
>
> static inline unsigned long arch_ftrace_get_symaddr(unsigned long fentry_ip)
> {
> -#ifdef CONFIG_X86_KERNEL_IBT
> - u32 instr;
> + if (is_endbr((void*)(fentry_ip - ENDBR_INSN_SIZE)))
> + fentry_ip -= ENDBENDBR_INSN_SIZE;
>
> - /* We want to be extra safe in case entry ip is on the page edge,
> - * but otherwise we need to avoid get_kernel_nofault()'s overhead.
> - */
> - if ((fentry_ip & ~PAGE_MASK) < ENDBR_INSN_SIZE) {
> - if (get_kernel_nofault(instr, (u32 *)(fentry_ip -
> ENDBR_INSN_SIZE)))
> - return fentry_ip;
> - } else {
> - instr = *(u32 *)(fentry_ip - ENDBR_INSN_SIZE);
> - }
> - if (is_endbr(instr))
> - fentry_ip -= ENDBR_INSN_SIZE;
> -#endif
> return fentry_ip;
> }
> #define ftrace_get_symaddr(fentry_ip)
> arch_ftrace_get_symaddr(fentry_ip)
> --- a/arch/x86/include/asm/ibt.h
> +++ b/arch/x86/include/asm/ibt.h
> @@ -65,7 +65,7 @@ static __always_inline __attribute_const
> return 0x001f0f66; /* osp nopl (%rax) */
> }
>
> -static inline bool is_endbr(u32 val)
> +static inline bool __is_endbr(u32 val)
> {
> if (val == gen_endbr_poison())
> return true;
> @@ -74,6 +74,7 @@ static inline bool is_endbr(u32 val)
> return val == gen_endbr();
> }
>
> +extern __noendbr bool is_endbr(u32 *val);
> extern __noendbr u64 ibt_save(bool disable);
> extern __noendbr void ibt_restore(u64 save);
>
> @@ -98,7 +99,7 @@ extern __noendbr void ibt_restore(u64 sa
>
> #define __noendbr
>
> -static inline bool is_endbr(u32 val) { return false; }
> +static inline bool is_endbr(u32 *val) { return false; }
>
> static inline u64 ibt_save(bool disable) { return 0; }
> static inline void ibt_restore(u64 save) { }
> --- a/arch/x86/kernel/alternative.c
> +++ b/arch/x86/kernel/alternative.c
> @@ -852,16 +852,24 @@ void __init_or_module noinline apply_ret
>
> #ifdef CONFIG_X86_KERNEL_IBT
>
> +__noendbr bool is_endbr(u32 *val)
> +{
> + u32 endbr;
> +
> + __get_kernel_nofault(&endbr, val, u32, Efault);
> + return __is_endbr(endbr);
> +
> +Efault:
> + return false;
> +}
> +
> static void poison_cfi(void *addr);
>
> static void __init_or_module poison_endbr(void *addr, bool warn)
> {
> - u32 endbr, poison = gen_endbr_poison();
> -
> - if (WARN_ON_ONCE(get_kernel_nofault(endbr, addr)))
> - return;
> + u32 poison = gen_endbr_poison();
>
> - if (!is_endbr(endbr)) {
> + if (!is_endbr(addr)) {
> WARN_ON_ONCE(warn);
> return;
> }
> @@ -984,7 +992,7 @@ static u32 cfi_seed __ro_after_init;
> static u32 cfi_rehash(u32 hash)
> {
> hash ^= cfi_seed;
> - while (unlikely(is_endbr(hash) || is_endbr(-hash))) {
> + while (unlikely(__is_endbr(hash) || __is_endbr(-hash))) {
> bool lsb = hash & 1;
> hash >>= 1;
> if (lsb)
> --- a/arch/x86/kernel/kprobes/core.c
> +++ b/arch/x86/kernel/kprobes/core.c
> @@ -373,16 +373,7 @@ static bool can_probe(unsigned long padd
> kprobe_opcode_t *arch_adjust_kprobe_addr(unsigned long addr, unsigned long
> offset,
> bool *on_func_entry)
> {
> - u32 insn;
> -
> - /*
> - * Since 'addr' is not guaranteed to be safe to access, use
> - * copy_from_kernel_nofault() to read the instruction:
> - */
> - if (copy_from_kernel_nofault(&insn, (void *)addr, sizeof(u32)))
> - return NULL;
> -
> - if (is_endbr(insn)) {
> + if (is_endbr((u32 *)addr)) {
> *on_func_entry = !offset || offset == 4;
> if (*on_func_entry)
> offset = 4;
> --- a/arch/x86/net/bpf_jit_comp.c
> +++ b/arch/x86/net/bpf_jit_comp.c
> @@ -641,7 +641,7 @@ int bpf_arch_text_poke(void *ip, enum bp
> * See emit_prologue(), for IBT builds the trampoline hook is preceded
> * with an ENDBR instruction.
> */
> - if (is_endbr(*(u32 *)ip))
> + if (is_endbr(ip))
> ip += ENDBR_INSN_SIZE;
>
> return __bpf_arch_text_poke(ip, t, old_addr, new_addr);
> @@ -3036,7 +3036,7 @@ static int __arch_prepare_bpf_trampoline
> /* skip patched call instruction and point orig_call to actual
> * body of the kernel function.
> */
> - if (is_endbr(*(u32 *)orig_call))
> + if (is_endbr(orig_call))
> orig_call += ENDBR_INSN_SIZE;
> orig_call += X86_PATCH_SIZE;
> }
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -1038,27 +1038,13 @@ static const struct bpf_func_proto bpf_g
> .arg1_type = ARG_PTR_TO_CTX,
> };
>
> -#ifdef CONFIG_X86_KERNEL_IBT
> -static unsigned long get_entry_ip(unsigned long fentry_ip)
> +static inline unsigned long get_entry_ip(unsigned long fentry_ip)
> {
> - u32 instr;
> -
> - /* We want to be extra safe in case entry ip is on the page edge,
> - * but otherwise we need to avoid get_kernel_nofault()'s overhead.
> - */
> - if ((fentry_ip & ~PAGE_MASK) < ENDBR_INSN_SIZE) {
> - if (get_kernel_nofault(instr, (u32 *)(fentry_ip -
> ENDBR_INSN_SIZE)))
> - return fentry_ip;
> - } else {
> - instr = *(u32 *)(fentry_ip - ENDBR_INSN_SIZE);
> - }
> - if (is_endbr(instr))
> + if (is_endbr((void *)(fentry_ip - ENDBR_INSN_SIZE)))
> fentry_ip -= ENDBR_INSN_SIZE;
> +
> return fentry_ip;
> }
> -#else
> -#define get_entry_ip(fentry_ip) fentry_ip
> -#endif
>
> BPF_CALL_1(bpf_get_func_ip_kprobe, struct pt_regs *, regs)
> {
>
--
Masami Hiramatsu (Google) <mhira...@kernel.org>
