On Thu, Sep 7, 2023 at 9:39 AM Puranjay Mohan <puranjay12@xxxxxxxxx> wrote:
>
> On Thu, Sep 07 2023, Alexei Starovoitov wrote:
>
> > On Thu, Sep 7, 2023 at 12:33 AM Puranjay Mohan <puranjay12@xxxxxxxxx> wrote:
> >>
> >> On Wed, Sep 06 2023, Alexei Starovoitov wrote:
> >>
> >> > On Fri, Sep 1, 2023 at 7:57 AM Puranjay Mohan <puranjay12@xxxxxxxxx> wrote:
> >> >>
> >> >> On Fri, Sep 01 2023, Puranjay Mohan wrote:
> >> >>
> >> >> > The problem here is that reg->subreg_def should be set as DEF_NOT_SUBREG for
> >> >> > registers that are used as destination registers of BPF_LDX |
> >> >> > BPF_MEMSX. I am seeing
> >> >> > the same problem on ARM32 and was going to send a patch today.
> >> >> >
> >> >> > The problem is that is_reg64() returns false for destination registers
> >> >> > of BPF_LDX | BPF_MEMSX.
> >> >> > But BPF_LDX | BPF_MEMSX always loads a 64 bit value because of the
> >> >> > sign extension so
> >> >> > is_reg64() should return true.
> >> >> >
> >> >> > I have written a patch that I will be sending as a reply to this.
> >> >> > Please let me know if that makes sense.
> >> >> >
> >> >>
> >> >> The check_reg_arg() function will mark reg->subreg_def = DEF_NOT_SUBREG for destination
> >> >> registers if is_reg64() returns true for these registers. My patch below make is_reg64()
> >> >> return true for destination registers of BPF_LDX with mod = BPF_MEMSX. I feel this is the
> >> >> correct way to fix this problem.
> >> >>
> >> >> Here is my patch:
> >> >>
> >> >> --- 8< ---
> >> >> From cf1bf5282183cf721926ab14d968d3d4097b89b8 Mon Sep 17 00:00:00 2001
> >> >> From: Puranjay Mohan <puranjay12@xxxxxxxxx>
> >> >> Date: Fri, 1 Sep 2023 11:18:59 +0000
> >> >> Subject: [PATCH bpf] bpf: verifier: mark destination of sign-extended load as
> >> >> 64 bit
> >> >>
> >> >> The verifier can emit instructions to zero-extend destination registers
> >> >> when the register is being used to keep 32 bit values. This behaviour is
> >> >> enabled only when the JIT sets bpf_jit_needs_zext() -> true. In the case
> >> >> of a sign extended load instruction, the destination register always has a
> >> >> 64-bit value, therefore the verifier should not emit zero-extend
> >> >> instructions for it.
> >> >>
> >> >> Change is_reg64() to return true if the register under consideration is a
> >> >> destination register of LDX instruction with mode = BPF_MEMSX.
> >> >>
> >> >> Fixes: 1f9a1ea821ff ("bpf: Support new sign-extension load insns")
> >> >> Signed-off-by: Puranjay Mohan <puranjay12@xxxxxxxxx>
> >> >> ---
> >> >> kernel/bpf/verifier.c | 2 +-
> >> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >> >>
> >> >> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> >> >> index bb78212fa5b2..93f84b868ccc 100644
> >> >> --- a/kernel/bpf/verifier.c
> >> >> +++ b/kernel/bpf/verifier.c
> >> >> @@ -3029,7 +3029,7 @@ static bool is_reg64(struct bpf_verifier_env *env, struct bpf_insn *insn,
> >> >>
> >> >> if (class == BPF_LDX) {
> >> >> if (t != SRC_OP)
> >> >> - return BPF_SIZE(code) == BPF_DW;
> >> >> + return (BPF_SIZE(code) == BPF_DW || BPF_MODE(code) == BPF_MEMSX);
> >> >
> >> > Looks like we have a bug here for normal LDX too.
> >> > This 'if' condition was inserting unnecessary zext for LDX.
> >> > It was harmless for LDX and broken for LDSX.
> >> > Both LDX and LDSX write all bits of 64-bit register.
> >> >
> >> > I think the proper fix is to remove above two lines.
> >> > wdyt?
> >>
> >> For LDX this returns true only if it is with a BPF_DW, for others it returns false.
> >> This means a zext is inserted for BPF_LDX | BPF_B/H/W.
> >>
> >> This is not a bug because LDX writes 64 bits of the register only with BPF_DW.
> >> With BPF_B/H/W It only writes the lower 32bits and needs zext for upper 32 bits.
> >
> > No. The interpreter writes all 64-bit for any LDX insn.
> > All JITs must do it as well.
> >
> >> On 32 bit architectures where a 64-bit BPF register is simulated with two 32-bit registers,
> >> explicit zext is required for BPF_LDX | BPF_B/H/W.
> >
> > zext JIT-aid done by the verifier has nothing to do with 32-bit architecture.
> > It's necessary on 64-bit as well when HW doesn't automatically zero out
> > upper 32-bit like it does on arm64 and x86-64
>
> Yes, I agree that zext JIT-aid is required for all 32-bit architectures and some 64-bit architectures
> that can't automatically zero out the upper 32-bits.
> Basically any architecture that sets bpf_jit_needs_zext() -> true.
>
> >> So, we should not remove this.
> >
> > I still think we should.
>
> If we remove this then some JITs will not zero extend the upper 32-bits for BPF_LDX | BPF_B/H/W.
>
> My understanding is that Verifier sets prog->aux->verifier_zext if it emits zext instructions. If the verifier
> doesn't emit zext for LDX but sets prog->aux->verifier_zext that would cause wrong behavior for some JITs:
>
> Example code from ARM32 jit doing BPF_LDX | BPF_MEM | BPF_B:
>
> case BPF_B:
> /* Load a Byte */
> emit(ARM_LDRB_I(rd[1], rm, off), ctx);
> if (!ctx->prog->aux->verifier_zext)
> emit_a32_mov_i(rd[0], 0, ctx);
> break;
>
> Here if ctx->prog->aux->verifier_zext is set by the verifier, and zext was not emitted for LDX, JIT will not zero
> the upper 32-bits.
>
> RISCV32, PowerPC32, x86-32 JITs have similar code paths. Only MIPS32 JIT zero-extends for LDX without checking
> prog->aux->verifier_zext.
>
> So, if we want to stop emitting zext for LDX then we would need to modify all these JITs to always zext for LDX.
I guess we never clearly defined what 'needs_zext' is supposed to be,
so it wouldn't be fair to call 32-bit JITs buggy.
But we better address this issue now.
This 32-bit zeroing after LDX hurts mips64, s390, ppc64, riscv64.
I believe all 4 JITs emit proper zero extension into 64-bit register
by using single cpu instruction,
but they also define bpf_jit_needs_zext() as true,
so extra BPF_ZEXT_REG() is added by the verifier
and it is a pure run-time overhead.
It's better to remove
if (t != SRC_OP)
return BPF_SIZE(code) == BPF_DW;
from is_reg64() to avoid adding BPF_ZEXT_REG() insn
and fix 32-bit JITs at the same time.
RISCV32, PowerPC32, x86-32 JITs fixed in the first 3 patches
to always zero upper 32-bit after LDX and
then 4th patch to remove these two lines.
> Let me know if my understanding has some gaps, also if we decide to remove it, I am happy to send patches for it
> and fix the JITs that need modifications.
Thank you for working on it!
cc-ing JIT experts.
