[PATCH bpf-next] bpf: Remove redundant free_verifier_state()/pop_stack()

[Luis Gerhorst <luis.gerhorst@xxxxxx>]

This patch removes duplicated code.

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 also has a
simpler fix which was sent separately. This fix also makes sure the
push_*() callers always return an error for which
error_recoverable_with_nospec(err) is false. This is required because
otherwise we try to recover and access the stale `state`.

[1] https://lore.kernel.org/all/b6931bd0dd72327c55287862f821ca6c4c3eb69a.camel@xxxxxxxxx/
[2] https://lore.kernel.org/all/68497853.050a0220.33aa0e.036a.GAE@xxxxxxxxxx/

Reported-by: Eduard Zingerman <eddyz87@xxxxxxxxx>
Link: https://lore.kernel.org/all/b6931bd0dd72327c55287862f821ca6c4c3eb69a.camel@xxxxxxxxx/
Signed-off-by: Luis Gerhorst <luis.gerhorst@xxxxxx>
---
 kernel/bpf/verifier.c | 26 +++++++++++---------------
 1 file changed, 11 insertions(+), 15 deletions(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index d3bff0385a55..fa147c207c4b 100644
--- a/kernel/bpf/verifier.c
+++ b/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);

base-commit: 1d251153a480fc7467d00a8c5dabc55cc6166c43
-- 
2.49.0