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

[Eduard Zingerman <eddyz87@gmail.com>]

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.