[PATCH bpf-next v2 0/6] bpf: Support multi-attach for freplace programs

[PATCH bpf-next v2 0/6] bpf: Support multi-attach for freplace programs

[Toke Høiland-Jørgensen]

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 two 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 contains the change that actually allows attaching freplace programs in
multiple places. Patches 4-6 are the usual tools header updates, libbpf support
and selftest.

See the individual patches for details.

Changelog:

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

---

Toke Høiland-Jørgensen (6):
      bpf: change logging calls from verbose() to bpf_log() and use log pointer
      bpf: verifier: refactor check_attach_btf_id()
      bpf: support attaching freplace programs to multiple attach points
      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                           |  23 ++-
 include/linux/bpf_verifier.h                  |   9 +
 include/uapi/linux/bpf.h                      |   6 +-
 kernel/bpf/core.c                             |   1 -
 kernel/bpf/syscall.c                          |  78 +++++++-
 kernel/bpf/trampoline.c                       |  36 +++-
 kernel/bpf/verifier.c                         | 171 ++++++++++--------
 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  | 164 ++++++++++++++---
 .../bpf/progs/freplace_get_constant.c         |  15 ++
 13 files changed, 404 insertions(+), 128 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/progs/freplace_get_constant.c

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

[Toke Høiland-Jørgensen]

From: Toke Høiland-Jørgensen <toke@redhat.com>

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 <toke@redhat.com>
---
 include/linux/bpf.h          |    2 +-
 include/linux/bpf_verifier.h |    2 +-
 kernel/bpf/btf.c             |    6 +++--
 kernel/bpf/verifier.c        |   46 +++++++++++++++++++++---------------------
 4 files changed, 28 insertions(+), 28 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index c67c88ad35f8..154aab467728 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1314,7 +1314,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..f1ee7468ec28 100644
--- a/include/linux/bpf_verifier.h
+++ b/include/linux/bpf_verifier.h
@@ -347,7 +347,7 @@ 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)) ||
+	return (log && log->level && log->ubuf && !bpf_verifier_log_full(log)) ||
 		log->level == BPF_LOG_KERNEL;
 }
 
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 03d6d43bb1d6..e9122e260950 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -4285,7 +4285,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;
@@ -4293,7 +4293,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;
 	}
 
@@ -4305,7 +4305,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 3c1efc9d08fd..6f35b4924b4c 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -10736,6 +10736,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;
@@ -10757,23 +10758,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) {
@@ -10785,18 +10786,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;
 			}
@@ -10804,7 +10805,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) {
@@ -10812,7 +10813,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 &&
@@ -10834,13 +10835,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;
@@ -10849,17 +10850,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;
 		}
@@ -10882,7 +10883,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;
 		}
@@ -10893,8 +10894,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)
@@ -10906,18 +10906,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))
@@ -10936,7 +10936,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;
@@ -10948,7 +10948,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;
@@ -10959,7 +10959,7 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)
 		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);
 		}
 

[PATCH bpf-next v2 2/6] bpf: verifier: refactor check_attach_btf_id()

[Toke Høiland-Jørgensen]

From: Toke Høiland-Jørgensen <toke@redhat.com>

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 <toke@redhat.com>
---
 include/linux/bpf.h          |    9 ++
 include/linux/bpf_verifier.h |    9 ++
 kernel/bpf/trampoline.c      |   22 ++++++
 kernel/bpf/verifier.c        |  167 ++++++++++++++++++++++++------------------
 4 files changed, 134 insertions(+), 73 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 154aab467728..ca3a2a1812c2 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -570,6 +570,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),		\
@@ -626,6 +629,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 f1ee7468ec28..2934255f936a 100644
--- a/include/linux/bpf_verifier.h
+++ b/include/linux/bpf_verifier.h
@@ -446,4 +446,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 9be85aa4ec5f..fadfa330f728 100644
--- a/kernel/bpf/trampoline.c
+++ b/kernel/bpf/trampoline.c
@@ -331,6 +331,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 6f35b4924b4c..a1ab7298f53b 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -10731,31 +10731,23 @@ static int check_attach_modify_return(struct bpf_prog *prog, unsigned long addr)
 	return -EINVAL;
 }
 
-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->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");
@@ -10801,8 +10793,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");
@@ -10838,13 +10828,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) {
@@ -10874,13 +10862,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",
@@ -10890,12 +10872,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;
@@ -10904,13 +10884,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);
@@ -10922,24 +10895,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;
@@ -10951,27 +10914,85 @@ 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->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);
-		}
+	*tgt_addr = addr;
+	if (tgt_name)
+		*tgt_name = tname;
+	if (tgt_type)
+		*tgt_type = t;
+	return 0;
+}
 
-		if (ret)
-			goto out;
-		tr->func.addr = (void *)addr;
-		prog->aux->trampoline = tr;
-out:
-		mutex_unlock(&tr->mutex);
-		if (ret)
-			bpf_trampoline_put(tr);
+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->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);
 	}
 }
 

[PATCH bpf-next v2 3/6] bpf: support attaching freplace programs to multiple attach points

[Toke Høiland-Jørgensen]

From: Toke Høiland-Jørgensen <toke@redhat.com>

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() in the previous patch. It also moves the BPF
trampoline out of prog->aux and into struct bpf_tracking_link where it is
managed as part of the bpf_link structure. When a new target is specified,
a reference to the target program is also kept in the bpf_link structure
which removes the need to keep references to all attach targets in the
bpf_prog structure itself.

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 <toke@redhat.com>
---
 include/linux/bpf.h      |   14 +++++---
 include/uapi/linux/bpf.h |    6 ++--
 kernel/bpf/core.c        |    1 -
 kernel/bpf/syscall.c     |   78 ++++++++++++++++++++++++++++++++++++++++++----
 kernel/bpf/trampoline.c  |   14 ++++----
 kernel/bpf/verifier.c    |   16 ++++++---
 6 files changed, 100 insertions(+), 29 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index ca3a2a1812c2..ca7a710cdb57 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -568,8 +568,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);
@@ -621,11 +621,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;
 }
@@ -697,10 +699,12 @@ struct bpf_prog_aux {
 	bool attach_btf_trace; /* true if attaching to BTF-enabled raw tp */
 	bool func_proto_unreliable;
 	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;
+	/* 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 5e386389913a..01a0814a8cfe 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -574,8 +574,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/core.c b/kernel/bpf/core.c
index 9df4cc9a2907..ed4d7259316a 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -2087,7 +2087,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) {
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 7ea9dfbebd8c..2301d7485b4d 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>
@@ -2484,11 +2485,21 @@ struct bpf_link *bpf_link_get_from_fd(u32 ufd)
 struct bpf_tracing_link {
 	struct bpf_link link;
 	enum bpf_attach_type attach_type;
+	struct bpf_trampoline *trampoline;
+	struct bpf_prog *tgt_prog;
 };
 
 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);
+
+	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)
@@ -2528,10 +2539,17 @@ static const struct bpf_link_ops bpf_tracing_link_lops = {
 	.fill_link_info = bpf_tracing_link_fill_link_info,
 };
 
-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_link_primer link_primer;
+	struct bpf_trampoline *tr = NULL;
+	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) {
@@ -2560,6 +2578,43 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog)
 		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;
+		}
+		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;
+		}
+
+	} else if (btf_id) {
+		err = -EINVAL;
+		goto out_put_prog;
+	} else {
+		btf_id = prog->aux->attach_btf_id;
+		tgt_prog = prog->aux->linked_prog;
+		if (tgt_prog)
+			bpf_prog_inc(tgt_prog); /* we call bpf_prog_put() on link release */
+	}
+	err = bpf_check_attach_target(NULL, prog, tgt_prog, btf_id,
+				      &fmodel, &addr, NULL, NULL);
+	if (err)
+		goto out_put_prog;
+
+	if (tgt_prog)
+		key = ((u64)tgt_prog->aux->id) << 32 | btf_id;
+	else
+		key = btf_id;
+
+	err = bpf_trampoline_get(key, (void *)addr, &fmodel, &tr);
+	if (err)
+		goto out_put_prog;
+
 	link = kzalloc(sizeof(*link), GFP_USER);
 	if (!link) {
 		err = -ENOMEM;
@@ -2575,15 +2630,21 @@ 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, tr);
 	if (err) {
 		bpf_link_cleanup(&link_primer);
 		goto out_put_prog;
 	}
+	link->trampoline = tr;
+	link->tgt_prog = tgt_prog;
 
 	return bpf_link_settle(&link_primer);
 out_put_prog:
 	bpf_prog_put(prog);
+	if (tgt_prog)
+		bpf_prog_put(tgt_prog);
+	if (tr)
+		bpf_trampoline_put(tr);
 	return err;
 }
 
@@ -2661,7 +2722,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)
 {
@@ -2685,8 +2746,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;
@@ -2696,7 +2758,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/trampoline.c b/kernel/bpf/trampoline.c
index fadfa330f728..40797405f1a0 100644
--- a/kernel/bpf/trampoline.c
+++ b/kernel/bpf/trampoline.c
@@ -256,14 +256,13 @@ 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) {
@@ -296,7 +295,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]--;
@@ -307,13 +306,12 @@ 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) {
@@ -325,7 +323,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 a1ab7298f53b..1222031fc2e9 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -10896,6 +10896,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;
 
@@ -10938,7 +10944,6 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)
 	const char *tname;
 	long addr;
 	int ret;
-	u64 key;
 
 	if (prog->type == BPF_PROG_TYPE_STRUCT_OPS)
 		return check_struct_ops_btf_id(env);
@@ -10954,13 +10959,13 @@ 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 = tgt_prog->expected_attach_type;
 		}
-		key = ((u64)tgt_prog->aux->id) << 32 | btf_id;
-	} else {
-		key = btf_id;
 	}
 
 	prog->aux->attach_func_proto = t;
@@ -10991,8 +10996,7 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)
 			if (ret < 0)
 				return ret;
 		}
-		return bpf_trampoline_get(key, (void *)addr, &fmodel,
-					  &prog->aux->trampoline);
+		return 0;
 	}
 }
 

[PATCH bpf-next v2 4/6] tools: add new members to bpf_attr.raw_tracepoint in bpf.h

[Toke Høiland-Jørgensen]

From: Toke Høiland-Jørgensen <toke@redhat.com>

Sync addition of new members from main kernel tree.

Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
 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 5e386389913a..01a0814a8cfe 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -574,8 +574,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 bpf-next v2 5/6] libbpf: add support for supplying target to bpf_raw_tracepoint_open()

[Toke Høiland-Jørgensen]

From: Toke Høiland-Jørgensen <toke@redhat.com>

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 <toke@redhat.com>
---
 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 a7329b671c41..030c57f8d7c9 100644
--- a/tools/lib/bpf/bpf.c
+++ b/tools/lib/bpf/bpf.c
@@ -793,17 +793,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 dbef24ebcfcb..bae0003899f4 100644
--- a/tools/lib/bpf/bpf.h
+++ b/tools/lib/bpf/bpf.h
@@ -227,7 +227,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 c5d5c7664c3b..17320a5721a5 100644
--- a/tools/lib/bpf/libbpf.map
+++ b/tools/lib/bpf/libbpf.map
@@ -288,5 +288,6 @@ LIBBPF_0.1.0 {
 		bpf_map__value_size;
 		bpf_program__autoload;
 		bpf_program__set_autoload;
+		bpf_raw_tracepoint_open_opts;
 		btf__set_fd;
 } LIBBPF_0.0.9;

