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

Eduard Zingerman
2025-10-30T17:26:43Z

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? …