Re: [PATCH bpf-next v1 3/8] bpf: Support for kfuncs with KF_MAGIC_ARGS

[Eduard Zingerman <eddyz87@gmail.com>]

On Thu, 2025-10-30 at 09:31 -0700, Ihor Solodrai wrote:
> Hi Eduard, thank you for a quick review.
> 
> On 10/29/25 4:54 PM, Eduard Zingerman wrote:
> > On Wed, 2025-10-29 at 12:01 -0700, Ihor Solodrai wrote:
> > > A kernel function bpf_foo with KF_MAGIC_ARGS flag is expected to have
> >                                  ^^^^^^^^^^^^^
> > 		I don't like this name very much.
> > 		It bears very little context.
> > 		Imo, KF_IMPLICIT_ARGS fits the use case much better.
> 
> I know, naming is hard...
> 
> The issue is that it's not only the flag, across the code we need
> descriptive names for every "magic" thing:
>   * a flagged function
>     * how do we call it? kfunc_with_impl_args?
>   * a function that exists only in BTF (_impl)
>     * it's not an "implicit" function
>     * it's not exactly an "implementation" function
>     * "fake" is even worse than "magic" IMO, because it's not fake,
>       but you could argue it's magical :D
>     * btf_only_kfunc?
>   * describing arguments is simpler: "implicit" seems ok, although as
>     Alexei pointed out in previous iteration they are very much
>     explicit in the kernel [1]
> 
> For me, "(BPF) interface" and "(kernel) implementation" pair of terms
> makes sense, but then I think it would be logical to have both
> declarations in the kernel.
> 
> The advantage of "magic" in this context is that it doesn't have
> loaded meaning. But I agree this is a stretch, so can't insist.
> 
> [1] https://lore.kernel.org/bpf/CAADnVQLvuubey0A0Fk=bzN-=JG2UUQHRqBijZpuvqMQ+xy4W4g@mail.gmail.com/

- KF_IMPLICIT_ARGS
- explicit_args_id -- for prototype with full set of args
- implicit_args_id -- for prototype with missing args

[...]

> > > @@ -3349,8 +3400,37 @@ static int add_kfunc_call(struct bpf_verifier_env *env, u32 func_id, s16 offset)
> > >  		return -EINVAL;
> > >  	}
> > >  
> > > +	kfunc_flags = btf_kfunc_flags(desc_btf, func_id, env->prog);
> > >  	func_name = btf_name_by_offset(desc_btf, func->name_off);
> > >  	addr = kallsyms_lookup_name(func_name);
> > > +
> > > +	/* This may be an _impl kfunc with KF_MAGIC_ARGS counterpart */
> > > +	if (unlikely(!addr && !kfunc_flags)) {
> > > +		tmp_func_id = magic_kfunc_by_impl(func_id);
> > 
> > I think there is no need to hide magic_kfunc_by_impl() call behind the
> > above condition. It can be moved before kfunc_flags assignment.
> > Then it wont be necessary to textually repeat btf_name_by_offset() and
> > kallsyms_lookup_name() calls.
> 
> Not sure I follow...
> 
> Yes, !addr is enough to detect potential _impl function, but there is
> no way around name lookup in BTF and then another address lookup.
> 
> The _impl function doesn't have an address, so after failed
>   kallsyms_lookup_name("kfunc_impl");
> we must do
>   kallsyms_lookup_name("kfunc");
> to find the correct address.
> 
> Or do you suggest doing something like:
> 
>   tmp_func_id = magic_kfunc_by_impl(func_id);
>   if (tmp_func_id > 0)
>       func_id = tmp_func_id;
> 
> at the beginning of add_kfunc_call()?

This, just check for magic_kfunc_by_impl() and replace func_id.

[...]

> > > @@ -13632,10 +13718,28 @@ static int fetch_kfunc_meta(struct bpf_verifier_env *env,
> > >  	func_proto = btf_type_by_id(desc_btf, func->type);
> > >  
> > >  	kfunc_flags = btf_kfunc_flags_if_allowed(desc_btf, func_id, env->prog);
> > > -	if (!kfunc_flags) {
> > > -		return -EACCES;
> > > +	if (unlikely(!kfunc_flags)) {
> > 
> > What if we patch insn->imm to use the "fake" function id in add_kfunc_call()?
> > Then modifications to fetch_kfunc_meta() wont be necessary.
> 
> 
> I considered this. I wasn't sure it's safe to patch insn->imm at this
> stage of verification. Also I thought it may be harder to debug the
> verifier if we do btf id replacement in the calls pre-verification
> (because we lose the original btf id).

See no issues with that.

> Maybe I was too causious.
> 
> Alexei, Andrii, what do you think?

[...]