[PATCH bpf-next v8 00/11] bpf: Support multi-attach for freplace programs

[PATCH bpf-next v8 00/11] 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 patch fixes an issue that came up in review: The verifier will
currently allow MODIFY_RETURN tracing functions to attach to other BPF programs,
even though it is pretty clear from the commit messages introducing the
functionality that this was not the intention. This patch is included in the
series because the subsequent refactoring patches touch the same code.

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

Patch 4 moves prog_aux->linked_prog and the trampoline to be embedded in
bpf_tracing_link on attach, and freed by the link release logic, and introduces
a mutex to protect the writing of the pointers in prog->aux.

Based on these refactorings, it becomes pretty straight-forward to support
multiple-attach for freplace programs (patch 5). This is simply a matter of
creating a second bpf_tracing_link if a target is supplied. However, for API
consistency with other types of link attach, this option is added to the
BPF_LINK_CREATE API instead of extending bpf_raw_tracepoint_open().

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

Patches 7 is the accompanying libbpf update, and patches 8-11 are selftests:
patch 8 tests for the multi-freplace functionality itself; patch 9 is Jiri's
previous selftest for the fentry-to-freplace fix; patch 10 is a test for the
change introduced in patch 1, blocking MODIFY_RETURN functions from attaching to
other BPF programs; and finally, patch 11 removes MODIFY_RETURN functions from
the benchmark and test_overhead programs in selftests, as these were never
supposed to work in the first place.

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

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

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

Changelog:

v8:
  - Add a separate error message when trying to attach FMOD_REPLACE to tgt_prog
  - Better error messages in bpf_program__attach_freplace()
  - Don't lock mutex when setting tgt_* pointers in prog create and verifier
  - Remove fmod_ret programs from benchmarks in selftests (new patch 11)
  - Fix a few other nits in selftests

v7:
  - Add back missing ptype == prog->type check in link_create()
  - Use tracing_bpf_link_attach() instead of separate freplace_bpf_link_attach()
  - Don't break attachment of bpf_iters in libbpf (by clobbering link_create.iter_info)

v6:
  - Rebase to latest bpf-next
  - Simplify logic in bpf_tracing_prog_attach()
  - Don't create a new attach_type for link_create(), disambiguate on prog->type
    instead
  - Use raw_tracepoint_open() in libbpf bpf_program__attach_ftrace() if called
    with NULL target
  - Switch bpf_program__attach_ftrace() to take function name as parameter
    instead of btf_id
  - Add a patch disallowing MODIFY_RETURN programs from attaching to other BPF
    programs, and an accompanying selftest (patches 1 and 10)

v5:
  - Fix typo in inline function definition of bpf_trampoline_get()
  - Don't put bpf_tracing_link in prog->aux, use a mutex to protect tgt_prog and
    trampoline instead, and move them to the link on attach.
  - Restore Jiri as author of the last selftest patch

v4:
  - Cleanup the refactored check_attach_btf_id() to make the logic easier to follow
  - Fix cleanup paths for bpf_tracing_link
  - Use xchg() for removing the bpf_tracing_link from prog->aux and restore on (some) failures
  - Use BPF_LINK_CREATE operation to create link with target instead of extending raw_tracepoint_open
  - Fold update of tools/ UAPI header into main patch
  - Update arg dereference patch to use skeletons and set_attach_target()

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

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

---

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

Toke Høiland-Jørgensen (10):
      bpf: disallow attaching modify_return tracing functions to other BPF programs
      bpf: change logging calls from verbose() to bpf_log() and use log pointer
      bpf: verifier: refactor check_attach_btf_id()
      bpf: move prog->aux->linked_prog and trampoline into bpf_link on attach
      bpf: support attaching freplace programs to multiple attach points
      bpf: Fix context type resolving for extension programs
      libbpf: add support for freplace attachment in bpf_link_create
      selftests: add test for multiple attachments of freplace program
      selftests: Add selftest for disallowing modify_return attachment to freplace
      selftests: Remove fmod_ret from benchmarks and test_overhead


 include/linux/bpf.h                           |  26 +-
 include/linux/bpf_verifier.h                  |  14 +-
 include/uapi/linux/bpf.h                      |   9 +-
 kernel/bpf/btf.c                              |  21 +-
 kernel/bpf/core.c                             |   9 +-
 kernel/bpf/syscall.c                          | 135 ++++++++--
 kernel/bpf/trampoline.c                       |  32 ++-
 kernel/bpf/verifier.c                         | 250 ++++++++++--------
 tools/include/uapi/linux/bpf.h                |   9 +-
 tools/lib/bpf/bpf.c                           |  18 +-
 tools/lib/bpf/bpf.h                           |   3 +-
 tools/lib/bpf/libbpf.c                        |  44 ++-
 tools/lib/bpf/libbpf.h                        |   3 +
 tools/lib/bpf/libbpf.map                      |   1 +
 tools/testing/selftests/bpf/bench.c           |   5 -
 .../selftests/bpf/benchs/bench_rename.c       |  17 --
 .../selftests/bpf/benchs/bench_trigger.c      |  17 --
 .../selftests/bpf/prog_tests/fexit_bpf2bpf.c  | 212 ++++++++++++---
 .../selftests/bpf/prog_tests/test_overhead.c  |  14 +-
 .../selftests/bpf/prog_tests/trace_ext.c      | 111 ++++++++
 .../selftests/bpf/progs/fmod_ret_freplace.c   |  14 +
 .../bpf/progs/freplace_get_constant.c         |  15 ++
 .../selftests/bpf/progs/test_overhead.c       |   6 -
 .../selftests/bpf/progs/test_trace_ext.c      |  18 ++
 .../bpf/progs/test_trace_ext_tracing.c        |  25 ++
 .../selftests/bpf/progs/trigger_bench.c       |   7 -
 26 files changed, 771 insertions(+), 264 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/trace_ext.c
 create mode 100644 tools/testing/selftests/bpf/progs/fmod_ret_freplace.c
 create mode 100644 tools/testing/selftests/bpf/progs/freplace_get_constant.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_trace_ext.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_trace_ext_tracing.c

[PATCH bpf-next v8 01/11] bpf: disallow attaching modify_return tracing functions to other BPF programs

[Toke Høiland-Jørgensen]

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

From the checks and commit messages for modify_return, it seems it was
never the intention that it should be possible to attach a tracing program
with expected_attach_type == BPF_MODIFY_RETURN to another BPF program.
However, check_attach_modify_return() will only look at the function name,
so if the target function starts with "security_", the attach will be
allowed even for bpf2bpf attachment.

Fix this oversight by also blocking the modification if a target program is
supplied.

Fixes: 18644cec714a ("bpf: Fix use-after-free in fmod_ret check")
Fixes: 6ba43b761c41 ("bpf: Attachment verification for BPF_MODIFY_RETURN")
Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
 kernel/bpf/verifier.c |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 15ab889b0a3f..797e2b0d8bc2 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -11471,6 +11471,11 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)
 				verbose(env, "%s is not sleepable\n",
 					prog->aux->attach_func_name);
 		} else if (prog->expected_attach_type == BPF_MODIFY_RETURN) {
+			if (tgt_prog) {
+				verbose(env, "can't modify return codes of BPF programs\n");
+				ret = -EINVAL;
+				goto out;
+			}
 			ret = check_attach_modify_return(prog, addr);
 			if (ret)
 				verbose(env, "%s() is not modifiable\n",

[PATCH bpf-next v8 02/11] 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.

Acked-by: Andrii Nakryiko <andriin@fb.com>
Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
 include/linux/bpf.h          |    2 +-
 include/linux/bpf_verifier.h |    5 +++-
 kernel/bpf/btf.c             |    6 +++--
 kernel/bpf/verifier.c        |   50 +++++++++++++++++++++---------------------
 4 files changed, 32 insertions(+), 31 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index fc5c901c7542..1f9e7c22cc7e 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1402,7 +1402,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 2bb48a2c4d08..7bc9276c4ef4 100644
--- a/include/linux/bpf_verifier.h
+++ b/include/linux/bpf_verifier.h
@@ -347,8 +347,9 @@ static inline bool bpf_verifier_log_full(const struct bpf_verifier_log *log)
 
 static inline bool bpf_verifier_log_needed(const struct bpf_verifier_log *log)
 {
-	return (log->level && log->ubuf && !bpf_verifier_log_full(log)) ||
-		log->level == BPF_LOG_KERNEL;
+	return log &&
+		((log->level && log->ubuf && !bpf_verifier_log_full(log)) ||
+		 log->level == BPF_LOG_KERNEL);
 }
 
 #define BPF_MAX_SUBPROGS 256
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 5d3c36e13139..868c03a24d0a 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -4388,7 +4388,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;
@@ -4396,7 +4396,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;
 	}
 
@@ -4408,7 +4408,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 797e2b0d8bc2..81e1bdc492f8 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -11220,6 +11220,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;
@@ -11247,23 +11248,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) {
@@ -11275,18 +11276,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;
 			}
@@ -11294,7 +11295,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) {
@@ -11302,7 +11303,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 &&
@@ -11324,13 +11325,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;
@@ -11339,17 +11340,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;
 		}
@@ -11372,7 +11373,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;
 		}
@@ -11383,8 +11384,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)
@@ -11396,18 +11396,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))
@@ -11426,7 +11426,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;
@@ -11438,7 +11438,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;
@@ -11468,17 +11468,17 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)
 				break;
 			}
 			if (ret)
-				verbose(env, "%s is not sleepable\n",
+				bpf_log(log, "%s is not sleepable\n",
 					prog->aux->attach_func_name);
 		} else if (prog->expected_attach_type == BPF_MODIFY_RETURN) {
 			if (tgt_prog) {
-				verbose(env, "can't modify return codes of BPF programs\n");
+				bpf_log(log, "can't modify return codes of BPF programs\n");
 				ret = -EINVAL;
 				goto out;
 			}
 			ret = check_attach_modify_return(prog, addr);
 			if (ret)
-				verbose(env, "%s() is not modifiable\n",
+				bpf_log(log, "%s() is not modifiable\n",
 					prog->aux->attach_func_name);
 		}
 		if (ret)

[PATCH bpf-next v8 03/11] 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.

This also fixes a bug where a bunch of checks were skipped if a trampoline
already existed for the tracing target.

Fixes: 6ba43b761c41 ("bpf: Attachment verification for BPF_MODIFY_RETURN")
Fixes: 1e6c62a88215 ("bpf: Introduce sleepable BPF programs")
Acked-by: Andrii Nakryiko <andriin@fb.com>
Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
 include/linux/bpf.h          |    7 +
 include/linux/bpf_verifier.h |    9 ++
 kernel/bpf/trampoline.c      |   20 ++++
 kernel/bpf/verifier.c        |  200 ++++++++++++++++++++++++------------------
 4 files changed, 150 insertions(+), 86 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 1f9e7c22cc7e..a1760fd87815 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -621,6 +621,8 @@ 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);
+struct bpf_trampoline *bpf_trampoline_get(u64 key, void *addr,
+					  struct btf_func_model *fmodel);
 void bpf_trampoline_put(struct bpf_trampoline *tr);
 #define BPF_DISPATCHER_INIT(_name) {				\
 	.mutex = __MUTEX_INITIALIZER(_name.mutex),		\
@@ -677,6 +679,11 @@ static inline int bpf_trampoline_unlink_prog(struct bpf_prog *prog)
 {
 	return -ENOTSUPP;
 }
+static inline struct bpf_trampoline *bpf_trampoline_get(u64 key, void *addr,
+							struct btf_func_model *fmodel)
+{
+	return ERR_PTR(-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 7bc9276c4ef4..4fe718a5b4cd 100644
--- a/include/linux/bpf_verifier.h
+++ b/include/linux/bpf_verifier.h
@@ -450,4 +450,13 @@ bpf_prog_offload_remove_insns(struct bpf_verifier_env *env, u32 off, u32 cnt);
 int check_ctx_reg(struct bpf_verifier_env *env,
 		  const struct bpf_reg_state *reg, int regno);
 
+int bpf_check_attach_target(struct bpf_verifier_log *log,
+			    const struct bpf_prog *prog,
+			    const struct bpf_prog *tgt_prog,
+			    u32 btf_id,
+			    struct btf_func_model *fmodel,
+			    long *tgt_addr,
+			    const char **tgt_name,
+			    const struct btf_type **tgt_type);
+
 #endif /* _LINUX_BPF_VERIFIER_H */
diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
index 7dd523a7e32d..e86d32f7f7dc 100644
--- a/kernel/bpf/trampoline.c
+++ b/kernel/bpf/trampoline.c
@@ -336,6 +336,26 @@ int bpf_trampoline_unlink_prog(struct bpf_prog *prog)
 	return err;
 }
 
+struct bpf_trampoline *bpf_trampoline_get(u64 key, void *addr,
+					  struct btf_func_model *fmodel)
+{
+	struct bpf_trampoline *tr;
+
+	tr = bpf_trampoline_lookup(key);
+	if (!tr)
+		return NULL;
+
+	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);
+	return tr;
+}
+
 void bpf_trampoline_put(struct bpf_trampoline *tr)
 {
 	if (!tr)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 81e1bdc492f8..ad244a606d7a 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -11174,11 +11174,11 @@ static int check_struct_ops_btf_id(struct bpf_verifier_env *env)
 }
 #define SECURITY_PREFIX "security_"
 
-static int check_attach_modify_return(struct bpf_prog *prog, unsigned long addr)
+static int check_attach_modify_return(const struct bpf_prog *prog, unsigned long addr,
+				      const char *func_name)
 {
 	if (within_error_injection_list(addr) ||
-	    !strncmp(SECURITY_PREFIX, prog->aux->attach_func_name,
-		     sizeof(SECURITY_PREFIX) - 1))
+	    !strncmp(SECURITY_PREFIX, func_name, sizeof(SECURITY_PREFIX) - 1))
 		return 0;
 
 	return -EINVAL;
@@ -11215,43 +11215,29 @@ static int check_non_sleepable_error_inject(u32 btf_id)
 	return btf_id_set_contains(&btf_non_sleepable_error_inject, btf_id);
 }
 
-static int check_attach_btf_id(struct bpf_verifier_env *env)
+int bpf_check_attach_target(struct bpf_verifier_log *log,
+			    const struct bpf_prog *prog,
+			    const struct bpf_prog *tgt_prog,
+			    u32 btf_id,
+			    struct btf_func_model *fmodel,
+			    long *tgt_addr,
+			    const char **tgt_name,
+			    const struct btf_type **tgt_type)
 {
-	struct bpf_prog *prog = env->prog;
 	bool prog_extension = prog->type == BPF_PROG_TYPE_EXT;
-	struct bpf_prog *tgt_prog = prog->aux->linked_prog;
-	struct bpf_verifier_log *log = &env->log;
-	u32 btf_id = prog->aux->attach_btf_id;
 	const char prefix[] = "btf_trace_";
-	struct btf_func_model fmodel;
 	int ret = 0, subprog = -1, i;
-	struct bpf_trampoline *tr;
 	const struct btf_type *t;
 	bool conservative = true;
 	const char *tname;
 	struct btf *btf;
-	long addr;
-	u64 key;
-
-	if (prog->aux->sleepable && prog->type != BPF_PROG_TYPE_TRACING &&
-	    prog->type != BPF_PROG_TYPE_LSM) {
-		verbose(env, "Only fentry/fexit/fmod_ret and lsm programs can be sleepable\n");
-		return -EINVAL;
-	}
-
-	if (prog->type == BPF_PROG_TYPE_STRUCT_OPS)
-		return check_struct_ops_btf_id(env);
-
-	if (prog->type != BPF_PROG_TYPE_TRACING &&
-	    prog->type != BPF_PROG_TYPE_LSM &&
-	    !prog_extension)
-		return 0;
+	long addr = 0;
 
 	if (!btf_id) {
 		bpf_log(log, "Tracing programs must provide btf_id\n");
 		return -EINVAL;
 	}
-	btf = bpf_prog_get_target_btf(prog);
+	btf = tgt_prog ? tgt_prog->aux->btf : btf_vmlinux;
 	if (!btf) {
 		bpf_log(log,
 			"FENTRY/FEXIT program can only be attached to another program annotated with BTF\n");
@@ -11291,8 +11277,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");
@@ -11328,13 +11312,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) {
@@ -11364,13 +11346,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",
@@ -11380,12 +11356,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;
@@ -11394,13 +11368,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);
@@ -11412,24 +11379,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;
@@ -11441,8 +11398,7 @@ 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;
 			}
 		}
 
@@ -11467,30 +11423,102 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)
 			default:
 				break;
 			}
-			if (ret)
-				bpf_log(log, "%s is not sleepable\n",
-					prog->aux->attach_func_name);
+			if (ret) {
+				bpf_log(log, "%s is not sleepable\n", tname);
+				return ret;
+			}
 		} else if (prog->expected_attach_type == BPF_MODIFY_RETURN) {
 			if (tgt_prog) {
 				bpf_log(log, "can't modify return codes of BPF programs\n");
-				ret = -EINVAL;
-				goto out;
+				return -EINVAL;
+			}
+			ret = check_attach_modify_return(prog, addr, tname);
+			if (ret) {
+				bpf_log(log, "%s() is not modifiable\n", tname);
+				return ret;
 			}
-			ret = check_attach_modify_return(prog, addr);
-			if (ret)
-				bpf_log(log, "%s() is not modifiable\n",
-					prog->aux->attach_func_name);
 		}
