Re: [PATCH bpf-next v3 1/2] bpf: Allow void global functions in the verifier

["Emil Tsalapatis" <emil@etsalapatis.com>]

On Tue Feb 24, 2026 at 3:33 PM EST, Eduard Zingerman wrote:
> On Tue, 2026-02-24 at 14:53 -0500, Emil Tsalapatis wrote:
>> On Tue Feb 24, 2026 at 2:02 PM EST, Eduard Zingerman wrote:
>> > On Tue, 2026-02-24 at 13:46 -0500, Emil Tsalapatis wrote:
>> > > On Mon Feb 23, 2026 at 7:09 PM EST, Eduard Zingerman wrote:
>> > > > On Mon, 2026-02-23 at 16:50 -0500, Emil Tsalapatis wrote:
>> > > 
>> > > Ack on the comments for 2/2. For here:
>> > > > 
>> > > > [...]
>> > > > 
>> > > > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> > > > > index 0162f946032f..e997c3776fa7 100644
>> > > > > --- a/kernel/bpf/verifier.c
>> > > > > +++ b/kernel/bpf/verifier.c
>> > > > > @@ -444,6 +444,29 @@ static bool subprog_is_global(const struct bpf_verifier_env *env, int subprog)
>> > > > >  	return aux && aux[subprog].linkage == BTF_FUNC_GLOBAL;
>> > > > >  }
>> > > > >  
>> > > > > +static bool subprog_returns_void(struct bpf_verifier_env *env, int subprog)
>> > > > > +{
>> > > > > +	const struct btf_type *type, *func, *func_proto;
>> > > > > +	const struct btf *btf = env->prog->aux->btf;
>> > > > > +	u32 btf_id;
>> > > > > +
>> > > > > +	btf_id = env->prog->aux->func_info[subprog].type_id;
>> > > > > +
>> > > > > +	func = btf_type_by_id(btf, btf_id);
>> > > > > +	if (verifier_bug_if(!func, env, "btf_id %u not found", btf_id))
>> > > > > +		return false;
>> > > > > +
>> > > > > +	func_proto = btf_type_by_id(btf, func->type);
>> > > > > +	if (verifier_bug_if(!func_proto, env, "btf_id %u not found", func->type))
>> > > > > +		return false;
>> > > > > +
>> > > > > +	type = btf_type_skip_modifiers(btf, func_proto->type, NULL);
>> > > > > +	if (verifier_bug_if(!type, env, "btf_id %u not found", func_proto->type))
>> > > > > +		return false;
>> > > > > +
>> > > > 
>> > > > Nit: I there there are a few unnecessary 'verifier_bug_if()' checks here,
>> > > >      e.g. btf.c:btf_check_all_types() guarantees that func->type and func_proto->type
>> > > >      would be valid.
>> > > > 
>> > > 
>> > > Ack, just to make sure I got it right at all the verifier_bug_if() are
>> > > unnecessary. unnecessary because the BTF is already checked. There's no way we
>> > > have an existing type with invalid fields. We also know the subprog btf
>> > > ID is valid from check_btf_func_early that happens way before
>> > > do_check_subprogs where subprog_returns_void is used.
>> > 
>> > Certainly not needed for 'func->type' and 'func_proto->type' access.
>> > For 'func_info[subprog].type_id', idk -- depends on how defensive you
>> > want to be, I'd skip it as well.
>> > 
>> > > 
>> > > > > +	return btf_type_is_void(type);
>> > > > > +}
>> > > > > +
>> > > > >  static const char *subprog_name(const struct bpf_verifier_env *env, int subprog)
>> > > > >  {
>> > > > >  	struct bpf_func_info *info;
>> > > > 
>> > > > [...]
>> > > > 
>> > > > > @@ -17812,6 +17837,16 @@ static int check_return_code(struct bpf_verifier_env *env, int regno, const char
>> > > > >  
>> > > > >  	/* LSM and struct_ops func-ptr's return type could be "void" */
>> > > > >  	if (!is_subprog || frame->in_exception_callback_fn) {
>> > > > > +
>> > > > > +		/*
>> > > > > +		 * If the actual program is an extension, let it
>> > > > > +		 * return void - attaching will succeed only if the
>> > > > > +		 * program being replaced also returns void, and since
>> > > > > +		 * it has passed verification its actual type doesn't matter.
>> > > > > +		 */
>> > > > > +		if (env->prog->type == BPF_PROG_TYPE_EXT && subprog_returns_void(env, frame->subprogno))
>> > > > > +			return 0;
>> > > > > +
>> > > > >  		switch (prog_type) {
>> > > > >  		case BPF_PROG_TYPE_LSM:
>> > > > >  			if (prog->expected_attach_type == BPF_LSM_CGROUP)
>> > > > > @@ -17841,6 +17876,10 @@ static int check_return_code(struct bpf_verifier_env *env, int regno, const char
>> > > > >  		default:
>> > > > >  			break;
>> > > > >  		}
>> > > > > +	} else {
>> > > > > +		/* If this is a void global subprog, there is no return value. */
>> > > > > +		if (subprog_is_global(env, frame->subprogno) && subprog_returns_void(env, frame->subprogno))
>> > > > > +			return 0;
>> > > > 
>> > > > Suppose a global subprogram is verified and it calls bpf_throw().
>> > > > check_return_code() is called from check_kfunc_call() in such case
>> > > > with R1 as a parameter.  This check acts on the return type of the
>> > > > program, will it miss proper return value check for the program?
>> > > > 
>> > > 
>> > > Due to the short-circuiting herea a void global program can bpf_throw() 
>> > > with no issue. This is what we want, correct? The return type we check 
>> > > in that case would always be that of the u64 cookie AFAICT.
>> > 
>> > I mean the following situation:
>> > 
>> >   SEC(<I want some special return code>)
>> >   int foo(...) {
>> >     ... bar(...) ...;
>> >     return ...;
>> >   }
>> > 
>> >   // global func
>> >   void bar(...) { 
>> >     ... bpf_throw(<bad return code>) ...
>> >   }
>> > 
>> > In this case 'bar' would correspond to 'frame' in the check above and
>> > <bad return code> won't be checked.
>> > 
>> 
>> I see, yes this spuriously passes. Since the check_error_code only runs
>> for BPF_EXIT and bpf_throw, can we just check if we're inspecting
>> bpf_throw()'s return value like so?
>> 
>> bool is_bpf_throw = regno == BPF_REG_1;
>> ...
>> if (subprog_is_global(env, frame->subprogno) && subprog_returns_void(env, frame->subprogno)
>> 	&& !is_bpf_throw)
>> ...
>> 
>> We only need to use this check in two places in check_error_code
>> to solve the problem. With the fix we can also still throw from
>> void global() functions.
>
> Would it be possible to split check_return_code() in two:
> - check_return_code() that checks program return code
> - check_throw_return_code() with throw specific logic,
>   referring back to check_return_code() if necessary?
>

I will do that, check_return_code will need some refactoring but
the overall code will be cleaner (e.g., the retval_range() calculation
can be factored out into a pure function). I will tack on the patches
with the non-functional changes at the front of this series.

>> > > 
>> > > > >  	}
>> > > > >  
>> > > > >  	/* eBPF calling convention is such that R0 is used
>> > > > 
>> > > > [...]