BPF perf link and user-provided context value

Turn BPF_PROG_RUN into a proper always inlined function. No functional and performance changes are intended, but it makes it much easier to understand what's going on with how BPF programs are actually get executed. It's more obvious what types and callbacks are expected. Also extra () around input parameters can be dropped, as well as __ variable prefixes intended to avoid naming collisions, which makes the code simpler to read and write.

This refactoring also highlighted one possible issue. BPF_PROG_RUN is both a macro and an enum value (BPF_PROG_RUN == BPF_PROG_TEST_RUN). Turning BPF_PROG_RUN into a function causes naming conflict compilation error. So rename BPF_PROG_RUN into lower-case bpf_prog_run(), similar to bpf_prog_run_xdp(), bpf_prog_run_pin_on_cpu(), etc. To avoid unnecessary code churn across many networking calls to BPF_PROG_RUN, #define BPF_PROG_RUN as an alias to bpf_prog_run.

Signed-off-by Andrii Nakryiko <[email protected]>

---
  include/linux/filter.h | 58 +++++++++++++++++++++++++++---------------
  1 file changed, 37 insertions(+), 21 deletions(-)

include/linux/filter.h +35 -19

index ba36989f711a..e59c97c72233 100644
@@ -585,25 +585,41 @@ struct sk_filter {
 
  DECLARE_STATIC_KEY_FALSE(bpf_stats_enabled_key);
 
-#define __BPF_PROG_RUN(prog, ctx, dfunc)	({			\
-	u32 __ret;							\
-	cant_migrate();							\
-	if (static_branch_unlikely(&bpf_stats_enabled_key)) {		\
-		struct bpf_prog_stats *__stats;				\
-		u64 __start = sched_clock();				\
-		__ret = dfunc(ctx, (prog)->insnsi, (prog)->bpf_func);	\
-		__stats = this_cpu_ptr(prog->stats);			\
-		u64_stats_update_begin(&__stats->syncp);		\
-		__stats->cnt++;						\
-		__stats->nsecs += sched_clock() - __start;		\
-		u64_stats_update_end(&__stats->syncp);			\
-	} else {							\
-		__ret = dfunc(ctx, (prog)->insnsi, (prog)->bpf_func);	\
-	}								\
-	__ret; })
-
-#define BPF_PROG_RUN(prog, ctx)						\
-	__BPF_PROG_RUN(prog, ctx, bpf_dispatcher_nop_func)
+typedef unsigned int (*bpf_dispatcher_fn)(const void *ctx,
+					  const struct bpf_insn *insnsi,
+					  unsigned int (*bpf_func)(const void *,
+								   const struct bpf_insn *));
+
+static __always_inline u32 __bpf_prog_run(const struct bpf_prog *prog,
+					  const void *ctx,
+					  bpf_dispatcher_fn dfunc)
+{
+	u32 ret;
+
+	cant_migrate();
+	if (static_branch_unlikely(&bpf_stats_enabled_key)) {
+		struct bpf_prog_stats *stats;
+		u64 start = sched_clock();
+
+		ret = dfunc(ctx, prog->insnsi, prog->bpf_func);
+		stats = this_cpu_ptr(prog->stats);
+		u64_stats_update_begin(&stats->syncp);
+		stats->cnt++;
+		stats->nsecs += sched_clock() - start;
+		u64_stats_update_end(&stats->syncp);
+	} else {
+		ret = dfunc(ctx, prog->insnsi, prog->bpf_func);
+	}
+	return ret;
+}
+
+static __always_inline u32 bpf_prog_run(const struct bpf_prog *prog, const void *ctx)
+{
+	return __bpf_prog_run(prog, ctx, bpf_dispatcher_nop_func);
+}
+
+/* avoids name conflict with BPF_PROG_RUN enum definedi uapi/linux/bpf.h */
+#define BPF_PROG_RUN bpf_prog_run
 
  /*
   * Use in preemptible and therefore migratable context to make sure that

include/linux/filter.h +1 -1

  	migrate_enable();
  	return ret;
  }

@@ -768,7 +784,7 @@ static __always_inline u32 bpf_prog_run_xdp(const struct bpf_prog *prog,
  	 * under local_bh_disable(), which provides the needed RCU protection
  	 * for accessing map entries.
  	 */
-	return __BPF_PROG_RUN(prog, xdp, BPF_DISPATCHER_FUNC(xdp));
+	return __bpf_prog_run(prog, xdp, BPF_DISPATCHER_FUNC(xdp));
  }
 
  void bpf_prog_change_xdp(struct bpf_prog *prev_prog, struct bpf_prog *prog);

Quoted 88 lines · Andrii Nakryiko

Turn BPF_PROG_RUN into a proper always inlined function. No functional and performance changes are intended, but it makes it much easier to understand what's going on with how BPF programs are actually get executed. It's more obvious what types and callbacks are expected. Also extra () around input parameters can be dropped, as well as __ variable prefixes intended to avoid naming collisions, which makes the code simpler to read and write.

This refactoring also highlighted one possible issue. BPF_PROG_RUN is both a macro and an enum value (BPF_PROG_RUN == BPF_PROG_TEST_RUN). Turning BPF_PROG_RUN into a function causes naming conflict compilation error. So rename BPF_PROG_RUN into lower-case bpf_prog_run(), similar to bpf_prog_run_xdp(), bpf_prog_run_pin_on_cpu(), etc. To avoid unnecessary code churn across many networking calls to BPF_PROG_RUN, #define BPF_PROG_RUN as an alias to bpf_prog_run.

Signed-off-by Andrii Nakryiko <[email protected]>

---
  include/linux/filter.h | 58 +++++++++++++++++++++++++++---------------
  1 file changed, 37 insertions(+), 21 deletions(-)

include/linux/filter.h +35 -19

index ba36989f711a..e59c97c72233 100644
@@ -585,25 +585,41 @@ struct sk_filter {
 
  DECLARE_STATIC_KEY_FALSE(bpf_stats_enabled_key);
 
-#define __BPF_PROG_RUN(prog, ctx, dfunc)     ({                      \
-     u32 __ret;                                                      \
-     cant_migrate();                                                 \
-     if (static_branch_unlikely(&bpf_stats_enabled_key)) {           \
-             struct bpf_prog_stats *__stats;                         \
-             u64 __start = sched_clock();                            \
-             __ret = dfunc(ctx, (prog)->insnsi, (prog)->bpf_func);   \
-             __stats = this_cpu_ptr(prog->stats);                    \
-             u64_stats_update_begin(&__stats->syncp);                \
-             __stats->cnt++;                                         \
-             __stats->nsecs += sched_clock() - __start;              \
-             u64_stats_update_end(&__stats->syncp);                  \
-     } else {                                                        \
-             __ret = dfunc(ctx, (prog)->insnsi, (prog)->bpf_func);   \
-     }                                                               \
-     __ret; })
-
-#define BPF_PROG_RUN(prog, ctx)                                              \
-     __BPF_PROG_RUN(prog, ctx, bpf_dispatcher_nop_func)
+typedef unsigned int (*bpf_dispatcher_fn)(const void *ctx,
+                                       const struct bpf_insn *insnsi,
+                                       unsigned int (*bpf_func)(const void *,
+                                                                const struct bpf_insn *));
+
+static __always_inline u32 __bpf_prog_run(const struct bpf_prog *prog,
+                                       const void *ctx,
+                                       bpf_dispatcher_fn dfunc)
+{
+     u32 ret;
+
+     cant_migrate();
+     if (static_branch_unlikely(&bpf_stats_enabled_key)) {
+             struct bpf_prog_stats *stats;
+             u64 start = sched_clock();
+
+             ret = dfunc(ctx, prog->insnsi, prog->bpf_func);
+             stats = this_cpu_ptr(prog->stats);
+             u64_stats_update_begin(&stats->syncp);
+             stats->cnt++;
+             stats->nsecs += sched_clock() - start;
+             u64_stats_update_end(&stats->syncp);
+     } else {
+             ret = dfunc(ctx, prog->insnsi, prog->bpf_func);
+     }
+     return ret;
+}
+
+static __always_inline u32 bpf_prog_run(const struct bpf_prog *prog, const void *ctx)
+{
+     return __bpf_prog_run(prog, ctx, bpf_dispatcher_nop_func);
+}
+
+/* avoids name conflict with BPF_PROG_RUN enum definedi uapi/linux/bpf.h */
+#define BPF_PROG_RUN bpf_prog_run
 
  /*
   * Use in preemptible and therefore migratable context to make sure that

@@ -622,7 +638,7 @@ static inline u32 bpf_prog_run_pin_on_cpu(const struct bpf_prog *prog,
      u32 ret;
 

include/linux/filter.h static inline u32 bpf_prog_run_pin_on_cpu(const struct bpf_prog *prog, · line 641

Quoted 3 lines

include/linux/filter.h +1 -1

      migrate_disable();
-     ret = __BPF_PROG_RUN(prog, ctx, bpf_dispatcher_nop_func);
+     ret = __bpf_prog_run(prog, ctx, bpf_dispatcher_nop_func);

This can be replaced with bpf_prog_run(prog, ctx).

Quoted 13 lines

include/linux/filter.h +1 -1

      migrate_enable();
      return ret;
  }

@@ -768,7 +784,7 @@ static __always_inline u32 bpf_prog_run_xdp(const struct bpf_prog *prog,
       * under local_bh_disable(), which provides the needed RCU protection
       * for accessing map entries.
       */
-     return __BPF_PROG_RUN(prog, xdp, BPF_DISPATCHER_FUNC(xdp));
+     return __bpf_prog_run(prog, xdp, BPF_DISPATCHER_FUNC(xdp));
  }
 
  void bpf_prog_change_xdp(struct bpf_prog *prev_prog, struct bpf_prog *prog);

Similar to BPF_PROG_RUN, turn BPF_PROG_RUN_ARRAY macros into proper functions with all the same readability and maintainability benefits. Making them into functions required shuffling around bpf_set_run_ctx/bpf_reset_run_ctx functions. Also, explicitly specifying the type of the BPF prog run callback required adjusting __bpf_prog_run_save_cb() to accept const void *, casted internally to const struct sk_buff.

Further, split out a cgroup-specific BPF_PROG_RUN_ARRAY_CG and BPF_PROG_RUN_ARRAY_CG_FLAGS from the more generic BPF_PROG_RUN_ARRAY due to the differences in bpf_run_ctx used for those two different use cases.

I think BPF_PROG_RUN_ARRAY_CG would benefit from further refactoring to accept struct cgroup and enum bpf_attach_type instead of bpf_prog_array, fetching cgrp->bpf.effective[type] and RCU-dereferencing it internally. But that required including include/linux/cgroup-defs.h, which I wasn't sure is ok with everyone.

The remaining generic BPF_PROG_RUN_ARRAY function will be extended to pass-through user-provided context value in the next patch.

Signed-off-by Andrii Nakryiko <[email protected]>

Make internal perf_event_set_bpf_prog() use struct bpf_prog pointer as an input argument, which makes it easier to re-use for other internal uses (coming up for BPF link in the next patch). BPF program FD is not as convenient and in some cases it's not available. So switch to struct bpf_prog, move out refcounting outside and let caller do bpf_prog_put() in case of an error. This follows the approach of most of the other BPF internal functions.

Cc Peter Zijlstra <[email protected]>

Signed-off-by Andrii Nakryiko <[email protected]>

Make internal perf_event_set_bpf_prog() use struct bpf_prog pointer as an input argument, which makes it easier to re-use for other internal uses (coming up for BPF link in the next patch). BPF program FD is not as convenient and in some cases it's not available. So switch to struct bpf_prog, move out refcounting outside and let caller do bpf_prog_put() in case of an error. This follows the approach of most of the other BPF internal functions.

Cc Peter Zijlstra <[email protected]>

Signed-off-by Andrii Nakryiko <[email protected]>

Introduce a new type of BPF link - BPF perf link. This brings perf_event-based BPF program attachments (perf_event, tracepoints, kprobes, and uprobes) into the common BPF link infrastructure, allowing to list all active perf_event based attachments, auto-detaching BPF program from perf_event when link's FD is closed, get generic BPF link fdinfo/get_info functionality.

BPF_LINK_CREATE command expects perf_event's FD as target_fd. No extra flags are currently supported.

Force-detaching and atomic BPF program updates are not yet implemented, but with perf_event-based BPF links we now have common framework for this without the need to extend ioctl()-based perf_event interface.

One interesting consideration is a new value for bpf_attach_type, which BPF_LINK_CREATE command expects. Generally, it's either 1-to-1 mapping from bpf_attach_type to bpf_prog_type, or many-to-1 mapping from a subset of bpf_attach_types to one bpf_prog_type (e.g., see BPF_PROG_TYPE_SK_SKB or BPF_PROG_TYPE_CGROUP_SOCK). In this case, though, we have three different program types (KPROBE, TRACEPOINT, PERF_EVENT) using the same perf_event-based mechanism, so it's many bpf_prog_types to one bpf_attach_type. I chose to define a single BPF_PERF_EVENT attach type for all of them and adjust link_create()'s logic for checking correspondence between attach type and program type.

The alternative would be to define three new attach types (e.g., BPF_KPROBE, BPF_TRACEPOINT, and BPF_PERF_EVENT), but that seemed like unnecessary overkill and BPF_KPROBE will cause naming conflicts with BPF_KPROBE() macro, defined by libbpf. I chose to not do this to avoid unnecessary proliferation of bpf_attach_type enum values and not have to deal with naming conflicts.

(quoted diff) +34 -0

+static void bpf_perf_link_release(struct bpf_link *link)
+{
+	struct bpf_perf_link *perf_link = container_of(link, struct bpf_perf_link, link);
+	struct perf_event *event = perf_link->perf_file->private_data;
+
+	perf_event_free_bpf_prog(event);
+	fput(perf_link->perf_file);
+}
 