[PATCH bpf-next v2 6/6] selftests: add test for multiple attachments of freplace program

[Toke Høiland-Jørgensen]

From: Toke Høiland-Jørgensen <toke@redhat.com>

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

Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
 .../selftests/bpf/prog_tests/fexit_bpf2bpf.c       |  164 ++++++++++++++++----
 .../selftests/bpf/progs/freplace_get_constant.c    |   15 ++
 2 files changed, 150 insertions(+), 29 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 a895bfed55db..feb3e11c5445 100644
--- a/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c
+++ b/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c
@@ -2,35 +2,78 @@
 /* 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, *map;
+	const int zero = 0;
+	u64 *result = NULL;
+	__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 failed err %ld\n",
+			  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_object *obj = NULL, *tgt_obj;
+	int err, tgt_fd, i;
 	struct bpf_link **link = NULL;
 	struct bpf_program **prog = NULL;
 	__u32 duration = 0, retval;
-	struct bpf_map *data_map;
-	const int zero = 0;
-	u64 *result = NULL;
 
 	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",
+	if (CHECK(!link || !prog, "alloc_memory",
 		  "failed to alloc memory"))
 		goto close_prog;
 
@@ -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 %ld\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)
@@ -127,7 +164,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)
@@ -138,7 +175,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);
 }
 
 void test_fexit_bpf2bpf(void)
@@ -147,4 +252,5 @@ void test_fexit_bpf2bpf(void)
 	test_target_yes_callees();
 	test_func_replace();
 	test_func_replace_verify();
+	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";

Re: [PATCH bpf-next v2 3/6] bpf: support attaching freplace programs to multiple attach points

[Alexei Starovoitov]

On Wed, Jul 15, 2020 at 03:09:02PM +0200, Toke Høiland-Jørgensen wrote:
>  
> +	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;
> +		}
> +		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;
> +		}
> +
> +	} else if (btf_id) {
> +		err = -EINVAL;
> +		goto out_put_prog;
> +	} else {
> +		btf_id = prog->aux->attach_btf_id;
> +		tgt_prog = prog->aux->linked_prog;
> +		if (tgt_prog)
> +			bpf_prog_inc(tgt_prog); /* we call bpf_prog_put() on link release */

so the first prog_load cmd will beholding the first target prog?
This is complete non starter.
You didn't mention such decision anywhere.
The first ext prog will attach to the first dispatcher xdp prog,
then that ext prog will multi attach to second dispatcher xdp prog and
the first dispatcher prog will live in the kernel forever.
That's not what we discussed back in April.

> +	}
> +	err = bpf_check_attach_target(NULL, prog, tgt_prog, btf_id,
> +				      &fmodel, &addr, NULL, NULL);

This is a second check for btf id match?
What's the point? The first one was done at load time.
When tgt_prog_fd/tgt_btf_id are zero there is no need to recheck.

I really hope I'm misreading these patches, because they look very raw.

Re: [PATCH bpf-next v2 1/6] bpf: change logging calls from verbose() to bpf_log() and use log pointer

[Dan Carpenter]

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

Hi Toke,

url:    https://github.com/0day-ci/linux/commits/Toke-H-iland-J-rgensen/bpf-Support-multi-attach-for-freplace-programs/20200715-211145
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
config: x86_64-randconfig-m001-20200715 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-14) 9.3.0

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

smatch warnings:
include/linux/bpf_verifier.h:351 bpf_verifier_log_needed() error: we previously assumed 'log' could be null (see line 350)
include/linux/bpf_verifier.h:351 bpf_verifier_log_needed() error: we previously assumed 'log' could be null (see line 350)

# https://github.com/0day-ci/linux/commit/e33243ff1dd2cbb3628e4044dd9e00a6abf99c7e
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout e33243ff1dd2cbb3628e4044dd9e00a6abf99c7e
vim +/log +351 include/linux/bpf_verifier.h

06ee7115b0d174 Alexei Starovoitov     2019-04-01  347  
77d2e05abd4588 Martin KaFai Lau       2018-03-24  348  static inline bool bpf_verifier_log_needed(const struct bpf_verifier_log *log)
77d2e05abd4588 Martin KaFai Lau       2018-03-24  349  {
e33243ff1dd2cb Toke Høiland-Jørgensen 2020-07-15 @350  	return (log && log->level && log->ubuf && !bpf_verifier_log_full(log)) ||
                                                                ^^^
If "log" is NULL

8580ac9404f624 Alexei Starovoitov     2019-10-15 @351  		log->level == BPF_LOG_KERNEL;
                                                                ^^^^^^^^^^
then we are toasted.

77d2e05abd4588 Martin KaFai Lau       2018-03-24  352  }
77d2e05abd4588 Martin KaFai Lau       2018-03-24  353  

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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 37112 bytes --]

Re: [PATCH bpf-next v2 3/6] bpf: support attaching freplace programs to multiple attach points

[Dan Carpenter]

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

Hi Toke,

url:    https://github.com/0day-ci/linux/commits/Toke-H-iland-J-rgensen/bpf-Support-multi-attach-for-freplace-programs/20200715-211145
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
config: x86_64-randconfig-m001-20200715 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-14) 9.3.0

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

New smatch warnings:
kernel/bpf/verifier.c:10900 bpf_check_attach_target() error: we previously assumed 'tgt_prog' could be null (see line 10772)

Old smatch warnings:
include/linux/bpf_verifier.h:351 bpf_verifier_log_needed() error: we previously assumed 'log' could be null (see line 350)

# https://github.com/0day-ci/linux/commit/cc8571ec751a3a6065838e0b15105f8be0ced6fe
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout cc8571ec751a3a6065838e0b15105f8be0ced6fe
vim +/tgt_prog +10900 kernel/bpf/verifier.c

