[PATCH bpf-next v2 0/4] Attach a cookie to a tracing program.

[PATCH bpf-next v2 0/4] Attach a cookie to a tracing program.

[Kui-Feng Lee]

Allow users to attach a 64-bits cookie to a BPF program when linking
it to fentry, fexit, or fmod_ret of a function.

This patchset includes several major changes.

 - Define struct bpf_tramp_links to replace bpf_tramp_prog.
   struct bpf_tramp_links collects bpf_links of a trampoline

 - Generate a trampoline to call bpf_progs of given bpf_links.

 - Trampolines always set/reset bpf_run_ctx before/after
   calling/leaving a BPF program.

 - Attach a cookie to a bpf_link of fentry/fexit/fmod_ret.  The value
   will be available when running the associated bpf_prog.

The major differences from v1:

 - Remove program ID and flags from stacks.

 - Create bpf_trace_run_ctx on stacks.

 - Attach a cookie to a bpf_link.

 - Store the cookie of the running BPF program/link in bpf_trace_run_ctx.

v1: https://lore.kernel.org/all/20220126214809.3868787-1-kuifeng@fb.com/

Kui-Feng Lee (4):
  bpf, x86: Generate trampolines from bpf_links
  bpf, x86: Create bpf_trace_run_ctx on the caller thread's stack
  bpf, x86: Support BPF cookie for fentry/fexit/fmod_ret.
  selftest/bpf: The test cses of BPF cookie for fentry/fexit/fmod_ret.

 arch/x86/net/bpf_jit_comp.c                   | 68 ++++++++++----
 include/linux/bpf.h                           | 40 ++++----
 include/linux/bpf_types.h                     |  1 +
 include/uapi/linux/bpf.h                      |  2 +
 kernel/bpf/bpf_struct_ops.c                   | 68 +++++++++-----
 kernel/bpf/syscall.c                          | 19 ++--
 kernel/bpf/trampoline.c                       | 94 ++++++++++++-------
 kernel/trace/bpf_trace.c                      | 17 ++++
 net/bpf/bpf_dummy_struct_ops.c                | 35 ++++++-
 tools/bpf/bpftool/link.c                      |  1 +
 tools/include/uapi/linux/bpf.h                |  2 +
 tools/lib/bpf/bpf.c                           | 14 +++
 tools/lib/bpf/bpf.h                           |  1 +
 tools/lib/bpf/libbpf.map                      |  1 +
 .../selftests/bpf/prog_tests/bpf_cookie.c     | 61 ++++++++++++
 .../selftests/bpf/progs/test_bpf_cookie.c     | 24 +++++
 16 files changed, 344 insertions(+), 104 deletions(-)

-- 
2.30.2

[PATCH bpf-next v2 1/4] bpf, x86: Generate trampolines from bpf_links

[Kui-Feng Lee]

Replace struct bpf_tramp_progs with struct bpf_tramp_links to collect
bpf_links for a trampoline.

arch_prepare_bpf_trampoline() accepts an instance of struct
bpf_tramp_links as an argument that collects all bpf_links that a
trampoline should follow and call.

All callers, including bpf_struct_ops, of
arch_prepare_bpf_trampoline() creates bpf_tramp_links.  bpf_struct_ops
also create bpf_links for members to generate trampolines.

Signed-off-by: Kui-Feng Lee <kuifeng@fb.com>
---
 arch/x86/net/bpf_jit_comp.c    | 36 +++++++++--------
 include/linux/bpf.h            | 27 +++++++------
 include/linux/bpf_types.h      |  1 +
 include/uapi/linux/bpf.h       |  1 +
 kernel/bpf/bpf_struct_ops.c    | 68 +++++++++++++++++++++----------
 kernel/bpf/syscall.c           |  4 +-
 kernel/bpf/trampoline.c        | 73 +++++++++++++++++++---------------
 net/bpf/bpf_dummy_struct_ops.c | 35 +++++++++++++---
 tools/bpf/bpftool/link.c       |  1 +
 tools/include/uapi/linux/bpf.h |  1 +
 10 files changed, 157 insertions(+), 90 deletions(-)

diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index e6ff8f4f9ea4..1228e6e6a420 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -1743,10 +1743,12 @@ static void restore_regs(const struct btf_func_model *m, u8 **prog, int nr_args,
 }
 
 static int invoke_bpf_prog(const struct btf_func_model *m, u8 **pprog,
-			   struct bpf_prog *p, int stack_size, bool save_ret)
+			   struct bpf_link *l, int stack_size,
+			   bool save_ret)
 {
 	u8 *prog = *pprog;
 	u8 *jmp_insn;
+	struct bpf_prog *p = l->prog;
 
 	/* arg1: mov rdi, progs[i] */
 	emit_mov_imm64(&prog, BPF_REG_1, (long) p >> 32, (u32) (long) p);
@@ -1831,14 +1833,14 @@ static int emit_cond_near_jump(u8 **pprog, void *func, void *ip, u8 jmp_cond)
 }
 
 static int invoke_bpf(const struct btf_func_model *m, u8 **pprog,
-		      struct bpf_tramp_progs *tp, int stack_size,
+		      struct bpf_tramp_links *tl, int stack_size,
 		      bool save_ret)
 {
 	int i;
 	u8 *prog = *pprog;
 
-	for (i = 0; i < tp->nr_progs; i++) {
-		if (invoke_bpf_prog(m, &prog, tp->progs[i], stack_size,
+	for (i = 0; i < tl->nr_links; i++) {
+		if (invoke_bpf_prog(m, &prog, tl->links[i], stack_size,
 				    save_ret))
 			return -EINVAL;
 	}
@@ -1847,7 +1849,7 @@ static int invoke_bpf(const struct btf_func_model *m, u8 **pprog,
 }
 
 static int invoke_bpf_mod_ret(const struct btf_func_model *m, u8 **pprog,
-			      struct bpf_tramp_progs *tp, int stack_size,
+			      struct bpf_tramp_links *tl, int stack_size,
 			      u8 **branches)
 {
 	u8 *prog = *pprog;
@@ -1858,8 +1860,8 @@ static int invoke_bpf_mod_ret(const struct btf_func_model *m, u8 **pprog,
 	 */
 	emit_mov_imm32(&prog, false, BPF_REG_0, 0);
 	emit_stx(&prog, BPF_DW, BPF_REG_FP, BPF_REG_0, -8);
-	for (i = 0; i < tp->nr_progs; i++) {
-		if (invoke_bpf_prog(m, &prog, tp->progs[i], stack_size, true))
+	for (i = 0; i < tl->nr_links; i++) {
+		if (invoke_bpf_prog(m, &prog, tl->links[i], stack_size, true))
 			return -EINVAL;
 
 		/* mod_ret prog stored return value into [rbp - 8]. Emit:
@@ -1961,14 +1963,14 @@ static bool is_valid_bpf_tramp_flags(unsigned int flags)
  */
 int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *image_end,
 				const struct btf_func_model *m, u32 flags,
-				struct bpf_tramp_progs *tprogs,
+				struct bpf_tramp_links *tlinks,
 				void *orig_call)
 {
 	int ret, i, nr_args = m->nr_args;
 	int regs_off, ip_off, args_off, stack_size = nr_args * 8;
-	struct bpf_tramp_progs *fentry = &tprogs[BPF_TRAMP_FENTRY];
-	struct bpf_tramp_progs *fexit = &tprogs[BPF_TRAMP_FEXIT];
-	struct bpf_tramp_progs *fmod_ret = &tprogs[BPF_TRAMP_MODIFY_RETURN];
+	struct bpf_tramp_links *fentry = &tlinks[BPF_TRAMP_FENTRY];
+	struct bpf_tramp_links *fexit = &tlinks[BPF_TRAMP_FEXIT];
+	struct bpf_tramp_links *fmod_ret = &tlinks[BPF_TRAMP_MODIFY_RETURN];
 	u8 **branches = NULL;
 	u8 *prog;
 	bool save_ret;
@@ -2055,13 +2057,13 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
 		}
 	}
 
-	if (fentry->nr_progs)
+	if (fentry->nr_links)
 		if (invoke_bpf(m, &prog, fentry, regs_off,
 			       flags & BPF_TRAMP_F_RET_FENTRY_RET))
 			return -EINVAL;
 
-	if (fmod_ret->nr_progs) {
-		branches = kcalloc(fmod_ret->nr_progs, sizeof(u8 *),
+	if (fmod_ret->nr_links) {
+		branches = kcalloc(fmod_ret->nr_links, sizeof(u8 *),
 				   GFP_KERNEL);
 		if (!branches)
 			return -ENOMEM;
@@ -2088,7 +2090,7 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
 		prog += X86_PATCH_SIZE;
 	}
 
-	if (fmod_ret->nr_progs) {
+	if (fmod_ret->nr_links) {
 		/* From Intel 64 and IA-32 Architectures Optimization
 		 * Reference Manual, 3.4.1.4 Code Alignment, Assembly/Compiler
 		 * Coding Rule 11: All branch targets should be 16-byte
@@ -2098,12 +2100,12 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
 		/* Update the branches saved in invoke_bpf_mod_ret with the
 		 * aligned address of do_fexit.
 		 */
-		for (i = 0; i < fmod_ret->nr_progs; i++)
+		for (i = 0; i < fmod_ret->nr_links; i++)
 			emit_cond_near_jump(&branches[i], prog, branches[i],
 					    X86_JNE);
 	}
 
-	if (fexit->nr_progs)
+	if (fexit->nr_links)
 		if (invoke_bpf(m, &prog, fexit, regs_off, false)) {
 			ret = -EINVAL;
 			goto cleanup;
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 88449fbbe063..3dcae8550c21 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -674,11 +674,11 @@ struct btf_func_model {
 /* Each call __bpf_prog_enter + call bpf_func + call __bpf_prog_exit is ~50
  * bytes on x86.  Pick a number to fit into BPF_IMAGE_SIZE / 2
  */
-#define BPF_MAX_TRAMP_PROGS 38
+#define BPF_MAX_TRAMP_LINKS 38
 
-struct bpf_tramp_progs {
-	struct bpf_prog *progs[BPF_MAX_TRAMP_PROGS];
-	int nr_progs;
+struct bpf_tramp_links {
+	struct bpf_link *links[BPF_MAX_TRAMP_LINKS];
+	int nr_links;
 };
 
 /* Different use cases for BPF trampoline:
@@ -704,7 +704,7 @@ struct bpf_tramp_progs {
 struct bpf_tramp_image;
 int arch_prepare_bpf_trampoline(struct bpf_tramp_image *tr, void *image, void *image_end,
 				const struct btf_func_model *m, u32 flags,
-				struct bpf_tramp_progs *tprogs,
+				struct bpf_tramp_links *tlinks,
 				void *orig_call);
 /* these two functions are called from generated trampoline */
 u64 notrace __bpf_prog_enter(struct bpf_prog *prog);
@@ -803,9 +803,12 @@ static __always_inline __nocfi unsigned int bpf_dispatcher_nop_func(
 {
 	return bpf_func(ctx, insnsi);
 }
+
+struct bpf_link;
+
 #ifdef CONFIG_BPF_JIT
-int bpf_trampoline_link_prog(struct bpf_prog *prog, struct bpf_trampoline *tr);
-int bpf_trampoline_unlink_prog(struct bpf_prog *prog, struct bpf_trampoline *tr);
+int bpf_trampoline_link_prog(struct bpf_link *link, struct bpf_trampoline *tr);
+int bpf_trampoline_unlink_prog(struct bpf_link *link, struct bpf_trampoline *tr);
 struct bpf_trampoline *bpf_trampoline_get(u64 key,
 					  struct bpf_attach_target_info *tgt_info);
 void bpf_trampoline_put(struct bpf_trampoline *tr);
@@ -856,12 +859,12 @@ int bpf_jit_charge_modmem(u32 size);
 void bpf_jit_uncharge_modmem(u32 size);
 bool bpf_prog_has_trampoline(const struct bpf_prog *prog);
 #else
-static inline int bpf_trampoline_link_prog(struct bpf_prog *prog,
+static inline int bpf_trampoline_link_prog(struct bpf_link *link,
 					   struct bpf_trampoline *tr)
 {
 	return -ENOTSUPP;
 }
-static inline int bpf_trampoline_unlink_prog(struct bpf_prog *prog,
+static inline int bpf_trampoline_unlink_prog(struct bpf_link *link,
 					     struct bpf_trampoline *tr)
 {
 	return -ENOTSUPP;
@@ -960,7 +963,6 @@ struct bpf_prog_aux {
 	bool tail_call_reachable;
 	bool xdp_has_frags;
 	bool use_bpf_prog_pack;
-	struct hlist_node tramp_hlist;
 	/* BTF_KIND_FUNC_PROTO for valid attach_btf_id */
 	const struct btf_type *attach_func_proto;
 	/* function name for valid attach_btf_id */
@@ -1034,6 +1036,7 @@ struct bpf_link {
 	const struct bpf_link_ops *ops;
 	struct bpf_prog *prog;
 	struct work_struct work;
+	struct hlist_node tramp_hlist;
 };
 
 struct bpf_link_ops {
@@ -1084,8 +1087,8 @@ bool bpf_struct_ops_get(const void *kdata);
 void bpf_struct_ops_put(const void *kdata);
 int bpf_struct_ops_map_sys_lookup_elem(struct bpf_map *map, void *key,
 				       void *value);
-int bpf_struct_ops_prepare_trampoline(struct bpf_tramp_progs *tprogs,
-				      struct bpf_prog *prog,
+int bpf_struct_ops_prepare_trampoline(struct bpf_tramp_links *tlinks,
+				      struct bpf_link *link,
 				      const struct btf_func_model *model,
 				      void *image, void *image_end);
 static inline bool bpf_try_module_get(const void *data, struct module *owner)
diff --git a/include/linux/bpf_types.h b/include/linux/bpf_types.h
index 48a91c51c015..8228c86eb92b 100644
--- a/include/linux/bpf_types.h
+++ b/include/linux/bpf_types.h
@@ -140,3 +140,4 @@ BPF_LINK_TYPE(BPF_LINK_TYPE_XDP, xdp)
 #ifdef CONFIG_PERF_EVENTS
 BPF_LINK_TYPE(BPF_LINK_TYPE_PERF_EVENT, perf)
 #endif
+BPF_LINK_TYPE(BPF_LINK_TYPE_STRUCT_OPS, struct_ops)
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 99fab54ae9c0..9e34da50440c 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -1011,6 +1011,7 @@ enum bpf_link_type {
 	BPF_LINK_TYPE_NETNS = 5,
 	BPF_LINK_TYPE_XDP = 6,
 	BPF_LINK_TYPE_PERF_EVENT = 7,
+	BPF_LINK_TYPE_STRUCT_OPS = 8,
 
 	MAX_BPF_LINK_TYPE,
 };
diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
index 21069dbe9138..53ac8137f137 100644
--- a/kernel/bpf/bpf_struct_ops.c
+++ b/kernel/bpf/bpf_struct_ops.c
@@ -32,15 +32,15 @@ struct bpf_struct_ops_map {
 	const struct bpf_struct_ops *st_ops;
 	/* protect map_update */
 	struct mutex lock;
-	/* progs has all the bpf_prog that is populated
+	/* link has all the bpf_links that is populated
 	 * to the func ptr of the kernel's struct
 	 * (in kvalue.data).
 	 */
-	struct bpf_prog **progs;
+	struct bpf_link **links;
 	/* image is a page that has all the trampolines
 	 * that stores the func args before calling the bpf_prog.
 	 * A PAGE_SIZE "image" is enough to store all trampoline for
-	 * "progs[]".
+	 * "links[]".
 	 */
 	void *image;
 	/* uvalue->data stores the kernel struct
@@ -282,9 +282,9 @@ static void bpf_struct_ops_map_put_progs(struct bpf_struct_ops_map *st_map)
 	u32 i;
 
 	for (i = 0; i < btf_type_vlen(t); i++) {
-		if (st_map->progs[i]) {
-			bpf_prog_put(st_map->progs[i]);
-			st_map->progs[i] = NULL;
+		if (st_map->links[i]) {
+			bpf_link_put(st_map->links[i]);
+			st_map->links[i] = NULL;
 		}
 	}
 }
@@ -315,18 +315,32 @@ static int check_zero_holes(const struct btf_type *t, void *data)
 	return 0;
 }
 
-int bpf_struct_ops_prepare_trampoline(struct bpf_tramp_progs *tprogs,
-				      struct bpf_prog *prog,
+static void bpf_struct_ops_link_release(struct bpf_link *link)
+{
+}
+
+static void bpf_struct_ops_link_dealloc(struct bpf_link *link)
+{
+	kfree(link);
+}
+
+static const struct bpf_link_ops bpf_struct_ops_link_lops = {
+	.release = bpf_struct_ops_link_release,
+	.dealloc = bpf_struct_ops_link_dealloc,
+};
+
+int bpf_struct_ops_prepare_trampoline(struct bpf_tramp_links *tlinks,
+				      struct bpf_link *link,
 				      const struct btf_func_model *model,
 				      void *image, void *image_end)
 {
 	u32 flags;
 
-	tprogs[BPF_TRAMP_FENTRY].progs[0] = prog;
-	tprogs[BPF_TRAMP_FENTRY].nr_progs = 1;
+	tlinks[BPF_TRAMP_FENTRY].links[0] = link;
+	tlinks[BPF_TRAMP_FENTRY].nr_links = 1;
 	flags = model->ret_size > 0 ? BPF_TRAMP_F_RET_FENTRY_RET : 0;
 	return arch_prepare_bpf_trampoline(NULL, image, image_end,
-					   model, flags, tprogs, NULL);
+					   model, flags, tlinks, NULL);
 }
 
 static int bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key,
@@ -337,7 +351,7 @@ static int bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key,
 	struct bpf_struct_ops_value *uvalue, *kvalue;
 	const struct btf_member *member;
 	const struct btf_type *t = st_ops->type;
-	struct bpf_tramp_progs *tprogs = NULL;
+	struct bpf_tramp_links *tlinks = NULL;
 	void *udata, *kdata;
 	int prog_fd, err = 0;
 	void *image, *image_end;
@@ -361,8 +375,8 @@ static int bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key,
 	if (uvalue->state || refcount_read(&uvalue->refcnt))
 		return -EINVAL;
 
-	tprogs = kcalloc(BPF_TRAMP_MAX, sizeof(*tprogs), GFP_KERNEL);
-	if (!tprogs)
+	tlinks = kcalloc(BPF_TRAMP_MAX, sizeof(*tlinks), GFP_KERNEL);
+	if (!tlinks)
 		return -ENOMEM;
 
 	uvalue = (struct bpf_struct_ops_value *)st_map->uvalue;
@@ -385,6 +399,7 @@ static int bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key,
 	for_each_member(i, t, member) {
 		const struct btf_type *mtype, *ptype;
 		struct bpf_prog *prog;
+		struct bpf_link *link;
 		u32 moff;
 
 		moff = __btf_member_bit_offset(t, member) / 8;
@@ -438,16 +453,25 @@ static int bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key,
 			err = PTR_ERR(prog);
 			goto reset_unlock;
 		}
-		st_map->progs[i] = prog;
 
 		if (prog->type != BPF_PROG_TYPE_STRUCT_OPS ||
 		    prog->aux->attach_btf_id != st_ops->type_id ||
 		    prog->expected_attach_type != i) {
+			bpf_prog_put(prog);
 			err = -EINVAL;
 			goto reset_unlock;
 		}
 
-		err = bpf_struct_ops_prepare_trampoline(tprogs, prog,
+		link = kzalloc(sizeof(*link), GFP_USER);
+		if (!link) {
+			bpf_prog_put(prog);
+			err = -ENOMEM;
+			goto reset_unlock;
+		}
+		bpf_link_init(link, BPF_LINK_TYPE_STRUCT_OPS, &bpf_struct_ops_link_lops, prog);
+		st_map->links[i] = link;
+
+		err = bpf_struct_ops_prepare_trampoline(tlinks, link,
 							&st_ops->func_models[i],
 							image, image_end);
 		if (err < 0)
@@ -490,7 +514,7 @@ static int bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key,
 	memset(uvalue, 0, map->value_size);
 	memset(kvalue, 0, map->value_size);
 unlock:
-	kfree(tprogs);
+	kfree(tlinks);
 	mutex_unlock(&st_map->lock);
 	return err;
 }
@@ -545,9 +569,9 @@ static void bpf_struct_ops_map_free(struct bpf_map *map)
 {
 	struct bpf_struct_ops_map *st_map = (struct bpf_struct_ops_map *)map;
 
-	if (st_map->progs)
+	if (st_map->links)
 		bpf_struct_ops_map_put_progs(st_map);
-	bpf_map_area_free(st_map->progs);
+	bpf_map_area_free(st_map->links);
 	bpf_jit_free_exec(st_map->image);
 	bpf_map_area_free(st_map->uvalue);
 	bpf_map_area_free(st_map);
@@ -596,11 +620,11 @@ static struct bpf_map *bpf_struct_ops_map_alloc(union bpf_attr *attr)
 	map = &st_map->map;
 
 	st_map->uvalue = bpf_map_area_alloc(vt->size, NUMA_NO_NODE);
-	st_map->progs =
-		bpf_map_area_alloc(btf_type_vlen(t) * sizeof(struct bpf_prog *),
+	st_map->links =
+		bpf_map_area_alloc(btf_type_vlen(t) * sizeof(struct bpf_links *),
 				   NUMA_NO_NODE);
 	st_map->image = bpf_jit_alloc_exec(PAGE_SIZE);
-	if (!st_map->uvalue || !st_map->progs || !st_map->image) {
+	if (!st_map->uvalue || !st_map->links || !st_map->image) {
 		bpf_struct_ops_map_free(map);
 		return ERR_PTR(-ENOMEM);
 	}
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 9beb585be5a6..fecfc803785d 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -2651,7 +2651,7 @@ static void bpf_tracing_link_release(struct bpf_link *link)
 	struct bpf_tracing_link *tr_link =
 		container_of(link, struct bpf_tracing_link, link);
 
-	WARN_ON_ONCE(bpf_trampoline_unlink_prog(link->prog,
+	WARN_ON_ONCE(bpf_trampoline_unlink_prog(link,
 						tr_link->trampoline));
 
 	bpf_trampoline_put(tr_link->trampoline);
@@ -2839,7 +2839,7 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog,
 	if (err)
 		goto out_unlock;
 
-	err = bpf_trampoline_link_prog(prog, tr);
+	err = bpf_trampoline_link_prog(&link->link, tr);
 	if (err) {
 		bpf_link_cleanup(&link_primer);
 		link = NULL;
diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
index 0b41fa993825..54c695d49ec9 100644
--- a/kernel/bpf/trampoline.c
+++ b/kernel/bpf/trampoline.c
@@ -180,30 +180,30 @@ static int register_fentry(struct bpf_trampoline *tr, void *new_addr)
 	return ret;
 }
 
-static struct bpf_tramp_progs *
+static struct bpf_tramp_links *
 bpf_trampoline_get_progs(const struct bpf_trampoline *tr, int *total, bool *ip_arg)
 {
-	const struct bpf_prog_aux *aux;
-	struct bpf_tramp_progs *tprogs;
-	struct bpf_prog **progs;
+	struct bpf_link *link;
+	struct bpf_tramp_links *tlinks;
+	struct bpf_link **links;
 	int kind;
 
 	*total = 0;
-	tprogs = kcalloc(BPF_TRAMP_MAX, sizeof(*tprogs), GFP_KERNEL);
-	if (!tprogs)
+	tlinks = kcalloc(BPF_TRAMP_MAX, sizeof(*tlinks), GFP_KERNEL);
+	if (!tlinks)
 		return ERR_PTR(-ENOMEM);
 
 	for (kind = 0; kind < BPF_TRAMP_MAX; kind++) {
-		tprogs[kind].nr_progs = tr->progs_cnt[kind];
+		tlinks[kind].nr_links = tr->progs_cnt[kind];
 		*total += tr->progs_cnt[kind];
-		progs = tprogs[kind].progs;
+		links = tlinks[kind].links;
 
-		hlist_for_each_entry(aux, &tr->progs_hlist[kind], tramp_hlist) {
-			*ip_arg |= aux->prog->call_get_func_ip;
-			*progs++ = aux->prog;
+		hlist_for_each_entry(link, &tr->progs_hlist[kind], tramp_hlist) {
+			*ip_arg |= link->prog->call_get_func_ip;
+			*links++ = link;
 		}
 	}
-	return tprogs;
+	return tlinks;
 }
 
 static void __bpf_tramp_image_put_deferred(struct work_struct *work)
@@ -342,14 +342,14 @@ static struct bpf_tramp_image *bpf_tramp_image_alloc(u64 key, u32 idx)
 static int bpf_trampoline_update(struct bpf_trampoline *tr)
 {
 	struct bpf_tramp_image *im;
-	struct bpf_tramp_progs *tprogs;
+	struct bpf_tramp_links *tlinks;
 	u32 flags = BPF_TRAMP_F_RESTORE_REGS;
 	bool ip_arg = false;
 	int err, total;
 
-	tprogs = bpf_trampoline_get_progs(tr, &total, &ip_arg);
-	if (IS_ERR(tprogs))
-		return PTR_ERR(tprogs);
+	tlinks = bpf_trampoline_get_progs(tr, &total, &ip_arg);
+	if (IS_ERR(tlinks))
+		return PTR_ERR(tlinks);
 
 	if (total == 0) {
 		err = unregister_fentry(tr, tr->cur_image->image);
@@ -365,15 +365,15 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr)
 		goto out;
 	}
 
-	if (tprogs[BPF_TRAMP_FEXIT].nr_progs ||
-	    tprogs[BPF_TRAMP_MODIFY_RETURN].nr_progs)
+	if (tlinks[BPF_TRAMP_FEXIT].nr_links ||
+	    tlinks[BPF_TRAMP_MODIFY_RETURN].nr_links)
 		flags = BPF_TRAMP_F_CALL_ORIG | BPF_TRAMP_F_SKIP_FRAME;
 
 	if (ip_arg)
 		flags |= BPF_TRAMP_F_IP_ARG;
 
 	err = arch_prepare_bpf_trampoline(im, im->image, im->image + PAGE_SIZE,
-					  &tr->func.model, flags, tprogs,
+					  &tr->func.model, flags, tlinks,
 					  tr->func.addr);
 	if (err < 0)
 		goto out;
@@ -393,7 +393,7 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr)
 	tr->cur_image = im;
 	tr->selector++;
 out:
-	kfree(tprogs);
+	kfree(tlinks);
 	return err;
 }
 
@@ -419,13 +419,14 @@ static enum bpf_tramp_prog_type bpf_attach_type_to_tramp(struct bpf_prog *prog)
 	}
 }
 
-int bpf_trampoline_link_prog(struct bpf_prog *prog, struct bpf_trampoline *tr)
+int bpf_trampoline_link_prog(struct bpf_link *link, struct bpf_trampoline *tr)
 {
 	enum bpf_tramp_prog_type kind;
+	struct bpf_link *link_exiting;
 	int err = 0;
 	int cnt;
 
-	kind = bpf_attach_type_to_tramp(prog);
+	kind = bpf_attach_type_to_tramp(link->prog);
 	mutex_lock(&tr->mutex);
 	if (tr->extension_prog) {
 		/* cannot attach fentry/fexit if extension prog is attached.
@@ -441,25 +442,33 @@ int bpf_trampoline_link_prog(struct bpf_prog *prog, struct bpf_trampoline *tr)
 			err = -EBUSY;
 			goto out;
 		}
-		tr->extension_prog = prog;
+		tr->extension_prog = link->prog;
 		err = bpf_arch_text_poke(tr->func.addr, BPF_MOD_JUMP, NULL,
-					 prog->bpf_func);
+					 link->prog->bpf_func);
 		goto out;
 	}
-	if (cnt >= BPF_MAX_TRAMP_PROGS) {
+	if (cnt >= BPF_MAX_TRAMP_LINKS) {
 		err = -E2BIG;
 		goto out;
 	}
-	if (!hlist_unhashed(&prog->aux->tramp_hlist)) {
+	if (!hlist_unhashed(&link->tramp_hlist)) {
 		/* prog already linked */
 		err = -EBUSY;
 		goto out;
 	}
-	hlist_add_head(&prog->aux->tramp_hlist, &tr->progs_hlist[kind]);
+	hlist_for_each_entry(link_exiting, &tr->progs_hlist[kind], tramp_hlist) {
+		if (link_exiting->prog != link->prog)
+			continue;
+		/* prog already linked */
+		err = -EBUSY;
+		goto out;
+	}
+
+	hlist_add_head(&link->tramp_hlist, &tr->progs_hlist[kind]);
 	tr->progs_cnt[kind]++;
 	err = bpf_trampoline_update(tr);
 	if (err) {
-		hlist_del_init(&prog->aux->tramp_hlist);
+		hlist_del_init(&link->tramp_hlist);
 		tr->progs_cnt[kind]--;
 	}
 out:
@@ -468,12 +477,12 @@ int bpf_trampoline_link_prog(struct bpf_prog *prog, struct bpf_trampoline *tr)
 }
 
 /* bpf_trampoline_unlink_prog() should never fail. */
-int bpf_trampoline_unlink_prog(struct bpf_prog *prog, struct bpf_trampoline *tr)
+int bpf_trampoline_unlink_prog(struct bpf_link *link, struct bpf_trampoline *tr)
 {
 	enum bpf_tramp_prog_type kind;
 	int err;
 
-	kind = bpf_attach_type_to_tramp(prog);
+	kind = bpf_attach_type_to_tramp(link->prog);
 	mutex_lock(&tr->mutex);
 	if (kind == BPF_TRAMP_REPLACE) {
 		WARN_ON_ONCE(!tr->extension_prog);
@@ -482,7 +491,7 @@ int bpf_trampoline_unlink_prog(struct bpf_prog *prog, struct bpf_trampoline *tr)
 		tr->extension_prog = NULL;
 		goto out;
 	}
-	hlist_del_init(&prog->aux->tramp_hlist);
+	hlist_del_init(&link->tramp_hlist);
 	tr->progs_cnt[kind]--;
 	err = bpf_trampoline_update(tr);
 out:
@@ -647,7 +656,7 @@ void notrace __bpf_tramp_exit(struct bpf_tramp_image *tr)
 int __weak
 arch_prepare_bpf_trampoline(struct bpf_tramp_image *tr, void *image, void *image_end,
 			    const struct btf_func_model *m, u32 flags,
-			    struct bpf_tramp_progs *tprogs,
+			    struct bpf_tramp_links *tlinks,
 			    void *orig_call)
 {
 	return -ENOTSUPP;
diff --git a/net/bpf/bpf_dummy_struct_ops.c b/net/bpf/bpf_dummy_struct_ops.c
index d0e54e30658a..268b62456420 100644
--- a/net/bpf/bpf_dummy_struct_ops.c
+++ b/net/bpf/bpf_dummy_struct_ops.c
@@ -72,13 +72,28 @@ static int dummy_ops_call_op(void *image, struct bpf_dummy_ops_test_args *args)
 		    args->args[3], args->args[4]);
 }
 
+static void bpf_struct_ops_link_release(struct bpf_link *link)
+{
+}
+
+static void bpf_struct_ops_link_dealloc(struct bpf_link *link)
+{
+	kfree(link);
+}
+
+static const struct bpf_link_ops bpf_struct_ops_link_lops = {
+	.release = bpf_struct_ops_link_release,
+	.dealloc = bpf_struct_ops_link_dealloc,
+};
+
 int bpf_struct_ops_test_run(struct bpf_prog *prog, const union bpf_attr *kattr,
 			    union bpf_attr __user *uattr)
 {
 	const struct bpf_struct_ops *st_ops = &bpf_bpf_dummy_ops;
 	const struct btf_type *func_proto;
 	struct bpf_dummy_ops_test_args *args;
-	struct bpf_tramp_progs *tprogs;
+	struct bpf_tramp_links *tlinks;
+	struct bpf_link *link = NULL;
 	void *image = NULL;
 	unsigned int op_idx;
 	int prog_ret;
@@ -92,8 +107,8 @@ int bpf_struct_ops_test_run(struct bpf_prog *prog, const union bpf_attr *kattr,
 	if (IS_ERR(args))
 		return PTR_ERR(args);
 
-	tprogs = kcalloc(BPF_TRAMP_MAX, sizeof(*tprogs), GFP_KERNEL);
-	if (!tprogs) {
+	tlinks = kcalloc(BPF_TRAMP_MAX, sizeof(*tlinks), GFP_KERNEL);
+	if (!tlinks) {
 		err = -ENOMEM;
 		goto out;
 	}
@@ -105,10 +120,18 @@ int bpf_struct_ops_test_run(struct bpf_prog *prog, const union bpf_attr *kattr,
 	}
 	set_vm_flush_reset_perms(image);
 
+	link = kzalloc(sizeof(*link), GFP_USER);
+	if (!link) {
+		err = -ENOMEM;
+		goto out;
+	}
+	bpf_link_init(link, BPF_LINK_TYPE_STRUCT_OPS, &bpf_struct_ops_link_lops, prog);
+
 	op_idx = prog->expected_attach_type;
-	err = bpf_struct_ops_prepare_trampoline(tprogs, prog,
+	err = bpf_struct_ops_prepare_trampoline(tlinks, link,
 						&st_ops->func_models[op_idx],
 						image, image + PAGE_SIZE);
+
 	if (err < 0)
 		goto out;
 
@@ -124,7 +147,9 @@ int bpf_struct_ops_test_run(struct bpf_prog *prog, const union bpf_attr *kattr,
 out:
 	kfree(args);
 	bpf_jit_free_exec(image);
-	kfree(tprogs);
+	if (link)
+		bpf_link_put(link);
+	kfree(tlinks);
 	return err;
 }
 
diff --git a/tools/bpf/bpftool/link.c b/tools/bpf/bpftool/link.c
index 97dec81950e5..d49a2bdc983f 100644
--- a/tools/bpf/bpftool/link.c
+++ b/tools/bpf/bpftool/link.c
@@ -20,6 +20,7 @@ static const char * const link_type_name[] = {
 	[BPF_LINK_TYPE_CGROUP]			= "cgroup",
 	[BPF_LINK_TYPE_ITER]			= "iter",
 	[BPF_LINK_TYPE_NETNS]			= "netns",
+	[BPF_LINK_TYPE_STRUCT_OPS]               = "struct_ops",
 };
 
 static struct hashmap *link_table;
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 99fab54ae9c0..9e34da50440c 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -1011,6 +1011,7 @@ enum bpf_link_type {
 	BPF_LINK_TYPE_NETNS = 5,
 	BPF_LINK_TYPE_XDP = 6,
 	BPF_LINK_TYPE_PERF_EVENT = 7,
+	BPF_LINK_TYPE_STRUCT_OPS = 8,
 
 	MAX_BPF_LINK_TYPE,
 };
-- 
2.30.2

[PATCH bpf-next v2 2/4] bpf, x86: Create bpf_trace_run_ctx on the caller thread's stack

[Kui-Feng Lee]

BPF trampolines will create a bpf_trace_run_ctx on their stacks, and
set/reset the current bpf_run_ctx whenever calling/returning from a
bpf_prog.

Signed-off-by: Kui-Feng Lee <kuifeng@fb.com>
---
 arch/x86/net/bpf_jit_comp.c | 32 ++++++++++++++++++++++++++++++++
 include/linux/bpf.h         | 12 ++++++++----
 kernel/bpf/syscall.c        |  4 ++--
 kernel/bpf/trampoline.c     | 21 +++++++++++++++++----
 4 files changed, 59 insertions(+), 10 deletions(-)

diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index 1228e6e6a420..29775a475513 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -1748,10 +1748,33 @@ static int invoke_bpf_prog(const struct btf_func_model *m, u8 **pprog,
 {
 	u8 *prog = *pprog;
 	u8 *jmp_insn;
+	int ctx_cookie_off = offsetof(struct bpf_trace_run_ctx, bpf_cookie);
 	struct bpf_prog *p = l->prog;
 
+	EMIT1(0x52);		 /* push rdx */
+
+	/* mov rdi, 0 */
+	emit_mov_imm64(&prog, BPF_REG_1, 0, 0);
+
+	/* Prepare struct bpf_trace_run_ctx.
+	 * sub rsp, sizeof(struct bpf_trace_run_ctx)
+	 * mov rax, rsp
+	 * mov QWORD PTR [rax + ctx_cookie_off], rdi
+	 */
+	EMIT4(0x48, 0x83, 0xEC, sizeof(struct bpf_trace_run_ctx));
+	EMIT3(0x48, 0x89, 0xE0);
+	EMIT4(0x48, 0x89, 0x78, ctx_cookie_off);
+
+	/* mov rdi, rsp */
+	EMIT3(0x48, 0x89, 0xE7);
+	/* mov QWORD PTR [rdi + sizeof(struct bpf_trace_run_ctx)], rax */
+	emit_stx(&prog, BPF_DW, BPF_REG_1, BPF_REG_0, sizeof(struct bpf_trace_run_ctx));
+
 	/* arg1: mov rdi, progs[i] */
 	emit_mov_imm64(&prog, BPF_REG_1, (long) p >> 32, (u32) (long) p);
+	/* arg2: mov rsi, rsp (struct bpf_run_ctx *) */
+	EMIT3(0x48, 0x89, 0xE6);
+
 	if (emit_call(&prog,
 		      p->aux->sleepable ? __bpf_prog_enter_sleepable :
 		      __bpf_prog_enter, prog))
@@ -1797,11 +1820,20 @@ static int invoke_bpf_prog(const struct btf_func_model *m, u8 **pprog,
 	emit_mov_imm64(&prog, BPF_REG_1, (long) p >> 32, (u32) (long) p);
 	/* arg2: mov rsi, rbx <- start time in nsec */
 	emit_mov_reg(&prog, true, BPF_REG_2, BPF_REG_6);
+	/* arg3: mov rdx, rsp (struct bpf_run_ctx *) */
+	EMIT3(0x48, 0x89, 0xE2);
 	if (emit_call(&prog,
 		      p->aux->sleepable ? __bpf_prog_exit_sleepable :
 		      __bpf_prog_exit, prog))
 			return -EINVAL;
 
+	/* pop struct bpf_trace_run_ctx
+	 * add rsp, sizeof(struct bpf_trace_run_ctx)
+	 */
+	EMIT4(0x48, 0x83, 0xC4, sizeof(struct bpf_trace_run_ctx));
+
+	EMIT1(0x5A); /* pop rdx */
+
 	*pprog = prog;
 	return 0;
 }
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 3dcae8550c21..d20a23953696 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -681,6 +681,8 @@ struct bpf_tramp_links {
 	int nr_links;
 };
 
+struct bpf_trace_run_ctx;
+
 /* Different use cases for BPF trampoline:
  * 1. replace nop at the function entry (kprobe equivalent)
  *    flags = BPF_TRAMP_F_RESTORE_REGS
@@ -707,10 +709,11 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *tr, void *image, void *i
 				struct bpf_tramp_links *tlinks,
 				void *orig_call);
 /* these two functions are called from generated trampoline */
-u64 notrace __bpf_prog_enter(struct bpf_prog *prog);
-void notrace __bpf_prog_exit(struct bpf_prog *prog, u64 start);
-u64 notrace __bpf_prog_enter_sleepable(struct bpf_prog *prog);
-void notrace __bpf_prog_exit_sleepable(struct bpf_prog *prog, u64 start);
+u64 notrace __bpf_prog_enter(struct bpf_prog *prog, struct bpf_trace_run_ctx *run_ctx);
+void notrace __bpf_prog_exit(struct bpf_prog *prog, u64 start, struct bpf_trace_run_ctx *run_ctx);
+u64 notrace __bpf_prog_enter_sleepable(struct bpf_prog *prog, struct bpf_trace_run_ctx *run_ctx);
+void notrace __bpf_prog_exit_sleepable(struct bpf_prog *prog, u64 start,
+				       struct bpf_trace_run_ctx *run_ctx);
 void notrace __bpf_tramp_enter(struct bpf_tramp_image *tr);
 void notrace __bpf_tramp_exit(struct bpf_tramp_image *tr);
 
@@ -1291,6 +1294,7 @@ struct bpf_cg_run_ctx {
 struct bpf_trace_run_ctx {
 	struct bpf_run_ctx run_ctx;
 	u64 bpf_cookie;
+	struct bpf_run_ctx *saved_run_ctx;
 };
 
 static inline struct bpf_run_ctx *bpf_set_run_ctx(struct bpf_run_ctx *new_ctx)
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index fecfc803785d..a289ef55ea17 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -4793,13 +4793,13 @@ BPF_CALL_3(bpf_sys_bpf, int, cmd, union bpf_attr *, attr, u32, attr_size)
 			return -EINVAL;
 		}
 
-		if (!__bpf_prog_enter_sleepable(prog)) {
+		if (!__bpf_prog_enter_sleepable(prog, NULL)) {
 			/* recursion detected */
 			bpf_prog_put(prog);
 			return -EBUSY;
 		}
 		attr->test.retval = bpf_prog_run(prog, (void *) (long) attr->test.ctx_in);
-		__bpf_prog_exit_sleepable(prog, 0 /* bpf_prog_run does runtime stats */);
+		__bpf_prog_exit_sleepable(prog, 0 /* bpf_prog_run does runtime stats */, NULL);
 		bpf_prog_put(prog);
 		return 0;
 #endif
diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
index 54c695d49ec9..0b050aa2f159 100644
--- a/kernel/bpf/trampoline.c
+++ b/kernel/bpf/trampoline.c
@@ -580,9 +580,12 @@ static void notrace inc_misses_counter(struct bpf_prog *prog)
  * [2..MAX_U64] - execute bpf prog and record execution time.
  *     This is start time.
  */
-u64 notrace __bpf_prog_enter(struct bpf_prog *prog)
+u64 notrace __bpf_prog_enter(struct bpf_prog *prog, struct bpf_trace_run_ctx *run_ctx)
 	__acquires(RCU)
 {
+	if (run_ctx)
+		run_ctx->saved_run_ctx = bpf_set_run_ctx(&run_ctx->run_ctx);
+
 	rcu_read_lock();
 	migrate_disable();
 	if (unlikely(__this_cpu_inc_return(*(prog->active)) != 1)) {
@@ -614,17 +617,23 @@ static void notrace update_prog_stats(struct bpf_prog *prog,
 	}
 }
 
-void notrace __bpf_prog_exit(struct bpf_prog *prog, u64 start)
+void notrace __bpf_prog_exit(struct bpf_prog *prog, u64 start, struct bpf_trace_run_ctx *run_ctx)
 	__releases(RCU)
 {
+	if (run_ctx)
+		bpf_reset_run_ctx(run_ctx->saved_run_ctx);
+
 	update_prog_stats(prog, start);
 	__this_cpu_dec(*(prog->active));
 	migrate_enable();
 	rcu_read_unlock();
 }
 
-u64 notrace __bpf_prog_enter_sleepable(struct bpf_prog *prog)
+u64 notrace __bpf_prog_enter_sleepable(struct bpf_prog *prog, struct bpf_trace_run_ctx *run_ctx)
 {
+	if (run_ctx)
+		run_ctx->saved_run_ctx = bpf_set_run_ctx(&run_ctx->run_ctx);
+
 	rcu_read_lock_trace();
 	migrate_disable();
 	might_fault();
@@ -635,8 +644,12 @@ u64 notrace __bpf_prog_enter_sleepable(struct bpf_prog *prog)
 	return bpf_prog_start_time();
 }
 
-void notrace __bpf_prog_exit_sleepable(struct bpf_prog *prog, u64 start)
+void notrace __bpf_prog_exit_sleepable(struct bpf_prog *prog, u64 start,
+				       struct bpf_trace_run_ctx *run_ctx)
 {
+	if (run_ctx)
+		bpf_reset_run_ctx(run_ctx->saved_run_ctx);
+
 	update_prog_stats(prog, start);
 	__this_cpu_dec(*(prog->active));
 	migrate_enable();
-- 
2.30.2

Re: [PATCH bpf-next v2 2/4] bpf, x86: Create bpf_trace_run_ctx on the caller thread's stack

[Alexei Starovoitov]

On Tue, Mar 15, 2022 at 05:42:29PM -0700, Kui-Feng Lee wrote:
> BPF trampolines will create a bpf_trace_run_ctx on their stacks, and
> set/reset the current bpf_run_ctx whenever calling/returning from a
> bpf_prog.
> 
> Signed-off-by: Kui-Feng Lee <kuifeng@fb.com>
> ---
>  arch/x86/net/bpf_jit_comp.c | 32 ++++++++++++++++++++++++++++++++
>  include/linux/bpf.h         | 12 ++++++++----
>  kernel/bpf/syscall.c        |  4 ++--
>  kernel/bpf/trampoline.c     | 21 +++++++++++++++++----
>  4 files changed, 59 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> index 1228e6e6a420..29775a475513 100644
> --- a/arch/x86/net/bpf_jit_comp.c
> +++ b/arch/x86/net/bpf_jit_comp.c
> @@ -1748,10 +1748,33 @@ static int invoke_bpf_prog(const struct btf_func_model *m, u8 **pprog,
>  {
>  	u8 *prog = *pprog;
>  	u8 *jmp_insn;
> +	int ctx_cookie_off = offsetof(struct bpf_trace_run_ctx, bpf_cookie);
>  	struct bpf_prog *p = l->prog;
>  
> +	EMIT1(0x52);		 /* push rdx */

Why save/restore rdx?

> +
> +	/* mov rdi, 0 */
> +	emit_mov_imm64(&prog, BPF_REG_1, 0, 0);
> +
> +	/* Prepare struct bpf_trace_run_ctx.
> +	 * sub rsp, sizeof(struct bpf_trace_run_ctx)
> +	 * mov rax, rsp
> +	 * mov QWORD PTR [rax + ctx_cookie_off], rdi
> +	 */

How about the following instead:
sub rsp, sizeof(struct bpf_trace_run_ctx)
mov qword ptr [rsp + ctx_cookie_off], 0
?

> +	EMIT4(0x48, 0x83, 0xEC, sizeof(struct bpf_trace_run_ctx));
> +	EMIT3(0x48, 0x89, 0xE0);
> +	EMIT4(0x48, 0x89, 0x78, ctx_cookie_off);
> +
> +	/* mov rdi, rsp */
> +	EMIT3(0x48, 0x89, 0xE7);
> +	/* mov QWORD PTR [rdi + sizeof(struct bpf_trace_run_ctx)], rax */
> +	emit_stx(&prog, BPF_DW, BPF_REG_1, BPF_REG_0, sizeof(struct bpf_trace_run_ctx));

why not to do:
mov qword ptr[rsp + sizeof(struct bpf_trace_run_ctx)], rsp
?

> +
>  	/* arg1: mov rdi, progs[i] */
>  	emit_mov_imm64(&prog, BPF_REG_1, (long) p >> 32, (u32) (long) p);
> +	/* arg2: mov rsi, rsp (struct bpf_run_ctx *) */
> +	EMIT3(0x48, 0x89, 0xE6);
> +
>  	if (emit_call(&prog,
>  		      p->aux->sleepable ? __bpf_prog_enter_sleepable :
>  		      __bpf_prog_enter, prog))
> @@ -1797,11 +1820,20 @@ static int invoke_bpf_prog(const struct btf_func_model *m, u8 **pprog,
>  	emit_mov_imm64(&prog, BPF_REG_1, (long) p >> 32, (u32) (long) p);
>  	/* arg2: mov rsi, rbx <- start time in nsec */
>  	emit_mov_reg(&prog, true, BPF_REG_2, BPF_REG_6);
> +	/* arg3: mov rdx, rsp (struct bpf_run_ctx *) */
> +	EMIT3(0x48, 0x89, 0xE2);
>  	if (emit_call(&prog,
>  		      p->aux->sleepable ? __bpf_prog_exit_sleepable :
>  		      __bpf_prog_exit, prog))
>  			return -EINVAL;
>  
> +	/* pop struct bpf_trace_run_ctx
> +	 * add rsp, sizeof(struct bpf_trace_run_ctx)
> +	 */
> +	EMIT4(0x48, 0x83, 0xC4, sizeof(struct bpf_trace_run_ctx));

the sub rsp; add rsp pair for every prog call will add up.
Could you do sub rsp once at the beginning of trampoline?
And move
mov qword ptr[rsp + sizeof(struct bpf_trace_run_ctx)], rsp
to the beginning as well?

> +
> +	EMIT1(0x5A); /* pop rdx */
> +
>  	*pprog = prog;
>  	return 0;
>  }
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 3dcae8550c21..d20a23953696 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -681,6 +681,8 @@ struct bpf_tramp_links {
>  	int nr_links;
>  };
>  
> +struct bpf_trace_run_ctx;
> +
>  /* Different use cases for BPF trampoline:
>   * 1. replace nop at the function entry (kprobe equivalent)
>   *    flags = BPF_TRAMP_F_RESTORE_REGS
> @@ -707,10 +709,11 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *tr, void *image, void *i
>  				struct bpf_tramp_links *tlinks,
>  				void *orig_call);
>  /* these two functions are called from generated trampoline */
> -u64 notrace __bpf_prog_enter(struct bpf_prog *prog);
> -void notrace __bpf_prog_exit(struct bpf_prog *prog, u64 start);
> -u64 notrace __bpf_prog_enter_sleepable(struct bpf_prog *prog);
> -void notrace __bpf_prog_exit_sleepable(struct bpf_prog *prog, u64 start);
> +u64 notrace __bpf_prog_enter(struct bpf_prog *prog, struct bpf_trace_run_ctx *run_ctx);
> +void notrace __bpf_prog_exit(struct bpf_prog *prog, u64 start, struct bpf_trace_run_ctx *run_ctx);
> +u64 notrace __bpf_prog_enter_sleepable(struct bpf_prog *prog, struct bpf_trace_run_ctx *run_ctx);
> +void notrace __bpf_prog_exit_sleepable(struct bpf_prog *prog, u64 start,
> +				       struct bpf_trace_run_ctx *run_ctx);
>  void notrace __bpf_tramp_enter(struct bpf_tramp_image *tr);
>  void notrace __bpf_tramp_exit(struct bpf_tramp_image *tr);
>  
> @@ -1291,6 +1294,7 @@ struct bpf_cg_run_ctx {
>  struct bpf_trace_run_ctx {
>  	struct bpf_run_ctx run_ctx;
>  	u64 bpf_cookie;
> +	struct bpf_run_ctx *saved_run_ctx;
>  };
>  
>  static inline struct bpf_run_ctx *bpf_set_run_ctx(struct bpf_run_ctx *new_ctx)
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index fecfc803785d..a289ef55ea17 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -4793,13 +4793,13 @@ BPF_CALL_3(bpf_sys_bpf, int, cmd, union bpf_attr *, attr, u32, attr_size)
>  			return -EINVAL;
>  		}
>  
> -		if (!__bpf_prog_enter_sleepable(prog)) {
> +		if (!__bpf_prog_enter_sleepable(prog, NULL)) {
>  			/* recursion detected */
>  			bpf_prog_put(prog);
>  			return -EBUSY;
>  		}
>  		attr->test.retval = bpf_prog_run(prog, (void *) (long) attr->test.ctx_in);
> -		__bpf_prog_exit_sleepable(prog, 0 /* bpf_prog_run does runtime stats */);
> +		__bpf_prog_exit_sleepable(prog, 0 /* bpf_prog_run does runtime stats */, NULL);
>  		bpf_prog_put(prog);
>  		return 0;
>  #endif
> diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
> index 54c695d49ec9..0b050aa2f159 100644
> --- a/kernel/bpf/trampoline.c
> +++ b/kernel/bpf/trampoline.c
> @@ -580,9 +580,12 @@ static void notrace inc_misses_counter(struct bpf_prog *prog)
>   * [2..MAX_U64] - execute bpf prog and record execution time.
>   *     This is start time.
>   */
> -u64 notrace __bpf_prog_enter(struct bpf_prog *prog)
> +u64 notrace __bpf_prog_enter(struct bpf_prog *prog, struct bpf_trace_run_ctx *run_ctx)
>  	__acquires(RCU)
>  {
> +	if (run_ctx)
> +		run_ctx->saved_run_ctx = bpf_set_run_ctx(&run_ctx->run_ctx);

We can remove these branches from critical path if we path actual run_ctx
instead of NULL in bpf_sys_bpf, right?

> +
>  	rcu_read_lock();
>  	migrate_disable();
>  	if (unlikely(__this_cpu_inc_return(*(prog->active)) != 1)) {
> @@ -614,17 +617,23 @@ static void notrace update_prog_stats(struct bpf_prog *prog,
>  	}
>  }
>  
> -void notrace __bpf_prog_exit(struct bpf_prog *prog, u64 start)
> +void notrace __bpf_prog_exit(struct bpf_prog *prog, u64 start, struct bpf_trace_run_ctx *run_ctx)
>  	__releases(RCU)
>  {
> +	if (run_ctx)
> +		bpf_reset_run_ctx(run_ctx->saved_run_ctx);
> +
>  	update_prog_stats(prog, start);
>  	__this_cpu_dec(*(prog->active));
>  	migrate_enable();
>  	rcu_read_unlock();
>  }
>  
> -u64 notrace __bpf_prog_enter_sleepable(struct bpf_prog *prog)
> +u64 notrace __bpf_prog_enter_sleepable(struct bpf_prog *prog, struct bpf_trace_run_ctx *run_ctx)
>  {
> +	if (run_ctx)
> +		run_ctx->saved_run_ctx = bpf_set_run_ctx(&run_ctx->run_ctx);
> +
>  	rcu_read_lock_trace();
>  	migrate_disable();
>  	might_fault();
> @@ -635,8 +644,12 @@ u64 notrace __bpf_prog_enter_sleepable(struct bpf_prog *prog)
>  	return bpf_prog_start_time();
>  }
>  
> -void notrace __bpf_prog_exit_sleepable(struct bpf_prog *prog, u64 start)
> +void notrace __bpf_prog_exit_sleepable(struct bpf_prog *prog, u64 start,
> +				       struct bpf_trace_run_ctx *run_ctx)
>  {
> +	if (run_ctx)
> +		bpf_reset_run_ctx(run_ctx->saved_run_ctx);
> +
>  	update_prog_stats(prog, start);
>  	__this_cpu_dec(*(prog->active));
>  	migrate_enable();
> -- 
> 2.30.2
> 

-- 

Re: [PATCH bpf-next v2 2/4] bpf, x86: Create bpf_trace_run_ctx on the caller thread's stack

[Kui-Feng Lee]

On Fri, 2022-03-18 at 12:09 -0700, Alexei Starovoitov wrote:
> On Tue, Mar 15, 2022 at 05:42:29PM -0700, Kui-Feng Lee wrote:
> > BPF trampolines will create a bpf_trace_run_ctx on their stacks,
> > and
> > set/reset the current bpf_run_ctx whenever calling/returning from a
> > bpf_prog.
> > 
> > Signed-off-by: Kui-Feng Lee <kuifeng@fb.com>
> > ---
> >  arch/x86/net/bpf_jit_comp.c | 32 ++++++++++++++++++++++++++++++++
> >  include/linux/bpf.h         | 12 ++++++++----
> >  kernel/bpf/syscall.c        |  4 ++--
> >  kernel/bpf/trampoline.c     | 21 +++++++++++++++++----
> >  4 files changed, 59 insertions(+), 10 deletions(-)
> > 
> > diff --git a/arch/x86/net/bpf_jit_comp.c
> > b/arch/x86/net/bpf_jit_comp.c
> > index 1228e6e6a420..29775a475513 100644
> > --- a/arch/x86/net/bpf_jit_comp.c
> > +++ b/arch/x86/net/bpf_jit_comp.c
> > @@ -1748,10 +1748,33 @@ static int invoke_bpf_prog(const struct
> > btf_func_model *m, u8 **pprog,
> >  {
> >         u8 *prog = *pprog;
> >         u8 *jmp_insn;
> > +       int ctx_cookie_off = offsetof(struct bpf_trace_run_ctx,
> > bpf_cookie);
> >         struct bpf_prog *p = l->prog;
> >  
> > +       EMIT1(0x52);             /* push rdx */
> 
> Why save/restore rdx?

> 
> > +
> > +       /* mov rdi, 0 */
> > +       emit_mov_imm64(&prog, BPF_REG_1, 0, 0);
> > +
> > +       /* Prepare struct bpf_trace_run_ctx.
> > +        * sub rsp, sizeof(struct bpf_trace_run_ctx)
> > +        * mov rax, rsp
> > +        * mov QWORD PTR [rax + ctx_cookie_off], rdi
> > +        */
> 
> How about the following instead:
> sub rsp, sizeof(struct bpf_trace_run_ctx)
> mov qword ptr [rsp + ctx_cookie_off], 0
> ?

AFAIK, rsp can not be used with the base + displacement addressing
mode.  Although, it can be used with base + index + displacement
addressing mode.

> 
> > +       EMIT4(0x48, 0x83, 0xEC, sizeof(struct bpf_trace_run_ctx));
> > +       EMIT3(0x48, 0x89, 0xE0);
> > +       EMIT4(0x48, 0x89, 0x78, ctx_cookie_off);
> > +
> > +       /* mov rdi, rsp */
> > +       EMIT3(0x48, 0x89, 0xE7);
> > +       /* mov QWORD PTR [rdi + sizeof(struct bpf_trace_run_ctx)],
> > rax */
> > +       emit_stx(&prog, BPF_DW, BPF_REG_1, BPF_REG_0, sizeof(struct
> > bpf_trace_run_ctx));
> 
> why not to do:
> mov qword ptr[rsp + sizeof(struct bpf_trace_run_ctx)], rsp
> ?

The same reason as above.

> 
> > +
> >         /* arg1: mov rdi, progs[i] */
> >         emit_mov_imm64(&prog, BPF_REG_1, (long) p >> 32, (u32)
> > (long) p);
> > +       /* arg2: mov rsi, rsp (struct bpf_run_ctx *) */
> > +       EMIT3(0x48, 0x89, 0xE6);
> > +
> >         if (emit_call(&prog,
> >                       p->aux->sleepable ?
> > __bpf_prog_enter_sleepable :
> >                       __bpf_prog_enter, prog))
> > @@ -1797,11 +1820,20 @@ static int invoke_bpf_prog(const struct
> > btf_func_model *m, u8 **pprog,
> >         emit_mov_imm64(&prog, BPF_REG_1, (long) p >> 32, (u32)
> > (long) p);
> >         /* arg2: mov rsi, rbx <- start time in nsec */
> >         emit_mov_reg(&prog, true, BPF_REG_2, BPF_REG_6);
> > +       /* arg3: mov rdx, rsp (struct bpf_run_ctx *) */
> > +       EMIT3(0x48, 0x89, 0xE2);
> >         if (emit_call(&prog,
> >                       p->aux->sleepable ? __bpf_prog_exit_sleepable
> > :
> >                       __bpf_prog_exit, prog))
> >                         return -EINVAL;
> >  
> > +       /* pop struct bpf_trace_run_ctx
> > +        * add rsp, sizeof(struct bpf_trace_run_ctx)
> > +        */
> > +       EMIT4(0x48, 0x83, 0xC4, sizeof(struct bpf_trace_run_ctx));
> 
> the sub rsp; add rsp pair for every prog call will add up.
> Could you do sub rsp once at the beginning of trampoline?
> And move
> mov qword ptr[rsp + sizeof(struct bpf_trace_run_ctx)], rsp
> to the beginning as well?

Ok! I will move this part.

> 
> > +
> > +       EMIT1(0x5A); /* pop rdx */
> > +
> >         *pprog = prog;
> >         return 0;
> >  }
> > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > index 3dcae8550c21..d20a23953696 100644
> > --- a/include/linux/bpf.h
> > +++ b/include/linux/bpf.h
> > @@ -681,6 +681,8 @@ struct bpf_tramp_links {
> >         int nr_links;
> >  };
> >  
> > +struct bpf_trace_run_ctx;
> > +
> >  /* Different use cases for BPF trampoline:
> >   * 1. replace nop at the function entry (kprobe equivalent)
> >   *    flags = BPF_TRAMP_F_RESTORE_REGS
> > @@ -707,10 +709,11 @@ int arch_prepare_bpf_trampoline(struct
> > bpf_tramp_image *tr, void *image, void *i
> >                                 struct bpf_tramp_links *tlinks,
> >                                 void *orig_call);
> >  /* these two functions are called from generated trampoline */
> > -u64 notrace __bpf_prog_enter(struct bpf_prog *prog);
> > -void notrace __bpf_prog_exit(struct bpf_prog *prog, u64 start);
> > -u64 notrace __bpf_prog_enter_sleepable(struct bpf_prog *prog);
> > -void notrace __bpf_prog_exit_sleepable(struct bpf_prog *prog, u64
> > start);
> > +u64 notrace __bpf_prog_enter(struct bpf_prog *prog, struct
> > bpf_trace_run_ctx *run_ctx);
> > +void notrace __bpf_prog_exit(struct bpf_prog *prog, u64 start,
> > struct bpf_trace_run_ctx *run_ctx);
> > +u64 notrace __bpf_prog_enter_sleepable(struct bpf_prog *prog,
> > struct bpf_trace_run_ctx *run_ctx);
> > +void notrace __bpf_prog_exit_sleepable(struct bpf_prog *prog, u64
> > start,
> > +                                      struct bpf_trace_run_ctx
> > *run_ctx);
> >  void notrace __bpf_tramp_enter(struct bpf_tramp_image *tr);
> >  void notrace __bpf_tramp_exit(struct bpf_tramp_image *tr);
> >  
> > @@ -1291,6 +1294,7 @@ struct bpf_cg_run_ctx {
> >  struct bpf_trace_run_ctx {
> >         struct bpf_run_ctx run_ctx;
> >         u64 bpf_cookie;
> > +       struct bpf_run_ctx *saved_run_ctx;
> >  };
> >  
> >  static inline struct bpf_run_ctx *bpf_set_run_ctx(struct
> > bpf_run_ctx *new_ctx)
> > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> > index fecfc803785d..a289ef55ea17 100644
> > --- a/kernel/bpf/syscall.c
> > +++ b/kernel/bpf/syscall.c
> > @@ -4793,13 +4793,13 @@ BPF_CALL_3(bpf_sys_bpf, int, cmd, union
> > bpf_attr *, attr, u32, attr_size)
> >                         return -EINVAL;
> >                 }
> >  
> > -               if (!__bpf_prog_enter_sleepable(prog)) {
> > +               if (!__bpf_prog_enter_sleepable(prog, NULL)) {
> >                         /* recursion detected */
> >                         bpf_prog_put(prog);
> >                         return -EBUSY;
> >                 }
> >                 attr->test.retval = bpf_prog_run(prog, (void *)
> > (long) attr->test.ctx_in);
> > -               __bpf_prog_exit_sleepable(prog, 0 /* bpf_prog_run
> > does runtime stats */);
> > +               __bpf_prog_exit_sleepable(prog, 0 /* bpf_prog_run
> > does runtime stats */, NULL);
> >                 bpf_prog_put(prog);
> >                 return 0;
> >  #endif
> > diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
> > index 54c695d49ec9..0b050aa2f159 100644
> > --- a/kernel/bpf/trampoline.c
> > +++ b/kernel/bpf/trampoline.c
> > @@ -580,9 +580,12 @@ static void notrace inc_misses_counter(struct
> > bpf_prog *prog)
> >   * [2..MAX_U64] - execute bpf prog and record execution time.
> >   *     This is start time.
> >   */
> > -u64 notrace __bpf_prog_enter(struct bpf_prog *prog)
> > +u64 notrace __bpf_prog_enter(struct bpf_prog *prog, struct
> > bpf_trace_run_ctx *run_ctx)
> >         __acquires(RCU)
> >  {
> > +       if (run_ctx)
> > +               run_ctx->saved_run_ctx = bpf_set_run_ctx(&run_ctx-
> > >run_ctx);
> 
> We can remove these branches from critical path if we path actual
> run_ctx
> instead of NULL in bpf_sys_bpf, right?

Yes!

> 
> > +
> >         rcu_read_lock();
> >         migrate_disable();
> >         if (unlikely(__this_cpu_inc_return(*(prog->active)) != 1))
> > {
> > @@ -614,17 +617,23 @@ static void notrace update_prog_stats(struct
> > bpf_prog *prog,
> >         }
> >  }
> >  
> > -void notrace __bpf_prog_exit(struct bpf_prog *prog, u64 start)
> > +void notrace __bpf_prog_exit(struct bpf_prog *prog, u64 start,
> > struct bpf_trace_run_ctx *run_ctx)
> >         __releases(RCU)
> >  {
> > +       if (run_ctx)
> > +               bpf_reset_run_ctx(run_ctx->saved_run_ctx);
> > +
> >         update_prog_stats(prog, start);
> >         __this_cpu_dec(*(prog->active));
> >         migrate_enable();
> >         rcu_read_unlock();
> >  }
> >  
> > -u64 notrace __bpf_prog_enter_sleepable(struct bpf_prog *prog)
> > +u64 notrace __bpf_prog_enter_sleepable(struct bpf_prog *prog,
> > struct bpf_trace_run_ctx *run_ctx)
> >  {
> > +       if (run_ctx)
> > +               run_ctx->saved_run_ctx = bpf_set_run_ctx(&run_ctx-
> > >run_ctx);
> > +
> >         rcu_read_lock_trace();
> >         migrate_disable();
> >         might_fault();
> > @@ -635,8 +644,12 @@ u64 notrace __bpf_prog_enter_sleepable(struct
> > bpf_prog *prog)
> >         return bpf_prog_start_time();
> >  }
> >  
> > -void notrace __bpf_prog_exit_sleepable(struct bpf_prog *prog, u64
> > start)
> > +void notrace __bpf_prog_exit_sleepable(struct bpf_prog *prog, u64
> > start,
> > +                                      struct bpf_trace_run_ctx
> > *run_ctx)
> >  {
> > +       if (run_ctx)
> > +               bpf_reset_run_ctx(run_ctx->saved_run_ctx);
> > +
> >         update_prog_stats(prog, start);
> >         __this_cpu_dec(*(prog->active));
> >         migrate_enable();
> > -- 
> > 2.30.2
> > 
> 

Re: [PATCH bpf-next v2 2/4] bpf, x86: Create bpf_trace_run_ctx on the caller thread's stack

[Alexei Starovoitov]

On Sun, Mar 20, 2022 at 2:31 AM Kui-Feng Lee <kuifeng@fb.com> wrote:
>
> On Fri, 2022-03-18 at 12:09 -0700, Alexei Starovoitov wrote:
> > On Tue, Mar 15, 2022 at 05:42:29PM -0700, Kui-Feng Lee wrote:
> > > BPF trampolines will create a bpf_trace_run_ctx on their stacks,
> > > and
> > > set/reset the current bpf_run_ctx whenever calling/returning from a
> > > bpf_prog.
> > >
> > > Signed-off-by: Kui-Feng Lee <kuifeng@fb.com>
> > > ---
> > >  arch/x86/net/bpf_jit_comp.c | 32 ++++++++++++++++++++++++++++++++
> > >  include/linux/bpf.h         | 12 ++++++++----
> > >  kernel/bpf/syscall.c        |  4 ++--
> > >  kernel/bpf/trampoline.c     | 21 +++++++++++++++++----
> > >  4 files changed, 59 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/arch/x86/net/bpf_jit_comp.c
> > > b/arch/x86/net/bpf_jit_comp.c
> > > index 1228e6e6a420..29775a475513 100644
> > > --- a/arch/x86/net/bpf_jit_comp.c
> > > +++ b/arch/x86/net/bpf_jit_comp.c
> > > @@ -1748,10 +1748,33 @@ static int invoke_bpf_prog(const struct
> > > btf_func_model *m, u8 **pprog,
> > >  {
> > >         u8 *prog = *pprog;
> > >         u8 *jmp_insn;
> > > +       int ctx_cookie_off = offsetof(struct bpf_trace_run_ctx,
> > > bpf_cookie);
> > >         struct bpf_prog *p = l->prog;
> > >
> > > +       EMIT1(0x52);             /* push rdx */
> >
> > Why save/restore rdx?
>
> >
> > > +
> > > +       /* mov rdi, 0 */
> > > +       emit_mov_imm64(&prog, BPF_REG_1, 0, 0);
> > > +
> > > +       /* Prepare struct bpf_trace_run_ctx.
> > > +        * sub rsp, sizeof(struct bpf_trace_run_ctx)
> > > +        * mov rax, rsp
> > > +        * mov QWORD PTR [rax + ctx_cookie_off], rdi
> > > +        */
> >
> > How about the following instead:
> > sub rsp, sizeof(struct bpf_trace_run_ctx)
> > mov qword ptr [rsp + ctx_cookie_off], 0
> > ?
>
> AFAIK, rsp can not be used with the base + displacement addressing
> mode.  Although, it can be used with base + index + displacement
> addressing mode.

Where did you find this?

0:  48 c7 44 24 08 00 00    mov    QWORD PTR [rsp+0x8],0x0
7:  00 00

> >
> > > +       EMIT4(0x48, 0x83, 0xEC, sizeof(struct bpf_trace_run_ctx));
> > > +       EMIT3(0x48, 0x89, 0xE0);
> > > +       EMIT4(0x48, 0x89, 0x78, ctx_cookie_off);
> > > +
> > > +       /* mov rdi, rsp */
> > > +       EMIT3(0x48, 0x89, 0xE7);
> > > +       /* mov QWORD PTR [rdi + sizeof(struct bpf_trace_run_ctx)],
> > > rax */
> > > +       emit_stx(&prog, BPF_DW, BPF_REG_1, BPF_REG_0, sizeof(struct
> > > bpf_trace_run_ctx));
> >
> > why not to do:
> > mov qword ptr[rsp + sizeof(struct bpf_trace_run_ctx)], rsp
> > ?
>
> The same reason as above.

0:  48 89 64 24 08          mov    QWORD PTR [rsp+0x8],rsp

Re: [PATCH bpf-next v2 2/4] bpf, x86: Create bpf_trace_run_ctx on the caller thread's stack

[Kui-Feng Lee]

On Sun, 2022-03-20 at 13:08 -0700, Alexei Starovoitov wrote:
> On Sun, Mar 20, 2022 at 2:31 AM Kui-Feng Lee <kuifeng@fb.com> wrote:
> > 
> > On Fri, 2022-03-18 at 12:09 -0700, Alexei Starovoitov wrote:
> > > On Tue, Mar 15, 2022 at 05:42:29PM -0700, Kui-Feng Lee wrote:
> > > > BPF trampolines will create a bpf_trace_run_ctx on their
> > > > stacks,
> > > > and
> > > > set/reset the current bpf_run_ctx whenever calling/returning
> > > > from a
> > > > bpf_prog.
> > > > 
> > > > Signed-off-by: Kui-Feng Lee <kuifeng@fb.com>
> > > > ---
> > > >  arch/x86/net/bpf_jit_comp.c | 32
> > > > ++++++++++++++++++++++++++++++++
> > > >  include/linux/bpf.h         | 12 ++++++++----
> > > >  kernel/bpf/syscall.c        |  4 ++--
> > > >  kernel/bpf/trampoline.c     | 21 +++++++++++++++++----
> > > >  4 files changed, 59 insertions(+), 10 deletions(-)
> > > > 
> > > > diff --git a/arch/x86/net/bpf_jit_comp.c
> > > > b/arch/x86/net/bpf_jit_comp.c
> > > > index 1228e6e6a420..29775a475513 100644
> > > > --- a/arch/x86/net/bpf_jit_comp.c
> > > > +++ b/arch/x86/net/bpf_jit_comp.c
> > > > @@ -1748,10 +1748,33 @@ static int invoke_bpf_prog(const struct
> > > > btf_func_model *m, u8 **pprog,
> > > >  {
> > > >         u8 *prog = *pprog;
> > > >         u8 *jmp_insn;
> > > > +       int ctx_cookie_off = offsetof(struct bpf_trace_run_ctx,
> > > > bpf_cookie);
> > > >         struct bpf_prog *p = l->prog;
> > > > 
> > > > +       EMIT1(0x52);             /* push rdx */
> > > 
> > > Why save/restore rdx?
> > 
> > > 
> > > > +
> > > > +       /* mov rdi, 0 */
> > > > +       emit_mov_imm64(&prog, BPF_REG_1, 0, 0);
> > > > +
> > > > +       /* Prepare struct bpf_trace_run_ctx.
> > > > +        * sub rsp, sizeof(struct bpf_trace_run_ctx)
> > > > +        * mov rax, rsp
> > > > +        * mov QWORD PTR [rax + ctx_cookie_off], rdi
> > > > +        */
> > > 
> > > How about the following instead:
> > > sub rsp, sizeof(struct bpf_trace_run_ctx)
> > > mov qword ptr [rsp + ctx_cookie_off], 0
> > > ?
> > 
> > AFAIK, rsp can not be used with the base + displacement addressing
> > mode.  Although, it can be used with base + index + displacement
> > addressing mode.
> 
> Where did you find this?

I use the following document to figure out opcodes.

https://ref.x86asm.net/coder64.html#modrm_byte_32_64

It lists available addressing modes and codes.

By the way, I found I had missed SIB byte, that provides extra
features.  It seems working for this case.  I will try it.


> 
> 0:  48 c7 44 24 08 00 00    mov    QWORD PTR [rsp+0x8],0x0
> 7:  00 00
> 
> > > 
> > > > +       EMIT4(0x48, 0x83, 0xEC, sizeof(struct
> > > > bpf_trace_run_ctx));
> > > > +       EMIT3(0x48, 0x89, 0xE0);
> > > > +       EMIT4(0x48, 0x89, 0x78, ctx_cookie_off);
> > > > +
> > > > +       /* mov rdi, rsp */
> > > > +       EMIT3(0x48, 0x89, 0xE7);
> > > > +       /* mov QWORD PTR [rdi + sizeof(struct
> > > > bpf_trace_run_ctx)],
> > > > rax */
> > > > +       emit_stx(&prog, BPF_DW, BPF_REG_1, BPF_REG_0,
> > > > sizeof(struct
> > > > bpf_trace_run_ctx));
> > > 
> > > why not to do:
> > > mov qword ptr[rsp + sizeof(struct bpf_trace_run_ctx)], rsp
> > > ?
> > 
> > The same reason as above.
> 
> 0:  48 89 64 24 08          mov    QWORD PTR [rsp+0x8],rsp

Re: [PATCH bpf-next v2 2/4] bpf, x86: Create bpf_trace_run_ctx on the caller thread's stack

[Andrii Nakryiko]

On Tue, Mar 15, 2022 at 5:44 PM Kui-Feng Lee <kuifeng@fb.com> wrote:
>
> BPF trampolines will create a bpf_trace_run_ctx on their stacks, and
> set/reset the current bpf_run_ctx whenever calling/returning from a
> bpf_prog.
>
> Signed-off-by: Kui-Feng Lee <kuifeng@fb.com>
> ---
>  arch/x86/net/bpf_jit_comp.c | 32 ++++++++++++++++++++++++++++++++
>  include/linux/bpf.h         | 12 ++++++++----
>  kernel/bpf/syscall.c        |  4 ++--
>  kernel/bpf/trampoline.c     | 21 +++++++++++++++++----
>  4 files changed, 59 insertions(+), 10 deletions(-)
>

[...]

> diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
> index 54c695d49ec9..0b050aa2f159 100644
> --- a/kernel/bpf/trampoline.c
> +++ b/kernel/bpf/trampoline.c
> @@ -580,9 +580,12 @@ static void notrace inc_misses_counter(struct bpf_prog *prog)
>   * [2..MAX_U64] - execute bpf prog and record execution time.
>   *     This is start time.
>   */
> -u64 notrace __bpf_prog_enter(struct bpf_prog *prog)
> +u64 notrace __bpf_prog_enter(struct bpf_prog *prog, struct bpf_trace_run_ctx *run_ctx)
>         __acquires(RCU)
>  {
> +       if (run_ctx)
> +               run_ctx->saved_run_ctx = bpf_set_run_ctx(&run_ctx->run_ctx);
> +

In all current cases we bpf_set_run_ctx() after migrate_disable and
rcu_read_lock, let's keep this consistent (even if I don't remember if
that order matters or not).

>         rcu_read_lock();
>         migrate_disable();
>         if (unlikely(__this_cpu_inc_return(*(prog->active)) != 1)) {
> @@ -614,17 +617,23 @@ static void notrace update_prog_stats(struct bpf_prog *prog,
>         }
>  }
>
> -void notrace __bpf_prog_exit(struct bpf_prog *prog, u64 start)
> +void notrace __bpf_prog_exit(struct bpf_prog *prog, u64 start, struct bpf_trace_run_ctx *run_ctx)
>         __releases(RCU)
>  {
> +       if (run_ctx)
> +               bpf_reset_run_ctx(run_ctx->saved_run_ctx);
> +
>         update_prog_stats(prog, start);
>         __this_cpu_dec(*(prog->active));
>         migrate_enable();
>         rcu_read_unlock();
>  }
>
> -u64 notrace __bpf_prog_enter_sleepable(struct bpf_prog *prog)
> +u64 notrace __bpf_prog_enter_sleepable(struct bpf_prog *prog, struct bpf_trace_run_ctx *run_ctx)
>  {
> +       if (run_ctx)
> +               run_ctx->saved_run_ctx = bpf_set_run_ctx(&run_ctx->run_ctx);
> +
>         rcu_read_lock_trace();
>         migrate_disable();
>         might_fault();
> @@ -635,8 +644,12 @@ u64 notrace __bpf_prog_enter_sleepable(struct bpf_prog *prog)
>         return bpf_prog_start_time();
>  }
>
> -void notrace __bpf_prog_exit_sleepable(struct bpf_prog *prog, u64 start)
> +void notrace __bpf_prog_exit_sleepable(struct bpf_prog *prog, u64 start,
> +                                      struct bpf_trace_run_ctx *run_ctx)

now that we have entire run_ctx, can we move `start` into run_ctx and
simplify __bpf_prog_enter/exit calls a bit? Or extra indirection will
hurt performance and won't be compensated by simpler enter/exit
calling convention?

>  {
> +       if (run_ctx)
> +               bpf_reset_run_ctx(run_ctx->saved_run_ctx);
> +
>         update_prog_stats(prog, start);
>         __this_cpu_dec(*(prog->active));
>         migrate_enable();
> --
> 2.30.2
>

Re: [PATCH bpf-next v2 2/4] bpf, x86: Create bpf_trace_run_ctx on the caller thread's stack

[Alexei Starovoitov]

On Mon, Mar 21, 2022 at 4:04 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Tue, Mar 15, 2022 at 5:44 PM Kui-Feng Lee <kuifeng@fb.com> wrote:
> >
> > BPF trampolines will create a bpf_trace_run_ctx on their stacks, and
> > set/reset the current bpf_run_ctx whenever calling/returning from a
> > bpf_prog.
> >
> > Signed-off-by: Kui-Feng Lee <kuifeng@fb.com>
> > ---
> >  arch/x86/net/bpf_jit_comp.c | 32 ++++++++++++++++++++++++++++++++
> >  include/linux/bpf.h         | 12 ++++++++----
> >  kernel/bpf/syscall.c        |  4 ++--
> >  kernel/bpf/trampoline.c     | 21 +++++++++++++++++----
> >  4 files changed, 59 insertions(+), 10 deletions(-)
> >
>
> [...]
>
> > diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
> > index 54c695d49ec9..0b050aa2f159 100644
> > --- a/kernel/bpf/trampoline.c
> > +++ b/kernel/bpf/trampoline.c
> > @@ -580,9 +580,12 @@ static void notrace inc_misses_counter(struct bpf_prog *prog)
> >   * [2..MAX_U64] - execute bpf prog and record execution time.
> >   *     This is start time.
> >   */
> > -u64 notrace __bpf_prog_enter(struct bpf_prog *prog)
> > +u64 notrace __bpf_prog_enter(struct bpf_prog *prog, struct bpf_trace_run_ctx *run_ctx)
> >         __acquires(RCU)
> >  {
> > +       if (run_ctx)
> > +               run_ctx->saved_run_ctx = bpf_set_run_ctx(&run_ctx->run_ctx);
> > +
>
> In all current cases we bpf_set_run_ctx() after migrate_disable and
> rcu_read_lock, let's keep this consistent (even if I don't remember if
> that order matters or not).
>
> >         rcu_read_lock();
> >         migrate_disable();
> >         if (unlikely(__this_cpu_inc_return(*(prog->active)) != 1)) {
> > @@ -614,17 +617,23 @@ static void notrace update_prog_stats(struct bpf_prog *prog,
> >         }
> >  }
> >
> > -void notrace __bpf_prog_exit(struct bpf_prog *prog, u64 start)
> > +void notrace __bpf_prog_exit(struct bpf_prog *prog, u64 start, struct bpf_trace_run_ctx *run_ctx)
> >         __releases(RCU)
> >  {
> > +       if (run_ctx)
> > +               bpf_reset_run_ctx(run_ctx->saved_run_ctx);
> > +
> >         update_prog_stats(prog, start);
> >         __this_cpu_dec(*(prog->active));
> >         migrate_enable();
> >         rcu_read_unlock();
> >  }
> >
> > -u64 notrace __bpf_prog_enter_sleepable(struct bpf_prog *prog)
> > +u64 notrace __bpf_prog_enter_sleepable(struct bpf_prog *prog, struct bpf_trace_run_ctx *run_ctx)
> >  {
> > +       if (run_ctx)
> > +               run_ctx->saved_run_ctx = bpf_set_run_ctx(&run_ctx->run_ctx);
> > +
> >         rcu_read_lock_trace();
> >         migrate_disable();
> >         might_fault();
> > @@ -635,8 +644,12 @@ u64 notrace __bpf_prog_enter_sleepable(struct bpf_prog *prog)
> >         return bpf_prog_start_time();
> >  }
> >
> > -void notrace __bpf_prog_exit_sleepable(struct bpf_prog *prog, u64 start)
> > +void notrace __bpf_prog_exit_sleepable(struct bpf_prog *prog, u64 start,
> > +                                      struct bpf_trace_run_ctx *run_ctx)
>
> now that we have entire run_ctx, can we move `start` into run_ctx and
> simplify __bpf_prog_enter/exit calls a bit? Or extra indirection will
> hurt performance and won't be compensated by simpler enter/exit
> calling convention?

The "start" is an optional and temporary argument.
I suspect it will look odd inside run_ctx.
imo the current way is simpler.

Re: [PATCH bpf-next v2 2/4] bpf, x86: Create bpf_trace_run_ctx on the caller thread's stack

[Andrii Nakryiko]

On Mon, Mar 21, 2022 at 4:25 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Mon, Mar 21, 2022 at 4:04 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Tue, Mar 15, 2022 at 5:44 PM Kui-Feng Lee <kuifeng@fb.com> wrote:
> > >
> > > BPF trampolines will create a bpf_trace_run_ctx on their stacks, and
> > > set/reset the current bpf_run_ctx whenever calling/returning from a
> > > bpf_prog.
> > >
> > > Signed-off-by: Kui-Feng Lee <kuifeng@fb.com>
> > > ---
> > >  arch/x86/net/bpf_jit_comp.c | 32 ++++++++++++++++++++++++++++++++
> > >  include/linux/bpf.h         | 12 ++++++++----
> > >  kernel/bpf/syscall.c        |  4 ++--
> > >  kernel/bpf/trampoline.c     | 21 +++++++++++++++++----
> > >  4 files changed, 59 insertions(+), 10 deletions(-)
> > >
> >
> > [...]
> >
> > > diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
> > > index 54c695d49ec9..0b050aa2f159 100644
> > > --- a/kernel/bpf/trampoline.c
> > > +++ b/kernel/bpf/trampoline.c
> > > @@ -580,9 +580,12 @@ static void notrace inc_misses_counter(struct bpf_prog *prog)
> > >   * [2..MAX_U64] - execute bpf prog and record execution time.
> > >   *     This is start time.
> > >   */
> > > -u64 notrace __bpf_prog_enter(struct bpf_prog *prog)
> > > +u64 notrace __bpf_prog_enter(struct bpf_prog *prog, struct bpf_trace_run_ctx *run_ctx)
> > >         __acquires(RCU)
> > >  {
> > > +       if (run_ctx)
> > > +               run_ctx->saved_run_ctx = bpf_set_run_ctx(&run_ctx->run_ctx);
> > > +
> >
> > In all current cases we bpf_set_run_ctx() after migrate_disable and
> > rcu_read_lock, let's keep this consistent (even if I don't remember if
> > that order matters or not).
> >
> > >         rcu_read_lock();
> > >         migrate_disable();
> > >         if (unlikely(__this_cpu_inc_return(*(prog->active)) != 1)) {
> > > @@ -614,17 +617,23 @@ static void notrace update_prog_stats(struct bpf_prog *prog,
> > >         }
> > >  }
> > >
> > > -void notrace __bpf_prog_exit(struct bpf_prog *prog, u64 start)
> > > +void notrace __bpf_prog_exit(struct bpf_prog *prog, u64 start, struct bpf_trace_run_ctx *run_ctx)
> > >         __releases(RCU)
> > >  {
> > > +       if (run_ctx)
> > > +               bpf_reset_run_ctx(run_ctx->saved_run_ctx);
> > > +
> > >         update_prog_stats(prog, start);
> > >         __this_cpu_dec(*(prog->active));
> > >         migrate_enable();
> > >         rcu_read_unlock();
> > >  }
> > >
> > > -u64 notrace __bpf_prog_enter_sleepable(struct bpf_prog *prog)
> > > +u64 notrace __bpf_prog_enter_sleepable(struct bpf_prog *prog, struct bpf_trace_run_ctx *run_ctx)
> > >  {
> > > +       if (run_ctx)
> > > +               run_ctx->saved_run_ctx = bpf_set_run_ctx(&run_ctx->run_ctx);
> > > +
> > >         rcu_read_lock_trace();
> > >         migrate_disable();
> > >         might_fault();
> > > @@ -635,8 +644,12 @@ u64 notrace __bpf_prog_enter_sleepable(struct bpf_prog *prog)
> > >         return bpf_prog_start_time();
> > >  }
> > >
> > > -void notrace __bpf_prog_exit_sleepable(struct bpf_prog *prog, u64 start)
> > > +void notrace __bpf_prog_exit_sleepable(struct bpf_prog *prog, u64 start,
> > > +                                      struct bpf_trace_run_ctx *run_ctx)
> >
> > now that we have entire run_ctx, can we move `start` into run_ctx and
> > simplify __bpf_prog_enter/exit calls a bit? Or extra indirection will
> > hurt performance and won't be compensated by simpler enter/exit
> > calling convention?
>
> The "start" is an optional and temporary argument.
> I suspect it will look odd inside run_ctx.
> imo the current way is simpler.

So is saved_run_ctx (they have identical lifetimes), but I'm fine
either way. Was thinking it would result in simpler trampoline
generation code (both for humans and CPU), that's all.

Re: [PATCH bpf-next v2 2/4] bpf, x86: Create bpf_trace_run_ctx on the caller thread's stack

[Andrii Nakryiko]

On Tue, Mar 15, 2022 at 5:44 PM Kui-Feng Lee <kuifeng@fb.com> wrote:
>
> BPF trampolines will create a bpf_trace_run_ctx on their stacks, and
> set/reset the current bpf_run_ctx whenever calling/returning from a
> bpf_prog.
>
> Signed-off-by: Kui-Feng Lee <kuifeng@fb.com>
> ---
>  arch/x86/net/bpf_jit_comp.c | 32 ++++++++++++++++++++++++++++++++
>  include/linux/bpf.h         | 12 ++++++++----
>  kernel/bpf/syscall.c        |  4 ++--
>  kernel/bpf/trampoline.c     | 21 +++++++++++++++++----
>  4 files changed, 59 insertions(+), 10 deletions(-)
>
> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> index 1228e6e6a420..29775a475513 100644
> --- a/arch/x86/net/bpf_jit_comp.c
> +++ b/arch/x86/net/bpf_jit_comp.c
> @@ -1748,10 +1748,33 @@ static int invoke_bpf_prog(const struct btf_func_model *m, u8 **pprog,
>  {
>         u8 *prog = *pprog;
>         u8 *jmp_insn;
> +       int ctx_cookie_off = offsetof(struct bpf_trace_run_ctx, bpf_cookie);
>         struct bpf_prog *p = l->prog;
>
> +       EMIT1(0x52);             /* push rdx */
> +
> +       /* mov rdi, 0 */
> +       emit_mov_imm64(&prog, BPF_REG_1, 0, 0);
> +
> +       /* Prepare struct bpf_trace_run_ctx.
> +        * sub rsp, sizeof(struct bpf_trace_run_ctx)
> +        * mov rax, rsp
> +        * mov QWORD PTR [rax + ctx_cookie_off], rdi
> +        */
> +       EMIT4(0x48, 0x83, 0xEC, sizeof(struct bpf_trace_run_ctx));
> +       EMIT3(0x48, 0x89, 0xE0);
> +       EMIT4(0x48, 0x89, 0x78, ctx_cookie_off);
> +
> +       /* mov rdi, rsp */
> +       EMIT3(0x48, 0x89, 0xE7);
> +       /* mov QWORD PTR [rdi + sizeof(struct bpf_trace_run_ctx)], rax */
> +       emit_stx(&prog, BPF_DW, BPF_REG_1, BPF_REG_0, sizeof(struct bpf_trace_run_ctx));
> +
>         /* arg1: mov rdi, progs[i] */
>         emit_mov_imm64(&prog, BPF_REG_1, (long) p >> 32, (u32) (long) p);
> +       /* arg2: mov rsi, rsp (struct bpf_run_ctx *) */
> +       EMIT3(0x48, 0x89, 0xE6);
> +
>         if (emit_call(&prog,
>                       p->aux->sleepable ? __bpf_prog_enter_sleepable :
>                       __bpf_prog_enter, prog))
> @@ -1797,11 +1820,20 @@ static int invoke_bpf_prog(const struct btf_func_model *m, u8 **pprog,
>         emit_mov_imm64(&prog, BPF_REG_1, (long) p >> 32, (u32) (long) p);
>         /* arg2: mov rsi, rbx <- start time in nsec */
>         emit_mov_reg(&prog, true, BPF_REG_2, BPF_REG_6);
> +       /* arg3: mov rdx, rsp (struct bpf_run_ctx *) */
> +       EMIT3(0x48, 0x89, 0xE2);
>         if (emit_call(&prog,
>                       p->aux->sleepable ? __bpf_prog_exit_sleepable :
>                       __bpf_prog_exit, prog))
>                         return -EINVAL;
>
> +       /* pop struct bpf_trace_run_ctx
> +        * add rsp, sizeof(struct bpf_trace_run_ctx)
> +        */
> +       EMIT4(0x48, 0x83, 0xC4, sizeof(struct bpf_trace_run_ctx));
> +
> +       EMIT1(0x5A); /* pop rdx */
> +
>         *pprog = prog;
>         return 0;
>  }
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 3dcae8550c21..d20a23953696 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -681,6 +681,8 @@ struct bpf_tramp_links {
>         int nr_links;
>  };
>
> +struct bpf_trace_run_ctx;
> +
>  /* Different use cases for BPF trampoline:
>   * 1. replace nop at the function entry (kprobe equivalent)
>   *    flags = BPF_TRAMP_F_RESTORE_REGS
> @@ -707,10 +709,11 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *tr, void *image, void *i
>                                 struct bpf_tramp_links *tlinks,
>                                 void *orig_call);
>  /* these two functions are called from generated trampoline */
> -u64 notrace __bpf_prog_enter(struct bpf_prog *prog);
> -void notrace __bpf_prog_exit(struct bpf_prog *prog, u64 start);
> -u64 notrace __bpf_prog_enter_sleepable(struct bpf_prog *prog);
> -void notrace __bpf_prog_exit_sleepable(struct bpf_prog *prog, u64 start);
> +u64 notrace __bpf_prog_enter(struct bpf_prog *prog, struct bpf_trace_run_ctx *run_ctx);
> +void notrace __bpf_prog_exit(struct bpf_prog *prog, u64 start, struct bpf_trace_run_ctx *run_ctx);
> +u64 notrace __bpf_prog_enter_sleepable(struct bpf_prog *prog, struct bpf_trace_run_ctx *run_ctx);
> +void notrace __bpf_prog_exit_sleepable(struct bpf_prog *prog, u64 start,
> +                                      struct bpf_trace_run_ctx *run_ctx);
>  void notrace __bpf_tramp_enter(struct bpf_tramp_image *tr);
>  void notrace __bpf_tramp_exit(struct bpf_tramp_image *tr);
>
> @@ -1291,6 +1294,7 @@ struct bpf_cg_run_ctx {
>  struct bpf_trace_run_ctx {
>         struct bpf_run_ctx run_ctx;
>         u64 bpf_cookie;
> +       struct bpf_run_ctx *saved_run_ctx;
>  };

oh, and bpf_trace_run_ctx is used for kprobe/uprobe/tracepoint, let's
add a new struct bpf_tramp_run_ctx which would reflect that it is used
for BPF trampoline-based BPF programs. Otherwise it's confusing to
have saved_run_ctx for kprobe where we don't use that. Similarly, if
we move "start" timestamp, it will be a bit off. Not end of the world,
but I think keeping them separate would make sense over long run.

>
>  static inline struct bpf_run_ctx *bpf_set_run_ctx(struct bpf_run_ctx *new_ctx)

[...]

Re: [PATCH bpf-next v2 2/4] bpf, x86: Create bpf_trace_run_ctx on the caller thread's stack

[Kui-Feng Lee]

On Mon, 2022-03-21 at 16:08 -0700, Andrii Nakryiko wrote:
> On Tue, Mar 15, 2022 at 5:44 PM Kui-Feng Lee <kuifeng@fb.com> wrote:
> > 
> > @@ -1291,6 +1294,7 @@ struct bpf_cg_run_ctx {
> >  struct bpf_trace_run_ctx {
> >         struct bpf_run_ctx run_ctx;
> >         u64 bpf_cookie;
> > +       struct bpf_run_ctx *saved_run_ctx;
> >  };
> 
> oh, and bpf_trace_run_ctx is used for kprobe/uprobe/tracepoint, let's
> add a new struct bpf_tramp_run_ctx which would reflect that it is
> used
> for BPF trampoline-based BPF programs. Otherwise it's confusing to
> have saved_run_ctx for kprobe where we don't use that. Similarly, if
> we move "start" timestamp, it will be a bit off. Not end of the
> world,
> but I think keeping them separate would make sense over long run.

Ok!

Re: [PATCH bpf-next v2 2/4] bpf, x86: Create bpf_trace_run_ctx on the caller thread's stack

[Andrii Nakryiko]

On Tue, Mar 22, 2022 at 8:30 AM Kui-Feng Lee <kuifeng@fb.com> wrote:
>
> On Mon, 2022-03-21 at 16:08 -0700, Andrii Nakryiko wrote:
> > On Tue, Mar 15, 2022 at 5:44 PM Kui-Feng Lee <kuifeng@fb.com> wrote:
> > >
> > > @@ -1291,6 +1294,7 @@ struct bpf_cg_run_ctx {
> > >  struct bpf_trace_run_ctx {
> > >         struct bpf_run_ctx run_ctx;
> > >         u64 bpf_cookie;
> > > +       struct bpf_run_ctx *saved_run_ctx;
> > >  };
> >
> > oh, and bpf_trace_run_ctx is used for kprobe/uprobe/tracepoint, let's
> > add a new struct bpf_tramp_run_ctx which would reflect that it is
> > used
> > for BPF trampoline-based BPF programs. Otherwise it's confusing to
> > have saved_run_ctx for kprobe where we don't use that. Similarly, if
> > we move "start" timestamp, it will be a bit off. Not end of the
> > world,
> > but I think keeping them separate would make sense over long run.
>
> Ok!
>

We discussed this start timestamp with Alexei offline and concluded
that it's better to leave start as is, as we don't really store start
on the stack (we keep it in register), so moving it into run_ctx won't
give us any benefits.

[PATCH bpf-next v2 3/4] bpf, x86: Support BPF cookie for fentry/fexit/fmod_ret.

[Kui-Feng Lee]

Add a bpf_cookie field to attach a cookie to an instance of struct
bpf_link.  The cookie of a bpf_link will be installed when calling the
associated program to make it available to the program.

Signed-off-by: Kui-Feng Lee <kuifeng@fb.com>
---
 arch/x86/net/bpf_jit_comp.c    |  4 ++--
 include/linux/bpf.h            |  1 +
 include/uapi/linux/bpf.h       |  1 +
 kernel/bpf/syscall.c           | 11 +++++++----
 kernel/trace/bpf_trace.c       | 17 +++++++++++++++++
 tools/include/uapi/linux/bpf.h |  1 +
 tools/lib/bpf/bpf.c            | 14 ++++++++++++++
 tools/lib/bpf/bpf.h            |  1 +
 tools/lib/bpf/libbpf.map       |  1 +
 9 files changed, 45 insertions(+), 6 deletions(-)

diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index 29775a475513..5fab8530e909 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -1753,8 +1753,8 @@ static int invoke_bpf_prog(const struct btf_func_model *m, u8 **pprog,
 
 	EMIT1(0x52);		 /* push rdx */
 
-	/* mov rdi, 0 */
-	emit_mov_imm64(&prog, BPF_REG_1, 0, 0);
+	/* mov rdi, cookie */
+	emit_mov_imm64(&prog, BPF_REG_1, (long) l->cookie >> 32, (u32) (long) l->cookie);
 
 	/* Prepare struct bpf_trace_run_ctx.
 	 * sub rsp, sizeof(struct bpf_trace_run_ctx)
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index d20a23953696..9469f9264b4f 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1040,6 +1040,7 @@ struct bpf_link {
 	struct bpf_prog *prog;
 	struct work_struct work;
 	struct hlist_node tramp_hlist;
+	u64 cookie;
 };
 
 struct bpf_link_ops {
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 9e34da50440c..a52ec1797828 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -1429,6 +1429,7 @@ union bpf_attr {
 	struct { /* anonymous struct used by BPF_RAW_TRACEPOINT_OPEN command */
 		__u64 name;
 		__u32 prog_fd;
+		__u64 bpf_cookie;
 	} raw_tracepoint;
 
 	struct { /* anonymous struct for BPF_BTF_LOAD */
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index a289ef55ea17..cf6e22a3ab61 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -2703,7 +2703,8 @@ static const struct bpf_link_ops bpf_tracing_link_lops = {
 
 static int bpf_tracing_prog_attach(struct bpf_prog *prog,
 				   int tgt_prog_fd,
-				   u32 btf_id)
+				   u32 btf_id,
+				   u64 bpf_cookie)
 {
 	struct bpf_link_primer link_primer;
 	struct bpf_prog *tgt_prog = NULL;
@@ -2768,6 +2769,7 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog,
 	bpf_link_init(&link->link, BPF_LINK_TYPE_TRACING,
 		      &bpf_tracing_link_lops, prog);
 	link->attach_type = prog->expected_attach_type;
+	link->link.cookie = bpf_cookie;
 
 	mutex_lock(&prog->aux->dst_mutex);
 
@@ -3024,7 +3026,7 @@ static int bpf_perf_link_attach(const union bpf_attr *attr, struct bpf_prog *pro
 }
 #endif /* CONFIG_PERF_EVENTS */
 
-#define BPF_RAW_TRACEPOINT_OPEN_LAST_FIELD raw_tracepoint.prog_fd
+#define BPF_RAW_TRACEPOINT_OPEN_LAST_FIELD raw_tracepoint.bpf_cookie
 
 static int bpf_raw_tracepoint_open(const union bpf_attr *attr)
 {
@@ -3059,7 +3061,7 @@ static int bpf_raw_tracepoint_open(const union bpf_attr *attr)
 			tp_name = prog->aux->attach_func_name;
 			break;
 		}
-		err = bpf_tracing_prog_attach(prog, 0, 0);
+		err = bpf_tracing_prog_attach(prog, 0, 0, attr->raw_tracepoint.bpf_cookie);
 		if (err >= 0)
 			return err;
 		goto out_put_prog;
@@ -4251,7 +4253,8 @@ static int tracing_bpf_link_attach(const union bpf_attr *attr, bpfptr_t uattr,
 	else if (prog->type == BPF_PROG_TYPE_EXT)
 		return bpf_tracing_prog_attach(prog,
 					       attr->link_create.target_fd,
-					       attr->link_create.target_btf_id);
+					       attr->link_create.target_btf_id,
+					       0);
 	return -EINVAL;
 }
 
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index a2024ba32a20..1558b0476b3a 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -1063,6 +1063,21 @@ static const struct bpf_func_proto bpf_get_attach_cookie_proto_pe = {
 	.arg1_type	= ARG_PTR_TO_CTX,
 };
 
+BPF_CALL_1(bpf_get_attach_cookie_tracing, 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->bpf_cookie;
+}
+
+static const struct bpf_func_proto bpf_get_attach_cookie_proto_tracing = {
+	.func		= bpf_get_attach_cookie_tracing,
+	.gpl_only	= false,
+	.ret_type	= RET_INTEGER,
+	.arg1_type	= ARG_PTR_TO_CTX,
+};
+
 BPF_CALL_3(bpf_get_branch_snapshot, void *, buf, u32, size, u64, flags)
 {
 #ifndef CONFIG_X86
@@ -1687,6 +1702,8 @@ tracing_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 		return bpf_prog_has_trampoline(prog) ? &bpf_get_func_ret_proto : NULL;
 	case BPF_FUNC_get_func_arg_cnt:
 		return bpf_prog_has_trampoline(prog) ? &bpf_get_func_arg_cnt_proto : NULL;
+	case BPF_FUNC_get_attach_cookie:
+		return bpf_prog_has_trampoline(prog) ? &bpf_get_attach_cookie_proto_tracing : NULL;
 	default:
 		fn = raw_tp_prog_func_proto(func_id, prog);
 		if (!fn && prog->expected_attach_type == BPF_TRACE_ITER)
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 9e34da50440c..a52ec1797828 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -1429,6 +1429,7 @@ union bpf_attr {
 	struct { /* anonymous struct used by BPF_RAW_TRACEPOINT_OPEN command */
 		__u64 name;
 		__u32 prog_fd;
+		__u64 bpf_cookie;
 	} raw_tracepoint;
 
 	struct { /* anonymous struct for BPF_BTF_LOAD */
diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
index f69ce3a01385..dbbf09c84c21 100644
--- a/tools/lib/bpf/bpf.c
+++ b/tools/lib/bpf/bpf.c
@@ -1133,6 +1133,20 @@ int bpf_raw_tracepoint_open(const char *name, int prog_fd)
 	return libbpf_err_errno(fd);
 }
 
+int bpf_raw_tracepoint_cookie_open(const char *name, int prog_fd, __u64 bpf_cookie)
+{
+	union bpf_attr attr;
+	int fd;
+
+	memset(&attr, 0, sizeof(attr));
+	attr.raw_tracepoint.name = ptr_to_u64(name);
+	attr.raw_tracepoint.prog_fd = prog_fd;
+	attr.raw_tracepoint.bpf_cookie = bpf_cookie;
+
+	fd = sys_bpf_fd(BPF_RAW_TRACEPOINT_OPEN, &attr, sizeof(attr));
+	return libbpf_err_errno(fd);
+}
+
 int bpf_btf_load(const void *btf_data, size_t btf_size, const struct bpf_btf_load_opts *opts)
 {
 	const size_t attr_sz = offsetofend(union bpf_attr, btf_log_level);
diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
index 5253cb4a4c0a..23bebcdaf23b 100644
--- a/tools/lib/bpf/bpf.h
+++ b/tools/lib/bpf/bpf.h
@@ -477,6 +477,7 @@ LIBBPF_API int bpf_prog_query(int target_fd, enum bpf_attach_type type,
 			      __u32 query_flags, __u32 *attach_flags,
 			      __u32 *prog_ids, __u32 *prog_cnt);
 LIBBPF_API int bpf_raw_tracepoint_open(const char *name, int prog_fd);
+LIBBPF_API int bpf_raw_tracepoint_cookie_open(const char *name, int prog_fd, __u64 bpf_cookie);
 LIBBPF_API int bpf_task_fd_query(int pid, int fd, __u32 flags, char *buf,
 				 __u32 *buf_len, __u32 *prog_id, __u32 *fd_type,
 				 __u64 *probe_offset, __u64 *probe_addr);
diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
index df1b947792c8..20f947a385fa 100644
--- a/tools/lib/bpf/libbpf.map
+++ b/tools/lib/bpf/libbpf.map
@@ -434,6 +434,7 @@ LIBBPF_0.7.0 {
 		bpf_xdp_detach;
 		bpf_xdp_query;
 		bpf_xdp_query_id;
+		bpf_raw_tracepoint_cookie_open;
 		libbpf_probe_bpf_helper;
 		libbpf_probe_bpf_map_type;
 		libbpf_probe_bpf_prog_type;
-- 
2.30.2

Re: [PATCH bpf-next v2 3/4] bpf, x86: Support BPF cookie for fentry/fexit/fmod_ret.

[Alexei Starovoitov]

On Tue, Mar 15, 2022 at 05:42:30PM -0700, Kui-Feng Lee wrote:
> Add a bpf_cookie field to attach a cookie to an instance of struct
> bpf_link.  The cookie of a bpf_link will be installed when calling the
> associated program to make it available to the program.
> 
> Signed-off-by: Kui-Feng Lee <kuifeng@fb.com>
> ---
>  arch/x86/net/bpf_jit_comp.c    |  4 ++--
>  include/linux/bpf.h            |  1 +
>  include/uapi/linux/bpf.h       |  1 +
>  kernel/bpf/syscall.c           | 11 +++++++----
>  kernel/trace/bpf_trace.c       | 17 +++++++++++++++++
>  tools/include/uapi/linux/bpf.h |  1 +
>  tools/lib/bpf/bpf.c            | 14 ++++++++++++++
>  tools/lib/bpf/bpf.h            |  1 +
>  tools/lib/bpf/libbpf.map       |  1 +
>  9 files changed, 45 insertions(+), 6 deletions(-)

please split kernel and libbpf changes into two different patches.

> diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
> index f69ce3a01385..dbbf09c84c21 100644
> --- a/tools/lib/bpf/bpf.c
> +++ b/tools/lib/bpf/bpf.c
> @@ -1133,6 +1133,20 @@ int bpf_raw_tracepoint_open(const char *name, int prog_fd)
>  	return libbpf_err_errno(fd);
>  }
>  
> +int bpf_raw_tracepoint_cookie_open(const char *name, int prog_fd, __u64 bpf_cookie)

lets introduce opts style to raw_tp_open instead.

> +{
> +	union bpf_attr attr;
> +	int fd;
> +
> +	memset(&attr, 0, sizeof(attr));
> +	attr.raw_tracepoint.name = ptr_to_u64(name);
> +	attr.raw_tracepoint.prog_fd = prog_fd;
> +	attr.raw_tracepoint.bpf_cookie = bpf_cookie;
> +
> +	fd = sys_bpf_fd(BPF_RAW_TRACEPOINT_OPEN, &attr, sizeof(attr));
> +	return libbpf_err_errno(fd);
> +}
> +
>  int bpf_btf_load(const void *btf_data, size_t btf_size, const struct bpf_btf_load_opts *opts)
>  {
>  	const size_t attr_sz = offsetofend(union bpf_attr, btf_log_level);
> diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
> index 5253cb4a4c0a..23bebcdaf23b 100644
> --- a/tools/lib/bpf/bpf.h
> +++ b/tools/lib/bpf/bpf.h
> @@ -477,6 +477,7 @@ LIBBPF_API int bpf_prog_query(int target_fd, enum bpf_attach_type type,
>  			      __u32 query_flags, __u32 *attach_flags,
>  			      __u32 *prog_ids, __u32 *prog_cnt);
>  LIBBPF_API int bpf_raw_tracepoint_open(const char *name, int prog_fd);
> +LIBBPF_API int bpf_raw_tracepoint_cookie_open(const char *name, int prog_fd, __u64 bpf_cookie);
>  LIBBPF_API int bpf_task_fd_query(int pid, int fd, __u32 flags, char *buf,
>  				 __u32 *buf_len, __u32 *prog_id, __u32 *fd_type,
>  				 __u64 *probe_offset, __u64 *probe_addr);
> diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> index df1b947792c8..20f947a385fa 100644
> --- a/tools/lib/bpf/libbpf.map
> +++ b/tools/lib/bpf/libbpf.map
> @@ -434,6 +434,7 @@ LIBBPF_0.7.0 {
>  		bpf_xdp_detach;
>  		bpf_xdp_query;
>  		bpf_xdp_query_id;
> +		bpf_raw_tracepoint_cookie_open;
>  		libbpf_probe_bpf_helper;
>  		libbpf_probe_bpf_map_type;
>  		libbpf_probe_bpf_prog_type;
> -- 
> 2.30.2
> 

-- 

Re: [PATCH bpf-next v2 3/4] bpf, x86: Support BPF cookie for fentry/fexit/fmod_ret.

[Andrii Nakryiko]

On Fri, Mar 18, 2022 at 12:13 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Tue, Mar 15, 2022 at 05:42:30PM -0700, Kui-Feng Lee wrote:
> > Add a bpf_cookie field to attach a cookie to an instance of struct
> > bpf_link.  The cookie of a bpf_link will be installed when calling the
> > associated program to make it available to the program.
> >
> > Signed-off-by: Kui-Feng Lee <kuifeng@fb.com>
> > ---
> >  arch/x86/net/bpf_jit_comp.c    |  4 ++--
> >  include/linux/bpf.h            |  1 +
> >  include/uapi/linux/bpf.h       |  1 +
> >  kernel/bpf/syscall.c           | 11 +++++++----
> >  kernel/trace/bpf_trace.c       | 17 +++++++++++++++++
> >  tools/include/uapi/linux/bpf.h |  1 +
> >  tools/lib/bpf/bpf.c            | 14 ++++++++++++++
> >  tools/lib/bpf/bpf.h            |  1 +
> >  tools/lib/bpf/libbpf.map       |  1 +
> >  9 files changed, 45 insertions(+), 6 deletions(-)
>
> please split kernel and libbpf changes into two different patches.
>

+1

> > diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
> > index f69ce3a01385..dbbf09c84c21 100644
> > --- a/tools/lib/bpf/bpf.c
> > +++ b/tools/lib/bpf/bpf.c
> > @@ -1133,6 +1133,20 @@ int bpf_raw_tracepoint_open(const char *name, int prog_fd)
> >       return libbpf_err_errno(fd);
> >  }
> >
> > +int bpf_raw_tracepoint_cookie_open(const char *name, int prog_fd, __u64 bpf_cookie)
>
> lets introduce opts style to raw_tp_open instead.

I remember I brought this up earlier, but I forgot the outcome. What
if don't touch BPF_RAW_TRACEPOINT_OPEN and instead allow to create all
the same links through more universal BPF_LINK_CREATE command. And
only there we add bpf_cookie? There are few advantages:

1. We can separate raw_tracepoint and trampoline-based programs more
cleanly in UAPI (it will be two separate structs: link_create.raw_tp
with raw tracepoint name vs link_create.trampoline, or whatever the
name, with cookie and stuff). Remember that raw_tp won't support
bpf_cookie for now, so it would be another advantage not to promise
cookie in UAPI.

2. libbpf can be smart enough to pick either RAW_TRACEPOINT_OPEN (and
reject it if bpf_cookie is non-zero) or BPF_LINK_CREATE, depending on
kernel support. So users would need to only use bpf_link_create()
moving forward with all the backwards compatibility preserved.


>
> > +{
> > +     union bpf_attr attr;
> > +     int fd;
> > +
> > +     memset(&attr, 0, sizeof(attr));
> > +     attr.raw_tracepoint.name = ptr_to_u64(name);
> > +     attr.raw_tracepoint.prog_fd = prog_fd;
> > +     attr.raw_tracepoint.bpf_cookie = bpf_cookie;
> > +
> > +     fd = sys_bpf_fd(BPF_RAW_TRACEPOINT_OPEN, &attr, sizeof(attr));
> > +     return libbpf_err_errno(fd);
> > +}
> > +
> >  int bpf_btf_load(const void *btf_data, size_t btf_size, const struct bpf_btf_load_opts *opts)
> >  {
> >       const size_t attr_sz = offsetofend(union bpf_attr, btf_log_level);
> > diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
> > index 5253cb4a4c0a..23bebcdaf23b 100644
> > --- a/tools/lib/bpf/bpf.h
> > +++ b/tools/lib/bpf/bpf.h
> > @@ -477,6 +477,7 @@ LIBBPF_API int bpf_prog_query(int target_fd, enum bpf_attach_type type,
> >                             __u32 query_flags, __u32 *attach_flags,
> >                             __u32 *prog_ids, __u32 *prog_cnt);
> >  LIBBPF_API int bpf_raw_tracepoint_open(const char *name, int prog_fd);
> > +LIBBPF_API int bpf_raw_tracepoint_cookie_open(const char *name, int prog_fd, __u64 bpf_cookie);
> >  LIBBPF_API int bpf_task_fd_query(int pid, int fd, __u32 flags, char *buf,
> >                                __u32 *buf_len, __u32 *prog_id, __u32 *fd_type,
> >                                __u64 *probe_offset, __u64 *probe_addr);
> > diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> > index df1b947792c8..20f947a385fa 100644
> > --- a/tools/lib/bpf/libbpf.map
> > +++ b/tools/lib/bpf/libbpf.map
> > @@ -434,6 +434,7 @@ LIBBPF_0.7.0 {
> >               bpf_xdp_detach;
> >               bpf_xdp_query;
> >               bpf_xdp_query_id;
> > +             bpf_raw_tracepoint_cookie_open;

this (if still needed) should go into 0.8.0 section

> >               libbpf_probe_bpf_helper;
> >               libbpf_probe_bpf_map_type;
> >               libbpf_probe_bpf_prog_type;
> > --
> > 2.30.2
> >
>
> --

Re: [PATCH bpf-next v2 3/4] bpf, x86: Support BPF cookie for fentry/fexit/fmod_ret.

[Andrii Nakryiko]

On Mon, Mar 21, 2022 at 4:24 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Fri, Mar 18, 2022 at 12:13 PM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > On Tue, Mar 15, 2022 at 05:42:30PM -0700, Kui-Feng Lee wrote:
> > > Add a bpf_cookie field to attach a cookie to an instance of struct
> > > bpf_link.  The cookie of a bpf_link will be installed when calling the
> > > associated program to make it available to the program.
> > >
> > > Signed-off-by: Kui-Feng Lee <kuifeng@fb.com>
> > > ---
> > >  arch/x86/net/bpf_jit_comp.c    |  4 ++--
> > >  include/linux/bpf.h            |  1 +
> > >  include/uapi/linux/bpf.h       |  1 +
> > >  kernel/bpf/syscall.c           | 11 +++++++----
> > >  kernel/trace/bpf_trace.c       | 17 +++++++++++++++++
> > >  tools/include/uapi/linux/bpf.h |  1 +
> > >  tools/lib/bpf/bpf.c            | 14 ++++++++++++++
> > >  tools/lib/bpf/bpf.h            |  1 +
> > >  tools/lib/bpf/libbpf.map       |  1 +
> > >  9 files changed, 45 insertions(+), 6 deletions(-)
> >
> > please split kernel and libbpf changes into two different patches.
> >
>
> +1
>
> > > diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
> > > index f69ce3a01385..dbbf09c84c21 100644
> > > --- a/tools/lib/bpf/bpf.c
> > > +++ b/tools/lib/bpf/bpf.c
> > > @@ -1133,6 +1133,20 @@ int bpf_raw_tracepoint_open(const char *name, int prog_fd)
> > >       return libbpf_err_errno(fd);
> > >  }
> > >
> > > +int bpf_raw_tracepoint_cookie_open(const char *name, int prog_fd, __u64 bpf_cookie)
> >
> > lets introduce opts style to raw_tp_open instead.
>
> I remember I brought this up earlier, but I forgot the outcome. What
> if don't touch BPF_RAW_TRACEPOINT_OPEN and instead allow to create all
> the same links through more universal BPF_LINK_CREATE command. And
> only there we add bpf_cookie? There are few advantages:
>
> 1. We can separate raw_tracepoint and trampoline-based programs more
> cleanly in UAPI (it will be two separate structs: link_create.raw_tp
> with raw tracepoint name vs link_create.trampoline, or whatever the
> name, with cookie and stuff). Remember that raw_tp won't support
> bpf_cookie for now, so it would be another advantage not to promise
> cookie in UAPI.
>
> 2. libbpf can be smart enough to pick either RAW_TRACEPOINT_OPEN (and
> reject it if bpf_cookie is non-zero) or BPF_LINK_CREATE, depending on
> kernel support. So users would need to only use bpf_link_create()
> moving forward with all the backwards compatibility preserved.
>
>

Oh, and we need bpf_program__attach_trace_opts() as well (regardless
of whether it is BPF_RAW_TRACEPOINT_OPEN or BPF_LINK_CREATE).

> >
> > > +{
> > > +     union bpf_attr attr;
> > > +     int fd;
> > > +
> > > +     memset(&attr, 0, sizeof(attr));
> > > +     attr.raw_tracepoint.name = ptr_to_u64(name);
> > > +     attr.raw_tracepoint.prog_fd = prog_fd;
> > > +     attr.raw_tracepoint.bpf_cookie = bpf_cookie;
> > > +
> > > +     fd = sys_bpf_fd(BPF_RAW_TRACEPOINT_OPEN, &attr, sizeof(attr));
> > > +     return libbpf_err_errno(fd);
> > > +}
> > > +
> > >  int bpf_btf_load(const void *btf_data, size_t btf_size, const struct bpf_btf_load_opts *opts)
> > >  {
> > >       const size_t attr_sz = offsetofend(union bpf_attr, btf_log_level);
> > > diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
> > > index 5253cb4a4c0a..23bebcdaf23b 100644
> > > --- a/tools/lib/bpf/bpf.h
> > > +++ b/tools/lib/bpf/bpf.h
> > > @@ -477,6 +477,7 @@ LIBBPF_API int bpf_prog_query(int target_fd, enum bpf_attach_type type,
> > >                             __u32 query_flags, __u32 *attach_flags,
> > >                             __u32 *prog_ids, __u32 *prog_cnt);
> > >  LIBBPF_API int bpf_raw_tracepoint_open(const char *name, int prog_fd);
> > > +LIBBPF_API int bpf_raw_tracepoint_cookie_open(const char *name, int prog_fd, __u64 bpf_cookie);
> > >  LIBBPF_API int bpf_task_fd_query(int pid, int fd, __u32 flags, char *buf,
> > >                                __u32 *buf_len, __u32 *prog_id, __u32 *fd_type,
> > >                                __u64 *probe_offset, __u64 *probe_addr);
> > > diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> > > index df1b947792c8..20f947a385fa 100644
> > > --- a/tools/lib/bpf/libbpf.map
> > > +++ b/tools/lib/bpf/libbpf.map
> > > @@ -434,6 +434,7 @@ LIBBPF_0.7.0 {
> > >               bpf_xdp_detach;
> > >               bpf_xdp_query;
> > >               bpf_xdp_query_id;
> > > +             bpf_raw_tracepoint_cookie_open;
>
> this (if still needed) should go into 0.8.0 section
>
> > >               libbpf_probe_bpf_helper;
> > >               libbpf_probe_bpf_map_type;
> > >               libbpf_probe_bpf_prog_type;
> > > --
> > > 2.30.2
> > >
> >
> > --

Re: [PATCH bpf-next v2 3/4] bpf, x86: Support BPF cookie for fentry/fexit/fmod_ret.

[Kui-Feng Lee]

On Mon, 2022-03-21 at 16:37 -0700, Andrii Nakryiko wrote:
> On Mon, Mar 21, 2022 at 4:24 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> > 
> > On Fri, Mar 18, 2022 at 12:13 PM Alexei Starovoitov
> > <alexei.starovoitov@gmail.com> wrote:
> > > 
> > > On Tue, Mar 15, 2022 at 05:42:30PM -0700, Kui-Feng Lee wrote:
> > > > Add a bpf_cookie field to attach a cookie to an instance of
> > > > struct
> > > > bpf_link.  The cookie of a bpf_link will be installed when
> > > > calling the
> > > > associated program to make it available to the program.
> > > > 
> > > > Signed-off-by: Kui-Feng Lee <kuifeng@fb.com>
> > > > ---
> > > >  arch/x86/net/bpf_jit_comp.c    |  4 ++--
> > > >  include/linux/bpf.h            |  1 +
> > > >  include/uapi/linux/bpf.h       |  1 +
> > > >  kernel/bpf/syscall.c           | 11 +++++++----
> > > >  kernel/trace/bpf_trace.c       | 17 +++++++++++++++++
> > > >  tools/include/uapi/linux/bpf.h |  1 +
> > > >  tools/lib/bpf/bpf.c            | 14 ++++++++++++++
> > > >  tools/lib/bpf/bpf.h            |  1 +
> > > >  tools/lib/bpf/libbpf.map       |  1 +
> > > >  9 files changed, 45 insertions(+), 6 deletions(-)
> > > 
> > > please split kernel and libbpf changes into two different
> > > patches.
> > > 
> > 
> > +1
> > 
> > > > diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
> > > > index f69ce3a01385..dbbf09c84c21 100644
> > > > --- a/tools/lib/bpf/bpf.c
> > > > +++ b/tools/lib/bpf/bpf.c
> > > > @@ -1133,6 +1133,20 @@ int bpf_raw_tracepoint_open(const char
> > > > *name, int prog_fd)
> > > >       return libbpf_err_errno(fd);
> > > >  }
> > > > 
> > > > +int bpf_raw_tracepoint_cookie_open(const char *name, int
> > > > prog_fd, __u64 bpf_cookie)
> > > 
> > > lets introduce opts style to raw_tp_open instead.
> > 
> > I remember I brought this up earlier, but I forgot the outcome.
> > What
> > if don't touch BPF_RAW_TRACEPOINT_OPEN and instead allow to create
> > all
> > the same links through more universal BPF_LINK_CREATE command. And
> > only there we add bpf_cookie? There are few advantages:
> > 
> > 1. We can separate raw_tracepoint and trampoline-based programs
> > more
> > cleanly in UAPI (it will be two separate structs:
> > link_create.raw_tp
> > with raw tracepoint name vs link_create.trampoline, or whatever the
> > name, with cookie and stuff). Remember that raw_tp won't support
> > bpf_cookie for now, so it would be another advantage not to promise
> > cookie in UAPI.
> > 
> > 2. libbpf can be smart enough to pick either RAW_TRACEPOINT_OPEN
> > (and
> > reject it if bpf_cookie is non-zero) or BPF_LINK_CREATE, depending
> > on
> > kernel support. So users would need to only use bpf_link_create()
> > moving forward with all the backwards compatibility preserved.
> > 
> > 
> 
> Oh, and we need bpf_program__attach_trace_opts() as well (regardless
> of whether it is BPF_RAW_TRACEPOINT_OPEN or BPF_LINK_CREATE).
> 
> > 
I removed raw_tp_open_opts() since raw_tp won't support bpf_cookie.
Implemented only bpf_program__attach_trace_opts().

Re: [PATCH bpf-next v2 3/4] bpf, x86: Support BPF cookie for fentry/fexit/fmod_ret.

[Alexei Starovoitov]

On Mon, Mar 21, 2022 at 4:24 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> I remember I brought this up earlier, but I forgot the outcome. What
> if don't touch BPF_RAW_TRACEPOINT_OPEN and instead allow to create all
> the same links through more universal BPF_LINK_CREATE command. And
> only there we add bpf_cookie? There are few advantages:
>
> 1. We can separate raw_tracepoint and trampoline-based programs more
> cleanly in UAPI (it will be two separate structs: link_create.raw_tp
> with raw tracepoint name vs link_create.trampoline, or whatever the
> name, with cookie and stuff). Remember that raw_tp won't support
> bpf_cookie for now, so it would be another advantage not to promise
> cookie in UAPI.

What would it look like?
Technically link_create has prog_fd and perf_event.bpf_cookie
already.

        case BPF_PROG_TYPE_TRACING:
                ret = tracing_bpf_link_attach(attr, uattr, prog);
would just gain a few more checks for prog->expected_attach_type ?

Then link_create cmd will be equivalent to raw_tp_open.
With and without bpf_cookie.
?

Re: [PATCH bpf-next v2 3/4] bpf, x86: Support BPF cookie for fentry/fexit/fmod_ret.

[Andrii Nakryiko]

On Mon, Mar 21, 2022 at 6:15 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Mon, Mar 21, 2022 at 4:24 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > I remember I brought this up earlier, but I forgot the outcome. What
> > if don't touch BPF_RAW_TRACEPOINT_OPEN and instead allow to create all
> > the same links through more universal BPF_LINK_CREATE command. And
> > only there we add bpf_cookie? There are few advantages:
> >
> > 1. We can separate raw_tracepoint and trampoline-based programs more
> > cleanly in UAPI (it will be two separate structs: link_create.raw_tp
> > with raw tracepoint name vs link_create.trampoline, or whatever the
> > name, with cookie and stuff). Remember that raw_tp won't support
> > bpf_cookie for now, so it would be another advantage not to promise
> > cookie in UAPI.
>
> What would it look like?
> Technically link_create has prog_fd and perf_event.bpf_cookie
> already.
>
>         case BPF_PROG_TYPE_TRACING:
>                 ret = tracing_bpf_link_attach(attr, uattr, prog);
> would just gain a few more checks for prog->expected_attach_type ?
>
> Then link_create cmd will be equivalent to raw_tp_open.
> With and without bpf_cookie.
> ?

Yes, except I'd leave perf_event for perf_event-based attachments
(kprobe, uprobe, tracepoint) and would define a separate substruct for
trampoline-based programs. Something like this (I only compile-tested
it, of course). I've also simplified prog_type/expected_attach_type
logic a bit because it felt like a total maze to me and I was getting
lost all the time. Gmail will probably corrupt all the whitespaces,
sorry about that in advance.

Seems like we could already attach BPF_PROG_TYPE_EXT both through
RAW_TRACEPOINT_OPEN and LINK_CREATE, I didn't realize that. The
"patch" below leaves raw_tp handling
(BPF_PROG_TYPE_TRACING+BPF_TRACE_RAW_TP, BPF_PROG_TYPE_RAW_TRACEPOINT,
and BPF_PROG_TYPE_RAW_TRACEPOINT_WRITABLE) in RAW_TRACEPOINT_OPEN. If
we want to completely unify all the bpf_link creations under
LINK_CREATE, see extra small "patch" at the very bottom.

P.S. While looking at this, I also realized that we should probably
enforce zeros after all those kprobe_multi, trampoline, perf_event,
etc sections (depending on specific program type). We missed that
initially and don't enforce it right now (I don't think anyone sane
would rely on this, so probably best to fix this soon-ish).

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index d14b10b85e51..5692d4381e3b 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -1489,6 +1489,9 @@ union bpf_attr {
                 __aligned_u64    addrs;
                 __aligned_u64    cookies;
             } kprobe_multi;
+            struct {
+                __u64        cookie;
+            } trampoline;
         };
     } link_create;

diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index cdaa1152436a..819f30952cc4 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -2704,7 +2704,7 @@ static const struct bpf_link_ops bpf_tracing_link_lops = {

 static int bpf_tracing_prog_attach(struct bpf_prog *prog,
                    int tgt_prog_fd,
-                   u32 btf_id)
+                   u32 btf_id, u64 cookie)
 {
     struct bpf_link_primer link_primer;
     struct bpf_prog *tgt_prog = NULL;
@@ -2840,7 +2840,7 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog,
     if (err)
         goto out_unlock;

-    err = bpf_trampoline_link_prog(prog, tr);
+    err = bpf_trampoline_link_prog(prog, tr /* , cookie */);
     if (err) {
         bpf_link_cleanup(&link_primer);
         link = NULL;
@@ -3065,7 +3065,7 @@ static int bpf_raw_tracepoint_open(const union
bpf_attr *attr)
             tp_name = prog->aux->attach_func_name;
             break;
         }
-        err = bpf_tracing_prog_attach(prog, 0, 0);
+        err = bpf_tracing_prog_attach(prog, 0, 0, 0);
         if (err >= 0)
             return err;
         goto out_put_prog;
@@ -3189,7 +3189,12 @@ attach_type_to_prog_type(enum bpf_attach_type
attach_type)
     case BPF_CGROUP_SETSOCKOPT:
         return BPF_PROG_TYPE_CGROUP_SOCKOPT;
     case BPF_TRACE_ITER:
+    case BPF_TRACE_FENTRY:
+    case BPF_TRACE_FEXIT:
+    case BPF_MODIFY_RETURN:
         return BPF_PROG_TYPE_TRACING;
+    case BPF_LSM_MAC:
+        return BPF_PROG_TYPE_LSM;
     case BPF_SK_LOOKUP:
         return BPF_PROG_TYPE_SK_LOOKUP;
     case BPF_XDP:
@@ -4246,21 +4251,6 @@ static int bpf_map_do_batch(const union bpf_attr *attr,
     return err;
 }

-static int tracing_bpf_link_attach(const union bpf_attr *attr, bpfptr_t uattr,
-                   struct bpf_prog *prog)
-{
-    if (attr->link_create.attach_type != prog->expected_attach_type)
-        return -EINVAL;
-
-    if (prog->expected_attach_type == BPF_TRACE_ITER)
-        return bpf_iter_link_attach(attr, uattr, prog);
-    else if (prog->type == BPF_PROG_TYPE_EXT)
-        return bpf_tracing_prog_attach(prog,
-                           attr->link_create.target_fd,
-                           attr->link_create.target_btf_id);
-    return -EINVAL;
-}
-
 #define BPF_LINK_CREATE_LAST_FIELD link_create.kprobe_multi.cookies
 static int link_create(union bpf_attr *attr, bpfptr_t uattr)
 {
@@ -4282,15 +4272,13 @@ static int link_create(union bpf_attr *attr,
bpfptr_t uattr)

     switch (prog->type) {
     case BPF_PROG_TYPE_EXT:
-        ret = tracing_bpf_link_attach(attr, uattr, prog);
-        goto out;
+        break;
     case BPF_PROG_TYPE_PERF_EVENT:
     case BPF_PROG_TYPE_TRACEPOINT:
         if (attr->link_create.attach_type != BPF_PERF_EVENT) {
             ret = -EINVAL;
             goto out;
         }
-        ptype = prog->type;
         break;
     case BPF_PROG_TYPE_KPROBE:
         if (attr->link_create.attach_type != BPF_PERF_EVENT &&
@@ -4298,7 +4286,6 @@ static int link_create(union bpf_attr *attr,
bpfptr_t uattr)
             ret = -EINVAL;
             goto out;
         }
-        ptype = prog->type;
         break;
     default:
         ptype = attach_type_to_prog_type(attr->link_create.attach_type);
@@ -4309,7 +4296,7 @@ static int link_create(union bpf_attr *attr,
bpfptr_t uattr)
         break;
     }

-    switch (ptype) {
+    switch (prog->type) {
     case BPF_PROG_TYPE_CGROUP_SKB:
     case BPF_PROG_TYPE_CGROUP_SOCK:
     case BPF_PROG_TYPE_CGROUP_SOCK_ADDR:
@@ -4319,8 +4306,25 @@ static int link_create(union bpf_attr *attr,
bpfptr_t uattr)
     case BPF_PROG_TYPE_CGROUP_SOCKOPT:
         ret = cgroup_bpf_link_attach(attr, prog);
         break;
+    case BPF_PROG_TYPE_EXT:
+        ret = bpf_tracing_prog_attach(prog,
+                          attr->link_create.target_fd,
+                          attr->link_create.target_btf_id,
+                          attr->link_create.trampoline.cookie);
+        break;
+    case BPF_PROG_TYPE_LSM:
     case BPF_PROG_TYPE_TRACING:
-        ret = tracing_bpf_link_attach(attr, uattr, prog);
+        if (attr->link_create.attach_type != prog->expected_attach_type) {
+            ret = -EINVAL;
+            goto out;
+        }
+        if (prog->expected_attach_type == BPF_TRACE_ITER)
+            ret = bpf_iter_link_attach(attr, uattr, prog);
+        else
+            ret = bpf_tracing_prog_attach(prog,
+                              attr->link_create.target_fd,
+                              attr->link_create.target_btf_id,
+                              attr->link_create.trampoline.cookie);
         break;
     case BPF_PROG_TYPE_FLOW_DISSECTOR:
     case BPF_PROG_TYPE_SK_LOOKUP:



And then for moving raw_tp-related attachments into LINK_CREATE, I'd
add this to UAPI and wire everything in kernel/bpf/syscall.c
accordingly:

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 5692d4381e3b..a85403f89569 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -1492,6 +1492,9 @@ union bpf_attr {
                        struct {
                                __u64           cookie;
                        } trampoline;
+                       struct {
+                               __aligned_u64   name;
+                       } raw_tp;
                };
        } link_create;

Re: [PATCH bpf-next v2 3/4] bpf, x86: Support BPF cookie for fentry/fexit/fmod_ret.

[Kui-Feng Lee]

On Mon, 2022-03-21 at 21:32 -0700, Andrii Nakryiko wrote:
> On Mon, Mar 21, 2022 at 6:15 PM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> > 
> > On Mon, Mar 21, 2022 at 4:24 PM Andrii Nakryiko
> > <andrii.nakryiko@gmail.com> wrote:
> > > 
> > > I remember I brought this up earlier, but I forgot the outcome.
> > > What
> > > if don't touch BPF_RAW_TRACEPOINT_OPEN and instead allow to
> > > create all
> > > the same links through more universal BPF_LINK_CREATE command.
> > > And
> > > only there we add bpf_cookie? There are few advantages:
> > > 
> > > 1. We can separate raw_tracepoint and trampoline-based programs
> > > more
> > > cleanly in UAPI (it will be two separate structs:
> > > link_create.raw_tp
> > > with raw tracepoint name vs link_create.trampoline, or whatever
> > > the
> > > name, with cookie and stuff). Remember that raw_tp won't support
> > > bpf_cookie for now, so it would be another advantage not to
> > > promise
> > > cookie in UAPI.
> > 
> > What would it look like?
> > Technically link_create has prog_fd and perf_event.bpf_cookie
> > already.
> > 
> >         case BPF_PROG_TYPE_TRACING:
> >                 ret = tracing_bpf_link_attach(attr, uattr, prog);
> > would just gain a few more checks for prog->expected_attach_type ?
> > 
> > Then link_create cmd will be equivalent to raw_tp_open.
> > With and without bpf_cookie.
> > ?
> 
> Yes, except I'd leave perf_event for perf_event-based attachments
> (kprobe, uprobe, tracepoint) and would define a separate substruct
> for
> trampoline-based programs. Something like this (I only compile-tested
> it, of course). I've also simplified prog_type/expected_attach_type
> logic a bit because it felt like a total maze to me and I was getting
> lost all the time. Gmail will probably corrupt all the whitespaces,
> sorry about that in advance.
> 
> Seems like we could already attach BPF_PROG_TYPE_EXT both through
> RAW_TRACEPOINT_OPEN and LINK_CREATE, I didn't realize that. The
> "patch" below leaves raw_tp handling
> (BPF_PROG_TYPE_TRACING+BPF_TRACE_RAW_TP,
> BPF_PROG_TYPE_RAW_TRACEPOINT,
> and BPF_PROG_TYPE_RAW_TRACEPOINT_WRITABLE) in RAW_TRACEPOINT_OPEN. If
> we want to completely unify all the bpf_link creations under
> LINK_CREATE, see extra small "patch" at the very bottom.

I just implemented and tested a patch of tracing links with
bpf_link_create, so it can be done with both raw_tp_open and
bpf_link_create.

Do we want to remove raw_tp_open() eventually?  Should I remove
raw_tp_open() supports of cookies?

Re: [PATCH bpf-next v2 3/4] bpf, x86: Support BPF cookie for fentry/fexit/fmod_ret.

[Andrii Nakryiko]

On Tue, Apr 5, 2022 at 10:35 PM Kui-Feng Lee <kuifeng@fb.com> wrote:
>
> On Mon, 2022-03-21 at 21:32 -0700, Andrii Nakryiko wrote:
> > On Mon, Mar 21, 2022 at 6:15 PM Alexei Starovoitov
> > <alexei.starovoitov@gmail.com> wrote:
> > >
> > > On Mon, Mar 21, 2022 at 4:24 PM Andrii Nakryiko
> > > <andrii.nakryiko@gmail.com> wrote:
> > > >
> > > > I remember I brought this up earlier, but I forgot the outcome.
> > > > What
> > > > if don't touch BPF_RAW_TRACEPOINT_OPEN and instead allow to
> > > > create all
> > > > the same links through more universal BPF_LINK_CREATE command.
> > > > And
> > > > only there we add bpf_cookie? There are few advantages:
> > > >
> > > > 1. We can separate raw_tracepoint and trampoline-based programs
> > > > more
> > > > cleanly in UAPI (it will be two separate structs:
> > > > link_create.raw_tp
> > > > with raw tracepoint name vs link_create.trampoline, or whatever
> > > > the
> > > > name, with cookie and stuff). Remember that raw_tp won't support
> > > > bpf_cookie for now, so it would be another advantage not to
> > > > promise
> > > > cookie in UAPI.
> > >
> > > What would it look like?
> > > Technically link_create has prog_fd and perf_event.bpf_cookie
> > > already.
> > >
> > >         case BPF_PROG_TYPE_TRACING:
> > >                 ret = tracing_bpf_link_attach(attr, uattr, prog);
> > > would just gain a few more checks for prog->expected_attach_type ?
> > >
> > > Then link_create cmd will be equivalent to raw_tp_open.
> > > With and without bpf_cookie.
> > > ?
> >
> > Yes, except I'd leave perf_event for perf_event-based attachments
> > (kprobe, uprobe, tracepoint) and would define a separate substruct
> > for
> > trampoline-based programs. Something like this (I only compile-tested
> > it, of course). I've also simplified prog_type/expected_attach_type
> > logic a bit because it felt like a total maze to me and I was getting
> > lost all the time. Gmail will probably corrupt all the whitespaces,
> > sorry about that in advance.
> >
> > Seems like we could already attach BPF_PROG_TYPE_EXT both through
> > RAW_TRACEPOINT_OPEN and LINK_CREATE, I didn't realize that. The
> > "patch" below leaves raw_tp handling
> > (BPF_PROG_TYPE_TRACING+BPF_TRACE_RAW_TP,
> > BPF_PROG_TYPE_RAW_TRACEPOINT,
> > and BPF_PROG_TYPE_RAW_TRACEPOINT_WRITABLE) in RAW_TRACEPOINT_OPEN. If
> > we want to completely unify all the bpf_link creations under
> > LINK_CREATE, see extra small "patch" at the very bottom.
>
> I just implemented and tested a patch of tracing links with
> bpf_link_create, so it can be done with both raw_tp_open and
> bpf_link_create.
>

Nice, please send it as part of your cookie patch set in a separate patch.

> Do we want to remove raw_tp_open() eventually?  Should I remove
> raw_tp_open() supports of cookies?

We can't remove existing Linux UAPI, but we can stop extending them.
So I'd say let's add cookie only through CREATE_LINK and leave
RAW_TRACEPOINT_OPEN as is.

>
>

Re: [PATCH bpf-next v2 3/4] bpf, x86: Support BPF cookie for fentry/fexit/fmod_ret.

[Andrii Nakryiko]

On Tue, Mar 15, 2022 at 5:44 PM Kui-Feng Lee <kuifeng@fb.com> wrote:
>
> Add a bpf_cookie field to attach a cookie to an instance of struct
> bpf_link.  The cookie of a bpf_link will be installed when calling the
> associated program to make it available to the program.
>
> Signed-off-by: Kui-Feng Lee <kuifeng@fb.com>
> ---
>  arch/x86/net/bpf_jit_comp.c    |  4 ++--
>  include/linux/bpf.h            |  1 +
>  include/uapi/linux/bpf.h       |  1 +
>  kernel/bpf/syscall.c           | 11 +++++++----
>  kernel/trace/bpf_trace.c       | 17 +++++++++++++++++
>  tools/include/uapi/linux/bpf.h |  1 +
>  tools/lib/bpf/bpf.c            | 14 ++++++++++++++
>  tools/lib/bpf/bpf.h            |  1 +
>  tools/lib/bpf/libbpf.map       |  1 +
>  9 files changed, 45 insertions(+), 6 deletions(-)
>
> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> index 29775a475513..5fab8530e909 100644
> --- a/arch/x86/net/bpf_jit_comp.c
> +++ b/arch/x86/net/bpf_jit_comp.c
> @@ -1753,8 +1753,8 @@ static int invoke_bpf_prog(const struct btf_func_model *m, u8 **pprog,
>
>         EMIT1(0x52);             /* push rdx */
>
> -       /* mov rdi, 0 */
> -       emit_mov_imm64(&prog, BPF_REG_1, 0, 0);
> +       /* mov rdi, cookie */
> +       emit_mov_imm64(&prog, BPF_REG_1, (long) l->cookie >> 32, (u32) (long) l->cookie);

why __u64 to long casting? I don't think you need to cast anything at
all, but if you want to make that more explicit than just casting to
(u32) should be fine, no?

>
>         /* Prepare struct bpf_trace_run_ctx.
>          * sub rsp, sizeof(struct bpf_trace_run_ctx)
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index d20a23953696..9469f9264b4f 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -1040,6 +1040,7 @@ struct bpf_link {
>         struct bpf_prog *prog;
>         struct work_struct work;
>         struct hlist_node tramp_hlist;
> +       u64 cookie;

I was a bit hesitant about adding tramp_hlist into generic struct
bpf_link, but now with also cookie there I'm even more convinced that
it's not the right thing to do... Some BPF links won't have cookie,
some (like multi-kprobe) will have lots of them.

Should we create struct bpf_tramp_link {} which will have tramp_hlist
and cookie? As for tramp_hlist, we can probably also keep it back in
bpf_prog_aux and just fetch it through link->prog->aux->tramp_hlist in
trampoline code. This might reduce amount of code churn in patch 1.

Thoughts?

>  };
>
>  struct bpf_link_ops {

[...]

Re: [PATCH bpf-next v2 3/4] bpf, x86: Support BPF cookie for fentry/fexit/fmod_ret.

[Kui-Feng Lee]

On Mon, 2022-03-21 at 16:18 -0700, Andrii Nakryiko wrote:
> On Tue, Mar 15, 2022 at 5:44 PM Kui-Feng Lee <kuifeng@fb.com> wrote:
> > 
> > Add a bpf_cookie field to attach a cookie to an instance of struct
> > bpf_link.  The cookie of a bpf_link will be installed when calling
> > the
> > associated program to make it available to the program.
> > 
> > Signed-off-by: Kui-Feng Lee <kuifeng@fb.com>
> > ---
> >  arch/x86/net/bpf_jit_comp.c    |  4 ++--
> >  include/linux/bpf.h            |  1 +
> >  include/uapi/linux/bpf.h       |  1 +
> >  kernel/bpf/syscall.c           | 11 +++++++----
> >  kernel/trace/bpf_trace.c       | 17 +++++++++++++++++
> >  tools/include/uapi/linux/bpf.h |  1 +
> >  tools/lib/bpf/bpf.c            | 14 ++++++++++++++
> >  tools/lib/bpf/bpf.h            |  1 +
> >  tools/lib/bpf/libbpf.map       |  1 +
> >  9 files changed, 45 insertions(+), 6 deletions(-)
> > 
> > diff --git a/arch/x86/net/bpf_jit_comp.c
> > b/arch/x86/net/bpf_jit_comp.c
> > index 29775a475513..5fab8530e909 100644
> > --- a/arch/x86/net/bpf_jit_comp.c
> > +++ b/arch/x86/net/bpf_jit_comp.c
> > @@ -1753,8 +1753,8 @@ static int invoke_bpf_prog(const struct
> > btf_func_model *m, u8 **pprog,
> > 
> >         EMIT1(0x52);             /* push rdx */
> > 
> > -       /* mov rdi, 0 */
> > -       emit_mov_imm64(&prog, BPF_REG_1, 0, 0);
> > +       /* mov rdi, cookie */
> > +       emit_mov_imm64(&prog, BPF_REG_1, (long) l->cookie >> 32,
> > (u32) (long) l->cookie);
> 
> why __u64 to long casting? I don't think you need to cast anything at
> all, but if you want to make that more explicit than just casting to
> (u32) should be fine, no?
> 
> > 
> >         /* Prepare struct bpf_trace_run_ctx.
> >          * sub rsp, sizeof(struct bpf_trace_run_ctx)
> > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > index d20a23953696..9469f9264b4f 100644
> > --- a/include/linux/bpf.h
> > +++ b/include/linux/bpf.h
> > @@ -1040,6 +1040,7 @@ struct bpf_link {
> >         struct bpf_prog *prog;
> >         struct work_struct work;
> >         struct hlist_node tramp_hlist;
> > +       u64 cookie;
> 
> I was a bit hesitant about adding tramp_hlist into generic struct
> bpf_link, but now with also cookie there I'm even more convinced that
> it's not the right thing to do... Some BPF links won't have cookie,
> some (like multi-kprobe) will have lots of them.
> 
> Should we create struct bpf_tramp_link {} which will have tramp_hlist
> and cookie? As for tramp_hlist, we can probably also keep it back in
> bpf_prog_aux and just fetch it through link->prog->aux->tramp_hlist
> in
> trampoline code. This might reduce amount of code churn in patch 1.

Do you mean a struct likes like?

struct bpf_tramp_link {
  struct bpf_link link;
  struct hlist_node tramp_hlist;
  u64 cookie;
};

I like this idea since we don't use cookie for every bpf_link.
But, could you give me an example that we don't want a cookie?

Re: [PATCH bpf-next v2 3/4] bpf, x86: Support BPF cookie for fentry/fexit/fmod_ret.

[Andrii Nakryiko]

On Tue, Mar 22, 2022 at 9:08 AM Kui-Feng Lee <kuifeng@fb.com> wrote:
>
> On Mon, 2022-03-21 at 16:18 -0700, Andrii Nakryiko wrote:
> > On Tue, Mar 15, 2022 at 5:44 PM Kui-Feng Lee <kuifeng@fb.com> wrote:
> > >
> > > Add a bpf_cookie field to attach a cookie to an instance of struct
> > > bpf_link.  The cookie of a bpf_link will be installed when calling
> > > the
> > > associated program to make it available to the program.
> > >
> > > Signed-off-by: Kui-Feng Lee <kuifeng@fb.com>
> > > ---
> > >  arch/x86/net/bpf_jit_comp.c    |  4 ++--
> > >  include/linux/bpf.h            |  1 +
> > >  include/uapi/linux/bpf.h       |  1 +
> > >  kernel/bpf/syscall.c           | 11 +++++++----
> > >  kernel/trace/bpf_trace.c       | 17 +++++++++++++++++
> > >  tools/include/uapi/linux/bpf.h |  1 +
> > >  tools/lib/bpf/bpf.c            | 14 ++++++++++++++
> > >  tools/lib/bpf/bpf.h            |  1 +
> > >  tools/lib/bpf/libbpf.map       |  1 +
> > >  9 files changed, 45 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/arch/x86/net/bpf_jit_comp.c
> > > b/arch/x86/net/bpf_jit_comp.c
> > > index 29775a475513..5fab8530e909 100644
> > > --- a/arch/x86/net/bpf_jit_comp.c
> > > +++ b/arch/x86/net/bpf_jit_comp.c
> > > @@ -1753,8 +1753,8 @@ static int invoke_bpf_prog(const struct
> > > btf_func_model *m, u8 **pprog,
> > >
> > >         EMIT1(0x52);             /* push rdx */
> > >
> > > -       /* mov rdi, 0 */
> > > -       emit_mov_imm64(&prog, BPF_REG_1, 0, 0);
> > > +       /* mov rdi, cookie */
> > > +       emit_mov_imm64(&prog, BPF_REG_1, (long) l->cookie >> 32,
> > > (u32) (long) l->cookie);
> >
> > why __u64 to long casting? I don't think you need to cast anything at
> > all, but if you want to make that more explicit than just casting to
> > (u32) should be fine, no?
> >
> > >
> > >         /* Prepare struct bpf_trace_run_ctx.
> > >          * sub rsp, sizeof(struct bpf_trace_run_ctx)
> > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > > index d20a23953696..9469f9264b4f 100644
> > > --- a/include/linux/bpf.h
> > > +++ b/include/linux/bpf.h
> > > @@ -1040,6 +1040,7 @@ struct bpf_link {
> > >         struct bpf_prog *prog;
> > >         struct work_struct work;
> > >         struct hlist_node tramp_hlist;
> > > +       u64 cookie;
> >
> > I was a bit hesitant about adding tramp_hlist into generic struct
> > bpf_link, but now with also cookie there I'm even more convinced that
> > it's not the right thing to do... Some BPF links won't have cookie,
> > some (like multi-kprobe) will have lots of them.
> >
> > Should we create struct bpf_tramp_link {} which will have tramp_hlist
> > and cookie? As for tramp_hlist, we can probably also keep it back in
> > bpf_prog_aux and just fetch it through link->prog->aux->tramp_hlist
> > in
> > trampoline code. This might reduce amount of code churn in patch 1.
>
> Do you mean a struct likes like?
>
> struct bpf_tramp_link {
>   struct bpf_link link;
>   struct hlist_node tramp_hlist;
>   u64 cookie;
> };

something like this, yes. Keep in mind that we already use struct
bpf_tracing_link which is used for all trampoline-based programs,
except for struct_ops. So we can either somehow make struct_ops just
result struct bpf_tracing_link (cc Martin for ideas, he was thinking
about doing proper bpf_link support for struct_ops anyways), or we'll
need this kind of struct inheritance to reuse the same layout between
struct_ops and struct bpf_tracing_link.

>
> I like this idea since we don't use cookie for every bpf_link.
> But, could you give me an example that we don't want a cookie?
>

For example, currently cgroup-based programs don't have cookie
support. So doesn't raw_tp, btw. But it's not only cases when we don't
support cookie, it's also cases like bpf_kprobe_multi_link which has a
separate array of cookies, so this u64 cookie is useless in such case.

Re: [PATCH bpf-next v2 3/4] bpf, x86: Support BPF cookie for fentry/fexit/fmod_ret.

[Kui-Feng Lee]

On Tue, 2022-03-22 at 14:06 -0700, Andrii Nakryiko wrote:
> On Tue, Mar 22, 2022 at 9:08 AM Kui-Feng Lee <kuifeng@fb.com> wrote:
> > 
> > On Mon, 2022-03-21 at 16:18 -0700, Andrii Nakryiko wrote:
> > > On Tue, Mar 15, 2022 at 5:44 PM Kui-Feng Lee <kuifeng@fb.com>
> > > wrote:
> > > > 
> > > > Add a bpf_cookie field to attach a cookie to an instance of
> > > > struct
> > > > bpf_link.  The cookie of a bpf_link will be installed when
> > > > calling
> > > > the
> > > > associated program to make it available to the program.
> > > > 
> > > > Signed-off-by: Kui-Feng Lee <kuifeng@fb.com>
> > > > ---
> > > >  arch/x86/net/bpf_jit_comp.c    |  4 ++--
> > > >  include/linux/bpf.h            |  1 +
> > > >  include/uapi/linux/bpf.h       |  1 +
> > > >  kernel/bpf/syscall.c           | 11 +++++++----
> > > >  kernel/trace/bpf_trace.c       | 17 +++++++++++++++++
> > > >  tools/include/uapi/linux/bpf.h |  1 +
> > > >  tools/lib/bpf/bpf.c            | 14 ++++++++++++++
> > > >  tools/lib/bpf/bpf.h            |  1 +
> > > >  tools/lib/bpf/libbpf.map       |  1 +
> > > >  9 files changed, 45 insertions(+), 6 deletions(-)
> > > > 
> > > > diff --git a/arch/x86/net/bpf_jit_comp.c
> > > > b/arch/x86/net/bpf_jit_comp.c
> > > > index 29775a475513..5fab8530e909 100644
> > > > --- a/arch/x86/net/bpf_jit_comp.c
> > > > +++ b/arch/x86/net/bpf_jit_comp.c
> > > > @@ -1753,8 +1753,8 @@ static int invoke_bpf_prog(const struct
> > > > btf_func_model *m, u8 **pprog,
> > > > 
> > > >         EMIT1(0x52);             /* push rdx */
> > > > 
> > > > -       /* mov rdi, 0 */
> > > > -       emit_mov_imm64(&prog, BPF_REG_1, 0, 0);
> > > > +       /* mov rdi, cookie */
> > > > +       emit_mov_imm64(&prog, BPF_REG_1, (long) l->cookie >>
> > > > 32,
> > > > (u32) (long) l->cookie);
> > > 
> > > why __u64 to long casting? I don't think you need to cast
> > > anything at
> > > all, but if you want to make that more explicit than just casting
> > > to
> > > (u32) should be fine, no?
> > > 
> > > > 
> > > >         /* Prepare struct bpf_trace_run_ctx.
> > > >          * sub rsp, sizeof(struct bpf_trace_run_ctx)
> > > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > > > index d20a23953696..9469f9264b4f 100644
> > > > --- a/include/linux/bpf.h
> > > > +++ b/include/linux/bpf.h
> > > > @@ -1040,6 +1040,7 @@ struct bpf_link {
> > > >         struct bpf_prog *prog;
> > > >         struct work_struct work;
> > > >         struct hlist_node tramp_hlist;
> > > > +       u64 cookie;
> > > 
> > > I was a bit hesitant about adding tramp_hlist into generic struct
> > > bpf_link, but now with also cookie there I'm even more convinced
> > > that
> > > it's not the right thing to do... Some BPF links won't have
> > > cookie,
> > > some (like multi-kprobe) will have lots of them.
> > > 
> > > Should we create struct bpf_tramp_link {} which will have
> > > tramp_hlist
> > > and cookie? As for tramp_hlist, we can probably also keep it back
> > > in
> > > bpf_prog_aux and just fetch it through link->prog->aux-
> > > >tramp_hlist
> > > in
> > > trampoline code. This might reduce amount of code churn in patch
> > > 1.
> > 
> > Do you mean a struct likes like?
> > 
> > struct bpf_tramp_link {
> >   struct bpf_link link;
> >   struct hlist_node tramp_hlist;
> >   u64 cookie;
> > };
> 
> something like this, yes. Keep in mind that we already use struct
> bpf_tracing_link which is used for all trampoline-based programs,
> except for struct_ops. So we can either somehow make struct_ops just
> result struct bpf_tracing_link (cc Martin for ideas, he was thinking
> about doing proper bpf_link support for struct_ops anyways), or we'll
> need this kind of struct inheritance to reuse the same layout between
> struct_ops and struct bpf_tracing_link.

I just introduced bpf_tramp_link. But, moved cookie to
bpf_tracing_link.  And, struct_ops creates bpf_tramp_link instead of
bpf_link.

[PATCH bpf-next v2 4/4] selftest/bpf: The test cses of BPF cookie for fentry/fexit/fmod_ret.

[Kui-Feng Lee]

Make sure BPF cookies are correct for fentry/fexit/fmod_ret.

Signed-off-by: Kui-Feng Lee <kuifeng@fb.com>
---
 .../selftests/bpf/prog_tests/bpf_cookie.c     | 61 +++++++++++++++++++
 .../selftests/bpf/progs/test_bpf_cookie.c     | 24 ++++++++
 2 files changed, 85 insertions(+)

diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c b/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c
index 0612e79a9281..6d06c5046e9c 100644
--- a/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c
+++ b/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c
@@ -237,6 +237,65 @@ static void pe_subtest(struct test_bpf_cookie *skel)
 	bpf_link__destroy(link);
 }
 
+static void tracing_subtest(struct test_bpf_cookie *skel)
+{
+	__u64 cookie;
+	int prog_fd;
+	int fentry_fd = -1, fexit_fd = -1, fmod_ret_fd = -1;
+	struct bpf_test_run_opts opts;
+
+	skel->bss->fentry_res = 0;
+	skel->bss->fexit_res = 0;
+
+	cookie = 0x100000;
+	prog_fd = bpf_program__fd(skel->progs.fentry_test1);
+	if (!ASSERT_GE(prog_fd, 0, "fentry.prog_fd"))
+		return;
+	fentry_fd = bpf_raw_tracepoint_cookie_open(NULL, prog_fd, cookie);
+	if (!ASSERT_GE(fentry_fd, 0, "fentry.open"))
+		return;
+
+	cookie = 0x200000;
+	prog_fd = bpf_program__fd(skel->progs.fexit_test1);
+	if (!ASSERT_GE(prog_fd, 0, "fexit.prog_fd"))
+		goto cleanup;
+	fexit_fd = bpf_raw_tracepoint_cookie_open(NULL, prog_fd, cookie);
+	if (!ASSERT_GE(fexit_fd, 0, "fexit.open"))
+		goto cleanup;
+
+	cookie = 0x300000;
+	prog_fd = bpf_program__fd(skel->progs.fmod_ret_test);
+	if (!ASSERT_GE(prog_fd, 0, "fmod_ret.prog_fd"))
+		goto cleanup;
+	fmod_ret_fd = bpf_raw_tracepoint_cookie_open(NULL, prog_fd, cookie);
+	if (!ASSERT_GE(fmod_ret_fd, 0, "fmod_ret.opoen"))
+		goto cleanup;
+
+	bzero(&opts, sizeof(opts));
+	opts.sz = sizeof(opts);
+	opts.repeat = 1;
+	prog_fd = bpf_program__fd(skel->progs.fentry_test1);
+	bpf_prog_test_run_opts(prog_fd, &opts);
+
+	bzero(&opts, sizeof(opts));
+	opts.sz = sizeof(opts);
+	opts.repeat = 1;
+	prog_fd = bpf_program__fd(skel->progs.fmod_ret_test);
+	bpf_prog_test_run_opts(prog_fd, &opts);
+
+	ASSERT_EQ(skel->bss->fentry_res, 0x100000, "fentry_res");
+	ASSERT_EQ(skel->bss->fexit_res, 0x200000, "fexit_res");
+	ASSERT_EQ(skel->bss->fmod_ret_res, 0x300000, "fmod_ret_res");
+
+cleanup:
+	if (fentry_fd >= 0)
+		close(fentry_fd);
+	if (fexit_fd >= 0)
+		close(fexit_fd);
+	if (fmod_ret_fd >= 0)
+		close(fmod_ret_fd);
+}
+
 void test_bpf_cookie(void)
 {
 	struct test_bpf_cookie *skel;
@@ -255,6 +314,8 @@ void test_bpf_cookie(void)
 		tp_subtest(skel);
 	if (test__start_subtest("perf_event"))
 		pe_subtest(skel);
+	if (test__start_subtest("tracing"))
+		tracing_subtest(skel);
 
 	test_bpf_cookie__destroy(skel);
 }
diff --git a/tools/testing/selftests/bpf/progs/test_bpf_cookie.c b/tools/testing/selftests/bpf/progs/test_bpf_cookie.c
index 2d3a7710e2ce..a9f83f46e7b7 100644
--- a/tools/testing/selftests/bpf/progs/test_bpf_cookie.c
+++ b/tools/testing/selftests/bpf/progs/test_bpf_cookie.c
@@ -14,6 +14,9 @@ int uprobe_res;
 int uretprobe_res;
 int tp_res;
 int pe_res;
+int fentry_res;
+int fexit_res;
+int fmod_ret_res;
 
 static void update(void *ctx, int *res)
 {
@@ -82,4 +85,25 @@ int handle_pe(struct pt_regs *ctx)
 	return 0;
 }
 
+SEC("fentry/bpf_fentry_test1")
+int BPF_PROG(fentry_test1, int a)
+{
+	update(ctx, &fentry_res);
+	return 0;
+}
+
+SEC("fexit/bpf_fentry_test1")
+int BPF_PROG(fexit_test1, int a, int ret)
+{
+	update(ctx, &fexit_res);
+	return 0;
+}
+
+SEC("fmod_ret/bpf_modify_return_test")
+int BPF_PROG(fmod_ret_test, int _a, int *_b, int _ret)
+{
+	update(ctx, &fmod_ret_res);
+	return 1234;
+}
+
 char _license[] SEC("license") = "GPL";
-- 
2.30.2

Re: [PATCH bpf-next v2 4/4] selftest/bpf: The test cses of BPF cookie for fentry/fexit/fmod_ret.

[Alexei Starovoitov]

On Tue, Mar 15, 2022 at 05:42:31PM -0700, Kui-Feng Lee wrote:
>  
> +SEC("fentry/bpf_fentry_test1")

Did we discuss whether it makes sense to specify cookie in the SEC() ?

Probably no one will be using cookie when prog is attached to a specific
function, but with support for poor man regex in SEC the cookie
might be useful?
Would we need a way to specify a set of cookies in SEC()?
Or specify a set of pairs of kernel_func+cookie?
None of it might be worth it.

Re: [PATCH bpf-next v2 4/4] selftest/bpf: The test cses of BPF cookie for fentry/fexit/fmod_ret.

[Kui-Feng Lee]

On Fri, 2022-03-18 at 12:21 -0700, Alexei Starovoitov wrote:
> On Tue, Mar 15, 2022 at 05:42:31PM -0700, Kui-Feng Lee wrote:
> >  
> > +SEC("fentry/bpf_fentry_test1")
> 
> Did we discuss whether it makes sense to specify cookie in the SEC()
> ?
> 
> Probably no one will be using cookie when prog is attached to a
> specific
> function, but with support for poor man regex in SEC the cookie
> might be useful?
> Would we need a way to specify a set of cookies in SEC()?
> Or specify a set of pairs of kernel_func+cookie?
> None of it might be worth it.

It makes sense to me to provide a way to specify cookies in the source
code of a BPF program.
However, it could be a very complicated syntax and/or difficult to
read.
Kernel_func+cookie, even Kernel_func_pattern+cookie, pairs are easy to
understand.
For more complicated cases, giving cookies at user space programs would
be a better choice.

Re: [PATCH bpf-next v2 4/4] selftest/bpf: The test cses of BPF cookie for fentry/fexit/fmod_ret.

[Andrii Nakryiko]

On Sun, Mar 20, 2022 at 1:44 AM Kui-Feng Lee <kuifeng@fb.com> wrote:
>
> On Fri, 2022-03-18 at 12:21 -0700, Alexei Starovoitov wrote:
> > On Tue, Mar 15, 2022 at 05:42:31PM -0700, Kui-Feng Lee wrote:
> > >
> > > +SEC("fentry/bpf_fentry_test1")
> >
> > Did we discuss whether it makes sense to specify cookie in the SEC()
> > ?
> >
> > Probably no one will be using cookie when prog is attached to a
> > specific
> > function, but with support for poor man regex in SEC the cookie
> > might be useful?
> > Would we need a way to specify a set of cookies in SEC()?
> > Or specify a set of pairs of kernel_func+cookie?
> > None of it might be worth it.
>
> It makes sense to me to provide a way to specify cookies in the source
> code of a BPF program.

I think it's not worth it. Think about this, if you have two fentry
programs with the same logic and you are able to *statically* define
two different cookies at compile time, the easy way to do this is:

static int my_logic(__u64 cookie) { /* do something smart that depends
on cookie value */ }

SEC("fentry/func1")
int BPF_PROG(handle_func1, ...) { return my_logic(123); }

SEC("fentry/func2")
int BPF_PROG(handle_func2, ...) {return my_logic(456); }

BPF cookie was added specifically for cases when this value is *only
known at runtime* and it's impractical/impossible to somehow embed it
into BPF program code. Let's not overcomplicate this, at least until
there is a strong use case for this.

> However, it could be a very complicated syntax and/or difficult to
> read.
> Kernel_func+cookie, even Kernel_func_pattern+cookie, pairs are easy to
> understand.
> For more complicated cases, giving cookies at user space programs would
> be a better choice.
>

Re: [PATCH bpf-next v2 4/4] selftest/bpf: The test cses of BPF cookie for fentry/fexit/fmod_ret.

[Andrii Nakryiko]

On Tue, Mar 15, 2022 at 5:44 PM Kui-Feng Lee <kuifeng@fb.com> wrote:
>
> Make sure BPF cookies are correct for fentry/fexit/fmod_ret.
>
> Signed-off-by: Kui-Feng Lee <kuifeng@fb.com>
> ---
>  .../selftests/bpf/prog_tests/bpf_cookie.c     | 61 +++++++++++++++++++
>  .../selftests/bpf/progs/test_bpf_cookie.c     | 24 ++++++++
>  2 files changed, 85 insertions(+)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c b/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c
> index 0612e79a9281..6d06c5046e9c 100644
> --- a/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c
> +++ b/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c
> @@ -237,6 +237,65 @@ static void pe_subtest(struct test_bpf_cookie *skel)
>         bpf_link__destroy(link);
>  }
>
> +static void tracing_subtest(struct test_bpf_cookie *skel)
> +{
> +       __u64 cookie;
> +       int prog_fd;
> +       int fentry_fd = -1, fexit_fd = -1, fmod_ret_fd = -1;
> +       struct bpf_test_run_opts opts;
> +
> +       skel->bss->fentry_res = 0;
> +       skel->bss->fexit_res = 0;
> +
> +       cookie = 0x100000;
> +       prog_fd = bpf_program__fd(skel->progs.fentry_test1);
> +       if (!ASSERT_GE(prog_fd, 0, "fentry.prog_fd"))
> +               return;

this can't return <= 0 if skeleton was loaded, don't bother doing
these checks, they just distract from the main objective of the test

> +       fentry_fd = bpf_raw_tracepoint_cookie_open(NULL, prog_fd, cookie);
> +       if (!ASSERT_GE(fentry_fd, 0, "fentry.open"))
> +               return;
> +
> +       cookie = 0x200000;
> +       prog_fd = bpf_program__fd(skel->progs.fexit_test1);
> +       if (!ASSERT_GE(prog_fd, 0, "fexit.prog_fd"))
> +               goto cleanup;
> +       fexit_fd = bpf_raw_tracepoint_cookie_open(NULL, prog_fd, cookie);
> +       if (!ASSERT_GE(fexit_fd, 0, "fexit.open"))
> +               goto cleanup;
> +
> +       cookie = 0x300000;
> +       prog_fd = bpf_program__fd(skel->progs.fmod_ret_test);
> +       if (!ASSERT_GE(prog_fd, 0, "fmod_ret.prog_fd"))
> +               goto cleanup;
> +       fmod_ret_fd = bpf_raw_tracepoint_cookie_open(NULL, prog_fd, cookie);
> +       if (!ASSERT_GE(fmod_ret_fd, 0, "fmod_ret.opoen"))
> +               goto cleanup;
> +
> +       bzero(&opts, sizeof(opts));
> +       opts.sz = sizeof(opts);
> +       opts.repeat = 1;

We have LIBBPF_OPTS() macro for working with opts, please use that.
But I think in this case NULL for opts will be equivalent. Seems like
kernel just ignores repeat parameter for tracing programs (and for
others repeat == 0 means repeat == 1).

> +       prog_fd = bpf_program__fd(skel->progs.fentry_test1);
> +       bpf_prog_test_run_opts(prog_fd, &opts);
> +
> +       bzero(&opts, sizeof(opts));
> +       opts.sz = sizeof(opts);
> +       opts.repeat = 1;
> +       prog_fd = bpf_program__fd(skel->progs.fmod_ret_test);
> +       bpf_prog_test_run_opts(prog_fd, &opts);
> +
> +       ASSERT_EQ(skel->bss->fentry_res, 0x100000, "fentry_res");
> +       ASSERT_EQ(skel->bss->fexit_res, 0x200000, "fexit_res");
> +       ASSERT_EQ(skel->bss->fmod_ret_res, 0x300000, "fmod_ret_res");
> +
> +cleanup:
> +       if (fentry_fd >= 0)
> +               close(fentry_fd);
> +       if (fexit_fd >= 0)
> +               close(fexit_fd);
> +       if (fmod_ret_fd >= 0)
> +               close(fmod_ret_fd);
> +}
> +
>  void test_bpf_cookie(void)
>  {
>         struct test_bpf_cookie *skel;
> @@ -255,6 +314,8 @@ void test_bpf_cookie(void)
>                 tp_subtest(skel);
>         if (test__start_subtest("perf_event"))
>                 pe_subtest(skel);
> +       if (test__start_subtest("tracing"))
> +               tracing_subtest(skel);

kprobes are also tracing, but this one is specifically about BPF
trampoline-based programs. Let's use "trampoline" here to
disambiguate.

>
>         test_bpf_cookie__destroy(skel);
>  }
> diff --git a/tools/testing/selftests/bpf/progs/test_bpf_cookie.c b/tools/testing/selftests/bpf/progs/test_bpf_cookie.c
> index 2d3a7710e2ce..a9f83f46e7b7 100644
> --- a/tools/testing/selftests/bpf/progs/test_bpf_cookie.c
> +++ b/tools/testing/selftests/bpf/progs/test_bpf_cookie.c
> @@ -14,6 +14,9 @@ int uprobe_res;
>  int uretprobe_res;
>  int tp_res;
>  int pe_res;
> +int fentry_res;
> +int fexit_res;
> +int fmod_ret_res;
>
>  static void update(void *ctx, int *res)
>  {
> @@ -82,4 +85,25 @@ int handle_pe(struct pt_regs *ctx)
>         return 0;
>  }
>
> +SEC("fentry/bpf_fentry_test1")
> +int BPF_PROG(fentry_test1, int a)
> +{
> +       update(ctx, &fentry_res);
> +       return 0;
> +}
> +
> +SEC("fexit/bpf_fentry_test1")
> +int BPF_PROG(fexit_test1, int a, int ret)
> +{
> +       update(ctx, &fexit_res);
> +       return 0;
> +}
> +
> +SEC("fmod_ret/bpf_modify_return_test")
> +int BPF_PROG(fmod_ret_test, int _a, int *_b, int _ret)
> +{
> +       update(ctx, &fmod_ret_res);
> +       return 1234;
> +}
> +
>  char _license[] SEC("license") = "GPL";
> --
> 2.30.2
>