[PATCH bpf-next v10 0/2] Fix attaching fentry/fexit/fmod_ret/lsm to modules

[PATCH bpf-next v10 0/2] Fix attaching fentry/fexit/fmod_ret/lsm to modules

[Viktor Malik]

I noticed that the verifier behaves incorrectly when attaching to fentry
of multiple functions of the same name located in different modules (or
in vmlinux). The reason for this is that if the target program is not
specified, the verifier will search kallsyms for the trampoline address
to attach to. The entire kallsyms is always searched, not respecting the
module in which the function to attach to is located.

As Yonghong correctly pointed out, there is yet another issue - the
trampoline acquires the module reference in register_fentry which means
that if the module is unloaded between the place where the address is
found in the verifier and register_fentry, it is possible that another
module is loaded to the same address in the meantime, which may lead to
errors.

This patch fixes the above issues by extracting the module name from the
BTF of the attachment target (which must be specified) and by doing the
search in kallsyms of the correct module. At the same time, the module
reference is acquired right after the address is found and only released
right before the program itself is unloaded.

---
Changes in v10:
- added the new test to DENYLIST.aarch64 (suggested by Andrii)
- renamed the test source file to match the test name

Changes in v9:
- two small changes suggested by Jiri Olsa and Jiri's ack

Changes in v8:
- added module_put to error paths in bpf_check_attach_target after the
  module reference is acquired

Changes in v7:
- refactored the module reference manipulation (comments by Jiri Olsa)
- cleaned up the test (comments by Andrii Nakryiko)

Changes in v6:
- storing the module reference inside bpf_prog_aux instead of
  bpf_trampoline and releasing it when the program is unloaded
  (suggested by Jiri Olsa)

Changes in v5:
- fixed acquiring and releasing of module references by trampolines to
  prevent modules being unloaded between address lookup and trampoline
  allocation

Changes in v4:
- reworked module kallsyms lookup approach using existing functions,
  verifier now calls btf_try_get_module to retrieve the module and
  find_kallsyms_symbol_value to get the symbol address (suggested by
  Alexei)
- included Jiri Olsa's comments
- improved description of the new test and added it as a comment into
  the test source

Changes in v3:
- added trivial implementation for kallsyms_lookup_name_in_module() for
  !CONFIG_MODULES (noticed by test robot, fix suggested by Hao Luo)

Changes in v2:
- introduced and used more space-efficient kallsyms lookup function,
  suggested by Jiri Olsa
- included Hao Luo's comments

Viktor Malik (2):
  bpf: Fix attaching fentry/fexit/fmod_ret/lsm to modules
  bpf/selftests: Test fentry attachment to shadowed functions

 include/linux/bpf.h                           |   2 +
 kernel/bpf/syscall.c                          |   6 +
 kernel/bpf/trampoline.c                       |  28 ----
 kernel/bpf/verifier.c                         |  18 ++-
 kernel/module/internal.h                      |   5 +
 net/bpf/test_run.c                            |   5 +
 tools/testing/selftests/bpf/DENYLIST.aarch64  |   1 +
 .../selftests/bpf/bpf_testmod/bpf_testmod.c   |   6 +
 .../bpf/prog_tests/module_fentry_shadow.c     | 128 ++++++++++++++++++
 9 files changed, 170 insertions(+), 29 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/module_fentry_shadow.c

-- 
2.39.2

[PATCH bpf-next v10 1/2] bpf: Fix attaching fentry/fexit/fmod_ret/lsm to modules

[Viktor Malik]

This resolves two problems with attachment of fentry/fexit/fmod_ret/lsm
to functions located in modules:

1. The verifier tries to find the address to attach to in kallsyms. This
   is always done by searching the entire kallsyms, not respecting the
   module in which the function is located. Such approach causes an
   incorrect attachment address to be computed if the function to attach
   to is shadowed by a function of the same name located earlier in
   kallsyms.

2. If the address to attach to is located in a module, the module
   reference is only acquired in register_fentry. If the module is
   unloaded between the place where the address is found
   (bpf_check_attach_target in the verifier) and register_fentry, it is
   possible that another module is loaded to the same address which may
   lead to potential errors.

