Re: [PATCH bpf-next v5 04/11] bpf: Track R2 of register-pair returns in precision backtracking

[Eduard Zingerman <eddyz87@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2116 bytes --]

On Thu, 2026-08-13 at 13:02 -0700, Yonghong Song wrote:

...

> @@ -520,7 +541,41 @@ static int backtrack_insn(struct bpf_verifier_env *env, int idx, int subseq_idx,
>  					return -EFAULT;
>  			}
>  		} else if (opcode == BPF_EXIT) {
> -			bool r0_precise;
> +			bool from_subprog_call, r0_precise, r2_precise;
> +			struct bpf_insn *call;
> +			int subprog;
> +
> +			/* BPF_EXIT in subprog or callback always returns
> +			 * right after the call instruction, so by checking
> +			 * whether the instruction at subseq_idx-1 is subprog
> +			 * call or not we can distinguish actual exit from
> +			 * *subprog* from exit from *callback*. In the former
> +			 * case, we need to propagate the precision of the
> +			 * return registers, if necessary. In the latter we
> +			 * never do that.
> +			 */
> +			from_subprog_call = subseq_idx - 1 >= 0 &&
> +					    bpf_pseudo_call(&env->prog->insnsi[subseq_idx - 1]);
> +
> +			/* Sample the return registers before the callback
> +			 * handling below clears R1-R5: unlike R0, R2 is an
> +			 * argument register as well, so that clear would drop
> +			 * a pair return on the floor.
> +			 */
> +			r0_precise = from_subprog_call && bt_is_reg_set(bt, BPF_REG_0);
> +			r2_precise = false;
> +			if (from_subprog_call && bt_is_reg_set(bt, BPF_REG_2)) {
> +				call = &env->prog->insnsi[subseq_idx - 1];
> +				subprog = bpf_find_subprog(env, subseq_idx + call->imm);
> +				if (subprog < 0)
> +					return -EFAULT;
> +				/* Only a callee that does return a pair defines
> +				 * R2. Leave the mask alone otherwise, so that
> +				 * the check below still catches an R2 that has
> +				 * no business being set.
> +				 */
> +				r2_precise = bpf_ret_reg_pair(env, subprog);
> +			}
>  
>  			/* Backtracking to a nested function call, 'idx' is a part of
>  			 * the inner frame 'subseq_idx' is a part of the outer frame.

I still think that the above complications are unnecessary.
The patch could be simplified by assuming that R2 always propagates
w/o loosing verification safety. E.g. as in the attachment.

[-- Attachment #2: backtracking.diff --]
[-- Type: text/x-patch, Size: 3828 bytes --]

diff --git a/kernel/bpf/backtrack.c b/kernel/bpf/backtrack.c
index a2b18a9f1694..98bf59d7ae56 100644
--- a/kernel/bpf/backtrack.c
+++ b/kernel/bpf/backtrack.c
@@ -423,6 +423,10 @@ static int backtrack_insn(struct bpf_verifier_env *env, int idx, int subseq_idx,
 				 */
 				verifier_bug_if(idx + 1 != subseq_idx, env,
 						"extra insn from subprog");
+				/* global subprog always sets R0 */
+				bt_clear_reg(bt, BPF_REG_0);
+				/* and if it does not set R2, main pass would catch it */
+				bt_clear_reg(bt, BPF_REG_2);
 				/* r1-r5 are invalidated after subprog call,
 				 * so for global func call it shouldn't be set
 				 * anymore
@@ -432,8 +436,6 @@ static int backtrack_insn(struct bpf_verifier_env *env, int idx, int subseq_idx,
 						     bt_reg_mask(bt));
 					return -EFAULT;
 				}
-				/* global subprog always sets R0 */
-				bt_clear_reg(bt, BPF_REG_0);
 				return 0;
 			} else {
 				/* static subprog call instruction, which
@@ -506,6 +508,8 @@ static int backtrack_insn(struct bpf_verifier_env *env, int idx, int subseq_idx,
 				return -ENOTSUPP;
 			/* regular helper call sets R0 */
 			bt_clear_reg(bt, BPF_REG_0);
+			/* kfunc might aslo set R2 */
+			bt_clear_reg(bt, BPF_REG_2);
 			if (bt_reg_mask(bt) & BPF_REGMASK_ARGS) {
 				/* if backtracking was looking for registers R1-R5
 				 * they should have been found already.
@@ -520,7 +524,25 @@ static int backtrack_insn(struct bpf_verifier_env *env, int idx, int subseq_idx,
 					return -EFAULT;
 			}
 		} else if (opcode == BPF_EXIT) {
-			bool r0_precise;
+			bool from_subprog_call, r0_precise, r2_precise;
+
+			/* BPF_EXIT in subprog or callback always returns
+			 * right after the call instruction, so by checking
+			 * whether the instruction at subseq_idx-1 is subprog
+			 * call or not we can distinguish actual exit from
+			 * *subprog* from exit from *callback*. In the former
+			 * case, we need to propagate the precision of the
+			 * return registers, if necessary. In the latter we
+			 * never do that.
+			 */
+			from_subprog_call = subseq_idx - 1 >= 0 &&
+					    bpf_pseudo_call(&env->prog->insnsi[subseq_idx - 1]);
+
+			/* Sample the return registers before the callback
+			 * handling below clears R1-R5.
+			 */
+			r0_precise = from_subprog_call && bt_is_reg_set(bt, BPF_REG_0);
+			r2_precise = from_subprog_call && bt_is_reg_set(bt, BPF_REG_2);
 
 			/* Backtracking to a nested function call, 'idx' is a part of
 			 * the inner frame 'subseq_idx' is a part of the outer frame.
@@ -533,30 +555,22 @@ static int backtrack_insn(struct bpf_verifier_env *env, int idx, int subseq_idx,
 			if (subseq_idx >= 0 && bpf_calls_callback(env, subseq_idx))
 				for (i = BPF_REG_1; i <= BPF_REG_5; i++)
 					bt_clear_reg(bt, i);
+
+			bt_clear_reg(bt, BPF_REG_0);
+			bt_clear_reg(bt, BPF_REG_2);
 			if (bt_reg_mask(bt) & BPF_REGMASK_ARGS) {
 				verifier_bug(env, "backtracking exit unexpected regs %x",
 					     bt_reg_mask(bt));
 				return -EFAULT;
 			}
 
-			/* BPF_EXIT in subprog or callback always returns
-			 * right after the call instruction, so by checking
-			 * whether the instruction at subseq_idx-1 is subprog
-			 * call or not we can distinguish actual exit from
-			 * *subprog* from exit from *callback*. In the former
-			 * case, we need to propagate r0 precision, if
-			 * necessary. In the former we never do that.
-			 */
-			r0_precise = subseq_idx - 1 >= 0 &&
-				     bpf_pseudo_call(&env->prog->insnsi[subseq_idx - 1]) &&
-				     bt_is_reg_set(bt, BPF_REG_0);
-
-			bt_clear_reg(bt, BPF_REG_0);
 			if (bt_subprog_enter(bt))
 				return -EFAULT;
 
 			if (r0_precise)
 				bt_set_reg(bt, BPF_REG_0);
+			if (r2_precise)
+				bt_set_reg(bt, BPF_REG_2);
 			/* r6-r9 and stack slots will stay set in caller frame
 			 * bitmasks until we return back from callee(s)
 			 */