-		if (ret)
-			goto out;
-		tr->func.addr = (void *)addr;
-		prog->aux->trampoline = tr;
-out:
-		mutex_unlock(&tr->mutex);
-		if (ret)
-			bpf_trampoline_put(tr);
+
+		break;
+	}
+	*tgt_addr = addr;
+	if (tgt_name)
+		*tgt_name = tname;
+	if (tgt_type)
+		*tgt_type = t;
+	return 0;
+}
+
+static int check_attach_btf_id(struct bpf_verifier_env *env)
+{
+	struct bpf_prog *prog = env->prog;
+	struct bpf_prog *tgt_prog = prog->aux->linked_prog;
+	u32 btf_id = prog->aux->attach_btf_id;
+	struct btf_func_model fmodel;
+	struct bpf_trampoline *tr;
+	const struct btf_type *t;
+	const char *tname;
+	long addr;
+	int ret;
+	u64 key;
+
+	if (prog->aux->sleepable && prog->type != BPF_PROG_TYPE_TRACING &&
+	    prog->type != BPF_PROG_TYPE_LSM) {
+		verbose(env, "Only fentry/fexit/fmod_ret and lsm programs can be sleepable\n");
+		return -EINVAL;
+	}
+
+	if (prog->type == BPF_PROG_TYPE_STRUCT_OPS)
+		return check_struct_ops_btf_id(env);
+
+	if (prog->type != BPF_PROG_TYPE_TRACING &&
+	    prog->type != BPF_PROG_TYPE_LSM &&
+	    prog->type != BPF_PROG_TYPE_EXT)
+		return 0;
+
+	ret = bpf_check_attach_target(&env->log, prog, tgt_prog, btf_id,
+				      &fmodel, &addr, &tname, &t);
+	if (ret)
 		return ret;
+
+	if (tgt_prog) {
+		if (prog->type == BPF_PROG_TYPE_EXT) {
+			env->ops = bpf_verifier_ops[tgt_prog->type];
+			prog->expected_attach_type =
+				tgt_prog->expected_attach_type;
+		}
+		key = ((u64)tgt_prog->aux->id) << 32 | btf_id;
+	} else {
+		key = btf_id;
 	}
+
+	/* remember two read only pointers that are valid for
+	 * the life time of the kernel
+	 */
+	prog->aux->attach_func_proto = t;
+	prog->aux->attach_func_name = tname;
+
+	if (prog->expected_attach_type == BPF_TRACE_RAW_TP) {
+		prog->aux->attach_btf_trace = true;
+		return 0;
+	} else if (prog->expected_attach_type == BPF_TRACE_ITER) {
+		if (!bpf_iter_prog_supported(prog))
+			return -EINVAL;
+		return 0;
+	}
+
+	if (prog->type == BPF_PROG_TYPE_LSM) {
+		ret = bpf_lsm_verify_prog(&env->log, prog);
+		if (ret < 0)
+			return ret;
+	}
+
+	tr = bpf_trampoline_get(key, (void *)addr, &fmodel);
+	if (!tr)
+		return -ENOMEM;
+
+	prog->aux->trampoline = tr;
+	return 0;
 }
 
 int bpf_check(struct bpf_prog **prog, union bpf_attr *attr,

[PATCH bpf-next v8 04/11] bpf: move prog->aux->linked_prog and trampoline into bpf_link on attach

[Toke Høiland-Jørgensen]

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

In preparation for allowing multiple attachments of freplace programs, move
the references to the target program and trampoline into the
bpf_tracing_link structure when that is created. To do this atomically,
introduce a new mutex in prog->aux to protect writing to the two pointers
to target prog and trampoline, and rename the members to make it clear that
they are related.

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

Acked-by: Andrii Nakryiko <andriin@fb.com>
Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
 include/linux/bpf.h     |   15 +++++++++------
 kernel/bpf/btf.c        |    6 +++---
 kernel/bpf/core.c       |    9 ++++++---
 kernel/bpf/syscall.c    |   47 +++++++++++++++++++++++++++++++++++++++--------
 kernel/bpf/trampoline.c |   12 ++++--------
 kernel/bpf/verifier.c   |    7 +++----
 6 files changed, 64 insertions(+), 32 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index a1760fd87815..f0fc110ac0fb 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -619,8 +619,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);
 struct bpf_trampoline *bpf_trampoline_get(u64 key, void *addr,
 					  struct btf_func_model *fmodel);
 void bpf_trampoline_put(struct bpf_trampoline *tr);
@@ -671,11 +671,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;
 }
