bpf: Remove redundant free_verifier_state()/pop_stack()

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`.

Moving free_verifier_state() and pop_stack(..., pop_log=false) to happen
after the bpf_vlog_reset() call in do_check_common() is fine because the
pop_stack() call that is moved does not call bpf_vlog_reset() with the
pop_log=false parameter.

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

Reported-by: Eduard Zingerman <eddyz87@gmail.com>
Link: https://lore.kernel.org/all/b6931bd0dd72327c55287862f821ca6c4c3eb69a.camel@gmail.com/
Acked-by: Eduard Zingerman <eddyz87@gmail.com>
Signed-off-by: Luis Gerhorst <luis.gerhorst@fau.de>
Link: https://lore.kernel.org/r/20250613090157.568349-2-luis.gerhorst@fau.de
Signed-off-by: Alexei Starovoitov <ast@kernel.org>

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index a1c312e216ca980bbd838977341a78fcfa4f37a7..279a6493326240a04eacfa0f188e493436f5f23f 100644 (file)
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -2097,7 +2097,7 @@ static struct bpf_verifier_state *push_stack(struct bpf_verifier_env *env,
 
        elem = kzalloc(sizeof(struct bpf_verifier_stack_elem), GFP_KERNEL_ACCOUNT);
        if (!elem)
-               goto err;
+               return NULL;
 
        elem->insn_idx = insn_idx;
        elem->prev_insn_idx = prev_insn_idx;
@@ -2107,12 +2107,12 @@ static struct bpf_verifier_state *push_stack(struct bpf_verifier_env *env,
        env->stack_size++;
        err = copy_verifier_state(&elem->st, cur);
        if (err)
-               goto err;
+               return NULL;
        elem->st.speculative |= speculative;
        if (env->stack_size > BPF_COMPLEXITY_LIMIT_JMP_SEQ) {
                verbose(env, "The sequence of %d jumps is too complex.\n",
                        env->stack_size);
-               goto err;
+               return NULL;
        }
        if (elem->st.parent) {
                ++elem->st.parent->branches;
@@ -2127,12 +2127,6 @@ 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));
-       return NULL;
 }
 
 #define CALLER_SAVED_REGS 6
@@ -2864,7 +2858,7 @@ static struct bpf_verifier_state *push_async_cb(struct bpf_verifier_env *env,
 
        elem = kzalloc(sizeof(struct bpf_verifier_stack_elem), GFP_KERNEL_ACCOUNT);
        if (!elem)
-               goto err;
+               return NULL;
 
        elem->insn_idx = insn_idx;
        elem->prev_insn_idx = prev_insn_idx;
@@ -2876,7 +2870,7 @@ static struct bpf_verifier_state *push_async_cb(struct bpf_verifier_env *env,
                verbose(env,
                        "The sequence of %d jumps is too complex for async cb.\n",
                        env->stack_size);
-               goto err;
+               return NULL;
        }
        /* Unlike push_stack() do not copy_verifier_state().
         * The caller state doesn't matter.
@@ -2887,19 +2881,13 @@ static struct bpf_verifier_state *push_async_cb(struct bpf_verifier_env *env,
        elem->st.in_sleepable = is_sleepable;
        frame = kzalloc(sizeof(*frame), GFP_KERNEL_ACCOUNT);
        if (!frame)
-               goto err;
+               return NULL;
        init_func_state(env, frame,
                        BPF_MAIN_FUNC /* callsite */,
                        0 /* frameno within this callchain */,
                        subprog /* subprog number within this prog */);
        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));
-       return NULL;
 }
 
 
@@ -22935,6 +22923,10 @@ static void free_states(struct bpf_verifier_env *env)
        struct bpf_scc_info *info;
        int i, j;
 
+       free_verifier_state(env->cur_state, true);
+       env->cur_state = NULL;
+       while (!pop_stack(env, NULL, NULL, false));
+
        list_for_each_safe(pos, tmp, &env->free_list) {
                sl = container_of(pos, struct bpf_verifier_state_list, node);
                free_verifier_state(&sl->state, false);
@@ -23086,14 +23078,6 @@ 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;
-       }
-       while (!pop_stack(env, NULL, NULL, false));
        if (!ret && pop_log)
                bpf_vlog_reset(&env->log, 0);
        free_states(env);