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

[Eduard Zingerman <eddyz87@gmail.com>]

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?

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