Since the attachment must contain the BTF of the program to attach to,
we extract the module from it and search for the function address in the
correct module (resolving problem no. 1). Then, the module reference is
taken directly in bpf_check_attach_target and stored in the bpf program
(in bpf_prog_aux). The reference is only released when the program is
unloaded (resolving problem no. 2).

Signed-off-by: Viktor Malik <vmalik@redhat.com>
Acked-by: Jiri Olsa <jolsa@kernel.org>
---
 include/linux/bpf.h      |  2 ++
 kernel/bpf/syscall.c     |  6 ++++++
 kernel/bpf/trampoline.c  | 28 ----------------------------
 kernel/bpf/verifier.c    | 18 +++++++++++++++++-
 kernel/module/internal.h |  5 +++++
 5 files changed, 30 insertions(+), 29 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index e64ff1e89fb2..4aade43f61f0 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1098,6 +1098,7 @@ struct bpf_trampoline {
 struct bpf_attach_target_info {
 	struct btf_func_model fmodel;
 	long tgt_addr;
+	struct module *tgt_mod;
 	const char *tgt_name;
 	const struct btf_type *tgt_type;
 };
@@ -1401,6 +1402,7 @@ struct bpf_prog_aux {
 	 * main prog always has linfo_idx == 0
 	 */
 	u32 linfo_idx;
+	struct module *mod;
 	u32 num_exentries;
 	struct exception_table_entry *extable;
 	union {
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index f406dfa13792..30fac7d9eddb 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -2051,6 +2051,7 @@ static void __bpf_prog_put_noref(struct bpf_prog *prog, bool deferred)
 {
 	bpf_prog_kallsyms_del_all(prog);
 	btf_put(prog->aux->btf);
+	module_put(prog->aux->mod);
 	kvfree(prog->aux->jited_linfo);
 	kvfree(prog->aux->linfo);
 	kfree(prog->aux->kfunc_tab);
@@ -3097,6 +3098,11 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog,
 		if (err)
 			goto out_unlock;
 
+		if (tgt_info.tgt_mod) {
+			module_put(prog->aux->mod);
+			prog->aux->mod = tgt_info.tgt_mod;
+		}
+
 		tr = bpf_trampoline_get(key, &tgt_info);
 		if (!tr) {
 			err = -ENOMEM;
diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
index d0ed7d6f5eec..f61d5138b12b 100644
--- a/kernel/bpf/trampoline.c
+++ b/kernel/bpf/trampoline.c
@@ -9,7 +9,6 @@
 #include <linux/btf.h>
 #include <linux/rcupdate_trace.h>
 #include <linux/rcupdate_wait.h>
-#include <linux/module.h>
 #include <linux/static_call.h>
 #include <linux/bpf_verifier.h>
 #include <linux/bpf_lsm.h>
@@ -172,26 +171,6 @@ static struct bpf_trampoline *bpf_trampoline_lookup(u64 key)
 	return tr;
 }
 
-static int bpf_trampoline_module_get(struct bpf_trampoline *tr)
-{
-	struct module *mod;
-	int err = 0;
-
-	preempt_disable();
-	mod = __module_text_address((unsigned long) tr->func.addr);
-	if (mod && !try_module_get(mod))
-		err = -ENOENT;
-	preempt_enable();
-	tr->mod = mod;
-	return err;
-}
-
-static void bpf_trampoline_module_put(struct bpf_trampoline *tr)
-{
-	module_put(tr->mod);
-	tr->mod = NULL;
-}
-
 static int unregister_fentry(struct bpf_trampoline *tr, void *old_addr)
 {
 	void *ip = tr->func.addr;
@@ -202,8 +181,6 @@ static int unregister_fentry(struct bpf_trampoline *tr, void *old_addr)
 	else
 		ret = bpf_arch_text_poke(ip, BPF_MOD_CALL, old_addr, NULL);
 
-	if (!ret)
-		bpf_trampoline_module_put(tr);
 	return ret;
 }
 
@@ -238,9 +215,6 @@ static int register_fentry(struct bpf_trampoline *tr, void *new_addr)
 		tr->func.ftrace_managed = true;
 	}
 
-	if (bpf_trampoline_module_get(tr))
-		return -ENOENT;
-
 	if (tr->func.ftrace_managed) {
 		ftrace_set_filter_ip(tr->fops, (unsigned long)ip, 0, 1);
 		ret = register_ftrace_direct_multi(tr->fops, (long)new_addr);
@@ -248,8 +222,6 @@ static int register_fentry(struct bpf_trampoline *tr, void *new_addr)
 		ret = bpf_arch_text_poke(ip, BPF_MOD_CALL, NULL, new_addr);
 	}
 
-	if (ret)
-		bpf_trampoline_module_put(tr);
 	return ret;
 }
 
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 45a082284464..3905bb20b9a1 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -24,6 +24,7 @@
 #include <linux/bpf_lsm.h>
 #include <linux/btf_ids.h>
 #include <linux/poison.h>
+#include "../module/internal.h"
 
 #include "disasm.h"
 
@@ -18259,6 +18260,7 @@ int bpf_check_attach_target(struct bpf_verifier_log *log,
 	const char *tname;
 	struct btf *btf;
 	long addr = 0;
+	struct module *mod = NULL;
 
 	if (!btf_id) {
 		bpf_log(log, "Tracing programs must provide btf_id\n");
@@ -18432,8 +18434,17 @@ int bpf_check_attach_target(struct bpf_verifier_log *log,
 			else
 				addr = (long) tgt_prog->aux->func[subprog]->bpf_func;
 		} else {
-			addr = kallsyms_lookup_name(tname);
+			if (btf_is_module(btf)) {
+				mod = btf_try_get_module(btf);
+				if (mod)
+					addr = find_kallsyms_symbol_value(mod, tname);
+				else
+					addr = 0;
+			} else {
+				addr = kallsyms_lookup_name(tname);
+			}
 			if (!addr) {
+				module_put(mod);
 				bpf_log(log,
 					"The address of function %s cannot be found\n",
 					tname);
@@ -18473,11 +18484,13 @@ int bpf_check_attach_target(struct bpf_verifier_log *log,
 				break;
 			}
 			if (ret) {
+				module_put(mod);
 				bpf_log(log, "%s is not sleepable\n", tname);
 				return ret;
 			}
 		} else if (prog->expected_attach_type == BPF_MODIFY_RETURN) {
 			if (tgt_prog) {
+				module_put(mod);
 				bpf_log(log, "can't modify return codes of BPF programs\n");
 				return -EINVAL;
 			}
@@ -18486,6 +18499,7 @@ int bpf_check_attach_target(struct bpf_verifier_log *log,
 			    !check_attach_modify_return(addr, tname))
 				ret = 0;
 			if (ret) {
+				module_put(mod);
 				bpf_log(log, "%s() is not modifiable\n", tname);
 				return ret;
 			}
@@ -18496,6 +18510,7 @@ int bpf_check_attach_target(struct bpf_verifier_log *log,
 	tgt_info->tgt_addr = addr;
 	tgt_info->tgt_name = tname;
 	tgt_info->tgt_type = t;
+	tgt_info->tgt_mod = mod;
 	return 0;
 }
 
@@ -18575,6 +18590,7 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)
 	/* store info about the attachment target that will be used later */
 	prog->aux->attach_func_proto = tgt_info.tgt_type;
 	prog->aux->attach_func_name = tgt_info.tgt_name;
+	prog->aux->mod = tgt_info.tgt_mod;
 
 	if (tgt_prog) {
 		prog->aux->saved_dst_prog_type = tgt_prog->type;
diff --git a/kernel/module/internal.h b/kernel/module/internal.h
index 2e2bf236f558..5cb103a46018 100644
--- a/kernel/module/internal.h
+++ b/kernel/module/internal.h
@@ -256,6 +256,11 @@ static inline bool sect_empty(const Elf_Shdr *sect)
 static inline void init_build_id(struct module *mod, const struct load_info *info) { }
 static inline void layout_symtab(struct module *mod, struct load_info *info) { }
 static inline void add_kallsyms(struct module *mod, const struct load_info *info) { }
+static inline unsigned long find_kallsyms_symbol_value(struct module *mod
+						       const char *name)
+{
+	return 0;
+}
 #endif /* CONFIG_KALLSYMS */
 
 #ifdef CONFIG_SYSFS
-- 
2.39.2

[PATCH bpf-next v10 2/2] bpf/selftests: Test fentry attachment to shadowed functions

[Viktor Malik]

Adds a new test that tries to attach a program to fentry of two
functions of the same name, one located in vmlinux and the other in
bpf_testmod.

To avoid conflicts with existing tests, a new function
"bpf_fentry_shadow_test" was created both in vmlinux and in bpf_testmod.

The previous commit fixed a bug which caused this test to fail. The
verifier would always use the vmlinux function's address as the target
trampoline address, hence trying to create two trampolines for a single
address, which is forbidden.

The test (similarly to other fentry/fexit tests) is not working on arm64
at the moment.

Signed-off-by: Viktor Malik <vmalik@redhat.com>
Acked-by: Jiri Olsa <jolsa@kernel.org>
---
 net/bpf/test_run.c                            |   5 +
 tools/testing/selftests/bpf/DENYLIST.aarch64  |   1 +
 .../selftests/bpf/bpf_testmod/bpf_testmod.c   |   6 +
 .../bpf/prog_tests/module_fentry_shadow.c     | 128 ++++++++++++++++++
 4 files changed, 140 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/module_fentry_shadow.c

diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
index 6a8b33a103a4..71226f68270d 100644
--- a/net/bpf/test_run.c
+++ b/net/bpf/test_run.c
@@ -560,6 +560,11 @@ long noinline bpf_kfunc_call_test4(signed char a, short b, int c, long d)
 	return (long)a + (long)b + (long)c + d;
 }
 
+int noinline bpf_fentry_shadow_test(int a)
+{
+	return a + 1;
+}
+
 struct prog_test_member1 {
 	int a;
 };
diff --git a/tools/testing/selftests/bpf/DENYLIST.aarch64 b/tools/testing/selftests/bpf/DENYLIST.aarch64
index 99cc33c51eaa..0a6837f97c32 100644
--- a/tools/testing/selftests/bpf/DENYLIST.aarch64
+++ b/tools/testing/selftests/bpf/DENYLIST.aarch64
@@ -44,6 +44,7 @@ lookup_key                                       # test_lookup_key__attach unexp
 lru_bug                                          # lru_bug__attach unexpected error: -524 (errno 524)
 modify_return                                    # modify_return__attach failed unexpected error: -524 (errno 524)
 module_attach                                    # skel_attach skeleton attach failed: -524
+module_fentry_shadow                             # bpf_link_create unexpected bpf_link_create: actual -524 < expected 0
 mptcp/base                                       # run_test mptcp unexpected error: -524 (errno 524)
 netcnt                                           # packets unexpected packets: actual 10001 != expected 10000
 rcu_read_lock                                    # failed to attach: ERROR: strerror_r(-524)=22
diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
index 5e6e85c8d77d..7999476b9446 100644
--- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
+++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
@@ -268,6 +268,12 @@ static const struct btf_kfunc_id_set bpf_testmod_kfunc_set = {
 	.set   = &bpf_testmod_check_kfunc_ids,
 };
 
+noinline int bpf_fentry_shadow_test(int a)
+{
+	return a + 2;
+}
+EXPORT_SYMBOL_GPL(bpf_fentry_shadow_test);
+
 extern int bpf_fentry_test1(int a);
 
 static int bpf_testmod_init(void)
diff --git a/tools/testing/selftests/bpf/prog_tests/module_fentry_shadow.c b/tools/testing/selftests/bpf/prog_tests/module_fentry_shadow.c
new file mode 100644
index 000000000000..c7636e18b1eb
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/module_fentry_shadow.c
@@ -0,0 +1,128 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2022 Red Hat */
+#include <test_progs.h>
+#include <bpf/btf.h>
+#include "bpf/libbpf_internal.h"
+#include "cgroup_helpers.h"
+
+static const char *module_name = "bpf_testmod";
+static const char *symbol_name = "bpf_fentry_shadow_test";
+
+static int get_bpf_testmod_btf_fd(void)
+{
+	struct bpf_btf_info info;
+	char name[64];
+	__u32 id = 0, len;
+	int err, fd;
+
+	while (true) {
+		err = bpf_btf_get_next_id(id, &id);
+		if (err) {
+			log_err("failed to iterate BTF objects");
+			return err;
+		}
+
+		fd = bpf_btf_get_fd_by_id(id);
+		if (fd < 0) {
+			if (errno == ENOENT)
+				continue; /* expected race: BTF was unloaded */
+			err = -errno;
+			log_err("failed to get FD for BTF object #%d", id);
+			return err;
+		}
+
+		len = sizeof(info);
+		memset(&info, 0, sizeof(info));
+		info.name = ptr_to_u64(name);
+		info.name_len = sizeof(name);
+
+		err = bpf_obj_get_info_by_fd(fd, &info, &len);
+		if (err) {
+			err = -errno;
+			log_err("failed to get info for BTF object #%d", id);
+			close(fd);
+			return err;
+		}
+
+		if (strcmp(name, module_name) == 0)
+			return fd;
+
+		close(fd);
+	}
+	return -ENOENT;
+}
+
+void test_module_fentry_shadow(void)
+{
+	struct btf *vmlinux_btf = NULL, *mod_btf = NULL;
+	int err, i;
+	int btf_fd[2] = {};
+	int prog_fd[2] = {};
+	int link_fd[2] = {};
+	__s32 btf_id[2] = {};
+
+	LIBBPF_OPTS(bpf_prog_load_opts, load_opts,
+		.expected_attach_type = BPF_TRACE_FENTRY,
+	);
+
+	const struct bpf_insn trace_program[] = {
+		BPF_MOV64_IMM(BPF_REG_0, 0),
+		BPF_EXIT_INSN(),
+	};
+
+	vmlinux_btf = btf__load_vmlinux_btf();
+	if (!ASSERT_OK_PTR(vmlinux_btf, "load_vmlinux_btf"))
+		return;
+
+	btf_fd[1] = get_bpf_testmod_btf_fd();
+	if (!ASSERT_GE(btf_fd[1], 0, "get_bpf_testmod_btf_fd"))
+		goto out;
+
+	mod_btf = btf_get_from_fd(btf_fd[1], vmlinux_btf);
+	if (!ASSERT_OK_PTR(mod_btf, "btf_get_from_fd"))
+		goto out;
+
+	btf_id[0] = btf__find_by_name_kind(vmlinux_btf, symbol_name, BTF_KIND_FUNC);
+	if (!ASSERT_GT(btf_id[0], 0, "btf_find_by_name"))
+		goto out;
+
+	btf_id[1] = btf__find_by_name_kind(mod_btf, symbol_name, BTF_KIND_FUNC);
+	if (!ASSERT_GT(btf_id[1], 0, "btf_find_by_name"))
+		goto out;
+
+	for (i = 0; i < 2; i++) {
+		load_opts.attach_btf_id = btf_id[i];
+		load_opts.attach_btf_obj_fd = btf_fd[i];
+		prog_fd[i] = bpf_prog_load(BPF_PROG_TYPE_TRACING, NULL, "GPL",
+					   trace_program,
+					   sizeof(trace_program) / sizeof(struct bpf_insn),
+					   &load_opts);
+		if (!ASSERT_GE(prog_fd[i], 0, "bpf_prog_load"))
+			goto out;
+
+		/* If the verifier incorrectly resolves addresses of the
+		 * shadowed functions and uses the same address for both the
+		 * vmlinux and the bpf_testmod functions, this will fail on
+		 * attempting to create two trampolines for the same address,
+		 * which is forbidden.
+		 */
+		link_fd[i] = bpf_link_create(prog_fd[i], 0, BPF_TRACE_FENTRY, NULL);
+		if (!ASSERT_GE(link_fd[i], 0, "bpf_link_create"))
+			goto out;
+	}
+
+	err = bpf_prog_test_run_opts(prog_fd[0], NULL);
+	ASSERT_OK(err, "running test");
+
+out:
+	btf__free(vmlinux_btf);
+	btf__free(mod_btf);
+	for (i = 0; i < 2; i++) {
+		if (btf_fd[i])
+			close(btf_fd[i]);
+		if (prog_fd[i] > 0)
+			close(prog_fd[i]);
+		if (link_fd[i] > 0)
+			close(link_fd[i]);
+	}
+}
-- 
2.39.2

Re: [PATCH bpf-next v10 1/2] bpf: Fix attaching fentry/fexit/fmod_ret/lsm to modules

[Luis Chamberlain]

Please add linux-modules in the future. My review below.

On Fri, Mar 10, 2023 at 08:40:59AM +0100, Viktor Malik wrote:
> This resolves two problems with attachment of fentry/fexit/fmod_ret/lsm
> to functions located in modules:
> 
> 1. The verifier tries to find the address to attach to in kallsyms. This
>    is always done by searching the entire kallsyms, not respecting the
>    module in which the function is located. Such approach causes an
>    incorrect attachment address to be computed if the function to attach
>    to is shadowed by a function of the same name located earlier in
>    kallsyms.

Just a heads up, I realize you have tried to fix the issue here using
semantics to get the module in other ways, but Nick's future work
expands kallsyms to allow symbols for modules to be looked for, even
if there are duplicates in kallsysms. He is working on that to help
with tooling for tracing, but here seems to be an example use case
which may be in-kernel. But you seem to indicate you've found an alternative
solution anyway.

Jiri, does live patching hunt for symbols in such opaque way too?
Or does it resolve it using something similar as the technique here
with a module notifier / its own linked list?

> 2. If the address to attach to is located in a module, the module
>    reference is only acquired in register_fentry. If the module is
>    unloaded between the place where the address is found
>    (bpf_check_attach_target in the verifier) and register_fentry, it is
>    possible that another module is loaded to the same address which may
>    lead to potential errors.
> 
> Since the attachment must contain the BTF of the program to attach to,
> we extract the module from it and search for the function address in the
> correct module (resolving problem no. 1). Then, the module reference is
> taken directly in bpf_check_attach_target and stored in the bpf program
> (in bpf_prog_aux). The reference is only released when the program is
> unloaded (resolving problem no. 2).
> 
> Signed-off-by: Viktor Malik <vmalik@redhat.com>
> Acked-by: Jiri Olsa <jolsa@kernel.org>
> ---

My review of the critical part below.

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 45a082284464..3905bb20b9a1 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -18432,8 +18434,17 @@ int bpf_check_attach_target(struct bpf_verifier_log *log,
>  			else
>  				addr = (long) tgt_prog->aux->func[subprog]->bpf_func;
>  		} else {
> -			addr = kallsyms_lookup_name(tname);
> +			if (btf_is_module(btf)) {
> +				mod = btf_try_get_module(btf);
> +				if (mod)
> +					addr = find_kallsyms_symbol_value(mod, tname);
> +				else
> +					addr = 0;
> +			} else {
> +				addr = kallsyms_lookup_name(tname);
> +			}
>  			if (!addr) {
> +				module_put(mod);
>  				bpf_log(log,
>  					"The address of function %s cannot be found\n",
>  					tname);

If btf_modules linked list is ensured to not remove the btf module
during this operation, sure this is safe, as per the new guidelines I've
posted for try_module_get() this seems to be using try_module_get()
using the implied protection.

Please review the docs. *If* it respects that usage then feel free to
add:

Reviewed-by: Luis Chamberlain <mcgrof@kernel.org>

  Luis

Re: [PATCH bpf-next v10 1/2] bpf: Fix attaching fentry/fexit/fmod_ret/lsm to modules

[Viktor Malik]

On 3/10/23 20:16, Luis Chamberlain wrote:
> Please add linux-modules in the future. My review below.

Sorry for missing that, I'll add it next time.

> 
> On Fri, Mar 10, 2023 at 08:40:59AM +0100, Viktor Malik wrote:

[snip]

> My review of the critical part below.
> 
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index 45a082284464..3905bb20b9a1 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
>> @@ -18432,8 +18434,17 @@ int bpf_check_attach_target(struct bpf_verifier_log *log,
>>   			else
>>   				addr = (long) tgt_prog->aux->func[subprog]->bpf_func;
>>   		} else {
>> -			addr = kallsyms_lookup_name(tname);
>> +			if (btf_is_module(btf)) {
>> +				mod = btf_try_get_module(btf);
>> +				if (mod)
>> +					addr = find_kallsyms_symbol_value(mod, tname);
>> +				else
>> +					addr = 0;
>> +			} else {
>> +				addr = kallsyms_lookup_name(tname);
>> +			}
>>   			if (!addr) {
>> +				module_put(mod);
>>   				bpf_log(log,
>>   					"The address of function %s cannot be found\n",
>>   					tname);
> 
> If btf_modules linked list is ensured to not remove the btf module
> during this operation, sure this is safe, as per the new guidelines I've
> posted for try_module_get() this seems to be using try_module_get()
> using the implied protection.

I believe that is the case. btf_try_get_module checks the
BTF_F_MODULE_LIVE flag before calling try_module_get and the flag is set
only when the module notifier callback is called with MODULE_STATE_LIVE.
In addition, all BTF module operations are called under the same mutex,
so the module cannot be removed in-between.

> 
> Please review the docs. *If* it respects that usage then feel free to
> add:
> 
> Reviewed-by: Luis Chamberlain <mcgrof@kernel.org>

Thanks for the review. Unless there are more change requests, I'll leave
it up to the maintainers to add the tag.

Viktor

> 
>    Luis
> 

Re: [PATCH bpf-next v10 0/2] Fix attaching fentry/fexit/fmod_ret/lsm to modules

[patchwork-bot+netdevbpf]

Hello:

This series was applied to bpf/bpf-next.git (master)
by Alexei Starovoitov <ast@kernel.org>:

On Fri, 10 Mar 2023 08:40:58 +0100 you wrote:
> I noticed that the verifier behaves incorrectly when attaching to fentry
> of multiple functions of the same name located in different modules (or
> in vmlinux). The reason for this is that if the target program is not
> specified, the verifier will search kallsyms for the trampoline address
> to attach to. The entire kallsyms is always searched, not respecting the
> module in which the function to attach to is located.
> 
> [...]

Here is the summary with links:
  - [bpf-next,v10,1/2] bpf: Fix attaching fentry/fexit/fmod_ret/lsm to modules
    https://git.kernel.org/bpf/bpf-next/c/2a6427ca8f3a
  - [bpf-next,v10,2/2] bpf/selftests: Test fentry attachment to shadowed functions
    https://git.kernel.org/bpf/bpf-next/c/873cc3835d80

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH bpf-next v10 1/2] bpf: Fix attaching fentry/fexit/fmod_ret/lsm to modules

[kernel test robot]

Hi Viktor,

Thank you for the patch! Yet something to improve:

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

url:    https://github.com/intel-lab-lkp/linux/commits/Viktor-Malik/bpf-Fix-attaching-fentry-fexit-fmod_ret-lsm-to-modules/20230310-154848
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
patch link:    https://lore.kernel.org/r/3f6a9d8ae850532b5ef864ef16327b0f7a669063.1678432753.git.vmalik%40redhat.com
patch subject: [PATCH bpf-next v10 1/2] bpf: Fix attaching fentry/fexit/fmod_ret/lsm to modules
config: arm-buildonly-randconfig-r005-20230312 (https://download.01.org/0day-ci/archive/20230316/202303160919.SGyfD0uE-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project 67409911353323ca5edf2049ef0df54132fa1ca7)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install arm cross compiling tool for clang build
        # apt-get install binutils-arm-linux-gnueabi
        # https://github.com/intel-lab-lkp/linux/commit/a6713fb8bbf7954ee98fec48f2a1f1e33814d92a
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Viktor-Malik/bpf-Fix-attaching-fentry-fexit-fmod_ret-lsm-to-modules/20230310-154848
        git checkout a6713fb8bbf7954ee98fec48f2a1f1e33814d92a
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash kernel/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303160919.SGyfD0uE-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from kernel/bpf/verifier.c:27:
>> kernel/bpf/../module/internal.h:260:14: error: expected ')'
                                                          const char *name)
                                                          ^
   kernel/bpf/../module/internal.h:259:55: note: to match this '('
   static inline unsigned long find_kallsyms_symbol_value(struct module *mod
                                                         ^
>> kernel/bpf/verifier.c:18440:45: error: too many arguments to function call, expected single argument 'mod', have 2 arguments
                                           addr = find_kallsyms_symbol_value(mod, tname);
                                                  ~~~~~~~~~~~~~~~~~~~~~~~~~~      ^~~~~
   kernel/bpf/../module/internal.h:259:29: note: 'find_kallsyms_symbol_value' declared here
   static inline unsigned long find_kallsyms_symbol_value(struct module *mod
                               ^
   2 errors generated.


vim +260 kernel/bpf/../module/internal.h

   250	
   251	static inline bool sect_empty(const Elf_Shdr *sect)
   252	{
   253		return !(sect->sh_flags & SHF_ALLOC) || sect->sh_size == 0;
   254	}
   255	#else /* !CONFIG_KALLSYMS */
   256	static inline void init_build_id(struct module *mod, const struct load_info *info) { }
   257	static inline void layout_symtab(struct module *mod, struct load_info *info) { }
   258	static inline void add_kallsyms(struct module *mod, const struct load_info *info) { }
   259	static inline unsigned long find_kallsyms_symbol_value(struct module *mod
 > 260							       const char *name)
   261	{
   262		return 0;
   263	}
   264	#endif /* CONFIG_KALLSYMS */
   265	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [PATCH bpf-next v10 1/2] bpf: Fix attaching fentry/fexit/fmod_ret/lsm to modules

[Alexei Starovoitov]

On Wed, Mar 15, 2023 at 6:32 PM kernel test robot <lkp@intel.com> wrote:
>
> Hi Viktor,
>
> Thank you for the patch! Yet something to improve:

Argh. Comma is missing.
Viktor,
please send a fix asap.

Re: [PATCH bpf-next v10 1/2] bpf: Fix attaching fentry/fexit/fmod_ret/lsm to modules

[Alexei Starovoitov]

On Wed, Mar 15, 2023 at 6:34 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Wed, Mar 15, 2023 at 6:32 PM kernel test robot <lkp@intel.com> wrote:
> >
> > Hi Viktor,
> >
> > Thank you for the patch! Yet something to improve:
>
> Argh. Comma is missing.
> Viktor,
> please send a fix asap.

Actually, since it was at the top of bpf-next
I fixed it myself and force pushed bpf-next.

Re: [PATCH bpf-next v10 1/2] bpf: Fix attaching fentry/fexit/fmod_ret/lsm to modules

[Viktor Malik]

On 3/16/23 02:39, Alexei Starovoitov wrote:
> On Wed, Mar 15, 2023 at 6:34 PM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
>>
>> On Wed, Mar 15, 2023 at 6:32 PM kernel test robot <lkp@intel.com> wrote:
>>>
>>> Hi Viktor,
>>>
>>> Thank you for the patch! Yet something to improve:
>>
>> Argh. Comma is missing.
>> Viktor,
>> please send a fix asap.
> 
> Actually, since it was at the top of bpf-next
> I fixed it myself and force pushed bpf-next.
> 

Ah, I'm sorry, I completely missed that. Thanks for fixing it, Alexei!

Viktor