Re: [PATCH v2 2/2] LoongArch: BPF: Optimize 32-bit operations using native instructions

[bot+bpf-ci]

> LoongArch: BPF: Optimize 32-bit operations using native instructions

TL;DR: LoongArch BPF JIT now feeds non-sign-extended operands to 32-bit
div/mod; on cores without DIV32 (e.g. 3A5000) results may be wrong, possibly
breaking verifier-proven bounds.

> diff --git a/arch/loongarch/net/bpf_jit.c b/arch/loongarch/net/bpf_jit.c
> --- a/arch/loongarch/net/bpf_jit.c
> +++ b/arch/loongarch/net/bpf_jit.c

[ ... ]

> @@ -752,212 +741,232 @@ static int build_insn(const struct bpf_insn *insn, struct jit_ctx *ctx, bool ext
>
>  	/* dst = imm */
>  	case BPF_ALU | BPF_MOV | BPF_K:
> +		move_imm(ctx, dst, (u32)imm);
> +		return 0;

[ ... ]

>  	/* dst = dst / src */
>  	case BPF_ALU | BPF_DIV | BPF_X:
> +		if (!off)
> +			emit_insn(ctx, divwu, dst, dst, src);
> +		else
> +			emit_insn(ctx, divw, dst, dst, src);
> +		break;
>  	case BPF_ALU64 | BPF_DIV | BPF_X:
> -		if (!off) {
> -			emit_zext_32(ctx, dst, is32);
> -			move_reg(ctx, t1, src);
> -			emit_zext_32(ctx, t1, is32);
> -			emit_insn(ctx, divdu, dst, dst, t1);
> -			emit_zext_32(ctx, dst, is32);
> -		} else {
> -			emit_sext_32(ctx, dst, is32);
> -			move_reg(ctx, t1, src);
> -			emit_sext_32(ctx, t1, is32);
> -			emit_insn(ctx, divd, dst, dst, t1);
> -			emit_zext_32(ctx, dst, is32);
> -		}
> +		if (!off)
> +			emit_insn(ctx, divdu, dst, dst, src);
> +		else
> +			emit_insn(ctx, divd, dst, dst, src);
>  		break;

Can divw/divwu be used directly on dst and src here?

The LoongArch ISA only defines the result of div.w, div.wu, mod.w and
mod.wu on LA64 when both rj and rk hold sign-extended 32-bit values.  The
exception is a core that reports CPUCFG2_DIV32 (BIT(26) in
arch/loongarch/include/asm/loongarch.h), where the upper 32 bits of the
inputs are ignored.  Cores without that bit, such as the LA464 in the
3A5000, may return any value otherwise.  Nothing in the JIT checks for
CPUCFG2_DIV32.

The operands here are often not sign-extended.  Every ALU32 result is now
zero-extended at the common exit of build_insn():

> +	if (is32)
> +		emit_insn(ctx, bstrpickd, dst, dst, 31, 0);
> +
>  	return 0;

so any 32-bit value with bit 31 set is left in the register as
0x00000000xxxxxxxx.  A BPF_LDX | BPF_W load (ldwu) is zero-extended in the
same way, and src can hold arbitrary upper bits coming from ALU64 ops or
pointers, since ALU32 only consumes the low 32 bits.

For example, with w0 = -6 followed by w0 s/= 3, rj is 0x00000000fffffffa
when divw executes, so the quotient is not guaranteed to be -2 on a core
without DIV32.  Likewise, a ldwu of 0xffffffff followed by w1 %= 16 gives
modwu a rj that is not a sign-extended value.

The old code widened both operands (zext for unsigned, sext for signed)
and then used the 64-bit divdu/divd/moddu/modd, which are well defined on
every LA64 core.  The MIPS64 JIT, which has the same ISA restriction,
still sign-extends the operands before its 32-bit div/mod.

[ ... ]

>  	/* dst = dst / imm */
>  	case BPF_ALU | BPF_DIV | BPF_K:
> +		move_imm(ctx, t1, (u32)imm);
> +		if (!off)
> +			emit_insn(ctx, divwu, dst, dst, t1);
> +		else
> +			emit_insn(ctx, divw, dst, dst, t1);
> +		break;

In the K forms the divisor is built with move_imm(ctx, t1, (u32)imm), so a
negative signed divisor, or an unsigned divisor of 0x80000000 or above,
also ends up in t1 as a zero-extended value rather than a sign-extended
one.

[ ... ]

>  	/* dst = dst % src */
>  	case BPF_ALU | BPF_MOD | BPF_X:
> +		if (!off)
> +			emit_insn(ctx, modwu, dst, dst, src);
> +		else
> +			emit_insn(ctx, modw, dst, dst, src);
> +		break;

[ ... ]

>  	/* dst = dst % imm */
>  	case BPF_ALU | BPF_MOD | BPF_K:
> +		move_imm(ctx, t1, (u32)imm);
> +		if (!off)
> +			emit_insn(ctx, modwu, dst, dst, t1);
> +		else
> +			emit_insn(ctx, modw, dst, dst, t1);
> +		break;

The same question applies to the modwu/modw uses in these two cases.

The verifier narrows 32-bit div/mod results, for example
scalar32_min_max_umod() bounds the result of w1 %= 16 to [0, 15].  If the
JITed code returns a different value on a core without DIV32, can the
program end up with a value outside the range the verifier proved, for
instance when it is used as a map value offset?

The commit message reports test_bpf, test_verifier and test_progs passing,
but does not say which CPU was used.  Would those results be reproducible
on a core without CPUCFG2_DIV32?

Would it be better to sign-extend both operands into temporaries (for
example addw t, reg, $zero) before divw/divwu/modw/modwu, to gate the
native 32-bit forms on CPUCFG2_DIV32, or to keep the previous 64-bit
sequence for these four opcodes?


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