[PATCH bpf-next] bpf, arm32: build constants inline instead of using a literal pool

[PATCH bpf-next] bpf, arm32: build constants inline instead of using a literal pool

[Ramin Moussavi]

ARMv5 and ARMv6 have no MOVW/MOVT, so emit_mov_i_no8m() put every
constant that is not an imm8m into a literal pool behind the epilogue and
loaded it with LDR rX, [pc, #off].  That offset field is 12 bits and the
pool sits at the end of the image, so from roughly 4 KB of JITed code on,
the load no longer reaches it.
Commit 0b59d8806a31 ("ARM: net: delegate filter to kernel interpreter
when imm_offset() return value can't fit into 12bits.") handles that by
setting FLAG_IMM_OVERFLOW and dropping the whole program to the
interpreter, and already noted that multiple pools "would better be done
as a feature instead of fix".

Build the constant in the register instead: a single MVN when the
inverted value is an imm8m, otherwise one MOV/ORR per non-zero byte,
since a byte at a byte-aligned position is always an imm8m.  The arm64
JIT does the same in emit_a64_mov_i64() with MOVZ/MOVN and MOVK.  Without
a pool there is no offset to overflow, so programs of any size are
translated, and FLAG_IMM_OVERFLOW, ctx->imms, ctx->imm_count and
ctx->epilogue_bytes go away with it.

It costs up to four instructions per hard constant instead of one.  The
pool was not much cheaper: imm_offset() counts duplicates on the sizing
pass, so every use already took 4 bytes of LDR plus 4 bytes of slot.

The 4 KB ceiling is low enough to be hit by the kernel itself.  An ARMv5
kernel fails to boot when CONFIG_BPF_JIT_ALWAYS_ON=y and
CONFIG_NET_PTP_CLASSIFY=y are set together: the first leaves no
interpreter to fall back to, the second makes sock_init() build the PTP
classifier, which needs 5124 bytes of ARM code.  bpf_prog_create() then
returns -ENOTSUPP and ptp_classifier_init() turns that into a BUG_ON.
Neither option is unusual - NET_PTP_CLASSIFY is selected by
PTP_1588_CLOCK, which defaults to ETHERNET, which defaults to y:

  kernel BUG at net/core/ptp_classifier.c:223!
  PC is at ptp_classifier_init+0x3c/0x70
   ptp_classifier_init from sock_init+0xac/0xf0

seccomp reaches the JIT the same way, so filters of that size are likely
rejected there as well; that was not measured.

ARMv7 and later are unaffected, the MOVW/MOVT path is unchanged.

Tested on versatile_defconfig (ARM926EJ-S) under qemu with
CONFIG_BPF_JIT_ALWAYS_ON=y: the kernel boots again and SO_ATTACH_FILTER
accepts classic filters up to BPF_MAXINSNS, all of which the JIT now
translates.  That is the classic limit of 4096 instructions; eBPF
programs loaded through bpf() can be far larger and were not part of the
test.  The emitted sequences were verified separately by executing them
for 6.2 million values.

Assisted-by: Claude:claude-opus-5
Signed-off-by: Ramin Moussavi <ramin.moussavi@yacoub.de>
---
 arch/arm/net/bpf_jit_32.c | 114 +++++++++-----------------------------
 arch/arm/net/bpf_jit_32.h |   3 +
 2 files changed, 29 insertions(+), 88 deletions(-)

diff --git a/arch/arm/net/bpf_jit_32.c b/arch/arm/net/bpf_jit_32.c
index 9ede81afbc50..d3b5d7745068 100644
--- a/arch/arm/net/bpf_jit_32.c
+++ b/arch/arm/net/bpf_jit_32.c
@@ -130,8 +130,6 @@ enum {
 #define TMP_REG_2	(MAX_BPF_JIT_REG + 1)	/* TEMP Register 2 */
 #define TCALL_CNT	(MAX_BPF_JIT_REG + 2)	/* Tail Call Count */
 
-#define FLAG_IMM_OVERFLOW	(1 << 0)
-
 /*
  * Map eBPF registers to ARM 32bit registers or stack scratch space.
  *
@@ -193,10 +191,6 @@ static const s8 bpf2a32[][2] = {
  * offsets		:	array of eBPF instruction offsets in
  *				JITed code.
  * target		:	final JITed code.
- * epilogue_bytes	:	no of bytes used in epilogue.
- * imm_count		:	no of immediate counts used for global
- *				variables.
- * imms			:	array of global variable addresses.
  */
 
 struct jit_ctx {
@@ -209,11 +203,6 @@ struct jit_ctx {
 	u32 *offsets;
 	u32 *target;
 	u32 stack_size;
-#if __LINUX_ARM_ARCH__ < 7
-	u16 epilogue_bytes;
-	u16 imm_count;
-	u32 *imms;
-#endif
 };
 
 /*
@@ -404,53 +393,6 @@ static void jit_fill_hole(void *area, unsigned int size)
 #define _STACK_SIZE	(ctx->prog->aux->stack_depth + SCRATCH_SIZE)
 #define STACK_SIZE	ALIGN(_STACK_SIZE, STACK_ALIGNMENT)
 
-#if __LINUX_ARM_ARCH__ < 7
-
-static u16 imm_offset(u32 k, struct jit_ctx *ctx)
-{
-	unsigned int i = 0, offset;
-	u16 imm;
-
-	/* on the "fake" run we just count them (duplicates included) */
-	if (ctx->target == NULL) {
-		ctx->imm_count++;
-		return 0;
-	}
-
-	while ((i < ctx->imm_count) && ctx->imms[i]) {
-		if (ctx->imms[i] == k)
-			break;
-		i++;
-	}
-
-	if (ctx->imms[i] == 0)
-		ctx->imms[i] = k;
-
-	/* constants go just after the epilogue */
-	offset =  ctx->offsets[ctx->prog->len - 1] * 4;
-	offset += ctx->prologue_bytes;
-	offset += ctx->epilogue_bytes;
-	offset += i * 4;
-
-	ctx->target[offset / 4] = k;
-
-	/* PC in ARM mode == address of the instruction + 8 */
-	imm = offset - (8 + ctx->idx * 4);
-
-	if (imm & ~0xfff) {
-		/*
-		 * literal pool is too far, signal it into flags. we
-		 * can only detect it on the second pass unfortunately.
-		 */
-		ctx->flags |= FLAG_IMM_OVERFLOW;
-		return 0;
-	}
-
-	return imm;
-}
-
-#endif /* __LINUX_ARM_ARCH__ */
-
 static inline int bpf2a32_offset(int bpf_to, int bpf_from,
 				 const struct jit_ctx *ctx) {
 	int to, from;
@@ -469,7 +411,30 @@ static inline int bpf2a32_offset(int bpf_to, int bpf_from,
 static inline void emit_mov_i_no8m(const u8 rd, u32 val, struct jit_ctx *ctx)
 {
 #if __LINUX_ARM_ARCH__ < 7
-	emit(ARM_LDR_I(rd, ARM_PC, imm_offset(val, ctx)), ctx);
+	/* A single MVN if the inverted value is an imm8m, else one
+	 * MOV/ORR per non-zero byte -- a byte is always an imm8m.
+	 */
+	int imm12 = imm8m(~val);
+	unsigned int shift;
+	bool first = true;
+
+	if (imm12 >= 0) {
+		emit(ARM_MVN_I(rd, imm12), ctx);
+		return;
+	}
+	for (shift = 0; shift < 32; shift += 8) {
+		u32 part = val & (0xffU << shift);
+
+		if (!part)
+			continue;
+		if (first)
+			emit(ARM_MOV_I(rd, imm8m(part)), ctx);
+		else
+			emit(ARM_ORR_I(rd, rd, imm8m(part)), ctx);
+		first = false;
+	}
+	if (first)
+		emit(ARM_MOV_I(rd, 0), ctx);
 #else
 	emit(ARM_MOVW(rd, val & 0xffff), ctx);
 	if (val > 0xffff)
@@ -2091,14 +2056,6 @@ static int build_insn(const struct bpf_insn *insn, struct jit_ctx *ctx)
 		pr_err_once("unknown opcode %02x\n", code);
 		return -EINVAL;
 	}
-
-	if (ctx->flags & FLAG_IMM_OVERFLOW)
-		/*
-		 * this instruction generated an overflow when
-		 * trying to access the literal pool, so
-		 * delegate this filter to the kernel interpreter.
-		 */
-		return -1;
 	return 0;
 }
 
@@ -2192,21 +2149,7 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_verifier_env *env, struct bpf_pr
 
 	ctx.epilogue_offset = ctx.idx;
 
-#if __LINUX_ARM_ARCH__ < 7
-	tmp_idx = ctx.idx;
 	build_epilogue(&ctx);
-	ctx.epilogue_bytes = (ctx.idx - tmp_idx) * 4;
-
-	ctx.idx += ctx.imm_count;
-	if (ctx.imm_count) {
-		ctx.imms = kcalloc(ctx.imm_count, sizeof(u32), GFP_KERNEL);
-		if (ctx.imms == NULL)
-			goto out_off;
-	}
-#else
-	/* there's nothing about the epilogue on ARMv7 */
-	build_epilogue(&ctx);
-#endif
 	/* Now we can get the actual image size of the JITed arm code.
 	 * Currently, we are not considering the THUMB-2 instructions
 	 * for jit, although it can decrease the size of the image.
@@ -2224,7 +2167,7 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_verifier_env *env, struct bpf_pr
 	 * we must fall back to the interpretation
 	 */
 	if (header == NULL)
-		goto out_imms;
+		goto out_off;
 
 	/* 2.) Actual pass to generate final JIT code */
 	ctx.target = (u32 *) image_ptr;
@@ -2254,11 +2197,6 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_verifier_env *env, struct bpf_pr
 	prog->jited = 1;
 	prog->jited_len = image_size;
 
-out_imms:
-#if __LINUX_ARM_ARCH__ < 7
-	if (ctx.imm_count)
-		kfree(ctx.imms);
-#endif
 out_off:
 	kfree(ctx.offsets);
 
@@ -2267,6 +2205,6 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_verifier_env *env, struct bpf_pr
 out_free:
 	image_ptr = NULL;
 	bpf_jit_binary_free(header);
-	goto out_imms;
+	goto out_off;
 }
 
diff --git a/arch/arm/net/bpf_jit_32.h b/arch/arm/net/bpf_jit_32.h
index 438f0e1f91a0..76d83b486ded 100644
--- a/arch/arm/net/bpf_jit_32.h
+++ b/arch/arm/net/bpf_jit_32.h
@@ -104,6 +104,7 @@
 #define ARM_INST_MOV_I		0x03a00000
 #define ARM_INST_MOVW		0x03000000
 #define ARM_INST_MOVT		0x03400000
+#define ARM_INST_MVN_I		0x03e00000
 
 #define ARM_INST_MUL		0x00000090
 
@@ -231,6 +232,8 @@
 #define ARM_MOVT(rd, imm)	\
 	(ARM_INST_MOVT | ((imm) >> 12) << 16 | (rd) << 12 | ((imm) & 0x0fff))
 
+#define ARM_MVN_I(rd, imm)	_AL3_I(ARM_INST_MVN, rd, 0, imm)
+
 #define ARM_MUL(rd, rm, rn)	(ARM_INST_MUL | (rd) << 16 | (rm) << 8 | (rn))
 
 #define ARM_POP(regs)		(ARM_INST_POP | (regs))
-- 
2.53.0

Re: [PATCH bpf-next] bpf, arm32: build constants inline instead of using a literal pool

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Low] bpf, arm32: non-standard multi-line comment style in
  emit_mov_i_no8m()
- [Low] bpf, arm32: unused flags field left in struct jit_ctx
--

commit 04b7597139a2756fa5ffbad42747333b65860a8c
Author: Ramin Moussavi <ramin.moussavi@yacoub.de>

bpf, arm32: build constants inline instead of using a literal pool

This commit replaces the literal pool used for non-imm8m constants on
ARMv5 and ARMv6 with inline MVN/MOV/ORR instruction sequences and removes
the literal pool tracking fields and overflow fallback.

> diff --git a/arch/arm/net/bpf_jit_32.c b/arch/arm/net/bpf_jit_32.c
> index 9ede81afbc50e..d3b5d77450687 100644
> --- a/arch/arm/net/bpf_jit_32.c
> +++ b/arch/arm/net/bpf_jit_32.c
> @@ -130,8 +130,6 @@ enum {
>  #define TMP_REG_2	(MAX_BPF_JIT_REG + 1)	/* TEMP Register 2 */
>  #define TCALL_CNT	(MAX_BPF_JIT_REG + 2)	/* Tail Call Count */
>  
> -#define FLAG_IMM_OVERFLOW	(1 << 0)
> -

[ ... ]

> @@ -209,11 +203,6 @@ struct jit_ctx {
>  	u32 *offsets;
>  	u32 *target;
>  	u32 stack_size;
> -#if __LINUX_ARM_ARCH__ < 7
> -	u16 epilogue_bytes;
> -	u16 imm_count;
> -	u32 *imms;
> -#endif
>  };

[Severity: Low]
With FLAG_IMM_OVERFLOW removed, is struct jit_ctx's flags member now
unused as well?  Nothing else in arch/arm/net/bpf_jit_32.c reads or writes
ctx->flags, so it can be dropped alongside epilogue_bytes, imm_count, and
imms.

[ ... ]

> @@ -469,7 +411,30 @@ static inline int bpf2a32_offset(int bpf_to, int bpf_from,
>  static inline void emit_mov_i_no8m(const u8 rd, u32 val, struct jit_ctx *ctx)
>  {
>  #if __LINUX_ARM_ARCH__ < 7
> -	emit(ARM_LDR_I(rd, ARM_PC, imm_offset(val, ctx)), ctx);
> +	/* A single MVN if the inverted value is an imm8m, else one
> +	 * MOV/ORR per non-zero byte -- a byte is always an imm8m.
> +	 */

[Severity: Low]
This isn't a bug, but BPF code uses the multi-line comment style where
the opening /* sits on its own line and the comment text starts on the
next line.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111121621.665533-1-ramin.moussavi@yacoub.de?part=1