Re: [PATCH v6] m68k, bpf: Add initial BPF JIT compiler support

[bot+bpf-ci]

> diff --git a/arch/m68k/net/bpf_jit_comp.c b/arch/m68k/net/bpf_jit_comp.c
> new file mode 100644
> index 000000000000..a04ce91eb8e8
> --- /dev/null
> +++ b/arch/m68k/net/bpf_jit_comp.c

[ ... ]

> +static int jit_atomic32(u32 *ptr, u32 val, int op, u32 cmp)
> +{
> +	unsigned long flags;
> +	u32 old;
> +
> +	local_irq_save(flags);
> +	old = *ptr;
> +	switch (op) {
> +	case BPF_ADD:
> +		*ptr = old + val;
> +		break;
> +	case BPF_AND:
> +		*ptr = old & val;
> +		break;
> +	case BPF_OR:
> +		*ptr = old | val;
> +		break;
> +	case BPF_XOR:
> +		*ptr = old ^ val;
> +		break;
> +	case BPF_XCHG:
> +		*ptr = val;
> +		break;
> +	case BPF_CMPXCHG:
> +		if (old == cmp)
> +			*ptr = val;
> +		break;
> +	}
> +	local_irq_restore(flags);
> +
> +	return old;
> +}

[ ... ]

