On Wed, 2025-06-11 at 23:14 +0200, Luis Gerhorst wrote: > 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> > --- Acked-by: Eduard Zingerman <eddyz87@xxxxxxxxx> > 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. > + */ Nit: I think these comments are unnecessary as same logic applies to many places. > 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)); Nit: while at it, I'd push both free_verifier_state() and pop_stack() into free_states() a few lines below. > if (!ret && pop_log) > bpf_vlog_reset(&env->log, 0); > > base-commit: 1d251153a480fc7467d00a8c5dabc55cc6166c43
