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

[Eduard Zingerman <eddyz87@gmail.com>]

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@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/
> Signed-off-by: Luis Gerhorst <luis.gerhorst@fau.de>
> ---

Acked-by: Eduard Zingerman <eddyz87@gmail.com>

>  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