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