Re: [PATCH bpf-next 1/2] bpf: Reduce stack frame size by using env->insn_buf for bpf insns

[Yonghong Song <yonghong.song@xxxxxxxxx>]

On 7/2/25 8:09 AM, Alexei Starovoitov wrote:
> On Tue, Jul 1, 2025 at 10:33 PM Yonghong Song <yonghong.song@xxxxxxxxx> wrote:
> > Arnd Bergmann reported an issue ([1]) where clang compiler (less than
> > llvm18) may trigger an error where the stack frame size exceeds the limit.
> > I can reproduce the error like below:
> >    kernel/bpf/verifier.c:24491:5: error: stack frame size (2552) exceeds limit (1280) in 'bpf_check'
> >        [-Werror,-Wframe-larger-than]
> >    kernel/bpf/verifier.c:19921:12: error: stack frame size (1368) exceeds limit (1280) in 'do_check'
> >        [-Werror,-Wframe-larger-than]
> >
> > Use env->insn_buf for bpf insns instead of putting these insns on the
> > stack. This can resolve the above 'bpf_check' error. The 'do_check' error
> > will be resolved in the next patch.
> >
> >    [1] https://lore.kernel.org/bpf/20250620113846.3950478-1-arnd@xxxxxxxxxx/
> >
> > Reported-by: Arnd Bergmann <arnd@xxxxxxxxxx>
> > Signed-off-by: Yonghong Song <yonghong.song@xxxxxxxxx>
> > ---
> >   kernel/bpf/verifier.c | 194 ++++++++++++++++++++----------------------
> >   1 file changed, 91 insertions(+), 103 deletions(-)
> >
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 90e688f81a48..29faef51065d 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -20939,26 +20939,27 @@ static bool insn_is_cond_jump(u8 code)
> >   static void opt_hard_wire_dead_code_branches(struct bpf_verifier_env *env)
> >   {
> >          struct bpf_insn_aux_data *aux_data = env->insn_aux_data;
> > -       struct bpf_insn ja = BPF_JMP_IMM(BPF_JA, 0, 0, 0);
> > +       struct bpf_insn *ja = env->insn_buf;
> This one replaces 8 bytes on stack with an 8 byte pointer.
> Does it actually make a difference in stack usage?
> I have my doubts.

Probably no difference. I just made change here just to remove
*all* 'struct bpf_insn var/var[]' on the stack. But as you suggested,
I will remove this change.

> >          struct bpf_insn *insn = env->prog->insnsi;
> >          const int insn_cnt = env->prog->len;
> >          int i;
> >
> > +       *ja = BPF_JMP_IMM(BPF_JA, 0, 0, 0);
> >          for (i = 0; i < insn_cnt; i++, insn++) {
> >                  if (!insn_is_cond_jump(insn->code))
> >                          continue;
> >
> >                  if (!aux_data[i + 1].seen)
> > -                       ja.off = insn->off;
> > +                       ja->off = insn->off;
> >                  else if (!aux_data[i + 1 + insn->off].seen)
> > -                       ja.off = 0;
> > +                       ja->off = 0;
> >                  else
> >                          continue;
> >
> >                  if (bpf_prog_is_offloaded(env->prog->aux))
> > -                       bpf_prog_offload_replace_insn(env, i, &ja);
> > +                       bpf_prog_offload_replace_insn(env, i, ja);
> >
> > -               memcpy(insn, &ja, sizeof(ja));
> > +               memcpy(insn, ja, sizeof(*ja));
> >          }
> >   }
> >
> > @@ -21017,7 +21018,9 @@ static int opt_remove_nops(struct bpf_verifier_env *env)
> >   static int opt_subreg_zext_lo32_rnd_hi32(struct bpf_verifier_env *env,
> >                                           const union bpf_attr *attr)
> >   {
> > -       struct bpf_insn *patch, zext_patch[2], rnd_hi32_patch[4];
> > +       struct bpf_insn *patch;
> > +       struct bpf_insn *zext_patch = env->insn_buf;
> > +       struct bpf_insn *rnd_hi32_patch = &env->insn_buf[2];
> >          struct bpf_insn_aux_data *aux = env->insn_aux_data;
> >          int i, patch_len, delta = 0, len = env->prog->len;
> >          struct bpf_insn *insns = env->prog->insnsi;
> > @@ -21195,13 +21198,12 @@ static int convert_ctx_accesses(struct bpf_verifier_env *env)
> >
> >                  if (env->insn_aux_data[i + delta].nospec) {
> >                          WARN_ON_ONCE(env->insn_aux_data[i + delta].alu_state);
> > -                       struct bpf_insn patch[] = {
> > -                               BPF_ST_NOSPEC(),
> > -                               *insn,
> > -                       };
> > +                       struct bpf_insn *patch = &insn_buf[0];
> why &..[0] ? Can it just be patch = insn_buf ?

There are a couple of cases in the existing code, e.g.,

$ grep -C 2 "&insn_buf\[0\]" verifier.c
                    (BPF_MODE(insn->code) == BPF_PROBE_MEM ||
                     BPF_MODE(insn->code) == BPF_PROBE_MEMSX)) {
                        struct bpf_insn *patch = &insn_buf[0];
                        u64 uaddress_limit = bpf_arch_uaddress_limit();

--
                        const u8 code_add = BPF_ALU64 | BPF_ADD | BPF_X;
                        const u8 code_sub = BPF_ALU64 | BPF_SUB | BPF_X;
                        struct bpf_insn *patch = &insn_buf[0];
                        bool issrc, isneg, isimm;
                        u32 off_reg;

I was using '... = &insn_buf[0]' to make code consistent.

Will change to use '... = insn_buf' including the above two cases.

> > -                       cnt = ARRAY_SIZE(patch);
> > -                       new_prog = bpf_patch_insn_data(env, i + delta, patch, cnt);
> > +                       *patch++ = BPF_ST_NOSPEC();
> > +                       *patch++ = *insn;
> > +                       cnt = patch - insn_buf;
> > +                       new_prog = bpf_patch_insn_data(env, i + delta, insn_buf, cnt);
> >                          if (!new_prog)
> >                                  return -ENOMEM;
> >
> > @@ -21269,13 +21271,12 @@ static int convert_ctx_accesses(struct bpf_verifier_env *env)
> >                          /* nospec_result is only used to mitigate Spectre v4 and
> >                           * to limit verification-time for Spectre v1.
> >                           */
> > -                       struct bpf_insn patch[] = {
> > -                               *insn,
> > -                               BPF_ST_NOSPEC(),
> > -                       };
> > +                       struct bpf_insn *patch = &insn_buf[0];
> >
> > -                       cnt = ARRAY_SIZE(patch);
> > -                       new_prog = bpf_patch_insn_data(env, i + delta, patch, cnt);
> > +                       *patch++ = *insn;
> > +                       *patch++ = BPF_ST_NOSPEC();
> > +                       cnt = patch - insn_buf;
> > +                       new_prog = bpf_patch_insn_data(env, i + delta, insn_buf, cnt);
> >                          if (!new_prog)
> >                                  return -ENOMEM;
> >
> > @@ -21945,13 +21946,12 @@ static int do_misc_fixups(struct bpf_verifier_env *env)
> >          u16 stack_depth_extra = 0;
> >
> >          if (env->seen_exception && !env->exception_callback_subprog) {
> > -               struct bpf_insn patch[] = {
> > -                       env->prog->insnsi[insn_cnt - 1],
> > -                       BPF_MOV64_REG(BPF_REG_0, BPF_REG_1),
> > -                       BPF_EXIT_INSN(),
> > -               };
> > +               struct bpf_insn *patch = &insn_buf[0];
> >
> > -               ret = add_hidden_subprog(env, patch, ARRAY_SIZE(patch));
> > +               *patch++ = env->prog->insnsi[insn_cnt - 1];
> > +               *patch++ = BPF_MOV64_REG(BPF_REG_0, BPF_REG_1);
> > +               *patch++ = BPF_EXIT_INSN();
> > +               ret = add_hidden_subprog(env, insn_buf, patch - insn_buf);
> >                  if (ret < 0)
> >                          return ret;
> >                  prog = env->prog;
> > @@ -21987,20 +21987,18 @@ static int do_misc_fixups(struct bpf_verifier_env *env)
> >                      insn->off == 1 && insn->imm == -1) {
> >                          bool is64 = BPF_CLASS(insn->code) == BPF_ALU64;
> >                          bool isdiv = BPF_OP(insn->code) == BPF_DIV;
> > -                       struct bpf_insn *patchlet;
> > -                       struct bpf_insn chk_and_sdiv[] = {
> > -                               BPF_RAW_INSN((is64 ? BPF_ALU64 : BPF_ALU) |
> > -                                            BPF_NEG | BPF_K, insn->dst_reg,
> > -                                            0, 0, 0),
> > -                       };
> > -                       struct bpf_insn chk_and_smod[] = {
> > -                               BPF_MOV32_IMM(insn->dst_reg, 0),
> > -                       };
> > +                       struct bpf_insn *patch = &insn_buf[0];
> > +
> > +                       if (isdiv)
> > +                               *patch++ = BPF_RAW_INSN((is64 ? BPF_ALU64 : BPF_ALU) |
> > +                                                       BPF_NEG | BPF_K, insn->dst_reg,
> > +                                                       0, 0, 0);
> > +                       else
> > +                               *patch++ = BPF_MOV32_IMM(insn->dst_reg, 0);
> >
> > -                       patchlet = isdiv ? chk_and_sdiv : chk_and_smod;
> > -                       cnt = isdiv ? ARRAY_SIZE(chk_and_sdiv) : ARRAY_SIZE(chk_and_smod);
> > +                       cnt = patch - insn_buf;
> >
> > -                       new_prog = bpf_patch_insn_data(env, i + delta, patchlet, cnt);
> > +                       new_prog = bpf_patch_insn_data(env, i + delta, insn_buf, cnt);
> >                          if (!new_prog)
> >                                  return -ENOMEM;
> >
> > @@ -22019,83 +22017,73 @@ static int do_misc_fixups(struct bpf_verifier_env *env)
> >                          bool isdiv = BPF_OP(insn->code) == BPF_DIV;
> >                          bool is_sdiv = isdiv && insn->off == 1;
> >                          bool is_smod = !isdiv && insn->off == 1;
> > -                       struct bpf_insn *patchlet;
> > -                       struct bpf_insn chk_and_div[] = {
> > -                               /* [R,W]x div 0 -> 0 */
> > -                               BPF_RAW_INSN((is64 ? BPF_JMP : BPF_JMP32) |
> > -                                            BPF_JNE | BPF_K, insn->src_reg,
> > -                                            0, 2, 0),
> > -                               BPF_ALU32_REG(BPF_XOR, insn->dst_reg, insn->dst_reg),
> > -                               BPF_JMP_IMM(BPF_JA, 0, 0, 1),
> > -                               *insn,
> > -                       };
> > -                       struct bpf_insn chk_and_mod[] = {
> > -                               /* [R,W]x mod 0 -> [R,W]x */
> > -                               BPF_RAW_INSN((is64 ? BPF_JMP : BPF_JMP32) |
> > -                                            BPF_JEQ | BPF_K, insn->src_reg,
> > -                                            0, 1 + (is64 ? 0 : 1), 0),
> > -                               *insn,
> > -                               BPF_JMP_IMM(BPF_JA, 0, 0, 1),
> > -                               BPF_MOV32_REG(insn->dst_reg, insn->dst_reg),
> > -                       };
> > -                       struct bpf_insn chk_and_sdiv[] = {
> > +                       struct bpf_insn *patch = &insn_buf[0];
> > +
> > +                       if (is_sdiv) {
> >                                  /* [R,W]x sdiv 0 -> 0
> >                                   * LLONG_MIN sdiv -1 -> LLONG_MIN
> >                                   * INT_MIN sdiv -1 -> INT_MIN
> >                                   */
> > -                               BPF_MOV64_REG(BPF_REG_AX, insn->src_reg),
> > -                               BPF_RAW_INSN((is64 ? BPF_ALU64 : BPF_ALU) |
> > -                                            BPF_ADD | BPF_K, BPF_REG_AX,
> > -                                            0, 0, 1),
> > -                               BPF_RAW_INSN((is64 ? BPF_JMP : BPF_JMP32) |
> > -                                            BPF_JGT | BPF_K, BPF_REG_AX,
> > -                                            0, 4, 1),
> > -                               BPF_RAW_INSN((is64 ? BPF_JMP : BPF_JMP32) |
> > -                                            BPF_JEQ | BPF_K, BPF_REG_AX,
> > -                                            0, 1, 0),
> > -                               BPF_RAW_INSN((is64 ? BPF_ALU64 : BPF_ALU) |
> > -                                            BPF_MOV | BPF_K, insn->dst_reg,
> > -                                            0, 0, 0),
> > +                               *patch++ = BPF_MOV64_REG(BPF_REG_AX, insn->src_reg);
> > +                               *patch++ = BPF_RAW_INSN((is64 ? BPF_ALU64 : BPF_ALU) |
> > +                                                       BPF_ADD | BPF_K, BPF_REG_AX,
> > +                                                       0, 0, 1);
> > +                               *patch++ = BPF_RAW_INSN((is64 ? BPF_JMP : BPF_JMP32) |
> > +                                                       BPF_JGT | BPF_K, BPF_REG_AX,
> > +                                                       0, 4, 1);
> > +                               *patch++ = BPF_RAW_INSN((is64 ? BPF_JMP : BPF_JMP32) |
> > +                                                       BPF_JEQ | BPF_K, BPF_REG_AX,
> > +                                                       0, 1, 0);
> > +                               *patch++ = BPF_RAW_INSN((is64 ? BPF_ALU64 : BPF_ALU) |
> > +                                                       BPF_MOV | BPF_K, insn->dst_reg,
> > +                                                       0, 0, 0);
> >                                  /* BPF_NEG(LLONG_MIN) == -LLONG_MIN == LLONG_MIN */
> > -                               BPF_RAW_INSN((is64 ? BPF_ALU64 : BPF_ALU) |
> > -                                            BPF_NEG | BPF_K, insn->dst_reg,
> > -                                            0, 0, 0),
> > -                               BPF_JMP_IMM(BPF_JA, 0, 0, 1),
> > -                               *insn,
> > -                       };
> > -                       struct bpf_insn chk_and_smod[] = {
> > +                               *patch++ = BPF_RAW_INSN((is64 ? BPF_ALU64 : BPF_ALU) |
> > +                                                       BPF_NEG | BPF_K, insn->dst_reg,
> > +                                                       0, 0, 0);
> > +                               *patch++ = BPF_JMP_IMM(BPF_JA, 0, 0, 1);
> > +                               *patch++ = *insn;
> > +                               cnt = patch - insn_buf;
> > +                       } else if (is_smod) {
> >                                  /* [R,W]x mod 0 -> [R,W]x */
> >                                  /* [R,W]x mod -1 -> 0 */
> > -                               BPF_MOV64_REG(BPF_REG_AX, insn->src_reg),
> > -                               BPF_RAW_INSN((is64 ? BPF_ALU64 : BPF_ALU) |
> > -                                            BPF_ADD | BPF_K, BPF_REG_AX,
> > -                                            0, 0, 1),
> > -                               BPF_RAW_INSN((is64 ? BPF_JMP : BPF_JMP32) |
> > -                                            BPF_JGT | BPF_K, BPF_REG_AX,
> > -                                            0, 3, 1),
> > -                               BPF_RAW_INSN((is64 ? BPF_JMP : BPF_JMP32) |
> > -                                            BPF_JEQ | BPF_K, BPF_REG_AX,
> > -                                            0, 3 + (is64 ? 0 : 1), 1),
> > -                               BPF_MOV32_IMM(insn->dst_reg, 0),
> > -                               BPF_JMP_IMM(BPF_JA, 0, 0, 1),
> > -                               *insn,
> > -                               BPF_JMP_IMM(BPF_JA, 0, 0, 1),
> > -                               BPF_MOV32_REG(insn->dst_reg, insn->dst_reg),
> > -                       };
> > -
> > -                       if (is_sdiv) {
> > -                               patchlet = chk_and_sdiv;
> > -                               cnt = ARRAY_SIZE(chk_and_sdiv);
> > -                       } else if (is_smod) {
> > -                               patchlet = chk_and_smod;
> > -                               cnt = ARRAY_SIZE(chk_and_smod) - (is64 ? 2 : 0);
> > +                               *patch++ = BPF_MOV64_REG(BPF_REG_AX, insn->src_reg);
> > +                               *patch++ = BPF_RAW_INSN((is64 ? BPF_ALU64 : BPF_ALU) |
> > +                                                       BPF_ADD | BPF_K, BPF_REG_AX,
> > +                                                       0, 0, 1);
> > +                               *patch++ = BPF_RAW_INSN((is64 ? BPF_JMP : BPF_JMP32) |
> > +                                                       BPF_JGT | BPF_K, BPF_REG_AX,
> > +                                                       0, 3, 1);
> > +                               *patch++ = BPF_RAW_INSN((is64 ? BPF_JMP : BPF_JMP32) |
> > +                                                       BPF_JEQ | BPF_K, BPF_REG_AX,
> > +                                                       0, 3 + (is64 ? 0 : 1), 1);
> > +                               *patch++ = BPF_MOV32_IMM(insn->dst_reg, 0);
> > +                               *patch++ = BPF_JMP_IMM(BPF_JA, 0, 0, 1);
> > +                               *patch++ = *insn;
> > +                               *patch++ = BPF_JMP_IMM(BPF_JA, 0, 0, 1);
> > +                               *patch++ = BPF_MOV32_REG(insn->dst_reg, insn->dst_reg);
> > +                               cnt = (patch - insn_buf) - (is64 ? 2 : 0);
> > +                       } else if (isdiv) {
> > +                               /* [R,W]x div 0 -> 0 */
> > +                               *patch++ = BPF_RAW_INSN((is64 ? BPF_JMP : BPF_JMP32) |
> > +                                                       BPF_JNE | BPF_K, insn->src_reg,
> > +                                                       0, 2, 0);
> > +                               *patch++ = BPF_ALU32_REG(BPF_XOR, insn->dst_reg, insn->dst_reg);
> > +                               *patch++ = BPF_JMP_IMM(BPF_JA, 0, 0, 1);
> > +                               *patch++ = *insn;
> > +                               cnt = patch - insn_buf;
> >                          } else {
> > -                               patchlet = isdiv ? chk_and_div : chk_and_mod;
> > -                               cnt = isdiv ? ARRAY_SIZE(chk_and_div) :
> > -                                             ARRAY_SIZE(chk_and_mod) - (is64 ? 2 : 0);
> > +                               /* [R,W]x mod 0 -> [R,W]x */
> > +                               *patch++ = BPF_RAW_INSN((is64 ? BPF_JMP : BPF_JMP32) |
> > +                                                       BPF_JEQ | BPF_K, insn->src_reg,
> > +                                                       0, 1 + (is64 ? 0 : 1), 0);
> > +                               *patch++ = *insn;
> > +                               *patch++ = BPF_JMP_IMM(BPF_JA, 0, 0, 1);
> > +                               *patch++ = BPF_MOV32_REG(insn->dst_reg, insn->dst_reg);
> > +                               cnt = (patch - insn_buf) - (is64 ? 2 : 0);
> hmm. why populate two extra insn just to drop them ?
> Don't add the last two insn instead.
> I would also simplify the previous jmp32 vs jmp generation by
> testing if (is64) once and at the end cnt = patch - insn_buf;

Make sense. Will do.

> --
> pw-bot: cr