Re: [PATCH bpf-next v1 1/6] bpf: implement KF_IMPLICIT_PROG_AUX_ARG flag

Andrii Nakryiko
2025-09-26T15:11:37Z

On Fri, Sep 26, 2025 at 5:10 AM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Fri, Sep 26, 2025 at 12:07 AM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Thu, Sep 25, 2025 at 3:58 PM Kumar Kartikeya Dwivedi
> > <memxor@gmail.com> wrote:
> > >
> > > On Fri, 26 Sept 2025 at 00:54, Andrii Nakryiko
> > > <andrii.nakryiko@gmail.com> wrote:
> > > >
> > > > On Thu, Sep 25, 2025 at 12:35 PM Alexei Starovoitov
> > > > <alexei.starovoitov@gmail.com> wrote:
> > > > >
> > > > > On Thu, Sep 25, 2025 at 6:23 PM Andrii Nakryiko
> > > > > <andrii.nakryiko@gmail.com> wrote:
> > > > > >
> > > > > > I do see the benefit of having the generic "KF_MAGIC_ARG(s)" flag on
> > > > > > the kernel side of things and having access to full BTF information
> > > > > > for parameters to let verifier know what specific kind of magic
> > > > > > argument that kfunc has, though. So as an alternative, maybe we can
> > > > > > create both a kfunc definition *meant for BPF programs* (i.e., without
> > > > > > magic argument(s)), and then have a full original definition (produced
> > > > > > by pahole, it will need to understand KF_MAGIC_ARGS anyways) with full
> > > > > > type information *for internal BPF verifier needs*. I don't know
> > > > > > what's the best way to do that, maybe just a special ".magic" suffix,
> > > > > > just to let the verifier easily find that? On the kernel side, if
> > > > > > kfunc has BPF_MAGIC_ARGS kflag we just look up "my_fancy_kfunc.magic"
> > > > > > FUNC definition?
> > > > >
> > > > > Interesting idea. Maybe to simplify backward compat the pahole can
> > > > > emit two BTFs: kfunc_foo(args), kfunc_foo_impl(args, void *aux)
> > > > > into vmlinux BTF.
> > > > > bpftool will emit both in vmlinux.h and bpf side doesn't need to change.
> > > > > libbpf doesn't need to change either.
> > > > > The verifier would need a special check to resolve two kfunc BTFs
> > > > > name into one kallsym name, since both kfuncs is one actual function
> > > > > on the kernel.
> > > > > bpf_wq_set_callback_impl() definition doesn't change. Only:
> > > > > -BTF_ID_FLAGS(func, bpf_wq_set_callback_impl)
> > > > > +BTF_ID_FLAGS(func, bpf_wq_set_callback_impl, KF_PROG_ARG)
> > > > >
> > > > > and the verifier can check that the last arg is aux__prog when
> > > > > KF_PROG_ARG is specified.
> > > > >
> > > > > The runtime performance will be slightly better too, since
> > > > > no need for wrappers like:
> > > > >
> > > > > +__bpf_kfunc int bpf_wq_set_callback_impl(struct bpf_wq *wq,
> > > > > + int (callback_fn)(void *map, int *key, void *value),
> > > > > + unsigned int flags,
> > > > > + void *aux__prog)
> > > > > +{
> > > > > + return bpf_wq_set_callback(wq, callback_fn, flags, aux__prog);
> > > > > +}
> > > > >
> > > > > It's just one jmpl insn, but still.
> > > >
> > > > So basically xxx_impl() will be a phantom function that verifier will
> > > > recognize and it will need to have corresponding xxx() kfunc with
> > > > corresponding KF_PROG_ARG for everything to work. Makes sense.
> > > >
> > > > Two notes:
> > > >
> > > > a) KF flag would need to be more generically named, because we'll have
> > > > other implicit arguments (like those for bpf_obj_new_impl, for
> > > > example), which will be distinguished based on their BTF type
>
> agree
>
> > > >
> > > > b) bpf_stream_vprintk() throws a bit of a wrench into all this because
> > > > it doesn't follow _impl naming convention. Any suggestions on how to
> > > > deal with that?
> > >
> > > We can probably do a compat break for this kfunc alone for now, it's
> > > not been a long time since it's been out (1 release) not much adoption
> > > yet.
> >
> > Ideally there would be 0 releases with that name :) it's just
> > tantalizing that we can s/bpf_stream_vprintk/bpf_stream_vprintk_impl/
> > in like 8 places and avoid this altogether, but it is so late in the
> > release that I suspect no one will want to do this last minute "fix".
>
> I think we can still do it even after 6.17 is released and
> backport it as a fix.

ok then, sounds like a plan