Re: [PATCH 4.14 1/4] bpf: Do not use ax register in interpreter on div/mod

On Mon, Aug 30, 2021 at 03:32:08PM -0300, Thadeu Lima de Souza Cascardo wrote:

From: Daniel Borkmann daniel@iogearbox.net

Partially undo old commit 144cd91c4c2b ("bpf: move tmp variable into ax register in interpreter"). The reason we need this here is because ax register will be used for holding temporary state for div/mod instruction which otherwise interpreter would corrupt. This will cause a small +8 byte stack increase for interpreter, but with the gain that we can use it from verifier rewrites as scratch register.

Signed-off-by: Daniel Borkmann daniel@iogearbox.net Reviewed-by: John Fastabend john.fastabend@gmail.com [cascardo: This partial revert is needed in order to support using AX for the following two commits, as there is no JMP32 on 4.19.y] Signed-off-by: Thadeu Lima de Souza Cascardo cascardo@canonical.com

---

kernel/bpf/core.c | 32 +++++++++++++++----------------- 1 file changed, 15 insertions(+), 17 deletions(-)

diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c index e7211b0fa27c..30d871be9974 100644 --- a/kernel/bpf/core.c +++ b/kernel/bpf/core.c @@ -616,9 +616,6 @@ static int bpf_jit_blind_insn(const struct bpf_insn *from, * below. * * Constant blinding is only used by JITs, not in the interpreter.

• * The interpreter uses AX in some occasions as a local temporary
  

• * register e.g. in DIV or MOD instructions.
  

• *
  

  • In restricted circumstances, the verifier can also use the AX
  • register for rewrites as long as they do not interfere with
  • the above cases!

@@ -951,6 +948,7 @@ static unsigned int ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn, u32 tail_call_cnt = 0; void *ptr; int off;

• u64 tmp;

#define CONT ({ insn++; goto select_insn; }) #define CONT_JMP ({ insn++; goto select_insn; }) @@ -1013,22 +1011,22 @@ static unsigned int ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn, ALU64_MOD_X: if (unlikely(SRC == 0)) return 0;

• div64_u64_rem(DST, SRC, &AX);
  

• DST = AX;
  

• div64_u64_rem(DST, SRC, &tmp);
  

• DST = tmp;
  

  CONT; ALU_MOD_X: if (unlikely((u32)SRC == 0)) return 0;

• AX = (u32) DST;
  

• DST = do_div(AX, (u32) SRC);
  

• tmp = (u32) DST;
  

• DST = do_div(tmp, (u32) SRC);
  

  CONT; ALU64_MOD_K:

• div64_u64_rem(DST, IMM, &AX);
  

• DST = AX;
  

• div64_u64_rem(DST, IMM, &tmp);
  

• DST = tmp;
  

  CONT; ALU_MOD_K:

• AX = (u32) DST;
  

• DST = do_div(AX, (u32) IMM);
  

• tmp = (u32) DST;
  

• DST = do_div(tmp, (u32) IMM);
  

  CONT; ALU64_DIV_X: if (unlikely(SRC == 0))

@@ -1038,17 +1036,17 @@ static unsigned int ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn, ALU_DIV_X: if (unlikely((u32)SRC == 0)) return 0;

• AX = (u32) DST;
  

• do_div(AX, (u32) SRC);
  

• DST = (u32) AX;
  

• tmp = (u32) DST;
  

• do_div(tmp, (u32) SRC);
  

• DST = (u32) tmp;
  

  CONT; ALU64_DIV_K: DST = div64_u64(DST, IMM); CONT; ALU_DIV_K:

• AX = (u32) DST;
  

• do_div(AX, (u32) IMM);
  

• DST = (u32) AX;
  

• tmp = (u32) DST;
  

• do_div(tmp, (u32) IMM);
  

• DST = (u32) tmp;
  

  CONT; ALU_END_TO_BE: switch (IMM) {

-- 2.30.2

Oops, no, this patch causes build errors:

kernel/bpf/core.c: In function ‘___bpf_prog_run’: kernel/bpf/core.c:951:13: error: redeclaration of ‘tmp’ with no linkage 951 | u64 tmp; | ^~~ kernel/bpf/core.c:839:13: note: previous declaration of ‘tmp’ with type ‘u64’ {aka ‘long long unsigned int’} 839 | u64 tmp; | ^~~ make[2]: *** [scripts/Makefile.build:329: kernel/bpf/core.o] Error 1

Please fix up and resend the whole series, as I will go drop these 3 patches from the 4.14.y queue now.

greg k-h