@@ -746,7 +748,9 @@ struct bpf_prog_aux {
 	u32 max_rdonly_access;
 	u32 max_rdwr_access;
 	const struct bpf_ctx_arg_aux *ctx_arg_info;
-	struct bpf_prog *linked_prog;
+	struct mutex tgt_mutex; /* protects tgt_* pointers below, *after* prog becomes visible */
+	struct bpf_prog *tgt_prog;
+	struct bpf_trampoline *tgt_trampoline;
 	bool verifier_zext; /* Zero extensions has been inserted by verifier. */
 	bool offload_requested;
 	bool attach_btf_trace; /* true if attaching to BTF-enabled raw tp */
@@ -754,7 +758,6 @@ struct bpf_prog_aux {
 	bool sleepable;
 	bool tail_call_reachable;
 	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;
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 868c03a24d0a..76cc6ae46821 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -3706,7 +3706,7 @@ struct btf *btf_parse_vmlinux(void)
 
 struct btf *bpf_prog_get_target_btf(const struct bpf_prog *prog)
 {
-	struct bpf_prog *tgt_prog = prog->aux->linked_prog;
+	struct bpf_prog *tgt_prog = prog->aux->tgt_prog;
 
 	if (tgt_prog) {
 		return tgt_prog->aux->btf;
@@ -3733,7 +3733,7 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
 		    struct bpf_insn_access_aux *info)
 {
 	const struct btf_type *t = prog->aux->attach_func_proto;
-	struct bpf_prog *tgt_prog = prog->aux->linked_prog;
+	struct bpf_prog *tgt_prog = prog->aux->tgt_prog;
 	struct btf *btf = bpf_prog_get_target_btf(prog);
 	const char *tname = prog->aux->attach_func_name;
 	struct bpf_verifier_log *log = info->log;
@@ -4559,7 +4559,7 @@ int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog,
 		return -EFAULT;
 	}
 	if (prog_type == BPF_PROG_TYPE_EXT)
-		prog_type = prog->aux->linked_prog->type;
+		prog_type = prog->aux->tgt_prog->type;
 
 	t = btf_type_by_id(btf, t->type);
 	if (!t || !btf_type_is_func_proto(t)) {
diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index c4811b139caa..0eb5f7501e29 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -99,6 +99,7 @@ struct bpf_prog *bpf_prog_alloc_no_stats(unsigned int size, gfp_t gfp_extra_flag
 
 	INIT_LIST_HEAD_RCU(&fp->aux->ksym.lnode);
 	mutex_init(&fp->aux->used_maps_mutex);
+	mutex_init(&fp->aux->tgt_mutex);
 
 	return fp;
 }
@@ -255,6 +256,7 @@ void __bpf_prog_free(struct bpf_prog *fp)
 {
 	if (fp->aux) {
 		mutex_destroy(&fp->aux->used_maps_mutex);
+		mutex_destroy(&fp->aux->tgt_mutex);
 		free_percpu(fp->aux->stats);
 		kfree(fp->aux->poke_tab);
 		kfree(fp->aux);
@@ -2138,7 +2140,8 @@ 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);
+	if (aux->tgt_trampoline)
+		bpf_trampoline_put(aux->tgt_trampoline);
 	for (i = 0; i < aux->func_cnt; i++)
 		bpf_jit_free(aux->func[i]);
 	if (aux->func_cnt) {
@@ -2154,8 +2157,8 @@ void bpf_prog_free(struct bpf_prog *fp)
 {
 	struct bpf_prog_aux *aux = fp->aux;
 
-	if (aux->linked_prog)
-		bpf_prog_put(aux->linked_prog);
+	if (aux->tgt_prog)
+		bpf_prog_put(aux->tgt_prog);
 	INIT_WORK(&aux->work, bpf_prog_free_deferred);
 	schedule_work(&aux->work);
 }
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index ec68d3a23a2b..a2db33f4753e 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -2161,7 +2161,7 @@ static int bpf_prog_load(union bpf_attr *attr, union bpf_attr __user *uattr)
 			err = PTR_ERR(tgt_prog);
 			goto free_prog_nouncharge;
 		}
-		prog->aux->linked_prog = tgt_prog;
+		prog->aux->tgt_prog = tgt_prog;
 	}
 
 	prog->aux->offload_requested = !!attr->prog_ifindex;
@@ -2494,11 +2494,22 @@ 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)
@@ -2541,7 +2552,9 @@ static const struct bpf_link_ops bpf_tracing_link_lops = {
 static int bpf_tracing_prog_attach(struct bpf_prog *prog)
 {
 	struct bpf_link_primer link_primer;
+	struct bpf_prog *tgt_prog = NULL;
 	struct bpf_tracing_link *link;
+	struct bpf_trampoline *tr;
 	int err;
 
 	switch (prog->type) {
@@ -2579,19 +2592,37 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog)
 		      &bpf_tracing_link_lops, prog);
 	link->attach_type = prog->expected_attach_type;
 
-	err = bpf_link_prime(&link->link, &link_primer);
-	if (err) {
-		kfree(link);
-		goto out_put_prog;
+	mutex_lock(&prog->aux->tgt_mutex);
+
+	if (!prog->aux->tgt_trampoline) {
+		err = -ENOENT;
+		goto out_unlock;
 	}
+	tr = prog->aux->tgt_trampoline;
+	tgt_prog = prog->aux->tgt_prog;
+
+	err = bpf_link_prime(&link->link, &link_primer);
+	if (err)
+		goto out_unlock;
 
-	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 = NULL;
+		goto out_unlock;
 	}
 
+	link->tgt_prog = tgt_prog;
+	link->trampoline = tr;
+
+	prog->aux->tgt_prog = NULL;
+	prog->aux->tgt_trampoline = NULL;
+	mutex_unlock(&prog->aux->tgt_mutex);
+
 	return bpf_link_settle(&link_primer);
+out_unlock:
+	mutex_unlock(&prog->aux->tgt_mutex);
+	kfree(link);
 out_put_prog:
 	bpf_prog_put(prog);
 	return err;
diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
index e86d32f7f7dc..3145615647a5 100644
--- a/kernel/bpf/trampoline.c
+++ b/kernel/bpf/trampoline.c
@@ -261,14 +261,12 @@ static enum bpf_tramp_prog_type bpf_attach_type_to_tramp(struct bpf_prog *prog)
 	}
 }
 
-int bpf_trampoline_link_prog(struct bpf_prog *prog)
+int bpf_trampoline_link_prog(struct bpf_prog *prog, struct bpf_trampoline *tr)
 {
 	enum bpf_tramp_prog_type kind;
-	struct bpf_trampoline *tr;
 	int err = 0;
 	int cnt;
 
-	tr = prog->aux->trampoline;
 	kind = bpf_attach_type_to_tramp(prog);
 	mutex_lock(&tr->mutex);
 	if (tr->extension_prog) {
@@ -301,7 +299,7 @@ int bpf_trampoline_link_prog(struct bpf_prog *prog)
 	}
 	hlist_add_head(&prog->aux->tramp_hlist, &tr->progs_hlist[kind]);
 	tr->progs_cnt[kind]++;
-	err = bpf_trampoline_update(prog->aux->trampoline);
+	err = bpf_trampoline_update(tr);
 	if (err) {
 		hlist_del(&prog->aux->tramp_hlist);
 		tr->progs_cnt[kind]--;
@@ -312,13 +310,11 @@ int bpf_trampoline_link_prog(struct bpf_prog *prog)
 }
 
 /* bpf_trampoline_unlink_prog() should never fail. */
-int bpf_trampoline_unlink_prog(struct bpf_prog *prog)
+int bpf_trampoline_unlink_prog(struct bpf_prog *prog, struct bpf_trampoline *tr)
 {
 	enum bpf_tramp_prog_type kind;
-	struct bpf_trampoline *tr;
 	int err;
 
-	tr = prog->aux->trampoline;
 	kind = bpf_attach_type_to_tramp(prog);
 	mutex_lock(&tr->mutex);
 	if (kind == BPF_TRAMP_REPLACE) {
@@ -330,7 +326,7 @@ int bpf_trampoline_unlink_prog(struct bpf_prog *prog)
 	}
 	hlist_del(&prog->aux->tramp_hlist);
 	tr->progs_cnt[kind]--;
-	err = bpf_trampoline_update(prog->aux->trampoline);
+	err = bpf_trampoline_update(tr);
 out:
 	mutex_unlock(&tr->mutex);
 	return err;
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index ad244a606d7a..647fac170f19 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -2643,8 +2643,7 @@ static int check_map_access(struct bpf_verifier_env *env, u32 regno,
 
 static enum bpf_prog_type resolve_prog_type(struct bpf_prog *prog)
 {
-	return prog->aux->linked_prog ? prog->aux->linked_prog->type
-				      : prog->type;
+	return prog->aux->tgt_prog ? prog->aux->tgt_prog->type : prog->type;
 }
 
 static bool may_access_direct_pkt_data(struct bpf_verifier_env *env,
@@ -11452,8 +11451,8 @@ int bpf_check_attach_target(struct bpf_verifier_log *log,
 static int check_attach_btf_id(struct bpf_verifier_env *env)
 {
 	struct bpf_prog *prog = env->prog;
-	struct bpf_prog *tgt_prog = prog->aux->linked_prog;
 	u32 btf_id = prog->aux->attach_btf_id;
+	struct bpf_prog *tgt_prog = prog->aux->tgt_prog;
 	struct btf_func_model fmodel;
 	struct bpf_trampoline *tr;
 	const struct btf_type *t;
@@ -11517,7 +11516,7 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)
 	if (!tr)
 		return -ENOMEM;
 
-	prog->aux->trampoline = tr;
+	prog->aux->tgt_trampoline = tr;
 	return 0;
 }
 

[PATCH bpf-next v8 05/11] 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 the UAPI for bpf_link_Create with a target
btf ID that can be used to supply the new attachment point along with the
target program fd. The target must be compatible with the target that was
supplied at program load time.

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

The code could theoretically support multiple-attach of other types of
tracing programs as well, but since I don't have a use case for any of
those, there is no API support for doing so.

Acked-by: Andrii Nakryiko <andriin@fb.com>
Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
 include/linux/bpf.h            |    2 +
 include/uapi/linux/bpf.h       |    9 +++-
 kernel/bpf/syscall.c           |  102 +++++++++++++++++++++++++++++++++-------
 kernel/bpf/verifier.c          |    9 ++++
 tools/include/uapi/linux/bpf.h |    9 +++-
 5 files changed, 108 insertions(+), 23 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index f0fc110ac0fb..dfbab195a166 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -751,6 +751,8 @@ struct bpf_prog_aux {
 	struct mutex tgt_mutex; /* protects tgt_* pointers below, *after* prog becomes visible */
 	struct bpf_prog *tgt_prog;
 	struct bpf_trampoline *tgt_trampoline;
+	enum bpf_prog_type tgt_prog_type;
+	enum bpf_attach_type tgt_attach_type;
 	bool verifier_zext; /* Zero extensions has been inserted by verifier. */
 	bool offload_requested;
 	bool attach_btf_trace; /* true if attaching to BTF-enabled raw tp */
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index a22812561064..feff1ed49f86 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -632,8 +632,13 @@ union bpf_attr {
 		};
 		__u32		attach_type;	/* attach type */
 		__u32		flags;		/* extra flags */
-		__aligned_u64	iter_info;	/* extra bpf_iter_link_info */
-		__u32		iter_info_len;	/* iter_info length */
+		union {
+			__u32		target_btf_id;	/* btf_id of target to attach to */
+			struct {
+				__aligned_u64	iter_info;	/* extra bpf_iter_link_info */
+				__u32		iter_info_len;	/* iter_info length */
+			};
+		};
 	} link_create;
 
 	struct { /* struct used by BPF_LINK_UPDATE command */
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index a2db33f4753e..5671a99f1350 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>
@@ -2549,12 +2550,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_prog *tgt_prog = NULL;
+	struct bpf_trampoline *tr = NULL;
 	struct bpf_tracing_link *link;
-	struct bpf_trampoline *tr;
+	struct btf_func_model fmodel;
+	u64 key = 0;
+	long addr;
 	int err;
 
 	switch (prog->type) {
@@ -2583,6 +2589,28 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog)
 		goto out_put_prog;
 	}
 
+	if (!!tgt_prog_fd != !!btf_id) {
+		err = -EINVAL;
+		goto out_put_prog;
+	}
+
+	if (tgt_prog_fd) {
+		/* For now we only allow new targets for BPF_PROG_TYPE_EXT */
+		if (prog->type != BPF_PROG_TYPE_EXT) {
+			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;
+		}
+
+		key = ((u64)tgt_prog->aux->id) << 32 | btf_id;
+	}
+
 	link = kzalloc(sizeof(*link), GFP_USER);
 	if (!link) {
 		err = -ENOMEM;
@@ -2594,12 +2622,28 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog)
 
 	mutex_lock(&prog->aux->tgt_mutex);
 
-	if (!prog->aux->tgt_trampoline) {
+	if (!prog->aux->tgt_trampoline && !tgt_prog) {
 		err = -ENOENT;
 		goto out_unlock;
 	}
-	tr = prog->aux->tgt_trampoline;
-	tgt_prog = prog->aux->tgt_prog;
+
+	if (!prog->aux->tgt_trampoline ||
+	    (key && key != prog->aux->tgt_trampoline->key)) {
+
+		err = bpf_check_attach_target(NULL, prog, tgt_prog, btf_id,
+					      &fmodel, &addr, NULL, NULL);
+		if (err)
+			goto out_unlock;
+
+		tr = bpf_trampoline_get(key, (void *)addr, &fmodel);
+		if (!tr) {
+			err = -ENOMEM;
+			goto out_unlock;
+		}
+	} else {
+		tr = prog->aux->tgt_trampoline;
+		tgt_prog = prog->aux->tgt_prog;
+	}
 
 	err = bpf_link_prime(&link->link, &link_primer);
 	if (err)
@@ -2614,16 +2658,24 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog)
 
 	link->tgt_prog = tgt_prog;
 	link->trampoline = tr;
-
-	prog->aux->tgt_prog = NULL;
-	prog->aux->tgt_trampoline = NULL;
+	if (tr == prog->aux->tgt_trampoline) {
+		/* if we got a new ref from syscall, drop existing one from prog */
+		if (tgt_prog_fd)
+			bpf_prog_put(prog->aux->tgt_prog);
+		prog->aux->tgt_trampoline = NULL;
+		prog->aux->tgt_prog = NULL;
+	}
 	mutex_unlock(&prog->aux->tgt_mutex);
 
 	return bpf_link_settle(&link_primer);
 out_unlock:
+	if (tr && tr != prog->aux->tgt_trampoline)
+		bpf_trampoline_put(tr);
 	mutex_unlock(&prog->aux->tgt_mutex);
 	kfree(link);
 out_put_prog:
+	if (tgt_prog_fd && tgt_prog)
+		bpf_prog_put(tgt_prog);
 	bpf_prog_put(prog);
 	return err;
 }
@@ -2737,7 +2789,7 @@ 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, 0, 0);
 	case BPF_PROG_TYPE_RAW_TRACEPOINT:
 	case BPF_PROG_TYPE_RAW_TRACEPOINT_WRITABLE:
 		if (strncpy_from_user(buf,
@@ -3921,10 +3973,15 @@ static int bpf_map_do_batch(const union bpf_attr *attr,
 
 static int tracing_bpf_link_attach(const union bpf_attr *attr, struct bpf_prog *prog)
 {
-	if (attr->link_create.attach_type == BPF_TRACE_ITER &&
-	    prog->expected_attach_type == BPF_TRACE_ITER)
-		return bpf_iter_link_attach(attr, prog);
+	if (attr->link_create.attach_type != prog->expected_attach_type)
+		return -EINVAL;
 
+	if (prog->expected_attach_type == BPF_TRACE_ITER)
+		return bpf_iter_link_attach(attr, prog);
+	else if (prog->type == BPF_PROG_TYPE_EXT)
+		return bpf_tracing_prog_attach(prog,
+					       attr->link_create.target_fd,
+					       attr->link_create.target_btf_id);
 	return -EINVAL;
 }
 
@@ -3938,18 +3995,25 @@ static int link_create(union bpf_attr *attr)
 	if (CHECK_ATTR(BPF_LINK_CREATE))
 		return -EINVAL;
 
-	ptype = attach_type_to_prog_type(attr->link_create.attach_type);
-	if (ptype == BPF_PROG_TYPE_UNSPEC)
-		return -EINVAL;
-
-	prog = bpf_prog_get_type(attr->link_create.prog_fd, ptype);
+	prog = bpf_prog_get(attr->link_create.prog_fd);
 	if (IS_ERR(prog))
 		return PTR_ERR(prog);
 
 	ret = bpf_prog_attach_check_attach_type(prog,
 						attr->link_create.attach_type);
 	if (ret)
-		goto err_out;
+		goto out;
+
+	if (prog->type == BPF_PROG_TYPE_EXT) {
+		ret = tracing_bpf_link_attach(attr, prog);
+		goto out;
+	}
+
+	ptype = attach_type_to_prog_type(attr->link_create.attach_type);
+	if (ptype == BPF_PROG_TYPE_UNSPEC || ptype != prog->type) {
+		ret = -EINVAL;
+		goto out;
+	}
 
 	switch (ptype) {
 	case BPF_PROG_TYPE_CGROUP_SKB:
@@ -3977,7 +4041,7 @@ static int link_create(union bpf_attr *attr)
 		ret = -EINVAL;
 	}
 
-err_out:
+out:
 	if (ret < 0)
 		bpf_prog_put(prog);
 	return ret;
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 647fac170f19..cd67bae4a05e 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -11379,6 +11379,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;
 
@@ -11481,6 +11487,9 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)
 		return ret;
 
 	if (tgt_prog) {
+		prog->aux->tgt_prog_type = tgt_prog->type;
+		prog->aux->tgt_attach_type = tgt_prog->expected_attach_type;
+
 		if (prog->type == BPF_PROG_TYPE_EXT) {
 			env->ops = bpf_verifier_ops[tgt_prog->type];
 			prog->expected_attach_type =
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index a22812561064..feff1ed49f86 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -632,8 +632,13 @@ union bpf_attr {
 		};
 		__u32		attach_type;	/* attach type */
 		__u32		flags;		/* extra flags */
-		__aligned_u64	iter_info;	/* extra bpf_iter_link_info */
-		__u32		iter_info_len;	/* iter_info length */
+		union {
+			__u32		target_btf_id;	/* btf_id of target to attach to */
+			struct {
+				__aligned_u64	iter_info;	/* extra bpf_iter_link_info */
+				__u32		iter_info_len;	/* iter_info length */
+			};
+		};
 	} link_create;
 
 	struct { /* struct used by BPF_LINK_UPDATE command */

[PATCH bpf-next v8 06/11] bpf: Fix context type resolving for extension programs

[Toke Høiland-Jørgensen]

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

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

Having following program:

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

with its extension:

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

and tracing that extension with:

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

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

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

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

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

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

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

diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 76cc6ae46821..93cb8bfebe3b 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -3860,7 +3860,14 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
 
 	info->reg_type = PTR_TO_BTF_ID;
 	if (tgt_prog) {
-		ret = btf_translate_to_vmlinux(log, btf, t, tgt_prog->type, arg);
+		enum bpf_prog_type tgt_type;
+
+		if (tgt_prog->type == BPF_PROG_TYPE_EXT)
+			tgt_type = tgt_prog->aux->tgt_prog_type;
+		else
+			tgt_type = tgt_prog->type;
+
+		ret = btf_translate_to_vmlinux(log, btf, t, tgt_type, arg);
 		if (ret > 0) {
 			info->btf_id = ret;
 			return true;

[PATCH bpf-next v8 07/11] libbpf: add support for freplace attachment in bpf_link_create

[Toke Høiland-Jørgensen]

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

This adds support for supplying a target btf ID for the bpf_link_create()
operation, and adds a new bpf_program__attach_freplace() high-level API for
attaching freplace functions with a target.

Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
 tools/lib/bpf/bpf.c      |   18 +++++++++++++++---
 tools/lib/bpf/bpf.h      |    3 ++-
 tools/lib/bpf/libbpf.c   |   44 +++++++++++++++++++++++++++++++++++++++-----
 tools/lib/bpf/libbpf.h   |    3 +++
 tools/lib/bpf/libbpf.map |    1 +
 5 files changed, 60 insertions(+), 9 deletions(-)

diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
index 2baa1308737c..75f627094790 100644
--- a/tools/lib/bpf/bpf.c
+++ b/tools/lib/bpf/bpf.c
@@ -586,19 +586,31 @@ int bpf_link_create(int prog_fd, int target_fd,
 		    enum bpf_attach_type attach_type,
 		    const struct bpf_link_create_opts *opts)
 {
+	__u32 target_btf_id, iter_info_len;
 	union bpf_attr attr;
 
 	if (!OPTS_VALID(opts, bpf_link_create_opts))
 		return -EINVAL;
 
+	iter_info_len = OPTS_GET(opts, iter_info_len, 0);
+	target_btf_id = OPTS_GET(opts, target_btf_id, 0);
+
+	if (iter_info_len && target_btf_id)
+		return -EINVAL;
+
 	memset(&attr, 0, sizeof(attr));
 	attr.link_create.prog_fd = prog_fd;
 	attr.link_create.target_fd = target_fd;
 	attr.link_create.attach_type = attach_type;
 	attr.link_create.flags = OPTS_GET(opts, flags, 0);
-	attr.link_create.iter_info =
-		ptr_to_u64(OPTS_GET(opts, iter_info, (void *)0));
-	attr.link_create.iter_info_len = OPTS_GET(opts, iter_info_len, 0);
+
+	if (iter_info_len) {
+		attr.link_create.iter_info =
+			ptr_to_u64(OPTS_GET(opts, iter_info, (void *)0));
+		attr.link_create.iter_info_len = iter_info_len;
+	} else if (target_btf_id) {
+		attr.link_create.target_btf_id = target_btf_id;
+	}
 
 	return sys_bpf(BPF_LINK_CREATE, &attr, sizeof(attr));
 }
diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
index 8c1ac4b42f90..6b8dbe24adc9 100644
--- a/tools/lib/bpf/bpf.h
+++ b/tools/lib/bpf/bpf.h
@@ -174,8 +174,9 @@ struct bpf_link_create_opts {
 	__u32 flags;
 	union bpf_iter_link_info *iter_info;
 	__u32 iter_info_len;
+	__u32 target_btf_id;
 };
-#define bpf_link_create_opts__last_field iter_info_len
+#define bpf_link_create_opts__last_field target_btf_id
 
 LIBBPF_API int bpf_link_create(int prog_fd, int target_fd,
 			       enum bpf_attach_type attach_type,
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 570235dbc922..e9a2ad039e9a 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -9390,9 +9390,11 @@ static struct bpf_link *attach_iter(const struct bpf_sec_def *sec,
 }
 
 static struct bpf_link *
-bpf_program__attach_fd(struct bpf_program *prog, int target_fd,
+bpf_program__attach_fd(struct bpf_program *prog, int target_fd, int btf_id,
 		       const char *target_name)
 {
+	DECLARE_LIBBPF_OPTS(bpf_link_create_opts, opts,
+			    .target_btf_id = btf_id);
 	enum bpf_attach_type attach_type;
 	char errmsg[STRERR_BUFSIZE];
 	struct bpf_link *link;
@@ -9410,7 +9412,7 @@ bpf_program__attach_fd(struct bpf_program *prog, int target_fd,
 	link->detach = &bpf_link__detach_fd;
 
 	attach_type = bpf_program__get_expected_attach_type(prog);
-	link_fd = bpf_link_create(prog_fd, target_fd, attach_type, NULL);
+	link_fd = bpf_link_create(prog_fd, target_fd, attach_type, &opts);
 	if (link_fd < 0) {
 		link_fd = -errno;
 		free(link);
@@ -9426,19 +9428,51 @@ bpf_program__attach_fd(struct bpf_program *prog, int target_fd,
 struct bpf_link *
 bpf_program__attach_cgroup(struct bpf_program *prog, int cgroup_fd)
 {
-	return bpf_program__attach_fd(prog, cgroup_fd, "cgroup");
+	return bpf_program__attach_fd(prog, cgroup_fd, 0, "cgroup");
 }
 
 struct bpf_link *
 bpf_program__attach_netns(struct bpf_program *prog, int netns_fd)
 {
-	return bpf_program__attach_fd(prog, netns_fd, "netns");
+	return bpf_program__attach_fd(prog, netns_fd, 0, "netns");
 }
 
 struct bpf_link *bpf_program__attach_xdp(struct bpf_program *prog, int ifindex)
 {
 	/* target_fd/target_ifindex use the same field in LINK_CREATE */
-	return bpf_program__attach_fd(prog, ifindex, "xdp");
+	return bpf_program__attach_fd(prog, ifindex, 0, "xdp");
+}
+
+struct bpf_link *bpf_program__attach_freplace(struct bpf_program *prog,
+					      int target_fd,
+					      const char *attach_func_name)
+{
+	int btf_id;
+
+	if (!!target_fd != !!attach_func_name) {
+		pr_warn("prog '%s': supply none or both of target_fd and attach_func_name\n",
+			prog->name);
+		return ERR_PTR(-EINVAL);
+	}
+
+	if (prog->type != BPF_PROG_TYPE_EXT) {
+		pr_warn("prog '%s': only BPF_PROG_TYPE_EXT can attach as freplace",
+			prog->name);
+		return ERR_PTR(-EINVAL);
+	}
+
+	if (target_fd) {
+		btf_id = libbpf_find_prog_btf_id(attach_func_name, target_fd);
+		if (btf_id < 0)
+			return ERR_PTR(btf_id);
+
+		return bpf_program__attach_fd(prog, target_fd, btf_id, "freplace");
+	} else {
+		/* no target, so use raw_tracepoint_open for compatibility
+		 * with old kernels
+		 */
+		return bpf_program__attach_trace(prog);
+	}
 }
 
 struct bpf_link *
diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
index a750f67a23f6..6909ee81113a 100644
--- a/tools/lib/bpf/libbpf.h
+++ b/tools/lib/bpf/libbpf.h
@@ -261,6 +261,9 @@ LIBBPF_API struct bpf_link *
 bpf_program__attach_netns(struct bpf_program *prog, int netns_fd);
 LIBBPF_API struct bpf_link *
 bpf_program__attach_xdp(struct bpf_program *prog, int ifindex);
+LIBBPF_API struct bpf_link *
+bpf_program__attach_freplace(struct bpf_program *prog,
+			     int target_fd, const char *attach_func_name);
 
 struct bpf_map;
 
diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
index 5f054dadf082..b1c537873b23 100644
--- a/tools/lib/bpf/libbpf.map
+++ b/tools/lib/bpf/libbpf.map
@@ -303,6 +303,7 @@ LIBBPF_0.1.0 {
 LIBBPF_0.2.0 {
 	global:
 		bpf_prog_bind_map;
+		bpf_program__attach_freplace;
 		bpf_program__section_name;
 		perf_buffer__buffer_cnt;
 		perf_buffer__buffer_fd;

[PATCH bpf-next v8 08/11] 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.

Acked-by: Andrii Nakryiko <andriin@fb.com>
Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
 .../selftests/bpf/prog_tests/fexit_bpf2bpf.c       |  156 ++++++++++++++++----
 .../selftests/bpf/progs/freplace_get_constant.c    |   15 ++
 2 files changed, 139 insertions(+), 32 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/progs/freplace_get_constant.c

diff --git a/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c b/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c
index eda682727787..2b94e827b2c5 100644
--- a/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c
+++ b/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c
@@ -2,36 +2,79 @@
 /* Copyright (c) 2019 Facebook */
 #include <test_progs.h>
 #include <network_helpers.h>
+#include <bpf/btf.h>
+
+typedef int (*test_cb)(struct bpf_object *obj);
+
+static int check_data_map(struct bpf_object *obj, int prog_cnt, bool reset)
+{
+	struct bpf_map *data_map = NULL, *map;
+	__u64 *result = NULL;
+	const int zero = 0;
+	__u32 duration = 0;
+	int ret = -1, i;
+
+	result = malloc((prog_cnt + 32 /* spare */) * sizeof(__u64));
+	if (CHECK(!result, "alloc_memory", "failed to alloc memory"))
+		return -ENOMEM;
+
+	bpf_object__for_each_map(map, obj)
+		if (bpf_map__is_internal(map)) {
+			data_map = map;
+			break;
+		}
+	if (CHECK(!data_map, "find_data_map", "data map not found\n"))
+		goto out;
+
+	ret = bpf_map_lookup_elem(bpf_map__fd(data_map), &zero, result);
+	if (CHECK(ret, "get_result",
+		  "failed to get output data: %d\n", ret))
+		goto out;
+
+	for (i = 0; i < prog_cnt; i++) {
+		if (CHECK(result[i] != 1, "result",
+			  "fexit_bpf2bpf result[%d] failed err %llu\n",
+			  i, result[i]))
+			goto out;
+		result[i] = 0;
+	}
+	if (reset) {
+		ret = bpf_map_update_elem(bpf_map__fd(data_map), &zero, result, 0);
+		if (CHECK(ret, "reset_result", "failed to reset result\n"))
+			goto out;
+	}
+
+	ret = 0;
+out:
+	free(result);
+	return ret;
+}
 
 static void test_fexit_bpf2bpf_common(const char *obj_file,
 				      const char *target_obj_file,
 				      int prog_cnt,
 				      const char **prog_name,
-				      bool run_prog)
+				      bool run_prog,
+				      test_cb cb)
 {
-	struct bpf_object *obj = NULL, *pkt_obj;
-	int err, pkt_fd, i;
-	struct bpf_link **link = NULL;
+	struct bpf_object *obj = NULL, *tgt_obj;
 	struct bpf_program **prog = NULL;
+	struct bpf_link **link = NULL;
 	__u32 duration = 0, retval;
-	struct bpf_map *data_map;
-	const int zero = 0;
-	__u64 *result = NULL;
+	int err, tgt_fd, i;
 
 	err = bpf_prog_load(target_obj_file, BPF_PROG_TYPE_UNSPEC,
-			    &pkt_obj, &pkt_fd);
+			    &tgt_obj, &tgt_fd);
 	if (CHECK(err, "tgt_prog_load", "file %s err %d errno %d\n",
 		  target_obj_file, err, errno))
 		return;
 	DECLARE_LIBBPF_OPTS(bpf_object_open_opts, opts,
-			    .attach_prog_fd = pkt_fd,
+			    .attach_prog_fd = tgt_fd,
 			   );
 
 	link = calloc(sizeof(struct bpf_link *), prog_cnt);
 	prog = calloc(sizeof(struct bpf_program *), prog_cnt);
-	result = malloc((prog_cnt + 32 /* spare */) * sizeof(__u64));
-	if (CHECK(!link || !prog || !result, "alloc_memory",
-		  "failed to alloc memory"))
+	if (CHECK(!link || !prog, "alloc_memory", "failed to alloc memory"))
 		goto close_prog;
 
 	obj = bpf_object__open_file(obj_file, &opts);
@@ -53,39 +96,33 @@ static void test_fexit_bpf2bpf_common(const char *obj_file,
 			goto close_prog;
 	}
 
-	if (!run_prog)
-		goto close_prog;
+	if (cb) {
+		err = cb(obj);
+		if (err)
+			goto close_prog;
+	}
 
-	data_map = bpf_object__find_map_by_name(obj, "fexit_bp.bss");
-	if (CHECK(!data_map, "find_data_map", "data map not found\n"))
+	if (!run_prog)
 		goto close_prog;
 
-	err = bpf_prog_test_run(pkt_fd, 1, &pkt_v6, sizeof(pkt_v6),
+	err = bpf_prog_test_run(tgt_fd, 1, &pkt_v6, sizeof(pkt_v6),
 				NULL, NULL, &retval, &duration);
 	CHECK(err || retval, "ipv6",
 	      "err %d errno %d retval %d duration %d\n",
 	      err, errno, retval, duration);
 
-	err = bpf_map_lookup_elem(bpf_map__fd(data_map), &zero, result);
-	if (CHECK(err, "get_result",
-		  "failed to get output data: %d\n", err))
+	if (check_data_map(obj, prog_cnt, false))
 		goto close_prog;
 
-	for (i = 0; i < prog_cnt; i++)
-		if (CHECK(result[i] != 1, "result", "fexit_bpf2bpf failed err %llu\n",
-			  result[i]))
-			goto close_prog;
-
 close_prog:
 	for (i = 0; i < prog_cnt; i++)
 		if (!IS_ERR_OR_NULL(link[i]))
 			bpf_link__destroy(link[i]);
 	if (!IS_ERR_OR_NULL(obj))
 		bpf_object__close(obj);
-	bpf_object__close(pkt_obj);
+	bpf_object__close(tgt_obj);
 	free(link);
 	free(prog);
-	free(result);
 }
 
 static void test_target_no_callees(void)
@@ -96,7 +133,7 @@ static void test_target_no_callees(void)
 	test_fexit_bpf2bpf_common("./fexit_bpf2bpf_simple.o",
 				  "./test_pkt_md_access.o",
 				  ARRAY_SIZE(prog_name),
-				  prog_name, true);
+				  prog_name, true, NULL);
 }
 
 static void test_target_yes_callees(void)
@@ -110,7 +147,7 @@ static void test_target_yes_callees(void)
 	test_fexit_bpf2bpf_common("./fexit_bpf2bpf.o",
 				  "./test_pkt_access.o",
 				  ARRAY_SIZE(prog_name),
-				  prog_name, true);
+				  prog_name, true, NULL);
 }
 
 static void test_func_replace(void)
@@ -128,7 +165,7 @@ static void test_func_replace(void)
 	test_fexit_bpf2bpf_common("./fexit_bpf2bpf.o",
 				  "./test_pkt_access.o",
 				  ARRAY_SIZE(prog_name),
-				  prog_name, true);
+				  prog_name, true, NULL);
 }
 
 static void test_func_replace_verify(void)
@@ -139,7 +176,60 @@ 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";
+	struct bpf_program *prog = NULL;
+	struct bpf_object *tgt_obj;
+	__u32 duration = 0, retval;
+	struct bpf_link *link;
+	int err = 0, tgt_fd;
+
+	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;
+
+	link = bpf_program__attach_freplace(prog, tgt_fd, tgt_name);
+	if (CHECK(IS_ERR(link), "second_link", "failed to attach second link prog_fd %d tgt_fd %d\n", bpf_program__fd(prog), tgt_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:
+	bpf_link__destroy(link);
+	bpf_object__close(tgt_obj);
+	return err;
+}
+
+static void test_func_replace_multi(void)
+{
+	const char *prog_name[] = {
+		"freplace/get_constant",
+	};
+	test_fexit_bpf2bpf_common("./freplace_get_constant.o",
+				  "./test_pkt_access.o",
+				  ARRAY_SIZE(prog_name),
+				  prog_name, true, test_second_attach);
 }
 
 static void test_func_sockmap_update(void)
@@ -150,7 +240,7 @@ static void test_func_sockmap_update(void)
 	test_fexit_bpf2bpf_common("./freplace_cls_redirect.o",
 				  "./test_cls_redirect.o",
 				  ARRAY_SIZE(prog_name),
-				  prog_name, false);
+				  prog_name, false, NULL);
 }
 
 static void test_obj_load_failure_common(const char *obj_file,
@@ -222,4 +312,6 @@ void test_fexit_bpf2bpf(void)
 		test_func_replace_return_code();
 	if (test__start_subtest("func_map_prog_compatibility"))
 		test_func_map_prog_compatibility();
+	if (test__start_subtest("func_replace_multi"))
+		test_func_replace_multi();
 }
diff --git a/tools/testing/selftests/bpf/progs/freplace_get_constant.c b/tools/testing/selftests/bpf/progs/freplace_get_constant.c
new file mode 100644
index 000000000000..8f0ecf94e533
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/freplace_get_constant.c
@@ -0,0 +1,15 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_endian.h>
+
+volatile __u64 test_get_constant = 0;
+SEC("freplace/get_constant")
+int new_get_constant(long val)
+{
+	if (val != 123)
+		return 0;
+	test_get_constant = 1;
+	return test_get_constant; /* original get_constant() returns val - 122 */
+}
+char _license[] SEC("license") = "GPL";

[PATCH bpf-next v8 09/11] selftests/bpf: Adding test for arg dereference in extension trace

[Toke Høiland-Jørgensen]

From: Jiri Olsa <jolsa@kernel.org>

Adding test that setup following program:

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

with its extension:

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

and tracing that extension with:

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

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

Acked-by: Andrii Nakryiko <andriin@fb.com>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
 tools/testing/selftests/bpf/prog_tests/trace_ext.c |  111 ++++++++++++++++++++
 tools/testing/selftests/bpf/progs/test_trace_ext.c |   18 +++
 .../selftests/bpf/progs/test_trace_ext_tracing.c   |   25 +++++
 3 files changed, 154 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/trace_ext.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_trace_ext.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_trace_ext_tracing.c

diff --git a/tools/testing/selftests/bpf/prog_tests/trace_ext.c b/tools/testing/selftests/bpf/prog_tests/trace_ext.c
new file mode 100644
index 000000000000..924441d4362d
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/trace_ext.c
@@ -0,0 +1,111 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#define _GNU_SOURCE
+#include <test_progs.h>
+#include <network_helpers.h>
+#include <sys/stat.h>
+#include <linux/sched.h>
+#include <sys/syscall.h>
+
+#include "test_pkt_md_access.skel.h"
+#include "test_trace_ext.skel.h"
+#include "test_trace_ext_tracing.skel.h"
+
+static __u32 duration;
+
+void test_trace_ext(void)
+{
+	struct test_pkt_md_access *skel_pkt = NULL;
+	struct test_trace_ext_tracing *skel_trace = NULL;
+	struct test_trace_ext_tracing__bss *bss_trace;
+	struct test_trace_ext *skel_ext = NULL;
+	struct test_trace_ext__bss *bss_ext;
+	int err, pkt_fd, ext_fd;
+	struct bpf_program *prog;
+	char buf[100];
+	__u32 retval;
+	__u64 len;
+
+	/* open/load/attach test_pkt_md_access */
+	skel_pkt = test_pkt_md_access__open_and_load();
+	if (CHECK(!skel_pkt, "setup", "classifier/test_pkt_md_access open failed\n"))
+		goto cleanup;
+
+	err = test_pkt_md_access__attach(skel_pkt);
+	if (CHECK(err, "setup", "classifier/test_pkt_md_access attach failed: %d\n", err))
+		goto cleanup;
+
+	prog = skel_pkt->progs.test_pkt_md_access;
+	pkt_fd = bpf_program__fd(prog);
+
+	/* open extension */
+	skel_ext = test_trace_ext__open();
+	if (CHECK(!skel_ext, "setup", "freplace/test_pkt_md_access open failed\n"))
+		goto cleanup;
+
+	/* set extension's attach target - test_pkt_md_access  */
+	prog = skel_ext->progs.test_pkt_md_access_new;
+	bpf_program__set_attach_target(prog, pkt_fd, "test_pkt_md_access");
+
+	/* load/attach extension */
+	err = test_trace_ext__load(skel_ext);
+	if (CHECK(err, "setup", "freplace/test_pkt_md_access load failed\n")) {
+		libbpf_strerror(err, buf, sizeof(buf));
+		fprintf(stderr, "%s\n", buf);
+		goto cleanup;
+	}
+
+	err = test_trace_ext__attach(skel_ext);
+	if (CHECK(err, "setup", "freplace/test_pkt_md_access attach failed: %d\n", err))
+		goto cleanup;
+
+	prog = skel_ext->progs.test_pkt_md_access_new;
+	ext_fd = bpf_program__fd(prog);
+
+	/* open tracing  */
+	skel_trace = test_trace_ext_tracing__open();
+	if (CHECK(!skel_trace, "setup", "tracing/test_pkt_md_access_new open failed\n"))
+		goto cleanup;
+
+	/* set tracing's attach target - fentry */
+	prog = skel_trace->progs.fentry;
+	bpf_program__set_attach_target(prog, ext_fd, "test_pkt_md_access_new");
+
+	/* set tracing's attach target - fexit */
+	prog = skel_trace->progs.fexit;
+	bpf_program__set_attach_target(prog, ext_fd, "test_pkt_md_access_new");
+
+	/* load/attach tracing */
+	err = test_trace_ext_tracing__load(skel_trace);
+	if (CHECK(err, "setup", "tracing/test_pkt_md_access_new load failed\n")) {
+		libbpf_strerror(err, buf, sizeof(buf));
+		fprintf(stderr, "%s\n", buf);
+		goto cleanup;
+	}
+
+	err = test_trace_ext_tracing__attach(skel_trace);
+	if (CHECK(err, "setup", "tracing/test_pkt_md_access_new attach failed: %d\n", err))
+		goto cleanup;
+
+	/* trigger the test */
+	err = bpf_prog_test_run(pkt_fd, 1, &pkt_v4, sizeof(pkt_v4),
+				NULL, NULL, &retval, &duration);
+	CHECK(err || retval, "run", "err %d errno %d retval %d\n", err, errno, retval);
+
+	bss_ext = skel_ext->bss;
+	bss_trace = skel_trace->bss;
+
+	len = bss_ext->ext_called;
+
+	CHECK(bss_ext->ext_called == 0,
+		"check", "failed to trigger freplace/test_pkt_md_access\n");
+	CHECK(bss_trace->fentry_called != len,
+		"check", "failed to trigger fentry/test_pkt_md_access_new\n");
+	CHECK(bss_trace->fexit_called != len,
+		"check", "failed to trigger fexit/test_pkt_md_access_new\n");
+
+cleanup:
+	test_trace_ext_tracing__destroy(skel_trace);
+	test_trace_ext__destroy(skel_ext);
+	test_pkt_md_access__destroy(skel_pkt);
+}
diff --git a/tools/testing/selftests/bpf/progs/test_trace_ext.c b/tools/testing/selftests/bpf/progs/test_trace_ext.c
new file mode 100644
index 000000000000..d19a634d0e78
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_trace_ext.c
@@ -0,0 +1,18 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2019 Facebook
+#include <linux/bpf.h>
+#include <stdbool.h>
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_endian.h>
+#include <bpf/bpf_tracing.h>
+
+__u64 ext_called = 0;
+
+SEC("freplace/test_pkt_md_access")
+int test_pkt_md_access_new(struct __sk_buff *skb)
+{
+	ext_called = skb->len;
+	return 0;
+}
+
+char _license[] SEC("license") = "GPL";
diff --git a/tools/testing/selftests/bpf/progs/test_trace_ext_tracing.c b/tools/testing/selftests/bpf/progs/test_trace_ext_tracing.c
new file mode 100644
index 000000000000..52f3baf98f20
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_trace_ext_tracing.c
@@ -0,0 +1,25 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include "vmlinux.h"
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+
+__u64 fentry_called = 0;
+
+SEC("fentry/test_pkt_md_access_new")
+int BPF_PROG(fentry, struct sk_buff *skb)
+{
+	fentry_called = skb->len;
+	return 0;
+}
+
+__u64 fexit_called = 0;
+
+SEC("fexit/test_pkt_md_access_new")
+int BPF_PROG(fexit, struct sk_buff *skb)
+{
+	fexit_called = skb->len;
+	return 0;
+}
+
+char _license[] SEC("license") = "GPL";

[PATCH bpf-next v8 10/11] selftests: Add selftest for disallowing modify_return attachment to freplace

[Toke Høiland-Jørgensen]

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

This adds a selftest that ensures that modify_return tracing programs
cannot be attached to freplace programs. The security_ prefix is added to
the freplace program because that would otherwise let it pass the check for
modify_return.

Acked-by: Andrii Nakryiko <andriin@fb.com>
Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
 .../selftests/bpf/prog_tests/fexit_bpf2bpf.c       |   56 ++++++++++++++++++++
 .../selftests/bpf/progs/fmod_ret_freplace.c        |   14 +++++
 .../selftests/bpf/progs/freplace_get_constant.c    |    2 -
 3 files changed, 71 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/bpf/progs/fmod_ret_freplace.c

diff --git a/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c b/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c
index 2b94e827b2c5..5c0448910426 100644
--- a/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c
+++ b/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c
@@ -232,6 +232,60 @@ static void test_func_replace_multi(void)
 				  prog_name, true, test_second_attach);
 }
 
+static void test_fmod_ret_freplace(void)
+{
+	struct bpf_object *freplace_obj = NULL, *pkt_obj, *fmod_obj = NULL;
+	const char *freplace_name = "./freplace_get_constant.o";
+	const char *fmod_ret_name = "./fmod_ret_freplace.o";
+	DECLARE_LIBBPF_OPTS(bpf_object_open_opts, opts);
+	const char *tgt_name = "./test_pkt_access.o";
+	struct bpf_link *freplace_link = NULL;
+	struct bpf_program *prog;
+	__u32 duration = 0;
+	int err, pkt_fd;
+
+	err = bpf_prog_load(tgt_name, BPF_PROG_TYPE_UNSPEC,
+			    &pkt_obj, &pkt_fd);
+	/* the target prog should load fine */
+	if (CHECK(err, "tgt_prog_load", "file %s err %d errno %d\n",
+		  tgt_name, err, errno))
+		return;
+	opts.attach_prog_fd = pkt_fd;
+
+	freplace_obj = bpf_object__open_file(freplace_name, &opts);
+	if (CHECK(IS_ERR_OR_NULL(freplace_obj), "freplace_obj_open",
+		  "failed to open %s: %ld\n", freplace_name,
+		  PTR_ERR(freplace_obj)))
+		goto out;
+
+	err = bpf_object__load(freplace_obj);
+	if (CHECK(err, "freplace_obj_load", "err %d\n", err))
+		goto out;
+
+	prog = bpf_program__next(NULL, freplace_obj);
+	freplace_link = bpf_program__attach_trace(prog);
+	if (CHECK(IS_ERR(freplace_link), "freplace_attach_trace", "failed to link\n"))
+		goto out;
+
+	opts.attach_prog_fd = bpf_program__fd(prog);
+	fmod_obj = bpf_object__open_file(fmod_ret_name, &opts);
+	if (CHECK(IS_ERR_OR_NULL(fmod_obj), "fmod_obj_open",
+		  "failed to open %s: %ld\n", fmod_ret_name,
+		  PTR_ERR(fmod_obj)))
+		goto out;
+
+	err = bpf_object__load(fmod_obj);
+	if (CHECK(!err, "fmod_obj_load", "loading fmod_ret should fail\n"))
+		goto out;
+
+out:
+	bpf_link__destroy(freplace_link);
+	bpf_object__close(freplace_obj);
+	bpf_object__close(fmod_obj);
+	bpf_object__close(pkt_obj);
+}
+
+
 static void test_func_sockmap_update(void)
 {
 	const char *prog_name[] = {
@@ -314,4 +368,6 @@ void test_fexit_bpf2bpf(void)
 		test_func_map_prog_compatibility();
 	if (test__start_subtest("func_replace_multi"))
 		test_func_replace_multi();
+	if (test__start_subtest("fmod_ret_freplace"))
+		test_fmod_ret_freplace();
 }
diff --git a/tools/testing/selftests/bpf/progs/fmod_ret_freplace.c b/tools/testing/selftests/bpf/progs/fmod_ret_freplace.c
new file mode 100644
index 000000000000..c8943ccee6c0
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/fmod_ret_freplace.c
@@ -0,0 +1,14 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+
+volatile __u64 test_fmod_ret = 0;
+SEC("fmod_ret/security_new_get_constant")
+int BPF_PROG(fmod_ret_test, long val, int ret)
+{
+	test_fmod_ret = 1;
+	return 120;
+}
+
+char _license[] SEC("license") = "GPL";
diff --git a/tools/testing/selftests/bpf/progs/freplace_get_constant.c b/tools/testing/selftests/bpf/progs/freplace_get_constant.c
index 8f0ecf94e533..705e4b64dfc2 100644
--- a/tools/testing/selftests/bpf/progs/freplace_get_constant.c
+++ b/tools/testing/selftests/bpf/progs/freplace_get_constant.c
@@ -5,7 +5,7 @@
 
 volatile __u64 test_get_constant = 0;
 SEC("freplace/get_constant")
-int new_get_constant(long val)
+int security_new_get_constant(long val)
 {
 	if (val != 123)
 		return 0;

[PATCH bpf-next v8 11/11] selftests: Remove fmod_ret from benchmarks and test_overhead

[Toke Høiland-Jørgensen]

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

The benchmark code and the test_overhead prog_test included fmod_ret
programs that attached to various functions in the kernel. However, these
functions were never listed as allowed for return modification, so this
only worked because of the verifier skipping tests when a trampoline
already existed for the attach point. Now that the verifier checks have
been fixed, remove fmod_ret from the affected tests so they all work again.

Fixes: 4eaf0b5c5e04 ("selftest/bpf: Fmod_ret prog and implement test_overhead as part of bench")
Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
 tools/testing/selftests/bpf/bench.c                |    5 -----
 tools/testing/selftests/bpf/benchs/bench_rename.c  |   17 -----------------
 tools/testing/selftests/bpf/benchs/bench_trigger.c |   17 -----------------
 .../selftests/bpf/prog_tests/test_overhead.c       |   14 +-------------
 tools/testing/selftests/bpf/progs/test_overhead.c  |    6 ------
 tools/testing/selftests/bpf/progs/trigger_bench.c  |    7 -------
 6 files changed, 1 insertion(+), 65 deletions(-)

diff --git a/tools/testing/selftests/bpf/bench.c b/tools/testing/selftests/bpf/bench.c
index 1a427685a8a8..d1a4a55335f8 100644
--- a/tools/testing/selftests/bpf/bench.c
+++ b/tools/testing/selftests/bpf/bench.c
@@ -311,14 +311,12 @@ extern const struct bench bench_rename_kretprobe;
 extern const struct bench bench_rename_rawtp;
 extern const struct bench bench_rename_fentry;
 extern const struct bench bench_rename_fexit;
-extern const struct bench bench_rename_fmodret;
 extern const struct bench bench_trig_base;
 extern const struct bench bench_trig_tp;
 extern const struct bench bench_trig_rawtp;
 extern const struct bench bench_trig_kprobe;
 extern const struct bench bench_trig_fentry;
 extern const struct bench bench_trig_fentry_sleep;
-extern const struct bench bench_trig_fmodret;
 extern const struct bench bench_rb_libbpf;
 extern const struct bench bench_rb_custom;
 extern const struct bench bench_pb_libbpf;
@@ -333,14 +331,12 @@ static const struct bench *benchs[] = {
 	&bench_rename_rawtp,
 	&bench_rename_fentry,
 	&bench_rename_fexit,
-	&bench_rename_fmodret,
 	&bench_trig_base,
 	&bench_trig_tp,
 	&bench_trig_rawtp,
 	&bench_trig_kprobe,
 	&bench_trig_fentry,
 	&bench_trig_fentry_sleep,
-	&bench_trig_fmodret,
 	&bench_rb_libbpf,
 	&bench_rb_custom,
 	&bench_pb_libbpf,
@@ -464,4 +460,3 @@ int main(int argc, char **argv)
 
 	return 0;
 }
-
diff --git a/tools/testing/selftests/bpf/benchs/bench_rename.c b/tools/testing/selftests/bpf/benchs/bench_rename.c
index e74cff40f4fe..a967674098ad 100644
--- a/tools/testing/selftests/bpf/benchs/bench_rename.c
+++ b/tools/testing/selftests/bpf/benchs/bench_rename.c
@@ -106,12 +106,6 @@ static void setup_fexit()
 	attach_bpf(ctx.skel->progs.prog5);
 }
 
-static void setup_fmodret()
-{
-	setup_ctx();
-	attach_bpf(ctx.skel->progs.prog6);
-}
-
 static void *consumer(void *input)
 {
 	return NULL;
@@ -182,14 +176,3 @@ const struct bench bench_rename_fexit = {
 	.report_progress = hits_drops_report_progress,
 	.report_final = hits_drops_report_final,
 };
-
-const struct bench bench_rename_fmodret = {
-	.name = "rename-fmodret",
-	.validate = validate,
-	.setup = setup_fmodret,
-	.producer_thread = producer,
-	.consumer_thread = consumer,
-	.measure = measure,
-	.report_progress = hits_drops_report_progress,
-	.report_final = hits_drops_report_final,
-};
diff --git a/tools/testing/selftests/bpf/benchs/bench_trigger.c b/tools/testing/selftests/bpf/benchs/bench_trigger.c
index 2a0b6c9885a4..93ab7b280b25 100644
--- a/tools/testing/selftests/bpf/benchs/bench_trigger.c
+++ b/tools/testing/selftests/bpf/benchs/bench_trigger.c
@@ -96,12 +96,6 @@ static void trigger_fentry_sleep_setup()
 	attach_bpf(ctx.skel->progs.bench_trigger_fentry_sleep);
 }
 
-static void trigger_fmodret_setup()
-{
-	setup_ctx();
-	attach_bpf(ctx.skel->progs.bench_trigger_fmodret);
-}
-
 static void *trigger_consumer(void *input)
 {
 	return NULL;
@@ -171,14 +165,3 @@ const struct bench bench_trig_fentry_sleep = {
 	.report_progress = hits_drops_report_progress,
 	.report_final = hits_drops_report_final,
 };
-
-const struct bench bench_trig_fmodret = {
-	.name = "trig-fmodret",
-	.validate = trigger_validate,
-	.setup = trigger_fmodret_setup,
-	.producer_thread = trigger_producer,
-	.consumer_thread = trigger_consumer,
-	.measure = trigger_measure,
-	.report_progress = hits_drops_report_progress,
-	.report_final = hits_drops_report_final,
-};
diff --git a/tools/testing/selftests/bpf/prog_tests/test_overhead.c b/tools/testing/selftests/bpf/prog_tests/test_overhead.c
index 2702df2b2343..9966685866fd 100644
--- a/tools/testing/selftests/bpf/prog_tests/test_overhead.c
+++ b/tools/testing/selftests/bpf/prog_tests/test_overhead.c
@@ -61,10 +61,9 @@ void test_test_overhead(void)
 	const char *raw_tp_name = "raw_tp/task_rename";
 	const char *fentry_name = "fentry/__set_task_comm";
 	const char *fexit_name = "fexit/__set_task_comm";
-	const char *fmodret_name = "fmod_ret/__set_task_comm";
 	const char *kprobe_func = "__set_task_comm";
 	struct bpf_program *kprobe_prog, *kretprobe_prog, *raw_tp_prog;
-	struct bpf_program *fentry_prog, *fexit_prog, *fmodret_prog;
+	struct bpf_program *fentry_prog, *fexit_prog;
 	struct bpf_object *obj;
 	struct bpf_link *link;
 	int err, duration = 0;
@@ -97,11 +96,6 @@ void test_test_overhead(void)
 	if (CHECK(!fexit_prog, "find_probe",
 		  "prog '%s' not found\n", fexit_name))
 		goto cleanup;
-	fmodret_prog = bpf_object__find_program_by_title(obj, fmodret_name);
-	if (CHECK(!fmodret_prog, "find_probe",
-		  "prog '%s' not found\n", fmodret_name))
-		goto cleanup;
-
 	err = bpf_object__load(obj);
 	if (CHECK(err, "obj_load", "err %d\n", err))
 		goto cleanup;
@@ -148,12 +142,6 @@ void test_test_overhead(void)
 	test_run("fexit");
 	bpf_link__destroy(link);
 
-	/* attach fmod_ret */
-	link = bpf_program__attach_trace(fmodret_prog);
-	if (CHECK(IS_ERR(link), "attach fmod_ret", "err %ld\n", PTR_ERR(link)))
-		goto cleanup;
-	test_run("fmod_ret");
-	bpf_link__destroy(link);
 cleanup:
 	prctl(PR_SET_NAME, comm, 0L, 0L, 0L);
 	bpf_object__close(obj);
diff --git a/tools/testing/selftests/bpf/progs/test_overhead.c b/tools/testing/selftests/bpf/progs/test_overhead.c
index 42403d088abc..abb7344b531f 100644
--- a/tools/testing/selftests/bpf/progs/test_overhead.c
+++ b/tools/testing/selftests/bpf/progs/test_overhead.c
@@ -39,10 +39,4 @@ int BPF_PROG(prog5, struct task_struct *tsk, const char *buf, bool exec)
 	return 0;
 }
 
-SEC("fmod_ret/__set_task_comm")
-int BPF_PROG(prog6, struct task_struct *tsk, const char *buf, bool exec)
-{
-	return !tsk;
-}
-
 char _license[] SEC("license") = "GPL";
diff --git a/tools/testing/selftests/bpf/progs/trigger_bench.c b/tools/testing/selftests/bpf/progs/trigger_bench.c
index 9a4d09590b3d..1af23ac0c37c 100644
--- a/tools/testing/selftests/bpf/progs/trigger_bench.c
+++ b/tools/testing/selftests/bpf/progs/trigger_bench.c
@@ -45,10 +45,3 @@ int bench_trigger_fentry_sleep(void *ctx)
 	__sync_add_and_fetch(&hits, 1);
 	return 0;
 }
-
-SEC("fmod_ret/__x64_sys_getpgid")
-int bench_trigger_fmodret(void *ctx)
-{
-	__sync_add_and_fetch(&hits, 1);
-	return -22;
-}

Re: [PATCH bpf-next v8 01/11] bpf: disallow attaching modify_return tracing functions to other BPF programs

[Andrii Nakryiko]

On Tue, Sep 22, 2020 at 11:39 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> From: Toke Høiland-Jørgensen <toke@redhat.com>
>
> From the checks and commit messages for modify_return, it seems it was
> never the intention that it should be possible to attach a tracing program
> with expected_attach_type == BPF_MODIFY_RETURN to another BPF program.
> However, check_attach_modify_return() will only look at the function name,
> so if the target function starts with "security_", the attach will be
> allowed even for bpf2bpf attachment.
>
> Fix this oversight by also blocking the modification if a target program is
> supplied.
>
> Fixes: 18644cec714a ("bpf: Fix use-after-free in fmod_ret check")
> Fixes: 6ba43b761c41 ("bpf: Attachment verification for BPF_MODIFY_RETURN")
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> ---

Acked-by: Andrii Nakryiko <andriin@fb.com>

>  kernel/bpf/verifier.c |    5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 15ab889b0a3f..797e2b0d8bc2 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -11471,6 +11471,11 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)
>                                 verbose(env, "%s is not sleepable\n",
>                                         prog->aux->attach_func_name);
>                 } else if (prog->expected_attach_type == BPF_MODIFY_RETURN) {
> +                       if (tgt_prog) {
> +                               verbose(env, "can't modify return codes of BPF programs\n");
> +                               ret = -EINVAL;
> +                               goto out;
> +                       }
>                         ret = check_attach_modify_return(prog, addr);
>                         if (ret)
>                                 verbose(env, "%s() is not modifiable\n",
>

Re: [PATCH bpf-next v8 07/11] libbpf: add support for freplace attachment in bpf_link_create

[Andrii Nakryiko]

On Tue, Sep 22, 2020 at 11:39 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> From: Toke Høiland-Jørgensen <toke@redhat.com>
>
> This adds support for supplying a target btf ID for the bpf_link_create()
> operation, and adds a new bpf_program__attach_freplace() high-level API for
> attaching freplace functions with a target.
>
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> ---

LGTM.

Acked-by: Andrii Nakryiko <andriin@fb.com>

>  tools/lib/bpf/bpf.c      |   18 +++++++++++++++---
>  tools/lib/bpf/bpf.h      |    3 ++-
>  tools/lib/bpf/libbpf.c   |   44 +++++++++++++++++++++++++++++++++++++++-----
>  tools/lib/bpf/libbpf.h   |    3 +++
>  tools/lib/bpf/libbpf.map |    1 +
>  5 files changed, 60 insertions(+), 9 deletions(-)
>

[...]

Re: [PATCH bpf-next v8 11/11] selftests: Remove fmod_ret from benchmarks and test_overhead

[Andrii Nakryiko]

On Tue, Sep 22, 2020 at 11:39 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> From: Toke Høiland-Jørgensen <toke@redhat.com>
>
> The benchmark code and the test_overhead prog_test included fmod_ret
> programs that attached to various functions in the kernel. However, these
> functions were never listed as allowed for return modification, so this
> only worked because of the verifier skipping tests when a trampoline
> already existed for the attach point. Now that the verifier checks have
> been fixed, remove fmod_ret from the affected tests so they all work again.
>
> Fixes: 4eaf0b5c5e04 ("selftest/bpf: Fmod_ret prog and implement test_overhead as part of bench")
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> ---

Acked-by: Andrii Nakryiko <andriin@fb.com>

>  tools/testing/selftests/bpf/bench.c                |    5 -----
>  tools/testing/selftests/bpf/benchs/bench_rename.c  |   17 -----------------
>  tools/testing/selftests/bpf/benchs/bench_trigger.c |   17 -----------------
>  .../selftests/bpf/prog_tests/test_overhead.c       |   14 +-------------
>  tools/testing/selftests/bpf/progs/test_overhead.c  |    6 ------
>  tools/testing/selftests/bpf/progs/trigger_bench.c  |    7 -------
>  6 files changed, 1 insertion(+), 65 deletions(-)
>

[...]

Re: [PATCH bpf-next v8 07/11] libbpf: add support for freplace attachment in bpf_link_create

[Toke Høiland-Jørgensen]

Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:

> On Tue, Sep 22, 2020 at 11:39 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>>
>> From: Toke Høiland-Jørgensen <toke@redhat.com>
>>
>> This adds support for supplying a target btf ID for the bpf_link_create()
>> operation, and adds a new bpf_program__attach_freplace() high-level API for
>> attaching freplace functions with a target.
>>
>> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
>> ---
>
> LGTM.
>
> Acked-by: Andrii Nakryiko <andriin@fb.com>

Awesome! Thanks again for your (as always) thorough review (for the
whole series, of course) :)

-Toke

Re: [PATCH bpf-next v8 03/11] bpf: verifier: refactor check_attach_btf_id()

[Alexei Starovoitov]

On Tue, Sep 22, 2020 at 08:38:37PM +0200, Toke Høiland-Jørgensen wrote:
> 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.

raw_tp_open part is no longer correct.

Also could you re-phrase it that 'stores a bunch of state about the attechment'
is still the case. It doesn't store into bpf_prog directly, but returns instead.

> This also fixes a bug where a bunch of checks were skipped if a trampoline
> already existed for the tracing target.

This time we were lucky. When you see such selftests failures please debug
them before submitting the patches. The reviewers should not be pointing out
that the patch broke some tests.
If anything breaks please mention it in the cover letter.

> -static int check_attach_modify_return(struct bpf_prog *prog, unsigned long addr)
> +static int check_attach_modify_return(const struct bpf_prog *prog, unsigned long addr,
> +				      const char *func_name)

Since you're adding 'func_name' why keep 'prog' there? Pls drop it.

>  {
>  	if (within_error_injection_list(addr) ||
> -	    !strncmp(SECURITY_PREFIX, prog->aux->attach_func_name,
> -		     sizeof(SECURITY_PREFIX) - 1))
> +	    !strncmp(SECURITY_PREFIX, func_name, sizeof(SECURITY_PREFIX) - 1))
>  		return 0;
>  
>  	return -EINVAL;
> @@ -11215,43 +11215,29 @@ static int check_non_sleepable_error_inject(u32 btf_id)
>  	return btf_id_set_contains(&btf_non_sleepable_error_inject, btf_id);
>  }
>  
> -static int check_attach_btf_id(struct bpf_verifier_env *env)
> +int bpf_check_attach_target(struct bpf_verifier_log *log,
> +			    const struct bpf_prog *prog,
> +			    const struct bpf_prog *tgt_prog,
> +			    u32 btf_id,
> +			    struct btf_func_model *fmodel,
> +			    long *tgt_addr,
> +			    const char **tgt_name,
> +			    const struct btf_type **tgt_type)

How about grouping the return args into
struct bpf_attach_target_info {
 struct btf_func_model fmodel;
 long tgt_addr;
 const char *tgt_name;
 const struct btf_type *tgt_type;
};
allocate it on stack in the caller and pass a pointer into this function?

The same way pass the whole &bpf_attach_target_info into bpf_trampoline_get().
It will use fmodel and tgt_addr out of it, but it doesn't hurt to pass
the whole thing.

Overall I like the refactoring, but this prototype and conditional
if (tgt_name) *tgt_name =; and if (tgt_type) makes it harder to comprehend.

>  		if (!tgt_prog->jited) {
>  			bpf_log(log, "Can attach to only JITed progs\n");
> @@ -11328,13 +11312,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;

Could you please refactor key computation into a helper as well?
Especially since it will be used out of verifier.c and from syscall.c.
Something like this for bpf.h
static inline u64 bpf_trampoline_compute_key(struct bpf_prog *tgt_prog, u32 btf_id)
{
        if (tgt_prog) {
                return ((u64)tgt_prog->aux->id) << 32 | btf_id;
        } else {
                return btf_id;
        }
}

> +	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;
>  	}

and here it would be:
if (tgt_prog && prog->type == BPF_PROG_TYPE_EXT) {
	env->ops = bpf_verifier_ops[tgt_prog->type];
	prog->expected_attach_type = tgt_prog->expected_attach_type;
}
key = bpf_trampoline_compute_key(tgt_prog, btf_id);

otherwise above 'if' groups two separate things.
It's not pretty in the existing code, no doubt, but since you're doing
nice cleanup let's make it clean here too.

> +
> +	/* remember two read only pointers that are valid for
> +	 * the life time of the kernel
> +	 */

Here this comment is not correct.
It was correct in the place you copy-pasted it from, but not here.
Please think it through and adjust accordingly.

Re: [PATCH bpf-next v8 04/11] bpf: move prog->aux->linked_prog and trampoline into bpf_link on attach

[Alexei Starovoitov]

On Tue, Sep 22, 2020 at 08:38:38PM +0200, Toke Høiland-Jørgensen wrote:
> @@ -746,7 +748,9 @@ struct bpf_prog_aux {
>  	u32 max_rdonly_access;
>  	u32 max_rdwr_access;
>  	const struct bpf_ctx_arg_aux *ctx_arg_info;
> -	struct bpf_prog *linked_prog;

This change breaks bpf_preload and selftests test_bpffs.
There is really no excuse not to run the selftests.

I think I will just start marking patches as changes-requested when I see that
they break tests without replying and without reviewing.
Please respect reviewer's time.

> +	struct mutex tgt_mutex; /* protects tgt_* pointers below, *after* prog becomes visible */
> +	struct bpf_prog *tgt_prog;
> +	struct bpf_trampoline *tgt_trampoline;
>  	bool verifier_zext; /* Zero extensions has been inserted by verifier. */
>  	bool offload_requested;
>  	bool attach_btf_trace; /* true if attaching to BTF-enabled raw tp */
...
>  struct bpf_tracing_link {
>  	struct bpf_link link;
>  	enum bpf_attach_type attach_type;
> +	struct bpf_trampoline *trampoline;
> +	struct bpf_prog *tgt_prog;

imo it's confusing to have 'tgt_prog' to mean two different things.
In prog->aux->tgt_prog it means target prog to attach to in the future.
Whereas here it means the existing prog that was used to attached to.
They kinda both 'target progs' but would be good to disambiguate.
May be keep it as 'tgt_prog' here and
rename to 'dest_prog' and 'dest_trampoline' in prog->aux ?

>  };
>  
>  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);

I had to scratch my head quite a bit before I understood this NULL check.
Could you add a comment saying that tr_link->tgt_prog can be NULL
when trampoline is for kernel function ?

Re: [PATCH bpf-next v8 05/11] bpf: support attaching freplace programs to multiple attach points

[Alexei Starovoitov]

On Tue, Sep 22, 2020 at 08:38:39PM +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) {
> +			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;
> +		}
> +
> +		key = ((u64)tgt_prog->aux->id) << 32 | btf_id;

key = bpf_trampoline_compute_key(tgt_prog, btf_id);
would be handy here.

> +	}
> +
>  	link = kzalloc(sizeof(*link), GFP_USER);
>  	if (!link) {
>  		err = -ENOMEM;
> @@ -2594,12 +2622,28 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog)
>  
>  	mutex_lock(&prog->aux->tgt_mutex);
>  
> -	if (!prog->aux->tgt_trampoline) {
> +	if (!prog->aux->tgt_trampoline && !tgt_prog) {
>  		err = -ENOENT;
>  		goto out_unlock;
>  	}

Could you add a comment explaining all cases, since it's hard to follow right
now and few month later no one will remember.
As far as I understood:
prog->aux->dest_trampoline != NULL -> the program was just loaded and not attached to anything.
prog->aux->dest_trampoline == NULL -> the program was loaded and raw_tp_open-ed.
tgt_prog != NULL only when sepcifying tgt_prog_fd + target_btf_id in link_create api.
tgt_prog == NULL when this function is called from raw_tp_open.

Only the case of both NULL is invalid.

> -	tr = prog->aux->tgt_trampoline;
> -	tgt_prog = prog->aux->tgt_prog;
> +
> +	if (!prog->aux->tgt_trampoline ||
> +	    (key && key != prog->aux->tgt_trampoline->key)) {
> +
> +		err = bpf_check_attach_target(NULL, prog, tgt_prog, btf_id,
> +					      &fmodel, &addr, NULL, NULL);
> +		if (err)
> +			goto out_unlock;
> +
> +		tr = bpf_trampoline_get(key, (void *)addr, &fmodel);
> +		if (!tr) {
> +			err = -ENOMEM;
> +			goto out_unlock;
> +		}
> +	} else {

This 'else' is the case when a prog was loaded and _not_ raw_tp_open-ed
and the user is doing link_create with tgt_prog_fd + target_btf_id
into exactly the same place as attach_btf_id during load?
So this is the alternative api to raw_tp_open, right?
Please explain this in commit log and in comments.
It's not some minor detail.

> +		tr = prog->aux->tgt_trampoline;
> +		tgt_prog = prog->aux->tgt_prog;
> +	}
>  
>  	err = bpf_link_prime(&link->link, &link_primer);
>  	if (err)
> @@ -2614,16 +2658,24 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog)
>  
>  	link->tgt_prog = tgt_prog;
>  	link->trampoline = tr;
> -
> -	prog->aux->tgt_prog = NULL;
> -	prog->aux->tgt_trampoline = NULL;
> +	if (tr == prog->aux->tgt_trampoline) {
> +		/* if we got a new ref from syscall, drop existing one from prog */
> +		if (tgt_prog_fd)
> +			bpf_prog_put(prog->aux->tgt_prog);
> +		prog->aux->tgt_trampoline = NULL;
> +		prog->aux->tgt_prog = NULL;
> +	}

What happens when the user did prog load with attach_btf_id into one tgt_prog
but then link_create into a different tgt_prog?
bpf_check_attach_target + bpf_trampoline_get will allocate new trampoline (potentially)
and tr != prog->aux->dest_trampoline,
so we won't trigger the above code.
prog->aux->dest_prog/dest_tramoline will still point to some prog.
Later raw_tp_open will succeed and prog will be attached to two places.
I would probably make it unconditional that both raw_tp_open
and link_create clear dest_prog/dest_trampoline, but can be convinced otherwise.
What use case do you have in mind to allow that?
Anyway it needs to be documented and tests written.

> +		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;

May be call them saved_tgt_prog_type and saved_tgt_attach_type ?
Since that's another variant of 'target' meaning. Here the prog type survives
the target prog, since it will be still valid even when the first prog it was
attached to will be unloaded.

Re: [PATCH bpf-next v8 11/11] selftests: Remove fmod_ret from benchmarks and test_overhead

[Alexei Starovoitov]

On Tue, Sep 22, 2020 at 08:38:45PM +0200, Toke Høiland-Jørgensen wrote:
> -const struct bench bench_trig_fmodret = {
> -	.name = "trig-fmodret",
> -	.validate = trigger_validate,
> -	.setup = trigger_fmodret_setup,
> -	.producer_thread = trigger_producer,
> -	.consumer_thread = trigger_consumer,
> -	.measure = trigger_measure,
> -	.report_progress = hits_drops_report_progress,
> -	.report_final = hits_drops_report_final,
> -};
> diff --git a/tools/testing/selftests/bpf/progs/trigger_bench.c b/tools/testing/selftests/bpf/progs/trigger_bench.c
> index 9a4d09590b3d..1af23ac0c37c 100644
> --- a/tools/testing/selftests/bpf/progs/trigger_bench.c
> +++ b/tools/testing/selftests/bpf/progs/trigger_bench.c
> @@ -45,10 +45,3 @@ int bench_trigger_fentry_sleep(void *ctx)
>  	__sync_add_and_fetch(&hits, 1);
>  	return 0;
>  }
> -
> -SEC("fmod_ret/__x64_sys_getpgid")
> -int bench_trigger_fmodret(void *ctx)
> -{
> -	__sync_add_and_fetch(&hits, 1);
> -	return -22;
> -}

why are you removing this? There is no problem here.
All syscalls are error-injectable.
I'm surprised Andrii acked this :(

Re: [PATCH bpf-next v8 11/11] selftests: Remove fmod_ret from benchmarks and test_overhead

[Andrii Nakryiko]

On Wed, Sep 23, 2020 at 6:08 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Tue, Sep 22, 2020 at 08:38:45PM +0200, Toke Høiland-Jørgensen wrote:
> > -const struct bench bench_trig_fmodret = {
> > -     .name = "trig-fmodret",
> > -     .validate = trigger_validate,
> > -     .setup = trigger_fmodret_setup,
> > -     .producer_thread = trigger_producer,
> > -     .consumer_thread = trigger_consumer,
> > -     .measure = trigger_measure,
> > -     .report_progress = hits_drops_report_progress,
> > -     .report_final = hits_drops_report_final,
> > -};
> > diff --git a/tools/testing/selftests/bpf/progs/trigger_bench.c b/tools/testing/selftests/bpf/progs/trigger_bench.c
> > index 9a4d09590b3d..1af23ac0c37c 100644
> > --- a/tools/testing/selftests/bpf/progs/trigger_bench.c
> > +++ b/tools/testing/selftests/bpf/progs/trigger_bench.c
> > @@ -45,10 +45,3 @@ int bench_trigger_fentry_sleep(void *ctx)
> >       __sync_add_and_fetch(&hits, 1);
> >       return 0;
> >  }
> > -
> > -SEC("fmod_ret/__x64_sys_getpgid")
> > -int bench_trigger_fmodret(void *ctx)
> > -{
> > -     __sync_add_and_fetch(&hits, 1);
> > -     return -22;
> > -}
>
> why are you removing this? There is no problem here.
> All syscalls are error-injectable.
> I'm surprised Andrii acked this :(

Andrii didn't know that all syscalls are error-injectable, thanks for
catching :) after fmod_ret/__set_task_comm I just assumed that I've
been abusing fmod_ret all this time...

Re: [PATCH bpf-next v8 04/11] bpf: move prog->aux->linked_prog and trampoline into bpf_link on attach

[Toke Høiland-Jørgensen]

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

> On Tue, Sep 22, 2020 at 08:38:38PM +0200, Toke Høiland-Jørgensen wrote:
>> @@ -746,7 +748,9 @@ struct bpf_prog_aux {
>>  	u32 max_rdonly_access;
>>  	u32 max_rdwr_access;
>>  	const struct bpf_ctx_arg_aux *ctx_arg_info;
>> -	struct bpf_prog *linked_prog;
>
> This change breaks bpf_preload and selftests test_bpffs.
> There is really no excuse not to run the selftests.

I did run the tests, and saw no more breakages after applying my patches
than before. Which didn't catch this, because this is the current state
of bpf-next selftests:

# ./test_progs  | grep FAIL
test_lookup_update:FAIL:map1_leak inner_map1 leaked!
#10/1 lookup_update:FAIL
#10 btf_map_in_map:FAIL
configure_stack:FAIL:BPF load failed; run with -vv for more info
#72 sk_assign:FAIL
test_test_bpffs:FAIL:bpffs test  failed 255
#96 test_bpffs:FAIL
Summary: 113/844 PASSED, 14 SKIPPED, 4 FAILED

The test_bpffs failure happens because the umh is missing from the
.config; and when I tried to fix this I ended up with:

[..]
  CC [M]  kernel/bpf/preload/bpf_preload_kern.o

Auto-detecting system features:
...                        libelf: [ OFF ]
...                          zlib: [ OFF ]
...                           bpf: [ OFF ]

No libelf found

...which I just put down to random breakage, turned off the umh and
continued on my way (ignoring the failed test). Until you wrote this I
did not suspect this would be something I needed to pay attention to.
Now that you did mention it, I'll obviously go investigate some more, my
point is just that in this instance it's not accurate to assume I just
didn't run the tests... :)

> I think I will just start marking patches as changes-requested when I see that
> they break tests without replying and without reviewing.
> Please respect reviewer's time.

That is completely fine if the tests are working in the first place. And
even when they're not (like in this case), pointing it out is fine, and
I'll obviously go investigate. But please at least reply to the email,
not all of us watch patchwork regularly.

(I'll fix all your other comments and respin; thanks!)

-Toke

Re: [PATCH bpf-next v8 04/11] bpf: move prog->aux->linked_prog and trampoline into bpf_link on attach

[Alexei Starovoitov]

On Thu, Sep 24, 2020 at 7:34 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> ...which I just put down to random breakage, turned off the umh and
> continued on my way (ignoring the failed test). Until you wrote this I
> did not suspect this would be something I needed to pay attention to.
> Now that you did mention it, I'll obviously go investigate some more, my
> point is just that in this instance it's not accurate to assume I just
> didn't run the tests... :)

Ignoring failures is the same as not running them.
I expect all developers to confirm that they see "0 FAILED" before
sending any patches.

>
> > I think I will just start marking patches as changes-requested when I see that
> > they break tests without replying and without reviewing.
> > Please respect reviewer's time.
>
> That is completely fine if the tests are working in the first place. And
> even when they're not (like in this case), pointing it out is fine, and
> I'll obviously go investigate. But please at least reply to the email,
> not all of us watch patchwork regularly.

Please see Documentation/bpf/bpf_devel_QA.rst.
patchwork status is the way we communicate the intent.
If the patch is not in the queue it won't be acted upon.

Re: [PATCH bpf-next v8 04/11] bpf: move prog->aux->linked_prog and trampoline into bpf_link on attach

[Andrii Nakryiko]

On Thu, Sep 24, 2020 at 7:36 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> Alexei Starovoitov <alexei.starovoitov@gmail.com> writes:
>
> > On Tue, Sep 22, 2020 at 08:38:38PM +0200, Toke Høiland-Jørgensen wrote:
> >> @@ -746,7 +748,9 @@ struct bpf_prog_aux {
> >>      u32 max_rdonly_access;
> >>      u32 max_rdwr_access;
> >>      const struct bpf_ctx_arg_aux *ctx_arg_info;
> >> -    struct bpf_prog *linked_prog;
> >
> > This change breaks bpf_preload and selftests test_bpffs.
> > There is really no excuse not to run the selftests.
>
> I did run the tests, and saw no more breakages after applying my patches
> than before. Which didn't catch this, because this is the current state
> of bpf-next selftests:
>
> # ./test_progs  | grep FAIL
> test_lookup_update:FAIL:map1_leak inner_map1 leaked!
> #10/1 lookup_update:FAIL
> #10 btf_map_in_map:FAIL

this failure suggests you are not running the latest kernel, btw


> configure_stack:FAIL:BPF load failed; run with -vv for more info
> #72 sk_assign:FAIL
> test_test_bpffs:FAIL:bpffs test  failed 255
> #96 test_bpffs:FAIL
> Summary: 113/844 PASSED, 14 SKIPPED, 4 FAILED
>
> The test_bpffs failure happens because the umh is missing from the
> .config; and when I tried to fix this I ended up with:

yeah, seems like selftests/bpf/config needs to be updated to mention
UMH-related config values:

CONFIG_BPF_PRELOAD=y
CONFIG_BPF_PRELOAD_UMD=m|y

with that test_bpffs shouldn't fail on master

>
> [..]
>   CC [M]  kernel/bpf/preload/bpf_preload_kern.o
>
> Auto-detecting system features:
> ...                        libelf: [ OFF ]
> ...                          zlib: [ OFF ]
> ...                           bpf: [ OFF ]
>
> No libelf found

might be worthwhile to look into why detection fails, might be
something with Makefiles or your environment

>
> ...which I just put down to random breakage, turned off the umh and
> continued on my way (ignoring the failed test). Until you wrote this I
> did not suspect this would be something I needed to pay attention to.
> Now that you did mention it, I'll obviously go investigate some more, my
> point is just that in this instance it's not accurate to assume I just
> didn't run the tests... :)

Don't just assume some tests are always broken. Either ask or
investigate on your own. Such cases do happen from time to time while
we wait for a fix in bpf to get merged into bpf-next or vice versa,
but it's rare. We now have two different CI systems running selftests
all the time, in addition to running them locally as well, so any
permanent test failure is very apparent and annoying, so we fix them
quickly. So, when in doubt - ask or fix.

>
> > I think I will just start marking patches as changes-requested when I see that
> > they break tests without replying and without reviewing.
> > Please respect reviewer's time.
>
> That is completely fine if the tests are working in the first place. And

They are and hopefully moving forward that would be your assumption.

> even when they're not (like in this case), pointing it out is fine, and
> I'll obviously go investigate. But please at least reply to the email,
> not all of us watch patchwork regularly.
>
> (I'll fix all your other comments and respin; thanks!)
>
> -Toke
>

Re: [PATCH bpf-next v8 04/11] bpf: move prog->aux->linked_prog and trampoline into bpf_link on attach

[Toke Høiland-Jørgensen]

Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:

> On Thu, Sep 24, 2020 at 7:36 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>>
>> Alexei Starovoitov <alexei.starovoitov@gmail.com> writes:
>>
>> > On Tue, Sep 22, 2020 at 08:38:38PM +0200, Toke Høiland-Jørgensen wrote:
>> >> @@ -746,7 +748,9 @@ struct bpf_prog_aux {
>> >>      u32 max_rdonly_access;
>> >>      u32 max_rdwr_access;
>> >>      const struct bpf_ctx_arg_aux *ctx_arg_info;
>> >> -    struct bpf_prog *linked_prog;
>> >
>> > This change breaks bpf_preload and selftests test_bpffs.
>> > There is really no excuse not to run the selftests.
>>
>> I did run the tests, and saw no more breakages after applying my patches
>> than before. Which didn't catch this, because this is the current state
>> of bpf-next selftests:
>>
>> # ./test_progs  | grep FAIL
>> test_lookup_update:FAIL:map1_leak inner_map1 leaked!
>> #10/1 lookup_update:FAIL
>> #10 btf_map_in_map:FAIL
>
> this failure suggests you are not running the latest kernel, btw

I did see that discussion (about the reverted patch), and figured that
was the case. So I did a 'git pull' just before testing, and still got
this.

$ git describe HEAD
v5.9-rc3-2681-g182bf3f3ddb6

so any other ideas? :)

>> configure_stack:FAIL:BPF load failed; run with -vv for more info
>> #72 sk_assign:FAIL

(and what about this one, now that I'm asking?)

>> test_test_bpffs:FAIL:bpffs test  failed 255
>> #96 test_bpffs:FAIL
>> Summary: 113/844 PASSED, 14 SKIPPED, 4 FAILED
>>
>> The test_bpffs failure happens because the umh is missing from the
>> .config; and when I tried to fix this I ended up with:
>
> yeah, seems like selftests/bpf/config needs to be updated to mention
> UMH-related config values:
>
> CONFIG_BPF_PRELOAD=y
> CONFIG_BPF_PRELOAD_UMD=m|y
>
> with that test_bpffs shouldn't fail on master

Yup, did get that far, and got the below...

>>
>> [..]
>>   CC [M]  kernel/bpf/preload/bpf_preload_kern.o
>>
>> Auto-detecting system features:
>> ...                        libelf: [ OFF ]
>> ...                          zlib: [ OFF ]
>> ...                           bpf: [ OFF ]
>>
>> No libelf found
>
> might be worthwhile to look into why detection fails, might be
> something with Makefiles or your environment

I think it's actually another instance of the bug I fixed with this
commit:

1eb832ac2dee ("tools/bpf: build: Make sure resolve_btfids cleans up after itself")

which I finally remembered after being tickled by the error message
seeming familiar. And indeed, manually removing the 'feature' directory
in kernel/bpf/preload seems to fix the issue, so I'm planning to go fix
that Makefile as well...

>> ...which I just put down to random breakage, turned off the umh and
>> continued on my way (ignoring the failed test). Until you wrote this I
>> did not suspect this would be something I needed to pay attention to.
>> Now that you did mention it, I'll obviously go investigate some more, my
>> point is just that in this instance it's not accurate to assume I just
>> didn't run the tests... :)
>
> Don't just assume some tests are always broken. Either ask or
> investigate on your own. Such cases do happen from time to time while
> we wait for a fix in bpf to get merged into bpf-next or vice versa,
> but it's rare. We now have two different CI systems running selftests
> all the time, in addition to running them locally as well, so any
> permanent test failure is very apparent and annoying, so we fix them
> quickly. So, when in doubt - ask or fix.

That's good to know; and I do think the situation has improved
immensely. There was a time when the selftests broke every other week
(or so it felt, at least), and I guess I'm still a bit scarred from
that.

One thing that would be really useful would be to have a 'reference
config' or something like that. Missing config options are a common
reason for test failures (as we have just seen above), and it's not
always obvious which option is missing for each test. Even something
like grepping .config for BPF doesn't catch everything. If you already
have a CI running, just pointing to that config would be a good start
(especially if it has history). In an ideal world I think it would be
great if each test could detect whether the kernel has the right config
set for its features and abort with a clear error message if it isn't...

>> > I think I will just start marking patches as changes-requested when I see that
>> > they break tests without replying and without reviewing.
>> > Please respect reviewer's time.
>>
>> That is completely fine if the tests are working in the first place. And
>
> They are and hopefully moving forward that would be your assumption.

Sure, with the exception of the two tests still failing that I mentioned
above. Which I'm hoping you can help figure out the reason for :)

-Toke

Re: [PATCH bpf-next v8 04/11] bpf: move prog->aux->linked_prog and trampoline into bpf_link on attach

[Toke Høiland-Jørgensen]

>> > I think I will just start marking patches as changes-requested when I see that
>> > they break tests without replying and without reviewing.
>> > Please respect reviewer's time.
>>
>> That is completely fine if the tests are working in the first place. And
>> even when they're not (like in this case), pointing it out is fine, and
>> I'll obviously go investigate. But please at least reply to the email,
>> not all of us watch patchwork regularly.
>
> Please see Documentation/bpf/bpf_devel_QA.rst.
> patchwork status is the way we communicate the intent.
> If the patch is not in the queue it won't be acted upon.

I do realise that you guys use patchwork as the status tracker, but from
a submitter PoV, in practice a change there is coupled with an email
either requesting something change, or notifying of merge. Which is
fine, and I'm not asking you to do anything differently. I'm just
suggesting that if you start silently marking patches as 'changes
requested' without emailing the submitter explaining why, that will just
going to end up creating confusion, and you'll get questions and/or
identical resubmissions. So it won't actually solve anything...

(And to be clear, I'm not saying this because I plan to deliberately
submit patches with broken selftests in the future!)

-Toke

Re: [PATCH bpf-next v8 04/11] bpf: move prog->aux->linked_prog and trampoline into bpf_link on attach

[Toke Høiland-Jørgensen]

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

>> +	struct mutex tgt_mutex; /* protects tgt_* pointers below, *after* prog becomes visible */
>> +	struct bpf_prog *tgt_prog;
>> +	struct bpf_trampoline *tgt_trampoline;
>>  	bool verifier_zext; /* Zero extensions has been inserted by verifier. */
>>  	bool offload_requested;
>>  	bool attach_btf_trace; /* true if attaching to BTF-enabled raw tp */
> ...
>>  struct bpf_tracing_link {
>>  	struct bpf_link link;
>>  	enum bpf_attach_type attach_type;
>> +	struct bpf_trampoline *trampoline;
>> +	struct bpf_prog *tgt_prog;
>
> imo it's confusing to have 'tgt_prog' to mean two different things.
> In prog->aux->tgt_prog it means target prog to attach to in the future.
> Whereas here it means the existing prog that was used to attached to.
> They kinda both 'target progs' but would be good to disambiguate.
> May be keep it as 'tgt_prog' here and
> rename to 'dest_prog' and 'dest_trampoline' in prog->aux ?

I started changing this as you suggested, but I think it actually makes
the code weirder. We'll end up with a lot of 'tgt_prog =
prog->aux->dest_prog' assignments in the verifier, unless we also rename
all of the local variables, which I think is just code churn for very
little gain (the existing 'target' meaning is quite clear, I think).

I also think it's quite natural that the target moves; I mean, it's
literally the same pointer being re-assigned from prog->aux to the link.
We could rename the link member to 'attached_tgt_prog' or something like
that, but I'm not sure it helps (and I don't see much of a problem in
the first place).

WDYT?

-Toke

Re: [PATCH bpf-next v8 04/11] bpf: move prog->aux->linked_prog and trampoline into bpf_link on attach

[Andrii Nakryiko]

On Thu, Sep 24, 2020 at 2:24 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
>
> > On Thu, Sep 24, 2020 at 7:36 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
> >>
> >> Alexei Starovoitov <alexei.starovoitov@gmail.com> writes:
> >>
> >> > On Tue, Sep 22, 2020 at 08:38:38PM +0200, Toke Høiland-Jørgensen wrote:
> >> >> @@ -746,7 +748,9 @@ struct bpf_prog_aux {
> >> >>      u32 max_rdonly_access;
> >> >>      u32 max_rdwr_access;
> >> >>      const struct bpf_ctx_arg_aux *ctx_arg_info;
> >> >> -    struct bpf_prog *linked_prog;
> >> >
> >> > This change breaks bpf_preload and selftests test_bpffs.
> >> > There is really no excuse not to run the selftests.
> >>
> >> I did run the tests, and saw no more breakages after applying my patches
> >> than before. Which didn't catch this, because this is the current state
> >> of bpf-next selftests:
> >>
> >> # ./test_progs  | grep FAIL
> >> test_lookup_update:FAIL:map1_leak inner_map1 leaked!
> >> #10/1 lookup_update:FAIL
> >> #10 btf_map_in_map:FAIL
> >
> > this failure suggests you are not running the latest kernel, btw
>
> I did see that discussion (about the reverted patch), and figured that
> was the case. So I did a 'git pull' just before testing, and still got
> this.
>
> $ git describe HEAD
> v5.9-rc3-2681-g182bf3f3ddb6
>
> so any other ideas? :)

That memory leak was fixed in 1d4e1eab456e ("bpf: Fix map leak in
HASH_OF_MAPS map") at the end of July. So while your git repo might be
checked out on a recent enough commit, could it be that the kernel
that you are running is not what you think you are running?

I specifically built kernel from the same commit and double-checked:

[vmuser@archvm bpf]$ uname -r
5.9.0-rc6-01779-g182bf3f3ddb6
[vmuser@archvm bpf]$ sudo ./test_progs -t map_in_map
#10/1 lookup_update:OK
#10/2 diff_size:OK
#10 btf_map_in_map:OK
Summary: 1/2 PASSED, 0 SKIPPED, 0 FAILED

>
> >> configure_stack:FAIL:BPF load failed; run with -vv for more info
> >> #72 sk_assign:FAIL
>
> (and what about this one, now that I'm asking?)

Did you run with -vv? Jakub Sitnicki (cc'd) might probably help, if
you provide a bit more details.

>
> >> test_test_bpffs:FAIL:bpffs test  failed 255
> >> #96 test_bpffs:FAIL
> >> Summary: 113/844 PASSED, 14 SKIPPED, 4 FAILED
> >>
> >> The test_bpffs failure happens because the umh is missing from the
> >> .config; and when I tried to fix this I ended up with:
> >
> > yeah, seems like selftests/bpf/config needs to be updated to mention
> > UMH-related config values:
> >
> > CONFIG_BPF_PRELOAD=y
> > CONFIG_BPF_PRELOAD_UMD=m|y
> >
> > with that test_bpffs shouldn't fail on master
>
> Yup, did get that far, and got the below...
>
> >>
> >> [..]
> >>   CC [M]  kernel/bpf/preload/bpf_preload_kern.o
> >>
> >> Auto-detecting system features:
> >> ...                        libelf: [ OFF ]
> >> ...                          zlib: [ OFF ]
> >> ...                           bpf: [ OFF ]
> >>
> >> No libelf found
> >
> > might be worthwhile to look into why detection fails, might be
> > something with Makefiles or your environment
>
> I think it's actually another instance of the bug I fixed with this
> commit:
>
> 1eb832ac2dee ("tools/bpf: build: Make sure resolve_btfids cleans up after itself")
>
> which I finally remembered after being tickled by the error message
> seeming familiar. And indeed, manually removing the 'feature' directory
> in kernel/bpf/preload seems to fix the issue, so I'm planning to go fix
> that Makefile as well...
>

glad we got to the bottom of it

> >> ...which I just put down to random breakage, turned off the umh and
> >> continued on my way (ignoring the failed test). Until you wrote this I
> >> did not suspect this would be something I needed to pay attention to.
> >> Now that you did mention it, I'll obviously go investigate some more, my
> >> point is just that in this instance it's not accurate to assume I just
> >> didn't run the tests... :)
> >
> > Don't just assume some tests are always broken. Either ask or
> > investigate on your own. Such cases do happen from time to time while
> > we wait for a fix in bpf to get merged into bpf-next or vice versa,
> > but it's rare. We now have two different CI systems running selftests
> > all the time, in addition to running them locally as well, so any
> > permanent test failure is very apparent and annoying, so we fix them
> > quickly. So, when in doubt - ask or fix.
>
> That's good to know; and I do think the situation has improved
> immensely. There was a time when the selftests broke every other week
> (or so it felt, at least), and I guess I'm still a bit scarred from
> that.
>
> One thing that would be really useful would be to have a 'reference
> config' or something like that. Missing config options are a common
> reason for test failures (as we have just seen above), and it's not
> always obvious which option is missing for each test. Even something
> like grepping .config for BPF doesn't catch everything. If you already
> have a CI running, just pointing to that config would be a good start
> (especially if it has history). In an ideal world I think it would be
> great if each test could detect whether the kernel has the right config
> set for its features and abort with a clear error message if it isn't...

so tools/testing/selftests/bpf/config is intended to list all the
config values necessary, but given we don't update them often we
forget to update them when selftests requiring extra kernel config are
added, unfortunately.

As for CI's config, check [0], that's what we use to build kernels.
Kernel config is intentionally pretty minimal and is running in a
single-user mode in pretty stripped down environment, so might not
work as is for full-blown VM. But you can still take a look.

  [0] https://github.com/libbpf/libbpf/blob/master/travis-ci/vmtest/configs/latest.config

>
> >> > I think I will just start marking patches as changes-requested when I see that
> >> > they break tests without replying and without reviewing.
> >> > Please respect reviewer's time.
> >>
> >> That is completely fine if the tests are working in the first place. And
> >
> > They are and hopefully moving forward that would be your assumption.
>
> Sure, with the exception of the two tests still failing that I mentioned
> above. Which I'm hoping you can help figure out the reason for :)
>
> -Toke
>

Re: [PATCH bpf-next v8 04/11] bpf: move prog->aux->linked_prog and trampoline into bpf_link on attach

[Toke Høiland-Jørgensen]

Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:

> On Thu, Sep 24, 2020 at 2:24 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>>
>> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
>>
>> > On Thu, Sep 24, 2020 at 7:36 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>> >>
>> >> Alexei Starovoitov <alexei.starovoitov@gmail.com> writes:
>> >>
>> >> > On Tue, Sep 22, 2020 at 08:38:38PM +0200, Toke Høiland-Jørgensen wrote:
>> >> >> @@ -746,7 +748,9 @@ struct bpf_prog_aux {
>> >> >>      u32 max_rdonly_access;
>> >> >>      u32 max_rdwr_access;
>> >> >>      const struct bpf_ctx_arg_aux *ctx_arg_info;
>> >> >> -    struct bpf_prog *linked_prog;
>> >> >
>> >> > This change breaks bpf_preload and selftests test_bpffs.
>> >> > There is really no excuse not to run the selftests.
>> >>
>> >> I did run the tests, and saw no more breakages after applying my patches
>> >> than before. Which didn't catch this, because this is the current state
>> >> of bpf-next selftests:
>> >>
>> >> # ./test_progs  | grep FAIL
>> >> test_lookup_update:FAIL:map1_leak inner_map1 leaked!
>> >> #10/1 lookup_update:FAIL
>> >> #10 btf_map_in_map:FAIL
>> >
>> > this failure suggests you are not running the latest kernel, btw
>>
>> I did see that discussion (about the reverted patch), and figured that
>> was the case. So I did a 'git pull' just before testing, and still got
>> this.
>>
>> $ git describe HEAD
>> v5.9-rc3-2681-g182bf3f3ddb6
>>
>> so any other ideas? :)
>
> That memory leak was fixed in 1d4e1eab456e ("bpf: Fix map leak in
> HASH_OF_MAPS map") at the end of July. So while your git repo might be
> checked out on a recent enough commit, could it be that the kernel
> that you are running is not what you think you are running?

Nah, I'm running these in a one-shot virtual machine with virtme-run.

> I specifically built kernel from the same commit and double-checked:
>
> [vmuser@archvm bpf]$ uname -r
> 5.9.0-rc6-01779-g182bf3f3ddb6
> [vmuser@archvm bpf]$ sudo ./test_progs -t map_in_map
> #10/1 lookup_update:OK
> #10/2 diff_size:OK
> #10 btf_map_in_map:OK
> Summary: 1/2 PASSED, 0 SKIPPED, 0 FAILED

Trying the same, while manually entering the VM:

[root@(none) bpf]# uname -r
5.9.0-rc6-02685-g64363ff12e8f
[root@(none) bpf]# ./test_progs -t map_in_map
test_lookup_update:PASS:skel_open 0 nsec
test_lookup_update:PASS:skel_attach 0 nsec
test_lookup_update:PASS:inner1 0 nsec
test_lookup_update:PASS:inner2 0 nsec
test_lookup_update:PASS:inner1 0 nsec
test_lookup_update:PASS:inner2 0 nsec
test_lookup_update:PASS:map1_id 0 nsec
test_lookup_update:PASS:map2_id 0 nsec
kern_sync_rcu:PASS:inner_map_create 0 nsec
kern_sync_rcu:PASS:outer_map_create 0 nsec
kern_sync_rcu:PASS:outer_map_update 0 nsec
test_lookup_update:PASS:sync_rcu 0 nsec
kern_sync_rcu:PASS:inner_map_create 0 nsec
kern_sync_rcu:PASS:outer_map_create 0 nsec
kern_sync_rcu:PASS:outer_map_update 0 nsec
test_lookup_update:PASS:sync_rcu 0 nsec
test_lookup_update:FAIL:map1_leak inner_map1 leaked!
#10/1 lookup_update:FAIL
#10/2 diff_size:OK
#10 btf_map_in_map:FAIL
Summary: 0/1 PASSED, 0 SKIPPED, 2 FAILED


>> >> configure_stack:FAIL:BPF load failed; run with -vv for more info
>> >> #72 sk_assign:FAIL
>>
>> (and what about this one, now that I'm asking?)
>
> Did you run with -vv? Jakub Sitnicki (cc'd) might probably help, if
> you provide a bit more details.

No, I didn't, silly me. Turned out that was also just a missing config
option - thanks! :)

>> One thing that would be really useful would be to have a 'reference
>> config' or something like that. Missing config options are a common
>> reason for test failures (as we have just seen above), and it's not
>> always obvious which option is missing for each test. Even something
>> like grepping .config for BPF doesn't catch everything. If you already
>> have a CI running, just pointing to that config would be a good start
>> (especially if it has history). In an ideal world I think it would be
>> great if each test could detect whether the kernel has the right config
>> set for its features and abort with a clear error message if it isn't...
>
> so tools/testing/selftests/bpf/config is intended to list all the
> config values necessary, but given we don't update them often we
> forget to update them when selftests requiring extra kernel config are
> added, unfortunately.

Ah, that's useful! I wonder how difficult it would be to turn this into
a 'make bpfconfig' top-level make target (similar to 'make defconfig')?

That way, it could be run automatically, and we would also catch
anything missing?

> As for CI's config, check [0], that's what we use to build kernels.
> Kernel config is intentionally pretty minimal and is running in a
> single-user mode in pretty stripped down environment, so might not
> work as is for full-blown VM. But you can still take a look.
>
>   [0] https://github.com/libbpf/libbpf/blob/master/travis-ci/vmtest/configs/latest.config

Well that's how I'm running my own tests (as mentioned above), so that
might be useful, actually! I'll go take a look, thanks :)

-Toke

Re: [PATCH bpf-next v8 04/11] bpf: move prog->aux->linked_prog and trampoline into bpf_link on attach

[Andrii Nakryiko]

On Thu, Sep 24, 2020 at 3:20 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
>
> > On Thu, Sep 24, 2020 at 2:24 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
> >>
> >> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
> >>
> >> > On Thu, Sep 24, 2020 at 7:36 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
> >> >>
> >> >> Alexei Starovoitov <alexei.starovoitov@gmail.com> writes:
> >> >>
> >> >> > On Tue, Sep 22, 2020 at 08:38:38PM +0200, Toke Høiland-Jørgensen wrote:
> >> >> >> @@ -746,7 +748,9 @@ struct bpf_prog_aux {
> >> >> >>      u32 max_rdonly_access;
> >> >> >>      u32 max_rdwr_access;
> >> >> >>      const struct bpf_ctx_arg_aux *ctx_arg_info;
> >> >> >> -    struct bpf_prog *linked_prog;
> >> >> >
> >> >> > This change breaks bpf_preload and selftests test_bpffs.
> >> >> > There is really no excuse not to run the selftests.
> >> >>
> >> >> I did run the tests, and saw no more breakages after applying my patches
> >> >> than before. Which didn't catch this, because this is the current state
> >> >> of bpf-next selftests:
> >> >>
> >> >> # ./test_progs  | grep FAIL
> >> >> test_lookup_update:FAIL:map1_leak inner_map1 leaked!
> >> >> #10/1 lookup_update:FAIL
> >> >> #10 btf_map_in_map:FAIL
> >> >
> >> > this failure suggests you are not running the latest kernel, btw
> >>
> >> I did see that discussion (about the reverted patch), and figured that
> >> was the case. So I did a 'git pull' just before testing, and still got
> >> this.
> >>
> >> $ git describe HEAD
> >> v5.9-rc3-2681-g182bf3f3ddb6
> >>
> >> so any other ideas? :)
> >
> > That memory leak was fixed in 1d4e1eab456e ("bpf: Fix map leak in
> > HASH_OF_MAPS map") at the end of July. So while your git repo might be
> > checked out on a recent enough commit, could it be that the kernel
> > that you are running is not what you think you are running?
>
> Nah, I'm running these in a one-shot virtual machine with virtme-run.
>
> > I specifically built kernel from the same commit and double-checked:
> >
> > [vmuser@archvm bpf]$ uname -r
> > 5.9.0-rc6-01779-g182bf3f3ddb6
> > [vmuser@archvm bpf]$ sudo ./test_progs -t map_in_map
> > #10/1 lookup_update:OK
> > #10/2 diff_size:OK
> > #10 btf_map_in_map:OK
> > Summary: 1/2 PASSED, 0 SKIPPED, 0 FAILED
>
> Trying the same, while manually entering the VM:
>
> [root@(none) bpf]# uname -r
> 5.9.0-rc6-02685-g64363ff12e8f

I don't see 64363ff12e8f sha in my repo, so I still don't know what
commit your kernel is built off of. But I believe that you have the
latest kernel, you'll just need to debug this on your own, though,
because this test was never flaky for me, I can't repro the failure.

> [root@(none) bpf]# ./test_progs -t map_in_map
> test_lookup_update:PASS:skel_open 0 nsec
> test_lookup_update:PASS:skel_attach 0 nsec
> test_lookup_update:PASS:inner1 0 nsec
> test_lookup_update:PASS:inner2 0 nsec
> test_lookup_update:PASS:inner1 0 nsec
> test_lookup_update:PASS:inner2 0 nsec
> test_lookup_update:PASS:map1_id 0 nsec
> test_lookup_update:PASS:map2_id 0 nsec
> kern_sync_rcu:PASS:inner_map_create 0 nsec
> kern_sync_rcu:PASS:outer_map_create 0 nsec
> kern_sync_rcu:PASS:outer_map_update 0 nsec
> test_lookup_update:PASS:sync_rcu 0 nsec
> kern_sync_rcu:PASS:inner_map_create 0 nsec
> kern_sync_rcu:PASS:outer_map_create 0 nsec
> kern_sync_rcu:PASS:outer_map_update 0 nsec
> test_lookup_update:PASS:sync_rcu 0 nsec

try adding sleep(few seconds, enough for RCU grace period to pass)
here and see if that helps

if not, please printk() around to see why the inner_map1 wasn't freed

> test_lookup_update:FAIL:map1_leak inner_map1 leaked!
> #10/1 lookup_update:FAIL
> #10/2 diff_size:OK
> #10 btf_map_in_map:FAIL
> Summary: 0/1 PASSED, 0 SKIPPED, 2 FAILED
>
>
> >> >> configure_stack:FAIL:BPF load failed; run with -vv for more info
> >> >> #72 sk_assign:FAIL
> >>
> >> (and what about this one, now that I'm asking?)
> >
> > Did you run with -vv? Jakub Sitnicki (cc'd) might probably help, if
> > you provide a bit more details.
>
> No, I didn't, silly me. Turned out that was also just a missing config
> option - thanks! :)

ok, cool

>
> >> One thing that would be really useful would be to have a 'reference
> >> config' or something like that. Missing config options are a common
> >> reason for test failures (as we have just seen above), and it's not
> >> always obvious which option is missing for each test. Even something
> >> like grepping .config for BPF doesn't catch everything. If you already
> >> have a CI running, just pointing to that config would be a good start
> >> (especially if it has history). In an ideal world I think it would be
> >> great if each test could detect whether the kernel has the right config
> >> set for its features and abort with a clear error message if it isn't...
> >
> > so tools/testing/selftests/bpf/config is intended to list all the
> > config values necessary, but given we don't update them often we
> > forget to update them when selftests requiring extra kernel config are
> > added, unfortunately.
>
> Ah, that's useful! I wonder how difficult it would be to turn this into
> a 'make bpfconfig' top-level make target (similar to 'make defconfig')?
>
> That way, it could be run automatically, and we would also catch
> anything missing?

no idea, might be worth trying

>
> > As for CI's config, check [0], that's what we use to build kernels.
> > Kernel config is intentionally pretty minimal and is running in a
> > single-user mode in pretty stripped down environment, so might not
> > work as is for full-blown VM. But you can still take a look.
> >
> >   [0] https://github.com/libbpf/libbpf/blob/master/travis-ci/vmtest/configs/latest.config
>
> Well that's how I'm running my own tests (as mentioned above), so that
> might be useful, actually! I'll go take a look, thanks :)

glad I could help

>
> -Toke
>

Re: [PATCH bpf-next v8 04/11] bpf: move prog->aux->linked_prog and trampoline into bpf_link on attach

[Toke Høiland-Jørgensen]

Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:

>> [root@(none) bpf]# ./test_progs -t map_in_map
>> test_lookup_update:PASS:skel_open 0 nsec
>> test_lookup_update:PASS:skel_attach 0 nsec
>> test_lookup_update:PASS:inner1 0 nsec
>> test_lookup_update:PASS:inner2 0 nsec
>> test_lookup_update:PASS:inner1 0 nsec
>> test_lookup_update:PASS:inner2 0 nsec
>> test_lookup_update:PASS:map1_id 0 nsec
>> test_lookup_update:PASS:map2_id 0 nsec
>> kern_sync_rcu:PASS:inner_map_create 0 nsec
>> kern_sync_rcu:PASS:outer_map_create 0 nsec
>> kern_sync_rcu:PASS:outer_map_update 0 nsec
>> test_lookup_update:PASS:sync_rcu 0 nsec
>> kern_sync_rcu:PASS:inner_map_create 0 nsec
>> kern_sync_rcu:PASS:outer_map_create 0 nsec
>> kern_sync_rcu:PASS:outer_map_update 0 nsec
>> test_lookup_update:PASS:sync_rcu 0 nsec
>
> try adding sleep(few seconds, enough for RCU grace period to pass)
> here and see if that helps
>
> if not, please printk() around to see why the inner_map1 wasn't freed

Aha, found it! It happened because my kernel was built with
PREEMPT_VOLUNTARY. Changing that to PREEMPT fixed the test, and got me
to:

Summary: 116/853 PASSED, 14 SKIPPED, 0 FAILED

So yay! Thanks for your help with debugging :)

-Toke

Re: [PATCH bpf-next v8 11/11] selftests: Remove fmod_ret from benchmarks and test_overhead

[Toke Høiland-Jørgensen]

Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:

> On Wed, Sep 23, 2020 at 6:08 PM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
>>
>> On Tue, Sep 22, 2020 at 08:38:45PM +0200, Toke Høiland-Jørgensen wrote:
>> > -const struct bench bench_trig_fmodret = {
>> > -     .name = "trig-fmodret",
>> > -     .validate = trigger_validate,
>> > -     .setup = trigger_fmodret_setup,
>> > -     .producer_thread = trigger_producer,
>> > -     .consumer_thread = trigger_consumer,
>> > -     .measure = trigger_measure,
>> > -     .report_progress = hits_drops_report_progress,
>> > -     .report_final = hits_drops_report_final,
>> > -};
>> > diff --git a/tools/testing/selftests/bpf/progs/trigger_bench.c b/tools/testing/selftests/bpf/progs/trigger_bench.c
>> > index 9a4d09590b3d..1af23ac0c37c 100644
>> > --- a/tools/testing/selftests/bpf/progs/trigger_bench.c
>> > +++ b/tools/testing/selftests/bpf/progs/trigger_bench.c
>> > @@ -45,10 +45,3 @@ int bench_trigger_fentry_sleep(void *ctx)
>> >       __sync_add_and_fetch(&hits, 1);
>> >       return 0;
>> >  }
>> > -
>> > -SEC("fmod_ret/__x64_sys_getpgid")
>> > -int bench_trigger_fmodret(void *ctx)
>> > -{
>> > -     __sync_add_and_fetch(&hits, 1);
>> > -     return -22;
>> > -}
>>
>> why are you removing this? There is no problem here.
>> All syscalls are error-injectable.
>> I'm surprised Andrii acked this :(
>
> Andrii didn't know that all syscalls are error-injectable, thanks for
> catching :) after fmod_ret/__set_task_comm I just assumed that I've
> been abusing fmod_ret all this time...

I didn't know that either. Shall I just drop your ACK from the next
version so you can take another look?

-Toke

Re: [PATCH bpf-next v8 04/11] bpf: move prog->aux->linked_prog and trampoline into bpf_link on attach

[Alexei Starovoitov]

On Thu, Sep 24, 2020 at 3:00 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> Alexei Starovoitov <alexei.starovoitov@gmail.com> writes:
>
> >> +    struct mutex tgt_mutex; /* protects tgt_* pointers below, *after* prog becomes visible */
> >> +    struct bpf_prog *tgt_prog;
> >> +    struct bpf_trampoline *tgt_trampoline;
> >>      bool verifier_zext; /* Zero extensions has been inserted by verifier. */
> >>      bool offload_requested;
> >>      bool attach_btf_trace; /* true if attaching to BTF-enabled raw tp */
> > ...
> >>  struct bpf_tracing_link {
> >>      struct bpf_link link;
> >>      enum bpf_attach_type attach_type;
> >> +    struct bpf_trampoline *trampoline;
> >> +    struct bpf_prog *tgt_prog;
> >
> > imo it's confusing to have 'tgt_prog' to mean two different things.
> > In prog->aux->tgt_prog it means target prog to attach to in the future.
> > Whereas here it means the existing prog that was used to attached to.
> > They kinda both 'target progs' but would be good to disambiguate.
> > May be keep it as 'tgt_prog' here and
> > rename to 'dest_prog' and 'dest_trampoline' in prog->aux ?
>
> I started changing this as you suggested, but I think it actually makes
> the code weirder. We'll end up with a lot of 'tgt_prog =
> prog->aux->dest_prog' assignments in the verifier, unless we also rename
> all of the local variables, which I think is just code churn for very
> little gain (the existing 'target' meaning is quite clear, I think).

you mean "churn" just for this patch. that's fine.
But it will make names more accurate for everyone reading it afterwards.
Hence I prefer distinct and specific names where possible.

> I also think it's quite natural that the target moves; I mean, it's
> literally the same pointer being re-assigned from prog->aux to the link.
> We could rename the link member to 'attached_tgt_prog' or something like
> that, but I'm not sure it helps (and I don't see much of a problem in
> the first place).

'attached_tgt_prog' will not be the correct name.
There is 'prog' inside the link already. That's 'attached' prog.
Not this one. This one is the 'attached_to' prog.
But such name would be too long.
imo calling it 'dest_prog' in aux is shorter and more obvious.

Re: [PATCH bpf-next v8 04/11] bpf: move prog->aux->linked_prog and trampoline into bpf_link on attach

[Toke Høiland-Jørgensen]

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

> On Thu, Sep 24, 2020 at 3:00 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>>
>> Alexei Starovoitov <alexei.starovoitov@gmail.com> writes:
>>
>> >> +    struct mutex tgt_mutex; /* protects tgt_* pointers below, *after* prog becomes visible */
>> >> +    struct bpf_prog *tgt_prog;
>> >> +    struct bpf_trampoline *tgt_trampoline;
>> >>      bool verifier_zext; /* Zero extensions has been inserted by verifier. */
>> >>      bool offload_requested;
>> >>      bool attach_btf_trace; /* true if attaching to BTF-enabled raw tp */
>> > ...
>> >>  struct bpf_tracing_link {
>> >>      struct bpf_link link;
>> >>      enum bpf_attach_type attach_type;
>> >> +    struct bpf_trampoline *trampoline;
>> >> +    struct bpf_prog *tgt_prog;
>> >
>> > imo it's confusing to have 'tgt_prog' to mean two different things.
>> > In prog->aux->tgt_prog it means target prog to attach to in the future.
>> > Whereas here it means the existing prog that was used to attached to.
>> > They kinda both 'target progs' but would be good to disambiguate.
>> > May be keep it as 'tgt_prog' here and
>> > rename to 'dest_prog' and 'dest_trampoline' in prog->aux ?
>>
>> I started changing this as you suggested, but I think it actually makes
>> the code weirder. We'll end up with a lot of 'tgt_prog =
>> prog->aux->dest_prog' assignments in the verifier, unless we also rename
>> all of the local variables, which I think is just code churn for very
>> little gain (the existing 'target' meaning is quite clear, I think).
>
> you mean "churn" just for this patch. that's fine.
> But it will make names more accurate for everyone reading it afterwards.
> Hence I prefer distinct and specific names where possible.
>
>> I also think it's quite natural that the target moves; I mean, it's
>> literally the same pointer being re-assigned from prog->aux to the link.
>> We could rename the link member to 'attached_tgt_prog' or something like
>> that, but I'm not sure it helps (and I don't see much of a problem in
>> the first place).
>
> 'attached_tgt_prog' will not be the correct name.
> There is 'prog' inside the link already. That's 'attached' prog.
> Not this one. This one is the 'attached_to' prog.
> But such name would be too long.
> imo calling it 'dest_prog' in aux is shorter and more obvious.

Meh, don't really see how it helps ('destination' and 'target' are
literally synonyms). But I don't care enough to bikeshed about it
either, so I'll just do a search/replace...

-Toke