[PATCH RESEND bpf-next v3 0/9] bpf: Support multi-attach for freplace programs

STALE2147d

20 messages, 2 authors, 2020-09-15 · open the first message on its own page

[PATCH RESEND bpf-next v3 0/9] bpf: Support multi-attach for freplace programs

From: Toke Høiland-Jørgensen <hidden>
Date: 2020-09-11 09:59:44

This series adds support attaching freplace BPF programs to multiple targets.
This is needed to support incremental attachment of multiple XDP programs using
the libxdp dispatcher model.

The first three patches are refactoring patches: The first one is a trivial
change to the logging in the verifier, split out to make the subsequent refactor
easier to read. Patch 2 refactors check_attach_btf_id() so that the checks on
program and target compatibility can be reused when attaching to a secondary
location.

Patch 3 changes prog_aux->linked_prog to be an embedded bpf_tracing_link that is
initialised at program load time. This nicely encapsulates both the trampoline
and the prog reference, and moves the release of these references into bpf_link
teardown. At raw_tracepoint_open() time (i.e., when the link is attached), it
will be removed from the extension prog, and primed as a regular bpf_link.

Based on these refactorings, it becomes pretty straight-forward to support
multiple-attach for freplace programs (patch 4). This is simply a matter of
creating a second bpf_tracing_link if a target is supplied to
raw_tracepoint_open().

Patch 5 is a port of Jiri Olsa's patch to support fentry/fexit on freplace
programs. His approach of getting the target type from the target program
reference no longer works after we've gotten rid of linked_prog (because the
bpf_tracing_link reference disappears on attach). Instead, we used the saved
reference to the target prog type that is also used to verify compatibility on
secondary freplace attachment.

Patches 6-7 are tools and libbpf updates, and patches 8-9 are selftests, the
first one for the multi-freplace functionality itself, and the second one is
Jiri's previous selftest for the fentry-to-freplace fix.

With this series, libxdp and xdp-tools can successfully attach multiple programs
one at a time. To play with this, use the 'freplace-multi-attach' branch of
xdp-tools:

$ git clone --recurse-submodules --branch freplace-multi-attach https://github.com/xdp-project/xdp-tools
$ cd xdp-tools
$ make
$ sudo ./xdp-loader/xdp-loader load veth0 lib/testing/xdp_drop.o
$ sudo ./xdp-loader/xdp-loader load veth0 lib/testing/xdp_pass.o
$ sudo ./xdp-loader/xdp-loader status

The series is also available here:
https://git.kernel.org/pub/scm/linux/kernel/git/toke/linux.git/log/?h=bpf-freplace-multi-attach-alt-03

Changelog:

v3:
  - Get rid of prog_aux->linked_prog entirely in favour of a bpf_tracing_link
  - Incorporate Jiri's fix for attaching fentry to freplace programs

v2:
  - Drop the log arguments from bpf_raw_tracepoint_open
  - Fix kbot errors
  - Rebase to latest bpf-next

---

Jiri Olsa (1):
      selftests/bpf: Adding test for arg dereference in extension trace

Toke Høiland-Jørgensen (8):
      bpf: change logging calls from verbose() to bpf_log() and use log pointer
      bpf: verifier: refactor check_attach_btf_id()
      bpf: wrap prog->aux->linked_prog in a bpf_tracing_link
      bpf: support attaching freplace programs to multiple attach points
      bpf: Fix context type resolving for extension programs
      tools: add new members to bpf_attr.raw_tracepoint in bpf.h
      libbpf: add support for supplying target to bpf_raw_tracepoint_open()
      selftests: add test for multiple attachments of freplace program


 include/linux/bpf.h                           |  33 ++-
 include/linux/bpf_verifier.h                  |   9 +
 include/uapi/linux/bpf.h                      |   6 +-
 kernel/bpf/btf.c                              |  22 +-
 kernel/bpf/core.c                             |   5 +-
 kernel/bpf/syscall.c                          | 161 +++++++++--
 kernel/bpf/trampoline.c                       |  34 ++-
 kernel/bpf/verifier.c                         | 251 ++++++++++--------
 tools/include/uapi/linux/bpf.h                |   6 +-
 tools/lib/bpf/bpf.c                           |  13 +-
 tools/lib/bpf/bpf.h                           |   9 +
 tools/lib/bpf/libbpf.map                      |   1 +
 .../selftests/bpf/prog_tests/fexit_bpf2bpf.c  | 171 +++++++++---
 .../selftests/bpf/prog_tests/trace_ext.c      |  93 +++++++
 .../bpf/progs/freplace_get_constant.c         |  15 ++
 .../selftests/bpf/progs/test_trace_ext.c      |  18 ++
 .../bpf/progs/test_trace_ext_tracing.c        |  25 ++
 17 files changed, 683 insertions(+), 189 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/trace_ext.c
 create mode 100644 tools/testing/selftests/bpf/progs/freplace_get_constant.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_trace_ext.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_trace_ext_tracing.c

[PATCH RESEND bpf-next v3 1/9] bpf: change logging calls from verbose() to bpf_log() and use log pointer

From: Toke Høiland-Jørgensen <hidden>
Date: 2020-09-11 09:59:53

From: Toke Høiland-Jørgensen <redacted>

In preparation for moving code around, change a bunch of references to
env->log (and the verbose() logging helper) to use bpf_log() and a direct
pointer to struct bpf_verifier_log. While we're touching the function
signature, mark the 'prog' argument to bpf_check_type_match() as const.

Also enhance the bpf_verifier_log_needed() check to handle NULL pointers
for the log struct so we can re-use the code with logging disabled.

Signed-off-by: Toke Høiland-Jørgensen <redacted>
---
 include/linux/bpf.h          |    2 +-
 include/linux/bpf_verifier.h |    5 +++-
 kernel/bpf/btf.c             |    6 +++--
 kernel/bpf/verifier.c        |   48 +++++++++++++++++++++---------------------
 4 files changed, 31 insertions(+), 30 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index c6d9f2c444f4..5ad4a935a24e 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h

@@ -1394,7 +1394,7 @@ int btf_check_func_arg_match(struct bpf_verifier_env *env, int subprog,
 			     struct bpf_reg_state *regs);
 int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog,
 			  struct bpf_reg_state *reg);
