Re: [PATCH bpf-next v2 1/3] bpf: new btf kfunc hooks for tracepoint and perf event

[JP Kobryn <inwardvessel@gmail.com>]

On Tue, Aug 27, 2024 at 03:42:34PM -0700, Andrii Nakryiko wrote:
> On Tue, Aug 27, 2024 at 2:01 PM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > On Mon, Aug 26, 2024 at 3:48 PM JP Kobryn <inwardvessel@gmail.com> wrote:
> > >
> > > The additional hooks (and prog-to-hook mapping) for tracepoint and perf
> > > event programs allow for registering kfuncs to be used within these
> > > program types.
> > >
> > > Signed-off-by: JP Kobryn <inwardvessel@gmail.com>
> > > ---
> > >  kernel/bpf/btf.c | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > >
> > > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > > index 520f49f422fe..4816e309314e 100644
> > > --- a/kernel/bpf/btf.c
> > > +++ b/kernel/bpf/btf.c
> > > @@ -210,6 +210,7 @@ enum btf_kfunc_hook {
> > >         BTF_KFUNC_HOOK_TC,
> > >         BTF_KFUNC_HOOK_STRUCT_OPS,
> > >         BTF_KFUNC_HOOK_TRACING,
> > > +       BTF_KFUNC_HOOK_TRACEPOINT,
> > >         BTF_KFUNC_HOOK_SYSCALL,
> > >         BTF_KFUNC_HOOK_FMODRET,
> > >         BTF_KFUNC_HOOK_CGROUP_SKB,
> > > @@ -219,6 +220,7 @@ enum btf_kfunc_hook {
> > >         BTF_KFUNC_HOOK_LWT,
> > >         BTF_KFUNC_HOOK_NETFILTER,
> > >         BTF_KFUNC_HOOK_KPROBE,
> > > +       BTF_KFUNC_HOOK_PERF_EVENT,
> > >         BTF_KFUNC_HOOK_MAX,
> > >  };
> > >
> > > @@ -8306,6 +8308,8 @@ static int bpf_prog_type_to_kfunc_hook(enum bpf_prog_type prog_type)
> > >         case BPF_PROG_TYPE_TRACING:
> > >         case BPF_PROG_TYPE_LSM:
> > >                 return BTF_KFUNC_HOOK_TRACING;
> > > +       case BPF_PROG_TYPE_TRACEPOINT:
> > > +               return BTF_KFUNC_HOOK_TRACEPOINT;
> >
> > why special case tp and perf_event and only limit them to cpumask?
> > The following would be equally safe, no?
> 
> Assuming we don't have kfuncs that accepts program context (like
> bpf_get_stack(), if it was a kfunc) and that doesn't access
> bpf_run_ctx (like bpf_get_func_ip()). We just need to be careful about
> adding new special kfuncs like that going forward (not sure how to
> best ensure we don't forget, though). Other than that I agree that
> it's all "tracing".

What Alexei is suggesting works. I did something similar in v1[0] where I
associated BPF_PROG_TYPE_TRACEPOINT with BTF_KFUNC_HOOK_TRACING. But it
occurred to me that this circumvents the registration process during
initialization, so I want to make sure if this is or is not acceptable. See
below for my thoughts.
> 
> >          case BPF_PROG_TYPE_TRACING:
> >          case BPF_PROG_TYPE_LSM:
> >  +       case BPF_PROG_TYPE_TRACEPOINT:
> >  +       case BPF_PROG_TYPE_PERF_EVENT:
> >                 return BTF_KFUNC_HOOK_TRACING;
> > ?

With this change, anywhere we do
register_btf_kfunc_id_set(BPF_PROG_TYPE_TRACING, &some_kfunc_set),
BTF_KFUNC_HOOK_TRACING becomes allowed. So even if we never register the
extra program types like PROG_TYPE_PERF_EVENT, we still allow them as a
side effect since at runtime the program type mapping returns HOOK_TRACING.
Any program type associated with this hook will be allowed even though not
explicitly registered. My take on v2 was moving towards the element of
least surprise, and thought the explicit registration with the new hooks
made sense. I'm fine though, if we prefer this style above with the
implicit registration. Let me know and I can make a v3 if needed.

[0] https://lore.kernel.org/all/20240814235800.15253-3-inwardvessel@gmail.com/