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

[Luis Gerhorst]

Eduard Zingerman <eddyz87@xxxxxxxxx> writes:

> On Wed, 2025-06-11 at 23:14 +0200, Luis Gerhorst wrote:
> 
>>  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.

In that case I turned `goto err` into `return NULL` directly.

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

Both is in v2, thanks! (Also reran the syzbot reproducer with it.)