linux-kernel - Re: [PATCH bpf-next] bpf: Remove redundant free_verifier_state()/pop

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <878qlw3t76.fsf@fau.de>
Date: Fri, 13 Jun 2025 11:07:57 +0200
From: Luis Gerhorst <luis.gerhorst@....de>
To: Eduard Zingerman <eddyz87@...il.com>
Cc: Alexei Starovoitov <ast@...nel.org>,  Daniel Borkmann
 <daniel@...earbox.net>,  John Fastabend <john.fastabend@...il.com>,
  Andrii Nakryiko <andrii@...nel.org>,  Martin KaFai Lau
 <martin.lau@...ux.dev>,  Song Liu <song@...nel.org>,  Yonghong Song
 <yonghong.song@...ux.dev>,  KP Singh <kpsingh@...nel.org>,  Stanislav
 Fomichev <sdf@...ichev.me>,  Hao Luo <haoluo@...gle.com>,  Jiri Olsa
 <jolsa@...nel.org>,  bpf@...r.kernel.org,  linux-kernel@...r.kernel.org
Subject: Re: [PATCH bpf-next] bpf: Remove redundant
 free_verifier_state()/pop_stack()

Eduard Zingerman <eddyz87@...il.com> 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.)