-int btf_check_type_match(struct bpf_verifier_env *env, struct bpf_prog *prog,
+int btf_check_type_match(struct bpf_verifier_log *log, const struct bpf_prog *prog,
 			 struct btf *btf, const struct btf_type *t);
 
 struct bpf_prog *bpf_prog_by_id(u32 id);

diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
index 53c7bd568c5d..20009e766805 100644
--- a/include/linux/bpf_verifier.h
+++ b/include/linux/bpf_verifier.h

@@ -347,8 +347,9 @@ static inline bool bpf_verifier_log_full(const struct bpf_verifier_log *log)
 
 static inline bool bpf_verifier_log_needed(const struct bpf_verifier_log *log)
 {
-	return (log->level && log->ubuf && !bpf_verifier_log_full(log)) ||
-		log->level == BPF_LOG_KERNEL;
+	return log &&
+		((log->level && log->ubuf && !bpf_verifier_log_full(log)) ||
+		 log->level == BPF_LOG_KERNEL);
 }
 
 #define BPF_MAX_SUBPROGS 256

diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index f9ac6935ab3c..2ace56c99c36 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c

@@ -4401,7 +4401,7 @@ static int btf_check_func_type_match(struct bpf_verifier_log *log,
 }
 
 /* Compare BTFs of given program with BTF of target program */
-int btf_check_type_match(struct bpf_verifier_env *env, struct bpf_prog *prog,
+int btf_check_type_match(struct bpf_verifier_log *log, const struct bpf_prog *prog,
 			 struct btf *btf2, const struct btf_type *t2)
 {
 	struct btf *btf1 = prog->aux->btf;

@@ -4409,7 +4409,7 @@ int btf_check_type_match(struct bpf_verifier_env *env, struct bpf_prog *prog,
 	u32 btf_id = 0;
 
 	if (!prog->aux->func_info) {
-		bpf_log(&env->log, "Program extension requires BTF\n");
+		bpf_log(log, "Program extension requires BTF\n");
 		return -EINVAL;
 	}

@@ -4421,7 +4421,7 @@ int btf_check_type_match(struct bpf_verifier_env *env, struct bpf_prog *prog,
 	if (!t1 || !btf_type_is_func(t1))
 		return -EFAULT;
 
-	return btf_check_func_type_match(&env->log, btf1, t1, btf2, t2);
+	return btf_check_func_type_match(log, btf1, t1, btf2, t2);
 }
 
 /* Compare BTF of a function with given bpf_reg_state.

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 814bc6c1ad16..0be7a187fb7f 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c

@@ -11043,6 +11043,7 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)
 	struct bpf_prog *prog = env->prog;
 	bool prog_extension = prog->type == BPF_PROG_TYPE_EXT;
 	struct bpf_prog *tgt_prog = prog->aux->linked_prog;
+	struct bpf_verifier_log *log = &env->log;
 	u32 btf_id = prog->aux->attach_btf_id;
 	const char prefix[] = "btf_trace_";
 	struct btf_func_model fmodel;

@@ -11070,23 +11071,23 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)
 		return 0;
 
 	if (!btf_id) {
-		verbose(env, "Tracing programs must provide btf_id\n");
+		bpf_log(log, "Tracing programs must provide btf_id\n");
 		return -EINVAL;
 	}
 	btf = bpf_prog_get_target_btf(prog);
 	if (!btf) {
-		verbose(env,
+		bpf_log(log,
 			"FENTRY/FEXIT program can only be attached to another program annotated with BTF\n");
 		return -EINVAL;
 	}
 	t = btf_type_by_id(btf, btf_id);
 	if (!t) {
-		verbose(env, "attach_btf_id %u is invalid\n", btf_id);
+		bpf_log(log, "attach_btf_id %u is invalid\n", btf_id);
 		return -EINVAL;
 	}
 	tname = btf_name_by_offset(btf, t->name_off);
 	if (!tname) {
-		verbose(env, "attach_btf_id %u doesn't have a name\n", btf_id);
+		bpf_log(log, "attach_btf_id %u doesn't have a name\n", btf_id);
 		return -EINVAL;
 	}
 	if (tgt_prog) {

@@ -11098,18 +11099,18 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)
 				break;
 			}
 		if (subprog == -1) {
-			verbose(env, "Subprog %s doesn't exist\n", tname);
+			bpf_log(log, "Subprog %s doesn't exist\n", tname);
 			return -EINVAL;
 		}
 		conservative = aux->func_info_aux[subprog].unreliable;
 		if (prog_extension) {
 			if (conservative) {
-				verbose(env,
+				bpf_log(log,
 					"Cannot replace static functions\n");
 				return -EINVAL;
 			}
 			if (!prog->jit_requested) {
-				verbose(env,
+				bpf_log(log,
 					"Extension programs should be JITed\n");
 				return -EINVAL;
 			}

@@ -11117,7 +11118,7 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)
 			prog->expected_attach_type = tgt_prog->expected_attach_type;
 		}
 		if (!tgt_prog->jited) {
-			verbose(env, "Can attach to only JITed progs\n");
+			bpf_log(log, "Can attach to only JITed progs\n");
 			return -EINVAL;
 		}
 		if (tgt_prog->type == prog->type) {

@@ -11125,7 +11126,7 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)
 			 * Cannot attach program extension to another extension.
 			 * It's ok to attach fentry/fexit to extension program.
 			 */
-			verbose(env, "Cannot recursively attach\n");
+			bpf_log(log, "Cannot recursively attach\n");
 			return -EINVAL;
 		}
 		if (tgt_prog->type == BPF_PROG_TYPE_TRACING &&

@@ -11147,13 +11148,13 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)
 			 * reasonable stack size. Hence extending fentry is not
 			 * allowed.
 			 */
-			verbose(env, "Cannot extend fentry/fexit\n");
+			bpf_log(log, "Cannot extend fentry/fexit\n");
 			return -EINVAL;
 		}
 		key = ((u64)aux->id) << 32 | btf_id;
 	} else {
 		if (prog_extension) {
-			verbose(env, "Cannot replace kernel functions\n");
+			bpf_log(log, "Cannot replace kernel functions\n");
 			return -EINVAL;
 		}
 		key = btf_id;

@@ -11162,17 +11163,17 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)
 	switch (prog->expected_attach_type) {
 	case BPF_TRACE_RAW_TP:
 		if (tgt_prog) {
-			verbose(env,
+			bpf_log(log,
 				"Only FENTRY/FEXIT progs are attachable to another BPF prog\n");
 			return -EINVAL;
 		}
 		if (!btf_type_is_typedef(t)) {
-			verbose(env, "attach_btf_id %u is not a typedef\n",
+			bpf_log(log, "attach_btf_id %u is not a typedef\n",
 				btf_id);
 			return -EINVAL;
 		}
 		if (strncmp(prefix, tname, sizeof(prefix) - 1)) {
-			verbose(env, "attach_btf_id %u points to wrong type name %s\n",
+			bpf_log(log, "attach_btf_id %u points to wrong type name %s\n",
 				btf_id, tname);
 			return -EINVAL;
 		}

@@ -11195,7 +11196,7 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)
 		return 0;
 	case BPF_TRACE_ITER:
 		if (!btf_type_is_func(t)) {
-			verbose(env, "attach_btf_id %u is not a function\n",
+			bpf_log(log, "attach_btf_id %u is not a function\n",
 				btf_id);
 			return -EINVAL;
 		}

@@ -11206,8 +11207,7 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)
 		prog->aux->attach_func_proto = t;
 		if (!bpf_iter_prog_supported(prog))
 			return -EINVAL;
-		ret = btf_distill_func_proto(&env->log, btf, t,
-					     tname, &fmodel);
+		ret = btf_distill_func_proto(log, btf, t, tname, &fmodel);
 		return ret;
 	default:
 		if (!prog_extension)

@@ -11219,18 +11219,18 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)
 	case BPF_TRACE_FEXIT:
 		prog->aux->attach_func_name = tname;
 		if (prog->type == BPF_PROG_TYPE_LSM) {
-			ret = bpf_lsm_verify_prog(&env->log, prog);
+			ret = bpf_lsm_verify_prog(log, prog);
 			if (ret < 0)
 				return ret;
 		}
 
 		if (!btf_type_is_func(t)) {
-			verbose(env, "attach_btf_id %u is not a function\n",
+			bpf_log(log, "attach_btf_id %u is not a function\n",
 				btf_id);
 			return -EINVAL;
 		}
 		if (prog_extension &&
-		    btf_check_type_match(env, prog, btf, t))
+		    btf_check_type_match(log, prog, btf, t))
 			return -EINVAL;
 		t = btf_type_by_id(btf, t->type);
 		if (!btf_type_is_func_proto(t))

@@ -11249,7 +11249,7 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)
 			prog->aux->attach_func_proto = NULL;
 			t = NULL;
 		}
-		ret = btf_distill_func_proto(&env->log, btf, t,
+		ret = btf_distill_func_proto(log, btf, t,
 					     tname, &tr->func.model);
 		if (ret < 0)
 			goto out;

@@ -11261,7 +11261,7 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)
 		} else {
 			addr = kallsyms_lookup_name(tname);
 			if (!addr) {
-				verbose(env,
+				bpf_log(log,
 					"The address of function %s cannot be found\n",
 					tname);
 				ret = -ENOENT;

@@ -11291,12 +11291,12 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)
 				break;
 			}
 			if (ret)
-				verbose(env, "%s is not sleepable\n",
+				bpf_log(log, "%s is not sleepable\n",
 					prog->aux->attach_func_name);
 		} else if (prog->expected_attach_type == BPF_MODIFY_RETURN) {
 			ret = check_attach_modify_return(prog, addr);
 			if (ret)
-				verbose(env, "%s() is not modifiable\n",
+				bpf_log(log, "%s() is not modifiable\n",
 					prog->aux->attach_func_name);
 		}
 		if (ret)

[PATCH RESEND bpf-next v3 3/9] bpf: wrap prog->aux->linked_prog in a bpf_tracing_link

From: Toke Høiland-Jørgensen <hidden>
Date: 2020-09-11 09:59:54

From: Toke Høiland-Jørgensen <redacted>

The bpf_tracing_link structure is a convenient data structure to contain
the reference to a linked program; in preparation for supporting multiple
attachments for the same freplace program, move the linked_prog in
prog->aux into a bpf_tracing_link wrapper.

With this change, it is no longer possible to attach the same tracing
program multiple times (detaching in-between), since the reference from the
tracing program to the target disappears on the first attach. However,
since the next patch will let the caller supply an attach target, that will
also make it possible to attach to the same place multiple times.

Signed-off-by: Toke Høiland-Jørgensen <redacted>
---
 include/linux/bpf.h     |   21 +++++++++---
 kernel/bpf/btf.c        |   13 +++++---
 kernel/bpf/core.c       |    5 +--
 kernel/bpf/syscall.c    |   81 +++++++++++++++++++++++++++++++++++++----------
 kernel/bpf/trampoline.c |   12 ++-----
 kernel/bpf/verifier.c   |   13 +++++---
 6 files changed, 102 insertions(+), 43 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 7f19c3216370..722c60f1c1fc 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h

@@ -26,6 +26,7 @@ struct bpf_verifier_log;
 struct perf_event;
 struct bpf_prog;
 struct bpf_prog_aux;
+struct bpf_tracing_link;
 struct bpf_map;
 struct sock;
 struct seq_file;

@@ -614,8 +615,8 @@ static __always_inline unsigned int bpf_dispatcher_nop_func(
 }
 #ifdef CONFIG_BPF_JIT
 struct bpf_trampoline *bpf_trampoline_lookup(u64 key);
-int bpf_trampoline_link_prog(struct bpf_prog *prog);
-int bpf_trampoline_unlink_prog(struct bpf_prog *prog);
+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_get(u64 key, void *addr,
 		       struct btf_func_model *fmodel,
 		       struct bpf_trampoline **trampoline);

@@ -667,11 +668,13 @@ static inline struct bpf_trampoline *bpf_trampoline_lookup(u64 key)
 {
 	return NULL;
 }
-static inline int bpf_trampoline_link_prog(struct bpf_prog *prog)
+static inline int bpf_trampoline_link_prog(struct bpf_prog *prog,
+					   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_prog *prog,
+					     struct bpf_trampoline *tr)
 {
 	return -ENOTSUPP;
 }

@@ -740,14 +743,13 @@ struct bpf_prog_aux {
 	u32 max_rdonly_access;
 	u32 max_rdwr_access;
 	const struct bpf_ctx_arg_aux *ctx_arg_info;
-	struct bpf_prog *linked_prog;
+	struct bpf_tracing_link *tgt_link;
 	bool verifier_zext; /* Zero extensions has been inserted by verifier. */
 	bool offload_requested;
 	bool attach_btf_trace; /* true if attaching to BTF-enabled raw tp */
 	bool func_proto_unreliable;
 	bool sleepable;
 	enum bpf_tramp_prog_type trampoline_prog_type;
-	struct bpf_trampoline *trampoline;
 	struct hlist_node tramp_hlist;
 	/* BTF_KIND_FUNC_PROTO for valid attach_btf_id */
 	const struct btf_type *attach_func_proto;

@@ -827,6 +829,13 @@ struct bpf_link {
 	struct work_struct work;
 };
 
+struct bpf_tracing_link {
+	struct bpf_link link;
+	enum bpf_attach_type attach_type;
+	struct bpf_trampoline *trampoline;
+	struct bpf_prog *tgt_prog;
+};
+
 struct bpf_link_ops {
 	void (*release)(struct bpf_link *link);
 	void (*dealloc)(struct bpf_link *link);

diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 2ace56c99c36..e10f13f8251c 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c

@@ -3706,10 +3706,10 @@ struct btf *btf_parse_vmlinux(void)
 
 struct btf *bpf_prog_get_target_btf(const struct bpf_prog *prog)
 {
-	struct bpf_prog *tgt_prog = prog->aux->linked_prog;
+	struct bpf_tracing_link *tgt_link = prog->aux->tgt_link;
 
-	if (tgt_prog) {
-		return tgt_prog->aux->btf;
+	if (tgt_link && tgt_link->tgt_prog) {
+		return tgt_link->tgt_prog->aux->btf;
 	} else {
 		return btf_vmlinux;
 	}

@@ -3733,14 +3733,17 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
 		    struct bpf_insn_access_aux *info)
 {
 	const struct btf_type *t = prog->aux->attach_func_proto;
-	struct bpf_prog *tgt_prog = prog->aux->linked_prog;
 	struct btf *btf = bpf_prog_get_target_btf(prog);
 	const char *tname = prog->aux->attach_func_name;
 	struct bpf_verifier_log *log = info->log;
+	struct bpf_prog *tgt_prog = NULL;
 	const struct btf_param *args;
 	u32 nr_args, arg;
 	int i, ret;
 
+	if (prog->aux->tgt_link)
+		tgt_prog = prog->aux->tgt_link->tgt_prog;
+
 	if (off % 8) {
 		bpf_log(log, "func '%s' offset %d is not multiple of 8\n",
 			tname, off);

@@ -4572,7 +4575,7 @@ int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog,
 		return -EFAULT;
 	}
 	if (prog_type == BPF_PROG_TYPE_EXT)
-		prog_type = prog->aux->linked_prog->type;
+		prog_type = prog->aux->tgt_link->tgt_prog->type;
 
 	t = btf_type_by_id(btf, t->type);
 	if (!t || !btf_type_is_func_proto(t)) {

diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index ed0b3578867c..54c125cec218 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c

@@ -2130,7 +2130,6 @@ static void bpf_prog_free_deferred(struct work_struct *work)
 	if (aux->prog->has_callchain_buf)
 		put_callchain_buffers();
 #endif
-	bpf_trampoline_put(aux->trampoline);
 	for (i = 0; i < aux->func_cnt; i++)
 		bpf_jit_free(aux->func[i]);
 	if (aux->func_cnt) {

@@ -2146,8 +2145,8 @@ void bpf_prog_free(struct bpf_prog *fp)
 {
 	struct bpf_prog_aux *aux = fp->aux;
 
-	if (aux->linked_prog)
-		bpf_prog_put(aux->linked_prog);
+	if (aux->tgt_link)
+		bpf_link_put(&aux->tgt_link->link);
 	INIT_WORK(&aux->work, bpf_prog_free_deferred);
 	schedule_work(&aux->work);
 }

diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 4108ef3b828b..2d238aa8962e 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c

@@ -2095,10 +2095,13 @@ static bool is_perfmon_prog_type(enum bpf_prog_type prog_type)
 /* last field in 'union bpf_attr' used by this command */
 #define	BPF_PROG_LOAD_LAST_FIELD attach_prog_fd
 
+static struct bpf_tracing_link *bpf_tracing_link_create(struct bpf_prog *prog,
+							struct bpf_prog *tgt_prog);
+
 static int bpf_prog_load(union bpf_attr *attr, union bpf_attr __user *uattr)
 {
 	enum bpf_prog_type type = attr->prog_type;
-	struct bpf_prog *prog;
+	struct bpf_prog *prog, *tgt_prog = NULL;
 	int err;
 	char license[128];
 	bool is_gpl;

@@ -2154,14 +2157,27 @@ static int bpf_prog_load(union bpf_attr *attr, union bpf_attr __user *uattr)
 	prog->expected_attach_type = attr->expected_attach_type;
 	prog->aux->attach_btf_id = attr->attach_btf_id;
 	if (attr->attach_prog_fd) {
-		struct bpf_prog *tgt_prog;
-
 		tgt_prog = bpf_prog_get(attr->attach_prog_fd);
 		if (IS_ERR(tgt_prog)) {
 			err = PTR_ERR(tgt_prog);
 			goto free_prog_nouncharge;
 		}
-		prog->aux->linked_prog = tgt_prog;
+	}
+
+	if (tgt_prog || prog->aux->attach_btf_id) {
+		struct bpf_tracing_link *link;
+
+		link = bpf_tracing_link_create(prog, tgt_prog);
+		if (IS_ERR(link)) {
+			err = PTR_ERR(link);
+			if (tgt_prog)
+				bpf_prog_put(tgt_prog);
+			goto free_prog_nouncharge;
+		}
+		prog->aux->tgt_link = link;
+
+		/* avoid circular ref - will be set on link activation */
+		link->link.prog = NULL;
 	}
 
 	prog->aux->offload_requested = !!attr->prog_ifindex;

@@ -2495,14 +2511,21 @@ struct bpf_link *bpf_link_get_from_fd(u32 ufd)
 	return link;
 }
 
-struct bpf_tracing_link {
-	struct bpf_link link;
-	enum bpf_attach_type attach_type;
-};
-
 static void bpf_tracing_link_release(struct bpf_link *link)
 {
-	WARN_ON_ONCE(bpf_trampoline_unlink_prog(link->prog));
+	struct bpf_tracing_link *tr_link =
+		container_of(link, struct bpf_tracing_link, link);
+
+	if (tr_link->trampoline) {
+		if (link->id)
+			WARN_ON_ONCE(bpf_trampoline_unlink_prog(link->prog,
+								tr_link->trampoline));
+
+		bpf_trampoline_put(tr_link->trampoline);
+	}
+
+	if (tr_link->tgt_prog)
+		bpf_prog_put(tr_link->tgt_prog);
 }
 
 static void bpf_tracing_link_dealloc(struct bpf_link *link)

@@ -2542,10 +2565,27 @@ static const struct bpf_link_ops bpf_tracing_link_lops = {
 	.fill_link_info = bpf_tracing_link_fill_link_info,
 };
 
+static struct bpf_tracing_link *bpf_tracing_link_create(struct bpf_prog *prog,
+							struct bpf_prog *tgt_prog)
+{
+	struct bpf_tracing_link *link;
+
+	link = kzalloc(sizeof(*link), GFP_USER);
+	if (!link)
+		return ERR_PTR(-ENOMEM);
+
+	bpf_link_init(&link->link, BPF_LINK_TYPE_TRACING,
+		      &bpf_tracing_link_lops, prog);
+	link->attach_type = prog->expected_attach_type;
+	link->tgt_prog = tgt_prog;
+
+	return link;
+}
+
 static int bpf_tracing_prog_attach(struct bpf_prog *prog)
 {
+	struct bpf_tracing_link *link, *olink;
 	struct bpf_link_primer link_primer;
-	struct bpf_tracing_link *link;
 	int err;
 
 	switch (prog->type) {

@@ -2574,14 +2614,16 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog)
 		goto out_put_prog;
 	}
 
-	link = kzalloc(sizeof(*link), GFP_USER);
+	link = READ_ONCE(prog->aux->tgt_link);
 	if (!link) {
-		err = -ENOMEM;
+		err = -ENOENT;
+		goto out_put_prog;
+	}
+	olink = cmpxchg(&prog->aux->tgt_link, link, NULL);
+	if (olink != link) {
+		err = -ENOENT;
 		goto out_put_prog;
 	}
-	bpf_link_init(&link->link, BPF_LINK_TYPE_TRACING,
-		      &bpf_tracing_link_lops, prog);
-	link->attach_type = prog->expected_attach_type;
 
 	err = bpf_link_prime(&link->link, &link_primer);
 	if (err) {

@@ -2589,12 +2631,17 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog)
 		goto out_put_prog;
 	}
 
-	err = bpf_trampoline_link_prog(prog);
+	err = bpf_trampoline_link_prog(prog, link->trampoline);
 	if (err) {
 		bpf_link_cleanup(&link_primer);
 		goto out_put_prog;
 	}
 
+	/* at this point the link is no longer referenced from struct bpf_prog,
+	 * so we can populate this without introducing a circular reference.
+	 */
+	link->link.prog = prog;
+
 	return bpf_link_settle(&link_primer);
 out_put_prog:
 	bpf_prog_put(prog);

diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
index cb442c7ece10..abdd431dd502 100644
--- a/kernel/bpf/trampoline.c
+++ b/kernel/bpf/trampoline.c

@@ -261,14 +261,12 @@ static enum bpf_tramp_prog_type bpf_attach_type_to_tramp(struct bpf_prog *prog)
 	}
 }
 
-int bpf_trampoline_link_prog(struct bpf_prog *prog)
+int bpf_trampoline_link_prog(struct bpf_prog *prog, struct bpf_trampoline *tr)
 {
 	enum bpf_tramp_prog_type kind;
-	struct bpf_trampoline *tr;
 	int err = 0;
 	int cnt;
 
-	tr = prog->aux->trampoline;
 	kind = bpf_attach_type_to_tramp(prog);
 	mutex_lock(&tr->mutex);
 	if (tr->extension_prog) {

@@ -301,7 +299,7 @@ int bpf_trampoline_link_prog(struct bpf_prog *prog)
 	}
 	hlist_add_head(&prog->aux->tramp_hlist, &tr->progs_hlist[kind]);
 	tr->progs_cnt[kind]++;
-	err = bpf_trampoline_update(prog->aux->trampoline);
+	err = bpf_trampoline_update(tr);
 	if (err) {
 		hlist_del(&prog->aux->tramp_hlist);
 		tr->progs_cnt[kind]--;

@@ -312,13 +310,11 @@ int bpf_trampoline_link_prog(struct bpf_prog *prog)
 }
 
 /* bpf_trampoline_unlink_prog() should never fail. */
-int bpf_trampoline_unlink_prog(struct bpf_prog *prog)
+int bpf_trampoline_unlink_prog(struct bpf_prog *prog, struct bpf_trampoline *tr)
 {
 	enum bpf_tramp_prog_type kind;
-	struct bpf_trampoline *tr;
 	int err;
 
-	tr = prog->aux->trampoline;
 	kind = bpf_attach_type_to_tramp(prog);
 	mutex_lock(&tr->mutex);
 	if (kind == BPF_TRAMP_REPLACE) {

@@ -330,7 +326,7 @@ int bpf_trampoline_unlink_prog(struct bpf_prog *prog)
 	}
 	hlist_del(&prog->aux->tramp_hlist);
 	tr->progs_cnt[kind]--;
-	err = bpf_trampoline_update(prog->aux->trampoline);
+	err = bpf_trampoline_update(tr);
 out:
 	mutex_unlock(&tr->mutex);
 	return err;

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index f2624784b915..d6714c24c4e7 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c

@@ -2628,8 +2628,10 @@ static int check_map_access(struct bpf_verifier_env *env, u32 regno,
 
 static enum bpf_prog_type resolve_prog_type(struct bpf_prog *prog)
 {
-	return prog->aux->linked_prog ? prog->aux->linked_prog->type
-				      : prog->type;
+	if (prog->aux->tgt_link && prog->aux->tgt_link->tgt_prog)
+		return prog->aux->tgt_link->tgt_prog->type;
+
+	return prog->type;
 }
 
 static bool may_access_direct_pkt_data(struct bpf_verifier_env *env,

@@ -11266,8 +11268,8 @@ int bpf_check_attach_target(struct bpf_verifier_log *log,
 static int check_attach_btf_id(struct bpf_verifier_env *env)
 {
 	struct bpf_prog *prog = env->prog;
-	struct bpf_prog *tgt_prog = prog->aux->linked_prog;
 	u32 btf_id = prog->aux->attach_btf_id;
+	struct bpf_prog *tgt_prog = NULL;
 	struct btf_func_model fmodel;
 	const struct btf_type *t;
 	const char *tname;

@@ -11275,6 +11277,9 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)
 	int ret;
 	u64 key;
 
+	if (prog->aux->tgt_link)
+		tgt_prog = prog->aux->tgt_link->tgt_prog;
+
 	if (prog->aux->sleepable && prog->type != BPF_PROG_TYPE_TRACING &&
 	    prog->type != BPF_PROG_TYPE_LSM) {
 		verbose(env, "Only fentry/fexit/fmod_ret and lsm programs can be sleepable\n");

@@ -11334,7 +11339,7 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)
 				return ret;
 		}
 		return bpf_trampoline_get(key, (void *)addr, &fmodel,
-					  &prog->aux->trampoline);
+					  &prog->aux->tgt_link->trampoline);
 	}
 }

Re: [PATCH RESEND bpf-next v3 3/9] bpf: wrap prog->aux->linked_prog in a bpf_tracing_link

From: Andrii Nakryiko <hidden>
Date: 2020-09-11 19:58:02

On Fri, Sep 11, 2020 at 3:00 AM Toke Høiland-Jørgensen [off-list ref] wrote:

quoted hunk

From: Toke Høiland-Jørgensen <redacted>

The bpf_tracing_link structure is a convenient data structure to contain
the reference to a linked program; in preparation for supporting multiple
attachments for the same freplace program, move the linked_prog in
prog->aux into a bpf_tracing_link wrapper.

With this change, it is no longer possible to attach the same tracing
program multiple times (detaching in-between), since the reference from the
tracing program to the target disappears on the first attach. However,
since the next patch will let the caller supply an attach target, that will
also make it possible to attach to the same place multiple times.

Signed-off-by: Toke Høiland-Jørgensen <redacted>
---
 include/linux/bpf.h     |   21 +++++++++---
 kernel/bpf/btf.c        |   13 +++++---
 kernel/bpf/core.c       |    5 +--
 kernel/bpf/syscall.c    |   81 +++++++++++++++++++++++++++++++++++++----------
 kernel/bpf/trampoline.c |   12 ++-----
 kernel/bpf/verifier.c   |   13 +++++---
 6 files changed, 102 insertions(+), 43 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 7f19c3216370..722c60f1c1fc 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h

@@ -26,6 +26,7 @@ struct bpf_verifier_log;
 struct perf_event;
 struct bpf_prog;
 struct bpf_prog_aux;
+struct bpf_tracing_link;
 struct bpf_map;
 struct sock;
 struct seq_file;

@@ -614,8 +615,8 @@ static __always_inline unsigned int bpf_dispatcher_nop_func(
 }
 #ifdef CONFIG_BPF_JIT
 struct bpf_trampoline *bpf_trampoline_lookup(u64 key);
-int bpf_trampoline_link_prog(struct bpf_prog *prog);
-int bpf_trampoline_unlink_prog(struct bpf_prog *prog);
+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_get(u64 key, void *addr,
                       struct btf_func_model *fmodel,
                       struct bpf_trampoline **trampoline);

@@ -667,11 +668,13 @@ static inline struct bpf_trampoline *bpf_trampoline_lookup(u64 key)
 {
        return NULL;
 }
-static inline int bpf_trampoline_link_prog(struct bpf_prog *prog)
+static inline int bpf_trampoline_link_prog(struct bpf_prog *prog,
+                                          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_prog *prog,
+                                            struct bpf_trampoline *tr)
 {
        return -ENOTSUPP;
 }

@@ -740,14 +743,13 @@ struct bpf_prog_aux {
        u32 max_rdonly_access;
        u32 max_rdwr_access;
        const struct bpf_ctx_arg_aux *ctx_arg_info;
-       struct bpf_prog *linked_prog;
+       struct bpf_tracing_link *tgt_link;
        bool verifier_zext; /* Zero extensions has been inserted by verifier. */
        bool offload_requested;
        bool attach_btf_trace; /* true if attaching to BTF-enabled raw tp */
        bool func_proto_unreliable;
        bool sleepable;
        enum bpf_tramp_prog_type trampoline_prog_type;
-       struct bpf_trampoline *trampoline;
        struct hlist_node tramp_hlist;
        /* BTF_KIND_FUNC_PROTO for valid attach_btf_id */
        const struct btf_type *attach_func_proto;

@@ -827,6 +829,13 @@ struct bpf_link {
        struct work_struct work;
 };

+struct bpf_tracing_link {
+       struct bpf_link link;
+       enum bpf_attach_type attach_type;
+       struct bpf_trampoline *trampoline;
+       struct bpf_prog *tgt_prog;
+};
+
 struct bpf_link_ops {
        void (*release)(struct bpf_link *link);
        void (*dealloc)(struct bpf_link *link);

diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 2ace56c99c36..e10f13f8251c 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c

@@ -3706,10 +3706,10 @@ struct btf *btf_parse_vmlinux(void)

 struct btf *bpf_prog_get_target_btf(const struct bpf_prog *prog)
 {
-       struct bpf_prog *tgt_prog = prog->aux->linked_prog;
+       struct bpf_tracing_link *tgt_link = prog->aux->tgt_link;

-       if (tgt_prog) {
-               return tgt_prog->aux->btf;
+       if (tgt_link && tgt_link->tgt_prog) {
+               return tgt_link->tgt_prog->aux->btf;
        } else {
                return btf_vmlinux;
        }

@@ -3733,14 +3733,17 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
                    struct bpf_insn_access_aux *info)
 {
        const struct btf_type *t = prog->aux->attach_func_proto;
-       struct bpf_prog *tgt_prog = prog->aux->linked_prog;
        struct btf *btf = bpf_prog_get_target_btf(prog);
        const char *tname = prog->aux->attach_func_name;
        struct bpf_verifier_log *log = info->log;
+       struct bpf_prog *tgt_prog = NULL;
        const struct btf_param *args;
        u32 nr_args, arg;
        int i, ret;

+       if (prog->aux->tgt_link)
+               tgt_prog = prog->aux->tgt_link->tgt_prog;
+
        if (off % 8) {
                bpf_log(log, "func '%s' offset %d is not multiple of 8\n",
                        tname, off);

@@ -4572,7 +4575,7 @@ int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog,
                return -EFAULT;
        }
        if (prog_type == BPF_PROG_TYPE_EXT)
-               prog_type = prog->aux->linked_prog->type;
+               prog_type = prog->aux->tgt_link->tgt_prog->type;

        t = btf_type_by_id(btf, t->type);
        if (!t || !btf_type_is_func_proto(t)) {

diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index ed0b3578867c..54c125cec218 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c

@@ -2130,7 +2130,6 @@ static void bpf_prog_free_deferred(struct work_struct *work)
        if (aux->prog->has_callchain_buf)
                put_callchain_buffers();
 #endif
-       bpf_trampoline_put(aux->trampoline);
        for (i = 0; i < aux->func_cnt; i++)
                bpf_jit_free(aux->func[i]);
        if (aux->func_cnt) {

@@ -2146,8 +2145,8 @@ void bpf_prog_free(struct bpf_prog *fp)
 {
        struct bpf_prog_aux *aux = fp->aux;

-       if (aux->linked_prog)
-               bpf_prog_put(aux->linked_prog);
+       if (aux->tgt_link)
+               bpf_link_put(&aux->tgt_link->link);

Until the link is primed, you shouldn't bpf_link_put() it. At this
stage the link itself is just a piece of memory that needs to be
kfree()'d. And your circular dependency problem doesn't exist anymore.
You'll have to put a trampoline and target prog manually here, though
(but you have a similar problem below as well, so might just have a
small helper to do this). But I think it's simpler that relying on an
artificial "defunct" state of not-yet-activated bpf_link, which you do
with the dance around link->prog = NULL.

quoted hunk

        INIT_WORK(&aux->work, bpf_prog_free_deferred);
        schedule_work(&aux->work);
 }

diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 4108ef3b828b..2d238aa8962e 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c

@@ -2095,10 +2095,13 @@ static bool is_perfmon_prog_type(enum bpf_prog_type prog_type)
 /* last field in 'union bpf_attr' used by this command */
 #define        BPF_PROG_LOAD_LAST_FIELD attach_prog_fd

+static struct bpf_tracing_link *bpf_tracing_link_create(struct bpf_prog *prog,
+                                                       struct bpf_prog *tgt_prog);
+
 static int bpf_prog_load(union bpf_attr *attr, union bpf_attr __user *uattr)
 {
        enum bpf_prog_type type = attr->prog_type;
-       struct bpf_prog *prog;
+       struct bpf_prog *prog, *tgt_prog = NULL;
        int err;
        char license[128];
        bool is_gpl;

@@ -2154,14 +2157,27 @@ static int bpf_prog_load(union bpf_attr *attr, union bpf_attr __user *uattr)
        prog->expected_attach_type = attr->expected_attach_type;
        prog->aux->attach_btf_id = attr->attach_btf_id;
        if (attr->attach_prog_fd) {
-               struct bpf_prog *tgt_prog;
-
                tgt_prog = bpf_prog_get(attr->attach_prog_fd);
                if (IS_ERR(tgt_prog)) {
                        err = PTR_ERR(tgt_prog);
                        goto free_prog_nouncharge;
                }
-               prog->aux->linked_prog = tgt_prog;
+       }
+
+       if (tgt_prog || prog->aux->attach_btf_id) {
+               struct bpf_tracing_link *link;
+
+               link = bpf_tracing_link_create(prog, tgt_prog);
+               if (IS_ERR(link)) {
+                       err = PTR_ERR(link);
+                       if (tgt_prog)
+                               bpf_prog_put(tgt_prog);
+                       goto free_prog_nouncharge;
+               }
+               prog->aux->tgt_link = link;
+
+               /* avoid circular ref - will be set on link activation */
+               link->link.prog = NULL;

See above. I think this might work, but it's sort of an entire new
pattern for links and I'd like to avoid having to think about defunct
-> active -> defunct bpf_link state transitions. All you have to do is
clean up the not-yet-activated link appropriately, but otherwise no
unnecessary link internals manipulations are necessary.

quoted hunk

        }

        prog->aux->offload_requested = !!attr->prog_ifindex;

@@ -2495,14 +2511,21 @@ struct bpf_link *bpf_link_get_from_fd(u32 ufd)
        return link;
 }

-struct bpf_tracing_link {
-       struct bpf_link link;
-       enum bpf_attach_type attach_type;
-};
-
 static void bpf_tracing_link_release(struct bpf_link *link)
 {
-       WARN_ON_ONCE(bpf_trampoline_unlink_prog(link->prog));
+       struct bpf_tracing_link *tr_link =
+               container_of(link, struct bpf_tracing_link, link);
+
+       if (tr_link->trampoline) {
+               if (link->id)

this link->id check won't be necessary, as bpf_tracing_link_release
would be called only on fully constructed and activated link.

quoted hunk

+                       WARN_ON_ONCE(bpf_trampoline_unlink_prog(link->prog,
+                                                               tr_link->trampoline));
+
+               bpf_trampoline_put(tr_link->trampoline);
+       }
+
+       if (tr_link->tgt_prog)
+               bpf_prog_put(tr_link->tgt_prog);
 }

 static void bpf_tracing_link_dealloc(struct bpf_link *link)

@@ -2542,10 +2565,27 @@ static const struct bpf_link_ops bpf_tracing_link_lops = {
        .fill_link_info = bpf_tracing_link_fill_link_info,
 };

+static struct bpf_tracing_link *bpf_tracing_link_create(struct bpf_prog *prog,
+                                                       struct bpf_prog *tgt_prog)
+{
+       struct bpf_tracing_link *link;
+
+       link = kzalloc(sizeof(*link), GFP_USER);
+       if (!link)
+               return ERR_PTR(-ENOMEM);
+
+       bpf_link_init(&link->link, BPF_LINK_TYPE_TRACING,
+                     &bpf_tracing_link_lops, prog);
+       link->attach_type = prog->expected_attach_type;
+       link->tgt_prog = tgt_prog;
+
+       return link;
+}
+
 static int bpf_tracing_prog_attach(struct bpf_prog *prog)
 {
+       struct bpf_tracing_link *link, *olink;
        struct bpf_link_primer link_primer;
-       struct bpf_tracing_link *link;
        int err;

        switch (prog->type) {
@@ -2574,14 +2614,16 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog)
                goto out_put_prog;
        }

-       link = kzalloc(sizeof(*link), GFP_USER);
+       link = READ_ONCE(prog->aux->tgt_link);
        if (!link) {
-               err = -ENOMEM;
+               err = -ENOENT;
+               goto out_put_prog;
+       }
+       olink = cmpxchg(&prog->aux->tgt_link, link, NULL);
+       if (olink != link) {
+               err = -ENOENT;
                goto out_put_prog;
        }

Wouldn't single xchg to NULL be sufficient to achieve the same?
READ_ONCE + cmpxchg seems unnecessary to me.

-       bpf_link_init(&link->link, BPF_LINK_TYPE_TRACING,
-                     &bpf_tracing_link_lops, prog);
-       link->attach_type = prog->expected_attach_type;

        err = bpf_link_prime(&link->link, &link_primer);
        if (err) {

if priming errors out, you need to put target prog and trampoline,
kfree(link) won't do it (and calling bpf_link_cleanup() is not correct
before priming). See above as well.

BTW, one interesting side effect of all this is that if your initial
attach failed, you won't be able to try again, because
prog->aux->tgt_link is gone. If that's the problem, we'll need to
introduce locking and copy that link, try to attach, then clear out
prog->aug->tgt_link only if we succeeded. Just bringing this up, as it
might not be obvious (or I might be wrong :).

quoted hunk

@@ -2589,12 +2631,17 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog)
                goto out_put_prog;
        }

-       err = bpf_trampoline_link_prog(prog);
+       err = bpf_trampoline_link_prog(prog, link->trampoline);
        if (err) {
                bpf_link_cleanup(&link_primer);
                goto out_put_prog;
        }

+       /* at this point the link is no longer referenced from struct bpf_prog,
+        * so we can populate this without introducing a circular reference.
+        */
+       link->link.prog = prog;

this won't be necessary either, as per above discussion on circular dependencies

+
        return bpf_link_settle(&link_primer);
 out_put_prog:
        bpf_prog_put(prog);

[...]

Re: [PATCH RESEND bpf-next v3 3/9] bpf: wrap prog->aux->linked_prog in a bpf_tracing_link

From: Toke Høiland-Jørgensen <hidden>
Date: 2020-09-11 20:56:33

Andrii Nakryiko [off-list ref] writes:

On Fri, Sep 11, 2020 at 3:00 AM Toke Høiland-Jørgensen [off-list ref] wrote:

quoted

From: Toke Høiland-Jørgensen <redacted>

The bpf_tracing_link structure is a convenient data structure to contain
the reference to a linked program; in preparation for supporting multiple
attachments for the same freplace program, move the linked_prog in
prog->aux into a bpf_tracing_link wrapper.

With this change, it is no longer possible to attach the same tracing
program multiple times (detaching in-between), since the reference from the
tracing program to the target disappears on the first attach. However,
since the next patch will let the caller supply an attach target, that will
also make it possible to attach to the same place multiple times.

Signed-off-by: Toke Høiland-Jørgensen <redacted>
---
 include/linux/bpf.h     |   21 +++++++++---
 kernel/bpf/btf.c        |   13 +++++---
 kernel/bpf/core.c       |    5 +--
 kernel/bpf/syscall.c    |   81 +++++++++++++++++++++++++++++++++++++----------
 kernel/bpf/trampoline.c |   12 ++-----
 kernel/bpf/verifier.c   |   13 +++++---
 6 files changed, 102 insertions(+), 43 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 7f19c3216370..722c60f1c1fc 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h

@@ -26,6 +26,7 @@ struct bpf_verifier_log;
 struct perf_event;
 struct bpf_prog;
 struct bpf_prog_aux;
+struct bpf_tracing_link;
 struct bpf_map;
 struct sock;
 struct seq_file;

@@ -614,8 +615,8 @@ static __always_inline unsigned int bpf_dispatcher_nop_func(
 }
 #ifdef CONFIG_BPF_JIT
 struct bpf_trampoline *bpf_trampoline_lookup(u64 key);
-int bpf_trampoline_link_prog(struct bpf_prog *prog);
-int bpf_trampoline_unlink_prog(struct bpf_prog *prog);
+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_get(u64 key, void *addr,
                       struct btf_func_model *fmodel,
                       struct bpf_trampoline **trampoline);

@@ -667,11 +668,13 @@ static inline struct bpf_trampoline *bpf_trampoline_lookup(u64 key)
 {
        return NULL;
 }
-static inline int bpf_trampoline_link_prog(struct bpf_prog *prog)
+static inline int bpf_trampoline_link_prog(struct bpf_prog *prog,
+                                          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_prog *prog,
+                                            struct bpf_trampoline *tr)
 {
        return -ENOTSUPP;
 }

@@ -740,14 +743,13 @@ struct bpf_prog_aux {
        u32 max_rdonly_access;
        u32 max_rdwr_access;
        const struct bpf_ctx_arg_aux *ctx_arg_info;
-       struct bpf_prog *linked_prog;
+       struct bpf_tracing_link *tgt_link;
        bool verifier_zext; /* Zero extensions has been inserted by verifier. */
        bool offload_requested;
        bool attach_btf_trace; /* true if attaching to BTF-enabled raw tp */
        bool func_proto_unreliable;
        bool sleepable;
        enum bpf_tramp_prog_type trampoline_prog_type;
-       struct bpf_trampoline *trampoline;
        struct hlist_node tramp_hlist;
        /* BTF_KIND_FUNC_PROTO for valid attach_btf_id */
        const struct btf_type *attach_func_proto;

@@ -827,6 +829,13 @@ struct bpf_link {
        struct work_struct work;
 };

+struct bpf_tracing_link {
+       struct bpf_link link;
+       enum bpf_attach_type attach_type;
+       struct bpf_trampoline *trampoline;
+       struct bpf_prog *tgt_prog;
+};
+
 struct bpf_link_ops {
        void (*release)(struct bpf_link *link);
        void (*dealloc)(struct bpf_link *link);

diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 2ace56c99c36..e10f13f8251c 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c

@@ -3706,10 +3706,10 @@ struct btf *btf_parse_vmlinux(void)

 struct btf *bpf_prog_get_target_btf(const struct bpf_prog *prog)
 {
-       struct bpf_prog *tgt_prog = prog->aux->linked_prog;
+       struct bpf_tracing_link *tgt_link = prog->aux->tgt_link;

-       if (tgt_prog) {
-               return tgt_prog->aux->btf;
+       if (tgt_link && tgt_link->tgt_prog) {
+               return tgt_link->tgt_prog->aux->btf;
        } else {
                return btf_vmlinux;
        }

@@ -3733,14 +3733,17 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
                    struct bpf_insn_access_aux *info)
 {
        const struct btf_type *t = prog->aux->attach_func_proto;
-       struct bpf_prog *tgt_prog = prog->aux->linked_prog;
        struct btf *btf = bpf_prog_get_target_btf(prog);
        const char *tname = prog->aux->attach_func_name;
        struct bpf_verifier_log *log = info->log;
+       struct bpf_prog *tgt_prog = NULL;
        const struct btf_param *args;
        u32 nr_args, arg;
        int i, ret;

+       if (prog->aux->tgt_link)
+               tgt_prog = prog->aux->tgt_link->tgt_prog;
+
        if (off % 8) {
                bpf_log(log, "func '%s' offset %d is not multiple of 8\n",
                        tname, off);

@@ -4572,7 +4575,7 @@ int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog,
                return -EFAULT;
        }
        if (prog_type == BPF_PROG_TYPE_EXT)
-               prog_type = prog->aux->linked_prog->type;
+               prog_type = prog->aux->tgt_link->tgt_prog->type;

        t = btf_type_by_id(btf, t->type);
        if (!t || !btf_type_is_func_proto(t)) {

diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index ed0b3578867c..54c125cec218 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c

@@ -2130,7 +2130,6 @@ static void bpf_prog_free_deferred(struct work_struct *work)
        if (aux->prog->has_callchain_buf)
                put_callchain_buffers();
 #endif
-       bpf_trampoline_put(aux->trampoline);
        for (i = 0; i < aux->func_cnt; i++)
                bpf_jit_free(aux->func[i]);
        if (aux->func_cnt) {

@@ -2146,8 +2145,8 @@ void bpf_prog_free(struct bpf_prog *fp)
 {
        struct bpf_prog_aux *aux = fp->aux;

-       if (aux->linked_prog)
-               bpf_prog_put(aux->linked_prog);
+       if (aux->tgt_link)
+               bpf_link_put(&aux->tgt_link->link);

Until the link is primed, you shouldn't bpf_link_put() it. At this
stage the link itself is just a piece of memory that needs to be
kfree()'d. And your circular dependency problem doesn't exist anymore.
You'll have to put a trampoline and target prog manually here, though
(but you have a similar problem below as well, so might just have a
small helper to do this). But I think it's simpler that relying on an
artificial "defunct" state of not-yet-activated bpf_link, which you do
with the dance around link->prog = NULL.

Yeah, makes sense. I initially figured that would be 'breaking the
abstraction' of bpf_link, but I ended up having to do that anyway, so
you're right I might as well treat it as a piece of memory here.

[...]

quoted

@@ -2574,14 +2614,16 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog)
                goto out_put_prog;
        }

-       link = kzalloc(sizeof(*link), GFP_USER);
+       link = READ_ONCE(prog->aux->tgt_link);
        if (!link) {
-               err = -ENOMEM;
+               err = -ENOENT;
+               goto out_put_prog;
+       }
+       olink = cmpxchg(&prog->aux->tgt_link, link, NULL);
+       if (olink != link) {
+               err = -ENOENT;
                goto out_put_prog;
        }

Wouldn't single xchg to NULL be sufficient to achieve the same?
READ_ONCE + cmpxchg seems unnecessary to me.

It would, but in the next patch I'm introducing a check on the contents
of the link before cmpxchg'ing it, so figured it was easier to just use
the same pattern here.

quoted

-       bpf_link_init(&link->link, BPF_LINK_TYPE_TRACING,
-                     &bpf_tracing_link_lops, prog);
-       link->attach_type = prog->expected_attach_type;

        err = bpf_link_prime(&link->link, &link_primer);
        if (err) {

if priming errors out, you need to put target prog and trampoline,
kfree(link) won't do it (and calling bpf_link_cleanup() is not correct
before priming). See above as well.

Ah yes, good catch!

BTW, one interesting side effect of all this is that if your initial
attach failed, you won't be able to try again, because
prog->aux->tgt_link is gone. If that's the problem, we'll need to
introduce locking and copy that link, try to attach, then clear out
prog->aug->tgt_link only if we succeeded. Just bringing this up, as it
might not be obvious (or I might be wrong :).

Yeah, did think about that. From a purist PoV you're right that a
"destructive attempt" is not ideal; but we already agreed that clearing
out the link on attach was an acceptable change in behaviour. And I
figured that a failure in link_prim() or trampoline_link_prog() would be
quite rare, so not something we'd want to expend a lot of effort
ensuring was really atomic...

-Toke

[PATCH RESEND bpf-next v3 2/9] bpf: verifier: refactor check_attach_btf_id()

From: Toke Høiland-Jørgensen <hidden>
Date: 2020-09-11 09:59:59

From: Toke Høiland-Jørgensen <redacted>

The check_attach_btf_id() function really does three things:

1. It performs a bunch of checks on the program to ensure that the
   attachment is valid.

2. It stores a bunch of state about the attachment being requested in
   the verifier environment and struct bpf_prog objects.

3. It allocates a trampoline for the attachment.

This patch splits out (1.) and (3.) into separate functions in preparation
for reusing them when the actual attachment is happening (in the
raw_tracepoint_open syscall operation), which will allow tracing programs
to have multiple (compatible) attachments.

No functional change is intended with this patch.

Signed-off-by: Toke Høiland-Jørgensen <redacted>
---
 include/linux/bpf.h          |    9 ++
 include/linux/bpf_verifier.h |    9 ++
 kernel/bpf/trampoline.c      |   22 ++++
 kernel/bpf/verifier.c        |  233 +++++++++++++++++++++++-------------------
 4 files changed, 170 insertions(+), 103 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 5ad4a935a24e..7f19c3216370 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h

@@ -616,6 +616,9 @@ static __always_inline unsigned int bpf_dispatcher_nop_func(
 struct bpf_trampoline *bpf_trampoline_lookup(u64 key);
 int bpf_trampoline_link_prog(struct bpf_prog *prog);
 int bpf_trampoline_unlink_prog(struct bpf_prog *prog);
+int bpf_trampoline_get(u64 key, void *addr,
+		       struct btf_func_model *fmodel,
+		       struct bpf_trampoline **trampoline);
 void bpf_trampoline_put(struct bpf_trampoline *tr);
 #define BPF_DISPATCHER_INIT(_name) {				\
 	.mutex = __MUTEX_INITIALIZER(_name.mutex),		\

@@ -672,6 +675,12 @@ static inline int bpf_trampoline_unlink_prog(struct bpf_prog *prog)
 {
 	return -ENOTSUPP;
 }
+static inline int bpf_trampoline_get(u64 key, void *addr,
+				     struct btf_func_model *fmodel,
+				     struct bpf_trampoline **trampoline)
+{
+	return -EOPNOTSUPP;
+}
 static inline void bpf_trampoline_put(struct bpf_trampoline *tr) {}
 #define DEFINE_BPF_DISPATCHER(name)
 #define DECLARE_BPF_DISPATCHER(name)

diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
index 20009e766805..db3db0b69aad 100644
--- a/include/linux/bpf_verifier.h
+++ b/include/linux/bpf_verifier.h

@@ -447,4 +447,13 @@ bpf_prog_offload_remove_insns(struct bpf_verifier_env *env, u32 off, u32 cnt);
 int check_ctx_reg(struct bpf_verifier_env *env,
 		  const struct bpf_reg_state *reg, int regno);
 
+int bpf_check_attach_target(struct bpf_verifier_log *log,
+			    const struct bpf_prog *prog,
+			    const struct bpf_prog *tgt_prog,
+			    u32 btf_id,
+			    struct btf_func_model *fmodel,
+			    long *tgt_addr,
+			    const char **tgt_name,
+			    const struct btf_type **tgt_type);
+
 #endif /* _LINUX_BPF_VERIFIER_H */

diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
index 7dd523a7e32d..cb442c7ece10 100644
--- a/kernel/bpf/trampoline.c
+++ b/kernel/bpf/trampoline.c

@@ -336,6 +336,28 @@ int bpf_trampoline_unlink_prog(struct bpf_prog *prog)
 	return err;
 }
 
+int bpf_trampoline_get(u64 key, void *addr,
+		       struct btf_func_model *fmodel,
+		       struct bpf_trampoline **trampoline)
+{
+	struct bpf_trampoline *tr;
+
+	tr = bpf_trampoline_lookup(key);
+	if (!tr)
+		return -ENOMEM;
+
+	mutex_lock(&tr->mutex);
+	if (tr->func.addr)
+		goto out;
+
+	memcpy(&tr->func.model, fmodel, sizeof(*fmodel));
+	tr->func.addr = addr;
+out:
+	mutex_unlock(&tr->mutex);
+	*trampoline = tr;
+	return 0;
+}
+
 void bpf_trampoline_put(struct bpf_trampoline *tr)
 {
 	if (!tr)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 0be7a187fb7f..f2624784b915 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c

@@ -11038,43 +11038,29 @@ static int check_non_sleepable_error_inject(u32 btf_id)
 	return btf_id_set_contains(&btf_non_sleepable_error_inject, btf_id);
 }
 
-static int check_attach_btf_id(struct bpf_verifier_env *env)
+int bpf_check_attach_target(struct bpf_verifier_log *log,
+			    const struct bpf_prog *prog,
+			    const struct bpf_prog *tgt_prog,
+			    u32 btf_id,
+			    struct btf_func_model *fmodel,
+			    long *tgt_addr,
+			    const char **tgt_name,
+			    const struct btf_type **tgt_type)
 {
-	struct bpf_prog *prog = env->prog;
 	bool prog_extension = prog->type == BPF_PROG_TYPE_EXT;
-	struct bpf_prog *tgt_prog = prog->aux->linked_prog;
-	struct bpf_verifier_log *log = &env->log;
-	u32 btf_id = prog->aux->attach_btf_id;
 	const char prefix[] = "btf_trace_";
-	struct btf_func_model fmodel;
 	int ret = 0, subprog = -1, i;
-	struct bpf_trampoline *tr;
 	const struct btf_type *t;
 	bool conservative = true;
 	const char *tname;
 	struct btf *btf;
-	long addr;
-	u64 key;
-
-	if (prog->aux->sleepable && prog->type != BPF_PROG_TYPE_TRACING &&
-	    prog->type != BPF_PROG_TYPE_LSM) {
-		verbose(env, "Only fentry/fexit/fmod_ret and lsm programs can be sleepable\n");
-		return -EINVAL;
-	}
-
-	if (prog->type == BPF_PROG_TYPE_STRUCT_OPS)
-		return check_struct_ops_btf_id(env);
-
-	if (prog->type != BPF_PROG_TYPE_TRACING &&
-	    prog->type != BPF_PROG_TYPE_LSM &&
-	    !prog_extension)
-		return 0;
+	long addr = 0;
 
 	if (!btf_id) {
 		bpf_log(log, "Tracing programs must provide btf_id\n");
 		return -EINVAL;
 	}
-	btf = bpf_prog_get_target_btf(prog);
+	btf = tgt_prog ? tgt_prog->aux->btf : btf_vmlinux;
 	if (!btf) {
 		bpf_log(log,
 			"FENTRY/FEXIT program can only be attached to another program annotated with BTF\n");

@@ -11114,8 +11100,6 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)
 					"Extension programs should be JITed\n");
 				return -EINVAL;
 			}
-			env->ops = bpf_verifier_ops[tgt_prog->type];
-			prog->expected_attach_type = tgt_prog->expected_attach_type;
 		}
 		if (!tgt_prog->jited) {
 			bpf_log(log, "Can attach to only JITed progs\n");

@@ -11151,13 +11135,11 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)
 			bpf_log(log, "Cannot extend fentry/fexit\n");
 			return -EINVAL;
 		}
-		key = ((u64)aux->id) << 32 | btf_id;
 	} else {
 		if (prog_extension) {
 			bpf_log(log, "Cannot replace kernel functions\n");
 			return -EINVAL;
 		}
-		key = btf_id;
 	}
 
 	switch (prog->expected_attach_type) {

@@ -11187,13 +11169,7 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)
 			/* should never happen in valid vmlinux build */
 			return -EINVAL;
 
-		/* remember two read only pointers that are valid for
-		 * the life time of the kernel
-		 */
-		prog->aux->attach_func_name = tname;
-		prog->aux->attach_func_proto = t;
-		prog->aux->attach_btf_trace = true;
-		return 0;
+		break;
 	case BPF_TRACE_ITER:
 		if (!btf_type_is_func(t)) {
 			bpf_log(log, "attach_btf_id %u is not a function\n",

@@ -11203,12 +11179,10 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)
 		t = btf_type_by_id(btf, t->type);
 		if (!btf_type_is_func_proto(t))
 			return -EINVAL;
-		prog->aux->attach_func_name = tname;
-		prog->aux->attach_func_proto = t;
-		if (!bpf_iter_prog_supported(prog))
-			return -EINVAL;
-		ret = btf_distill_func_proto(log, btf, t, tname, &fmodel);
-		return ret;
+		ret = btf_distill_func_proto(log, btf, t, tname, fmodel);
+		if (ret)
+			return ret;
+		break;
 	default:
 		if (!prog_extension)
 			return -EINVAL;

@@ -11217,13 +11191,6 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)
 	case BPF_LSM_MAC:
 	case BPF_TRACE_FENTRY:
 	case BPF_TRACE_FEXIT:
-		prog->aux->attach_func_name = tname;
-		if (prog->type == BPF_PROG_TYPE_LSM) {
-			ret = bpf_lsm_verify_prog(log, prog);
-			if (ret < 0)
-				return ret;
-		}
-
 		if (!btf_type_is_func(t)) {
 			bpf_log(log, "attach_btf_id %u is not a function\n",
 				btf_id);

@@ -11235,24 +11202,14 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)
 		t = btf_type_by_id(btf, t->type);
 		if (!btf_type_is_func_proto(t))
 			return -EINVAL;
-		tr = bpf_trampoline_lookup(key);
-		if (!tr)
-			return -ENOMEM;
-		/* t is either vmlinux type or another program's type */
-		prog->aux->attach_func_proto = t;
-		mutex_lock(&tr->mutex);
-		if (tr->func.addr) {
-			prog->aux->trampoline = tr;
-			goto out;
-		}
-		if (tgt_prog && conservative) {
-			prog->aux->attach_func_proto = NULL;
+
+		if (tgt_prog && conservative)
 			t = NULL;
-		}
-		ret = btf_distill_func_proto(log, btf, t,
-					     tname, &tr->func.model);
+
+		ret = btf_distill_func_proto(log, btf, t, tname, fmodel);
 		if (ret < 0)
-			goto out;
+			return ret;
+
 		if (tgt_prog) {
 			if (subprog == 0)
 				addr = (long) tgt_prog->bpf_func;

@@ -11264,50 +11221,120 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)
 				bpf_log(log,
 					"The address of function %s cannot be found\n",
 					tname);
-				ret = -ENOENT;
-				goto out;
+				return -ENOENT;
 			}
 		}
+		break;
+	}
 
-		if (prog->aux->sleepable) {
-			ret = -EINVAL;
-			switch (prog->type) {
-			case BPF_PROG_TYPE_TRACING:
-				/* fentry/fexit/fmod_ret progs can be sleepable only if they are
-				 * attached to ALLOW_ERROR_INJECTION and are not in denylist.
-				 */
-				if (!check_non_sleepable_error_inject(btf_id) &&
-				    within_error_injection_list(addr))
-					ret = 0;
-				break;
-			case BPF_PROG_TYPE_LSM:
-				/* LSM progs check that they are attached to bpf_lsm_*() funcs.
-				 * Only some of them are sleepable.
-				 */
-				if (check_sleepable_lsm_hook(btf_id))
-					ret = 0;
-				break;
-			default:
-				break;
-			}
-			if (ret)
-				bpf_log(log, "%s is not sleepable\n",
-					prog->aux->attach_func_name);
-		} else if (prog->expected_attach_type == BPF_MODIFY_RETURN) {
-			ret = check_attach_modify_return(prog, addr);
-			if (ret)
-				bpf_log(log, "%s() is not modifiable\n",
-					prog->aux->attach_func_name);
+	if (prog->aux->sleepable) {
+		ret = -EINVAL;
+		switch (prog->type) {
+		case BPF_PROG_TYPE_TRACING:
+			/* fentry/fexit/fmod_ret progs can be sleepable only if they are
+			 * attached to ALLOW_ERROR_INJECTION and are not in denylist.
+			 */
+			if (!check_non_sleepable_error_inject(btf_id) &&
+			    within_error_injection_list(addr))
+				ret = 0;
+			break;
+		case BPF_PROG_TYPE_LSM:
+			/* LSM progs check that they are attached to bpf_lsm_*() funcs.
+			 * Only some of them are sleepable.
+			 */
+			if (check_sleepable_lsm_hook(btf_id))
+				ret = 0;
+			break;
+		default:
+			break;
 		}
-		if (ret)
-			goto out;
-		tr->func.addr = (void *)addr;
-		prog->aux->trampoline = tr;
-out:
-		mutex_unlock(&tr->mutex);
-		if (ret)
-			bpf_trampoline_put(tr);
+		if (ret) {
+			bpf_log(log, "%s is not sleepable\n",
+				prog->aux->attach_func_name);
+			return ret;
+		}
+	}
+
+	*tgt_addr = addr;
+	if (tgt_name)
+		*tgt_name = tname;
+	if (tgt_type)
+		*tgt_type = t;
+	return 0;
+}
+
+static int check_attach_btf_id(struct bpf_verifier_env *env)
+{
+	struct bpf_prog *prog = env->prog;
+	struct bpf_prog *tgt_prog = prog->aux->linked_prog;
+	u32 btf_id = prog->aux->attach_btf_id;
+	struct btf_func_model fmodel;
+	const struct btf_type *t;
+	const char *tname;
+	long addr;
+	int ret;
+	u64 key;
+
+	if (prog->aux->sleepable && prog->type != BPF_PROG_TYPE_TRACING &&
+	    prog->type != BPF_PROG_TYPE_LSM) {
+		verbose(env, "Only fentry/fexit/fmod_ret and lsm programs can be sleepable\n");
+		return -EINVAL;
+	}
+
+	if (prog->type == BPF_PROG_TYPE_STRUCT_OPS)
+		return check_struct_ops_btf_id(env);
+
+	if (prog->type != BPF_PROG_TYPE_TRACING &&
+	    prog->type != BPF_PROG_TYPE_LSM &&
+	    prog->type != BPF_PROG_TYPE_EXT)
+		return 0;
+
+	ret = bpf_check_attach_target(&env->log, prog, tgt_prog, btf_id,
+				      &fmodel, &addr, &tname, &t);
+	if (ret)
 		return ret;
+
+	if (tgt_prog) {
+		if (prog->type == BPF_PROG_TYPE_EXT) {
+			env->ops = bpf_verifier_ops[tgt_prog->type];
+			prog->expected_attach_type =
+				tgt_prog->expected_attach_type;
+		}
+		key = ((u64)tgt_prog->aux->id) << 32 | btf_id;
+	} else {
+		key = btf_id;
+	}
+
+	prog->aux->attach_func_proto = t;
+	prog->aux->attach_func_name = tname;
+
+	switch (prog->expected_attach_type) {
+	case BPF_TRACE_RAW_TP:
+		/* remember two read only pointers that are valid for
+		 * the life time of the kernel
+		 */
+		prog->aux->attach_btf_trace = true;
+		return 0;
+	case BPF_TRACE_ITER:
+		if (!bpf_iter_prog_supported(prog))
+			return -EINVAL;
+		return 0;
+	case BPF_MODIFY_RETURN:
+		ret = check_attach_modify_return(prog, addr);
+		if (ret) {
+			verbose(env, "%s() is not modifiable\n",
+				prog->aux->attach_func_name);
+			return ret;
+		}
+		fallthrough;
+	default:
+		if (prog->type == BPF_PROG_TYPE_LSM) {
+			ret = bpf_lsm_verify_prog(&env->log, prog);
+			if (ret < 0)
+				return ret;
+		}
+		return bpf_trampoline_get(key, (void *)addr, &fmodel,
+					  &prog->aux->trampoline);
 	}
 }

Re: [PATCH RESEND bpf-next v3 2/9] bpf: verifier: refactor check_attach_btf_id()

From: Andrii Nakryiko <hidden>
Date: 2020-09-11 19:13:48

On Fri, Sep 11, 2020 at 3:00 AM Toke Høiland-Jørgensen [off-list ref] wrote:

From: Toke Høiland-Jørgensen <redacted>

The check_attach_btf_id() function really does three things:

1. It performs a bunch of checks on the program to ensure that the
   attachment is valid.

2. It stores a bunch of state about the attachment being requested in
   the verifier environment and struct bpf_prog objects.

3. It allocates a trampoline for the attachment.

This patch splits out (1.) and (3.) into separate functions in preparation
for reusing them when the actual attachment is happening (in the
raw_tracepoint_open syscall operation), which will allow tracing programs
to have multiple (compatible) attachments.

No functional change is intended with this patch.

Signed-off-by: Toke Høiland-Jørgensen <redacted>
---

I can't tell if there are any functional changes or not, tbh. The
logic is quite complicated and full of intricate details. I did leave
some suggestions on hopefully simplifying code flow in some places
(and ensuring it's harder to break it on future changes), but I hope
Alexei will give it a very thorough review and check that none of the
subtle details broke.

quoted hunk

 include/linux/bpf.h          |    9 ++
 include/linux/bpf_verifier.h |    9 ++
 kernel/bpf/trampoline.c      |   22 ++++
 kernel/bpf/verifier.c        |  233 +++++++++++++++++++++++-------------------
 4 files changed, 170 insertions(+), 103 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 5ad4a935a24e..7f19c3216370 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h

@@ -616,6 +616,9 @@ static __always_inline unsigned int bpf_dispatcher_nop_func(
 struct bpf_trampoline *bpf_trampoline_lookup(u64 key);
 int bpf_trampoline_link_prog(struct bpf_prog *prog);
 int bpf_trampoline_unlink_prog(struct bpf_prog *prog);
+int bpf_trampoline_get(u64 key, void *addr,
+                      struct btf_func_model *fmodel,
+                      struct bpf_trampoline **trampoline);

Given trampoline is clearly a single output parameter, having `struct
bpf_trampoline *` as a return type and encoding error with ERR_PTR()
seems like a more common and natural way (within kernel code)?

Though I do get why you did it this way and with my below suggestion
of having separate switch clauses this reduces duplicated code, so I
guess I don't mind it now.

quoted hunk

 void bpf_trampoline_put(struct bpf_trampoline *tr);
 #define BPF_DISPATCHER_INIT(_name) {                           \
        .mutex = __MUTEX_INITIALIZER(_name.mutex),              \

@@ -672,6 +675,12 @@ static inline int bpf_trampoline_unlink_prog(struct bpf_prog *prog)
 {
        return -ENOTSUPP;
 }
+static inline int bpf_trampoline_get(u64 key, void *addr,
+                                    struct btf_func_model *fmodel,
+                                    struct bpf_trampoline **trampoline)
+{
+       return -EOPNOTSUPP;
+}
 static inline void bpf_trampoline_put(struct bpf_trampoline *tr) {}
 #define DEFINE_BPF_DISPATCHER(name)
 #define DECLARE_BPF_DISPATCHER(name)

diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
index 20009e766805..db3db0b69aad 100644
--- a/include/linux/bpf_verifier.h
+++ b/include/linux/bpf_verifier.h

@@ -447,4 +447,13 @@ bpf_prog_offload_remove_insns(struct bpf_verifier_env *env, u32 off, u32 cnt);
 int check_ctx_reg(struct bpf_verifier_env *env,
                  const struct bpf_reg_state *reg, int regno);

+int bpf_check_attach_target(struct bpf_verifier_log *log,
+                           const struct bpf_prog *prog,
+                           const struct bpf_prog *tgt_prog,
+                           u32 btf_id,
+                           struct btf_func_model *fmodel,
+                           long *tgt_addr,
+                           const char **tgt_name,
+                           const struct btf_type **tgt_type);
+
 #endif /* _LINUX_BPF_VERIFIER_H */

diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
index 7dd523a7e32d..cb442c7ece10 100644
--- a/kernel/bpf/trampoline.c
+++ b/kernel/bpf/trampoline.c

@@ -336,6 +336,28 @@ int bpf_trampoline_unlink_prog(struct bpf_prog *prog)
        return err;
 }

+int bpf_trampoline_get(u64 key, void *addr,
+                      struct btf_func_model *fmodel,
+                      struct bpf_trampoline **trampoline)
+{
+       struct bpf_trampoline *tr;
+
+       tr = bpf_trampoline_lookup(key);
+       if (!tr)
+               return -ENOMEM;
+
+       mutex_lock(&tr->mutex);
+       if (tr->func.addr)
+               goto out;
+
+       memcpy(&tr->func.model, fmodel, sizeof(*fmodel));
+       tr->func.addr = addr;
+out:
+       mutex_unlock(&tr->mutex);
+       *trampoline = tr;
+       return 0;
+}
+
 void bpf_trampoline_put(struct bpf_trampoline *tr)
 {
        if (!tr)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 0be7a187fb7f..f2624784b915 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c

@@ -11038,43 +11038,29 @@ static int check_non_sleepable_error_inject(u32 btf_id)
        return btf_id_set_contains(&btf_non_sleepable_error_inject, btf_id);
 }

-static int check_attach_btf_id(struct bpf_verifier_env *env)
+int bpf_check_attach_target(struct bpf_verifier_log *log,
+                           const struct bpf_prog *prog,
+                           const struct bpf_prog *tgt_prog,
+                           u32 btf_id,
+                           struct btf_func_model *fmodel,
+                           long *tgt_addr,
+                           const char **tgt_name,
+                           const struct btf_type **tgt_type)
 {
-       struct bpf_prog *prog = env->prog;
        bool prog_extension = prog->type == BPF_PROG_TYPE_EXT;
-       struct bpf_prog *tgt_prog = prog->aux->linked_prog;
-       struct bpf_verifier_log *log = &env->log;
-       u32 btf_id = prog->aux->attach_btf_id;
        const char prefix[] = "btf_trace_";
-       struct btf_func_model fmodel;
        int ret = 0, subprog = -1, i;
-       struct bpf_trampoline *tr;
        const struct btf_type *t;
        bool conservative = true;
        const char *tname;
        struct btf *btf;
-       long addr;
-       u64 key;
-
-       if (prog->aux->sleepable && prog->type != BPF_PROG_TYPE_TRACING &&
-           prog->type != BPF_PROG_TYPE_LSM) {
-               verbose(env, "Only fentry/fexit/fmod_ret and lsm programs can be sleepable\n");
-               return -EINVAL;
-       }
-
-       if (prog->type == BPF_PROG_TYPE_STRUCT_OPS)
-               return check_struct_ops_btf_id(env);
-
-       if (prog->type != BPF_PROG_TYPE_TRACING &&
-           prog->type != BPF_PROG_TYPE_LSM &&
-           !prog_extension)
-               return 0;
+       long addr = 0;

        if (!btf_id) {
                bpf_log(log, "Tracing programs must provide btf_id\n");
                return -EINVAL;
        }
-       btf = bpf_prog_get_target_btf(prog);
+       btf = tgt_prog ? tgt_prog->aux->btf : btf_vmlinux;
        if (!btf) {
                bpf_log(log,
                        "FENTRY/FEXIT program can only be attached to another program annotated with BTF\n");

@@ -11114,8 +11100,6 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)
                                        "Extension programs should be JITed\n");
                                return -EINVAL;
                        }
-                       env->ops = bpf_verifier_ops[tgt_prog->type];
-                       prog->expected_attach_type = tgt_prog->expected_attach_type;
                }
                if (!tgt_prog->jited) {
                        bpf_log(log, "Can attach to only JITed progs\n");

@@ -11151,13 +11135,11 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)
                        bpf_log(log, "Cannot extend fentry/fexit\n");
                        return -EINVAL;
                }
-               key = ((u64)aux->id) << 32 | btf_id;
        } else {
                if (prog_extension) {
                        bpf_log(log, "Cannot replace kernel functions\n");
                        return -EINVAL;
                }
-               key = btf_id;
        }

        switch (prog->expected_attach_type) {

@@ -11187,13 +11169,7 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)
                        /* should never happen in valid vmlinux build */
                        return -EINVAL;

-               /* remember two read only pointers that are valid for
-                * the life time of the kernel
-                */
-               prog->aux->attach_func_name = tname;
-               prog->aux->attach_func_proto = t;
-               prog->aux->attach_btf_trace = true;
-               return 0;
+               break;
        case BPF_TRACE_ITER:
                if (!btf_type_is_func(t)) {
                        bpf_log(log, "attach_btf_id %u is not a function\n",

@@ -11203,12 +11179,10 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)
                t = btf_type_by_id(btf, t->type);
                if (!btf_type_is_func_proto(t))
                        return -EINVAL;
-               prog->aux->attach_func_name = tname;
-               prog->aux->attach_func_proto = t;
-               if (!bpf_iter_prog_supported(prog))
-                       return -EINVAL;
-               ret = btf_distill_func_proto(log, btf, t, tname, &fmodel);
-               return ret;
+               ret = btf_distill_func_proto(log, btf, t, tname, fmodel);
+               if (ret)
+                       return ret;
+               break;
        default:
                if (!prog_extension)
                        return -EINVAL;

@@ -11217,13 +11191,6 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)
        case BPF_LSM_MAC:
        case BPF_TRACE_FENTRY:
        case BPF_TRACE_FEXIT:
-               prog->aux->attach_func_name = tname;
-               if (prog->type == BPF_PROG_TYPE_LSM) {
-                       ret = bpf_lsm_verify_prog(log, prog);
-                       if (ret < 0)
-                               return ret;
-               }
-
                if (!btf_type_is_func(t)) {
                        bpf_log(log, "attach_btf_id %u is not a function\n",
                                btf_id);

@@ -11235,24 +11202,14 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)
                t = btf_type_by_id(btf, t->type);
                if (!btf_type_is_func_proto(t))
                        return -EINVAL;
-               tr = bpf_trampoline_lookup(key);
-               if (!tr)
-                       return -ENOMEM;
-               /* t is either vmlinux type or another program's type */
-               prog->aux->attach_func_proto = t;
-               mutex_lock(&tr->mutex);
-               if (tr->func.addr) {
-                       prog->aux->trampoline = tr;
-                       goto out;
-               }
-               if (tgt_prog && conservative) {
-                       prog->aux->attach_func_proto = NULL;
+
+               if (tgt_prog && conservative)
                        t = NULL;
-               }
-               ret = btf_distill_func_proto(log, btf, t,
-                                            tname, &tr->func.model);
+
+               ret = btf_distill_func_proto(log, btf, t, tname, fmodel);
                if (ret < 0)
-                       goto out;
+                       return ret;
+
                if (tgt_prog) {
                        if (subprog == 0)
                                addr = (long) tgt_prog->bpf_func;

@@ -11264,50 +11221,120 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)
                                bpf_log(log,
                                        "The address of function %s cannot be found\n",
                                        tname);
-                               ret = -ENOENT;
-                               goto out;
+                               return -ENOENT;
                        }
                }
+               break;
+       }

-               if (prog->aux->sleepable) {
-                       ret = -EINVAL;
-                       switch (prog->type) {
-                       case BPF_PROG_TYPE_TRACING:
-                               /* fentry/fexit/fmod_ret progs can be sleepable only if they are
-                                * attached to ALLOW_ERROR_INJECTION and are not in denylist.
-                                */
-                               if (!check_non_sleepable_error_inject(btf_id) &&
-                                   within_error_injection_list(addr))
-                                       ret = 0;
-                               break;
-                       case BPF_PROG_TYPE_LSM:
-                               /* LSM progs check that they are attached to bpf_lsm_*() funcs.
-                                * Only some of them are sleepable.
-                                */
-                               if (check_sleepable_lsm_hook(btf_id))
-                                       ret = 0;
-                               break;
-                       default:
-                               break;
-                       }
-                       if (ret)
-                               bpf_log(log, "%s is not sleepable\n",
-                                       prog->aux->attach_func_name);
-               } else if (prog->expected_attach_type == BPF_MODIFY_RETURN) {
-                       ret = check_attach_modify_return(prog, addr);
-                       if (ret)
-                               bpf_log(log, "%s() is not modifiable\n",
-                                       prog->aux->attach_func_name);
+       if (prog->aux->sleepable) {
+               ret = -EINVAL;
+               switch (prog->type) {
+               case BPF_PROG_TYPE_TRACING:
+                       /* fentry/fexit/fmod_ret progs can be sleepable only if they are
+                        * attached to ALLOW_ERROR_INJECTION and are not in denylist.
+                        */
+                       if (!check_non_sleepable_error_inject(btf_id) &&
+                           within_error_injection_list(addr))
+                               ret = 0;
+                       break;
+               case BPF_PROG_TYPE_LSM:
+                       /* LSM progs check that they are attached to bpf_lsm_*() funcs.
+                        * Only some of them are sleepable.
+                        */
+                       if (check_sleepable_lsm_hook(btf_id))
+                               ret = 0;
+                       break;
+               default:
+                       break;
                }
-               if (ret)
-                       goto out;
-               tr->func.addr = (void *)addr;
-               prog->aux->trampoline = tr;
-out:
-               mutex_unlock(&tr->mutex);
-               if (ret)
-                       bpf_trampoline_put(tr);
+               if (ret) {
+                       bpf_log(log, "%s is not sleepable\n",
+                               prog->aux->attach_func_name);
+                       return ret;

the flow would be simpler if you just bpf_log(log, "%s is not
sleepable\n", prog->aux->attach_func_name); and return error here,
same for second case below. I don't think saving single bpf_log
repetition deserves this complication.

+               }
+       }
+
+       *tgt_addr = addr;
+       if (tgt_name)
+               *tgt_name = tname;
+       if (tgt_type)
+               *tgt_type = t;
+       return 0;
+}
+
+static int check_attach_btf_id(struct bpf_verifier_env *env)
+{
+       struct bpf_prog *prog = env->prog;
+       struct bpf_prog *tgt_prog = prog->aux->linked_prog;
+       u32 btf_id = prog->aux->attach_btf_id;
+       struct btf_func_model fmodel;
+       const struct btf_type *t;
+       const char *tname;
+       long addr;
+       int ret;
+       u64 key;
+
+       if (prog->aux->sleepable && prog->type != BPF_PROG_TYPE_TRACING &&
+           prog->type != BPF_PROG_TYPE_LSM) {
+               verbose(env, "Only fentry/fexit/fmod_ret and lsm programs can be sleepable\n");
+               return -EINVAL;
+       }
+
+       if (prog->type == BPF_PROG_TYPE_STRUCT_OPS)
+               return check_struct_ops_btf_id(env);
+
+       if (prog->type != BPF_PROG_TYPE_TRACING &&
+           prog->type != BPF_PROG_TYPE_LSM &&
+           prog->type != BPF_PROG_TYPE_EXT)
+               return 0;
+
+       ret = bpf_check_attach_target(&env->log, prog, tgt_prog, btf_id,
+                                     &fmodel, &addr, &tname, &t);
+       if (ret)
                return ret;
+
+       if (tgt_prog) {
+               if (prog->type == BPF_PROG_TYPE_EXT) {
+                       env->ops = bpf_verifier_ops[tgt_prog->type];
+                       prog->expected_attach_type =
+                               tgt_prog->expected_attach_type;
+               }
+               key = ((u64)tgt_prog->aux->id) << 32 | btf_id;
+       } else {
+               key = btf_id;
+       }
+
+       prog->aux->attach_func_proto = t;
+       prog->aux->attach_func_name = tname;
+
+       switch (prog->expected_attach_type) {
+       case BPF_TRACE_RAW_TP:
+               /* remember two read only pointers that are valid for
+                * the life time of the kernel
+                */

comment is misplaced now, it was referring to
prog->aux->attach_func_proto and prog->aux->attach_func_name

+               prog->aux->attach_btf_trace = true;
+               return 0;
+       case BPF_TRACE_ITER:
+               if (!bpf_iter_prog_supported(prog))
+                       return -EINVAL;
+               return 0;
+       case BPF_MODIFY_RETURN:
+               ret = check_attach_modify_return(prog, addr);
+               if (ret) {
+                       verbose(env, "%s() is not modifiable\n",
+                               prog->aux->attach_func_name);
+                       return ret;
+               }
+               fallthrough;

this fallthrough is only to do bpf_trampoline_get in the end, is that
right? Why not just call bpf_trampoline_get() here? That's not a lot
of duplication at all, but will be easier to follow and make sure
everything is right.

+       default:
+               if (prog->type == BPF_PROG_TYPE_LSM) {
+                       ret = bpf_lsm_verify_prog(&env->log, prog);
+                       if (ret < 0)
+                               return ret;
+               }

Same here, BPF_PROG_TYPE_LSM (and we can use BPF_LSM_MAC expected
attach type, right?) is clearly a separate case, deserving its own
case?

As for the default one, let's keep it for _EXT case (and enforce it as
original code did). For BPF_TRACE_FEXIT and BPF_TRACE_FENTRY, let's
have a separate case as well.

These switches with default clauses and fallthroughs... they are mind
bending, it's really hard and time consuming to ensure one has
considered all possible code paths leading to this default case. It's
all already quite complicated, let's not add to the complexity.

+               return bpf_trampoline_get(key, (void *)addr, &fmodel,
+                                         &prog->aux->trampoline);
        }
 }

Re: [PATCH RESEND bpf-next v3 2/9] bpf: verifier: refactor check_attach_btf_id()

From: Toke Høiland-Jørgensen <hidden>
Date: 2020-09-11 19:51:10

Andrii Nakryiko [off-list ref] writes:

On Fri, Sep 11, 2020 at 3:00 AM Toke Høiland-Jørgensen [off-list ref] wrote:

quoted

From: Toke Høiland-Jørgensen <redacted>

The check_attach_btf_id() function really does three things:

1. It performs a bunch of checks on the program to ensure that the
   attachment is valid.

2. It stores a bunch of state about the attachment being requested in
   the verifier environment and struct bpf_prog objects.

3. It allocates a trampoline for the attachment.

This patch splits out (1.) and (3.) into separate functions in preparation
for reusing them when the actual attachment is happening (in the
raw_tracepoint_open syscall operation), which will allow tracing programs
to have multiple (compatible) attachments.

No functional change is intended with this patch.

Signed-off-by: Toke Høiland-Jørgensen <redacted>
---

I can't tell if there are any functional changes or not, tbh. The
logic is quite complicated and full of intricate details. I did leave
some suggestions on hopefully simplifying code flow in some places
(and ensuring it's harder to break it on future changes), but I hope
Alexei will give it a very thorough review and check that none of the
subtle details broke.

Yeah, totally agree this is gnarly... :/
Which is also why I chickened out of doing any further changes in an
attempt to simplify the flow, but rather kept as much as the existing
structure as possible (with somewhat mixed results, I suppose).

Let's see what Alexei thinks. I guess I can take another crack at it, in
which case, thank you for the suggestions for simplifying things!

-Toke

[PATCH RESEND bpf-next v3 7/9] libbpf: add support for supplying target to bpf_raw_tracepoint_open()

From: Toke Høiland-Jørgensen <hidden>
Date: 2020-09-11 10:00:15

From: Toke Høiland-Jørgensen <redacted>

This adds support for supplying a target fd and btf ID for the
raw_tracepoint_open() BPF operation, using a new bpf_raw_tracepoint_opts
structure. This can be used for attaching freplace programs to multiple
destinations.

Signed-off-by: Toke Høiland-Jørgensen <redacted>
---
 tools/lib/bpf/bpf.c      |   13 ++++++++++++-
 tools/lib/bpf/bpf.h      |    9 +++++++++
 tools/lib/bpf/libbpf.map |    1 +
 3 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
index 82b983ff6569..25c62993c406 100644
--- a/tools/lib/bpf/bpf.c
+++ b/tools/lib/bpf/bpf.c

@@ -804,17 +804,28 @@ int bpf_obj_get_info_by_fd(int bpf_fd, void *info, __u32 *info_len)
 	return err;
 }
 
-int bpf_raw_tracepoint_open(const char *name, int prog_fd)
+int bpf_raw_tracepoint_open_opts(const char *name, int prog_fd,
+				 struct bpf_raw_tracepoint_opts *opts)
 {
 	union bpf_attr attr;
 
+	if (!OPTS_VALID(opts, bpf_raw_tracepoint_opts))
+		return -EINVAL;
+
 	memset(&attr, 0, sizeof(attr));
 	attr.raw_tracepoint.name = ptr_to_u64(name);
 	attr.raw_tracepoint.prog_fd = prog_fd;
+	attr.raw_tracepoint.tgt_prog_fd = OPTS_GET(opts, tgt_prog_fd, 0);
+	attr.raw_tracepoint.tgt_btf_id = OPTS_GET(opts, tgt_btf_id, 0);
 
 	return sys_bpf(BPF_RAW_TRACEPOINT_OPEN, &attr, sizeof(attr));
 }
 
+int bpf_raw_tracepoint_open(const char *name, int prog_fd)
+{
+	return bpf_raw_tracepoint_open_opts(name, prog_fd, NULL);
+}
+
 int bpf_load_btf(void *btf, __u32 btf_size, char *log_buf, __u32 log_buf_size,
 		 bool do_log)
 {

diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
index 015d13f25fcc..30e8854374c0 100644
--- a/tools/lib/bpf/bpf.h
+++ b/tools/lib/bpf/bpf.h

@@ -233,7 +233,16 @@ LIBBPF_API int bpf_obj_get_info_by_fd(int bpf_fd, void *info, __u32 *info_len);
 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);
+struct bpf_raw_tracepoint_opts {
+	size_t sz; /* size of this struct for forward/backward compatibility */
+	int tgt_prog_fd; /* target program to attach to */
+	__u32 tgt_btf_id; /* BTF ID of target function */
+};
+#define bpf_raw_tracepoint_opts__last_field tgt_btf_id
+
 LIBBPF_API int bpf_raw_tracepoint_open(const char *name, int prog_fd);
+LIBBPF_API int bpf_raw_tracepoint_open_opts(const char *name, int prog_fd,
+					    struct bpf_raw_tracepoint_opts *opts);
 LIBBPF_API int bpf_load_btf(void *btf, __u32 btf_size, char *log_buf,
 			    __u32 log_buf_size, bool do_log);
 LIBBPF_API int bpf_task_fd_query(int pid, int fd, __u32 flags, char *buf,

diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
index 92ceb48a5ca2..a23d9f3f940c 100644
--- a/tools/lib/bpf/libbpf.map
+++ b/tools/lib/bpf/libbpf.map

@@ -303,6 +303,7 @@ LIBBPF_0.1.0 {
 LIBBPF_0.2.0 {
 	global:
 		bpf_program__section_name;
+		bpf_raw_tracepoint_open_opts;
 		perf_buffer__buffer_cnt;
 		perf_buffer__buffer_fd;
 		perf_buffer__epoll_fd;

Re: [PATCH RESEND bpf-next v3 7/9] libbpf: add support for supplying target to bpf_raw_tracepoint_open()

From: Andrii Nakryiko <hidden>
Date: 2020-09-11 21:21:23

On Fri, Sep 11, 2020 at 3:00 AM Toke Høiland-Jørgensen [off-list ref] wrote:

quoted hunk

From: Toke Høiland-Jørgensen <redacted>

This adds support for supplying a target fd and btf ID for the
raw_tracepoint_open() BPF operation, using a new bpf_raw_tracepoint_opts
structure. This can be used for attaching freplace programs to multiple
destinations.

Signed-off-by: Toke Høiland-Jørgensen <redacted>
---
 tools/lib/bpf/bpf.c      |   13 ++++++++++++-
 tools/lib/bpf/bpf.h      |    9 +++++++++
 tools/lib/bpf/libbpf.map |    1 +
 3 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
index 82b983ff6569..25c62993c406 100644
--- a/tools/lib/bpf/bpf.c
+++ b/tools/lib/bpf/bpf.c

@@ -804,17 +804,28 @@ int bpf_obj_get_info_by_fd(int bpf_fd, void *info, __u32 *info_len)
        return err;
 }

-int bpf_raw_tracepoint_open(const char *name, int prog_fd)
+int bpf_raw_tracepoint_open_opts(const char *name, int prog_fd,

We've had discussion around naming low-level APIs with options and
agreed with Andrey Ignatov that we'll follow the _xattr suffix naming
for low-level APIs. So let's keep it consistent, I suppose...

Please also add bpf_program__attach_trace_opts() for high-level API.

+                                struct bpf_raw_tracepoint_opts *opts)
 {
        union bpf_attr attr;

+       if (!OPTS_VALID(opts, bpf_raw_tracepoint_opts))
+               return -EINVAL;
+
        memset(&attr, 0, sizeof(attr));
        attr.raw_tracepoint.name = ptr_to_u64(name);
        attr.raw_tracepoint.prog_fd = prog_fd;
+       attr.raw_tracepoint.tgt_prog_fd = OPTS_GET(opts, tgt_prog_fd, 0);
+       attr.raw_tracepoint.tgt_btf_id = OPTS_GET(opts, tgt_btf_id, 0);

        return sys_bpf(BPF_RAW_TRACEPOINT_OPEN, &attr, sizeof(attr));
 }

[...]

[PATCH RESEND bpf-next v3 9/9] selftests/bpf: Adding test for arg dereference in extension trace

From: Toke Høiland-Jørgensen <hidden>
Date: 2020-09-11 10:00:20

From: Jiri Olsa <jolsa@kernel.org>

Adding test that setup following program:

  SEC("classifier/test_pkt_md_access")
  int test_pkt_md_access(struct __sk_buff *skb)

with its extension:

  SEC("freplace/test_pkt_md_access")
  int test_pkt_md_access_new(struct __sk_buff *skb)

and tracing that extension with:

  SEC("fentry/test_pkt_md_access_new")
  int BPF_PROG(fentry, struct sk_buff *skb)

The test verifies that the tracing program can
dereference skb argument properly.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/testing/selftests/bpf/prog_tests/trace_ext.c |   93 ++++++++++++++++++++
 tools/testing/selftests/bpf/progs/test_trace_ext.c |   18 ++++
 .../selftests/bpf/progs/test_trace_ext_tracing.c   |   25 +++++
 3 files changed, 136 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/trace_ext.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_trace_ext.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_trace_ext_tracing.c

diff --git a/tools/testing/selftests/bpf/prog_tests/trace_ext.c b/tools/testing/selftests/bpf/prog_tests/trace_ext.c
new file mode 100644
index 000000000000..1089dafb4653
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/trace_ext.c

@@ -0,0 +1,93 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#define _GNU_SOURCE
+#include <test_progs.h>
+#include <network_helpers.h>
+#include <sys/stat.h>
+#include <linux/sched.h>
+#include <sys/syscall.h>
+
+#include "test_trace_ext.skel.h"
+#include "test_trace_ext_tracing.skel.h"
+
+static __u32 duration;
+
+void test_trace_ext(void)
+{
+	struct test_trace_ext_tracing *skel_trace = NULL;
+	struct test_trace_ext_tracing__bss *bss_trace;
+	const char *file = "./test_pkt_md_access.o";
+	struct test_trace_ext *skel_ext = NULL;
+	struct test_trace_ext__bss *bss_ext;
+	int err, prog_fd, ext_fd;
+	struct bpf_object *obj;
+	char buf[100];
+	__u32 retval;
+	__u64 len;
+
+	err = bpf_prog_load(file, BPF_PROG_TYPE_SCHED_CLS, &obj, &prog_fd);
+	if (CHECK_FAIL(err))
+		return;
+
+	DECLARE_LIBBPF_OPTS(bpf_object_open_opts, opts,
+			    .attach_prog_fd = prog_fd,
+	);
+
+	skel_ext = test_trace_ext__open_opts(&opts);
+	if (CHECK(!skel_ext, "setup", "freplace/test_pkt_md_access open failed\n"))
+		goto cleanup;
+
+	err = test_trace_ext__load(skel_ext);
+	if (CHECK(err, "setup", "freplace/test_pkt_md_access load failed\n")) {
+		libbpf_strerror(err, buf, sizeof(buf));
+		fprintf(stderr, "%s\n", buf);
+		goto cleanup;
+	}
+
+	err = test_trace_ext__attach(skel_ext);
+	if (CHECK(err, "setup", "freplace/test_pkt_md_access attach failed: %d\n", err))
+		goto cleanup;
+
+	ext_fd = bpf_program__fd(skel_ext->progs.test_pkt_md_access_new);
+
+	DECLARE_LIBBPF_OPTS(bpf_object_open_opts, opts_trace,
+			    .attach_prog_fd = ext_fd,
+	);
+
+	skel_trace = test_trace_ext_tracing__open_opts(&opts_trace);
+	if (CHECK(!skel_trace, "setup", "tracing/test_pkt_md_access_new open failed\n"))
+		goto cleanup;
+
+	err = test_trace_ext_tracing__load(skel_trace);
+	if (CHECK(err, "setup", "tracing/test_pkt_md_access_new load failed\n")) {
+		libbpf_strerror(err, buf, sizeof(buf));
+		fprintf(stderr, "%s\n", buf);
+		goto cleanup;
+	}
+
+	err = test_trace_ext_tracing__attach(skel_trace);
+	if (CHECK(err, "setup", "tracing/test_pkt_md_access_new attach failed: %d\n", err))
+		goto cleanup;
+
+	err = bpf_prog_test_run(prog_fd, 1, &pkt_v4, sizeof(pkt_v4),
+				NULL, NULL, &retval, &duration);
+	CHECK(err || retval, "",
+	      "err %d errno %d retval %d duration %d\n",
+	      err, errno, retval, duration);
+
+	bss_ext = skel_ext->bss;
+	bss_trace = skel_trace->bss;
+
+	len = bss_ext->ext_called;
+
+	CHECK(bss_ext->ext_called == 0,
+		"check", "failed to trigger freplace/test_pkt_md_access\n");
+	CHECK(bss_trace->fentry_called != len,
+		"check", "failed to trigger fentry/test_pkt_md_access_new\n");
+	CHECK(bss_trace->fexit_called != len,
+		"check", "failed to trigger fexit/test_pkt_md_access_new\n");
+
+cleanup:
+	test_trace_ext__destroy(skel_ext);
+	bpf_object__close(obj);
+}

diff --git a/tools/testing/selftests/bpf/progs/test_trace_ext.c b/tools/testing/selftests/bpf/progs/test_trace_ext.c
new file mode 100644
index 000000000000..a6318f6b52ee
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_trace_ext.c

@@ -0,0 +1,18 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2019 Facebook
+#include <linux/bpf.h>
+#include <stdbool.h>
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_endian.h>
+#include <bpf/bpf_tracing.h>
+
+volatile __u64 ext_called = 0;
+
+SEC("freplace/test_pkt_md_access")
+int test_pkt_md_access_new(struct __sk_buff *skb)
+{
+	ext_called = skb->len;
+	return 0;
+}
+
+char _license[] SEC("license") = "GPL";

diff --git a/tools/testing/selftests/bpf/progs/test_trace_ext_tracing.c b/tools/testing/selftests/bpf/progs/test_trace_ext_tracing.c
new file mode 100644
index 000000000000..9e52a831446f
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_trace_ext_tracing.c

@@ -0,0 +1,25 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include "vmlinux.h"
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+
+volatile __u64 fentry_called = 0;
+
+SEC("fentry/test_pkt_md_access_new")
+int BPF_PROG(fentry, struct sk_buff *skb)
+{
+	fentry_called = skb->len;
+	return 0;
+}
+
+volatile __u64 fexit_called = 0;
+
+SEC("fexit/test_pkt_md_access_new")
+int BPF_PROG(fexit, struct sk_buff *skb)
+{
+	fexit_called = skb->len;
+	return 0;
+}
+
+char _license[] SEC("license") = "GPL";

[PATCH RESEND bpf-next v3 6/9] tools: add new members to bpf_attr.raw_tracepoint in bpf.h

From: Toke Høiland-Jørgensen <hidden>
Date: 2020-09-11 10:00:38

From: Toke Høiland-Jørgensen <redacted>

Sync addition of new members from main kernel tree.

Signed-off-by: Toke Høiland-Jørgensen <redacted>
---
 tools/include/uapi/linux/bpf.h |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 90359cab501d..0885ab6ac8d9 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h

@@ -595,8 +595,10 @@ union bpf_attr {
 	} query;
 
 	struct { /* anonymous struct used by BPF_RAW_TRACEPOINT_OPEN command */
-		__u64 name;
-		__u32 prog_fd;
+		__u64		name;
+		__u32		prog_fd;
+		__u32		tgt_prog_fd;
+		__u32		tgt_btf_id;
 	} raw_tracepoint;
 
 	struct { /* anonymous struct for BPF_BTF_LOAD */

Re: [PATCH RESEND bpf-next v3 6/9] tools: add new members to bpf_attr.raw_tracepoint in bpf.h

From: Andrii Nakryiko <hidden>
Date: 2020-09-11 21:16:50

On Fri, Sep 11, 2020 at 3:01 AM Toke Høiland-Jørgensen [off-list ref] wrote:

From: Toke Høiland-Jørgensen <redacted>

Sync addition of new members from main kernel tree.

Signed-off-by: Toke Høiland-Jørgensen <redacted>
---

There is no more need to split these updates into separate patches,
feel free to fold it into the patch that changes original header.

quoted hunk

 tools/include/uapi/linux/bpf.h |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 90359cab501d..0885ab6ac8d9 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h

@@ -595,8 +595,10 @@ union bpf_attr {
        } query;

        struct { /* anonymous struct used by BPF_RAW_TRACEPOINT_OPEN command */
-               __u64 name;
-               __u32 prog_fd;
+               __u64           name;
+               __u32           prog_fd;
+               __u32           tgt_prog_fd;
+               __u32           tgt_btf_id;
        } raw_tracepoint;

        struct { /* anonymous struct for BPF_BTF_LOAD */

[PATCH RESEND bpf-next v3 4/9] bpf: support attaching freplace programs to multiple attach points

From: Toke Høiland-Jørgensen <hidden>
Date: 2020-09-11 10:00:44

From: Toke Høiland-Jørgensen <redacted>

This enables support for attaching freplace programs to multiple attach
points. It does this by amending UAPI for bpf_raw_tracepoint_open with a
target prog fd and btf ID pair that can be used to supply the new
attachment point. The target must be compatible with the target that was
supplied at program load time.

The implementation reuses the checks that were factored out of
check_attach_btf_id() to ensure compatibility between the BTF types of the
old and new attachment. If these match, a new bpf_tracing_link will be
created for the new attach target, allowing multiple attachments to
co-exist simultaneously.

The code could theoretically support multiple-attach of other types of
tracing programs as well, but since I don't have a use case for any of
those, the bpf_tracing_prog_attach() function will reject new targets for
anything other than PROG_TYPE_EXT programs.

Signed-off-by: Toke Høiland-Jørgensen <redacted>
---
 include/linux/bpf.h      |    3 +
 include/uapi/linux/bpf.h |    6 ++-
 kernel/bpf/syscall.c     |   96 +++++++++++++++++++++++++++++++++++++++-------
 kernel/bpf/verifier.c    |    9 ++++
 4 files changed, 97 insertions(+), 17 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 722c60f1c1fc..c6b856b2d296 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h

@@ -753,6 +753,9 @@ struct bpf_prog_aux {
 	struct hlist_node tramp_hlist;
 	/* BTF_KIND_FUNC_PROTO for valid attach_btf_id */
 	const struct btf_type *attach_func_proto;
+	/* target BPF prog types for trace programs */
+	enum bpf_prog_type tgt_prog_type;
+	enum bpf_attach_type tgt_attach_type;
 	/* function name for valid attach_btf_id */
 	const char *attach_func_name;
 	struct bpf_prog **func;

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 90359cab501d..0885ab6ac8d9 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h

@@ -595,8 +595,10 @@ union bpf_attr {
 	} query;
 
 	struct { /* anonymous struct used by BPF_RAW_TRACEPOINT_OPEN command */
-		__u64 name;
-		__u32 prog_fd;
+		__u64		name;
+		__u32		prog_fd;
+		__u32		tgt_prog_fd;
+		__u32		tgt_btf_id;
 	} raw_tracepoint;
 
 	struct { /* anonymous struct for BPF_BTF_LOAD */

diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 2d238aa8962e..7b1da5f063eb 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c

@@ -4,6 +4,7 @@
 #include <linux/bpf.h>
 #include <linux/bpf_trace.h>
 #include <linux/bpf_lirc.h>
+#include <linux/bpf_verifier.h>
 #include <linux/btf.h>
 #include <linux/syscalls.h>
 #include <linux/slab.h>

@@ -2582,10 +2583,16 @@ static struct bpf_tracing_link *bpf_tracing_link_create(struct bpf_prog *prog,
 	return link;
 }
 
-static int bpf_tracing_prog_attach(struct bpf_prog *prog)
+static int bpf_tracing_prog_attach(struct bpf_prog *prog,
+				   int tgt_prog_fd,
+				   u32 btf_id)
 {
-	struct bpf_tracing_link *link, *olink;
 	struct bpf_link_primer link_primer;
+	struct bpf_prog *tgt_prog = NULL;
+	struct bpf_tracing_link *link;
+	struct btf_func_model fmodel;
+	long addr;
+	u64 key;
 	int err;
 
 	switch (prog->type) {

@@ -2613,28 +2620,80 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog)
 		err = -EINVAL;
 		goto out_put_prog;
 	}
+	if (tgt_prog_fd) {
+		/* For now we only allow new targets for BPF_PROG_TYPE_EXT */
+		if (prog->type != BPF_PROG_TYPE_EXT ||
+		    !btf_id) {
+			err = -EINVAL;
+			goto out_put_prog;
+		}
 
-	link = READ_ONCE(prog->aux->tgt_link);
-	if (!link) {
-		err = -ENOENT;
+		tgt_prog = bpf_prog_get(tgt_prog_fd);
+		if (IS_ERR(tgt_prog)) {
+			err = PTR_ERR(tgt_prog);
+			tgt_prog = NULL;
+			goto out_put_prog;
+		}
+
+		key = ((u64)tgt_prog->aux->id) << 32 | btf_id;
+	} else if (btf_id) {
+		err = -EINVAL;
 		goto out_put_prog;
 	}
-	olink = cmpxchg(&prog->aux->tgt_link, link, NULL);
-	if (olink != link) {
-		err = -ENOENT;
-		goto out_put_prog;
+
+	link = READ_ONCE(prog->aux->tgt_link);
+	if (link) {
+		if (tgt_prog && link->trampoline->key != key) {
+			link = NULL;
+		} else {
+			struct bpf_tracing_link *olink;
+
+			olink = cmpxchg(&prog->aux->tgt_link, link, NULL);
+			if (olink != link) {
+				link = NULL;
+			} else if (tgt_prog) {
+				/* re-using link that already has ref on
+				 * tgt_prog, don't take another
+				 */
+				bpf_prog_put(tgt_prog);
+				tgt_prog = NULL;
+			}
+		}
+	}
+
+	if (!link) {
+		if (!tgt_prog) {
+			err = -ENOENT;
+			goto out_put_prog;
+		}
+
+		err = bpf_check_attach_target(NULL, prog, tgt_prog, btf_id,
+					      &fmodel, &addr, NULL, NULL);
+		if (err)
+			goto out_put_prog;
+
+		link = bpf_tracing_link_create(prog, tgt_prog);
+		if (IS_ERR(link)) {
+			err = PTR_ERR(link);
+			goto out_put_prog;
+		}
+		tgt_prog = NULL;
+
+		err = bpf_trampoline_get(key, (void *)addr, &fmodel, &link->trampoline);
+		if (err)
+			goto out_put_link;
 	}
 
 	err = bpf_link_prime(&link->link, &link_primer);
 	if (err) {
 		kfree(link);
-		goto out_put_prog;
+		goto out_put_link;
 	}
 
 	err = bpf_trampoline_link_prog(prog, link->trampoline);
 	if (err) {
 		bpf_link_cleanup(&link_primer);
-		goto out_put_prog;
+		goto out_put_link;
 	}
 
 	/* at this point the link is no longer referenced from struct bpf_prog,

@@ -2643,8 +2702,12 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog)
 	link->link.prog = prog;
 
 	return bpf_link_settle(&link_primer);
+out_put_link:
+	bpf_link_put(&link->link);
 out_put_prog:
 	bpf_prog_put(prog);
+	if (tgt_prog)
+		bpf_prog_put(tgt_prog);
 	return err;
 }

@@ -2722,7 +2785,7 @@ static const struct bpf_link_ops bpf_raw_tp_link_lops = {
 	.fill_link_info = bpf_raw_tp_link_fill_link_info,
 };
 
-#define BPF_RAW_TRACEPOINT_OPEN_LAST_FIELD raw_tracepoint.prog_fd
+#define BPF_RAW_TRACEPOINT_OPEN_LAST_FIELD raw_tracepoint.tgt_btf_id
 
 static int bpf_raw_tracepoint_open(const union bpf_attr *attr)
 {

@@ -2746,8 +2809,9 @@ static int bpf_raw_tracepoint_open(const union bpf_attr *attr)
 	case BPF_PROG_TYPE_EXT:
 	case BPF_PROG_TYPE_LSM:
 		if (attr->raw_tracepoint.name) {
-			/* The attach point for this category of programs
-			 * should be specified via btf_id during program load.
+			/* The attach point for this category of programs should
+			 * be specified via btf_id during program load, or using
+			 * tgt_btf_id.
 			 */
 			err = -EINVAL;
 			goto out_put_prog;

@@ -2757,7 +2821,9 @@ static int bpf_raw_tracepoint_open(const union bpf_attr *attr)
 			tp_name = prog->aux->attach_func_name;
 			break;
 		}
-		return bpf_tracing_prog_attach(prog);
+		return bpf_tracing_prog_attach(prog,
+					       attr->raw_tracepoint.tgt_prog_fd,
+					       attr->raw_tracepoint.tgt_btf_id);
 	case BPF_PROG_TYPE_RAW_TRACEPOINT:
 	case BPF_PROG_TYPE_RAW_TRACEPOINT_WRITABLE:
 		if (strncpy_from_user(buf,

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index d6714c24c4e7..df01e71b118d 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c

@@ -11205,6 +11205,12 @@ int bpf_check_attach_target(struct bpf_verifier_log *log,
 		if (!btf_type_is_func_proto(t))
 			return -EINVAL;
 
+		if ((prog->aux->tgt_prog_type &&
+		     prog->aux->tgt_prog_type != tgt_prog->type) ||
+		    (prog->aux->tgt_attach_type &&
+		     prog->aux->tgt_attach_type != tgt_prog->expected_attach_type))
+			return -EINVAL;
+
 		if (tgt_prog && conservative)
 			t = NULL;

@@ -11300,6 +11306,9 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)
 		return ret;
 
 	if (tgt_prog) {
+		prog->aux->tgt_prog_type = tgt_prog->type;
+		prog->aux->tgt_attach_type = tgt_prog->expected_attach_type;
+
 		if (prog->type == BPF_PROG_TYPE_EXT) {
 			env->ops = bpf_verifier_ops[tgt_prog->type];
 			prog->expected_attach_type =

Re: [PATCH RESEND bpf-next v3 4/9] bpf: support attaching freplace programs to multiple attach points

From: Andrii Nakryiko <hidden>
Date: 2020-09-11 21:10:50

On Fri, Sep 11, 2020 at 3:01 AM Toke Høiland-Jørgensen [off-list ref] wrote:

From: Toke Høiland-Jørgensen <redacted>

This enables support for attaching freplace programs to multiple attach
points. It does this by amending UAPI for bpf_raw_tracepoint_open with a
target prog fd and btf ID pair that can be used to supply the new
attachment point. The target must be compatible with the target that was
supplied at program load time.

The implementation reuses the checks that were factored out of
check_attach_btf_id() to ensure compatibility between the BTF types of the
old and new attachment. If these match, a new bpf_tracing_link will be
created for the new attach target, allowing multiple attachments to
co-exist simultaneously.

The code could theoretically support multiple-attach of other types of
tracing programs as well, but since I don't have a use case for any of
those, the bpf_tracing_prog_attach() function will reject new targets for
anything other than PROG_TYPE_EXT programs.

Signed-off-by: Toke Høiland-Jørgensen <redacted>
---

It feels like using a semi-constructed bpf_tracing_link inside
prog->aux->tgt_link is just an unnecessary complication, after reading
this and previous patches. Seems more straightforward and simpler to
store tgt_attach_type/tgt_prog_type (permanently) and
tgt_prog/tgt_trampoline (until first attachment) in prog->aux and then
properly create bpf_link on attach.

quoted hunk

 include/linux/bpf.h      |    3 +
 include/uapi/linux/bpf.h |    6 ++-
 kernel/bpf/syscall.c     |   96 +++++++++++++++++++++++++++++++++++++++-------
 kernel/bpf/verifier.c    |    9 ++++
 4 files changed, 97 insertions(+), 17 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 722c60f1c1fc..c6b856b2d296 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h

@@ -753,6 +753,9 @@ struct bpf_prog_aux {
        struct hlist_node tramp_hlist;
        /* BTF_KIND_FUNC_PROTO for valid attach_btf_id */
        const struct btf_type *attach_func_proto;
+       /* target BPF prog types for trace programs */
+       enum bpf_prog_type tgt_prog_type;
+       enum bpf_attach_type tgt_attach_type;
        /* function name for valid attach_btf_id */
        const char *attach_func_name;
        struct bpf_prog **func;

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 90359cab501d..0885ab6ac8d9 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h

@@ -595,8 +595,10 @@ union bpf_attr {
        } query;

        struct { /* anonymous struct used by BPF_RAW_TRACEPOINT_OPEN command */
-               __u64 name;
-               __u32 prog_fd;
+               __u64           name;
+               __u32           prog_fd;
+               __u32           tgt_prog_fd;
+               __u32           tgt_btf_id;
        } raw_tracepoint;

rant: any chance of putting this into LINK_CREATE instead of extending
very unfortunately named RAW_TRACEPOINT_OPEN?

quoted hunk

        struct { /* anonymous struct for BPF_BTF_LOAD */

diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 2d238aa8962e..7b1da5f063eb 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c

@@ -4,6 +4,7 @@
 #include <linux/bpf.h>
 #include <linux/bpf_trace.h>
 #include <linux/bpf_lirc.h>
+#include <linux/bpf_verifier.h>
 #include <linux/btf.h>
 #include <linux/syscalls.h>
 #include <linux/slab.h>

@@ -2582,10 +2583,16 @@ static struct bpf_tracing_link *bpf_tracing_link_create(struct bpf_prog *prog,
        return link;
 }

-static int bpf_tracing_prog_attach(struct bpf_prog *prog)
+static int bpf_tracing_prog_attach(struct bpf_prog *prog,
+                                  int tgt_prog_fd,
+                                  u32 btf_id)
 {
-       struct bpf_tracing_link *link, *olink;
        struct bpf_link_primer link_primer;
+       struct bpf_prog *tgt_prog = NULL;
+       struct bpf_tracing_link *link;
+       struct btf_func_model fmodel;
+       long addr;
+       u64 key;
        int err;

        switch (prog->type) {

@@ -2613,28 +2620,80 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog)
                err = -EINVAL;
                goto out_put_prog;
        }
+       if (tgt_prog_fd) {
+               /* For now we only allow new targets for BPF_PROG_TYPE_EXT */
+               if (prog->type != BPF_PROG_TYPE_EXT ||
+                   !btf_id) {
+                       err = -EINVAL;
+                       goto out_put_prog;
+               }

-       link = READ_ONCE(prog->aux->tgt_link);
-       if (!link) {
-               err = -ENOENT;
+               tgt_prog = bpf_prog_get(tgt_prog_fd);
+               if (IS_ERR(tgt_prog)) {
+                       err = PTR_ERR(tgt_prog);
+                       tgt_prog = NULL;
+                       goto out_put_prog;
+               }
+
+               key = ((u64)tgt_prog->aux->id) << 32 | btf_id;
+       } else if (btf_id) {
+               err = -EINVAL;
                goto out_put_prog;
        }
-       olink = cmpxchg(&prog->aux->tgt_link, link, NULL);
-       if (olink != link) {
-               err = -ENOENT;
-               goto out_put_prog;
+
+       link = READ_ONCE(prog->aux->tgt_link);
+       if (link) {
+               if (tgt_prog && link->trampoline->key != key) {

I think we need to have a proper locking about this. Imagine two
attaches racing, both read non-NULL tgt_link, one of them proceeds to
cmpxchg, attach, detach, and free link. Then this one wakes up and
tries to access freed memory here. We are coordinating multiple
threads on this, it needs to be locked, at least for simplicity, given
that performance is not critical here.

+                       link = NULL;
+               } else {
+                       struct bpf_tracing_link *olink;
+
+                       olink = cmpxchg(&prog->aux->tgt_link, link, NULL);
+                       if (olink != link) {
+                               link = NULL;
+                       } else if (tgt_prog) {
+                               /* re-using link that already has ref on
+                                * tgt_prog, don't take another
+                                */
+                               bpf_prog_put(tgt_prog);
+                               tgt_prog = NULL;
+                       }
+               }
+       }
+
+       if (!link) {
+               if (!tgt_prog) {
+                       err = -ENOENT;
+                       goto out_put_prog;
+               }
+
+               err = bpf_check_attach_target(NULL, prog, tgt_prog, btf_id,
+                                             &fmodel, &addr, NULL, NULL);
+               if (err)
+                       goto out_put_prog;
+
+               link = bpf_tracing_link_create(prog, tgt_prog);
+               if (IS_ERR(link)) {
+                       err = PTR_ERR(link);
+                       goto out_put_prog;
+               }
+               tgt_prog = NULL;
+
+               err = bpf_trampoline_get(key, (void *)addr, &fmodel, &link->trampoline);
+               if (err)
+                       goto out_put_link;

see previous patch, let's avoid bpf_link_put before bpf_link_settle.
bpf_link_cleanup() is for after priming, otherwise it's just a kfree.

        }

        err = bpf_link_prime(&link->link, &link_primer);
        if (err) {
                kfree(link);
-               goto out_put_prog;
+               goto out_put_link;

hm... did you try running this with KASAN? you are freeing link and
then bpf_link_put() on it?

        }

        err = bpf_trampoline_link_prog(prog, link->trampoline);
        if (err) {
                bpf_link_cleanup(&link_primer);
-               goto out_put_prog;
+               goto out_put_link;

similarly, you've already bpf_link_cleanup()'d, no need to do extra
bpf_link_put()

quoted hunk

        }

        /* at this point the link is no longer referenced from struct bpf_prog,

@@ -2643,8 +2702,12 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog)
        link->link.prog = prog;

        return bpf_link_settle(&link_primer);
+out_put_link:
+       bpf_link_put(&link->link);
 out_put_prog:
        bpf_prog_put(prog);
+       if (tgt_prog)
+               bpf_prog_put(tgt_prog);
        return err;
 }

[...]

Re: [PATCH RESEND bpf-next v3 4/9] bpf: support attaching freplace programs to multiple attach points

From: Toke Høiland-Jørgensen <hidden>
Date: 2020-09-14 16:10:04

Andrii Nakryiko [off-list ref] writes:

On Fri, Sep 11, 2020 at 3:01 AM Toke Høiland-Jørgensen [off-list ref] wrote:

quoted

From: Toke Høiland-Jørgensen <redacted>

This enables support for attaching freplace programs to multiple attach
points. It does this by amending UAPI for bpf_raw_tracepoint_open with a
target prog fd and btf ID pair that can be used to supply the new
attachment point. The target must be compatible with the target that was
supplied at program load time.

The implementation reuses the checks that were factored out of
check_attach_btf_id() to ensure compatibility between the BTF types of the
old and new attachment. If these match, a new bpf_tracing_link will be
created for the new attach target, allowing multiple attachments to
co-exist simultaneously.

The code could theoretically support multiple-attach of other types of
tracing programs as well, but since I don't have a use case for any of
those, the bpf_tracing_prog_attach() function will reject new targets for
anything other than PROG_TYPE_EXT programs.

Signed-off-by: Toke Høiland-Jørgensen <redacted>
---

It feels like using a semi-constructed bpf_tracing_link inside
prog->aux->tgt_link is just an unnecessary complication, after reading
this and previous patches. Seems more straightforward and simpler to
store tgt_attach_type/tgt_prog_type (permanently) and
tgt_prog/tgt_trampoline (until first attachment) in prog->aux and then
properly create bpf_link on attach.

I updated v4 with your comments, but kept the link in prog->aux; the
reason being that having a container for the two pointers makes it
possible to atomically swap it out with xchg() as you suggested
previously. Could you please take a look at v4? If you still think it's
better to just keep two separate pointers (and add a lock) in prog->aux,
I can change it to that. But I'd rather avoid the lock if possible...

-Toke

Re: [PATCH RESEND bpf-next v3 4/9] bpf: support attaching freplace programs to multiple attach points

From: Andrii Nakryiko <hidden>
Date: 2020-09-14 23:10:24

On Mon, Sep 14, 2020 at 9:08 AM Toke Høiland-Jørgensen [off-list ref] wrote:

Andrii Nakryiko [off-list ref] writes:

quoted

On Fri, Sep 11, 2020 at 3:01 AM Toke Høiland-Jørgensen [off-list ref] wrote:

quoted

From: Toke Høiland-Jørgensen <redacted>

This enables support for attaching freplace programs to multiple attach
points. It does this by amending UAPI for bpf_raw_tracepoint_open with a
target prog fd and btf ID pair that can be used to supply the new
attachment point. The target must be compatible with the target that was
supplied at program load time.

The implementation reuses the checks that were factored out of
check_attach_btf_id() to ensure compatibility between the BTF types of the
old and new attachment. If these match, a new bpf_tracing_link will be
created for the new attach target, allowing multiple attachments to
co-exist simultaneously.

The code could theoretically support multiple-attach of other types of
tracing programs as well, but since I don't have a use case for any of
those, the bpf_tracing_prog_attach() function will reject new targets for
anything other than PROG_TYPE_EXT programs.

Signed-off-by: Toke Høiland-Jørgensen <redacted>
---

It feels like using a semi-constructed bpf_tracing_link inside
prog->aux->tgt_link is just an unnecessary complication, after reading
this and previous patches. Seems more straightforward and simpler to
store tgt_attach_type/tgt_prog_type (permanently) and
tgt_prog/tgt_trampoline (until first attachment) in prog->aux and then
properly create bpf_link on attach.

I updated v4 with your comments, but kept the link in prog->aux; the
reason being that having a container for the two pointers makes it
possible to atomically swap it out with xchg() as you suggested
previously. Could you please take a look at v4? If you still think it's
better to just keep two separate pointers (and add a lock) in prog->aux,
I can change it to that. But I'd rather avoid the lock if possible...

I took a very quick look at this specific bit, planning to do another
pass tomorrow.

What's the problem with adding a mutex to bpf_prog_aux? In your case,
now you introduced (unlikely, but still) extra state transition for
tgt_link from non-NULL to NULL and then back to non-NULL? And why?
Just to use atomic xchg, while using atomic operation is not an
absolute necessity because it's not a performance-critical path at
all. We are not optimizing for millions of freplace attachments a
second, right? On the other hand, having a mutex there won't require
restoration logic, it will be dead simple, obvious and
straightforward. So yeah, I still think mutex is better there.

BTW, check Stanislav's latest patch set. He's adding used_maps_mutex
to bpf_prog_aux with no problems at all. It seems to me that we might
want to generalize that used_maps_mutex to be just bpf_prog_aux's
mutex ('prog_aux_mutex' or whatever we'd call it) and use it for such
kinds of low-frequency bpf_prog metadata manipulations/checks.

Thoughts?

-Toke

Re: [PATCH RESEND bpf-next v3 4/9] bpf: support attaching freplace programs to multiple attach points

From: Toke Høiland-Jørgensen <hidden>
Date: 2020-09-15 11:37:31

Andrii Nakryiko [off-list ref] writes:

On Mon, Sep 14, 2020 at 9:08 AM Toke Høiland-Jørgensen [off-list ref] wrote:

quoted

Andrii Nakryiko [off-list ref] writes:

quoted

On Fri, Sep 11, 2020 at 3:01 AM Toke Høiland-Jørgensen [off-list ref] wrote:

quoted

From: Toke Høiland-Jørgensen <redacted>

This enables support for attaching freplace programs to multiple attach
points. It does this by amending UAPI for bpf_raw_tracepoint_open with a
target prog fd and btf ID pair that can be used to supply the new
attachment point. The target must be compatible with the target that was
supplied at program load time.

The implementation reuses the checks that were factored out of
check_attach_btf_id() to ensure compatibility between the BTF types of the
old and new attachment. If these match, a new bpf_tracing_link will be
created for the new attach target, allowing multiple attachments to
co-exist simultaneously.

The code could theoretically support multiple-attach of other types of
tracing programs as well, but since I don't have a use case for any of
those, the bpf_tracing_prog_attach() function will reject new targets for
anything other than PROG_TYPE_EXT programs.

Signed-off-by: Toke Høiland-Jørgensen <redacted>
---

It feels like using a semi-constructed bpf_tracing_link inside
prog->aux->tgt_link is just an unnecessary complication, after reading
this and previous patches. Seems more straightforward and simpler to
store tgt_attach_type/tgt_prog_type (permanently) and
tgt_prog/tgt_trampoline (until first attachment) in prog->aux and then
properly create bpf_link on attach.

I updated v4 with your comments, but kept the link in prog->aux; the
reason being that having a container for the two pointers makes it
possible to atomically swap it out with xchg() as you suggested
previously. Could you please take a look at v4? If you still think it's
better to just keep two separate pointers (and add a lock) in prog->aux,
I can change it to that. But I'd rather avoid the lock if possible...

I took a very quick look at this specific bit, planning to do another
pass tomorrow.

What's the problem with adding a mutex to bpf_prog_aux? In your case,
now you introduced (unlikely, but still) extra state transition for
tgt_link from non-NULL to NULL and then back to non-NULL? And why?
Just to use atomic xchg, while using atomic operation is not an
absolute necessity because it's not a performance-critical path at
all. We are not optimizing for millions of freplace attachments a
second, right? On the other hand, having a mutex there won't require
restoration logic, it will be dead simple, obvious and
straightforward. So yeah, I still think mutex is better there.

So I went ahead and implemented a mutex-based version of this. I'm not
sure I agree with "dead simple", I'd say it's on par with the previous
version; and that is only if I explicitly limit the scope of the mutex
to *writing* of the tgt_* pointers (i.e., I haven't gone through and
protected all the reads from within the verifier).

The mutex version does have the benefit of still making it possible to
retry a bpf_raw_tracepoint_open() if it fails, so I guess that is a
benefit; I'll post it as v5 and you can have a look :)

BTW, check Stanislav's latest patch set. He's adding used_maps_mutex
to bpf_prog_aux with no problems at all. It seems to me that we might
want to generalize that used_maps_mutex to be just bpf_prog_aux's
mutex ('prog_aux_mutex' or whatever we'd call it) and use it for such
kinds of low-frequency bpf_prog metadata manipulations/checks.

I'm not sure I like the idea of widening the scope of the mutex. Or at
least I think that should be done as a follow-up patch that does a
proper analysis of all the different fields it is supposed to protect
and makes sure no deadlocks creep in.

-Toke

[PATCH RESEND bpf-next v3 5/9] bpf: Fix context type resolving for extension programs

From: Toke Høiland-Jørgensen <hidden>
Date: 2020-09-11 10:00:45

From: Toke Høiland-Jørgensen <redacted>

Eelco reported we can't properly access arguments if the tracing
program is attached to extension program.

Having following program:

  SEC("classifier/test_pkt_md_access")
  int test_pkt_md_access(struct __sk_buff *skb)

with its extension:

  SEC("freplace/test_pkt_md_access")
  int test_pkt_md_access_new(struct __sk_buff *skb)

and tracing that extension with:

  SEC("fentry/test_pkt_md_access_new")
  int BPF_PROG(fentry, struct sk_buff *skb)

It's not possible to access skb argument in the fentry program,
with following error from verifier:

  ; int BPF_PROG(fentry, struct sk_buff *skb)
  0: (79) r1 = *(u64 *)(r1 +0)
  invalid bpf_context access off=0 size=8

The problem is that btf_ctx_access gets the context type for the
traced program, which is in this case the extension.

But when we trace extension program, we want to get the context
type of the program that the extension is attached to, so we can
access the argument properly in the trace program.

This version of the patch is tweaked slightly from Jiri's original one,
since the refactoring in the previous patches means we have to get the
target prog type from the new variable in prog->aux instead of directly
from the target prog.

Reported-by: Eelco Chaudron <echaudro@redhat.com>
Suggested-by: Jiri Olsa <jolsa@kernel.org>
Signed-off-by: Toke Høiland-Jørgensen <redacted>
---
 kernel/bpf/btf.c |    9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index e10f13f8251c..1a48253ba168 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c

@@ -3863,7 +3863,14 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
 
 	info->reg_type = PTR_TO_BTF_ID;
 	if (tgt_prog) {
-		ret = btf_translate_to_vmlinux(log, btf, t, tgt_prog->type, arg);
+		enum bpf_prog_type tgt_type;
+
+		if (tgt_prog->type == BPF_PROG_TYPE_EXT)
+			tgt_type = tgt_prog->aux->tgt_prog_type;
+		else
+			tgt_type = tgt_prog->type;
+
+		ret = btf_translate_to_vmlinux(log, btf, t, tgt_type, arg);
 		if (ret > 0) {
 			info->btf_id = ret;
 			return true;

[PATCH RESEND bpf-next v3 8/9] selftests: add test for multiple attachments of freplace program

From: Toke Høiland-Jørgensen <hidden>
Date: 2020-09-11 10:06:42

From: Toke Høiland-Jørgensen <redacted>

This adds a selftest for attaching an freplace program to multiple targets
simultaneously.

Signed-off-by: Toke Høiland-Jørgensen <redacted>
---
 .../selftests/bpf/prog_tests/fexit_bpf2bpf.c       |  171 ++++++++++++++++----
 .../selftests/bpf/progs/freplace_get_constant.c    |   15 ++
 2 files changed, 154 insertions(+), 32 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/progs/freplace_get_constant.c

diff --git a/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c b/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c
index eda682727787..cdd0c74f2fbb 100644
--- a/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c
+++ b/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c

@@ -2,36 +2,79 @@
 /* Copyright (c) 2019 Facebook */
 #include <test_progs.h>
 #include <network_helpers.h>
+#include <bpf/btf.h>
+
+typedef int (*test_cb)(struct bpf_object *obj);
+
+static int check_data_map(struct bpf_object *obj, int prog_cnt, bool reset)
+{
+	struct bpf_map *data_map = NULL, *map;
+	__u64 *result = NULL;
+	const int zero = 0;
+	__u32 duration = 0;
+	int ret = -1, i;
+
+	result = malloc((prog_cnt + 32 /* spare */) * sizeof(__u64));
+	if (CHECK(!result, "alloc_memory", "failed to alloc memory"))
+		return -ENOMEM;
+
+	bpf_object__for_each_map(map, obj)
+		if (bpf_map__is_internal(map)) {
+			data_map = map;
+			break;
+		}
+	if (CHECK(!data_map, "find_data_map", "data map not found\n"))
+		goto out;
+
+	ret = bpf_map_lookup_elem(bpf_map__fd(data_map), &zero, result);
+	if (CHECK(ret, "get_result",
+		  "failed to get output data: %d\n", ret))
+		goto out;
+
+	for (i = 0; i < prog_cnt; i++) {
+		if (CHECK(result[i] != 1, "result",
+			  "fexit_bpf2bpf result[%d] failed err %llu\n",
+			  i, result[i]))
+			goto out;
+		result[i] = 0;
+	}
+	if (reset) {
+		ret = bpf_map_update_elem(bpf_map__fd(data_map), &zero, result, 0);
+		if (CHECK(ret, "reset_result", "failed to reset result\n"))
+			goto out;
+	}
+
+	ret = 0;
+out:
+	free(result);
+	return ret;
+}
 
 static void test_fexit_bpf2bpf_common(const char *obj_file,
 				      const char *target_obj_file,
 				      int prog_cnt,
 				      const char **prog_name,
-				      bool run_prog)
+				      bool run_prog,
+				      test_cb cb)
 {
-	struct bpf_object *obj = NULL, *pkt_obj;
-	int err, pkt_fd, i;
-	struct bpf_link **link = NULL;
+	struct bpf_object *obj = NULL, *tgt_obj;
 	struct bpf_program **prog = NULL;
+	struct bpf_link **link = NULL;
 	__u32 duration = 0, retval;
-	struct bpf_map *data_map;
-	const int zero = 0;
-	__u64 *result = NULL;
+	int err, tgt_fd, i;
 
 	err = bpf_prog_load(target_obj_file, BPF_PROG_TYPE_UNSPEC,
-			    &pkt_obj, &pkt_fd);
+			    &tgt_obj, &tgt_fd);
 	if (CHECK(err, "tgt_prog_load", "file %s err %d errno %d\n",
 		  target_obj_file, err, errno))
 		return;
 	DECLARE_LIBBPF_OPTS(bpf_object_open_opts, opts,
-			    .attach_prog_fd = pkt_fd,
+			    .attach_prog_fd = tgt_fd,
 			   );
 
 	link = calloc(sizeof(struct bpf_link *), prog_cnt);
 	prog = calloc(sizeof(struct bpf_program *), prog_cnt);
-	result = malloc((prog_cnt + 32 /* spare */) * sizeof(__u64));
-	if (CHECK(!link || !prog || !result, "alloc_memory",
-		  "failed to alloc memory"))
+	if (CHECK(!link || !prog, "alloc_memory", "failed to alloc memory"))
 		goto close_prog;
 
 	obj = bpf_object__open_file(obj_file, &opts);

@@ -53,39 +96,33 @@ static void test_fexit_bpf2bpf_common(const char *obj_file,
 			goto close_prog;
 	}
 
-	if (!run_prog)
-		goto close_prog;
+	if (cb) {
+		err = cb(obj);
+		if (err)
+			goto close_prog;
+	}
 
-	data_map = bpf_object__find_map_by_name(obj, "fexit_bp.bss");
-	if (CHECK(!data_map, "find_data_map", "data map not found\n"))
+	if (!run_prog)
 		goto close_prog;
 
-	err = bpf_prog_test_run(pkt_fd, 1, &pkt_v6, sizeof(pkt_v6),
+	err = bpf_prog_test_run(tgt_fd, 1, &pkt_v6, sizeof(pkt_v6),
 				NULL, NULL, &retval, &duration);
 	CHECK(err || retval, "ipv6",
 	      "err %d errno %d retval %d duration %d\n",
 	      err, errno, retval, duration);
 
-	err = bpf_map_lookup_elem(bpf_map__fd(data_map), &zero, result);
-	if (CHECK(err, "get_result",
-		  "failed to get output data: %d\n", err))
+	if (check_data_map(obj, prog_cnt, false))
 		goto close_prog;
 
-	for (i = 0; i < prog_cnt; i++)
-		if (CHECK(result[i] != 1, "result", "fexit_bpf2bpf failed err %llu\n",
-			  result[i]))
-			goto close_prog;
-
 close_prog:
 	for (i = 0; i < prog_cnt; i++)
 		if (!IS_ERR_OR_NULL(link[i]))
 			bpf_link__destroy(link[i]);
 	if (!IS_ERR_OR_NULL(obj))
 		bpf_object__close(obj);
-	bpf_object__close(pkt_obj);
+	bpf_object__close(tgt_obj);
 	free(link);
 	free(prog);
-	free(result);
 }
 
 static void test_target_no_callees(void)

@@ -96,7 +133,7 @@ static void test_target_no_callees(void)
 	test_fexit_bpf2bpf_common("./fexit_bpf2bpf_simple.o",
 				  "./test_pkt_md_access.o",
 				  ARRAY_SIZE(prog_name),
-				  prog_name, true);
+				  prog_name, true, NULL);
 }
 
 static void test_target_yes_callees(void)

@@ -110,7 +147,7 @@ static void test_target_yes_callees(void)
 	test_fexit_bpf2bpf_common("./fexit_bpf2bpf.o",
 				  "./test_pkt_access.o",
 				  ARRAY_SIZE(prog_name),
-				  prog_name, true);
+				  prog_name, true, NULL);
 }
 
 static void test_func_replace(void)

@@ -128,7 +165,7 @@ static void test_func_replace(void)
 	test_fexit_bpf2bpf_common("./fexit_bpf2bpf.o",
 				  "./test_pkt_access.o",
 				  ARRAY_SIZE(prog_name),
-				  prog_name, true);
+				  prog_name, true, NULL);
 }
 
 static void test_func_replace_verify(void)

@@ -139,7 +176,75 @@ static void test_func_replace_verify(void)
 	test_fexit_bpf2bpf_common("./freplace_connect4.o",
 				  "./connect4_prog.o",
 				  ARRAY_SIZE(prog_name),
-				  prog_name, false);
+				  prog_name, false, NULL);
+}
+
+static int test_second_attach(struct bpf_object *obj)
+{
+	const char *prog_name = "freplace/get_constant";
+	const char *tgt_name = prog_name + 9; /* cut off freplace/ */
+	const char *tgt_obj_file = "./test_pkt_access.o";
+	int err = 0, tgt_fd, tgt_btf_id, link_fd = -1;
+	struct bpf_program *prog = NULL;
+	struct bpf_object *tgt_obj;
+	__u32 duration = 0, retval;
+	struct btf *btf;
+
+	prog = bpf_object__find_program_by_title(obj, prog_name);
+	if (CHECK(!prog, "find_prog", "prog %s not found\n", prog_name))
+		return -ENOENT;
+
+	err = bpf_prog_load(tgt_obj_file, BPF_PROG_TYPE_UNSPEC,
+			    &tgt_obj, &tgt_fd);
+	if (CHECK(err, "second_prog_load", "file %s err %d errno %d\n",
+		  tgt_obj_file, err, errno))
+		return err;
+
+	btf = bpf_object__btf(tgt_obj);
+	tgt_btf_id = btf__find_by_name_kind(btf, tgt_name, BTF_KIND_FUNC);
+	if (CHECK(tgt_btf_id < 0, "find_btf", "no BTF ID found for func %s\n", prog_name)) {
+		err = -ENOENT;
+		goto out;
+	}
+
+	DECLARE_LIBBPF_OPTS(bpf_raw_tracepoint_opts, opts,
+			    .tgt_prog_fd = tgt_fd,
+			    .tgt_btf_id = tgt_btf_id,
+			   );
+	link_fd = bpf_raw_tracepoint_open_opts(NULL, bpf_program__fd(prog), &opts);
+	if (CHECK(link_fd < 0, "second_link", "err %d errno %d",
+		  link_fd, errno)) {
+		err = link_fd;
+		goto out;
+	}
+
+	err = bpf_prog_test_run(tgt_fd, 1, &pkt_v6, sizeof(pkt_v6),
+				NULL, NULL, &retval, &duration);
+	if (CHECK(err || retval, "ipv6",
+		  "err %d errno %d retval %d duration %d\n",
+		  err, errno, retval, duration))
+		goto out;
+
+	err = check_data_map(obj, 1, true);
+	if (err)
+		goto out;
+
+out:
+	if (link_fd >= 0)
+		close(link_fd);
+	bpf_object__close(tgt_obj);
+	return err;
+}
+
+static void test_func_replace_multi(void)
+{
+	const char *prog_name[] = {
+		"freplace/get_constant",
+	};
+	test_fexit_bpf2bpf_common("./freplace_get_constant.o",
+				  "./test_pkt_access.o",
+				  ARRAY_SIZE(prog_name),
+				  prog_name, true, test_second_attach);
 }
 
 static void test_func_sockmap_update(void)

@@ -150,7 +255,7 @@ static void test_func_sockmap_update(void)
 	test_fexit_bpf2bpf_common("./freplace_cls_redirect.o",
 				  "./test_cls_redirect.o",
 				  ARRAY_SIZE(prog_name),
-				  prog_name, false);
+				  prog_name, false, NULL);
 }
 
 static void test_obj_load_failure_common(const char *obj_file,

@@ -222,4 +327,6 @@ void test_fexit_bpf2bpf(void)
 		test_func_replace_return_code();
 	if (test__start_subtest("func_map_prog_compatibility"))
 		test_func_map_prog_compatibility();
+	if (test__start_subtest("func_replace_multi"))
+		test_func_replace_multi();
 }

diff --git a/tools/testing/selftests/bpf/progs/freplace_get_constant.c b/tools/testing/selftests/bpf/progs/freplace_get_constant.c
new file mode 100644
index 000000000000..8f0ecf94e533
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/freplace_get_constant.c

@@ -0,0 +1,15 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_endian.h>
+
+volatile __u64 test_get_constant = 0;
+SEC("freplace/get_constant")
+int new_get_constant(long val)
+{
+	if (val != 123)
+		return 0;
+	test_get_constant = 1;
+	return test_get_constant; /* original get_constant() returns val - 122 */
+}
+char _license[] SEC("license") = "GPL";

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help