Re: [PATCH bpf-next v3 5/7] bpf, arm64: Support to poke bpf prog
From: Jakub Sitnicki <jakub@cloudflare.com>
Date: 2022-05-10 11:45:41
Also in:
bpf, linux-arm-kernel, linux-kselftest, lkml
Thanks for incorporating the attach to BPF progs bits into the series. I have a couple minor comments. Please see below. On Sun, Apr 24, 2022 at 11:40 AM -04, Xu Kuohai wrote:
quoted hunk ↗ jump to hunk
1. Set up the bpf prog entry in the same way as fentry to support trampoline. Now bpf prog entry looks like this: bti c // if BTI enabled mov x9, x30 // save lr nop // to be replaced with jump instruction paciasp // if PAC enabled 2. Update bpf_arch_text_poke() to poke bpf prog. If the instruction to be poked is bpf prog's first instruction, skip to the nop instruction in the prog entry. Signed-off-by: Xu Kuohai <redacted> --- arch/arm64/net/bpf_jit.h | 1 + arch/arm64/net/bpf_jit_comp.c | 41 +++++++++++++++++++++++++++-------- 2 files changed, 33 insertions(+), 9 deletions(-)diff --git a/arch/arm64/net/bpf_jit.h b/arch/arm64/net/bpf_jit.h index 194c95ccc1cf..1c4b0075a3e2 100644 --- a/arch/arm64/net/bpf_jit.h +++ b/arch/arm64/net/bpf_jit.h@@ -270,6 +270,7 @@ #define A64_BTI_C A64_HINT(AARCH64_INSN_HINT_BTIC) #define A64_BTI_J A64_HINT(AARCH64_INSN_HINT_BTIJ) #define A64_BTI_JC A64_HINT(AARCH64_INSN_HINT_BTIJC) +#define A64_NOP A64_HINT(AARCH64_INSN_HINT_NOP) /* DMB */ #define A64_DMB_ISH aarch64_insn_gen_dmb(AARCH64_INSN_MB_ISH)diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c index 3f9bdfec54c4..293bdefc5d0c 100644 --- a/arch/arm64/net/bpf_jit_comp.c +++ b/arch/arm64/net/bpf_jit_comp.c@@ -237,14 +237,23 @@ static bool is_lsi_offset(int offset, int scale) return true; } -/* Tail call offset to jump into */ -#if IS_ENABLED(CONFIG_ARM64_BTI_KERNEL) || \ - IS_ENABLED(CONFIG_ARM64_PTR_AUTH_KERNEL) -#define PROLOGUE_OFFSET 9 +#if IS_ENABLED(CONFIG_ARM64_BTI_KERNEL) +#define BTI_INSNS 1 +#else +#define BTI_INSNS 0 +#endif + +#if IS_ENABLED(CONFIG_ARM64_PTR_AUTH_KERNEL) +#define PAC_INSNS 1 #else -#define PROLOGUE_OFFSET 8 +#define PAC_INSNS 0 #endif
Above can be folded into: #define BTI_INSNS (IS_ENABLED(CONFIG_ARM64_BTI_KERNEL) ? 1 : 0) #define PAC_INSNS (IS_ENABLED(CONFIG_ARM64_PTR_AUTH_KERNEL) ? 1 : 0)
quoted hunk ↗ jump to hunk
+/* Tail call offset to jump into */ +#define PROLOGUE_OFFSET (BTI_INSNS + 2 + PAC_INSNS + 8) +/* Offset of nop instruction in bpf prog entry to be poked */ +#define POKE_OFFSET (BTI_INSNS + 1) + static int build_prologue(struct jit_ctx *ctx, bool ebpf_from_cbpf) { const struct bpf_prog *prog = ctx->prog;@@ -281,12 +290,15 @@ static int build_prologue(struct jit_ctx *ctx, bool ebpf_from_cbpf) * */ + if (IS_ENABLED(CONFIG_ARM64_BTI_KERNEL)) + emit(A64_BTI_C, ctx);
I'm no arm64 expert, but this looks like a fix for BTI. Currently we never emit BTI because ARM64_BTI_KERNEL depends on ARM64_PTR_AUTH_KERNEL, while BTI must be the first instruction for the jump target [1]. Am I following correctly? [1] https://lwn.net/Articles/804982/
quoted hunk ↗ jump to hunk
+ + emit(A64_MOV(1, A64_R(9), A64_LR), ctx); + emit(A64_NOP, ctx); + /* Sign lr */ if (IS_ENABLED(CONFIG_ARM64_PTR_AUTH_KERNEL)) emit(A64_PACIASP, ctx); - /* BTI landing pad */ - else if (IS_ENABLED(CONFIG_ARM64_BTI_KERNEL)) - emit(A64_BTI_C, ctx); /* Save FP and LR registers to stay align with ARM64 AAPCS */ emit(A64_PUSH(A64_FP, A64_LR, A64_SP), ctx);@@ -1552,9 +1564,11 @@ int bpf_arch_text_poke(void *ip, enum bpf_text_poke_type poke_type, u32 old_insn; u32 new_insn; u32 replaced; + unsigned long offset = ~0UL; enum aarch64_insn_branch_type branch_type; + char namebuf[KSYM_NAME_LEN]; - if (!is_bpf_text_address((long)ip)) + if (!__bpf_address_lookup((unsigned long)ip, NULL, &offset, namebuf)) /* Only poking bpf text is supported. Since kernel function * entry is set up by ftrace, we reply on ftrace to poke kernel * functions. For kernel funcitons, bpf_arch_text_poke() is only@@ -1565,6 +1579,15 @@ int bpf_arch_text_poke(void *ip, enum bpf_text_poke_type poke_type, */ return -EINVAL; + /* bpf entry */ + if (offset == 0UL) + /* skip to the nop instruction in bpf prog entry: + * bti c // if BTI enabled + * mov x9, x30 + * nop + */ + ip = (u32 *)ip + POKE_OFFSET;
This is very much personal preference, however, I find the use pointer
arithmetic too clever here. Would go for a more verbose:
offset = POKE_OFFSET * AARCH64_INSN_SIZE;
ip = (void *)((unsigned long)ip + offset);
+ if (poke_type == BPF_MOD_CALL) branch_type = AARCH64_INSN_BRANCH_LINK; else
I think it'd make more sense to merge this patch with patch 4 (the preceding one). Initial implementation of of bpf_arch_text_poke() from patch 4 is not fully functional, as it will always fail for bpf_arch_text_poke(ip, BPF_MOD_CALL, ...) calls. At least, I find it a bit confusing. Otherwise than that: Reviewed-by: Jakub Sitnicki <jakub@cloudflare.com>