> +static void emit_atomic(const struct bpf_insn *insn, struct jit_ctx *ctx)
> +{
> +	const s8 *src = bpf_get_reg64_ref(insn->src_reg, tmp2, ctx);
> +	const s8 *dst = bpf_get_reg64_ref(insn->dst_reg, tmp1, ctx);
> +
> +	if (BPF_SIZE(insn->code) == BPF_W) {
> +		s8 s_lo = bpf_get_reg32(src[1], tmp2[1], ctx);
> +
> +		if (insn->imm != BPF_CMPXCHG) {
> +			if (is_stacked(dst[1])) {
> +				emit_16(ctx, 0x206e);		/* movea.l d16(%fp), %a0 */
> +				emit_16(ctx, (u16)STACK_OFFSET(dst[1]));
> +			} else {
> +				emit_16(ctx, 0x2040 | dst[1]);	/* movea.l dst, %a0 */
> +			}
> +
> +			emit_16(ctx, 0xd1fc);			/* adda.l #imm, %a0 */
> +			emit_16(ctx, (u16)((s16)insn->off >> 16));
> +			emit_16(ctx, (u16)insn->off);
> +
> +			if (BPF_OP(insn->code) == BPF_XCHG) {
> +				emit_16(ctx, 0x4eb9);
> +				emit_32(ctx, (u32)jit_atomic32);
> +			} else {
> +				emit_16(ctx, 0x2f00 | s_lo);
> +				emit_16(ctx, 0x2f3c);
> +				emit_32(ctx, insn->imm);
> +				emit_16(ctx, 0x2f08);
> +				emit_16(ctx, 0x4eb9);
> +				emit_32(ctx, (u32)jit_atomic32);
> +				emit_16(ctx, 0x4fef);
> +				emit_16(ctx, 12);
> +			}

[ ... ]

> +	case BPF_STX | BPF_ATOMIC | BPF_W:
> +	case BPF_STX | BPF_ATOMIC | BPF_DW:
> +		emit_atomic(insn, ctx);
> +		break;

Can this code handle 32-bit BPF_LOAD_ACQ and BPF_STORE_REL atomics?

Looking at build_insn(), every BPF_STX | BPF_ATOMIC | BPF_W instruction is
sent to emit_atomic() without checking insn->imm.

On 32-bit arches, the verifier accepts BPF_W-sized BPF_LOAD_ACQ (0x100)
and BPF_STORE_REL (0x110). In check_atomic() from kernel/bpf/verifier.c,
these operations are only rejected when BPF_SIZE is BPF_DW and
BITS_PER_LONG != 64:

    case BPF_LOAD_ACQ:
        if (BPF_SIZE(insn->code) == BPF_DW && BITS_PER_LONG != 64) {
            ...
            return -EOPNOTSUPP;
        }
        return check_atomic_load(env, insn);

For BPF_LOAD_ACQ, emit_atomic() loads %a0 from dst_reg + off and calls
jit_atomic32(ptr = dst_reg + off, val = src, op = 0x100, cmp). The switch
in jit_atomic32() has no case for 0x100, so it only runs `old = *ptr;` on
dst_reg + off.

But dst_reg is the destination register for a load. check_load_mem() in
the verifier only checks dst_reg with DST_OP_NO_MARK, so it can hold any
scalar or be uninitialized. The resulting dereference of dst_reg + off
(instead of src_reg + off) can read from an arbitrary address with no
exception-table entry.

Also, is_fetch is false (0x100 & BPF_FETCH == 0), so dst_reg is never
written back. The verifier tracks dst_reg as holding the loaded value, but
at runtime it keeps its old value.

For BPF_STORE_REL, jit_atomic32() is called with op = 0x110. No case
matches, so the store is silently dropped.

Other 32-bit JITs (mips32, x86_32, riscv32, parisc32) reject unsupported
atomic imm values. Could emit_atomic() or build_insn() accept only the
imm values that jit_atomic32/jit_atomic64 implement and return -EOPNOTSUPP
for the rest?

[ ... ]

> +static void build_prologue(struct jit_ctx *ctx)
> +{
> +	int bpf_stack = MAX_BPF_STACK;
> +	int total_stack = SCRATCH_SIZE + bpf_stack;

Can this stack reservation overflow the kernel stack?

Every function reserves SCRATCH_SIZE + MAX_BPF_STACK = 96 + 512 = 608
bytes, plus 16 bytes for d2-d5, 8 bytes for the return address and saved
%fp, and emit_call() pushes 40 bytes of arguments for each bpf-to-bpf
call.

That's 632 bytes for the main frame and 672 bytes for each nested subprog
frame, independent of prog->aux->stack_depth.

The verifier allows MAX_CALL_FRAMES (16) nested frames. For CAP_BPF
programs, it rounds each frame's stack_depth up to 16, so subprogs with no
stack use cost nothing against the 512-byte combined limit.

A program with a chain of 16 small static subprogs is therefore accepted
and uses 632 + 15 * 672 = 10712 bytes of kernel stack on top of the
caller's stack.

On m68k, THREAD_SIZE is 8KB for every configuration where this JIT is
enabled, thread_info sits at the base of the stack, and there is no guard
page. Even 12 levels (632 + 11 * 672 = 8024 bytes) overflow by themselves.

Shallower nesting can overflow when a program runs in softirq on top of an
already deep stack. Callbacks invoked from helpers such as bpf_loop add a
C frame plus another full JIT frame per level.

The main entry needs MAX_BPF_STACK because a tail-call target reuses its
frame. Subprogs can never be tail-call targets, and they cannot issue tail
calls here because bpf_jit_supports_subprog_tailcalls() is the weak false
default.

Could subprog frames be sized from the actual stack_depth instead of always
reserving MAX_BPF_STACK?

[ ... ]

> +	case BPF_JMP | BPF_CALL:
> +		return emit_call(insn, ctx);

Does this code copy back the callee's R2 for aggregate returns?

For a subprog whose BTF return type is 9-16 bytes,
bpf_compute_subprog_ret_regs() in kernel/bpf/verifier.c sets
subprog_info->ret_reg_pair and prog->jit_required.

On exit, prepare_func_exit() copies the callee's R2 state into the caller,
including pointer types for static subprogs.

In this JIT, BPF R2 is not a machine register. It is a slot at
STACK_OFFSET() from %fp, so each frame has its own copy.

The callee writes R2 into its own frame, BPF_EXIT runs build_epilogue(),
and unlk discards that frame. emit_call() only fixes up d0/d1 for R0 and
never copies R2 back.

The caller's R2 slot keeps its old value (for example the argument it
passed in).

This creates a verifier/runtime mismatch. For example, after
'r2 = 0xdeadbeef; call sub' where sub returns {r0, r2 = map_value_ptr},
the verifier thinks r2 is a PTR_TO_MAP_VALUE in '*(u64 *)(r2 + 0) = x',
but at run time the store goes to 0xdeadbeef.

Because jit_required is set, there is no interpreter fallback.

Every other JIT that supports BPF_PSEUDO_CALL keeps R2 in a hardware
register, and arm32/x86_32 reject subprog calls.

Could the callee's R2 be copied into the caller's R2 slot after a
BPF_PSEUDO_CALL, for example by returning it in d2/d3 or through a scratch
location?

[ ... ]

> +	default:
> +		return -EOPNOTSUPP;
> +	}

Does this code handle the BPF_ST | BPF_NOSPEC instruction?

build_insn() has no 'case BPF_ST | BPF_NOSPEC:', so the speculation
barrier the verifier inserts falls to the default and returns -EOPNOTSUPP.

m68k does not override bpf_jit_bypass_spec_v1() or v4(), which default to
false.

For any program loaded without CAP_PERFMON while mitigations are on
(unprivileged, CAP_BPF-only, or through a BPF token), the verifier emits
BPF_ST_NOSPEC() for speculative-path sanitation and after stack spills, in
kernel/bpf/fixups.c:958-962 and 1035-1042.

build_body() then fails and the whole program is not JITed. With
CONFIG_BPF_JIT_ALWAYS_ON, which this patch makes selectable by adding
HAVE_EBPF_JIT, and whenever prog->jit_required is set, the load fails with
-ENOTSUPP instead of falling back to the interpreter.

test_bpf.ko runs as root and so never hits this.

Every other eBPF JIT, including arm32, mips32 and x86_32, handles
BPF_ST | BPF_NOSPEC as a no-op or a barrier.

Could this case be added, or should bpf_jit_bypass_spec_v1/v4() be
overridden if m68k does not speculate?


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/36350420232