Re: [PATCH bpf-next v2 2/3] bpf: Fix aux usage after do_check_insn()

[Eduard Zingerman]

On Sat, 2025-06-28 at 16:50 +0200, Luis Gerhorst wrote:

[...]

> @@ -19955,11 +19960,11 @@ static int do_check(struct bpf_verifier_env *env)
>  			/* Prevent this speculative path from ever reaching the
>  			 * insn that would have been unsafe to execute.
>  			 */
> -			cur_aux(env)->nospec = true;
> +			prev_aux(env)->nospec = true;

I don't like the prev_aux() call in this position, as one needs to
understand that after do_check_insn() call what was current became
previous. This at-least requires a comment. Implementation with a
temporary variable (as at the bottom of this email), imo, is less
cognitive load.

>  			/* IF it was an ADD/SUB insn, potentially remove any
>  			 * markings for alu sanitization.
>  			 */
> -			cur_aux(env)->alu_state = 0;
> +			prev_aux(env)->alu_state = 0;
>  			goto process_bpf_exit;
>  		} else if (err < 0) {
>  			return err;

[...]

---

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index a136d9b1b25f..a923614b7104 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -19953,6 +19953,7 @@ static int do_check(struct bpf_verifier_env *env)
 	bool pop_log = !(env->log.level & BPF_LOG_LEVEL2);
 	struct bpf_verifier_state *state = env->cur_state;
 	struct bpf_insn *insns = env->prog->insnsi;
+	struct bpf_insn_aux_data *insn_aux;
 	int insn_cnt = env->prog->len;
 	bool do_print_state = false;
 	int prev_insn_idx = -1;
@@ -19972,6 +19973,7 @@ static int do_check(struct bpf_verifier_env *env)
 		}
 
 		insn = &insns[env->insn_idx];
+		insn_aux = &env->insn_aux_data[env->insn_idx];
 
 		if (++env->insn_processed > BPF_COMPLEXITY_LIMIT_INSNS) {
 			verbose(env,
@@ -20048,7 +20050,7 @@ static int do_check(struct bpf_verifier_env *env)
 		/* Reduce verification complexity by stopping speculative path
 		 * verification when a nospec is encountered.
 		 */
-		if (state->speculative && cur_aux(env)->nospec)
+		if (state->speculative && insn_aux->nospec)
 			goto process_bpf_exit;
 
 		err = do_check_insn(env, &do_print_state);
@@ -20056,11 +20058,11 @@ static int do_check(struct bpf_verifier_env *env)
 			/* Prevent this speculative path from ever reaching the
 			 * insn that would have been unsafe to execute.
 			 */
-			cur_aux(env)->nospec = true;
+			insn_aux->nospec = true;
 			/* If it was an ADD/SUB insn, potentially remove any
 			 * markings for alu sanitization.
 			 */
-			cur_aux(env)->alu_state = 0;
+			insn_aux->alu_state = 0;
 			goto process_bpf_exit;
 		} else if (err < 0) {
 			return err;
@@ -20069,7 +20071,7 @@ static int do_check(struct bpf_verifier_env *env)
 		}
 		WARN_ON_ONCE(err);
 
-		if (state->speculative && cur_aux(env)->nospec_result) {
+		if (state->speculative && insn_aux->nospec_result) {
 			/* If we are on a path that performed a jump-op, this
 			 * may skip a nospec patched-in after the jump. This can
 			 * currently never happen because nospec_result is only