c2d0f6ffe7709e Toke Høiland-Jørgensen 2020-07-15  10734  int bpf_check_attach_target(struct bpf_verifier_log *log,
c2d0f6ffe7709e Toke Høiland-Jørgensen 2020-07-15  10735  			    const struct bpf_prog *prog,
c2d0f6ffe7709e Toke Høiland-Jørgensen 2020-07-15  10736  			    const struct bpf_prog *tgt_prog,
c2d0f6ffe7709e Toke Høiland-Jørgensen 2020-07-15  10737  			    u32 btf_id,
c2d0f6ffe7709e Toke Høiland-Jørgensen 2020-07-15  10738  			    struct btf_func_model *fmodel,
c2d0f6ffe7709e Toke Høiland-Jørgensen 2020-07-15  10739  			    long *tgt_addr,
c2d0f6ffe7709e Toke Høiland-Jørgensen 2020-07-15  10740  			    const char **tgt_name,
c2d0f6ffe7709e Toke Høiland-Jørgensen 2020-07-15  10741  			    const struct btf_type **tgt_type)
38207291604401 Martin KaFai Lau       2019-10-24  10742  {
be8704ff07d237 Alexei Starovoitov     2020-01-20  10743  	bool prog_extension = prog->type == BPF_PROG_TYPE_EXT;
f1b9509c2fb0ef Alexei Starovoitov     2019-10-30  10744  	const char prefix[] = "btf_trace_";
5b92a28aae4dd0 Alexei Starovoitov     2019-11-14  10745  	int ret = 0, subprog = -1, i;
38207291604401 Martin KaFai Lau       2019-10-24  10746  	const struct btf_type *t;
5b92a28aae4dd0 Alexei Starovoitov     2019-11-14  10747  	bool conservative = true;
38207291604401 Martin KaFai Lau       2019-10-24  10748  	const char *tname;
5b92a28aae4dd0 Alexei Starovoitov     2019-11-14  10749  	struct btf *btf;
c2d0f6ffe7709e Toke Høiland-Jørgensen 2020-07-15  10750  	long addr = 0;
38207291604401 Martin KaFai Lau       2019-10-24  10751  
f1b9509c2fb0ef Alexei Starovoitov     2019-10-30  10752  	if (!btf_id) {
e33243ff1dd2cb Toke Høiland-Jørgensen 2020-07-15  10753  		bpf_log(log, "Tracing programs must provide btf_id\n");
f1b9509c2fb0ef Alexei Starovoitov     2019-10-30  10754  		return -EINVAL;
f1b9509c2fb0ef Alexei Starovoitov     2019-10-30  10755  	}
5b92a28aae4dd0 Alexei Starovoitov     2019-11-14  10756  	btf = bpf_prog_get_target_btf(prog);
5b92a28aae4dd0 Alexei Starovoitov     2019-11-14  10757  	if (!btf) {
e33243ff1dd2cb Toke Høiland-Jørgensen 2020-07-15  10758  		bpf_log(log,
5b92a28aae4dd0 Alexei Starovoitov     2019-11-14  10759  			"FENTRY/FEXIT program can only be attached to another program annotated with BTF\n");
5b92a28aae4dd0 Alexei Starovoitov     2019-11-14  10760  		return -EINVAL;
5b92a28aae4dd0 Alexei Starovoitov     2019-11-14  10761  	}
5b92a28aae4dd0 Alexei Starovoitov     2019-11-14  10762  	t = btf_type_by_id(btf, btf_id);
38207291604401 Martin KaFai Lau       2019-10-24  10763  	if (!t) {
e33243ff1dd2cb Toke Høiland-Jørgensen 2020-07-15  10764  		bpf_log(log, "attach_btf_id %u is invalid\n", btf_id);
38207291604401 Martin KaFai Lau       2019-10-24  10765  		return -EINVAL;
38207291604401 Martin KaFai Lau       2019-10-24  10766  	}
5b92a28aae4dd0 Alexei Starovoitov     2019-11-14  10767  	tname = btf_name_by_offset(btf, t->name_off);
f1b9509c2fb0ef Alexei Starovoitov     2019-10-30  10768  	if (!tname) {
e33243ff1dd2cb Toke Høiland-Jørgensen 2020-07-15  10769  		bpf_log(log, "attach_btf_id %u doesn't have a name\n", btf_id);
f1b9509c2fb0ef Alexei Starovoitov     2019-10-30  10770  		return -EINVAL;
f1b9509c2fb0ef Alexei Starovoitov     2019-10-30  10771  	}
5b92a28aae4dd0 Alexei Starovoitov     2019-11-14 @10772  	if (tgt_prog) {
5b92a28aae4dd0 Alexei Starovoitov     2019-11-14  10773  		struct bpf_prog_aux *aux = tgt_prog->aux;
5b92a28aae4dd0 Alexei Starovoitov     2019-11-14  10774  
5b92a28aae4dd0 Alexei Starovoitov     2019-11-14  10775  		for (i = 0; i < aux->func_info_cnt; i++)
5b92a28aae4dd0 Alexei Starovoitov     2019-11-14  10776  			if (aux->func_info[i].type_id == btf_id) {
5b92a28aae4dd0 Alexei Starovoitov     2019-11-14  10777  				subprog = i;
5b92a28aae4dd0 Alexei Starovoitov     2019-11-14  10778  				break;
5b92a28aae4dd0 Alexei Starovoitov     2019-11-14  10779  			}
5b92a28aae4dd0 Alexei Starovoitov     2019-11-14  10780  		if (subprog == -1) {
e33243ff1dd2cb Toke Høiland-Jørgensen 2020-07-15  10781  			bpf_log(log, "Subprog %s doesn't exist\n", tname);
5b92a28aae4dd0 Alexei Starovoitov     2019-11-14  10782  			return -EINVAL;
5b92a28aae4dd0 Alexei Starovoitov     2019-11-14  10783  		}
5b92a28aae4dd0 Alexei Starovoitov     2019-11-14  10784  		conservative = aux->func_info_aux[subprog].unreliable;
be8704ff07d237 Alexei Starovoitov     2020-01-20  10785  		if (prog_extension) {
be8704ff07d237 Alexei Starovoitov     2020-01-20  10786  			if (conservative) {
e33243ff1dd2cb Toke Høiland-Jørgensen 2020-07-15  10787  				bpf_log(log,
be8704ff07d237 Alexei Starovoitov     2020-01-20  10788  					"Cannot replace static functions\n");
be8704ff07d237 Alexei Starovoitov     2020-01-20  10789  				return -EINVAL;
be8704ff07d237 Alexei Starovoitov     2020-01-20  10790  			}
be8704ff07d237 Alexei Starovoitov     2020-01-20  10791  			if (!prog->jit_requested) {
e33243ff1dd2cb Toke Høiland-Jørgensen 2020-07-15  10792  				bpf_log(log,
be8704ff07d237 Alexei Starovoitov     2020-01-20  10793  					"Extension programs should be JITed\n");
be8704ff07d237 Alexei Starovoitov     2020-01-20  10794  				return -EINVAL;
be8704ff07d237 Alexei Starovoitov     2020-01-20  10795  			}
be8704ff07d237 Alexei Starovoitov     2020-01-20  10796  		}
be8704ff07d237 Alexei Starovoitov     2020-01-20  10797  		if (!tgt_prog->jited) {
e33243ff1dd2cb Toke Høiland-Jørgensen 2020-07-15  10798  			bpf_log(log, "Can attach to only JITed progs\n");
be8704ff07d237 Alexei Starovoitov     2020-01-20  10799  			return -EINVAL;
be8704ff07d237 Alexei Starovoitov     2020-01-20  10800  		}
be8704ff07d237 Alexei Starovoitov     2020-01-20  10801  		if (tgt_prog->type == prog->type) {
be8704ff07d237 Alexei Starovoitov     2020-01-20  10802  			/* Cannot fentry/fexit another fentry/fexit program.
be8704ff07d237 Alexei Starovoitov     2020-01-20  10803  			 * Cannot attach program extension to another extension.
be8704ff07d237 Alexei Starovoitov     2020-01-20  10804  			 * It's ok to attach fentry/fexit to extension program.
be8704ff07d237 Alexei Starovoitov     2020-01-20  10805  			 */
e33243ff1dd2cb Toke Høiland-Jørgensen 2020-07-15  10806  			bpf_log(log, "Cannot recursively attach\n");
be8704ff07d237 Alexei Starovoitov     2020-01-20  10807  			return -EINVAL;
be8704ff07d237 Alexei Starovoitov     2020-01-20  10808  		}
be8704ff07d237 Alexei Starovoitov     2020-01-20  10809  		if (tgt_prog->type == BPF_PROG_TYPE_TRACING &&
be8704ff07d237 Alexei Starovoitov     2020-01-20  10810  		    prog_extension &&
be8704ff07d237 Alexei Starovoitov     2020-01-20  10811  		    (tgt_prog->expected_attach_type == BPF_TRACE_FENTRY ||
be8704ff07d237 Alexei Starovoitov     2020-01-20  10812  		     tgt_prog->expected_attach_type == BPF_TRACE_FEXIT)) {
be8704ff07d237 Alexei Starovoitov     2020-01-20  10813  			/* Program extensions can extend all program types
be8704ff07d237 Alexei Starovoitov     2020-01-20  10814  			 * except fentry/fexit. The reason is the following.
be8704ff07d237 Alexei Starovoitov     2020-01-20  10815  			 * The fentry/fexit programs are used for performance
be8704ff07d237 Alexei Starovoitov     2020-01-20  10816  			 * analysis, stats and can be attached to any program
be8704ff07d237 Alexei Starovoitov     2020-01-20  10817  			 * type except themselves. When extension program is
be8704ff07d237 Alexei Starovoitov     2020-01-20  10818  			 * replacing XDP function it is necessary to allow
be8704ff07d237 Alexei Starovoitov     2020-01-20  10819  			 * performance analysis of all functions. Both original
be8704ff07d237 Alexei Starovoitov     2020-01-20  10820  			 * XDP program and its program extension. Hence
be8704ff07d237 Alexei Starovoitov     2020-01-20  10821  			 * attaching fentry/fexit to BPF_PROG_TYPE_EXT is
be8704ff07d237 Alexei Starovoitov     2020-01-20  10822  			 * allowed. If extending of fentry/fexit was allowed it
be8704ff07d237 Alexei Starovoitov     2020-01-20  10823  			 * would be possible to create long call chain
be8704ff07d237 Alexei Starovoitov     2020-01-20  10824  			 * fentry->extension->fentry->extension beyond
be8704ff07d237 Alexei Starovoitov     2020-01-20  10825  			 * reasonable stack size. Hence extending fentry is not
be8704ff07d237 Alexei Starovoitov     2020-01-20  10826  			 * allowed.
be8704ff07d237 Alexei Starovoitov     2020-01-20  10827  			 */
e33243ff1dd2cb Toke Høiland-Jørgensen 2020-07-15  10828  			bpf_log(log, "Cannot extend fentry/fexit\n");
be8704ff07d237 Alexei Starovoitov     2020-01-20  10829  			return -EINVAL;
be8704ff07d237 Alexei Starovoitov     2020-01-20  10830  		}
5b92a28aae4dd0 Alexei Starovoitov     2019-11-14  10831  	} else {
be8704ff07d237 Alexei Starovoitov     2020-01-20  10832  		if (prog_extension) {
e33243ff1dd2cb Toke Høiland-Jørgensen 2020-07-15  10833  			bpf_log(log, "Cannot replace kernel functions\n");
be8704ff07d237 Alexei Starovoitov     2020-01-20  10834  			return -EINVAL;
be8704ff07d237 Alexei Starovoitov     2020-01-20  10835  		}
5b92a28aae4dd0 Alexei Starovoitov     2019-11-14  10836  	}
f1b9509c2fb0ef Alexei Starovoitov     2019-10-30  10837  
f1b9509c2fb0ef Alexei Starovoitov     2019-10-30  10838  	switch (prog->expected_attach_type) {
f1b9509c2fb0ef Alexei Starovoitov     2019-10-30  10839  	case BPF_TRACE_RAW_TP:
5b92a28aae4dd0 Alexei Starovoitov     2019-11-14  10840  		if (tgt_prog) {
e33243ff1dd2cb Toke Høiland-Jørgensen 2020-07-15  10841  			bpf_log(log,
5b92a28aae4dd0 Alexei Starovoitov     2019-11-14  10842  				"Only FENTRY/FEXIT progs are attachable to another BPF prog\n");
5b92a28aae4dd0 Alexei Starovoitov     2019-11-14  10843  			return -EINVAL;
5b92a28aae4dd0 Alexei Starovoitov     2019-11-14  10844  		}
38207291604401 Martin KaFai Lau       2019-10-24  10845  		if (!btf_type_is_typedef(t)) {
e33243ff1dd2cb Toke Høiland-Jørgensen 2020-07-15  10846  			bpf_log(log, "attach_btf_id %u is not a typedef\n",
38207291604401 Martin KaFai Lau       2019-10-24  10847  				btf_id);
38207291604401 Martin KaFai Lau       2019-10-24  10848  			return -EINVAL;
38207291604401 Martin KaFai Lau       2019-10-24  10849  		}
f1b9509c2fb0ef Alexei Starovoitov     2019-10-30  10850  		if (strncmp(prefix, tname, sizeof(prefix) - 1)) {
e33243ff1dd2cb Toke Høiland-Jørgensen 2020-07-15  10851  			bpf_log(log, "attach_btf_id %u points to wrong type name %s\n",
38207291604401 Martin KaFai Lau       2019-10-24  10852  				btf_id, tname);
38207291604401 Martin KaFai Lau       2019-10-24  10853  			return -EINVAL;
38207291604401 Martin KaFai Lau       2019-10-24  10854  		}
38207291604401 Martin KaFai Lau       2019-10-24  10855  		tname += sizeof(prefix) - 1;
5b92a28aae4dd0 Alexei Starovoitov     2019-11-14  10856  		t = btf_type_by_id(btf, t->type);
38207291604401 Martin KaFai Lau       2019-10-24  10857  		if (!btf_type_is_ptr(t))
38207291604401 Martin KaFai Lau       2019-10-24  10858  			/* should never happen in valid vmlinux build */
38207291604401 Martin KaFai Lau       2019-10-24  10859  			return -EINVAL;
5b92a28aae4dd0 Alexei Starovoitov     2019-11-14  10860  		t = btf_type_by_id(btf, t->type);
38207291604401 Martin KaFai Lau       2019-10-24  10861  		if (!btf_type_is_func_proto(t))
38207291604401 Martin KaFai Lau       2019-10-24  10862  			/* should never happen in valid vmlinux build */
38207291604401 Martin KaFai Lau       2019-10-24  10863  			return -EINVAL;
38207291604401 Martin KaFai Lau       2019-10-24  10864  
c2d0f6ffe7709e Toke Høiland-Jørgensen 2020-07-15  10865  		break;
15d83c4d7cef5c Yonghong Song          2020-05-09  10866  	case BPF_TRACE_ITER:
15d83c4d7cef5c Yonghong Song          2020-05-09  10867  		if (!btf_type_is_func(t)) {
e33243ff1dd2cb Toke Høiland-Jørgensen 2020-07-15  10868  			bpf_log(log, "attach_btf_id %u is not a function\n",
15d83c4d7cef5c Yonghong Song          2020-05-09  10869  				btf_id);
15d83c4d7cef5c Yonghong Song          2020-05-09  10870  			return -EINVAL;
15d83c4d7cef5c Yonghong Song          2020-05-09  10871  		}
15d83c4d7cef5c Yonghong Song          2020-05-09  10872  		t = btf_type_by_id(btf, t->type);
15d83c4d7cef5c Yonghong Song          2020-05-09  10873  		if (!btf_type_is_func_proto(t))
15d83c4d7cef5c Yonghong Song          2020-05-09  10874  			return -EINVAL;
c2d0f6ffe7709e Toke Høiland-Jørgensen 2020-07-15  10875  		ret = btf_distill_func_proto(log, btf, t, tname, fmodel);
c2d0f6ffe7709e Toke Høiland-Jørgensen 2020-07-15  10876  		if (ret)
15d83c4d7cef5c Yonghong Song          2020-05-09  10877  			return ret;
c2d0f6ffe7709e Toke Høiland-Jørgensen 2020-07-15  10878  		break;
be8704ff07d237 Alexei Starovoitov     2020-01-20  10879  	default:
be8704ff07d237 Alexei Starovoitov     2020-01-20  10880  		if (!prog_extension)
be8704ff07d237 Alexei Starovoitov     2020-01-20  10881  			return -EINVAL;
be8704ff07d237 Alexei Starovoitov     2020-01-20  10882  		/* fallthrough */
ae24082331d9bb KP Singh               2020-03-04  10883  	case BPF_MODIFY_RETURN:
9e4e01dfd3254c KP Singh               2020-03-29  10884  	case BPF_LSM_MAC:
fec56f5890d93f Alexei Starovoitov     2019-11-14  10885  	case BPF_TRACE_FENTRY:
fec56f5890d93f Alexei Starovoitov     2019-11-14  10886  	case BPF_TRACE_FEXIT:
fec56f5890d93f Alexei Starovoitov     2019-11-14  10887  		if (!btf_type_is_func(t)) {
e33243ff1dd2cb Toke Høiland-Jørgensen 2020-07-15  10888  			bpf_log(log, "attach_btf_id %u is not a function\n",
fec56f5890d93f Alexei Starovoitov     2019-11-14  10889  				btf_id);
fec56f5890d93f Alexei Starovoitov     2019-11-14  10890  			return -EINVAL;
fec56f5890d93f Alexei Starovoitov     2019-11-14  10891  		}
be8704ff07d237 Alexei Starovoitov     2020-01-20  10892  		if (prog_extension &&
e33243ff1dd2cb Toke Høiland-Jørgensen 2020-07-15  10893  		    btf_check_type_match(log, prog, btf, t))
be8704ff07d237 Alexei Starovoitov     2020-01-20  10894  			return -EINVAL;
5b92a28aae4dd0 Alexei Starovoitov     2019-11-14  10895  		t = btf_type_by_id(btf, t->type);
fec56f5890d93f Alexei Starovoitov     2019-11-14  10896  		if (!btf_type_is_func_proto(t))
fec56f5890d93f Alexei Starovoitov     2019-11-14  10897  			return -EINVAL;
c2d0f6ffe7709e Toke Høiland-Jørgensen 2020-07-15  10898  
cc8571ec751a3a Toke Høiland-Jørgensen 2020-07-15  10899  		if ((prog->aux->tgt_prog_type &&
cc8571ec751a3a Toke Høiland-Jørgensen 2020-07-15 @10900  		     prog->aux->tgt_prog_type != tgt_prog->type) ||
                                                                                                         ^^^^^^^^^^^^^^

cc8571ec751a3a Toke Høiland-Jørgensen 2020-07-15  10901  		    (prog->aux->tgt_attach_type &&
cc8571ec751a3a Toke Høiland-Jørgensen 2020-07-15  10902  		     prog->aux->tgt_attach_type != tgt_prog->expected_attach_type))
                                                                                                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Not checked.

cc8571ec751a3a Toke Høiland-Jørgensen 2020-07-15  10903  			return -EINVAL;
cc8571ec751a3a Toke Høiland-Jørgensen 2020-07-15  10904  
c2d0f6ffe7709e Toke Høiland-Jørgensen 2020-07-15  10905  		if (tgt_prog && conservative)
5b92a28aae4dd0 Alexei Starovoitov     2019-11-14  10906  			t = NULL;
c2d0f6ffe7709e Toke Høiland-Jørgensen 2020-07-15  10907  
c2d0f6ffe7709e Toke Høiland-Jørgensen 2020-07-15  10908  		ret = btf_distill_func_proto(log, btf, t, tname, fmodel);
fec56f5890d93f Alexei Starovoitov     2019-11-14  10909  		if (ret < 0)
c2d0f6ffe7709e Toke Høiland-Jørgensen 2020-07-15  10910  			return ret;
c2d0f6ffe7709e Toke Høiland-Jørgensen 2020-07-15  10911  
5b92a28aae4dd0 Alexei Starovoitov     2019-11-14  10912  		if (tgt_prog) {
e9eeec58c992c4 Yonghong Song          2019-12-04  10913  			if (subprog == 0)
e9eeec58c992c4 Yonghong Song          2019-12-04  10914  				addr = (long) tgt_prog->bpf_func;
e9eeec58c992c4 Yonghong Song          2019-12-04  10915  			else
5b92a28aae4dd0 Alexei Starovoitov     2019-11-14  10916  				addr = (long) tgt_prog->aux->func[subprog]->bpf_func;
5b92a28aae4dd0 Alexei Starovoitov     2019-11-14  10917  		} else {
fec56f5890d93f Alexei Starovoitov     2019-11-14  10918  			addr = kallsyms_lookup_name(tname);
fec56f5890d93f Alexei Starovoitov     2019-11-14  10919  			if (!addr) {
e33243ff1dd2cb Toke Høiland-Jørgensen 2020-07-15  10920  				bpf_log(log,
fec56f5890d93f Alexei Starovoitov     2019-11-14  10921  					"The address of function %s cannot be found\n",
fec56f5890d93f Alexei Starovoitov     2019-11-14  10922  					tname);
c2d0f6ffe7709e Toke Høiland-Jørgensen 2020-07-15  10923  				return -ENOENT;
fec56f5890d93f Alexei Starovoitov     2019-11-14  10924  			}
5b92a28aae4dd0 Alexei Starovoitov     2019-11-14  10925  		}
c2d0f6ffe7709e Toke Høiland-Jørgensen 2020-07-15  10926  		break;
c2d0f6ffe7709e Toke Høiland-Jørgensen 2020-07-15  10927  	}
18644cec714aab Alexei Starovoitov     2020-05-28  10928  
c2d0f6ffe7709e Toke Høiland-Jørgensen 2020-07-15  10929  	*tgt_addr = addr;
c2d0f6ffe7709e Toke Høiland-Jørgensen 2020-07-15  10930  	if (tgt_name)
c2d0f6ffe7709e Toke Høiland-Jørgensen 2020-07-15  10931  		*tgt_name = tname;
c2d0f6ffe7709e Toke Høiland-Jørgensen 2020-07-15  10932  	if (tgt_type)
c2d0f6ffe7709e Toke Høiland-Jørgensen 2020-07-15  10933  		*tgt_type = t;
c2d0f6ffe7709e Toke Høiland-Jørgensen 2020-07-15  10934  	return 0;
18644cec714aab Alexei Starovoitov     2020-05-28  10935  }

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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 37112 bytes --]

