On Fri, 2025-06-13 at 11:01 +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`. > > 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@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/ > Acked-by: Eduard Zingerman <eddyz87@xxxxxxxxx> > Signed-off-by: Luis Gerhorst <luis.gerhorst@xxxxxx> > --- Tried v2, all looks good. [...] > @@ -22934,6 +22922,11 @@ static void free_states(struct bpf_verifier_env *env) > struct bpf_scc_info *info; > int i, j; > > + WARN_ON_ONCE(!env->cur_state); Tbh I woudn't do this a warning, just an 'if (env->cur_state) ...', but that's immaterial. Given current way do_check_common() is written env->cur_state != NULL at this point, so the patch is safe to land. > + 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);
