[PATCH bpf-next v1 1/2] bpf: removed unused 'env' parameter from is_reg64 and insn_has_def32

[PATCH bpf-next v1 1/2] bpf: removed unused 'env' parameter from is_reg64 and insn_has_def32

[Eduard Zingerman]

Parameter 'env' is not used by is_reg64() and insn_has_def32()
functions. Remove the parameter to make it clear that neither function
depends on 'env' state, e.g. env->insn_aux_data.

Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
---
 kernel/bpf/verifier.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 0806295945e4..69eb2b5c2218 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -3663,7 +3663,7 @@ static int mark_irq_flag_read(struct bpf_verifier_env *env, struct bpf_reg_state
  * code only. It returns TRUE if the source or destination register operates
  * on 64-bit, otherwise return FALSE.
  */
-static bool is_reg64(struct bpf_verifier_env *env, struct bpf_insn *insn,
+static bool is_reg64(struct bpf_insn *insn,
 		     u32 regno, struct bpf_reg_state *reg, enum reg_arg_type t)
 {
 	u8 code, class, op;
@@ -3774,14 +3774,14 @@ static int insn_def_regno(const struct bpf_insn *insn)
 }
 
 /* Return TRUE if INSN has defined any 32-bit value explicitly. */
-static bool insn_has_def32(struct bpf_verifier_env *env, struct bpf_insn *insn)
+static bool insn_has_def32(struct bpf_insn *insn)
 {
 	int dst_reg = insn_def_regno(insn);
 
 	if (dst_reg == -1)
 		return false;
 
-	return !is_reg64(env, insn, dst_reg, NULL, DST_OP);
+	return !is_reg64(insn, dst_reg, NULL, DST_OP);
 }
 
 static void mark_insn_zext(struct bpf_verifier_env *env,
@@ -3812,7 +3812,7 @@ static int __check_reg_arg(struct bpf_verifier_env *env, struct bpf_reg_state *r
 	mark_reg_scratched(env, regno);
 
 	reg = &regs[regno];
-	rw64 = is_reg64(env, insn, regno, reg, t);
+	rw64 = is_reg64(insn, regno, reg, t);
 	if (t == SRC_OP) {
 		/* check whether register used as source operand can be read */
 		if (reg->type == NOT_INIT) {
@@ -20712,7 +20712,7 @@ static void adjust_insn_aux_data(struct bpf_verifier_env *env,
 	 * (cnt == 1) is taken or not. There is no guarantee INSN at OFF is the
 	 * original insn at old prog.
 	 */
-	old_data[off].zext_dst = insn_has_def32(env, insn + off + cnt - 1);
+	old_data[off].zext_dst = insn_has_def32(insn + off + cnt - 1);
 
 	if (cnt == 1)
 		return;
@@ -20724,7 +20724,7 @@ static void adjust_insn_aux_data(struct bpf_verifier_env *env,
 	for (i = off; i < off + cnt - 1; i++) {
 		/* Expand insni[off]'s seen count to the patched range. */
 		new_data[i].seen = old_seen;
-		new_data[i].zext_dst = insn_has_def32(env, insn + i);
+		new_data[i].zext_dst = insn_has_def32(insn + i);
 	}
 	env->insn_aux_data = new_data;
 	vfree(old_data);
@@ -21131,7 +21131,7 @@ static int opt_subreg_zext_lo32_rnd_hi32(struct bpf_verifier_env *env,
 			 *       BPF_STX + SRC_OP, so it is safe to pass NULL
 			 *       here.
 			 */
-			if (is_reg64(env, &insn, load_reg, NULL, DST_OP)) {
+			if (is_reg64(&insn, load_reg, NULL, DST_OP)) {
 				if (class == BPF_LD &&
 				    BPF_MODE(code) == BPF_IMM)
 					i++;
-- 
2.47.3

[PATCH bpf-next v1 2/2] bpf: use realloc in bpf_patch_insn_data

[Eduard Zingerman]

Avoid excessive vzalloc/vfree calls when patching instructions in
do_misc_fixups(). bpf_patch_insn_data() uses vzalloc to allocate new
memory for env->insn_aux_data for each patch as follows:

  struct bpf_prog *bpf_patch_insn_data(env, ...)
  {
    ...
    new_data = vzalloc(... O(program size) ...);
    ...
    adjust_insn_aux_data(env, new_data, ...);
    ...
  }

  void adjust_insn_aux_data(env, new_data, ...)
  {
    ...
    memcpy(new_data, env->insn_aux_data);
    vfree(env->insn_aux_data);
    env->insn_aux_data = new_data;
    ...
  }

The vzalloc/vfree pair is hot in perf report collected for e.g.
pyperf180 test case. It can be replaced with a call to vrealloc in a
hope to reduce the number of actual memory allocations.

This is a stop-gap solution, as bpf_patch_insn_data is still hot in
the profile. More comprehansive solutions had been discussed before
e.g. as in [1].

Perf stat w/o this patch:

  $ perf stat -B --all-kernel -r10 -- ./veristat -q pyperf180.bpf.o
    ...
           2201.25 msec task-clock                       #    0.973 CPUs utilized               ( +-  2.20% )
               188      context-switches                 #   85.406 /sec                        ( +-  9.29% )
                15      cpu-migrations                   #    6.814 /sec                        ( +-  5.64% )
                 5      page-faults                      #    2.271 /sec                        ( +-  3.27% )
        4315057974      instructions                     #    1.28  insn per cycle
                                                  #    0.33  stalled cycles per insn     ( +-  0.03% )
        3366141387      cycles                           #    1.529 GHz                         ( +-  0.21% )
        1420810964      stalled-cycles-frontend          #   42.21% frontend cycles idle        ( +-  0.23% )
        1049956791      branches                         #  476.981 M/sec                       ( +-  0.03% )
          60591781      branch-misses                    #    5.77% of all branches             ( +-  0.07% )

            2.2632 +- 0.0527 seconds time elapsed  ( +-  2.33% )

Perf stat with this patch:

           1132.77 msec task-clock                       #    0.976 CPUs utilized               ( +-  3.47% )
                80      context-switches                 #   70.623 /sec                        ( +- 31.57% )
                 1      cpu-migrations                   #    0.883 /sec                        ( +- 37.27% )
                 5      page-faults                      #    4.414 /sec                        ( +-  3.59% )
        3307816503      instructions                     #    2.20  insn per cycle
                                                  #    0.15  stalled cycles per insn     ( +-  0.05% )
        1506171011      cycles                           #    1.330 GHz                         ( +-  0.55% )
         488914539      stalled-cycles-frontend          #   32.46% frontend cycles idle        ( +-  0.94% )
         729783557      branches                         #  644.246 M/sec                       ( +-  0.05% )
          17312298      branch-misses                    #    2.37% of all branches             ( +-  0.23% )

            1.1602 +- 0.0443 seconds time elapsed  ( +-  3.82% )

[1] https://lore.kernel.org/bpf/CAEf4BzY_E8MSL4mD0UPuuiDcbJhh9e2xQo2=5w+ppRWWiYSGvQ@mail.gmail.com/

Suggested-by: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
---
 kernel/bpf/verifier.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 69eb2b5c2218..6ef7dc6079a4 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -20699,12 +20699,11 @@ static void convert_pseudo_ld_imm64(struct bpf_verifier_env *env)
  * [0, off) and [off, end) to new locations, so the patched range stays zero
  */
 static void adjust_insn_aux_data(struct bpf_verifier_env *env,
-				 struct bpf_insn_aux_data *new_data,
 				 struct bpf_prog *new_prog, u32 off, u32 cnt)
 {
-	struct bpf_insn_aux_data *old_data = env->insn_aux_data;
+	struct bpf_insn_aux_data *data = env->insn_aux_data;
 	struct bpf_insn *insn = new_prog->insnsi;
-	u32 old_seen = old_data[off].seen;
+	u32 old_seen = data[off].seen;
 	u32 prog_len;
 	int i;
 
@@ -20712,22 +20711,19 @@ static void adjust_insn_aux_data(struct bpf_verifier_env *env,
 	 * (cnt == 1) is taken or not. There is no guarantee INSN at OFF is the
 	 * original insn at old prog.
 	 */
-	old_data[off].zext_dst = insn_has_def32(insn + off + cnt - 1);
+	data[off].zext_dst = insn_has_def32(insn + off + cnt - 1);
 
 	if (cnt == 1)
 		return;
 	prog_len = new_prog->len;
 
-	memcpy(new_data, old_data, sizeof(struct bpf_insn_aux_data) * off);
-	memcpy(new_data + off + cnt - 1, old_data + off,
-	       sizeof(struct bpf_insn_aux_data) * (prog_len - off - cnt + 1));
+	memmove(data + off + cnt - 1, data + off,
+		sizeof(struct bpf_insn_aux_data) * (prog_len - off - cnt + 1));
 	for (i = off; i < off + cnt - 1; i++) {
 		/* Expand insni[off]'s seen count to the patched range. */
-		new_data[i].seen = old_seen;
-		new_data[i].zext_dst = insn_has_def32(insn + i);
+		data[i].seen = old_seen;
+		data[i].zext_dst = insn_has_def32(insn + i);
 	}
-	env->insn_aux_data = new_data;
-	vfree(old_data);
 }
 
 static void adjust_subprog_starts(struct bpf_verifier_env *env, u32 off, u32 len)
@@ -20765,10 +20761,14 @@ static struct bpf_prog *bpf_patch_insn_data(struct bpf_verifier_env *env, u32 of
 	struct bpf_insn_aux_data *new_data = NULL;
 
 	if (len > 1) {
-		new_data = vzalloc(array_size(env->prog->len + len - 1,
-					      sizeof(struct bpf_insn_aux_data)));
+		new_data = vrealloc(env->insn_aux_data,
+				    array_size(env->prog->len + len - 1,
+					       sizeof(struct bpf_insn_aux_data)),
+				    GFP_KERNEL_ACCOUNT | __GFP_ZERO);
 		if (!new_data)
 			return NULL;
+
+		env->insn_aux_data = new_data;
 	}
 
 	new_prog = bpf_patch_insn_single(env->prog, off, patch, len);
@@ -20780,7 +20780,7 @@ static struct bpf_prog *bpf_patch_insn_data(struct bpf_verifier_env *env, u32 of
 		vfree(new_data);
 		return NULL;
 	}
-	adjust_insn_aux_data(env, new_data, new_prog, off, len);
+	adjust_insn_aux_data(env, new_prog, off, len);
 	adjust_subprog_starts(env, off, len);
 	adjust_poke_descs(new_prog, off, len);
 	return new_prog;
-- 
2.47.3

Re: [PATCH bpf-next v1 2/2] bpf: use realloc in bpf_patch_insn_data

[Eduard Zingerman]

On Wed, 2025-08-06 at 13:09 -0700, Eduard Zingerman wrote:

[...]

> @@ -20712,22 +20711,19 @@ static void adjust_insn_aux_data(struct bpf_verifier_env *env,
>  	 * (cnt == 1) is taken or not. There is no guarantee INSN at OFF is the
>  	 * original insn at old prog.
>  	 */
> -	old_data[off].zext_dst = insn_has_def32(insn + off + cnt - 1);
> +	data[off].zext_dst = insn_has_def32(insn + off + cnt - 1);
>  
>  	if (cnt == 1)
>  		return;
>  	prog_len = new_prog->len;
>  
> -	memcpy(new_data, old_data, sizeof(struct bpf_insn_aux_data) * off);
> -	memcpy(new_data + off + cnt - 1, old_data + off,
> -	       sizeof(struct bpf_insn_aux_data) * (prog_len - off - cnt + 1));
> +	memmove(data + off + cnt - 1, data + off,
> +		sizeof(struct bpf_insn_aux_data) * (prog_len - off - cnt + 1));
>  	for (i = off; i < off + cnt - 1; i++) {
>  		/* Expand insni[off]'s seen count to the patched range. */
> -		new_data[i].seen = old_seen;
> -		new_data[i].zext_dst = insn_has_def32(insn + i);
> +		data[i].seen = old_seen;
> +		data[i].zext_dst = insn_has_def32(insn + i);
>  	}
> -	env->insn_aux_data = new_data;
> -	vfree(old_data);
>  }

veristat-meta job failed on the CI [1] because the following piece is missing:

  @@ -20719,6 +20719,7 @@ static void adjust_insn_aux_data(struct bpf_verifier_env *env,
   
          memmove(data + off + cnt - 1, data + off,
                  sizeof(struct bpf_insn_aux_data) * (prog_len - off - cnt + 1));
  +       memset(data + off, 0, sizeof(struct bpf_insn_aux_data) * (cnt - 1));
          for (i = off; i < off + cnt - 1; i++) {
                  /* Expand insni[off]'s seen count to the patched range. */
                  data[i].seen = old_seen;

I'm trying to figure out if I can add a selftest for this.

[1] https://github.com/kernel-patches/bpf/actions/runs/16787563163/job/47542309875

[...]

Re: [PATCH bpf-next v1 2/2] bpf: use realloc in bpf_patch_insn_data

[Eduard Zingerman]

On Wed, 2025-08-06 at 16:04 -0700, Eduard Zingerman wrote:
> On Wed, 2025-08-06 at 13:09 -0700, Eduard Zingerman wrote:
>
> [...]
>
> > @@ -20712,22 +20711,19 @@ static void adjust_insn_aux_data(struct bpf_verifier_env *env,
> >  	 * (cnt == 1) is taken or not. There is no guarantee INSN at OFF is the
> >  	 * original insn at old prog.
> >  	 */
> > -	old_data[off].zext_dst = insn_has_def32(insn + off + cnt - 1);
> > +	data[off].zext_dst = insn_has_def32(insn + off + cnt - 1);
> >
> >  	if (cnt == 1)
> >  		return;
> >  	prog_len = new_prog->len;
> >
> > -	memcpy(new_data, old_data, sizeof(struct bpf_insn_aux_data) * off);
> > -	memcpy(new_data + off + cnt - 1, old_data + off,
> > -	       sizeof(struct bpf_insn_aux_data) * (prog_len - off - cnt + 1));
> > +	memmove(data + off + cnt - 1, data + off,
> > +		sizeof(struct bpf_insn_aux_data) * (prog_len - off - cnt + 1));
> >  	for (i = off; i < off + cnt - 1; i++) {
> >  		/* Expand insni[off]'s seen count to the patched range. */
> > -		new_data[i].seen = old_seen;
> > -		new_data[i].zext_dst = insn_has_def32(insn + i);
> > +		data[i].seen = old_seen;
> > +		data[i].zext_dst = insn_has_def32(insn + i);
> >  	}
> > -	env->insn_aux_data = new_data;
> > -	vfree(old_data);
> >  }
>
> veristat-meta job failed on the CI [1] because the following piece is missing:
>
>   @@ -20719,6 +20719,7 @@ static void adjust_insn_aux_data(struct bpf_verifier_env *env,
>
>           memmove(data + off + cnt - 1, data + off,
>                   sizeof(struct bpf_insn_aux_data) * (prog_len - off - cnt + 1));
>   +       memset(data + off, 0, sizeof(struct bpf_insn_aux_data) * (cnt - 1));
>           for (i = off; i < off + cnt - 1; i++) {
>                   /* Expand insni[off]'s seen count to the patched range. */
>                   data[i].seen = old_seen;
>
> I'm trying to figure out if I can add a selftest for this.
>
> [1] https://github.com/kernel-patches/bpf/actions/runs/16787563163/job/47542309875
>
> [...]

The error reported by verifier is "verifier bug: error during ctx access conversion (0)",
signaled from convert_ctx_accesses(). The rewrite is attempted because
`env->insn_aux_data[i + delta].ptr_type` is set to 12 (PTR_TO_SOCK_COMMON).
The instruction for which rewrite is attempted is a load or store
instruction introduced as a result of inline_bpf_loop() call.
It has a wrong offset for bpf_sock_convert_ctx_access() rewrite,
hence rewrite attempt is unsuccessful and the above mentioned error is reported.
`env->insn_aux_data[i + delta].ptr_type` is set for the instruction in question
because of missing memset(0). It is a value of the insn_aux_data inherited
from an instruction occurring at a small offset after bpf_loop call.

Here is a similar reproducer, but for PTR_TO_CTX (== 2):

  struct { ... } map0 SEC(".maps"); // any valid map definition
  struct { ... } map1 SEC(".maps");
  struct { ... } map2 SEC(".maps");
  
  SEC("xdp")
  __success
  __naked void bug1(void)
  {
          asm volatile ("                                 \
          r0 = %[map0] ll;       /* 0 */                  \
          r0 = %[map1] ll;       /* 2 */                  \
          r1 = 1;                /* 4 */                  \
          r2 = dummy ll;         /* 5 */                  \
          r3 = 0;                /* 7 */                  \
          r4 = 0;                /* 8 */                  \
          call %[bpf_loop];      /* 9 */                  \
          r0 = 0;                /* 10 */                 \
          r0 = 0;                /* 11 */                 \
          r0 = %[map2] ll;       /* 12 */                 \
          exit;                                           \
  "       :
          : __imm(bpf_loop),
            __imm_addr(map0),
            __imm_addr(map1),
            __imm_addr(map2)
          : __clobber_all);
  }

Instruction `call %[bpf_loop]` is replaced by a sequence:

  9:  if r1 <= 0x800000 goto pc+2
  10: w0 = -7
  11: goto pc+16
  12: *(u64 *)(r10 -24) = r6
  ...

Note the store at offset (12). Because of the missing memset(0) it
inherits insn_aux_data fields from original instruction #12: `r0 = %[map2] ll`.
`struct bpf_insn_aux_data` is defined as follows:

   struct bpf_insn_aux_data {
         union {
                 enum bpf_reg_type ptr_type;
				 ...
                 struct {
                         u32 map_index;          /* index into used_maps[] */
                         ...
                 };
				 ...
         };
   }

Here fields .ptr_type and .map_index have same offset.
The example above forces convert_ctx_accesses() to interpret .map_index as a .ptr_type.
The .map_index at offset 12 happens to be 2, which corresponds to PTR_TO_CTX.
convert_ctx_accesses() attempts to rewrite `12: *(u64 *)(r10 -24) = r6` and fails.

All in all, I think this test is fragile, so I'll post v2 w/o a selftest.