Re: [PATCH bpf-next v5 04/11] bpf: Track R2 of register-pair returns in precision backtracking
Eduard Zingerman <[email protected]>
| Newsgroups | org.kernel.vger.bpf |
|---|---|
| Message-ID | <[email protected]> |
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.
backtracking.diff
(text/x-patch, 3.7 KB)
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)
*/