Re: [PATCH bpf-next 1/5] bpf, x86: Add support for BPF_MOVSX in the i386 JIT

[bot+bpf-ci]

>     bpf, x86: Add support for BPF_MOVSX in the i386 JIT
>
>     The i386 JIT lowers register BPF_MOVSX as an ordinary move because
>     do_jit() does not inspect insn->off. The generated code therefore copies
>     the source instead of sign-extending the selected low bits. Context-access
>     conversion can also introduce MOVSX while lowering signed loads, so
>     rejecting BPF_MEMSX does not protect those paths.
>
>     Stage the source low word in EAX and use the native byte and word
>     sign-extension instructions. Derive the high word from the sign bit for
>     ALU64 and retain verifier-managed zero extension for ALU32.
>
>     Assisted-by: Codex:gpt-6
>     Signed-off-by: Nicholas Dudar <main.kalliope@xxxxxxxxx>

This looks like a bug fix for MOVSX support that was added to x86_64 but
not i386. Should this include:

  Fixes: 8100928c8814 ("bpf: Support new sign-extension mov insns")

> diff --git a/arch/x86/net/bpf_jit_comp32.c b/arch/x86/net/bpf_jit_comp32.c
> index 852baf2e4db4d..9cbeabac4f27f 100644
> --- a/arch/x86/net/bpf_jit_comp32.c
> +++ b/arch/x86/net/bpf_jit_comp32.c
> @@ -266,6 +266,51 @@ static inline void emit_ia32_mov_r64(const bool is64, const u8 dst[],
>  		emit_ia32_mov_i(dst_hi, 0, dstk, pprog);
>  }
>
> +/* dst = sign_extend(src, insn->off) */
> +static inline void emit_ia32_movsx_r64(const struct bpf_insn *insn, u8 **pprog,
> +				       const struct bpf_prog_aux *aux)
> +{
> +	const u8 *dst = bpf2ia32[insn->dst_reg];
> +	const u8 *src = bpf2ia32[insn->src_reg];
> +	bool dstk = insn->dst_reg != BPF_REG_AX;
> +	bool sstk = insn->src_reg != BPF_REG_AX;
> +	bool is64 = BPF_CLASS(insn->code) == BPF_ALU64;

This isn't a bug, but would it read more consistently with the
neighbouring emitters to pass is64/dst/src/dstk/sstk as parameters?

Looking at the other ALU emitters in this file, they take the
already-decoded operands directly:

  emit_ia32_mov_r64(is64, dst, src, dstk, sstk, pprog, aux)
  emit_ia32_alu_r64(...)
  emit_ia32_mul_r64(...)

The new helper re-derives these five values from insn, even though
do_jit already has them in scope at the call site (lines 1719-1724).
The insn->off validation also happens twice: once in the do_jit gate
and again in the helper's switch.

> +	u8 *prog = *pprog;
> +	int cnt = 0;
> +
> +	if (sstk)
> +		/* mov eax,dword ptr [ebp+off] */
> +		EMIT3(0x8B, add_2reg(0x40, IA32_EBP, IA32_EAX),
> +		      STACK_VAR(src_lo));
> +	else
> +		/* mov eax,src_lo */
> +		EMIT2(0x89, add_2reg(0xC0, IA32_EAX, src_lo));
> +
> +	switch (insn->off) {
> +	case 8:
> +		/* movsx eax,al */
> +		EMIT3(0x0F, 0xBE, 0xC0);
> +		break;
> +	case 16:
> +		/* movsx eax,ax */
> +		EMIT3(0x0F, 0xBF, 0xC0);
> +		break;
> +	case 32:
> +		/* EAX already holds the low word; CDQ supplies the high word. */
> +		break;
> +	}
> +
> +	emit_ia32_mov_r(dst_lo, IA32_EAX, dstk, false, &prog);
> +	if (is64) {
> +		EMIT1(0x99); /* cdq */
> +		emit_ia32_mov_r(dst_hi, IA32_EDX, dstk, false, &prog);
> +	} else if (!aux->verifier_zext) {
> +		emit_ia32_mov_i(dst_hi, 0, dstk, &prog);
> +	}
> +
> +	*pprog = prog;
> +}

[ ... ]


---
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/34921540254