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

Commit Message

JP Kobryn Aug. 26, 2024, 10:48 p.m. UTC

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 <[e-mail]>
---
 kernel/bpf/btf.c | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Alexei Starovoitov Aug. 27, 2024, 9:01 p.m. UTC | #1

On Mon, Aug 26, 2024 at 3:48 PM JP Kobryn <[e-mail]> 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 <[e-mail]>
> ---
>  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?
         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;
?

Andrii Nakryiko Aug. 27, 2024, 10:42 p.m. UTC | #2

On Tue, Aug 27, 2024 at 2:01 PM Alexei Starovoitov
<[e-mail]> wrote:
>
> On Mon, Aug 26, 2024 at 3:48 PM JP Kobryn <[e-mail]> 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 <[e-mail]>
> > ---
> >  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".

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

JP Kobryn Aug. 28, 2024, 7:08 p.m. UTC | #3

On Tue, Aug 27, 2024 at 03:42:34PM -0700, Andrii Nakryiko wrote:
> On Tue, Aug 27, 2024 at 2:01 PM Alexei Starovoitov
> <[e-mail]> wrote:
> >
> > On Mon, Aug 26, 2024 at 3:48 PM JP Kobryn <[e-mail]> 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 <[e-mail]>
> > > ---
> > >  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/[e-mail]/

Alexei Starovoitov Aug. 31, 2024, 2:14 a.m. UTC | #4

On Wed, Aug 28, 2024 at 12:08 PM JP Kobryn <[e-mail]> wrote:
>
> 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.

Correct. kfuncs differentiate by type of their arguments.
prog_type -> helper was an old style when context was the only
thing we had. So we had to differentiate by prog_type.
kfuncs are currently register by prog_type too, but that will go away.
We're planning to do large refactoring in that area to satisfy
sched-ext requests. It's not the prog type that would allow or
disallow certain kfuncs but rather hook point plus custom flags.

Patch

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;
 	case BPF_PROG_TYPE_SYSCALL:
 		return BTF_KFUNC_HOOK_SYSCALL;
 	case BPF_PROG_TYPE_CGROUP_SKB:
@@ -8326,6 +8330,8 @@  static int bpf_prog_type_to_kfunc_hook(enum bpf_prog_type prog_type)
 		return BTF_KFUNC_HOOK_NETFILTER;
 	case BPF_PROG_TYPE_KPROBE:
 		return BTF_KFUNC_HOOK_KPROBE;
+	case BPF_PROG_TYPE_PERF_EVENT:
+		return BTF_KFUNC_HOOK_PERF_EVENT;
 	default:
 		return BTF_KFUNC_HOOK_MAX;
 	}