bpf: Remove redundant free_verifier_state()/pop_stack()

Eduard points out [1]:

    Same cleanup cycles are done in push_stack() and push_async_cb(),
    both functions are only reachable from do_check_common() via
    do_check() -> do_check_insn().

    Hence, I think that cur state should not be freed in push_*()
    functions and pop_stack() loop there is not needed.

This would also fix the 'symptom' for [2], but the issue has a more
logical (and also simpler) fix which was sent separately.

Caller must return an error for which error_recoverable_with_nospec(err)
is false, otherwise we try to recover and access the stale state.

[1] https://lore.kernel.org/all/[email protected]/
[2] https://lore.kernel.org/all/[email protected]/

Reported-by: Eduard Zingerman <[email protected]>
Signed-off-by: Luis Gerhorst <[email protected]>

Author: luisgerhorst

kernel/bpf/verifier.c

@@ -2066,10 +2066,10 @@ static struct bpf_verifier_state *push_stack(struct bpf_verifier_env *env,
 	}
 	return &elem->st;
 err:
-	free_verifier_state(env->cur_state, true);
-	env->cur_state = NULL;
-	/* pop all elements and return */
-	while (!pop_stack(env, NULL, NULL, false));
+	/* free_verifier_state() and pop_stack() loop will be done in
+	 * do_check_common(). Caller must return an error for which
+	 * error_recoverable_with_nospec(err) is false.
+	 */
 	return NULL;
 }
 
@@ -2838,10 +2838,10 @@ static struct bpf_verifier_state *push_async_cb(struct bpf_verifier_env *env,
 	elem->st.frame[0] = frame;
 	return &elem->st;
 err:
-	free_verifier_state(env->cur_state, true);
-	env->cur_state = NULL;
-	/* pop all elements and return */
-	while (!pop_stack(env, NULL, NULL, false));
+	/* free_verifier_state() and pop_stack() loop will be done in
+	 * do_check_common(). Caller must return an error for which
+	 * error_recoverable_with_nospec(err) is false.
+	 */
 	return NULL;
 }
 
@@ -22904,13 +22904,9 @@ static int do_check_common(struct bpf_verifier_env *env, int subprog)
 
 	ret = do_check(env);
 out:
-	/* check for NULL is necessary, since cur_state can be freed inside
-	 * do_check() under memory pressure.
-	 */
-	if (env->cur_state) {
-		free_verifier_state(env->cur_state, true);
-		env->cur_state = NULL;
-	}
+	WARN_ON_ONCE(!env->cur_state);
+	free_verifier_state(env->cur_state, true);
+	env->cur_state = NULL;
 	while (!pop_stack(env, NULL, NULL, false));
 	if (!ret && pop_log)
 		bpf_vlog_reset(&env->log, 0);