All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH bpf-next 0/5] Attach a cookie to a tracing program.
@ 2022-01-26 21:48 Kui-Feng Lee
  2022-01-26 21:48 ` [PATCH bpf-next 1/5] bpf: Add a flags value on trampoline frames Kui-Feng Lee
                   ` (5 more replies)
  0 siblings, 6 replies; 18+ messages in thread
From: Kui-Feng Lee @ 2022-01-26 21:48 UTC (permalink / raw)
  To: bpf, ast, daniel, andrii; +Cc: Kui-Feng Lee

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

This changeset includes several major changes.

 - Add a new field bpf_cookie to struct raw_tracepoint, so that a user
   can attach a cookie to a program.

 - Store flags in trampoline frames to provide the flexibility of
   storing more values in these frames.

 - Store the program ID of the current BPF program in the trampoline
   frame.

 - The implmentation of bpf_get_attach_cookie() for tracing programs
   to read the attached cookie.

Kui-Feng Lee (5):
  bpf: Add a flags value on trampoline frames.
  bpf: Detect if a program needs its program ID.
  bpf, x86: Store program ID to trampoline frames.
  bpf: Attach a cookie to a BPF program.
  bpf: Implement bpf_get_attach_cookie() for tracing programs.

 arch/x86/net/bpf_jit_comp.c                   | 53 ++++++++++++++---
 include/linux/bpf.h                           |  3 +
 include/linux/filter.h                        |  3 +-
 include/uapi/linux/bpf.h                      |  1 +
 kernel/bpf/syscall.c                          | 12 ++--
 kernel/bpf/trampoline.c                       | 10 +++-
 kernel/bpf/verifier.c                         |  5 +-
 kernel/trace/bpf_trace.c                      | 45 ++++++++++++++-
 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 +
 .../selftests/bpf/prog_tests/bpf_cookie.c     | 57 +++++++++++++++++++
 .../selftests/bpf/progs/test_bpf_cookie.c     | 24 ++++++++
 14 files changed, 211 insertions(+), 19 deletions(-)

-- 
2.30.2


^ permalink raw reply	[flat|nested] 18+ messages in thread
* [PATCH bpf-next 1/5] bpf: Add a flags value on trampoline frames.
  2022-01-26 21:48 [PATCH bpf-next 0/5] Attach a cookie to a tracing program Kui-Feng Lee
@ 2022-01-26 21:48 ` Kui-Feng Lee
  2022-01-26 21:48 ` [PATCH bpf-next 2/5] bpf: Detect if a program needs its program ID Kui-Feng Lee
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 18+ messages in thread
From: Kui-Feng Lee @ 2022-01-26 21:48 UTC (permalink / raw)
  To: bpf, ast, daniel, andrii; +Cc: Kui-Feng Lee

The flags indicate what values are below nargs.  It appears only if
one or more values are there.  The order in the flags, from LSB to
MSB, indicates the order of values in the trampoline frame.  LSB is at
the highest location, close to the flags and nargs.

Signed-off-by: Kui-Feng Lee <kuifeng@fb.com>
---
 arch/x86/net/bpf_jit_comp.c | 17 ++++++++++++++++-
 kernel/bpf/verifier.c       |  2 +-
 kernel/trace/bpf_trace.c    | 23 ++++++++++++++++++++++-
 3 files changed, 39 insertions(+), 3 deletions(-)

diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index ce1f86f245c9..1c2d3ef57dad 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -1976,7 +1976,7 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
 				void *orig_call)
 {
 	int ret, i, nr_args = m->nr_args;
-	int regs_off, ip_off, args_off, stack_size = nr_args * 8;
+	int regs_off, flags_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];
@@ -2019,6 +2019,11 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
 	stack_size += 8;
 	args_off = stack_size;
 
+	if (flags & BPF_TRAMP_F_IP_ARG)
+		stack_size += 8; /* room for flags */
+
+	flags_off = stack_size;
+
 	if (flags & BPF_TRAMP_F_IP_ARG)
 		stack_size += 8; /* room for IP address argument */
 
@@ -2044,6 +2049,16 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
 	emit_mov_imm64(&prog, BPF_REG_0, 0, (u32) nr_args);
 	emit_stx(&prog, BPF_DW, BPF_REG_FP, BPF_REG_0, -args_off);
 