+static int bpf_perf_link_attach(const union bpf_attr *attr, struct bpf_prog *prog)
+{
+	struct bpf_link_primer link_primer;
+	struct bpf_perf_link *link;
+	struct perf_event *event;
+	struct file *perf_file;
+	int err;
+
+	if (attr->link_create.flags)
+		return -EINVAL;
+
+	perf_file = perf_event_get(attr->link_create.target_fd);
+	if (IS_ERR(perf_file))
+		return PTR_ERR(perf_file);
+
+	link = kzalloc(sizeof(*link), GFP_USER);
+	if (!link) {
+		err = -ENOMEM;
+		goto out_put_file;
+	}
+	bpf_link_init(&link->link, BPF_LINK_TYPE_PERF_EVENT, &bpf_perf_link_lops, prog);
+	link->perf_file = perf_file;
+
+	err = bpf_link_prime(&link->link, &link_primer);
+	if (err) {
+		kfree(link);

kernel/bpf/syscall.c +8 -0

+	bpf_prog_inc(prog);
+
+	return bpf_link_settle(&link_primer);
+
+out_put_file:
+	fput(perf_file);
+	return err;
+}

Quoted 24 lines · Andrii Nakryiko

Introduce a new type of BPF link - BPF perf link. This brings perf_event-based BPF program attachments (perf_event, tracepoints, kprobes, and uprobes) into the common BPF link infrastructure, allowing to list all active perf_event based attachments, auto-detaching BPF program from perf_event when link's FD is closed, get generic BPF link fdinfo/get_info functionality.

BPF_LINK_CREATE command expects perf_event's FD as target_fd. No extra flags are currently supported.

Force-detaching and atomic BPF program updates are not yet implemented, but with perf_event-based BPF links we now have common framework for this without the need to extend ioctl()-based perf_event interface.

One interesting consideration is a new value for bpf_attach_type, which BPF_LINK_CREATE command expects. Generally, it's either 1-to-1 mapping from bpf_attach_type to bpf_prog_type, or many-to-1 mapping from a subset of bpf_attach_types to one bpf_prog_type (e.g., see BPF_PROG_TYPE_SK_SKB or BPF_PROG_TYPE_CGROUP_SOCK). In this case, though, we have three different program types (KPROBE, TRACEPOINT, PERF_EVENT) using the same perf_event-based mechanism, so it's many bpf_prog_types to one bpf_attach_type. I chose to define a single BPF_PERF_EVENT attach type for all of them and adjust link_create()'s logic for checking correspondence between attach type and program type.

Quoted 6 lines

The alternative would be to define three new attach types (e.g., BPF_KPROBE, BPF_TRACEPOINT, and BPF_PERF_EVENT), but that seemed like unnecessary overkill and BPF_KPROBE will cause naming conflicts with BPF_KPROBE() macro, defined by libbpf. I chose to not do this to avoid unnecessary proliferation of bpf_attach_type enum values and not have to deal with naming conflicts.

So I have no idea what all that means... I don't speak BPF. That said, the patch doesn't look terrible.

One little question below, but otherwise:

Acked-by Peter Zijlstra (Intel) <[email protected]>

Quoted 42 lines

(quoted diff) +41 -0

+static void bpf_perf_link_release(struct bpf_link *link)
+{
+     struct bpf_perf_link *perf_link = container_of(link, struct bpf_perf_link, link);
+     struct perf_event *event = perf_link->perf_file->private_data;
+
+     perf_event_free_bpf_prog(event);
+     fput(perf_link->perf_file);
+}
 
+static int bpf_perf_link_attach(const union bpf_attr *attr, struct bpf_prog *prog)
+{
+     struct bpf_link_primer link_primer;
+     struct bpf_perf_link *link;
+     struct perf_event *event;
+     struct file *perf_file;
+     int err;
+
+     if (attr->link_create.flags)
+             return -EINVAL;
+
+     perf_file = perf_event_get(attr->link_create.target_fd);
+     if (IS_ERR(perf_file))
+             return PTR_ERR(perf_file);
+
+     link = kzalloc(sizeof(*link), GFP_USER);
+     if (!link) {
+             err = -ENOMEM;
+             goto out_put_file;
+     }
+     bpf_link_init(&link->link, BPF_LINK_TYPE_PERF_EVENT, &bpf_perf_link_lops, prog);
+     link->perf_file = perf_file;
+
+     err = bpf_link_prime(&link->link, &link_primer);
+     if (err) {
+             kfree(link);
+             goto out_put_file;
+     }
+
+     event = perf_file->private_data;
+     err = perf_event_set_bpf_prog(event, prog);
+     if (err) {
+             bpf_link_cleanup(&link_primer);

kernel/bpf/syscall.c static const struct bpf_link_ops bpf_raw_tp_link_lops = { · line 2971

Quoted 3 lines

kernel/bpf/syscall.c +3 -0

+             goto out_put_file;
+     }
+     /* perf_event_set_bpf_prog() doesn't take its own refcnt on prog */

Is that otherwise expected? AFAICT the previous users of that function were guaranteed the existance of the BPF program. But afaict there is nothing that prevents perf_event_*_bpf_prog() from doing the addition refcounting if that is more convenient.

Quoted 9 lines

kernel/bpf/syscall.c +8 -0

+     bpf_prog_inc(prog);
+
+     return bpf_link_settle(&link_primer);
+
+out_put_file:
+     fput(perf_file);
+     return err;
+}

include/linux/trace_events.h +0 -0

index ad413b382a3c..8ac92560d3a3 100644

Quoted 8 lines · Andrii Nakryiko

include/linux/trace_events.h +0 -0

index ad413b382a3c..8ac92560d3a3 100644
@@ -803,6 +803,9 @@ extern void ftrace_profile_free_filter(struct perf_event *event);
 void perf_trace_buf_update(void *record, u16 type);
 void *perf_trace_buf_alloc(int size, struct pt_regs **regs, int *rctxp);
 

include/linux/trace_events.h extern void ftrace_profile_free_filter(struct perf_event *event); · line 811

Quoted 6 lines

include/linux/trace_events.h +3 -0

+int perf_event_set_bpf_prog(struct perf_event *event, struct bpf_prog *prog);
+void perf_event_free_bpf_prog(struct perf_event *event);
+
 void bpf_trace_run1(struct bpf_prog *prog, u64 arg1);
 void bpf_trace_run2(struct bpf_prog *prog, u64 arg1, u64 arg2);
 void bpf_trace_run3(struct bpf_prog *prog, u64 arg1, u64 arg2,

Oh, I just noticed, is this the right header to put these in? Should this not go into include/linux/perf_event.h ?

Introduce a new type of BPF link - BPF perf link. This brings perf_event-based BPF program attachments (perf_event, tracepoints, kprobes, and uprobes) into the common BPF link infrastructure, allowing to list all active perf_event based attachments, auto-detaching BPF program from perf_event when link's FD is closed, get generic BPF link fdinfo/get_info functionality.

BPF_LINK_CREATE command expects perf_event's FD as target_fd. No extra flags are currently supported.

Force-detaching and atomic BPF program updates are not yet implemented, but with perf_event-based BPF links we now have common framework for this without the need to extend ioctl()-based perf_event interface.

One interesting consideration is a new value for bpf_attach_type, which BPF_LINK_CREATE command expects. Generally, it's either 1-to-1 mapping from bpf_attach_type to bpf_prog_type, or many-to-1 mapping from a subset of bpf_attach_types to one bpf_prog_type (e.g., see BPF_PROG_TYPE_SK_SKB or BPF_PROG_TYPE_CGROUP_SOCK). In this case, though, we have three different program types (KPROBE, TRACEPOINT, PERF_EVENT) using the same perf_event-based mechanism, so it's many bpf_prog_types to one bpf_attach_type. I chose to define a single BPF_PERF_EVENT attach type for all of them and adjust link_create()'s logic for checking correspondence between attach type and program type.

The alternative would be to define three new attach types (e.g., BPF_KPROBE, BPF_TRACEPOINT, and BPF_PERF_EVENT), but that seemed like unnecessary overkill and BPF_KPROBE will cause naming conflicts with BPF_KPROBE() macro, defined by libbpf. I chose to not do this to avoid unnecessary proliferation of bpf_attach_type enum values and not have to deal with naming conflicts.

Cc Peter Zijlstra <[email protected]>

Signed-off-by Andrii Nakryiko <[email protected]>

---
 include/linux/bpf_types.h      |   3 +
 include/linux/trace_events.h   |   3 +
 include/uapi/linux/bpf.h       |   2 +
 kernel/bpf/syscall.c           | 105 ++++++++++++++++++++++++++++++---
 kernel/events/core.c           |  10 ++--
 tools/include/uapi/linux/bpf.h |   2 +
 6 files changed, 112 insertions(+), 13 deletions(-)

include/linux/bpf_types.h +3 -0

index a9db1eae6796..0a1ada7f174d 100644
@@ -135,3 +135,6 @@ BPF_LINK_TYPE(BPF_LINK_TYPE_ITER, iter)
 #ifdef CONFIG_NET
 BPF_LINK_TYPE(BPF_LINK_TYPE_NETNS, netns)
 #endif
+#ifdef CONFIG_PERF_EVENTS
+BPF_LINK_TYPE(BPF_LINK_TYPE_PERF_EVENT, perf)
+#endif

include/linux/trace_events.h +3 -0

index ad413b382a3c..8ac92560d3a3 100644
@@ -803,6 +803,9 @@ extern void ftrace_profile_free_filter(struct perf_event *event);
 void perf_trace_buf_update(void *record, u16 type);
 void *perf_trace_buf_alloc(int size, struct pt_regs **regs, int *rctxp);
 
+int perf_event_set_bpf_prog(struct perf_event *event, struct bpf_prog *prog);
+void perf_event_free_bpf_prog(struct perf_event *event);
+
 void bpf_trace_run1(struct bpf_prog *prog, u64 arg1);
 void bpf_trace_run2(struct bpf_prog *prog, u64 arg1, u64 arg2);
 void bpf_trace_run3(struct bpf_prog *prog, u64 arg1, u64 arg2,

include/uapi/linux/bpf.h +1 -0

index 2db6925e04f4..00b1267ab4f0 100644
@@ -993,6 +993,7 @@ enum bpf_attach_type {
 	BPF_SK_SKB_VERDICT,
 	BPF_SK_REUSEPORT_SELECT,
 	BPF_SK_REUSEPORT_SELECT_OR_MIGRATE,
+	BPF_PERF_EVENT,
 	__MAX_BPF_ATTACH_TYPE
 };
 

Quoted 78 lines · Andrii Nakryiko

Introduce a new type of BPF link - BPF perf link. This brings perf_event-based BPF program attachments (perf_event, tracepoints, kprobes, and uprobes) into the common BPF link infrastructure, allowing to list all active perf_event based attachments, auto-detaching BPF program from perf_event when link's FD is closed, get generic BPF link fdinfo/get_info functionality.

BPF_LINK_CREATE command expects perf_event's FD as target_fd. No extra flags are currently supported.

Force-detaching and atomic BPF program updates are not yet implemented, but with perf_event-based BPF links we now have common framework for this without the need to extend ioctl()-based perf_event interface.

One interesting consideration is a new value for bpf_attach_type, which BPF_LINK_CREATE command expects. Generally, it's either 1-to-1 mapping from bpf_attach_type to bpf_prog_type, or many-to-1 mapping from a subset of bpf_attach_types to one bpf_prog_type (e.g., see BPF_PROG_TYPE_SK_SKB or BPF_PROG_TYPE_CGROUP_SOCK). In this case, though, we have three different program types (KPROBE, TRACEPOINT, PERF_EVENT) using the same perf_event-based mechanism, so it's many bpf_prog_types to one bpf_attach_type. I chose to define a single BPF_PERF_EVENT attach type for all of them and adjust link_create()'s logic for checking correspondence between attach type and program type.

The alternative would be to define three new attach types (e.g., BPF_KPROBE, BPF_TRACEPOINT, and BPF_PERF_EVENT), but that seemed like unnecessary overkill and BPF_KPROBE will cause naming conflicts with BPF_KPROBE() macro, defined by libbpf. I chose to not do this to avoid unnecessary proliferation of bpf_attach_type enum values and not have to deal with naming conflicts.

Cc Peter Zijlstra <[email protected]>

Signed-off-by Andrii Nakryiko <[email protected]>

---
 include/linux/bpf_types.h      |   3 +
 include/linux/trace_events.h   |   3 +
 include/uapi/linux/bpf.h       |   2 +
 kernel/bpf/syscall.c           | 105 ++++++++++++++++++++++++++++++---
 kernel/events/core.c           |  10 ++--
 tools/include/uapi/linux/bpf.h |   2 +
 6 files changed, 112 insertions(+), 13 deletions(-)

include/linux/bpf_types.h +3 -0

index a9db1eae6796..0a1ada7f174d 100644
@@ -135,3 +135,6 @@ BPF_LINK_TYPE(BPF_LINK_TYPE_ITER, iter)
 #ifdef CONFIG_NET
 BPF_LINK_TYPE(BPF_LINK_TYPE_NETNS, netns)
 #endif
+#ifdef CONFIG_PERF_EVENTS
+BPF_LINK_TYPE(BPF_LINK_TYPE_PERF_EVENT, perf)
+#endif

include/linux/trace_events.h +3 -0

index ad413b382a3c..8ac92560d3a3 100644
@@ -803,6 +803,9 @@ extern void ftrace_profile_free_filter(struct perf_event *event);
 void perf_trace_buf_update(void *record, u16 type);
 void *perf_trace_buf_alloc(int size, struct pt_regs **regs, int *rctxp);
 
+int perf_event_set_bpf_prog(struct perf_event *event, struct bpf_prog *prog);
+void perf_event_free_bpf_prog(struct perf_event *event);
+
 void bpf_trace_run1(struct bpf_prog *prog, u64 arg1);
 void bpf_trace_run2(struct bpf_prog *prog, u64 arg1, u64 arg2);
 void bpf_trace_run3(struct bpf_prog *prog, u64 arg1, u64 arg2,

include/uapi/linux/bpf.h +1 -0

index 2db6925e04f4..00b1267ab4f0 100644
@@ -993,6 +993,7 @@ enum bpf_attach_type {
      BPF_SK_SKB_VERDICT,
      BPF_SK_REUSEPORT_SELECT,
      BPF_SK_REUSEPORT_SELECT_OR_MIGRATE,
+     BPF_PERF_EVENT,
      __MAX_BPF_ATTACH_TYPE
 };
 

include/uapi/linux/bpf.h enum bpf_link_type { · line 1010

Quoted 5 lines

include/uapi/linux/bpf.h +1 -0

@@ -1006,6 +1007,7 @@ enum bpf_link_type {
      BPF_LINK_TYPE_ITER = 4,
      BPF_LINK_TYPE_NETNS = 5,
      BPF_LINK_TYPE_XDP = 6,
+     BPF_LINK_TYPE_PERF_EVENT = 6,

hi, should be 7

jirka

Introduce a new type of BPF link - BPF perf link. This brings perf_event-based BPF program attachments (perf_event, tracepoints, kprobes, and uprobes) into the common BPF link infrastructure, allowing to list all active perf_event based attachments, auto-detaching BPF program from perf_event when link's FD is closed, get generic BPF link fdinfo/get_info functionality.

BPF_LINK_CREATE command expects perf_event's FD as target_fd. No extra flags are currently supported.

Force-detaching and atomic BPF program updates are not yet implemented, but with perf_event-based BPF links we now have common framework for this without the need to extend ioctl()-based perf_event interface.

One interesting consideration is a new value for bpf_attach_type, which BPF_LINK_CREATE command expects. Generally, it's either 1-to-1 mapping from bpf_attach_type to bpf_prog_type, or many-to-1 mapping from a subset of bpf_attach_types to one bpf_prog_type (e.g., see BPF_PROG_TYPE_SK_SKB or BPF_PROG_TYPE_CGROUP_SOCK). In this case, though, we have three different program types (KPROBE, TRACEPOINT, PERF_EVENT) using the same perf_event-based mechanism, so it's many bpf_prog_types to one bpf_attach_type. I chose to define a single BPF_PERF_EVENT attach type for all of them and adjust link_create()'s logic for checking correspondence between attach type and program type.

The alternative would be to define three new attach types (e.g., BPF_KPROBE, BPF_TRACEPOINT, and BPF_PERF_EVENT), but that seemed like unnecessary overkill and BPF_KPROBE will cause naming conflicts with BPF_KPROBE() macro, defined by libbpf. I chose to not do this to avoid unnecessary proliferation of bpf_attach_type enum values and not have to deal with naming conflicts.

Cc Peter Zijlstra <[email protected]>

Signed-off-by Andrii Nakryiko <[email protected]>

---
  include/linux/bpf_types.h      |   3 +
  include/linux/trace_events.h   |   3 +
  include/uapi/linux/bpf.h       |   2 +
  kernel/bpf/syscall.c           | 105 ++++++++++++++++++++++++++++++---
  kernel/events/core.c           |  10 ++--
  tools/include/uapi/linux/bpf.h |   2 +
  6 files changed, 112 insertions(+), 13 deletions(-)

include/linux/bpf_types.h +3 -0

index a9db1eae6796..0a1ada7f174d 100644
@@ -135,3 +135,6 @@ BPF_LINK_TYPE(BPF_LINK_TYPE_ITER, iter)
  #ifdef CONFIG_NET
  BPF_LINK_TYPE(BPF_LINK_TYPE_NETNS, netns)
  #endif
+#ifdef CONFIG_PERF_EVENTS
+BPF_LINK_TYPE(BPF_LINK_TYPE_PERF_EVENT, perf)
+#endif

include/linux/trace_events.h +3 -0

index ad413b382a3c..8ac92560d3a3 100644
@@ -803,6 +803,9 @@ extern void ftrace_profile_free_filter(struct perf_event *event);
  void perf_trace_buf_update(void *record, u16 type);
  void *perf_trace_buf_alloc(int size, struct pt_regs **regs, int *rctxp);
 
+int perf_event_set_bpf_prog(struct perf_event *event, struct bpf_prog *prog);
+void perf_event_free_bpf_prog(struct perf_event *event);
+
  void bpf_trace_run1(struct bpf_prog *prog, u64 arg1);
  void bpf_trace_run2(struct bpf_prog *prog, u64 arg1, u64 arg2);
  void bpf_trace_run3(struct bpf_prog *prog, u64 arg1, u64 arg2,

include/uapi/linux/bpf.h +1 -0

index 2db6925e04f4..00b1267ab4f0 100644
@@ -993,6 +993,7 @@ enum bpf_attach_type {
  	BPF_SK_SKB_VERDICT,
  	BPF_SK_REUSEPORT_SELECT,
  	BPF_SK_REUSEPORT_SELECT_OR_MIGRATE,
+	BPF_PERF_EVENT,
  	__MAX_BPF_ATTACH_TYPE
  };
 

include/uapi/linux/bpf.h +0 -0

  	MAX_BPF_LINK_TYPE,
  };

kernel/bpf/syscall.c +33 -0

index 9a2068e39d23..80c03bedd6e6 100644
@@ -2906,6 +2906,79 @@ static const struct bpf_link_ops bpf_raw_tp_link_lops = {
  	.fill_link_info = bpf_raw_tp_link_fill_link_info,
  };
 
+#ifdef CONFIG_PERF_EVENTS
+struct bpf_perf_link {
+	struct bpf_link link;
+	struct file *perf_file;
+};
+
+static void bpf_perf_link_release(struct bpf_link *link)
+{
+	struct bpf_perf_link *perf_link = container_of(link, struct bpf_perf_link, link);
+	struct perf_event *event = perf_link->perf_file->private_data;
+
+	perf_event_free_bpf_prog(event);
+	fput(perf_link->perf_file);
+}
+
+static void bpf_perf_link_dealloc(struct bpf_link *link)
+{
+	struct bpf_perf_link *perf_link = container_of(link, struct bpf_perf_link, link);
+
+	kfree(perf_link);
+}
+
+static const struct bpf_link_ops bpf_perf_link_lops = {
+	.release = bpf_perf_link_release,
+	.dealloc = bpf_perf_link_dealloc,
+};
+
+static int bpf_perf_link_attach(const union bpf_attr *attr, struct bpf_prog *prog)
+{
+	struct bpf_link_primer link_primer;
+	struct bpf_perf_link *link;
+	struct perf_event *event;
+	struct file *perf_file;

kernel/bpf/syscall.c +7 -0

+	if (!link) {
+		err = -ENOMEM;
+		goto out_put_file;
+	}
+	bpf_link_init(&link->link, BPF_LINK_TYPE_PERF_EVENT, &bpf_perf_link_lops, prog);
+	link->perf_file = perf_file;
+

kernel/bpf/syscall.c +13 -0

+		goto out_put_file;
+	}
+	/* perf_event_set_bpf_prog() doesn't take its own refcnt on prog */
+	bpf_prog_inc(prog);
+
+	return bpf_link_settle(&link_primer);
+
+out_put_file:
+	fput(perf_file);
+	return err;
+}
+#endif /* CONFIG_PERF_EVENTS */
+
  #define BPF_RAW_TRACEPOINT_OPEN_LAST_FIELD raw_tracepoint.prog_fd
 

Quoted 78 lines · Andrii Nakryiko

Introduce a new type of BPF link - BPF perf link. This brings perf_event-based BPF program attachments (perf_event, tracepoints, kprobes, and uprobes) into the common BPF link infrastructure, allowing to list all active perf_event based attachments, auto-detaching BPF program from perf_event when link's FD is closed, get generic BPF link fdinfo/get_info functionality.

BPF_LINK_CREATE command expects perf_event's FD as target_fd. No extra flags are currently supported.

Force-detaching and atomic BPF program updates are not yet implemented, but with perf_event-based BPF links we now have common framework for this without the need to extend ioctl()-based perf_event interface.

One interesting consideration is a new value for bpf_attach_type, which BPF_LINK_CREATE command expects. Generally, it's either 1-to-1 mapping from bpf_attach_type to bpf_prog_type, or many-to-1 mapping from a subset of bpf_attach_types to one bpf_prog_type (e.g., see BPF_PROG_TYPE_SK_SKB or BPF_PROG_TYPE_CGROUP_SOCK). In this case, though, we have three different program types (KPROBE, TRACEPOINT, PERF_EVENT) using the same perf_event-based mechanism, so it's many bpf_prog_types to one bpf_attach_type. I chose to define a single BPF_PERF_EVENT attach type for all of them and adjust link_create()'s logic for checking correspondence between attach type and program type.

The alternative would be to define three new attach types (e.g., BPF_KPROBE, BPF_TRACEPOINT, and BPF_PERF_EVENT), but that seemed like unnecessary overkill and BPF_KPROBE will cause naming conflicts with BPF_KPROBE() macro, defined by libbpf. I chose to not do this to avoid unnecessary proliferation of bpf_attach_type enum values and not have to deal with naming conflicts.

Cc Peter Zijlstra <[email protected]>

Signed-off-by Andrii Nakryiko <[email protected]>

---
  include/linux/bpf_types.h      |   3 +
  include/linux/trace_events.h   |   3 +
  include/uapi/linux/bpf.h       |   2 +
  kernel/bpf/syscall.c           | 105 ++++++++++++++++++++++++++++++---
  kernel/events/core.c           |  10 ++--
  tools/include/uapi/linux/bpf.h |   2 +
  6 files changed, 112 insertions(+), 13 deletions(-)

include/linux/bpf_types.h +3 -0

index a9db1eae6796..0a1ada7f174d 100644
@@ -135,3 +135,6 @@ BPF_LINK_TYPE(BPF_LINK_TYPE_ITER, iter)
  #ifdef CONFIG_NET
  BPF_LINK_TYPE(BPF_LINK_TYPE_NETNS, netns)
  #endif
+#ifdef CONFIG_PERF_EVENTS
+BPF_LINK_TYPE(BPF_LINK_TYPE_PERF_EVENT, perf)
+#endif

include/linux/trace_events.h +3 -0

index ad413b382a3c..8ac92560d3a3 100644
@@ -803,6 +803,9 @@ extern void ftrace_profile_free_filter(struct perf_event *event);
  void perf_trace_buf_update(void *record, u16 type);
  void *perf_trace_buf_alloc(int size, struct pt_regs **regs, int *rctxp);
 
+int perf_event_set_bpf_prog(struct perf_event *event, struct bpf_prog *prog);
+void perf_event_free_bpf_prog(struct perf_event *event);
+
  void bpf_trace_run1(struct bpf_prog *prog, u64 arg1);
  void bpf_trace_run2(struct bpf_prog *prog, u64 arg1, u64 arg2);
  void bpf_trace_run3(struct bpf_prog *prog, u64 arg1, u64 arg2,

include/uapi/linux/bpf.h +1 -0

index 2db6925e04f4..00b1267ab4f0 100644
@@ -993,6 +993,7 @@ enum bpf_attach_type {
      BPF_SK_SKB_VERDICT,
      BPF_SK_REUSEPORT_SELECT,
      BPF_SK_REUSEPORT_SELECT_OR_MIGRATE,
+     BPF_PERF_EVENT,
      __MAX_BPF_ATTACH_TYPE
  };
 

include/uapi/linux/bpf.h enum bpf_link_type { · line 1010

Quoted 5 lines

include/uapi/linux/bpf.h +1 -0

@@ -1006,6 +1007,7 @@ enum bpf_link_type {
      BPF_LINK_TYPE_ITER = 4,
      BPF_LINK_TYPE_NETNS = 5,
      BPF_LINK_TYPE_XDP = 6,
+     BPF_LINK_TYPE_PERF_EVENT = 6,

As Jiri has pointed out, BPF_LINK_TYPE_PERF_EVENT = 7.

Quoted 49 lines

include/uapi/linux/bpf.h +0 -0

      MAX_BPF_LINK_TYPE,
  };

kernel/bpf/syscall.c +37 -0

index 9a2068e39d23..80c03bedd6e6 100644
@@ -2906,6 +2906,79 @@ static const struct bpf_link_ops bpf_raw_tp_link_lops = {
      .fill_link_info = bpf_raw_tp_link_fill_link_info,
  };
 
+#ifdef CONFIG_PERF_EVENTS
+struct bpf_perf_link {
+     struct bpf_link link;
+     struct file *perf_file;
+};
+
+static void bpf_perf_link_release(struct bpf_link *link)
+{
+     struct bpf_perf_link *perf_link = container_of(link, struct bpf_perf_link, link);
+     struct perf_event *event = perf_link->perf_file->private_data;
+
+     perf_event_free_bpf_prog(event);
+     fput(perf_link->perf_file);
+}
+
+static void bpf_perf_link_dealloc(struct bpf_link *link)
+{
+     struct bpf_perf_link *perf_link = container_of(link, struct bpf_perf_link, link);
+
+     kfree(perf_link);
+}
+
+static const struct bpf_link_ops bpf_perf_link_lops = {
+     .release = bpf_perf_link_release,
+     .dealloc = bpf_perf_link_dealloc,
+};
+
+static int bpf_perf_link_attach(const union bpf_attr *attr, struct bpf_prog *prog)
+{
+     struct bpf_link_primer link_primer;
+     struct bpf_perf_link *link;
+     struct perf_event *event;
+     struct file *perf_file;
+     int err;
+
+     if (attr->link_create.flags)
+             return -EINVAL;

kernel/bpf/syscall.c static const struct bpf_link_ops bpf_raw_tp_link_lops = { · line 2951

Quoted 6 lines

kernel/bpf/syscall.c +6 -0

+
+     perf_file = perf_event_get(attr->link_create.target_fd);
+     if (IS_ERR(perf_file))
+             return PTR_ERR(perf_file);
+
+     link = kzalloc(sizeof(*link), GFP_USER);

add __GFP_NOWARN flag?

Quoted 12 lines

kernel/bpf/syscall.c +11 -0

+     if (!link) {
+             err = -ENOMEM;
+             goto out_put_file;
+     }
+     bpf_link_init(&link->link, BPF_LINK_TYPE_PERF_EVENT, &bpf_perf_link_lops, prog);
+     link->perf_file = perf_file;
+
+     err = bpf_link_prime(&link->link, &link_primer);
+     if (err) {
+             kfree(link);
+             goto out_put_file;

kernel/bpf/syscall.c static const struct bpf_link_ops bpf_raw_tp_link_lops = { · line 2968

Quoted 6 lines

kernel/bpf/syscall.c +6 -0

+     }
+
+     event = perf_file->private_data;
+     err = perf_event_set_bpf_prog(event, prog);
+     if (err) {
+             bpf_link_cleanup(&link_primer);

Do you need kfree(link) here?

Quoted 16 lines

kernel/bpf/syscall.c +13 -0

+             goto out_put_file;
+     }
+     /* perf_event_set_bpf_prog() doesn't take its own refcnt on prog */
+     bpf_prog_inc(prog);
+
+     return bpf_link_settle(&link_primer);
+
+out_put_file:
+     fput(perf_file);
+     return err;
+}
+#endif /* CONFIG_PERF_EVENTS */
+
  #define BPF_RAW_TRACEPOINT_OPEN_LAST_FIELD raw_tracepoint.prog_fd
 

[...]

Quoted 82 lines · Yonghong Song

Quoted 78 lines · Andrii Nakryiko

Introduce a new type of BPF link - BPF perf link. This brings perf_event-based BPF program attachments (perf_event, tracepoints, kprobes, and uprobes) into the common BPF link infrastructure, allowing to list all active perf_event based attachments, auto-detaching BPF program from perf_event when link's FD is closed, get generic BPF link fdinfo/get_info functionality.

BPF_LINK_CREATE command expects perf_event's FD as target_fd. No extra flags are currently supported.

Force-detaching and atomic BPF program updates are not yet implemented, but with perf_event-based BPF links we now have common framework for this without the need to extend ioctl()-based perf_event interface.

One interesting consideration is a new value for bpf_attach_type, which BPF_LINK_CREATE command expects. Generally, it's either 1-to-1 mapping from bpf_attach_type to bpf_prog_type, or many-to-1 mapping from a subset of bpf_attach_types to one bpf_prog_type (e.g., see BPF_PROG_TYPE_SK_SKB or BPF_PROG_TYPE_CGROUP_SOCK). In this case, though, we have three different program types (KPROBE, TRACEPOINT, PERF_EVENT) using the same perf_event-based mechanism, so it's many bpf_prog_types to one bpf_attach_type. I chose to define a single BPF_PERF_EVENT attach type for all of them and adjust link_create()'s logic for checking correspondence between attach type and program type.

The alternative would be to define three new attach types (e.g., BPF_KPROBE, BPF_TRACEPOINT, and BPF_PERF_EVENT), but that seemed like unnecessary overkill and BPF_KPROBE will cause naming conflicts with BPF_KPROBE() macro, defined by libbpf. I chose to not do this to avoid unnecessary proliferation of bpf_attach_type enum values and not have to deal with naming conflicts.

Cc Peter Zijlstra <[email protected]>

Signed-off-by Andrii Nakryiko <[email protected]>

---
   include/linux/bpf_types.h      |   3 +
   include/linux/trace_events.h   |   3 +
   include/uapi/linux/bpf.h       |   2 +
   kernel/bpf/syscall.c           | 105 ++++++++++++++++++++++++++++++---
   kernel/events/core.c           |  10 ++--
   tools/include/uapi/linux/bpf.h |   2 +
   6 files changed, 112 insertions(+), 13 deletions(-)

include/linux/bpf_types.h +3 -0

index a9db1eae6796..0a1ada7f174d 100644
@@ -135,3 +135,6 @@ BPF_LINK_TYPE(BPF_LINK_TYPE_ITER, iter)
   #ifdef CONFIG_NET
   BPF_LINK_TYPE(BPF_LINK_TYPE_NETNS, netns)
   #endif
+#ifdef CONFIG_PERF_EVENTS
+BPF_LINK_TYPE(BPF_LINK_TYPE_PERF_EVENT, perf)
+#endif

include/linux/trace_events.h +3 -0

index ad413b382a3c..8ac92560d3a3 100644
@@ -803,6 +803,9 @@ extern void ftrace_profile_free_filter(struct perf_event *event);
   void perf_trace_buf_update(void *record, u16 type);
   void *perf_trace_buf_alloc(int size, struct pt_regs **regs, int *rctxp);
 
+int perf_event_set_bpf_prog(struct perf_event *event, struct bpf_prog *prog);
+void perf_event_free_bpf_prog(struct perf_event *event);
+
   void bpf_trace_run1(struct bpf_prog *prog, u64 arg1);
   void bpf_trace_run2(struct bpf_prog *prog, u64 arg1, u64 arg2);
   void bpf_trace_run3(struct bpf_prog *prog, u64 arg1, u64 arg2,

include/uapi/linux/bpf.h +1 -0

index 2db6925e04f4..00b1267ab4f0 100644
@@ -993,6 +993,7 @@ enum bpf_attach_type {
       BPF_SK_SKB_VERDICT,
       BPF_SK_REUSEPORT_SELECT,
       BPF_SK_REUSEPORT_SELECT_OR_MIGRATE,
+     BPF_PERF_EVENT,
       __MAX_BPF_ATTACH_TYPE
   };
 

Quoted 7 lines

include/uapi/linux/bpf.h enum bpf_link_type { · line 1010

Quoted 5 lines

include/uapi/linux/bpf.h +1 -0

@@ -1006,6 +1007,7 @@ enum bpf_link_type {
       BPF_LINK_TYPE_ITER = 4,
       BPF_LINK_TYPE_NETNS = 5,
       BPF_LINK_TYPE_XDP = 6,
+     BPF_LINK_TYPE_PERF_EVENT = 6,

As Jiri has pointed out, BPF_LINK_TYPE_PERF_EVENT = 7.

yep, fixed

Quoted 49 lines

Quoted 49 lines

include/uapi/linux/bpf.h +0 -0

       MAX_BPF_LINK_TYPE,
   };

kernel/bpf/syscall.c +37 -0

index 9a2068e39d23..80c03bedd6e6 100644
@@ -2906,6 +2906,79 @@ static const struct bpf_link_ops bpf_raw_tp_link_lops = {
       .fill_link_info = bpf_raw_tp_link_fill_link_info,
   };
 
+#ifdef CONFIG_PERF_EVENTS
+struct bpf_perf_link {
+     struct bpf_link link;
+     struct file *perf_file;
+};
+
+static void bpf_perf_link_release(struct bpf_link *link)
+{
+     struct bpf_perf_link *perf_link = container_of(link, struct bpf_perf_link, link);
+     struct perf_event *event = perf_link->perf_file->private_data;
+
+     perf_event_free_bpf_prog(event);
+     fput(perf_link->perf_file);
+}
+
+static void bpf_perf_link_dealloc(struct bpf_link *link)
+{
+     struct bpf_perf_link *perf_link = container_of(link, struct bpf_perf_link, link);
+
+     kfree(perf_link);
+}
+
+static const struct bpf_link_ops bpf_perf_link_lops = {
+     .release = bpf_perf_link_release,
+     .dealloc = bpf_perf_link_dealloc,
+};
+
+static int bpf_perf_link_attach(const union bpf_attr *attr, struct bpf_prog *prog)
+{
+     struct bpf_link_primer link_primer;
+     struct bpf_perf_link *link;
+     struct perf_event *event;
+     struct file *perf_file;
+     int err;
+
+     if (attr->link_create.flags)
+             return -EINVAL;

Quoted 8 lines

kernel/bpf/syscall.c static const struct bpf_link_ops bpf_raw_tp_link_lops = { · line 2951

Quoted 6 lines

kernel/bpf/syscall.c +6 -0

+
+     perf_file = perf_event_get(attr->link_create.target_fd);
+     if (IS_ERR(perf_file))
+             return PTR_ERR(perf_file);
+
+     link = kzalloc(sizeof(*link), GFP_USER);

add __GFP_NOWARN flag?

I looked at few other bpf_link_alloc places in this file, they don't use NOWARN flag. I think the idea with NOWARN flag is to avoid memory alloc warnings when amount of allocated memory depends on user-specified parameter (like the size of the map value). In this case it's just a single fixed-size kernel object, so while users can create lots of them, each is fixed in size. It's similar as any other kernel object (e.g., struct file). So I think it's good as is.

Quoted 13 lines

Quoted 13 lines

kernel/bpf/syscall.c +11 -0

+     if (!link) {
+             err = -ENOMEM;
+             goto out_put_file;
+     }
+     bpf_link_init(&link->link, BPF_LINK_TYPE_PERF_EVENT, &bpf_perf_link_lops, prog);
+     link->perf_file = perf_file;
+
+     err = bpf_link_prime(&link->link, &link_primer);
+     if (err) {
+             kfree(link);
+             goto out_put_file;

Quoted 8 lines

kernel/bpf/syscall.c static const struct bpf_link_ops bpf_raw_tp_link_lops = { · line 2968

Quoted 6 lines

kernel/bpf/syscall.c +6 -0

+     }
+
+     event = perf_file->private_data;
+     err = perf_event_set_bpf_prog(event, prog);
+     if (err) {
+             bpf_link_cleanup(&link_primer);

Do you need kfree(link) here?

bpf_link_cleanup() will call kfree() in deferred fashion. This is due to bpf_link_prime() allocating anon_inode file internally, so it needs to be freed carefully and that's what bpf_link_cleanup() is for.

Quoted 18 lines

Quoted 17 lines

kernel/bpf/syscall.c +13 -0

+             goto out_put_file;
+     }
+     /* perf_event_set_bpf_prog() doesn't take its own refcnt on prog */
+     bpf_prog_inc(prog);
+
+     return bpf_link_settle(&link_primer);
+
+out_put_file:
+     fput(perf_file);
+     return err;
+}
+#endif /* CONFIG_PERF_EVENTS */
+
   #define BPF_RAW_TRACEPOINT_OPEN_LAST_FIELD raw_tracepoint.prog_fd
 

[...]

Add ability for users to specify custom u64 value when creating BPF link for perf_event-backed BPF programs (kprobe/uprobe, perf_event, tracepoints).

include/linux/perf_event.h +1 -0

index 2d510ad750ed..97ab46802800 100644
@@ -762,6 +762,7 @@ struct perf_event {
 #ifdef CONFIG_BPF_SYSCALL
 	perf_overflow_handler_t		orig_overflow_handler;
 	struct bpf_prog			*prog;
+	u64				user_ctx;
 #endif
 
 #ifdef CONFIG_EVENT_TRACING

include/linux/trace_events.h +0 -0

index 8ac92560d3a3..4543852f1480 100644

Quoted 2 lines · Andrii Nakryiko

Add ability for users to specify custom u64 value when creating BPF link for perf_event-backed BPF programs (kprobe/uprobe, perf_event, tracepoints).

If I read this right, the value is dependent on the link, not the program. In which case:

Quoted 19 lines

include/linux/perf_event.h +1 -0

index 2d510ad750ed..97ab46802800 100644
@@ -762,6 +762,7 @@ struct perf_event {
 #ifdef CONFIG_BPF_SYSCALL
      perf_overflow_handler_t         orig_overflow_handler;
      struct bpf_prog                 *prog;
+     u64                             user_ctx;
 #endif
 
 #ifdef CONFIG_EVENT_TRACING

include/linux/trace_events.h +0 -0

index 8ac92560d3a3..4543852f1480 100644
@@ -675,7 +675,7 @@ trace_trigger_soft_disabled(struct trace_event_file *file)
 

include/linux/trace_events.h trace_trigger_soft_disabled(struct trace_event_file *file) · line 678

Quoted 4 lines

include/linux/trace_events.h +1 -1

 #ifdef CONFIG_BPF_EVENTS
 unsigned int trace_call_bpf(struct trace_event_call *call, void *ctx);
-int perf_event_attach_bpf_prog(struct perf_event *event, struct bpf_prog *prog);
+int perf_event_attach_bpf_prog(struct perf_event *event, struct bpf_prog *prog, u64 user_ctx);

This API would be misleading, because it is about setting the program.

Quoted 8 lines

include/linux/trace_events.h +0 -0

 void perf_event_detach_bpf_prog(struct perf_event *event);
 int perf_event_query_prog_array(struct perf_event *event, void __user *info);
 int bpf_probe_register(struct bpf_raw_event_map *btp, struct bpf_prog *prog);

@@ -9966,6 +9968,7 @@ static int perf_event_set_bpf_handler(struct perf_event *event, struct bpf_prog
      }
 

include/linux/trace_events.h static int perf_event_set_bpf_handler(struct perf_event *event, struct bpf_prog · line 9974

Quoted 5 lines

include/linux/trace_events.h +1 -0

      event->prog = prog;
+     event->user_ctx = user_ctx;
      event->orig_overflow_handler = READ_ONCE(event->overflow_handler);
      WRITE_ONCE(event->overflow_handler, bpf_overflow_handler);
      return 0;

Also, the name @user_ctx is a bit confusing. Would something like @bpf_cookie or somesuch not be a better name?

Combined would it not make more sense to add something like:

extern int perf_event_set_bpf_cookie(struct perf_event *event, u64 cookie);

Quoted 7 lines · Peter Zijlstra

Quoted 2 lines · Andrii Nakryiko

Add ability for users to specify custom u64 value when creating BPF link for perf_event-backed BPF programs (kprobe/uprobe, perf_event, tracepoints).

If I read this right, the value is dependent on the link, not the program. In which case:

You can see it both ways. BPF link in this (and at least few other cases) is just this invisible orchestrator of BPF program attachment/detachment. The underlying perf_event subsystem doesn't know about the existence of the BPF link at all. In the end, it's actually struct bpf_prog that is added to perf_event or into tp's bpf_prog_array list, and this user-provided value (bpf cookie per below) is associated with that particular attachment. So when we call trace_call_bpf() from tracepoint or kprobe/uprobe, there is no BPF link anywhere, it's just a list of bpf_prog_array_items, with bpf_prog pointer and associated user value. Note, exactly the same bpf_prog can be attached to another perf_event with a completely different cookie and that's expected and is fine.

So in short, perf_event just needs to know about attaching/detaching bpf_prog pointer (and this cookie), it doesn't need to know about bpf_link. Everything is handled the same regardless if bpf_link is used to attach or ioctl(PERF_EVENT_IOC_SET_BPF).

Quoted 11 lines

kernel/events/core.c static int perf_event_set_bpf_handler(struct perf_event *event, struct bpf_prog · line 9974

Quoted 8 lines

kernel/events/core.c +1 -0

@@ -9966,6 +9968,7 @@ static int perf_event_set_bpf_handler(struct perf_event *event, struct bpf_prog
      }
 
      event->prog = prog;
+     event->user_ctx = user_ctx;
      event->orig_overflow_handler = READ_ONCE(event->overflow_handler);
      WRITE_ONCE(event->overflow_handler, bpf_overflow_handler);
      return 0;

Also, the name @user_ctx is a bit confusing. Would something like @bpf_cookie or somesuch not be a better name?

I struggled to come up with a good name, user_ctx was the best I could do. But I do like bpf_cookie for this, thank you! I'll switch the terminology in the next revision.

Add ability for users to specify custom u64 value when creating BPF link for perf_event-backed BPF programs (kprobe/uprobe, perf_event, tracepoints).

This is useful for cases when the same BPF program is used for attaching and processing invocation of different tracepoints/kprobes/uprobes in a generic fashion, but such that each invocation is distinguished from each other (e.g., BPF program can look up additional information associated with a specific kernel function without having to rely on function IP lookups). This enables new use cases to be implemented simply and efficiently that previously were possible only through code generation (and thus multiple instances of almost identical BPF program) or compilation at runtime (BCC-style) on target hosts (even more expensive resource-wise). For uprobes it is not even possible in some cases to know function IP before hand (e.g., when attaching to shared library without PID filtering, in which case base load address is not known for a library).

This is done by storing u64 user_ctx in struct bpf_prog_array_item, corresponding to each attached and run BPF program. Given cgroup BPF programs already use 2 8-byte pointers for their needs and cgroup BPF programs don't have (yet?) support for user_ctx, reuse that space through union of cgroup_storage and new user_ctx field.

Make it available to kprobe/tracepoint BPF programs through bpf_trace_run_ctx. This is set by BPF_PROG_RUN_ARRAY, used by kprobe/uprobe/tracepoint BPF program execution code, which luckily is now also split from BPF_PROG_RUN_ARRAY_CG. This run context will be utilized by a new BPF helper giving access to this user context value from inside a BPF program. Generic perf_event BPF programs will access this value from perf_event itself through passed in BPF program context.

Cc Peter Zijlstra <[email protected]>

Signed-off-by Andrii Nakryiko <[email protected]>

---
  drivers/media/rc/bpf-lirc.c    |  4 ++--
  include/linux/bpf.h            | 16 +++++++++++++++-
  include/linux/perf_event.h     |  1 +
  include/linux/trace_events.h   |  6 +++---
  include/uapi/linux/bpf.h       |  7 +++++++
  kernel/bpf/core.c              | 29 ++++++++++++++++++-----------
  kernel/bpf/syscall.c           |  2 +-
  kernel/events/core.c           | 21 ++++++++++++++-------
  kernel/trace/bpf_trace.c       |  8 +++++---
  tools/include/uapi/linux/bpf.h |  7 +++++++
  10 files changed, 73 insertions(+), 28 deletions(-)

drivers/media/rc/bpf-lirc.c +1 -1

index afae0afe3f81..7490494273e4 100644
@@ -160,7 +160,7 @@ static int lirc_bpf_attach(struct rc_dev *rcdev, struct bpf_prog *prog)
  		goto unlock;
  	}
 
-	ret = bpf_prog_array_copy(old_array, NULL, prog, &new_array);
+	ret = bpf_prog_array_copy(old_array, NULL, prog, 0, &new_array);
  	if (ret < 0)
  		goto unlock;
 

  void bpf_trace_run1(struct bpf_prog *prog, u64 arg1);

include/uapi/linux/bpf.h +0 -0

index 00b1267ab4f0..bc1fd54a8f58 100644

include/uapi/linux/bpf.h +0 -0

  		};
  	} link_create;
 

Quoted 58 lines · Andrii Nakryiko

Add ability for users to specify custom u64 value when creating BPF link for perf_event-backed BPF programs (kprobe/uprobe, perf_event, tracepoints).

This is useful for cases when the same BPF program is used for attaching and processing invocation of different tracepoints/kprobes/uprobes in a generic fashion, but such that each invocation is distinguished from each other (e.g., BPF program can look up additional information associated with a specific kernel function without having to rely on function IP lookups). This enables new use cases to be implemented simply and efficiently that previously were possible only through code generation (and thus multiple instances of almost identical BPF program) or compilation at runtime (BCC-style) on target hosts (even more expensive resource-wise). For uprobes it is not even possible in some cases to know function IP before hand (e.g., when attaching to shared library without PID filtering, in which case base load address is not known for a library).

This is done by storing u64 user_ctx in struct bpf_prog_array_item, corresponding to each attached and run BPF program. Given cgroup BPF programs already use 2 8-byte pointers for their needs and cgroup BPF programs don't have (yet?) support for user_ctx, reuse that space through union of cgroup_storage and new user_ctx field.

Make it available to kprobe/tracepoint BPF programs through bpf_trace_run_ctx. This is set by BPF_PROG_RUN_ARRAY, used by kprobe/uprobe/tracepoint BPF program execution code, which luckily is now also split from BPF_PROG_RUN_ARRAY_CG. This run context will be utilized by a new BPF helper giving access to this user context value from inside a BPF program. Generic perf_event BPF programs will access this value from perf_event itself through passed in BPF program context.

Cc Peter Zijlstra <[email protected]>

Signed-off-by Andrii Nakryiko <[email protected]>

---
  drivers/media/rc/bpf-lirc.c    |  4 ++--
  include/linux/bpf.h            | 16 +++++++++++++++-
  include/linux/perf_event.h     |  1 +
  include/linux/trace_events.h   |  6 +++---
  include/uapi/linux/bpf.h       |  7 +++++++
  kernel/bpf/core.c              | 29 ++++++++++++++++++-----------
  kernel/bpf/syscall.c           |  2 +-
  kernel/events/core.c           | 21 ++++++++++++++-------
  kernel/trace/bpf_trace.c       |  8 +++++---
  tools/include/uapi/linux/bpf.h |  7 +++++++
  10 files changed, 73 insertions(+), 28 deletions(-)

drivers/media/rc/bpf-lirc.c +1 -1

index afae0afe3f81..7490494273e4 100644
@@ -160,7 +160,7 @@ static int lirc_bpf_attach(struct rc_dev *rcdev, struct bpf_prog *prog)
              goto unlock;
      }
 
-     ret = bpf_prog_array_copy(old_array, NULL, prog, &new_array);
+     ret = bpf_prog_array_copy(old_array, NULL, prog, 0, &new_array);
      if (ret < 0)
              goto unlock;
 

[...]

Quoted 6 lines

  void bpf_trace_run1(struct bpf_prog *prog, u64 arg1);

include/uapi/linux/bpf.h +0 -0

index 00b1267ab4f0..bc1fd54a8f58 100644

include/uapi/linux/bpf.h union bpf_attr { · line 1457

Quoted 10 lines

include/uapi/linux/bpf.h +7 -0

@@ -1448,6 +1448,13 @@ union bpf_attr {
                              __aligned_u64   iter_info;      /* extra bpf_iter_link_info */
                              __u32           iter_info_len;  /* iter_info length */
                      };
+                     struct {
+                             /* black box user-provided value passed through
+                              * to BPF program at the execution time and
+                              * accessible through bpf_get_user_ctx() BPF helper
+                              */
+                             __u64           user_ctx;
+                     } perf_event;

Is it possible to fold this field into previous union?

union {
__u32 target_btf_id; /* btf_id of
target to attach to */

                         struct {
                                 __aligned_u64   iter_info;      /*
extra bpf_iter_link_info */
                                 __u32           iter_info_len;  /*
iter_info length */
                         };
                 };

Quoted 3 lines

include/uapi/linux/bpf.h +0 -0

              };
      } link_create;
 

[...]

Quoted 85 lines · Yonghong Song

Quoted 58 lines · Andrii Nakryiko

Add ability for users to specify custom u64 value when creating BPF link for perf_event-backed BPF programs (kprobe/uprobe, perf_event, tracepoints).

This is useful for cases when the same BPF program is used for attaching and processing invocation of different tracepoints/kprobes/uprobes in a generic fashion, but such that each invocation is distinguished from each other (e.g., BPF program can look up additional information associated with a specific kernel function without having to rely on function IP lookups). This enables new use cases to be implemented simply and efficiently that previously were possible only through code generation (and thus multiple instances of almost identical BPF program) or compilation at runtime (BCC-style) on target hosts (even more expensive resource-wise). For uprobes it is not even possible in some cases to know function IP before hand (e.g., when attaching to shared library without PID filtering, in which case base load address is not known for a library).

This is done by storing u64 user_ctx in struct bpf_prog_array_item, corresponding to each attached and run BPF program. Given cgroup BPF programs already use 2 8-byte pointers for their needs and cgroup BPF programs don't have (yet?) support for user_ctx, reuse that space through union of cgroup_storage and new user_ctx field.

Make it available to kprobe/tracepoint BPF programs through bpf_trace_run_ctx. This is set by BPF_PROG_RUN_ARRAY, used by kprobe/uprobe/tracepoint BPF program execution code, which luckily is now also split from BPF_PROG_RUN_ARRAY_CG. This run context will be utilized by a new BPF helper giving access to this user context value from inside a BPF program. Generic perf_event BPF programs will access this value from perf_event itself through passed in BPF program context.

Cc Peter Zijlstra <[email protected]>

Signed-off-by Andrii Nakryiko <[email protected]>

---
   drivers/media/rc/bpf-lirc.c    |  4 ++--
   include/linux/bpf.h            | 16 +++++++++++++++-
   include/linux/perf_event.h     |  1 +
   include/linux/trace_events.h   |  6 +++---
   include/uapi/linux/bpf.h       |  7 +++++++
   kernel/bpf/core.c              | 29 ++++++++++++++++++-----------
   kernel/bpf/syscall.c           |  2 +-
   kernel/events/core.c           | 21 ++++++++++++++-------
   kernel/trace/bpf_trace.c       |  8 +++++---
   tools/include/uapi/linux/bpf.h |  7 +++++++
   10 files changed, 73 insertions(+), 28 deletions(-)

drivers/media/rc/bpf-lirc.c +1 -1

index afae0afe3f81..7490494273e4 100644
@@ -160,7 +160,7 @@ static int lirc_bpf_attach(struct rc_dev *rcdev, struct bpf_prog *prog)
               goto unlock;
       }
 
-     ret = bpf_prog_array_copy(old_array, NULL, prog, &new_array);
+     ret = bpf_prog_array_copy(old_array, NULL, prog, 0, &new_array);
       if (ret < 0)
               goto unlock;
 

[...]

Quoted 6 lines

   void bpf_trace_run1(struct bpf_prog *prog, u64 arg1);

include/uapi/linux/bpf.h +0 -0

index 00b1267ab4f0..bc1fd54a8f58 100644

include/uapi/linux/bpf.h union bpf_attr { · line 1457

Quoted 10 lines

include/uapi/linux/bpf.h +7 -0

@@ -1448,6 +1448,13 @@ union bpf_attr {
                               __aligned_u64   iter_info;      /* extra bpf_iter_link_info */
                               __u32           iter_info_len;  /* iter_info length */
                       };
+                     struct {
+                             /* black box user-provided value passed through
+                              * to BPF program at the execution time and
+                              * accessible through bpf_get_user_ctx() BPF helper
+                              */
+                             __u64           user_ctx;
+                     } perf_event;

Is it possible to fold this field into previous union?

union {
__u32 target_btf_id; /* btf_id of
target to attach to */

Quoted 9 lines

                          struct {
                                  __aligned_u64   iter_info;      /*
extra bpf_iter_link_info */
                                  __u32           iter_info_len;  /*
iter_info length */
                          };
                  };

I didn't want to do it, because different types of BPF links will accept this user_ctx (or now bpf_cookie). And then we'll have to have different locations of that field for different types of links.

For example, when/if we add this user_ctx to BPF iterator programs, having __u64 user_ctx in the same anonymous union will make it overlap with iter_info, which is a problem. So I want to have a link type-specific sections in LINK_CREATE command section, to allow the same field name at different locations.

I actually think that we should put iter_info/iter_info_len into a named field, like this (also added user_ctx for bpf_iter link as a demonstration):

struct {
     __aligned_u64 info;
     __u32         info_len;
     __aligned_u64 user_ctx;  /* see how it's at a different offset
than perf_event.user_ctx */
} iter;
struct {
     __u64         user_ctx;
} perf_event;

(of course keeping already existing fields in anonymous struct for backwards compatibility)

I decided to not do that in this patch set, though, to not distract from the main goal. But I think we should avoid this shared field "namespace" across different link types going forward.

Quoted 4 lines

Quoted 3 lines

include/uapi/linux/bpf.h +0 -0

               };
       } link_create;
 

[...]

Quoted 120 lines · Andrii Nakryiko

Quoted 85 lines · Yonghong Song

Quoted 58 lines · Andrii Nakryiko

Add ability for users to specify custom u64 value when creating BPF link for perf_event-backed BPF programs (kprobe/uprobe, perf_event, tracepoints).

This is useful for cases when the same BPF program is used for attaching and processing invocation of different tracepoints/kprobes/uprobes in a generic fashion, but such that each invocation is distinguished from each other (e.g., BPF program can look up additional information associated with a specific kernel function without having to rely on function IP lookups). This enables new use cases to be implemented simply and efficiently that previously were possible only through code generation (and thus multiple instances of almost identical BPF program) or compilation at runtime (BCC-style) on target hosts (even more expensive resource-wise). For uprobes it is not even possible in some cases to know function IP before hand (e.g., when attaching to shared library without PID filtering, in which case base load address is not known for a library).

This is done by storing u64 user_ctx in struct bpf_prog_array_item, corresponding to each attached and run BPF program. Given cgroup BPF programs already use 2 8-byte pointers for their needs and cgroup BPF programs don't have (yet?) support for user_ctx, reuse that space through union of cgroup_storage and new user_ctx field.

Make it available to kprobe/tracepoint BPF programs through bpf_trace_run_ctx. This is set by BPF_PROG_RUN_ARRAY, used by kprobe/uprobe/tracepoint BPF program execution code, which luckily is now also split from BPF_PROG_RUN_ARRAY_CG. This run context will be utilized by a new BPF helper giving access to this user context value from inside a BPF program. Generic perf_event BPF programs will access this value from perf_event itself through passed in BPF program context.

Cc Peter Zijlstra <[email protected]>

Signed-off-by Andrii Nakryiko <[email protected]>

---
   drivers/media/rc/bpf-lirc.c    |  4 ++--
   include/linux/bpf.h            | 16 +++++++++++++++-
   include/linux/perf_event.h     |  1 +
   include/linux/trace_events.h   |  6 +++---
   include/uapi/linux/bpf.h       |  7 +++++++
   kernel/bpf/core.c              | 29 ++++++++++++++++++-----------
   kernel/bpf/syscall.c           |  2 +-
   kernel/events/core.c           | 21 ++++++++++++++-------
   kernel/trace/bpf_trace.c       |  8 +++++---
   tools/include/uapi/linux/bpf.h |  7 +++++++
   10 files changed, 73 insertions(+), 28 deletions(-)

drivers/media/rc/bpf-lirc.c +1 -1

index afae0afe3f81..7490494273e4 100644
@@ -160,7 +160,7 @@ static int lirc_bpf_attach(struct rc_dev *rcdev, struct bpf_prog *prog)
               goto unlock;
       }
 
-     ret = bpf_prog_array_copy(old_array, NULL, prog, &new_array);
+     ret = bpf_prog_array_copy(old_array, NULL, prog, 0, &new_array);
       if (ret < 0)
               goto unlock;
 

[...]

Quoted 6 lines

   void bpf_trace_run1(struct bpf_prog *prog, u64 arg1);

include/uapi/linux/bpf.h +0 -0

index 00b1267ab4f0..bc1fd54a8f58 100644

include/uapi/linux/bpf.h union bpf_attr { · line 1457

Quoted 10 lines

include/uapi/linux/bpf.h +7 -0

@@ -1448,6 +1448,13 @@ union bpf_attr {
                               __aligned_u64   iter_info;      /* extra bpf_iter_link_info */
                               __u32           iter_info_len;  /* iter_info length */
                       };
+                     struct {
+                             /* black box user-provided value passed through
+                              * to BPF program at the execution time and
+                              * accessible through bpf_get_user_ctx() BPF helper
+                              */
+                             __u64           user_ctx;
+                     } perf_event;

Is it possible to fold this field into previous union?

union {
__u32 target_btf_id; /* btf_id of
target to attach to */

Quoted 9 lines

                          struct {
                                  __aligned_u64   iter_info;      /*
extra bpf_iter_link_info */
                                  __u32           iter_info_len;  /*
iter_info length */
                          };
                  };

I didn't want to do it, because different types of BPF links will accept this user_ctx (or now bpf_cookie). And then we'll have to have different locations of that field for different types of links.

For example, when/if we add this user_ctx to BPF iterator programs, having __u64 user_ctx in the same anonymous union will make it overlap with iter_info, which is a problem. So I want to have a link type-specific sections in LINK_CREATE command section, to allow the same field name at different locations.

I actually think that we should put iter_info/iter_info_len into a named field, like this (also added user_ctx for bpf_iter link as a demonstration):

struct {
     __aligned_u64 info;
     __u32         info_len;
     __aligned_u64 user_ctx;  /* see how it's at a different offset
than perf_event.user_ctx */
} iter;
struct {
     __u64         user_ctx;
} perf_event;

Quoted 2 lines

(of course keeping already existing fields in anonymous struct for backwards compatibility)

Okay, then since user_ctx may be used by many link types. How about just with the field "user_ctx" without struct perf_event.

Sometime like

__u64 user_ctx;

instead of

struct {
        __u64   user_ctx;
} perf_event;

Quoted 10 lines

I decided to not do that in this patch set, though, to not distract from the main goal. But I think we should avoid this shared field "namespace" across different link types going forward.

Quoted 4 lines

Quoted 3 lines

include/uapi/linux/bpf.h +0 -0

               };
       } link_create;
 

[...]

Quoted 124 lines · Yonghong Song

Quoted 120 lines · Andrii Nakryiko

Quoted 85 lines · Yonghong Song

Quoted 58 lines · Andrii Nakryiko

Add ability for users to specify custom u64 value when creating BPF link for perf_event-backed BPF programs (kprobe/uprobe, perf_event, tracepoints).

This is useful for cases when the same BPF program is used for attaching and processing invocation of different tracepoints/kprobes/uprobes in a generic fashion, but such that each invocation is distinguished from each other (e.g., BPF program can look up additional information associated with a specific kernel function without having to rely on function IP lookups). This enables new use cases to be implemented simply and efficiently that previously were possible only through code generation (and thus multiple instances of almost identical BPF program) or compilation at runtime (BCC-style) on target hosts (even more expensive resource-wise). For uprobes it is not even possible in some cases to know function IP before hand (e.g., when attaching to shared library without PID filtering, in which case base load address is not known for a library).

This is done by storing u64 user_ctx in struct bpf_prog_array_item, corresponding to each attached and run BPF program. Given cgroup BPF programs already use 2 8-byte pointers for their needs and cgroup BPF programs don't have (yet?) support for user_ctx, reuse that space through union of cgroup_storage and new user_ctx field.

Make it available to kprobe/tracepoint BPF programs through bpf_trace_run_ctx. This is set by BPF_PROG_RUN_ARRAY, used by kprobe/uprobe/tracepoint BPF program execution code, which luckily is now also split from BPF_PROG_RUN_ARRAY_CG. This run context will be utilized by a new BPF helper giving access to this user context value from inside a BPF program. Generic perf_event BPF programs will access this value from perf_event itself through passed in BPF program context.

Cc Peter Zijlstra <[email protected]>

Signed-off-by Andrii Nakryiko <[email protected]>

---
    drivers/media/rc/bpf-lirc.c    |  4 ++--
    include/linux/bpf.h            | 16 +++++++++++++++-
    include/linux/perf_event.h     |  1 +
    include/linux/trace_events.h   |  6 +++---
    include/uapi/linux/bpf.h       |  7 +++++++
    kernel/bpf/core.c              | 29 ++++++++++++++++++-----------
    kernel/bpf/syscall.c           |  2 +-
    kernel/events/core.c           | 21 ++++++++++++++-------
    kernel/trace/bpf_trace.c       |  8 +++++---
    tools/include/uapi/linux/bpf.h |  7 +++++++
    10 files changed, 73 insertions(+), 28 deletions(-)

drivers/media/rc/bpf-lirc.c +1 -1

index afae0afe3f81..7490494273e4 100644
@@ -160,7 +160,7 @@ static int lirc_bpf_attach(struct rc_dev *rcdev, struct bpf_prog *prog)
                goto unlock;
        }
 
-     ret = bpf_prog_array_copy(old_array, NULL, prog, &new_array);
+     ret = bpf_prog_array_copy(old_array, NULL, prog, 0, &new_array);
        if (ret < 0)
                goto unlock;
 

[...]

Quoted 6 lines

    void bpf_trace_run1(struct bpf_prog *prog, u64 arg1);

include/uapi/linux/bpf.h +0 -0

index 00b1267ab4f0..bc1fd54a8f58 100644

include/uapi/linux/bpf.h union bpf_attr { · line 1457

Quoted 10 lines

include/uapi/linux/bpf.h +7 -0

@@ -1448,6 +1448,13 @@ union bpf_attr {
                                __aligned_u64   iter_info;      /* extra bpf_iter_link_info */
                                __u32           iter_info_len;  /* iter_info length */
                        };
+                     struct {
+                             /* black box user-provided value passed through
+                              * to BPF program at the execution time and
+                              * accessible through bpf_get_user_ctx() BPF helper
+                              */
+                             __u64           user_ctx;
+                     } perf_event;

Is it possible to fold this field into previous union?

union {
__u32 target_btf_id; /* btf_id of
target to attach to */

Quoted 9 lines

                           struct {
                                   __aligned_u64   iter_info;      /*
extra bpf_iter_link_info */
                                   __u32           iter_info_len;  /*
iter_info length */
                           };
                   };

I didn't want to do it, because different types of BPF links will accept this user_ctx (or now bpf_cookie). And then we'll have to have different locations of that field for different types of links.

For example, when/if we add this user_ctx to BPF iterator programs, having __u64 user_ctx in the same anonymous union will make it overlap with iter_info, which is a problem. So I want to have a link type-specific sections in LINK_CREATE command section, to allow the same field name at different locations.

I actually think that we should put iter_info/iter_info_len into a named field, like this (also added user_ctx for bpf_iter link as a demonstration):

struct {
      __aligned_u64 info;
      __u32         info_len;
      __aligned_u64 user_ctx;  /* see how it's at a different offset
than perf_event.user_ctx */
} iter;
struct {
      __u64         user_ctx;
} perf_event;

Quoted 5 lines

Quoted 2 lines

(of course keeping already existing fields in anonymous struct for backwards compatibility)

Okay, then since user_ctx may be used by many link types. How about just with the field "user_ctx" without struct perf_event.

I'd love to do it because it is indeed generic and common field, like target_fd. But I'm not sure what you are proposing below. Where exactly that user_ctx (now called bpf_cookie) goes in your example? I see few possible options that allow preserving ABI backwards compatibility. Let's see if you and everyone else likes any of those better. I'll use the full LINK_CREATE sub-struct definition from bpf_attr to make it clear. And to demonstrate how this can be extended to bpf_iter in the future, please note this part as this is an important aspect.

1. Full backwards compatibility and per-link type sections (my current approach):

         struct { /* struct used by BPF_LINK_CREATE command */
                 __u32           prog_fd;
                 union {
                         __u32           target_fd;
                         __u32           target_ifindex;
                 };
                 __u32           attach_type;
                 __u32           flags;
                 union {
                         __u32           target_btf_id;
                         struct {
                                 __aligned_u64   iter_info;
                                 __u32           iter_info_len;
                         };
                         struct {
                                 __u64           bpf_cookie;
                         } perf_event;
                         struct {
                                 __aligned_u64   info;
                                 __u32           info_len;
                                 __aligned_u64   bpf_cookie;
                         } iter;
                };
         } link_create;

The good property here is that we can keep easily extending link type-specific sections with extra fields where needed. For common stuff like bpf_cookie it's suboptimal because we'll need to duplicate field definition in each struct inside that union, but I think that's fine. From end-user point of view, they will know which type of link they are creating, so the use will be straightforward. This is why I went with this approach. But let's consider alternatives.

2. Non-backwards compatible layout but extra flag to specify that new field layout is used.

         struct { /* struct used by BPF_LINK_CREATE command */
                 __u32           prog_fd;
                 union {
                         __u32           target_fd;
                         __u32           target_ifindex;
                 };
                 __u32           attach_type;
                 __u32           flags; /* this will start supporting
some new flag like BPF_F_LINK_CREATE_NEW */
                 __u64           bpf_cookie; /* common field now */
                 union { /* this parts is effectively deprecated now */
                         __u32           target_btf_id;
                         struct {
                                 __aligned_u64   iter_info;
                                 __u32           iter_info_len;
                         };
                         struct { /* this is new layout, but needs
BPF_F_LINK_CREATE_NEW, at least for ext/ and bpf_iter/ programs */
                             __u64       bpf_cookie;
                             union {
                                 struct {
                                     __u32     target_btf_id;
                                 } ext;
                                 struct {
                                     __aligned_u64 info;
                                     __u32         info_len;
                                 } iter;
                             }
                         }
                 };
         } link_create;

This makes bpf_cookie a common field, but at least for EXT (freplace/) and ITER (bpf_iter/) links we need to specify extra flag to specify that we are not using iter_info/iter_info_len/target_btf_id. bpf_iter then will use iter.info and iter.info_len, and can use plain bpf_cookie.

IMO, this is way too confusing and a maintainability nightmare.

I'm trying to guess what you are proposing, I can read it two ways, but let me know if I missed something.

3. Just add bpf_cookie field before link type-specific section.

         struct { /* struct used by BPF_LINK_CREATE command */
                 __u32           prog_fd;
                 union {
                         __u32           target_fd;
                         __u32           target_ifindex;
                 };
                 __u32           attach_type;
                 __u32           flags;
                 __u64           bpf_cookie;  // <<<<<<<<<< HERE
                 union {
                         __u32           target_btf_id;
                         struct {
                                 __aligned_u64   iter_info;
                                 __u32           iter_info_len;
                         };
                 };
         } link_create;

This looks really nice and would be great, but that changes offsets for target_btf_id/iter_info/iter_info_len, so a no go. The only way to rectify this is what proposal #2 above does with an extra flag.

4. Add bpf_cookie after link-type specific part:

         struct { /* struct used by BPF_LINK_CREATE command */
                 __u32           prog_fd;
                 union {
                         __u32           target_fd;
                         __u32           target_ifindex;
                 };
                 __u32           attach_type;
                 __u32           flags;
                 union {
                         __u32           target_btf_id;
                         struct {
                                 __aligned_u64   iter_info;
                                 __u32           iter_info_len;
                         };
                         struct {
                 };
                 __u64           bpf_cookie; // <<<<<<<<<<<<<<<<<< HERE
         } link_create;

This could work. But we are wasting 16 bytes currently used for target_btf_id/iter_info/iter_info_len. If we later need to do something link type-specific, we can add it to the existing union if we need <= 16 bytes, otherwise we'll need to start another union after bpf_cookie, splitting this into two link type-specific sections.

Overall, this might work, especially assuming we won't need to extend iter-specific portions. But I really hate that we didn't do named structs inside that union (i.e., ext.target_btf_id and iter.info/iter.info_len) and I'd like to rectify that in the follow up patches with named structs duplicating existing field layout, but with proper naming. But splitting this LINK_CREATE bpf_attr part into two unions would make it hard and awkward in the future.

So, thoughts? Did you have something else in mind that I missed?

Quoted 22 lines

Sometime like

__u64 user_ctx;

instead of

struct {
         __u64   user_ctx;
} perf_event;

Quoted 10 lines

I decided to not do that in this patch set, though, to not distract from the main goal. But I think we should avoid this shared field "namespace" across different link types going forward.

Quoted 4 lines

Quoted 3 lines

include/uapi/linux/bpf.h +0 -0

                };
        } link_create;
 

[...]

Quoted 282 lines · Andrii Nakryiko

Quoted 124 lines · Yonghong Song

Quoted 120 lines · Andrii Nakryiko

Quoted 85 lines · Yonghong Song

Quoted 58 lines · Andrii Nakryiko

Add ability for users to specify custom u64 value when creating BPF link for perf_event-backed BPF programs (kprobe/uprobe, perf_event, tracepoints).

This is useful for cases when the same BPF program is used for attaching and processing invocation of different tracepoints/kprobes/uprobes in a generic fashion, but such that each invocation is distinguished from each other (e.g., BPF program can look up additional information associated with a specific kernel function without having to rely on function IP lookups). This enables new use cases to be implemented simply and efficiently that previously were possible only through code generation (and thus multiple instances of almost identical BPF program) or compilation at runtime (BCC-style) on target hosts (even more expensive resource-wise). For uprobes it is not even possible in some cases to know function IP before hand (e.g., when attaching to shared library without PID filtering, in which case base load address is not known for a library).

This is done by storing u64 user_ctx in struct bpf_prog_array_item, corresponding to each attached and run BPF program. Given cgroup BPF programs already use 2 8-byte pointers for their needs and cgroup BPF programs don't have (yet?) support for user_ctx, reuse that space through union of cgroup_storage and new user_ctx field.

Make it available to kprobe/tracepoint BPF programs through bpf_trace_run_ctx. This is set by BPF_PROG_RUN_ARRAY, used by kprobe/uprobe/tracepoint BPF program execution code, which luckily is now also split from BPF_PROG_RUN_ARRAY_CG. This run context will be utilized by a new BPF helper giving access to this user context value from inside a BPF program. Generic perf_event BPF programs will access this value from perf_event itself through passed in BPF program context.

Cc Peter Zijlstra <[email protected]>

Signed-off-by Andrii Nakryiko <[email protected]>

---
    drivers/media/rc/bpf-lirc.c    |  4 ++--
    include/linux/bpf.h            | 16 +++++++++++++++-
    include/linux/perf_event.h     |  1 +
    include/linux/trace_events.h   |  6 +++---
    include/uapi/linux/bpf.h       |  7 +++++++
    kernel/bpf/core.c              | 29 ++++++++++++++++++-----------
    kernel/bpf/syscall.c           |  2 +-
    kernel/events/core.c           | 21 ++++++++++++++-------
    kernel/trace/bpf_trace.c       |  8 +++++---
    tools/include/uapi/linux/bpf.h |  7 +++++++
    10 files changed, 73 insertions(+), 28 deletions(-)

drivers/media/rc/bpf-lirc.c +1 -1

index afae0afe3f81..7490494273e4 100644
@@ -160,7 +160,7 @@ static int lirc_bpf_attach(struct rc_dev *rcdev, struct bpf_prog *prog)
                goto unlock;
        }
 
-     ret = bpf_prog_array_copy(old_array, NULL, prog, &new_array);
+     ret = bpf_prog_array_copy(old_array, NULL, prog, 0, &new_array);
        if (ret < 0)
                goto unlock;
 

[...]

Quoted 6 lines

    void bpf_trace_run1(struct bpf_prog *prog, u64 arg1);

include/uapi/linux/bpf.h +0 -0

index 00b1267ab4f0..bc1fd54a8f58 100644

include/uapi/linux/bpf.h union bpf_attr { · line 1457

Quoted 10 lines

include/uapi/linux/bpf.h +7 -0

@@ -1448,6 +1448,13 @@ union bpf_attr {
                                __aligned_u64   iter_info;      /* extra bpf_iter_link_info */
                                __u32           iter_info_len;  /* iter_info length */
                        };
+                     struct {
+                             /* black box user-provided value passed through
+                              * to BPF program at the execution time and
+                              * accessible through bpf_get_user_ctx() BPF helper
+                              */
+                             __u64           user_ctx;
+                     } perf_event;

Is it possible to fold this field into previous union?

union {
__u32 target_btf_id; /* btf_id of
target to attach to */

Quoted 9 lines

                           struct {
                                   __aligned_u64   iter_info;      /*
extra bpf_iter_link_info */
                                   __u32           iter_info_len;  /*
iter_info length */
                           };
                   };

I didn't want to do it, because different types of BPF links will accept this user_ctx (or now bpf_cookie). And then we'll have to have different locations of that field for different types of links.

For example, when/if we add this user_ctx to BPF iterator programs, having __u64 user_ctx in the same anonymous union will make it overlap with iter_info, which is a problem. So I want to have a link type-specific sections in LINK_CREATE command section, to allow the same field name at different locations.

I actually think that we should put iter_info/iter_info_len into a named field, like this (also added user_ctx for bpf_iter link as a demonstration):

struct {
      __aligned_u64 info;
      __u32         info_len;
      __aligned_u64 user_ctx;  /* see how it's at a different offset
than perf_event.user_ctx */
} iter;
struct {
      __u64         user_ctx;
} perf_event;

Quoted 5 lines

Quoted 2 lines

(of course keeping already existing fields in anonymous struct for backwards compatibility)

Okay, then since user_ctx may be used by many link types. How about just with the field "user_ctx" without struct perf_event.

I'd love to do it because it is indeed generic and common field, like target_fd. But I'm not sure what you are proposing below. Where exactly that user_ctx (now called bpf_cookie) goes in your example? I see few possible options that allow preserving ABI backwards compatibility. Let's see if you and everyone else likes any of those better. I'll use the full LINK_CREATE sub-struct definition from bpf_attr to make it clear. And to demonstrate how this can be extended to bpf_iter in the future, please note this part as this is an important aspect.

1. Full backwards compatibility and per-link type sections (my current approach):

         struct { /* struct used by BPF_LINK_CREATE command */
                 __u32           prog_fd;
                 union {
                         __u32           target_fd;
                         __u32           target_ifindex;
                 };
                 __u32           attach_type;
                 __u32           flags;
                 union {
                         __u32           target_btf_id;
                         struct {
                                 __aligned_u64   iter_info;
                                 __u32           iter_info_len;
                         };
                         struct {
                                 __u64           bpf_cookie;
                         } perf_event;
                         struct {
                                 __aligned_u64   info;
                                 __u32           info_len;
                                 __aligned_u64   bpf_cookie;
                         } iter;
                };
         } link_create;

The good property here is that we can keep easily extending link type-specific sections with extra fields where needed. For common stuff like bpf_cookie it's suboptimal because we'll need to duplicate field definition in each struct inside that union, but I think that's fine. From end-user point of view, they will know which type of link they are creating, so the use will be straightforward. This is why I went with this approach. But let's consider alternatives.

2. Non-backwards compatible layout but extra flag to specify that new field layout is used.

         struct { /* struct used by BPF_LINK_CREATE command */
                 __u32           prog_fd;
                 union {
                         __u32           target_fd;
                         __u32           target_ifindex;
                 };
                 __u32           attach_type;
                 __u32           flags; /* this will start supporting
some new flag like BPF_F_LINK_CREATE_NEW */
                 __u64           bpf_cookie; /* common field now */
                 union { /* this parts is effectively deprecated now */
                         __u32           target_btf_id;
                         struct {
                                 __aligned_u64   iter_info;
                                 __u32           iter_info_len;
                         };
                         struct { /* this is new layout, but needs
BPF_F_LINK_CREATE_NEW, at least for ext/ and bpf_iter/ programs */
                             __u64       bpf_cookie;
                             union {
                                 struct {
                                     __u32     target_btf_id;
                                 } ext;
                                 struct {
                                     __aligned_u64 info;
                                     __u32         info_len;
                                 } iter;
                             }
                         }
                 };
         } link_create;

This makes bpf_cookie a common field, but at least for EXT (freplace/) and ITER (bpf_iter/) links we need to specify extra flag to specify that we are not using iter_info/iter_info_len/target_btf_id. bpf_iter then will use iter.info and iter.info_len, and can use plain bpf_cookie.

IMO, this is way too confusing and a maintainability nightmare.

I'm trying to guess what you are proposing, I can read it two ways, but let me know if I missed something.

3. Just add bpf_cookie field before link type-specific section.

         struct { /* struct used by BPF_LINK_CREATE command */
                 __u32           prog_fd;
                 union {
                         __u32           target_fd;
                         __u32           target_ifindex;
                 };
                 __u32           attach_type;
                 __u32           flags;
                 __u64           bpf_cookie;  // <<<<<<<<<< HERE
                 union {
                         __u32           target_btf_id;
                         struct {
                                 __aligned_u64   iter_info;
                                 __u32           iter_info_len;
                         };
                 };
         } link_create;

This looks really nice and would be great, but that changes offsets for target_btf_id/iter_info/iter_info_len, so a no go. The only way to rectify this is what proposal #2 above does with an extra flag.

4. Add bpf_cookie after link-type specific part:

         struct { /* struct used by BPF_LINK_CREATE command */
                 __u32           prog_fd;
                 union {
                         __u32           target_fd;
                         __u32           target_ifindex;
                 };
                 __u32           attach_type;
                 __u32           flags;
                 union {
                         __u32           target_btf_id;
                         struct {
                                 __aligned_u64   iter_info;
                                 __u32           iter_info_len;
                         };
                         struct {
                 };
                 __u64           bpf_cookie; // <<<<<<<<<<<<<<<<<< HERE
         } link_create;

This could work. But we are wasting 16 bytes currently used for target_btf_id/iter_info/iter_info_len. If we later need to do something link type-specific, we can add it to the existing union if we need <= 16 bytes, otherwise we'll need to start another union after bpf_cookie, splitting this into two link type-specific sections.

Overall, this might work, especially assuming we won't need to extend iter-specific portions. But I really hate that we didn't do named structs inside that union (i.e., ext.target_btf_id and iter.info/iter.info_len) and I'd like to rectify that in the follow up patches with named structs duplicating existing field layout, but with proper naming. But splitting this LINK_CREATE bpf_attr part into two unions would make it hard and awkward in the future.

Quoted 1 line

So, thoughts? Did you have something else in mind that I missed?

What I proposed is your option 4. Yes, in the future if there is there are something we want to add to bpf iter, we can add to iter_info, so it should not be an issue. Any other new link_type may utilized the same union with

    struct {
       __aligned_u64  new_type_info;
       __u32          new_type_info_len;
    };

and this will put extensibility into new_type_info. I know this may be a little bit hassle but it should work.

Your option 1 should work too, which is what I proposed in the beginning to put into the union and we can feel free to add bpf_cookie for each individual link type. This is actually cleaner.

Quoted 23 lines

Quoted 23 lines

Sometime like

__u64 user_ctx;

instead of

struct {
         __u64   user_ctx;
} perf_event;

Quoted 10 lines

I decided to not do that in this patch set, though, to not distract from the main goal. But I think we should avoid this shared field "namespace" across different link types going forward.

Quoted 4 lines

Quoted 3 lines

include/uapi/linux/bpf.h +0 -0

                };
        } link_create;
 

[...]

Quoted 292 lines · Yonghong Song

Quoted 282 lines · Andrii Nakryiko

Quoted 124 lines · Yonghong Song

Quoted 120 lines · Andrii Nakryiko

Quoted 85 lines · Yonghong Song

Quoted 58 lines · Andrii Nakryiko

Add ability for users to specify custom u64 value when creating BPF link for perf_event-backed BPF programs (kprobe/uprobe, perf_event, tracepoints).

This is useful for cases when the same BPF program is used for attaching and processing invocation of different tracepoints/kprobes/uprobes in a generic fashion, but such that each invocation is distinguished from each other (e.g., BPF program can look up additional information associated with a specific kernel function without having to rely on function IP lookups). This enables new use cases to be implemented simply and efficiently that previously were possible only through code generation (and thus multiple instances of almost identical BPF program) or compilation at runtime (BCC-style) on target hosts (even more expensive resource-wise). For uprobes it is not even possible in some cases to know function IP before hand (e.g., when attaching to shared library without PID filtering, in which case base load address is not known for a library).

This is done by storing u64 user_ctx in struct bpf_prog_array_item, corresponding to each attached and run BPF program. Given cgroup BPF programs already use 2 8-byte pointers for their needs and cgroup BPF programs don't have (yet?) support for user_ctx, reuse that space through union of cgroup_storage and new user_ctx field.

Make it available to kprobe/tracepoint BPF programs through bpf_trace_run_ctx. This is set by BPF_PROG_RUN_ARRAY, used by kprobe/uprobe/tracepoint BPF program execution code, which luckily is now also split from BPF_PROG_RUN_ARRAY_CG. This run context will be utilized by a new BPF helper giving access to this user context value from inside a BPF program. Generic perf_event BPF programs will access this value from perf_event itself through passed in BPF program context.

Cc Peter Zijlstra <[email protected]>

Signed-off-by Andrii Nakryiko <[email protected]>

---
     drivers/media/rc/bpf-lirc.c    |  4 ++--
     include/linux/bpf.h            | 16 +++++++++++++++-
     include/linux/perf_event.h     |  1 +
     include/linux/trace_events.h   |  6 +++---
     include/uapi/linux/bpf.h       |  7 +++++++
     kernel/bpf/core.c              | 29 ++++++++++++++++++-----------
     kernel/bpf/syscall.c           |  2 +-
     kernel/events/core.c           | 21 ++++++++++++++-------
     kernel/trace/bpf_trace.c       |  8 +++++---
     tools/include/uapi/linux/bpf.h |  7 +++++++
     10 files changed, 73 insertions(+), 28 deletions(-)

drivers/media/rc/bpf-lirc.c +1 -1

index afae0afe3f81..7490494273e4 100644
@@ -160,7 +160,7 @@ static int lirc_bpf_attach(struct rc_dev *rcdev, struct bpf_prog *prog)
                 goto unlock;
         }
 
-     ret = bpf_prog_array_copy(old_array, NULL, prog, &new_array);
+     ret = bpf_prog_array_copy(old_array, NULL, prog, 0, &new_array);
         if (ret < 0)
                 goto unlock;
 

[...]

Quoted 6 lines

     void bpf_trace_run1(struct bpf_prog *prog, u64 arg1);

include/uapi/linux/bpf.h +0 -0

index 00b1267ab4f0..bc1fd54a8f58 100644

include/uapi/linux/bpf.h union bpf_attr { · line 1457

Quoted 10 lines

include/uapi/linux/bpf.h +7 -0

@@ -1448,6 +1448,13 @@ union bpf_attr {
                                 __aligned_u64   iter_info;      /* extra bpf_iter_link_info */
                                 __u32           iter_info_len;  /* iter_info length */
                         };
+                     struct {
+                             /* black box user-provided value passed through
+                              * to BPF program at the execution time and
+                              * accessible through bpf_get_user_ctx() BPF helper
+                              */
+                             __u64           user_ctx;
+                     } perf_event;

Is it possible to fold this field into previous union?

union {
__u32 target_btf_id; /* btf_id of
target to attach to */

Quoted 9 lines

                            struct {
                                    __aligned_u64   iter_info;      /*
extra bpf_iter_link_info */
                                    __u32           iter_info_len;  /*
iter_info length */
                            };
                    };

I didn't want to do it, because different types of BPF links will accept this user_ctx (or now bpf_cookie). And then we'll have to have different locations of that field for different types of links.

For example, when/if we add this user_ctx to BPF iterator programs, having __u64 user_ctx in the same anonymous union will make it overlap with iter_info, which is a problem. So I want to have a link type-specific sections in LINK_CREATE command section, to allow the same field name at different locations.

I actually think that we should put iter_info/iter_info_len into a named field, like this (also added user_ctx for bpf_iter link as a demonstration):

struct {
       __aligned_u64 info;
       __u32         info_len;
       __aligned_u64 user_ctx;  /* see how it's at a different offset
than perf_event.user_ctx */
} iter;
struct {
       __u64         user_ctx;
} perf_event;

Quoted 5 lines

Quoted 2 lines

(of course keeping already existing fields in anonymous struct for backwards compatibility)

Okay, then since user_ctx may be used by many link types. How about just with the field "user_ctx" without struct perf_event.

I'd love to do it because it is indeed generic and common field, like target_fd. But I'm not sure what you are proposing below. Where exactly that user_ctx (now called bpf_cookie) goes in your example? I see few possible options that allow preserving ABI backwards compatibility. Let's see if you and everyone else likes any of those better. I'll use the full LINK_CREATE sub-struct definition from bpf_attr to make it clear. And to demonstrate how this can be extended to bpf_iter in the future, please note this part as this is an important aspect.

1. Full backwards compatibility and per-link type sections (my current approach):

          struct { /* struct used by BPF_LINK_CREATE command */
                  __u32           prog_fd;
                  union {
                          __u32           target_fd;
                          __u32           target_ifindex;
                  };
                  __u32           attach_type;
                  __u32           flags;
                  union {
                          __u32           target_btf_id;
                          struct {
                                  __aligned_u64   iter_info;
                                  __u32           iter_info_len;
                          };
                          struct {
                                  __u64           bpf_cookie;
                          } perf_event;
                          struct {
                                  __aligned_u64   info;
                                  __u32           info_len;
                                  __aligned_u64   bpf_cookie;
                          } iter;
                 };
          } link_create;

The good property here is that we can keep easily extending link type-specific sections with extra fields where needed. For common stuff like bpf_cookie it's suboptimal because we'll need to duplicate field definition in each struct inside that union, but I think that's fine. From end-user point of view, they will know which type of link they are creating, so the use will be straightforward. This is why I went with this approach. But let's consider alternatives.

2. Non-backwards compatible layout but extra flag to specify that new field layout is used.

          struct { /* struct used by BPF_LINK_CREATE command */
                  __u32           prog_fd;
                  union {
                          __u32           target_fd;
                          __u32           target_ifindex;
                  };
                  __u32           attach_type;
                  __u32           flags; /* this will start supporting
some new flag like BPF_F_LINK_CREATE_NEW */
                  __u64           bpf_cookie; /* common field now */
                  union { /* this parts is effectively deprecated now */
                          __u32           target_btf_id;
                          struct {
                                  __aligned_u64   iter_info;
                                  __u32           iter_info_len;
                          };
                          struct { /* this is new layout, but needs
BPF_F_LINK_CREATE_NEW, at least for ext/ and bpf_iter/ programs */
                              __u64       bpf_cookie;
                              union {
                                  struct {
                                      __u32     target_btf_id;
                                  } ext;
                                  struct {
                                      __aligned_u64 info;
                                      __u32         info_len;
                                  } iter;
                              }
                          }
                  };
          } link_create;

This makes bpf_cookie a common field, but at least for EXT (freplace/) and ITER (bpf_iter/) links we need to specify extra flag to specify that we are not using iter_info/iter_info_len/target_btf_id. bpf_iter then will use iter.info and iter.info_len, and can use plain bpf_cookie.

IMO, this is way too confusing and a maintainability nightmare.

I'm trying to guess what you are proposing, I can read it two ways, but let me know if I missed something.

3. Just add bpf_cookie field before link type-specific section.

          struct { /* struct used by BPF_LINK_CREATE command */
                  __u32           prog_fd;
                  union {
                          __u32           target_fd;
                          __u32           target_ifindex;
                  };
                  __u32           attach_type;
                  __u32           flags;
                  __u64           bpf_cookie;  // <<<<<<<<<< HERE
                  union {
                          __u32           target_btf_id;
                          struct {
                                  __aligned_u64   iter_info;
                                  __u32           iter_info_len;
                          };
                  };
          } link_create;

This looks really nice and would be great, but that changes offsets for target_btf_id/iter_info/iter_info_len, so a no go. The only way to rectify this is what proposal #2 above does with an extra flag.

4. Add bpf_cookie after link-type specific part:

          struct { /* struct used by BPF_LINK_CREATE command */
                  __u32           prog_fd;
                  union {
                          __u32           target_fd;
                          __u32           target_ifindex;
                  };
                  __u32           attach_type;
                  __u32           flags;
                  union {
                          __u32           target_btf_id;
                          struct {
                                  __aligned_u64   iter_info;
                                  __u32           iter_info_len;
                          };
                          struct {
                  };
                  __u64           bpf_cookie; // <<<<<<<<<<<<<<<<<< HERE
          } link_create;

This could work. But we are wasting 16 bytes currently used for target_btf_id/iter_info/iter_info_len. If we later need to do something link type-specific, we can add it to the existing union if we need <= 16 bytes, otherwise we'll need to start another union after bpf_cookie, splitting this into two link type-specific sections.

Overall, this might work, especially assuming we won't need to extend iter-specific portions. But I really hate that we didn't do named structs inside that union (i.e., ext.target_btf_id and iter.info/iter.info_len) and I'd like to rectify that in the follow up patches with named structs duplicating existing field layout, but with proper naming. But splitting this LINK_CREATE bpf_attr part into two unions would make it hard and awkward in the future.

Quoted 1 line

So, thoughts? Did you have something else in mind that I missed?

What I proposed is your option 4. Yes, in the future if there is there are something we want to add to bpf iter, we can add to iter_info, so it should not be an issue. Any other new link_type may utilized the same union with

Quoted 7 lines

     struct {
        __aligned_u64  new_type_info;
        __u32          new_type_info_len;
     };

and this will put extensibility into new_type_info. I know this may be a little bit hassle but it should work.

I see what you mean. With this extra pointer we shouldn't need more than 16 bytes per link type. That's unnecessary complication for a lot of simpler types of links, unfortunately, though definitely an option.

We could have also done approach #4 but maybe leave 16-32 bytes before bpf_cookie for the union, so that it's much less likely that we'll run out of space there. Not very clean either, so I don't know.

I'll keep it here for discussion for now, let's see if anyone has strong preferences and opinions.

Quoted 3 lines

Your option 1 should work too, which is what I proposed in the beginning to put into the union and we can feel free to add bpf_cookie for each individual link type. This is actually cleaner.

Oh, you did? I must have misunderstood then. If you like approach #1, then it's what I'm doing right now, so let's keep it as is and let's see if anyone else has preferences.

Quoted 24 lines

Quoted 24 lines

Quoted 24 lines

Sometime like

__u64 user_ctx;

instead of

struct {
          __u64   user_ctx;
} perf_event;

Quoted 10 lines

I decided to not do that in this patch set, though, to not distract from the main goal. But I think we should avoid this shared field "namespace" across different link types going forward.

Quoted 4 lines

Quoted 3 lines

include/uapi/linux/bpf.h +0 -0

                 };
         } link_create;
 

[...]

Add new BPF helper, bpf_get_user_ctx(), which can be used by BPF programs to get access to the user_ctx value, specified during BPF program attachment (BPF link creation) time.

Currently all perf_event-backed BPF program types support bpf_get_user_ctx() helper. Follow-up patches will add support for fentry/fexit programs as well.

While at it, mark bpf_tracing_func_proto() as static to make it obvious that it's only used from within the kernel/trace/bpf_trace.c.

Cc Peter Zijlstra <[email protected]>

Signed-off-by Andrii Nakryiko <[email protected]>

---
  include/linux/bpf.h            |  3 ---
  include/uapi/linux/bpf.h       | 16 ++++++++++++++++
  kernel/trace/bpf_trace.c       | 35 +++++++++++++++++++++++++++++++++-
  tools/include/uapi/linux/bpf.h | 16 ++++++++++++++++
  4 files changed, 66 insertions(+), 4 deletions(-)

include/linux/bpf.h +0 -3

index 74b35faf0b73..94ebedc1e13a 100644
@@ -2110,9 +2110,6 @@ extern const struct bpf_func_proto bpf_btf_find_by_name_kind_proto;
  extern const struct bpf_func_proto bpf_sk_setsockopt_proto;
  extern const struct bpf_func_proto bpf_sk_getsockopt_proto;
 
-const struct bpf_func_proto *bpf_tracing_func_proto(
-	enum bpf_func_id func_id, const struct bpf_prog *prog);
-
  const struct bpf_func_proto *tracing_prog_func_proto(
    enum bpf_func_id func_id, const struct bpf_prog *prog);
 

include/uapi/linux/bpf.h +2 -0

index bc1fd54a8f58..96afeced3467 100644
@@ -4856,6 +4856,21 @@ union bpf_attr {
   * 		Get address of the traced function (for tracing and kprobe programs).
   * 	Return
   * 		Address of the traced function.
+ *
+ * u64 bpf_get_user_ctx(void *ctx)

include/uapi/linux/bpf.h +4 -0

+ * 	Return
+ *		Value specified by user at BPF link creation/attachment time
+ *		or 0, if it was not specified.
   */
  #define __BPF_FUNC_MAPPER(FN)		\
  	FN(unspec),			\

@@ -5032,6 +5047,7 @@ union bpf_attr {
  	FN(timer_start),		\
  	FN(timer_cancel),		\
  	FN(get_func_ip),		\
+	FN(get_user_ctx),		\
  	/* */
 
  /* integer value in 'imm' field of BPF_CALL instruction selects which helper

kernel/trace/bpf_trace.c +15 -1

index c9cf6a0d0fb3..b14978b3f6fb 100644
@@ -975,7 +975,34 @@ static const struct bpf_func_proto bpf_get_func_ip_proto_kprobe = {
  	.arg1_type	= ARG_PTR_TO_CTX,
  };
 
-const struct bpf_func_proto *
+BPF_CALL_1(bpf_get_user_ctx_trace, void *, ctx)
+{
+	struct bpf_trace_run_ctx *run_ctx;
+
+	run_ctx = container_of(current->bpf_ctx, struct bpf_trace_run_ctx, run_ctx);
+	return run_ctx->user_ctx;
+}
+
+static const struct bpf_func_proto bpf_get_user_ctx_proto_trace = {
+	.func		= bpf_get_user_ctx_trace,
+	.gpl_only	= false,
+	.ret_type	= RET_INTEGER,
+	.arg1_type	= ARG_PTR_TO_CTX,
+};
+

Quoted 43 lines · Andrii Nakryiko

Add new BPF helper, bpf_get_user_ctx(), which can be used by BPF programs to get access to the user_ctx value, specified during BPF program attachment (BPF link creation) time.

Currently all perf_event-backed BPF program types support bpf_get_user_ctx() helper. Follow-up patches will add support for fentry/fexit programs as well.

While at it, mark bpf_tracing_func_proto() as static to make it obvious that it's only used from within the kernel/trace/bpf_trace.c.

Cc Peter Zijlstra <[email protected]>

Signed-off-by Andrii Nakryiko <[email protected]>

---
  include/linux/bpf.h            |  3 ---
  include/uapi/linux/bpf.h       | 16 ++++++++++++++++
  kernel/trace/bpf_trace.c       | 35 +++++++++++++++++++++++++++++++++-
  tools/include/uapi/linux/bpf.h | 16 ++++++++++++++++
  4 files changed, 66 insertions(+), 4 deletions(-)

include/linux/bpf.h +0 -3

index 74b35faf0b73..94ebedc1e13a 100644
@@ -2110,9 +2110,6 @@ extern const struct bpf_func_proto bpf_btf_find_by_name_kind_proto;
  extern const struct bpf_func_proto bpf_sk_setsockopt_proto;
  extern const struct bpf_func_proto bpf_sk_getsockopt_proto;
 
-const struct bpf_func_proto *bpf_tracing_func_proto(
-     enum bpf_func_id func_id, const struct bpf_prog *prog);
-
  const struct bpf_func_proto *tracing_prog_func_proto(
    enum bpf_func_id func_id, const struct bpf_prog *prog);
 

include/uapi/linux/bpf.h +2 -0

index bc1fd54a8f58..96afeced3467 100644
@@ -4856,6 +4856,21 @@ union bpf_attr {
   *          Get address of the traced function (for tracing and kprobe programs).
   *  Return
   *          Address of the traced function.
+ *
+ * u64 bpf_get_user_ctx(void *ctx)

include/uapi/linux/bpf.h union bpf_attr { · line 4870

Quoted 10 lines

include/uapi/linux/bpf.h +10 -0

+ *   Description
+ *           Get user_ctx value provided (optionally) during the program
+ *           attachment. It might be different for each individual
+ *           attachment, even if BPF program itself is the same.
+ *           Expects BPF program context *ctx* as a first argument.
+ *
+ *           Supported for the following program types:
+ *                   - kprobe/uprobe;
+ *                   - tracepoint;
+ *                   - perf_event.

I think it is possible in the future we may need to support more program types with user_ctx, not just u64 but more than 64bit value. Should we may make this helper extensible like
long bpf_get_user_ctx(void *ctx, void *user_ctx, u32 user_ctx_len)