Re: [PATCH bpf-next v2 3/6] bpf: support attaching freplace programs to multiple attach points

[Toke Høiland-Jørgensen]

Alexei Starovoitov <alexei.starovoitov@gmail.com> writes:

> On Wed, Jul 15, 2020 at 03:09:02PM +0200, Toke Høiland-Jørgensen wrote:
>>  
>> +	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;
>> +		}
>> +		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;
>> +		}
>> +
>> +	} else if (btf_id) {
>> +		err = -EINVAL;
>> +		goto out_put_prog;
>> +	} else {
>> +		btf_id = prog->aux->attach_btf_id;
>> +		tgt_prog = prog->aux->linked_prog;
>> +		if (tgt_prog)
>> +			bpf_prog_inc(tgt_prog); /* we call bpf_prog_put() on link release */
>
> so the first prog_load cmd will beholding the first target prog?
> This is complete non starter.
> You didn't mention such decision anywhere.
> The first ext prog will attach to the first dispatcher xdp prog,
> then that ext prog will multi attach to second dispatcher xdp prog and
> the first dispatcher prog will live in the kernel forever.

Huh, yeah, you're right that's no good. Missing that was a think-o on my
part, sorry about that :/

> That's not what we discussed back in April.

No, you mentioned turning aux->linked_prog into a list. However once I
started looking at it I figured it was better to actually have all this
(the trampoline and ref) as part of the bpf_link structure, since
logically they're related.

But as you pointed out, the original reference sticks. So either that
needs to be removed, or I need to go back to the 'aux->linked_progs as a
list' idea. Any preference?

>> +	}
>> +	err = bpf_check_attach_target(NULL, prog, tgt_prog, btf_id,
>> +				      &fmodel, &addr, NULL, NULL);
>
> This is a second check for btf id match?
> What's the point? The first one was done at load time.
> When tgt_prog_fd/tgt_btf_id are zero there is no need to recheck.

It's not strictly needed if tgt_prog/btf_id is not set, but it doesn't
hurt either; and it was convenient to reuse it to resolve the func addr
for the trampoline + it means everything goes through the same code path.

> I really hope I'm misreading these patches, because they look very raw.

I don't think you are. I'll admit to them being a bit raw, but this was
as far as I got and since I'll be away for three weeks I figured it was
better to post them in case anyone else was interested in playing with
it.

So if anyone wants to pick these patches up while I'm gone, feel free;
otherwise, I'll get back to it after my vacation :)

-Toke

Re: [PATCH bpf-next v2 3/6] bpf: support attaching freplace programs to multiple attach points

[Alexei Starovoitov]

On Thu, Jul 16, 2020 at 12:50:05PM +0200, Toke Høiland-Jørgensen wrote:
> Alexei Starovoitov <alexei.starovoitov@gmail.com> writes:
> 
> > On Wed, Jul 15, 2020 at 03:09:02PM +0200, Toke Høiland-Jørgensen wrote:
> >>  
> >> +	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;
> >> +		}
> >> +		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;
> >> +		}
> >> +
> >> +	} else if (btf_id) {
> >> +		err = -EINVAL;
> >> +		goto out_put_prog;
> >> +	} else {
> >> +		btf_id = prog->aux->attach_btf_id;
> >> +		tgt_prog = prog->aux->linked_prog;
> >> +		if (tgt_prog)
> >> +			bpf_prog_inc(tgt_prog); /* we call bpf_prog_put() on link release */
> >
> > so the first prog_load cmd will beholding the first target prog?
> > This is complete non starter.
> > You didn't mention such decision anywhere.
> > The first ext prog will attach to the first dispatcher xdp prog,
> > then that ext prog will multi attach to second dispatcher xdp prog and
> > the first dispatcher prog will live in the kernel forever.
> 
> Huh, yeah, you're right that's no good. Missing that was a think-o on my
> part, sorry about that :/
> 
> > That's not what we discussed back in April.
> 
> No, you mentioned turning aux->linked_prog into a list. However once I
> started looking at it I figured it was better to actually have all this
> (the trampoline and ref) as part of the bpf_link structure, since
> logically they're related.
> 
> But as you pointed out, the original reference sticks. So either that
> needs to be removed, or I need to go back to the 'aux->linked_progs as a
> list' idea. Any preference?

Good question. Back then I was thinking about converting linked_prog into link
list, since standalone single linked_prog is quite odd, because attaching ext
prog to multiple tgt progs should have equivalent properties across all
attachments.
Back then bpf_link wasn't quite developed.
Now I feel moving into bpf_tracing_link is better.
I guess a link list of bpf_tracing_link-s from 'struct bpf_prog' might work.
At prog load time we can do bpf_link_init() only (without doing bpf_link_prime)
and keep this pre-populated bpf_link with target bpf prog and trampoline
in a link list accessed from 'struct bpf_prog'.
Then bpf_tracing_prog_attach() without extra tgt_prog_fd/btf_id would complete
that bpf_tracing_link by calling bpf_link_prime() and bpf_link_settle()
without allocating new one.
Something like:
struct bpf_tracing_link {
        struct bpf_link link;  /* ext prog pointer is hidding in there */
        enum bpf_attach_type attach_type;
        struct bpf_trampoline *tr;
        struct bpf_prog *tgt_prog; /* old aux->linked_prog */
};

ext prog -> aux -> link list of above bpf_tracing_link-s

It's a circular reference, obviously.
Need to think through the complications and locking.

bpf_tracing_prog_attach() with tgt_prog_fd/btf_id will alloc new bpf_tracing_link
and will add it to a link list.

Just a rough idea. I wonder what Andrii thinks.

> 
> >> +	}
> >> +	err = bpf_check_attach_target(NULL, prog, tgt_prog, btf_id,
> >> +				      &fmodel, &addr, NULL, NULL);
> >
> > This is a second check for btf id match?
> > What's the point? The first one was done at load time.
> > When tgt_prog_fd/tgt_btf_id are zero there is no need to recheck.
> 
> It's not strictly needed if tgt_prog/btf_id is not set, but it doesn't
> hurt either; and it was convenient to reuse it to resolve the func addr
> for the trampoline + it means everything goes through the same code path.