+	if (flags & BPF_TRAMP_F_IP_ARG) {
+		/* Store flags
+		 * move rax, flags
+		 * mov QWORD PTR [rbp - flags_off], rax
+		 */
+		emit_mov_imm64(&prog, BPF_REG_0, 0,
+			       (u32) (flags & BPF_TRAMP_F_IP_ARG));
+		emit_stx(&prog, BPF_DW, BPF_REG_FP, BPF_REG_0, -flags_off);
+	}
+
 	if (flags & BPF_TRAMP_F_IP_ARG) {
 		/* Store IP address of the traced function:
 		 * mov rax, QWORD PTR [rbp + 8]
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 8c5a46d41f28..ff91f5038010 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -13585,7 +13585,7 @@ static int do_misc_fixups(struct bpf_verifier_env *env)
 		if (prog_type == BPF_PROG_TYPE_TRACING &&
 		    insn->imm == BPF_FUNC_get_func_ip) {
 			/* Load IP address from ctx - 16 */
-			insn_buf[0] = BPF_LDX_MEM(BPF_DW, BPF_REG_0, BPF_REG_1, -16);
+			insn_buf[0] = BPF_LDX_MEM(BPF_DW, BPF_REG_0, BPF_REG_1, -24);
 
 			new_prog = bpf_patch_insn_data(env, i + delta, insn_buf, 1);
 			if (!new_prog)
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 06a9e220069e..bf2c9d11ad05 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -1009,10 +1009,31 @@ const struct bpf_func_proto bpf_snprintf_btf_proto = {
 	.arg5_type	= ARG_ANYTHING,
 };
 
+static int get_trampo_var_off(void *ctx, u32 flag)
+{
+	int off = 2;            /* All variables are placed before flags */
+	u32 flags = (u32)((u64 *)ctx)[-2];
+
+	if (!(flags & flag))
+		return -1;      /* The variable is not there */
+	if (flag & (flag - 1))
+		return -1;      /* 2 or more bits are set */
+
+	for (; flags & flag; flags &= flags - 1)
+		off++;
+
+	return off;
+}
+
 BPF_CALL_1(bpf_get_func_ip_tracing, void *, ctx)
 {
 	/* This helper call is inlined by verifier. */
-	return ((u64 *)ctx)[-2];
+	int off = get_trampo_var_off(ctx, BPF_TRAMP_F_IP_ARG);
+
+	if (off < 0)
+		return 0;
+
+	return ((u64 *)ctx)[-off];
 }
 
 static const struct bpf_func_proto bpf_get_func_ip_proto_tracing = {
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 18+ messages in thread
* [PATCH bpf-next 2/5] bpf: Detect if a program needs its program ID.
  2022-01-26 21:48 [PATCH bpf-next 0/5] Attach a cookie to a tracing program Kui-Feng Lee
  2022-01-26 21:48 ` [PATCH bpf-next 1/5] bpf: Add a flags value on trampoline frames Kui-Feng Lee
@ 2022-01-26 21:48 ` Kui-Feng Lee
  2022-01-26 21:48 ` [PATCH bpf-next 3/5] bpf, x86: Store program ID to trampoline frames Kui-Feng Lee
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 18+ messages in thread
From: Kui-Feng Lee @ 2022-01-26 21:48 UTC (permalink / raw)
  To: bpf, ast, daniel, andrii; +Cc: Kui-Feng Lee

Scan BPF bytecode to detect if a program calls any functions requiring
a program ID.  So far, bpf_get_attach_cookie() is the only function
that needs a program ID.

Signed-off-by: Kui-Feng Lee <kuifeng@fb.com>
---
 include/linux/filter.h  | 3 ++-
 kernel/bpf/trampoline.c | 7 ++++---
 kernel/bpf/verifier.c   | 3 +++
 3 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/include/linux/filter.h b/include/linux/filter.h
index d23e999dc032..4433c5d1bc19 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -572,7 +572,8 @@ struct bpf_prog {
 				has_callchain_buf:1, /* callchain buffer allocated? */
 				enforce_expected_attach_type:1, /* Enforce expected_attach_type checking at attach time */
 				call_get_stack:1, /* Do we call bpf_get_stack() or bpf_get_stackid() */
-				call_get_func_ip:1; /* Do we call get_func_ip() */
+				call_get_func_ip:1, /* Do we call get_func_ip() */
+				need_prog_id:1; /* Do we need program ID? */
 	enum bpf_prog_type	type;		/* Type of BPF program */
 	enum bpf_attach_type	expected_attach_type; /* For some prog types */
 	u32			len;		/* Number of filter blocks */
diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
index 4b6974a195c1..c65622e4216c 100644
--- a/kernel/bpf/trampoline.c
+++ b/kernel/bpf/trampoline.c
@@ -181,7 +181,7 @@ static int register_fentry(struct bpf_trampoline *tr, void *new_addr)
 }
 
 static struct bpf_tramp_progs *
-bpf_trampoline_get_progs(const struct bpf_trampoline *tr, int *total, bool *ip_arg)
+bpf_trampoline_get_progs(const struct bpf_trampoline *tr, int *total, bool *ip_arg, bool *prog_id)
 {
 	const struct bpf_prog_aux *aux;
 	struct bpf_tramp_progs *tprogs;
@@ -200,6 +200,7 @@ bpf_trampoline_get_progs(const struct bpf_trampoline *tr, int *total, bool *ip_a
 
 		hlist_for_each_entry(aux, &tr->progs_hlist[kind], tramp_hlist) {
 			*ip_arg |= aux->prog->call_get_func_ip;
+			*prog_id |= aux->prog->need_prog_id;
 			*progs++ = aux->prog;
 		}
 	}
@@ -344,10 +345,10 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr)
 	struct bpf_tramp_image *im;
 	struct bpf_tramp_progs *tprogs;
 	u32 flags = BPF_TRAMP_F_RESTORE_REGS;
-	bool ip_arg = false;
+	bool ip_arg = false, prog_id = false;
 	int err, total;
 
-	tprogs = bpf_trampoline_get_progs(tr, &total, &ip_arg);
+	tprogs = bpf_trampoline_get_progs(tr, &total, &ip_arg, &prog_id);
 	if (IS_ERR(tprogs))
 		return PTR_ERR(tprogs);
 
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index ff91f5038010..0359242e2a81 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -6812,6 +6812,9 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
 		env->prog->call_get_func_ip = true;
 	}
 
+	if (func_id == BPF_FUNC_get_attach_cookie)
+		env->prog->need_prog_id = true;
+
 	if (changes_data)
 		clear_all_pkt_pointers(env);
 	return 0;
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 18+ messages in thread
* [PATCH bpf-next 3/5] bpf, x86: Store program ID to trampoline frames.
  2022-01-26 21:48 [PATCH bpf-next 0/5] Attach a cookie to a tracing program Kui-Feng Lee
  2022-01-26 21:48 ` [PATCH bpf-next 1/5] bpf: Add a flags value on trampoline frames Kui-Feng Lee
  2022-01-26 21:48 ` [PATCH bpf-next 2/5] bpf: Detect if a program needs its program ID Kui-Feng Lee
@ 2022-01-26 21:48 ` Kui-Feng Lee
  2022-01-26 21:48 ` [PATCH bpf-next 4/5] bpf: Attach a cookie to a BPF program Kui-Feng Lee
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 18+ messages in thread
From: Kui-Feng Lee @ 2022-01-26 21:48 UTC (permalink / raw)
  To: bpf, ast, daniel, andrii; +Cc: Kui-Feng Lee

Store the program ID in the trampoline frame before calling a BPF
program if any of BPF programs called by the trampoline need its
program ID, i.e., need_prog_id is true.

Signed-off-by: Kui-Feng Lee <kuifeng@fb.com>
---
 arch/x86/net/bpf_jit_comp.c | 44 +++++++++++++++++++++++++++----------
 include/linux/bpf.h         |  2 ++
 kernel/bpf/trampoline.c     |  3 +++
 3 files changed, 37 insertions(+), 12 deletions(-)

diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index 1c2d3ef57dad..98ed42215887 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -1754,13 +1754,21 @@ 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_prog *p, int stack_size,
+			   int pgid_off, bool save_ret)
 {
 	u8 *prog = *pprog;
 	u8 *jmp_insn;
 
 	/* arg1: mov rdi, progs[i] */
 	emit_mov_imm64(&prog, BPF_REG_1, (long) p >> 32, (u32) (long) p);
+
+	/* Store BPF program ID
+	 * mov QWORD PTR [rbp - pgid_off], rdi
+	 */
+	if (pgid_off > 0)
+		emit_stx(&prog, BPF_DW, BPF_REG_FP, BPF_REG_1, -pgid_off);
+
 	if (emit_call(&prog,
 		      p->aux->sleepable ? __bpf_prog_enter_sleepable :
 		      __bpf_prog_enter, prog))
@@ -1843,14 +1851,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,
-		      bool save_ret)
+		      int pgid_off, 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,
-				    save_ret))
+				    pgid_off, save_ret))
 			return -EINVAL;
 	}
 	*pprog = prog;
@@ -1859,7 +1867,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,
-			      u8 **branches)
+			      int pgid_off, u8 **branches)
 {
 	u8 *prog = *pprog;
 	int i;
@@ -1870,7 +1878,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))
+		if (invoke_bpf_prog(m, &prog, tp->progs[i], stack_size,
+				    pgid_off, true))
 			return -EINVAL;
 
 		/* mod_ret prog stored return value into [rbp - 8]. Emit:
@@ -1976,7 +1985,7 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
 				void *orig_call)
 {
 	int ret, i, nr_args = m->nr_args;
-	int regs_off, flags_off, ip_off, args_off, stack_size = nr_args * 8;
+	int regs_off, flags_off, pgid_off = 0, 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];
@@ -2005,7 +2014,12 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
 	 *
 	 * RBP - args_off  [ args count      ]  always
 	 *
+	 * RBP - flags_off [ flags           ]  BPF_TRAMP_F_IP_ARG or
+	 *                                      BPF_TRAMP_F_PROG_ID flags.
+	 *
 	 * RBP - ip_off    [ traced function ]  BPF_TRAMP_F_IP_ARG flag
+	 *
+	 * RBP - pgid_off  [ program ID      ]  BPF_TRAMP_F_PROG_ID flags.
 	 */
 
 	/* room for return value of orig_call or fentry prog */
@@ -2019,7 +2033,7 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
 	stack_size += 8;
 	args_off = stack_size;
 
-	if (flags & BPF_TRAMP_F_IP_ARG)
+	if (flags & (BPF_TRAMP_F_IP_ARG | BPF_TRAMP_F_PROG_ID))
 		stack_size += 8; /* room for flags */
 
 	flags_off = stack_size;
@@ -2029,6 +2043,12 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
 
 	ip_off = stack_size;
 
+	/* room for program ID */
+	if (flags & BPF_TRAMP_F_PROG_ID) {
+		stack_size += 8;
+		pgid_off = stack_size;
+	}
+
 	if (flags & BPF_TRAMP_F_SKIP_FRAME)
 		/* skip patched call instruction and point orig_call to actual
 		 * body of the kernel function.
@@ -2049,13 +2069,13 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
 	emit_mov_imm64(&prog, BPF_REG_0, 0, (u32) nr_args);
 	emit_stx(&prog, BPF_DW, BPF_REG_FP, BPF_REG_0, -args_off);
 
-	if (flags & BPF_TRAMP_F_IP_ARG) {
+	if (flags & (BPF_TRAMP_F_IP_ARG | BPF_TRAMP_F_PROG_ID)) {
 		/* Store flags
 		 * move rax, flags
 		 * mov QWORD PTR [rbp - flags_off], rax
 		 */
 		emit_mov_imm64(&prog, BPF_REG_0, 0,
-			       (u32) (flags & BPF_TRAMP_F_IP_ARG));
+			       (u32) (flags & (BPF_TRAMP_F_IP_ARG | BPF_TRAMP_F_PROG_ID)));
 		emit_stx(&prog, BPF_DW, BPF_REG_FP, BPF_REG_0, -flags_off);
 	}
 
@@ -2082,7 +2102,7 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
 	}
 
 	if (fentry->nr_progs)
-		if (invoke_bpf(m, &prog, fentry, regs_off,
+		if (invoke_bpf(m, &prog, fentry, regs_off, pgid_off,
 			       flags & BPF_TRAMP_F_RET_FENTRY_RET))
 			return -EINVAL;
 
@@ -2092,7 +2112,7 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
 		if (!branches)
 			return -ENOMEM;
 
-		if (invoke_bpf_mod_ret(m, &prog, fmod_ret, regs_off,
+		if (invoke_bpf_mod_ret(m, &prog, fmod_ret, regs_off, pgid_off,
 				       branches)) {
 			ret = -EINVAL;
 			goto cleanup;
@@ -2130,7 +2150,7 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
 	}
 
 	if (fexit->nr_progs)
-		if (invoke_bpf(m, &prog, fexit, regs_off, false)) {
+		if (invoke_bpf(m, &prog, fexit, regs_off, pgid_off, false)) {
 			ret = -EINVAL;
 			goto cleanup;
 		}
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index e8ec8d2f2fe3..37353745fee5 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -656,6 +656,8 @@ struct btf_func_model {
 #define BPF_TRAMP_F_IP_ARG		BIT(3)
 /* Return the return value of fentry prog. Only used by bpf_struct_ops. */
 #define BPF_TRAMP_F_RET_FENTRY_RET	BIT(4)
+/* Store BPF program ID on the trampoline stack. */
+#define BPF_TRAMP_F_PROG_ID		BIT(5)
 
 /* 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
diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
index c65622e4216c..959a33619b36 100644
--- a/kernel/bpf/trampoline.c
+++ b/kernel/bpf/trampoline.c
@@ -373,6 +373,9 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr)
 	if (ip_arg)
 		flags |= BPF_TRAMP_F_IP_ARG;
 
+	if (prog_id)
+		flags |= BPF_TRAMP_F_PROG_ID;
+
 	err = arch_prepare_bpf_trampoline(im, im->image, im->image + PAGE_SIZE,
 					  &tr->func.model, flags, tprogs,
 					  tr->func.addr);
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 18+ messages in thread
* [PATCH bpf-next 4/5] bpf: Attach a cookie to a BPF program.
  2022-01-26 21:48 [PATCH bpf-next 0/5] Attach a cookie to a tracing program Kui-Feng Lee
                   ` (2 preceding siblings ...)
  2022-01-26 21:48 ` [PATCH bpf-next 3/5] bpf, x86: Store program ID to trampoline frames Kui-Feng Lee
@ 2022-01-26 21:48 ` Kui-Feng Lee
  2022-02-01  6:46   ` Andrii Nakryiko
  2022-01-26 21:48 ` [PATCH bpf-next 5/5] bpf: Implement bpf_get_attach_cookie() for tracing programs Kui-Feng Lee
  2022-01-27  5:17 ` [PATCH bpf-next 0/5] Attach a cookie to a tracing program Alexei Starovoitov
  5 siblings, 1 reply; 18+ messages in thread
From: Kui-Feng Lee @ 2022-01-26 21:48 UTC (permalink / raw)
  To: bpf, ast, daniel, andrii; +Cc: Kui-Feng Lee

Extend bpf() to attach a tracing program with a cookie by adding a
bpf_cookie field to bpf_attr for raw tracepoints.

Signed-off-by: Kui-Feng Lee <kuifeng@fb.com>
---
 include/linux/bpf.h            |  1 +
 include/uapi/linux/bpf.h       |  1 +
 kernel/bpf/syscall.c           | 12 ++++++++----
 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 +
 7 files changed, 27 insertions(+), 4 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 37353745fee5..d5196514e9bd 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1004,6 +1004,7 @@ struct bpf_prog_aux {
 		struct work_struct work;
 		struct rcu_head	rcu;
 	};
+	u64 cookie;
 };
 
 struct bpf_array_aux {
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 16a7574292a5..3fa27346ab4b 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -1425,6 +1425,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 72ce1edde950..79d057918c76 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -2696,7 +2696,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;
@@ -2832,6 +2833,8 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog,
 	if (err)
 		goto out_unlock;
 
+	prog->aux->cookie = bpf_cookie;
+
 	err = bpf_trampoline_link_prog(prog, tr);
 	if (err) {
 		bpf_link_cleanup(&link_primer);
@@ -3017,7 +3020,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)
 {
@@ -3052,7 +3055,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;
@@ -4244,7 +4247,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/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 16a7574292a5..3fa27346ab4b 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -1425,6 +1425,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 418b259166f8..c28b017de515 100644
--- a/tools/lib/bpf/bpf.c
+++ b/tools/lib/bpf/bpf.c
@@ -1131,6 +1131,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 c2e8327010f9..c3d2c6a4cb15 100644
--- a/tools/lib/bpf/bpf.h
+++ b/tools/lib/bpf/bpf.h
@@ -475,6 +475,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 e10f0822845a..05af5bb8de92 100644
--- a/tools/lib/bpf/libbpf.map
+++ b/tools/lib/bpf/libbpf.map
@@ -432,6 +432,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


^ permalink raw reply related	[flat|nested] 18+ messages in thread
* [PATCH bpf-next 5/5] bpf: Implement bpf_get_attach_cookie() for tracing programs.
  2022-01-26 21:48 [PATCH bpf-next 0/5] Attach a cookie to a tracing program Kui-Feng Lee
                   ` (3 preceding siblings ...)
  2022-01-26 21:48 ` [PATCH bpf-next 4/5] bpf: Attach a cookie to a BPF program Kui-Feng Lee
@ 2022-01-26 21:48 ` Kui-Feng Lee
  2022-01-26 23:38     ` kernel test robot
  2022-01-27  5:17 ` [PATCH bpf-next 0/5] Attach a cookie to a tracing program Alexei Starovoitov
  5 siblings, 1 reply; 18+ messages in thread
From: Kui-Feng Lee @ 2022-01-26 21:48 UTC (permalink / raw)
  To: bpf, ast, daniel, andrii; +Cc: Kui-Feng Lee

Implement bpf_get_attach_cookie() for fentry, fexit, and fmod_ret
tracing programs.  The cookie is from the bpf_prog from the program ID
of the current BPF program.

Signed-off-by: Kui-Feng Lee <kuifeng@fb.com>
---
 kernel/trace/bpf_trace.c                      | 22 +++++++
 .../selftests/bpf/prog_tests/bpf_cookie.c     | 57 +++++++++++++++++++
 .../selftests/bpf/progs/test_bpf_cookie.c     | 24 ++++++++
 3 files changed, 103 insertions(+)

diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index bf2c9d11ad05..7b1f4c8a10de 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -1084,6 +1084,26 @@ 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)
+{
+	const struct bpf_prog *prog;
+	int off = get_trampo_var_off(ctx, BPF_TRAMP_F_PROG_ID);
+
+	if (off < 0)
+		return 0;
+
+	prog = (const struct bpf_prog *)((u64 *)ctx)[-off];
+
+	return prog->aux->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
@@ -1706,6 +1726,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/testing/selftests/bpf/prog_tests/bpf_cookie.c b/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c
index 5eea3c3a40fe..41a73c0a3a8e 100644
--- a/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c
+++ b/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c
@@ -231,6 +231,61 @@ static void pe_subtest(struct test_bpf_cookie *skel)
 	bpf_link__destroy(link);
 }
 
+static void tracing_subtest(struct test_bpf_cookie *skel)
+{
+	__u64 cookie;
+	__u32 duration = 0, retval;
+	int prog_fd;
+	int fentry_fd = -1, fexit_fd = -1, fmod_ret_fd = -1;
+
+	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;
+
+	prog_fd = bpf_program__fd(skel->progs.fentry_test1);
+	bpf_prog_test_run(prog_fd, 1, NULL, 0,
+			  NULL, NULL, &retval, &duration);
+
+	prog_fd = bpf_program__fd(skel->progs.fmod_ret_test);
+	bpf_prog_test_run(prog_fd, 1, NULL, 0,
+			  NULL, NULL, &retval, &duration);
+
+	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;
@@ -249,6 +304,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


^ permalink raw reply related	[flat|nested] 18+ messages in thread
* Re: [PATCH bpf-next 5/5] bpf: Implement bpf_get_attach_cookie() for tracing programs.
  2022-01-26 21:48 ` [PATCH bpf-next 5/5] bpf: Implement bpf_get_attach_cookie() for tracing programs Kui-Feng Lee
@ 2022-01-26 23:38     ` kernel test robot
  0 siblings, 0 replies; 18+ messages in thread
From: kernel test robot @ 2022-01-26 23:38 UTC (permalink / raw)
  To: Kui-Feng Lee, bpf, ast, daniel, andrii; +Cc: kbuild-all, Kui-Feng Lee

Hi Kui-Feng,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on bpf-next/master]

url:    https://github.com/0day-ci/linux/commits/Kui-Feng-Lee/Attach-a-cookie-to-a-tracing-program/20220127-054929
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
config: mips-allmodconfig (https://download.01.org/0day-ci/archive/20220127/202201270704.kcMiWFUK-lkp@intel.com/config)
compiler: mips-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/6dffffc1fe68f386715b4ee396fb6a5c738fb065
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Kui-Feng-Lee/Attach-a-cookie-to-a-tracing-program/20220127-054929
        git checkout 6dffffc1fe68f386715b4ee396fb6a5c738fb065
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=mips SHELL=/bin/bash kernel/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   kernel/trace/bpf_trace.c: In function '____bpf_get_attach_cookie_tracing':
>> kernel/trace/bpf_trace.c:1095:16: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
    1095 |         prog = (const struct bpf_prog *)((u64 *)ctx)[-off];
         |                ^


vim +1095 kernel/trace/bpf_trace.c

  1086	
  1087	BPF_CALL_1(bpf_get_attach_cookie_tracing, void *, ctx)
  1088	{
  1089		const struct bpf_prog *prog;
  1090		int off = get_trampo_var_off(ctx, BPF_TRAMP_F_PROG_ID);
  1091	
  1092		if (off < 0)
  1093			return 0;
  1094	
> 1095		prog = (const struct bpf_prog *)((u64 *)ctx)[-off];
  1096	
  1097		return prog->aux->cookie;
  1098	}
  1099	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

^ permalink raw reply	[flat|nested] 18+ messages in thread
* Re: [PATCH bpf-next 5/5] bpf: Implement bpf_get_attach_cookie() for tracing programs.
@ 2022-01-26 23:38     ` kernel test robot
  0 siblings, 0 replies; 18+ messages in thread
From: kernel test robot @ 2022-01-26 23:38 UTC (permalink / raw)
  To: kbuild-all

[-- Attachment #1: Type: text/plain, Size: 2171 bytes --]

Hi Kui-Feng,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on bpf-next/master]

url:    https://github.com/0day-ci/linux/commits/Kui-Feng-Lee/Attach-a-cookie-to-a-tracing-program/20220127-054929
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
config: mips-allmodconfig (https://download.01.org/0day-ci/archive/20220127/202201270704.kcMiWFUK-lkp(a)intel.com/config)
compiler: mips-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/6dffffc1fe68f386715b4ee396fb6a5c738fb065
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Kui-Feng-Lee/Attach-a-cookie-to-a-tracing-program/20220127-054929
        git checkout 6dffffc1fe68f386715b4ee396fb6a5c738fb065
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=mips SHELL=/bin/bash kernel/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   kernel/trace/bpf_trace.c: In function '____bpf_get_attach_cookie_tracing':
>> kernel/trace/bpf_trace.c:1095:16: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
    1095 |         prog = (const struct bpf_prog *)((u64 *)ctx)[-off];
         |                ^


vim +1095 kernel/trace/bpf_trace.c

  1086	
  1087	BPF_CALL_1(bpf_get_attach_cookie_tracing, void *, ctx)
  1088	{
  1089		const struct bpf_prog *prog;
  1090		int off = get_trampo_var_off(ctx, BPF_TRAMP_F_PROG_ID);
  1091	
  1092		if (off < 0)
  1093			return 0;
  1094	
> 1095		prog = (const struct bpf_prog *)((u64 *)ctx)[-off];
  1096	
  1097		return prog->aux->cookie;
  1098	}
  1099	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

^ permalink raw reply	[flat|nested] 18+ messages in thread
* Re: [PATCH bpf-next 0/5] Attach a cookie to a tracing program.
  2022-01-26 21:48 [PATCH bpf-next 0/5] Attach a cookie to a tracing program Kui-Feng Lee
                   ` (4 preceding siblings ...)
  2022-01-26 21:48 ` [PATCH bpf-next 5/5] bpf: Implement bpf_get_attach_cookie() for tracing programs Kui-Feng Lee
@ 2022-01-27  5:17 ` Alexei Starovoitov
  2022-01-31 16:56   ` Jiri Olsa
  2022-02-01  6:45   ` Andrii Nakryiko
  5 siblings, 2 replies; 18+ messages in thread
From: Alexei Starovoitov @ 2022-01-27  5:17 UTC (permalink / raw)
  To: Kui-Feng Lee; +Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko

On Wed, Jan 26, 2022 at 1:48 PM Kui-Feng Lee <kuifeng@fb.com> wrote:
>
> Allow users to attach a 64-bits cookie to a BPF program when link it
> to fentry, fexit, or fmod_ret of a function.
>
> This changeset includes several major changes.
>
>  - Add a new field bpf_cookie to struct raw_tracepoint, so that a user
>    can attach a cookie to a program.
>
>  - Store flags in trampoline frames to provide the flexibility of
>    storing more values in these frames.
>
>  - Store the program ID of the current BPF program in the trampoline
>    frame.
>
>  - The implmentation of bpf_get_attach_cookie() for tracing programs
>    to read the attached cookie.

flags, prog_id, cookie... I don't follow what's going on here.

cookie is supposed to be per link.
Doing it for fentry only will be inconvenient for users.
For existing kprobes there is no good place to store it. iirc.
For multi attach kprobes there won't be a good place either.
I think cookie should be out of band.
Maybe lets try a resizable map[ip]->cookie and don't add
'cookie' arrays to multi-kprobe attach,
'cookie' field to kprobe, fentry, and other attach apis.
Adding 'cookie' to all of them is quite a bit of churn for kernel
and user space.
I think resizable bpf map[u64]->u64 solves this problem.
Maybe cookie isn't even needed.
If the bpf prog can have a clean map[bpf_get_func_ip()] that
doesn't have to be sized up front it will address the need.

^ permalink raw reply	[flat|nested] 18+ messages in thread
* Re: [PATCH bpf-next 0/5] Attach a cookie to a tracing program.
  2022-01-27  5:17 ` [PATCH bpf-next 0/5] Attach a cookie to a tracing program Alexei Starovoitov
@ 2022-01-31 16:56   ` Jiri Olsa
  2022-02-01  6:45   ` Andrii Nakryiko
  1 sibling, 0 replies; 18+ messages in thread
From: Jiri Olsa @ 2022-01-31 16:56 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: Kui-Feng Lee, bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko

On Wed, Jan 26, 2022 at 09:17:07PM -0800, Alexei Starovoitov wrote:
> On Wed, Jan 26, 2022 at 1:48 PM Kui-Feng Lee <kuifeng@fb.com> wrote:
> >
> > Allow users to attach a 64-bits cookie to a BPF program when link it
> > to fentry, fexit, or fmod_ret of a function.
> >
> > This changeset includes several major changes.
> >
> >  - Add a new field bpf_cookie to struct raw_tracepoint, so that a user
> >    can attach a cookie to a program.
> >
> >  - Store flags in trampoline frames to provide the flexibility of
> >    storing more values in these frames.
> >
> >  - Store the program ID of the current BPF program in the trampoline
> >    frame.
> >
> >  - The implmentation of bpf_get_attach_cookie() for tracing programs
> >    to read the attached cookie.
> 
> flags, prog_id, cookie... I don't follow what's going on here.
> 
> cookie is supposed to be per link.
> Doing it for fentry only will be inconvenient for users.
> For existing kprobes there is no good place to store it. iirc.
> For multi attach kprobes there won't be a good place either.
> I think cookie should be out of band.
> Maybe lets try a resizable map[ip]->cookie and don't add
> 'cookie' arrays to multi-kprobe attach,
> 'cookie' field to kprobe, fentry, and other attach apis.
> Adding 'cookie' to all of them is quite a bit of churn for kernel
> and user space.
> I think resizable bpf map[u64]->u64 solves this problem.

so you mean passing such map fd to link and have bpf_get_attach_cookie
using that map to get the cookie? that could be generic way for all
the links

I have the 'cookie arrays to multi-kprobe attach' code ready and I think 
it should be faster than map[ip] hash lookup? I'll need to check

jirka

> Maybe cookie isn't even needed.
> If the bpf prog can have a clean map[bpf_get_func_ip()] that
> doesn't have to be sized up front it will address the need.
> 


^ permalink raw reply	[flat|nested] 18+ messages in thread
* Re: [PATCH bpf-next 0/5] Attach a cookie to a tracing program.
  2022-01-27  5:17 ` [PATCH bpf-next 0/5] Attach a cookie to a tracing program Alexei Starovoitov
  2022-01-31 16:56   ` Jiri Olsa
@ 2022-02-01  6:45   ` Andrii Nakryiko
  2022-02-01 17:37     ` Kui-Feng Lee
  2022-02-01 19:32     ` Alexei Starovoitov
  1 sibling, 2 replies; 18+ messages in thread
From: Andrii Nakryiko @ 2022-02-01  6:45 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: Kui-Feng Lee, bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko

On Wed, Jan 26, 2022 at 9:17 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Wed, Jan 26, 2022 at 1:48 PM Kui-Feng Lee <kuifeng@fb.com> wrote:
> >
> > Allow users to attach a 64-bits cookie to a BPF program when link it
> > to fentry, fexit, or fmod_ret of a function.
> >
> > This changeset includes several major changes.
> >
> >  - Add a new field bpf_cookie to struct raw_tracepoint, so that a user
> >    can attach a cookie to a program.
> >
> >  - Store flags in trampoline frames to provide the flexibility of
> >    storing more values in these frames.
> >
> >  - Store the program ID of the current BPF program in the trampoline
> >    frame.
> >
> >  - The implmentation of bpf_get_attach_cookie() for tracing programs
> >    to read the attached cookie.
>
> flags, prog_id, cookie... I don't follow what's going on here.
>
> cookie is supposed to be per link.
> Doing it for fentry only will be inconvenient for users.
> For existing kprobes there is no good place to store it. iirc.

Hm... are you talking about current kprobes? We already have
bpf_cookie support for them. We store it in the associated perf_event,
it's actually pretty convenient.

> For multi attach kprobes there won't be a good place either.

As Jiri mentioned, for multi-attach kprobes the idea was to keep a
sorted array of ips and associated cookies to do log(N) search in
bpf_get_attach_cookie() helper.

For multi-attach fentry, we could use the same approach if we let
either bpf_link or bpf_prog available to fentry/fexit program at
runtime. Kui-Feng seems to be storing prog_id in BPF trampoline
generated code, I don't think that's the best approach, it would be
probably better to store bpf_link or bpf_prog pointer, whichever is
more convenient. I think we can't attach the same BPF program twice to
the same BPF trampoline, so storing this array of ip -> cookie
mappings in bpf_prog would work (because the mapping is unique), but
might be a more cumbersome. Storing bpf_link is conceptually better,
probably, but I haven't thought through if there are any problems if
we support updating bpf_link's underlying program.

But keep in mind, this patch set is basically an RFC to arrive at the
best approach that will also be compatible with multi-attach
fentry/fexit. Though it seems like we'll first need to establish the
need for bpf_cookie (I thought that was not controversial, my bad),
which is fine, see below.

> I think cookie should be out of band.
> Maybe lets try a resizable map[ip]->cookie and don't add
> 'cookie' arrays to multi-kprobe attach,
> 'cookie' field to kprobe, fentry, and other attach apis.
> Adding 'cookie' to all of them is quite a bit of churn for kernel
> and user space.

We don't need all BPF program types, but anything that's useful for
generic tracing is much more powerful with cookies. We have them for
kprobe, uprobe and perf_event programs already. For multi-attach
kprobe/kretprobe and fentry/fexit they are essentially a must to let
users use those BPF program types to their fullest.

> I think resizable bpf map[u64]->u64 solves this problem.
> Maybe cookie isn't even needed.
> If the bpf prog can have a clean map[bpf_get_func_ip()] that
> doesn't have to be sized up front it will address the need.

You mean for users to maintain their own BPF map and pre-populated it
before attaching their BPF programs? Or did I misunderstand?

Sizing such a BPF map is just one problem.

Figuring out the correct IP to use as a key is another and arguably
bigger hassle. Everyone will be forced to consult kallsyms, even if
their use case is simple and they don't need to parse kallsyms at all.
Normally, for fentry/fexit programs, users don't even need kallsyms,
as vmlinux BTF is used to find BTF ID and that's enough to load and
attach fentry/fexit.

And while for kprobe/fentry programs it's inconvenient but can be done
with a bunch of extra work, for uprobes there are situations where
it's impossible to know IP at all. One very specific example is USDTs
in shared library. If user needs to attach to USDT defined in a shared
library across *any* PID (current and future), it's impossible to do
without BPF cookie support, because each different process will load
shared library at unknown base address, so it's impossible to
calculate upfront any absolute IP to look up by. This is the reason
BCC allows to attach to USDTs in shared library only for one specific
process (i.e., PID has to be specified in such a case).


Or did you propose to maintain such IP -> cookie mapping inside the
kernel during bpf_link creation? I think the key would have to be at
least a (link ID, IP) pair, no? The question is whether it's going to
be more convenient to get link ID and IP for all supported BPF
programs at runtime and whether it will cause the same or more amount
of churn?

^ permalink raw reply	[flat|nested] 18+ messages in thread
* Re: [PATCH bpf-next 4/5] bpf: Attach a cookie to a BPF program.
  2022-01-26 21:48 ` [PATCH bpf-next 4/5] bpf: Attach a cookie to a BPF program Kui-Feng Lee
@ 2022-02-01  6:46   ` Andrii Nakryiko
  2022-02-01 20:17     ` Alexei Starovoitov
  0 siblings, 1 reply; 18+ messages in thread
From: Andrii Nakryiko @ 2022-02-01  6:46 UTC (permalink / raw)
  To: Kui-Feng Lee; +Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko

On Wed, Jan 26, 2022 at 1:48 PM Kui-Feng Lee <kuifeng@fb.com> wrote:
>
> Extend bpf() to attach a tracing program with a cookie by adding a
> bpf_cookie field to bpf_attr for raw tracepoints.
>
> Signed-off-by: Kui-Feng Lee <kuifeng@fb.com>
> ---
>  include/linux/bpf.h            |  1 +
>  include/uapi/linux/bpf.h       |  1 +
>  kernel/bpf/syscall.c           | 12 ++++++++----
>  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 +
>  7 files changed, 27 insertions(+), 4 deletions(-)
>

We normally split out kernel, libbpf, samples/bpf, selftests/bpf,
bpftool, etc changes into separate patches, if possible. Please do
that in the next revision.

> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 37353745fee5..d5196514e9bd 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -1004,6 +1004,7 @@ struct bpf_prog_aux {
>                 struct work_struct work;
>                 struct rcu_head rcu;
>         };
> +       u64 cookie;
>  };
>
>  struct bpf_array_aux {
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 16a7574292a5..3fa27346ab4b 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -1425,6 +1425,7 @@ union bpf_attr {
>         struct { /* anonymous struct used by BPF_RAW_TRACEPOINT_OPEN command */
>                 __u64 name;
>                 __u32 prog_fd;
> +               __u64 bpf_cookie;
>         } raw_tracepoint;
>

As an aside, Alexei, should we bite a bullet and allow attaching
raw_tp, fentry/fexit, and other tracing prog attachment through the
LINK_CREATE command? BPF_RAW_TRACEPOINT_OPEN makes little sense for
anything but raw_tp programs. Libbpf can do feature detection and
route between RAW_TRACEPOINT_OPEN and LINK_CREATE commands depending
on whether bpf_cookie and whatever other newer things we add, so that
it's compatible with old kernels. As we also add multi-attach
fentry/fexit, it would be nice to keep everything in LINK_CREATE
section of bpf_attr, similar to multi-attach kprobe case. WDYT?


>         struct { /* anonymous struct for BPF_BTF_LOAD */
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 72ce1edde950..79d057918c76 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -2696,7 +2696,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;
> @@ -2832,6 +2833,8 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog,
>         if (err)
>                 goto out_unlock;
>
> +       prog->aux->cookie = bpf_cookie;
> +
>         err = bpf_trampoline_link_prog(prog, tr);
>         if (err) {
>                 bpf_link_cleanup(&link_primer);
> @@ -3017,7 +3020,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)
>  {
> @@ -3052,7 +3055,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;
> @@ -4244,7 +4247,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/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
> index 16a7574292a5..3fa27346ab4b 100644
> --- a/tools/include/uapi/linux/bpf.h
> +++ b/tools/include/uapi/linux/bpf.h
> @@ -1425,6 +1425,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 418b259166f8..c28b017de515 100644
> --- a/tools/lib/bpf/bpf.c
> +++ b/tools/lib/bpf/bpf.c
> @@ -1131,6 +1131,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 c2e8327010f9..c3d2c6a4cb15 100644
> --- a/tools/lib/bpf/bpf.h
> +++ b/tools/lib/bpf/bpf.h
> @@ -475,6 +475,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 e10f0822845a..05af5bb8de92 100644
> --- a/tools/lib/bpf/libbpf.map
> +++ b/tools/lib/bpf/libbpf.map
> @@ -432,6 +432,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
>

^ permalink raw reply	[flat|nested] 18+ messages in thread
* Re: [PATCH bpf-next 0/5] Attach a cookie to a tracing program.
  2022-02-01  6:45   ` Andrii Nakryiko
@ 2022-02-01 17:37     ` Kui-Feng Lee
  2022-02-02  1:06       ` Andrii Nakryiko
  2022-02-01 19:32     ` Alexei Starovoitov
  1 sibling, 1 reply; 18+ messages in thread
From: Kui-Feng Lee @ 2022-02-01 17:37 UTC (permalink / raw)
  To: andrii.nakryiko, alexei.starovoitov; +Cc: daniel, ast, andrii, bpf

On Mon, 2022-01-31 at 22:45 -0800, Andrii Nakryiko wrote:
...... cut ......
> For multi-attach fentry, we could use the same approach if we let
> either bpf_link or bpf_prog available to fentry/fexit program at
> runtime. Kui-Feng seems to be storing prog_id in BPF trampoline
> generated code, I don't think that's the best approach, it would be
> probably better to store bpf_link or bpf_prog pointer, whichever is
> more convenient. I think we can't attach the same BPF program twice
> to

FYI, the prog_id used is casting from a bpf_prog pointer.  It gets a
bpf_prog by casting a prog_id back.


^ permalink raw reply	[flat|nested] 18+ messages in thread
* Re: [PATCH bpf-next 0/5] Attach a cookie to a tracing program.
  2022-02-01  6:45   ` Andrii Nakryiko
  2022-02-01 17:37     ` Kui-Feng Lee
@ 2022-02-01 19:32     ` Alexei Starovoitov
  2022-02-02  1:15       ` Andrii Nakryiko
  1 sibling, 1 reply; 18+ messages in thread
From: Alexei Starovoitov @ 2022-02-01 19:32 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: Kui-Feng Lee, bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko

On Mon, Jan 31, 2022 at 10:45:53PM -0800, Andrii Nakryiko wrote:
> 
> As Jiri mentioned, for multi-attach kprobes the idea was to keep a
> sorted array of ips and associated cookies to do log(N) search in
> bpf_get_attach_cookie() helper.
> 
> For multi-attach fentry, we could use the same approach if we let
> either bpf_link or bpf_prog available to fentry/fexit program at
> runtime.

Makes sense to me.
It's probably better to land multi-attach kprobe and fentry first,
so we don't need to refactor trampolines once again.
iirc the trampolines were not easy to refactor for Jiri.
I'm afraid that adding prog_id or a pointer to the trampoline
will complicate things even more for multi attach.

It's easy to store hard coded bpf_tramp_image pointer in the generated
trampoline. Storing prog_id or bpf_prog pointer there is a bit
harder, since the [sp-X] store needs to happen right in there invoke_bpf_prog()
(since there can be multiple progs per trampoline).

From there bpf_get_attach_cookie() can either do binary search
in the ip->cookie array or single load in case of non-multi attach.

Anyway the cookie support in trampoline seems to be easier to design
when there is a clarity on multi-attach fentry.

I would probably add support for cookie in raw_tp/tp_btf first,
since that part is not going to be affected by multi-* work.

> We don't need all BPF program types, but anything that's useful for
> generic tracing is much more powerful with cookies. We have them for
> kprobe, uprobe and perf_event programs already. For multi-attach
> kprobe/kretprobe and fentry/fexit they are essentially a must to let
> users use those BPF program types to their fullest.

agree. I missed the part that cookie is already supported with kuprobes
when attach is done via bpf_link_create.

^ permalink raw reply	[flat|nested] 18+ messages in thread
* Re: [PATCH bpf-next 4/5] bpf: Attach a cookie to a BPF program.
  2022-02-01  6:46   ` Andrii Nakryiko
@ 2022-02-01 20:17     ` Alexei Starovoitov
  2022-02-02  1:24       ` Andrii Nakryiko
  0 siblings, 1 reply; 18+ messages in thread
From: Alexei Starovoitov @ 2022-02-01 20:17 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: Kui-Feng Lee, bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko

On Mon, Jan 31, 2022 at 10:47 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
> >  struct bpf_array_aux {
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index 16a7574292a5..3fa27346ab4b 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -1425,6 +1425,7 @@ union bpf_attr {
> >         struct { /* anonymous struct used by BPF_RAW_TRACEPOINT_OPEN command */
> >                 __u64 name;
> >                 __u32 prog_fd;
> > +               __u64 bpf_cookie;
> >         } raw_tracepoint;
> >
>
> As an aside, Alexei, should we bite a bullet and allow attaching
> raw_tp, fentry/fexit, and other tracing prog attachment through the
> LINK_CREATE command? BPF_RAW_TRACEPOINT_OPEN makes little sense for
> anything but raw_tp programs.

raw_tp_open is used for raw_tp, tp_btf, lsm, fentry.
iirc it's creating a normal bpf_link underneath.
link_create doesn't exist for raw_tp and friends,
so this is the best place to add a cookie.
We can add an alias cmd (instead of raw_tp_open)
to make it a bit cleaner from uapi naming pov.
We can allow link_create to do the attach in all those cases as well,
but it's a different discussion.
link_create.perf_event.bpf_cookie isn't the best name.
That name was a cause of confusion for me.
I thought it applies to perf_event only,
but it's for kuprobe too.
So plenty of bikeshedding to do if we decide to do
link_create for raw_tp. Hence, for now, I'd add a cookie to
raw_tp/tp_btf/lsm/fentry like this patch is doing.

^ permalink raw reply	[flat|nested] 18+ messages in thread
* Re: [PATCH bpf-next 0/5] Attach a cookie to a tracing program.
  2022-02-01 17:37     ` Kui-Feng Lee
@ 2022-02-02  1:06       ` Andrii Nakryiko
  0 siblings, 0 replies; 18+ messages in thread
From: Andrii Nakryiko @ 2022-02-02  1:06 UTC (permalink / raw)
  To: Kui-Feng Lee; +Cc: alexei.starovoitov, daniel, ast, andrii, bpf

On Tue, Feb 1, 2022 at 9:37 AM Kui-Feng Lee <kuifeng@fb.com> wrote:
>
> On Mon, 2022-01-31 at 22:45 -0800, Andrii Nakryiko wrote:
> ...... cut ......
> > For multi-attach fentry, we could use the same approach if we let
> > either bpf_link or bpf_prog available to fentry/fexit program at
> > runtime. Kui-Feng seems to be storing prog_id in BPF trampoline
> > generated code, I don't think that's the best approach, it would be
> > probably better to store bpf_link or bpf_prog pointer, whichever is
> > more convenient. I think we can't attach the same BPF program twice
> > to
>
> FYI, the prog_id used is casting from a bpf_prog pointer.  It gets a
> bpf_prog by casting a prog_id back.
>

Ok, this is why we all got confused. There is already a concept of
"prog ID" (it is also visible to user-space, btw), but you were
actually storing an actual `struct bpf_prog *` pointer. So just a
misleading use of the term in this context.

^ permalink raw reply	[flat|nested] 18+ messages in thread
* Re: [PATCH bpf-next 0/5] Attach a cookie to a tracing program.
  2022-02-01 19:32     ` Alexei Starovoitov
@ 2022-02-02  1:15       ` Andrii Nakryiko
  0 siblings, 0 replies; 18+ messages in thread
From: Andrii Nakryiko @ 2022-02-02  1:15 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: Kui-Feng Lee, bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko

On Tue, Feb 1, 2022 at 11:32 AM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Mon, Jan 31, 2022 at 10:45:53PM -0800, Andrii Nakryiko wrote:
> >
> > As Jiri mentioned, for multi-attach kprobes the idea was to keep a
> > sorted array of ips and associated cookies to do log(N) search in
> > bpf_get_attach_cookie() helper.
> >
> > For multi-attach fentry, we could use the same approach if we let
> > either bpf_link or bpf_prog available to fentry/fexit program at
> > runtime.
>
> Makes sense to me.
> It's probably better to land multi-attach kprobe and fentry first,
> so we don't need to refactor trampolines once again.
> iirc the trampolines were not easy to refactor for Jiri.
> I'm afraid that adding prog_id or a pointer to the trampoline
> will complicate things even more for multi attach.

Yep, sure, makes sense to me.

>
> It's easy to store hard coded bpf_tramp_image pointer in the generated
> trampoline. Storing prog_id or bpf_prog pointer there is a bit
> harder, since the [sp-X] store needs to happen right in there invoke_bpf_prog()
> (since there can be multiple progs per trampoline).
>
> From there bpf_get_attach_cookie() can either do binary search
> in the ip->cookie array or single load in case of non-multi attach.
>
> Anyway the cookie support in trampoline seems to be easier to design
> when there is a clarity on multi-attach fentry.
>

True. bpf_tramp_image pointer probably won't work for multi-attach
fentry because one bpf_tramp_image will be shared by multiple
attachments (bpf_links), right?

Ideally we should provide a way to lookup "current bpf_link" at
runtime from BPF helpers (given fentry ctx) and do that binary search
there. There could be few other options (e.g., bpf_tramp_image +
binary search based on prog_id or bpf_prog pointer), but it's a bit
more cumbersome. It still feels like something per-program would need
to come from the context to be able to distinguish between multiple
attachments. But as you said, more clarity on multi-attach fentry
first would be best.

> I would probably add support for cookie in raw_tp/tp_btf first,
> since that part is not going to be affected by multi-* work.

Yep, let's try that first.

>
> > We don't need all BPF program types, but anything that's useful for
> > generic tracing is much more powerful with cookies. We have them for
> > kprobe, uprobe and perf_event programs already. For multi-attach
> > kprobe/kretprobe and fentry/fexit they are essentially a must to let
> > users use those BPF program types to their fullest.
>
> agree. I missed the part that cookie is already supported with kuprobes
> when attach is done via bpf_link_create.

^ permalink raw reply	[flat|nested] 18+ messages in thread
* Re: [PATCH bpf-next 4/5] bpf: Attach a cookie to a BPF program.
  2022-02-01 20:17     ` Alexei Starovoitov
@ 2022-02-02  1:24       ` Andrii Nakryiko
  0 siblings, 0 replies; 18+ messages in thread
From: Andrii Nakryiko @ 2022-02-02  1:24 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: Kui-Feng Lee, bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko

On Tue, Feb 1, 2022 at 12:17 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Mon, Jan 31, 2022 at 10:47 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> > >  struct bpf_array_aux {
> > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > > index 16a7574292a5..3fa27346ab4b 100644
> > > --- a/include/uapi/linux/bpf.h
> > > +++ b/include/uapi/linux/bpf.h
> > > @@ -1425,6 +1425,7 @@ union bpf_attr {
> > >         struct { /* anonymous struct used by BPF_RAW_TRACEPOINT_OPEN command */
> > >                 __u64 name;
> > >                 __u32 prog_fd;
> > > +               __u64 bpf_cookie;
> > >         } raw_tracepoint;
> > >
> >
> > As an aside, Alexei, should we bite a bullet and allow attaching
> > raw_tp, fentry/fexit, and other tracing prog attachment through the
> > LINK_CREATE command? BPF_RAW_TRACEPOINT_OPEN makes little sense for
> > anything but raw_tp programs.
>
> raw_tp_open is used for raw_tp, tp_btf, lsm, fentry.
> iirc it's creating a normal bpf_link underneath.
> link_create doesn't exist for raw_tp and friends,
> so this is the best place to add a cookie.
> We can add an alias cmd (instead of raw_tp_open)
> to make it a bit cleaner from uapi naming pov.
> We can allow link_create to do the attach in all those cases as well,
> but it's a different discussion.

I was actually proposing exactly the latter: to allow LINK_CREATE to
create all the programs that RAW_TRACEPOINT_OPEN allows. It's already
confusing because bpf_iter programs are created using LINK_CREATE
(realized that when I saw your recent patches). Also extension
programs are attached through LINK_CREATE. So while we can't get rid
of BPF_RAW_TRACEPOINT_OPEN, I hoped we can add lsm and fentry support
as well (I don't mind raw_tp/tp_btf there as well for completeness),
so at least in the future it would be we all just a universal
LINK_CREATE command.

> link_create.perf_event.bpf_cookie isn't the best name.
> That name was a cause of confusion for me.
> I thought it applies to perf_event only,
> but it's for kuprobe too.

Yeah, not great, but given it was "attach to perf_event FD" command,
it seemed like the most accurate name at the time :) I still don't
know what I'd call it today, apart from having separate (and
duplicate) link_create.kprobe.bpf_cookie,
link_create.uprobe.bpf_cookie, etc. At least libbpf is hiding it
behind bpf_program__attach_kprobe and bpf_program__attach_uprobe,
though.


> So plenty of bikeshedding to do if we decide to do
> link_create for raw_tp. Hence, for now, I'd add a cookie to
> raw_tp/tp_btf/lsm/fentry like this patch is doing.

Sure, that's fine, it was a long shot anyway. But I'd like to get back
to this discussion when we are going to multi-attach fentry/fexit :)

^ permalink raw reply	[flat|nested] 18+ messages in thread
end of thread, other threads:[~2022-02-02  1:24 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-26 21:48 [PATCH bpf-next 0/5] Attach a cookie to a tracing program Kui-Feng Lee
2022-01-26 21:48 ` [PATCH bpf-next 1/5] bpf: Add a flags value on trampoline frames Kui-Feng Lee
2022-01-26 21:48 ` [PATCH bpf-next 2/5] bpf: Detect if a program needs its program ID Kui-Feng Lee
2022-01-26 21:48 ` [PATCH bpf-next 3/5] bpf, x86: Store program ID to trampoline frames Kui-Feng Lee
2022-01-26 21:48 ` [PATCH bpf-next 4/5] bpf: Attach a cookie to a BPF program Kui-Feng Lee
2022-02-01  6:46   ` Andrii Nakryiko
2022-02-01 20:17     ` Alexei Starovoitov
2022-02-02  1:24       ` Andrii Nakryiko
2022-01-26 21:48 ` [PATCH bpf-next 5/5] bpf: Implement bpf_get_attach_cookie() for tracing programs Kui-Feng Lee
2022-01-26 23:38   ` kernel test robot
2022-01-26 23:38     ` kernel test robot
2022-01-27  5:17 ` [PATCH bpf-next 0/5] Attach a cookie to a tracing program Alexei Starovoitov
2022-01-31 16:56   ` Jiri Olsa
2022-02-01  6:45   ` Andrii Nakryiko
2022-02-01 17:37     ` Kui-Feng Lee
2022-02-02  1:06       ` Andrii Nakryiko
2022-02-01 19:32     ` Alexei Starovoitov
2022-02-02  1:15       ` Andrii Nakryiko

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.