On Fri, Jun 13, 2025 at 2:17 PM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote: > > 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. I removed it while applying, since it's useless. If do_check_common() changes in the future and cur_state is NULL here the warn will warn, but won't prevent the crash in the next line. Also for tricky things we switched to verifier_bug() instead of WARN. So no new WARNs allowed.