Doing the same work twice is a sign that this function needs to split
into more than 3 helpers, so the work is not repeated.

> 
> > I really hope I'm misreading these patches, because they look very raw.
> 
> I don't think you are. I'll admit to them being a bit raw, but this was
> as far as I got and since I'll be away for three weeks I figured it was
> better to post them in case anyone else was interested in playing with
> it.

Since it was v2 I figured you want it to land and it's ready.
Next time please mention the state of patches.
It's absolutely fine to post raw patches. It's fine to post stuff
that doesn't compile. But please explain the state in commit logs or cover.

Re: [PATCH bpf-next v2 3/6] bpf: support attaching freplace programs to multiple attach points

[Toke Høiland-Jørgensen]

Alexei Starovoitov <alexei.starovoitov@gmail.com> writes:

> On Thu, Jul 16, 2020 at 12:50:05PM +0200, Toke Høiland-Jørgensen wrote:
>> Alexei Starovoitov <alexei.starovoitov@gmail.com> writes:
>> 
>> > On Wed, Jul 15, 2020 at 03:09:02PM +0200, Toke Høiland-Jørgensen wrote:
>> >>  
>> >> +	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;
>> >> +		}
>> >> +		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;
>> >> +		}
>> >> +
>> >> +	} else if (btf_id) {
>> >> +		err = -EINVAL;
>> >> +		goto out_put_prog;
>> >> +	} else {
>> >> +		btf_id = prog->aux->attach_btf_id;
>> >> +		tgt_prog = prog->aux->linked_prog;
>> >> +		if (tgt_prog)
>> >> +			bpf_prog_inc(tgt_prog); /* we call bpf_prog_put() on link release */
>> >
>> > so the first prog_load cmd will beholding the first target prog?
>> > This is complete non starter.
>> > You didn't mention such decision anywhere.
>> > The first ext prog will attach to the first dispatcher xdp prog,
>> > then that ext prog will multi attach to second dispatcher xdp prog and
>> > the first dispatcher prog will live in the kernel forever.
>> 
>> Huh, yeah, you're right that's no good. Missing that was a think-o on my
>> part, sorry about that :/
>> 
>> > That's not what we discussed back in April.
>> 
>> No, you mentioned turning aux->linked_prog into a list. However once I
>> started looking at it I figured it was better to actually have all this
>> (the trampoline and ref) as part of the bpf_link structure, since
>> logically they're related.
>> 
>> But as you pointed out, the original reference sticks. So either that
>> needs to be removed, or I need to go back to the 'aux->linked_progs as a
>> list' idea. Any preference?
>
> Good question. Back then I was thinking about converting linked_prog into link
> list, since standalone single linked_prog is quite odd, because attaching ext
> prog to multiple tgt progs should have equivalent properties across all
> attachments.
> Back then bpf_link wasn't quite developed.
> Now I feel moving into bpf_tracing_link is better.
> I guess a link list of bpf_tracing_link-s from 'struct bpf_prog' might work.
> At prog load time we can do bpf_link_init() only (without doing bpf_link_prime)
> and keep this pre-populated bpf_link with target bpf prog and trampoline
> in a link list accessed from 'struct bpf_prog'.
> Then bpf_tracing_prog_attach() without extra tgt_prog_fd/btf_id would complete
> that bpf_tracing_link by calling bpf_link_prime() and bpf_link_settle()
> without allocating new one.
> Something like:
> struct bpf_tracing_link {
>         struct bpf_link link;  /* ext prog pointer is hidding in there */
>         enum bpf_attach_type attach_type;
>         struct bpf_trampoline *tr;
>         struct bpf_prog *tgt_prog; /* old aux->linked_prog */
> };
>
> ext prog -> aux -> link list of above bpf_tracing_link-s

Yeah, I thought along these lines as well (was thinking a new struct
referenced from bpf_tracing_link, but sure, why not just stick the whole
thing into aux?).

> It's a circular reference, obviously.
> Need to think through the complications and locking.

Yup, will do so when I get back to this. One other implication of this
change: If we make the linked_prog completely dynamic you can no longer
do:

link_fd = bpf_raw_tracepoint_open(prog);
close(link_fd);
link_fd = bpf_raw_tracepoint_open(prog):

since after that close(), the original linked_prog will be gone. Unless
we always leave at least one linked_prog alive? But then we can't
guarantee that it's the target that was supplied on program load if it
was reattached. Is that acceptable?

>> I don't think you are. I'll admit to them being a bit raw, but this was
>> as far as I got and since I'll be away for three weeks I figured it was
>> better to post them in case anyone else was interested in playing with
>> it.
>
> Since it was v2 I figured you want it to land and it's ready.
> Next time please mention the state of patches.
> It's absolutely fine to post raw patches. It's fine to post stuff
> that doesn't compile. But please explain the state in commit logs or cover.

Right, sorry that was not clear; will make sure to spell it out next
time.

-Toke

Re: [PATCH bpf-next v2 3/6] bpf: support attaching freplace programs to multiple attach points

[Andrii Nakryiko]

On Thu, Jul 16, 2020 at 7:06 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Thu, Jul 16, 2020 at 12:50:05PM +0200, Toke Høiland-Jørgensen wrote:
> > Alexei Starovoitov <alexei.starovoitov@gmail.com> writes:
> >
> > > On Wed, Jul 15, 2020 at 03:09:02PM +0200, Toke Høiland-Jørgensen wrote:
> > >>
> > >> +  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;
> > >> +          }
> > >> +          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;
> > >> +          }
> > >> +
> > >> +  } else if (btf_id) {
> > >> +          err = -EINVAL;
> > >> +          goto out_put_prog;
> > >> +  } else {
> > >> +          btf_id = prog->aux->attach_btf_id;
> > >> +          tgt_prog = prog->aux->linked_prog;
> > >> +          if (tgt_prog)
> > >> +                  bpf_prog_inc(tgt_prog); /* we call bpf_prog_put() on link release */
> > >
> > > so the first prog_load cmd will beholding the first target prog?
> > > This is complete non starter.
> > > You didn't mention such decision anywhere.
> > > The first ext prog will attach to the first dispatcher xdp prog,
> > > then that ext prog will multi attach to second dispatcher xdp prog and
> > > the first dispatcher prog will live in the kernel forever.
> >
> > Huh, yeah, you're right that's no good. Missing that was a think-o on my
> > part, sorry about that :/
> >
> > > That's not what we discussed back in April.
> >
> > No, you mentioned turning aux->linked_prog into a list. However once I
> > started looking at it I figured it was better to actually have all this
> > (the trampoline and ref) as part of the bpf_link structure, since
> > logically they're related.
> >
> > But as you pointed out, the original reference sticks. So either that
> > needs to be removed, or I need to go back to the 'aux->linked_progs as a
> > list' idea. Any preference?
>
> Good question. Back then I was thinking about converting linked_prog into link
> list, since standalone single linked_prog is quite odd, because attaching ext
> prog to multiple tgt progs should have equivalent properties across all
> attachments.
> Back then bpf_link wasn't quite developed.
> Now I feel moving into bpf_tracing_link is better.
> I guess a link list of bpf_tracing_link-s from 'struct bpf_prog' might work.
> At prog load time we can do bpf_link_init() only (without doing bpf_link_prime)
> and keep this pre-populated bpf_link with target bpf prog and trampoline
> in a link list accessed from 'struct bpf_prog'.
> Then bpf_tracing_prog_attach() without extra tgt_prog_fd/btf_id would complete
> that bpf_tracing_link by calling bpf_link_prime() and bpf_link_settle()
> without allocating new one.
> Something like:
> struct bpf_tracing_link {
>         struct bpf_link link;  /* ext prog pointer is hidding in there */
>         enum bpf_attach_type attach_type;
>         struct bpf_trampoline *tr;
>         struct bpf_prog *tgt_prog; /* old aux->linked_prog */
> };
>
> ext prog -> aux -> link list of above bpf_tracing_link-s
>
> It's a circular reference, obviously.
> Need to think through the complications and locking.
>
> bpf_tracing_prog_attach() with tgt_prog_fd/btf_id will alloc new bpf_tracing_link
> and will add it to a link list.
>
> Just a rough idea. I wonder what Andrii thinks.
>

I need to spend more time reading existing and new code to see all the
details, but I'll throw a slightly different proposal and let you guys
shoot it down.

So, what if instead of having linked_prog (as bpf_prog *, refcnt'ed),
at BPF_PROG_LOAD time we just record the target prog's ID. BPF
verifier, when doing its target prog checks would attempt to get
bpf_prog * reference; if by that time the target program is gone,
fail, of course. If not, everything proceeds as is, at the end of
verification target_prog is put until attach time.

Then at attach time, we either go with pre-recorded (in
prog->aux->linked_prog_id) target prog's ID or we get a new one from
RAW_TP_OPEN tgt_prog_fd. Either way, we bump refcnt on that target
prog and keep it with bpf_tracing_link (so link on detach would put
target_prog, that way it doesn't go away while EXT prog is attached).
Then do all the compatibility checks, and if everything works out,
bpf_tracing_link gets created, we record trampoline there, etc, etc.
Basically, instead of having an EXT prog holding a reference to the
target prog, only attachment (bpf_link) does that, which conceptually
also seems to make more sense to me. For verification we store prog ID
and don't hold target prog at all.


Now, there will be a problem once you attach EXT prog to a new XDP
root program and release a link against the original XDP root program.
First, I hope I understand the desired sequence right, here's an
example:

1. load XDP root prog X
2. load EXT prog with target prog X
3. attach EXT prog to prog X
4. load XDP root prog Y
5. attach EXT prog to prog Y (Y and X should be "compatible")
6. detach prog X (close bpf_link)

Is that the right sequence?

If yes, then the problem with storing ID of prog X in EXT
prog->aux->linked_prog_id is that you won't be able to re-attach to
new prog Z, because there won't be anything to check compatibility
against (prog X will be long time gone).

So we can do two things here:

1. on attach, replace ext_prog->aux->linked_prog_id with the latest
attached prog (prog Y ID from above example)
2. instead of recording target program FD/ID, capture BTF FD and/or
enough BTF information for checking compatibility.

Approach 2) seems like conceptually the right thing to do (record type
info we care about, not an **instance** of BPF program, compatible
with that type info), but technically might be harder.


That's my thoughts without digging too deep, so sorry if I'm making
some stupid assumptions.



[...]

Re: [PATCH bpf-next v2 3/6] bpf: support attaching freplace programs to multiple attach points

[Alexei Starovoitov]

On Fri, Jul 17, 2020 at 12:52:10PM +0200, Toke Høiland-Jørgensen wrote:
> 
> > It's a circular reference, obviously.
> > Need to think through the complications and locking.
> 
> Yup, will do so when I get back to this. One other implication of this
> change: If we make the linked_prog completely dynamic you can no longer
> do:
> 
> link_fd = bpf_raw_tracepoint_open(prog);
> close(link_fd);
> link_fd = bpf_raw_tracepoint_open(prog):
> 
> since after that close(), the original linked_prog will be gone. Unless
> we always leave at least one linked_prog alive? But then we can't
> guarantee that it's the target that was supplied on program load if it
> was reattached. Is that acceptable?

I think both options are fine.
We can start with simple case where close would destroy the last link
and if somebody complains we can keep 'at least one alive'.
This is such low level implementation detail that I don't think any user
can reliably count on it staying this way.

Re: [PATCH bpf-next v2 3/6] bpf: support attaching freplace programs to multiple attach points

[Alexei Starovoitov]

On Sun, Jul 19, 2020 at 10:02:48PM -0700, Andrii Nakryiko wrote:
> On Thu, Jul 16, 2020 at 7:06 PM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > On Thu, Jul 16, 2020 at 12:50:05PM +0200, Toke Høiland-Jørgensen wrote:
> > > Alexei Starovoitov <alexei.starovoitov@gmail.com> writes:
> > >
> > > > On Wed, Jul 15, 2020 at 03:09:02PM +0200, Toke Høiland-Jørgensen wrote:
> > > >>
> > > >> +  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;
> > > >> +          }
> > > >> +          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;
> > > >> +          }
> > > >> +
> > > >> +  } else if (btf_id) {
> > > >> +          err = -EINVAL;
> > > >> +          goto out_put_prog;
> > > >> +  } else {
> > > >> +          btf_id = prog->aux->attach_btf_id;
> > > >> +          tgt_prog = prog->aux->linked_prog;
> > > >> +          if (tgt_prog)
> > > >> +                  bpf_prog_inc(tgt_prog); /* we call bpf_prog_put() on link release */
> > > >
> > > > so the first prog_load cmd will beholding the first target prog?
> > > > This is complete non starter.
> > > > You didn't mention such decision anywhere.
> > > > The first ext prog will attach to the first dispatcher xdp prog,
> > > > then that ext prog will multi attach to second dispatcher xdp prog and
> > > > the first dispatcher prog will live in the kernel forever.
> > >
> > > Huh, yeah, you're right that's no good. Missing that was a think-o on my
> > > part, sorry about that :/
> > >
> > > > That's not what we discussed back in April.
> > >
> > > No, you mentioned turning aux->linked_prog into a list. However once I
> > > started looking at it I figured it was better to actually have all this
> > > (the trampoline and ref) as part of the bpf_link structure, since
> > > logically they're related.
> > >
> > > But as you pointed out, the original reference sticks. So either that
> > > needs to be removed, or I need to go back to the 'aux->linked_progs as a
> > > list' idea. Any preference?
> >
> > Good question. Back then I was thinking about converting linked_prog into link
> > list, since standalone single linked_prog is quite odd, because attaching ext
> > prog to multiple tgt progs should have equivalent properties across all
> > attachments.
> > Back then bpf_link wasn't quite developed.
> > Now I feel moving into bpf_tracing_link is better.
> > I guess a link list of bpf_tracing_link-s from 'struct bpf_prog' might work.
> > At prog load time we can do bpf_link_init() only (without doing bpf_link_prime)
> > and keep this pre-populated bpf_link with target bpf prog and trampoline
> > in a link list accessed from 'struct bpf_prog'.
> > Then bpf_tracing_prog_attach() without extra tgt_prog_fd/btf_id would complete
> > that bpf_tracing_link by calling bpf_link_prime() and bpf_link_settle()
> > without allocating new one.
> > Something like:
> > struct bpf_tracing_link {
> >         struct bpf_link link;  /* ext prog pointer is hidding in there */
> >         enum bpf_attach_type attach_type;
> >         struct bpf_trampoline *tr;
> >         struct bpf_prog *tgt_prog; /* old aux->linked_prog */
> > };
> >
> > ext prog -> aux -> link list of above bpf_tracing_link-s
> >
> > It's a circular reference, obviously.
> > Need to think through the complications and locking.
> >
> > bpf_tracing_prog_attach() with tgt_prog_fd/btf_id will alloc new bpf_tracing_link
> > and will add it to a link list.
> >
> > Just a rough idea. I wonder what Andrii thinks.
> >
> 
> I need to spend more time reading existing and new code to see all the
> details, but I'll throw a slightly different proposal and let you guys
> shoot it down.
> 
> So, what if instead of having linked_prog (as bpf_prog *, refcnt'ed),
> at BPF_PROG_LOAD time we just record the target prog's ID. BPF
> verifier, when doing its target prog checks would attempt to get
> bpf_prog * reference; if by that time the target program is gone,
> fail, of course. If not, everything proceeds as is, at the end of
> verification target_prog is put until attach time.
> 
> Then at attach time, we either go with pre-recorded (in
> prog->aux->linked_prog_id) target prog's ID or we get a new one from
> RAW_TP_OPEN tgt_prog_fd. Either way, we bump refcnt on that target
> prog and keep it with bpf_tracing_link (so link on detach would put
> target_prog, that way it doesn't go away while EXT prog is attached).
> Then do all the compatibility checks, and if everything works out,
> bpf_tracing_link gets created, we record trampoline there, etc, etc.
> Basically, instead of having an EXT prog holding a reference to the
> target prog, only attachment (bpf_link) does that, which conceptually
> also seems to make more sense to me. For verification we store prog ID
> and don't hold target prog at all.
> 
> 
> Now, there will be a problem once you attach EXT prog to a new XDP
> root program and release a link against the original XDP root program.
> First, I hope I understand the desired sequence right, here's an
> example:
> 
> 1. load XDP root prog X
> 2. load EXT prog with target prog X
> 3. attach EXT prog to prog X
> 4. load XDP root prog Y
> 5. attach EXT prog to prog Y (Y and X should be "compatible")
> 6. detach prog X (close bpf_link)
> 
> Is that the right sequence?
> 
> If yes, then the problem with storing ID of prog X in EXT
> prog->aux->linked_prog_id is that you won't be able to re-attach to
> new prog Z, because there won't be anything to check compatibility
> against (prog X will be long time gone).
> 
> So we can do two things here:
> 
> 1. on attach, replace ext_prog->aux->linked_prog_id with the latest
> attached prog (prog Y ID from above example)
> 2. instead of recording target program FD/ID, capture BTF FD and/or
> enough BTF information for checking compatibility.
> 
> Approach 2) seems like conceptually the right thing to do (record type
> info we care about, not an **instance** of BPF program, compatible
> with that type info), but technically might be harder.

I've read your proposal couple times and still don't get what you're
trying to solve with either ID or BTF info recording.
So that target prog doesn't get refcnt-ed? What's a problem with it?
Currently it's being refcnt-d in aux->linked_prog.
What I'm proposing about is to convert aux->linked_prog into a link list
of bpf_tracing_links which will contain linked_prog inside.
Conceptually that's what bpf_link is doing. It links two progs.
EXT prog is recorded in 'struct bpf_link' and
the target prog is recorded in 'struct bpf_tracing_link'.
So from bpf_link perspective everything seems clean to me.
The link list of bpf_tracing_link-s in EXT_prog->aux is only to preserve
existing api of prog_load cmd.

As far as step 5: attach EXT prog to prog Y (Y and X should be "compatible")
The chance of failure there should be minimal. libxdp/libdispatcher will
prepare rootlet XDP prog. It should really make sure that Y and X are compatible.
This should be invisible to users.

In addition we still need bpf_link_update_hook() I was talking about in April.
The full sequence is:
first user process:
 1. load XDP root prog X
 1' root_link = attach X to eth0
 2. load EXT prog with target prog X
 3. app1_link_fd = attach EXT prog to prog X
second user process:
 4. load XDP root prog Y
 4'. find EXT prog of the first user process
 5. app2_link_fd = attach EXT prog to prog Y (Y and X should be "compatible")
 6. bpf_link_update(root_link, X, Y); // now packet flows into Y and into EXT
   // while EXT is attached in two places
 7. app1_link_fd' = FD in second process that points to the same tracing link
    as app1_link_fd in the first process.
   bpf_link_update_hook(app1_link_fd', app2_link_fd)
the last operation need to update bpf_tracing_link that is held by app1
(which is the first user process) from the second user process. It needs to
retarget (update_hook) inside bpf_tracing_link from X to Y.
Since the processes are more or less not aware of each other.
One firewall holds link_fd that connects EXT to X,
but the second firewall (via libxdp) is updaing that tracing link
to re-hook EXT into Y.

Re: [PATCH bpf-next v2 3/6] bpf: support attaching freplace programs to multiple attach points

[Andrii Nakryiko]

On Mon, Jul 20, 2020 at 4:35 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Sun, Jul 19, 2020 at 10:02:48PM -0700, Andrii Nakryiko wrote:
> > On Thu, Jul 16, 2020 at 7:06 PM Alexei Starovoitov
> > <alexei.starovoitov@gmail.com> wrote:
> > >
> > > On Thu, Jul 16, 2020 at 12:50:05PM +0200, Toke Høiland-Jørgensen wrote:
> > > > Alexei Starovoitov <alexei.starovoitov@gmail.com> writes:
> > > >
> > > > > On Wed, Jul 15, 2020 at 03:09:02PM +0200, Toke Høiland-Jørgensen wrote:
> > > > >>
> > > > >> +  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;
> > > > >> +          }
> > > > >> +          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;
> > > > >> +          }
> > > > >> +
> > > > >> +  } else if (btf_id) {
> > > > >> +          err = -EINVAL;
> > > > >> +          goto out_put_prog;
> > > > >> +  } else {
> > > > >> +          btf_id = prog->aux->attach_btf_id;
> > > > >> +          tgt_prog = prog->aux->linked_prog;
> > > > >> +          if (tgt_prog)
> > > > >> +                  bpf_prog_inc(tgt_prog); /* we call bpf_prog_put() on link release */
> > > > >
> > > > > so the first prog_load cmd will beholding the first target prog?
> > > > > This is complete non starter.
> > > > > You didn't mention such decision anywhere.
> > > > > The first ext prog will attach to the first dispatcher xdp prog,
> > > > > then that ext prog will multi attach to second dispatcher xdp prog and
> > > > > the first dispatcher prog will live in the kernel forever.
> > > >
> > > > Huh, yeah, you're right that's no good. Missing that was a think-o on my
> > > > part, sorry about that :/
> > > >
> > > > > That's not what we discussed back in April.
> > > >
> > > > No, you mentioned turning aux->linked_prog into a list. However once I
> > > > started looking at it I figured it was better to actually have all this
> > > > (the trampoline and ref) as part of the bpf_link structure, since
> > > > logically they're related.
> > > >
> > > > But as you pointed out, the original reference sticks. So either that
> > > > needs to be removed, or I need to go back to the 'aux->linked_progs as a
> > > > list' idea. Any preference?
> > >
> > > Good question. Back then I was thinking about converting linked_prog into link
> > > list, since standalone single linked_prog is quite odd, because attaching ext
> > > prog to multiple tgt progs should have equivalent properties across all
> > > attachments.
> > > Back then bpf_link wasn't quite developed.
> > > Now I feel moving into bpf_tracing_link is better.
> > > I guess a link list of bpf_tracing_link-s from 'struct bpf_prog' might work.
> > > At prog load time we can do bpf_link_init() only (without doing bpf_link_prime)
> > > and keep this pre-populated bpf_link with target bpf prog and trampoline
> > > in a link list accessed from 'struct bpf_prog'.
> > > Then bpf_tracing_prog_attach() without extra tgt_prog_fd/btf_id would complete
> > > that bpf_tracing_link by calling bpf_link_prime() and bpf_link_settle()
> > > without allocating new one.
> > > Something like:
> > > struct bpf_tracing_link {
> > >         struct bpf_link link;  /* ext prog pointer is hidding in there */
> > >         enum bpf_attach_type attach_type;
> > >         struct bpf_trampoline *tr;
> > >         struct bpf_prog *tgt_prog; /* old aux->linked_prog */
> > > };
> > >
> > > ext prog -> aux -> link list of above bpf_tracing_link-s
> > >
> > > It's a circular reference, obviously.
> > > Need to think through the complications and locking.
> > >
> > > bpf_tracing_prog_attach() with tgt_prog_fd/btf_id will alloc new bpf_tracing_link
> > > and will add it to a link list.
> > >
> > > Just a rough idea. I wonder what Andrii thinks.
> > >
> >
> > I need to spend more time reading existing and new code to see all the
> > details, but I'll throw a slightly different proposal and let you guys
> > shoot it down.
> >
> > So, what if instead of having linked_prog (as bpf_prog *, refcnt'ed),
> > at BPF_PROG_LOAD time we just record the target prog's ID. BPF
> > verifier, when doing its target prog checks would attempt to get
> > bpf_prog * reference; if by that time the target program is gone,
> > fail, of course. If not, everything proceeds as is, at the end of
> > verification target_prog is put until attach time.
> >
> > Then at attach time, we either go with pre-recorded (in
> > prog->aux->linked_prog_id) target prog's ID or we get a new one from
> > RAW_TP_OPEN tgt_prog_fd. Either way, we bump refcnt on that target
> > prog and keep it with bpf_tracing_link (so link on detach would put
> > target_prog, that way it doesn't go away while EXT prog is attached).
> > Then do all the compatibility checks, and if everything works out,
> > bpf_tracing_link gets created, we record trampoline there, etc, etc.
> > Basically, instead of having an EXT prog holding a reference to the
> > target prog, only attachment (bpf_link) does that, which conceptually
> > also seems to make more sense to me. For verification we store prog ID
> > and don't hold target prog at all.
> >
> >
> > Now, there will be a problem once you attach EXT prog to a new XDP
> > root program and release a link against the original XDP root program.
> > First, I hope I understand the desired sequence right, here's an
> > example:
> >
> > 1. load XDP root prog X
> > 2. load EXT prog with target prog X
> > 3. attach EXT prog to prog X
> > 4. load XDP root prog Y
> > 5. attach EXT prog to prog Y (Y and X should be "compatible")
> > 6. detach prog X (close bpf_link)
> >
> > Is that the right sequence?
> >
> > If yes, then the problem with storing ID of prog X in EXT
> > prog->aux->linked_prog_id is that you won't be able to re-attach to
> > new prog Z, because there won't be anything to check compatibility
> > against (prog X will be long time gone).
> >
> > So we can do two things here:
> >
> > 1. on attach, replace ext_prog->aux->linked_prog_id with the latest
> > attached prog (prog Y ID from above example)
> > 2. instead of recording target program FD/ID, capture BTF FD and/or
> > enough BTF information for checking compatibility.
> >
> > Approach 2) seems like conceptually the right thing to do (record type
> > info we care about, not an **instance** of BPF program, compatible
> > with that type info), but technically might be harder.
>
> I've read your proposal couple times and still don't get what you're
> trying to solve with either ID or BTF info recording.
> So that target prog doesn't get refcnt-ed? What's a problem with it?
> Currently it's being refcnt-d in aux->linked_prog.
> What I'm proposing about is to convert aux->linked_prog into a link list
> of bpf_tracing_links which will contain linked_prog inside.
> Conceptually that's what bpf_link is doing. It links two progs.
> EXT prog is recorded in 'struct bpf_link' and
> the target prog is recorded in 'struct bpf_tracing_link'.
> So from bpf_link perspective everything seems clean to me.
> The link list of bpf_tracing_link-s in EXT_prog->aux is only to preserve
> existing api of prog_load cmd.

Right, I wanted to avoid taking a refcnt on aux->linked_prog during
PROG_LOAD. The reason for that was (and still is) that I don't get who
and when has to bpf_prog_put() original aux->linked_prog to allow the
prog X to be freed. I.e., after you re-attach to prog Y, how prog X is
released (assuming no active bpf_link is keeping it from being freed)?
That's my biggest confusion right now.

I also didn't like the idea of half-creating bpf_tracing_link on
PROG_LOAD and then turning it into a real link with bpf_link_settle on
attach. That sounded like a hack to me.

But now I'm also confused why we need to turn aux->linked_prog into a
list. Seems like we need it only for old-style attach that doesn't
specify tgt_prog_fd, no? Only in that case we'll use aux->linked_prog.
Otherwise we know the target prog from tgt_prog_fd. So I'll be honest
that I don't get the whole idea of maintaining a list of
bpf_tracing_links. It seems like it should be possible to make
bpf_tracing_link decoupled from any prog's aux and have their own
independent lifetime.

>
> As far as step 5: attach EXT prog to prog Y (Y and X should be "compatible")
> The chance of failure there should be minimal. libxdp/libdispatcher will
> prepare rootlet XDP prog. It should really make sure that Y and X are compatible.
> This should be invisible to users.

Right, of course, but the kernel needs to validate that anyways, which
is why I pointed that out. Or are you saying we should just assume
that they are valid?

>
> In addition we still need bpf_link_update_hook() I was talking about in April.
> The full sequence is:
> first user process:
>  1. load XDP root prog X
>  1' root_link = attach X to eth0
>  2. load EXT prog with target prog X
>  3. app1_link_fd = attach EXT prog to prog X
> second user process:
>  4. load XDP root prog Y
>  4'. find EXT prog of the first user process
>  5. app2_link_fd = attach EXT prog to prog Y (Y and X should be "compatible")
>  6. bpf_link_update(root_link, X, Y); // now packet flows into Y and into EXT
>    // while EXT is attached in two places
>  7. app1_link_fd' = FD in second process that points to the same tracing link
>     as app1_link_fd in the first process.
>    bpf_link_update_hook(app1_link_fd', app2_link_fd)
> the last operation need to update bpf_tracing_link that is held by app1
> (which is the first user process) from the second user process. It needs to
> retarget (update_hook) inside bpf_tracing_link from X to Y.
> Since the processes are more or less not aware of each other.
> One firewall holds link_fd that connects EXT to X,
> but the second firewall (via libxdp) is updaing that tracing link
> to re-hook EXT into Y.

Yeah, should be doable given that bpf_trampoline is independently refcounted.

Re: [PATCH bpf-next v2 3/6] bpf: support attaching freplace programs to multiple attach points

[Alexei Starovoitov]

On Mon, Jul 20, 2020 at 08:48:04PM -0700, Andrii Nakryiko wrote:
> 
> Right, I wanted to avoid taking a refcnt on aux->linked_prog during
> PROG_LOAD. The reason for that was (and still is) that I don't get who
> and when has to bpf_prog_put() original aux->linked_prog to allow the
> prog X to be freed. I.e., after you re-attach to prog Y, how prog X is
> released (assuming no active bpf_link is keeping it from being freed)?
> That's my biggest confusion right now.
> 
> I also didn't like the idea of half-creating bpf_tracing_link on
> PROG_LOAD and then turning it into a real link with bpf_link_settle on
> attach. That sounded like a hack to me.

The link is kinda already created during prog_load of EXT type.
Typically prog_load needs expected_attach_type that points to something
that is not going to disappear. In case of EXT progs the situation is different,
since the target can be unloaded. So the prog load cmd not only validates the
program extension but links target and ext prog together at the same time.
The target prog will be held until EXT prog is unloaded.
I think it's important to preserve this semantics to the users that the target prog
is frozen at load time and no races are going to happen later.
Otherwise it leads to double validation at attach time and races.

What raw_tp_open is doing right now is a hack. It allocates bpf_tracing_link,
registers it into link_idr and activates trampoline, but in reality that link is already there.
I think we can clean it up by creating bpf_tracing_link at prog load time.
Whether to register it at that time into link_idr is up to discussion.
(I think probably not).
Then raw_tp_open will activate that allocated bpf_tracing_link via trampoline,
_remove_ it from aux->linked_tracing_link (old linked_prog) and
return FD to the user.
So this partially created link at load_time will become complete link and
close of the link will detach EXT from the target and the target can be unloaded.
(Currently the target cannot be unloaded until EXT is loaded which is not great).
The EXT_prog->aux->linked_tracing_link (old linked_prog) will exist only during
the time between prog_load and raw_tp_open without args.
I think that would be a good clean up.
Then multi attach of EXT progs is clean too.
New raw_tp_open with tgt_prog_fd/tgt_btf_id will validate EXT against the new target,
link them via new bpf_tracing_link, activate it via trampoline and return FD.
No link list anywhere.
Note that this second validation of EXT against new target is light weight comparing
to the load. The first load goes through all EXT instructions with verifier ctx of
the target prog. The second validation needs to compare BTF proto tgr_prog_fd+tgt_btf_id
with EXT's btf_id only (and check tgt_prog_fd->type/expected_attach_type).
Since EXT was loaded earlier it has valid insns.
So if you're thinking "cannot we validate insns at load time, but then remember
tgt stuff instead of creating a partial link, and double validate BTF at raw_tp_open
when it's called without tgt_prog_fd?"
The answer is "yes, we can", but double validation of BTF I think is just a waste of cycles,
when tgt prog could have been held a bit between load and attach.
And it's race free. Whereas if we remember target prog_id at load then raw_tp_open is
shooting in the dark. Unlikely, but that prog_id could have been reused.

Re: [PATCH bpf-next v2 3/6] bpf: support attaching freplace programs to multiple attach points

[Andrii Nakryiko]

On Tue, Jul 21, 2020 at 5:29 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Mon, Jul 20, 2020 at 08:48:04PM -0700, Andrii Nakryiko wrote:
> >
> > Right, I wanted to avoid taking a refcnt on aux->linked_prog during
> > PROG_LOAD. The reason for that was (and still is) that I don't get who
> > and when has to bpf_prog_put() original aux->linked_prog to allow the
> > prog X to be freed. I.e., after you re-attach to prog Y, how prog X is
> > released (assuming no active bpf_link is keeping it from being freed)?
> > That's my biggest confusion right now.
> >
> > I also didn't like the idea of half-creating bpf_tracing_link on
> > PROG_LOAD and then turning it into a real link with bpf_link_settle on
> > attach. That sounded like a hack to me.
>
> The link is kinda already created during prog_load of EXT type.
> Typically prog_load needs expected_attach_type that points to something
> that is not going to disappear. In case of EXT progs the situation is different,
> since the target can be unloaded. So the prog load cmd not only validates the
> program extension but links target and ext prog together at the same time.
> The target prog will be held until EXT prog is unloaded.
> I think it's important to preserve this semantics to the users that the target prog
> is frozen at load time and no races are going to happen later.
> Otherwise it leads to double validation at attach time and races.

Yes, I was confused because of the step you describe below (removal of
linked_prog from aux->linked_prog and moving it into BPF link on
attach). With that move, it makes sense to have that bpf_prog refcnt
bump on load, makes everything simpler.

>
> What raw_tp_open is doing right now is a hack. It allocates bpf_tracing_link,
> registers it into link_idr and activates trampoline, but in reality that link is already there.

That's an interesting way to look at this. For me it always felt
normal, because real linking is happening inside
bpf_trampoline_link_prog(). But it's a minor technicality, it's not
important enough to discuss.

> I think we can clean it up by creating bpf_tracing_link at prog load time.
> Whether to register it at that time into link_idr is up to discussion.
> (I think probably not).

yeah, I agree, let's not


> Then raw_tp_open will activate that allocated bpf_tracing_link via trampoline,
> _remove_ it from aux->linked_tracing_link (old linked_prog) and
> return FD to the user.

Ok, so this move from aux->linked_prog into BPF link itself is what
was missing, I wasn't sure whether you proposed doing that. With that
it makes more sense, even if it's a bit "asymmetrical" in that you can
attach only once using old-style EXT attach, but can attach and
re-attach many times if you specify tgt_prog_fd. But I think it's also
fine, I just wish we always required tgt_prog_fd...

> So this partially created link at load_time will become complete link and
> close of the link will detach EXT from the target and the target can be unloaded.
> (Currently the target cannot be unloaded until EXT is loaded which is not great).
> The EXT_prog->aux->linked_tracing_link (old linked_prog) will exist only during
> the time between prog_load and raw_tp_open without args.
> I think that would be a good clean up.

yep, I agree

> Then multi attach of EXT progs is clean too.
> New raw_tp_open with tgt_prog_fd/tgt_btf_id will validate EXT against the new target,
> link them via new bpf_tracing_link, activate it via trampoline and return FD.
> No link list anywhere.
> Note that this second validation of EXT against new target is light weight comparing
> to the load. The first load goes through all EXT instructions with verifier ctx of
> the target prog. The second validation needs to compare BTF proto tgr_prog_fd+tgt_btf_id
> with EXT's btf_id only (and check tgt_prog_fd->type/expected_attach_type).
> Since EXT was loaded earlier it has valid insns.

Right, this matches what I understood about this re-attach logic, great.

> So if you're thinking "cannot we validate insns at load time, but then remember
> tgt stuff instead of creating a partial link, and double validate BTF at raw_tp_open
> when it's called without tgt_prog_fd?"
> The answer is "yes, we can", but double validation of BTF I think is just a waste of cycles,
> when tgt prog could have been held a bit between load and attach.
> And it's race free. Whereas if we remember target prog_id at load then raw_tp_open is
> shooting in the dark. Unlikely, but that prog_id could have been reused.

Sure, I agree that there is no need to complicate everything with ID
(now that I understand the proposal better). My confusion came from
two things:

1. Current API usage would allow PROG_LOAD of EXT program, would take
refcnt on target program. RAW_TP_OPEN + close link to detach. Then, if
necessary again RAW_TP_OPEN, and the second (and subsequent times)
would succeed. But it seems like we are changing that to only allow
one RAW_TP_OPEN if one doesn't provide tgt_prog_fd. I think it's
acceptable, but it wasn't clear to me.

2. You were talking about turning aux->linked_prog into a linked list
of bpf_tracing_links, but I couldn't see the point. In your latest
version you didn't talk about this list of links, so it seems like
that's not necessary after all, right? I like that.


So I think we are in agreement overall.

Just one technical moment, let me double-check my understanding again.
You seem to be favoring pre-creating bpf_tracing_link because there is
both tgt_prog (that we refcnt on EXT prog load) and we also lookup and
initialize trampoline in check_attach_btf_id(). Of course there is
also expected_attach_type, but that's a trivial known enum, so I'm
ignoring it. So because we have those two entities which on attach are
supposed to be owned by bpf_tracing_link, you just want to pre-create
a "shell" of bpf_tracing_link, and then on attach complete its
initialization, is that right? That certainly simplifies attach logic
a bit and I think it's fine.

But also it seems like we'll be creating and initializing a
**different** trampoline on re-attach to prog Y. Now attach will do
different things depending on whether tgt_prog_fd is provided or not.
So I wonder why not just unify this trampoline initialization and do
it at attach time? For all valid EXT use cases today the result is the
same: everything still works the same. For cases where we for some
reason can't initialize bpf_trampoline, that failure will happen at
attach time, not on a load time. But that seems fine, because that's
going to be the case for re-attach (with tgt_prog_fd) anyways. Looking
through the verifier code, it doesn't seem like it does anything much
with prog->aux->trampoline, unless I missed something, so it must be
ok to do it after load? It also seems to avoid this double BTF
validation concern you have, no? Thoughts?

Regardless, thanks for elaborating, I think I get it end-to-end now.

Re: [PATCH bpf-next v2 3/6] bpf: support attaching freplace programs to multiple attach points

[Alexei Starovoitov]

On Tue, Jul 21, 2020 at 11:02:04PM -0700, Andrii Nakryiko wrote:
> 
> Just one technical moment, let me double-check my understanding again.
> You seem to be favoring pre-creating bpf_tracing_link because there is
> both tgt_prog (that we refcnt on EXT prog load) and we also lookup and
> initialize trampoline in check_attach_btf_id(). Of course there is
> also expected_attach_type, but that's a trivial known enum, so I'm
> ignoring it. So because we have those two entities which on attach are
> supposed to be owned by bpf_tracing_link, you just want to pre-create
> a "shell" of bpf_tracing_link, and then on attach complete its
> initialization, is that right? That certainly simplifies attach logic
> a bit and I think it's fine.

Right. It just feels cleaner to group objects for the same purpose.

> But also it seems like we'll be creating and initializing a
> **different** trampoline on re-attach to prog Y. Now attach will do
> different things depending on whether tgt_prog_fd is provided or not.

Right, but it can be a common helper instead that is creating a 'shell'
of bpf_tracing_link.
Calling it from prog_load and from raw_tp_open is imo clean enough.
No copy paste of code.
If that was the concern.

> So I wonder why not just unify this trampoline initialization and do
> it at attach time? For all valid EXT use cases today the result is the
> same: everything still works the same. For cases where we for some
> reason can't initialize bpf_trampoline, that failure will happen at
> attach time, not on a load time. But that seems fine, because that's
> going to be the case for re-attach (with tgt_prog_fd) anyways. Looking
> through the verifier code, it doesn't seem like it does anything much
> with prog->aux->trampoline, unless I missed something, so it must be
> ok to do it after load? It also seems to avoid this double BTF
> validation concern you have, no? Thoughts?

bpf_trampoline_link_prog() is attach time call.
but bpf_trampoline_lookup() is one to one with the target.
When load_prog holds the target it's a right time to prep all things
about the target. Notice that key into trampoline_lookup() is
key = ((u64)aux->id) << 32 | btf_id;
of the target prog.
Can it be done at raw_tp_open time?
I guess so, but feels kinda weird to me to split the target preparation
job into several places.

Re: [PATCH bpf-next v2 3/6] bpf: support attaching freplace programs to multiple attach points

[Andrii Nakryiko]

On Wed, Jul 22, 2020 at 5:32 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Tue, Jul 21, 2020 at 11:02:04PM -0700, Andrii Nakryiko wrote:
> >
> > Just one technical moment, let me double-check my understanding again.
> > You seem to be favoring pre-creating bpf_tracing_link because there is
> > both tgt_prog (that we refcnt on EXT prog load) and we also lookup and
> > initialize trampoline in check_attach_btf_id(). Of course there is
> > also expected_attach_type, but that's a trivial known enum, so I'm
> > ignoring it. So because we have those two entities which on attach are
> > supposed to be owned by bpf_tracing_link, you just want to pre-create
> > a "shell" of bpf_tracing_link, and then on attach complete its
> > initialization, is that right? That certainly simplifies attach logic
> > a bit and I think it's fine.
>
> Right. It just feels cleaner to group objects for the same purpose.
>
> > But also it seems like we'll be creating and initializing a
> > **different** trampoline on re-attach to prog Y. Now attach will do
> > different things depending on whether tgt_prog_fd is provided or not.
>
> Right, but it can be a common helper instead that is creating a 'shell'
> of bpf_tracing_link.
> Calling it from prog_load and from raw_tp_open is imo clean enough.
> No copy paste of code.
> If that was the concern.
>
> > So I wonder why not just unify this trampoline initialization and do
> > it at attach time? For all valid EXT use cases today the result is the
> > same: everything still works the same. For cases where we for some
> > reason can't initialize bpf_trampoline, that failure will happen at
> > attach time, not on a load time. But that seems fine, because that's
> > going to be the case for re-attach (with tgt_prog_fd) anyways. Looking
> > through the verifier code, it doesn't seem like it does anything much
> > with prog->aux->trampoline, unless I missed something, so it must be
> > ok to do it after load? It also seems to avoid this double BTF
> > validation concern you have, no? Thoughts?
>
> bpf_trampoline_link_prog() is attach time call.
> but bpf_trampoline_lookup() is one to one with the target.
> When load_prog holds the target it's a right time to prep all things
> about the target. Notice that key into trampoline_lookup() is
> key = ((u64)aux->id) << 32 | btf_id;
> of the target prog.
> Can it be done at raw_tp_open time?
> I guess so, but feels kinda weird to me to split the target preparation
> job into several places.

ok, sounds good to me