Re: [PATCH bpf-next v2 07/22] bpf: Let the patch list remove instructions

[Alexei Starovoitov]

On Fri, Oct 30, 2026 at 04:51 AM Qiliang Yuan <odys.yuan@xxxxxxxxx> wrote:
>  	bpf_patch_image_install(prog, &img);
[...]
> +	/* before subprogs are removed, since it looks at them */
>  	for (i = 0; i < env->func_ptr_cnt; i++) {
>  		struct bpf_func_ptr *fp = &env->func_ptrs[i];
> +		int subprog;
>
>  		if (fp->xlated_off == BPF_FUNC_PTR_DELETED)
>  			continue;
> +		if (bpf_patch_image_deleted(&img, fp->xlated_off)) {
> +			subprog = bpf_find_subprog(env, fp->xlated_off);
> +			if (subprog <= 0 ||

Sashiko is right. It's real, and it crashes the kernel.

bpf_patch_image_install() already set prog->len to the new length.
bpf_find_subprog() gets an old offset and compares it with prog->len.
When the prog shrinks enough it doesn't find a live function.
The func ptr is marked as deleted and the slot in .rodata becomes NULL.

It needs two things:
- the first insn of the function goes away as a nop
- this commit removes at least (end of prog - start of function) insns

Plain C hits it:

  #include <linux/bpf.h>
  #include <bpf/bpf_helpers.h>

  struct st { int a; } g = { .a = 10 };

  #define OP(name, expr)					\
  static __noinline int name(struct st *s)		\
  {							\
  	if (!s)						\
  		return bpf_get_prandom_u32();		\
  	return expr;					\
  }
  OP(op_a, s->a + 2) OP(op_b, s->a + 3) OP(op_c, s->a + 4) OP(op_d, s->a + 5)
  OP(op_e, s->a + 6) OP(op_f, s->a + 7) OP(op_g, s->a + 8) OP(op_h, s->a + 9)

  static int (* const volatile ops[])(struct st *) = {
  	op_a, op_b, op_c, op_d, op_e, op_f, op_g, op_h
  };

  SEC("socket")
  int vt_all(struct __sk_buff *skb)
  {
  	int i, sum = 0;

  	for (i = 0; i < 8; i++)
  		sum += ops[i](&g);
  	return sum;
  }
  char _license[] SEC("license") = "GPL";

The verifier knows that &g is not NULL, so every op starts with a nop.

  bpf-next:     Return value: 124
  this series:  BUG: kernel NULL pointer dereference, address: 0000000000000000
                RIP: 0010:0x0

That's the shape of any table of ops.

The minimal fix is to do the lookup before the install.
On top of the whole series:

diff --git a/kernel/bpf/fixups.c b/kernel/bpf/fixups.c
index a442188647bc..de758ce7afac 100644
--- a/kernel/bpf/fixups.c
+++ b/kernel/bpf/fixups.c
@@ -451,6 +451,28 @@ int bpf_patch_list_commit(struct bpf_verifier_env *env)
 		}
 	}
 
+	/*
+	 * Before the new image is installed and subprogs are removed:
+	 * bpf_find_subprog() looks at prog->len and at the subprogs.
+	 */
+	for (i = 0; i < env->func_ptr_cnt; i++) {
+		struct bpf_func_ptr *fp = &env->func_ptrs[i];
+		int subprog;
+
+		if (fp->xlated_off == BPF_FUNC_PTR_DELETED)
+			continue;
+		if (bpf_patch_image_deleted(&img, fp->xlated_off)) {
+			subprog = bpf_find_subprog(env, fp->xlated_off);
+			if (subprog <= 0 ||
+			    img.new_off[env->subprog_info[subprog].start] ==
+			    img.new_off[env->subprog_info[subprog + 1].start]) {
+				fp->xlated_off = BPF_FUNC_PTR_DELETED;
+				continue;
+			}
+		}
+		fp->xlated_off = bpf_patch_image_remap(&img, img.new_off, fp->xlated_off);
+	}
+
 	bpf_patch_image_install(prog, &img);
 
 	for (i = 0, k = 0; i < img.cnt; i = end) {
@@ -474,25 +496,6 @@ int bpf_patch_list_commit(struct bpf_verifier_env *env)
 	env->insn_aux_data_len = img.new_cnt;
 	data = NULL;
 
-	/* before subprogs are removed, since it looks at them */
-	for (i = 0; i < env->func_ptr_cnt; i++) {
-		struct bpf_func_ptr *fp = &env->func_ptrs[i];
-		int subprog;
-
-		if (fp->xlated_off == BPF_FUNC_PTR_DELETED)
-			continue;
-		if (bpf_patch_image_deleted(&img, fp->xlated_off)) {
-			subprog = bpf_find_subprog(env, fp->xlated_off);
-			if (subprog <= 0 ||
-			    img.new_off[env->subprog_info[subprog].start] ==
-			    img.new_off[env->subprog_info[subprog + 1].start]) {
-				fp->xlated_off = BPF_FUNC_PTR_DELETED;
-				continue;
-			}
-		}
-		fp->xlated_off = bpf_patch_image_remap(&img, img.new_off, fp->xlated_off);
-	}
-
 	if (img.del)
 		patch_list_del_subprogs(env, img.new_off);
 

With it the prog above returns 124.

But that fixes one instance.
The commit changes prog->len, aux data, func ptrs, subprogs, line info and
insn arrays one after another, and helpers read env and prog in between.
So the order of the steps is load-bearing, and nothing says so.
Make each table a function of (old value, new_off[]), with no helper that
looks at env or prog, and install the image as the last step.
Then this class of bugs cannot happen.

Please add a selftest for it: callx through a pointer in .rodata, the callee
starts with a nop, and main loses more insns than the callee has.
All tests in patches 21 and 22 pass on bpf-next without the series,
so they don't tell whether a remap rule of the commit is wrong.

Also patches 4 and 6 don't apply anymore, since arena_scalar is gone.

Other than that the patches look great.

pw-bot: cr