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

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

[Viktor Malik]

While working on bpftrace support for BTF in modules [1], 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.

This patch fixes the above issue 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.

This also adds a new test in test_progs which tries to attach a program
to fentry of two functions of the same name, one located in vmlinux and
the other in bpf_testmod. Prior to the fix, the verifier would always
use the vmlinux function address as the target trampoline, attempting to
attach two functions to the same trampoline (which is prohibited).

[1] https://github.com/iovisor/bpftrace/pull/2315

---
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 (3):
  kallsyms: add space-efficient lookup in one module
  bpf: Fix attaching fentry/fexit/fmod_ret/lsm to modules
  bpf/selftests: Test fentry attachment to shadowed functions

 include/linux/btf.h                           |   1 +
 include/linux/module.h                        |   7 +
 kernel/bpf/btf.c                              |   5 +
 kernel/bpf/verifier.c                         |   5 +-
 kernel/module/kallsyms.c                      |  16 +++
 net/bpf/test_run.c                            |   5 +
 .../selftests/bpf/bpf_testmod/bpf_testmod.c   |   7 +
 .../bpf/prog_tests/module_attach_shadow.c     | 124 ++++++++++++++++++
 8 files changed, 169 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/module_attach_shadow.c

-- 
2.38.1

[PATCH bpf-next v3 1/3] kallsyms: add space-efficient lookup in one module

[Viktor Malik]

Until now, the only way to look up a symbol in kallsyms of a particular
module was using the "module_kallsyms_lookup_name" function with the
"module:symbol" string passed as a parameter. This syntax often requires
to build the parameter string on stack, needlessly wasting space.

This commit introduces function "kallsyms_lookup_name_in_module" which
takes the module and the symbol names as separate parameters and
therefore allows more space-efficient lookup.

Signed-off-by: Viktor Malik <vmalik@redhat.com>
---
 include/linux/module.h   |  7 +++++++
 kernel/module/kallsyms.c | 16 ++++++++++++++++
 2 files changed, 23 insertions(+)

diff --git a/include/linux/module.h b/include/linux/module.h
index 35876e89eb93..a06fbcc4013c 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -605,6 +605,7 @@ struct module *find_module(const char *name);
 int module_get_kallsym(unsigned int symnum, unsigned long *value, char *type,
 			char *name, char *module_name, int *exported);
 
+unsigned long kallsyms_lookup_name_in_module(const char *module_name, const char *name);
 /* Look for this name: can be of form module:name. */
 unsigned long module_kallsyms_lookup_name(const char *name);
 
@@ -783,6 +784,12 @@ static inline int module_get_kallsym(unsigned int symnum, unsigned long *value,
 	return -ERANGE;
 }
 
+static inline unsigned long kallsyms_lookup_name_in_module(const char *module_name,
+							   const char *name)
+{
+	return 0;
+}
+
 static inline unsigned long module_kallsyms_lookup_name(const char *name)
 {
 	return 0;
diff --git a/kernel/module/kallsyms.c b/kernel/module/kallsyms.c
index 4523f99b0358..c6c8227c7a45 100644
--- a/kernel/module/kallsyms.c
+++ b/kernel/module/kallsyms.c
@@ -482,6 +482,22 @@ static unsigned long __module_kallsyms_lookup_name(const char *name)
 	return 0;
 }
 
+unsigned long kallsyms_lookup_name_in_module(const char *module_name, const char *name)
+{
+	unsigned long ret;
+	struct module *mod;
+
+	preempt_disable();
+	mod = find_module_all(module_name, strlen(module_name), false);
+	if (mod)
+		ret = find_kallsyms_symbol_value(mod, name);
+	else
+		ret = 0;
+	preempt_enable();
+	return ret;
+
+}
+
 /* Look for this name: can be of form module:name. */
 unsigned long module_kallsyms_lookup_name(const char *name)
 {
-- 
2.38.1

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

[Viktor Malik]

When attaching fentry/fexit/fmod_ret/lsm to a function located in a
module without specifying the target program, 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.

This 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.

Since the attachment must contain the BTF of the program to attach to,
we may extract the module name from it (if the attach target is a
module) and search for the function address in the correct module.

Signed-off-by: Viktor Malik <vmalik@redhat.com>
Acked-by: Hao Luo <haoluo@google.com>
---
 include/linux/btf.h   | 1 +
 kernel/bpf/btf.c      | 5 +++++
 kernel/bpf/verifier.c | 5 ++++-
 3 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/include/linux/btf.h b/include/linux/btf.h
index cbd6e4096f8c..b7b791d1f3d6 100644
--- a/include/linux/btf.h
+++ b/include/linux/btf.h
@@ -188,6 +188,7 @@ u32 btf_obj_id(const struct btf *btf);
 bool btf_is_kernel(const struct btf *btf);
 bool btf_is_module(const struct btf *btf);
 struct module *btf_try_get_module(const struct btf *btf);
+const char *btf_module_name(const struct btf *btf);
 u32 btf_nr_types(const struct btf *btf);
 bool btf_member_is_reg_int(const struct btf *btf, const struct btf_type *s,
 			   const struct btf_member *m,
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index c80bd8709e69..f78e8060efa6 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -7208,6 +7208,11 @@ bool btf_is_module(const struct btf *btf)
 	return btf->kernel_btf && strcmp(btf->name, "vmlinux") != 0;
 }
 
+const char *btf_module_name(const struct btf *btf)
+{
+	return btf->name;
+}
+
 enum {
 	BTF_MODULE_F_LIVE = (1 << 0),
 };
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 1d51bd9596da..0c533db51f92 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -16483,7 +16483,10 @@ 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))
+				addr = kallsyms_lookup_name_in_module(btf_module_name(btf), tname);
+			else
+				addr = kallsyms_lookup_name(tname);
 			if (!addr) {
 				bpf_log(log,
 					"The address of function %s cannot be found\n",
-- 
2.38.1

[PATCH bpf-next v3 3/3] 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 attach two programs to the same
trampoline.

Signed-off-by: Viktor Malik <vmalik@redhat.com>
---
 net/bpf/test_run.c                            |   5 +
 .../selftests/bpf/bpf_testmod/bpf_testmod.c   |   7 +
 .../bpf/prog_tests/module_attach_shadow.c     | 124 ++++++++++++++++++
 3 files changed, 136 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/module_attach_shadow.c

diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
index 6094ef7cffcd..71e36a85573b 100644
--- a/net/bpf/test_run.c
+++ b/net/bpf/test_run.c
@@ -536,6 +536,11 @@ int noinline bpf_modify_return_test(int a, int *b)
 	return a + *b;
 }
 
+int noinline bpf_fentry_shadow_test(int a)
+{
+	return a + 1;
+}
+
 u64 noinline bpf_kfunc_call_test1(struct sock *sk, u32 a, u64 b, u32 c, u64 d)
 {
 	return a + b + c + d;
diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
index 5085fea3cac5..d23127a5ec68 100644
--- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
+++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
@@ -229,6 +229,13 @@ 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);
+ALLOW_ERROR_INJECTION(bpf_fentry_shadow_test, ERRNO);
+
 extern int bpf_fentry_test1(int a);
 
 static int bpf_testmod_init(void)
diff --git a/tools/testing/selftests/bpf/prog_tests/module_attach_shadow.c b/tools/testing/selftests/bpf/prog_tests/module_attach_shadow.c
new file mode 100644
index 000000000000..bf511e61ec1f
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/module_attach_shadow.c
@@ -0,0 +1,124 @@
+// 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";
+
+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) {
+			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] = {};
+
+	const struct bpf_insn trace_program[] = {
+		BPF_MOV64_IMM(BPF_REG_0, 0),
+		BPF_EXIT_INSN(),
+	};
+
+	LIBBPF_OPTS(bpf_prog_load_opts, load_opts,
+		.expected_attach_type = BPF_TRACE_FENTRY,
+	);
+
+	LIBBPF_OPTS(bpf_test_run_opts, test_opts);
+
+	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_GT(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;
+
+		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], &test_opts);
+	ASSERT_OK(err, "running test");
+
+out:
+	if (vmlinux_btf)
+		btf__free(vmlinux_btf);
+	if (mod_btf)
+		btf__free(mod_btf);
+	for (i = 0; i < 2; i++) {
+		if (btf_fd[i])
+			close(btf_fd[i]);
+		if (prog_fd[i])
+			close(prog_fd[i]);
+		if (link_fd[i])
+			close(link_fd[i]);
+	}
+}
-- 
2.38.1

Re: [PATCH bpf-next v3 3/3] bpf/selftests: Test fentry attachment to shadowed functions

[Jiri Olsa]

On Mon, Dec 05, 2022 at 04:26:06PM +0100, Viktor Malik wrote:
> 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 attach two programs to the same
> trampoline.

hi
looks good, few nits below

> 
> Signed-off-by: Viktor Malik <vmalik@redhat.com>
> ---
>  net/bpf/test_run.c                            |   5 +
>  .../selftests/bpf/bpf_testmod/bpf_testmod.c   |   7 +
>  .../bpf/prog_tests/module_attach_shadow.c     | 124 ++++++++++++++++++
>  3 files changed, 136 insertions(+)
>  create mode 100644 tools/testing/selftests/bpf/prog_tests/module_attach_shadow.c
> 
> diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
> index 6094ef7cffcd..71e36a85573b 100644
> --- a/net/bpf/test_run.c
> +++ b/net/bpf/test_run.c
> @@ -536,6 +536,11 @@ int noinline bpf_modify_return_test(int a, int *b)
>  	return a + *b;
>  }
>  
> +int noinline bpf_fentry_shadow_test(int a)
> +{
> +	return a + 1;
> +}
> +
>  u64 noinline bpf_kfunc_call_test1(struct sock *sk, u32 a, u64 b, u32 c, u64 d)
>  {
>  	return a + b + c + d;
> diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> index 5085fea3cac5..d23127a5ec68 100644
> --- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> +++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> @@ -229,6 +229,13 @@ 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);
> +ALLOW_ERROR_INJECTION(bpf_fentry_shadow_test, ERRNO);

why marked as ALLOW_ERROR_INJECTION?

> +
>  extern int bpf_fentry_test1(int a);
>  
>  static int bpf_testmod_init(void)
> diff --git a/tools/testing/selftests/bpf/prog_tests/module_attach_shadow.c b/tools/testing/selftests/bpf/prog_tests/module_attach_shadow.c
> new file mode 100644
> index 000000000000..bf511e61ec1f
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/module_attach_shadow.c
> @@ -0,0 +1,124 @@
> +// 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";
> +
> +int get_bpf_testmod_btf_fd(void)

should be static?

> +{
> +	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) {

I was checking how's libbpf doing this and found load_module_btfs,
which seems similar.. and it has one additional check in here:

                        if (errno == ENOENT)
                                continue; /* expected race: BTF was unloaded */

I guess it's not likely, but it's better to have it


SNIP

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

so IIUC the issue is that without the previous fix this will create
2 separate trampolines pointing to single address.. and we can have
just one trampoline for address.. so the 2nd trampoline update will
fail, because the trampoline location is already changed/taken ?

could you please put some description like that in the comment or
changelog?

thanks,
jirka

> +	}
> +
> +	err = bpf_prog_test_run_opts(prog_fd[0], &test_opts);
> +	ASSERT_OK(err, "running test");
> +
> +out:
> +	if (vmlinux_btf)
> +		btf__free(vmlinux_btf);
> +	if (mod_btf)
> +		btf__free(mod_btf);
> +	for (i = 0; i < 2; i++) {
> +		if (btf_fd[i])
> +			close(btf_fd[i]);
> +		if (prog_fd[i])
> +			close(prog_fd[i]);
> +		if (link_fd[i])
> +			close(link_fd[i]);
> +	}
> +}
> -- 
> 2.38.1
> 

Re: [PATCH bpf-next v3 3/3] bpf/selftests: Test fentry attachment to shadowed functions

[Viktor Malik]

On 12/5/22 21:49, Jiri Olsa wrote:
> On Mon, Dec 05, 2022 at 04:26:06PM +0100, Viktor Malik wrote:
>> 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 attach two programs to the same
>> trampoline.
> 
> hi
> looks good, few nits below
> 
>>
>> Signed-off-by: Viktor Malik <vmalik@redhat.com>
>> ---
>>   net/bpf/test_run.c                            |   5 +
>>   .../selftests/bpf/bpf_testmod/bpf_testmod.c   |   7 +
>>   .../bpf/prog_tests/module_attach_shadow.c     | 124 ++++++++++++++++++
>>   3 files changed, 136 insertions(+)
>>   create mode 100644 tools/testing/selftests/bpf/prog_tests/module_attach_shadow.c
>>
>> diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
>> index 6094ef7cffcd..71e36a85573b 100644
>> --- a/net/bpf/test_run.c
>> +++ b/net/bpf/test_run.c
>> @@ -536,6 +536,11 @@ int noinline bpf_modify_return_test(int a, int *b)
>>   	return a + *b;
>>   }
>>   
>> +int noinline bpf_fentry_shadow_test(int a)
>> +{
>> +	return a + 1;
>> +}
>> +
>>   u64 noinline bpf_kfunc_call_test1(struct sock *sk, u32 a, u64 b, u32 c, u64 d)
>>   {
>>   	return a + b + c + d;
>> diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
>> index 5085fea3cac5..d23127a5ec68 100644
>> --- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
>> +++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
>> @@ -229,6 +229,13 @@ 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);
>> +ALLOW_ERROR_INJECTION(bpf_fentry_shadow_test, ERRNO);
> 
> why marked as ALLOW_ERROR_INJECTION?

Right, not necessary, will remove.

> 
>> +
>>   extern int bpf_fentry_test1(int a);
>>   
>>   static int bpf_testmod_init(void)
>> diff --git a/tools/testing/selftests/bpf/prog_tests/module_attach_shadow.c b/tools/testing/selftests/bpf/prog_tests/module_attach_shadow.c
>> new file mode 100644
>> index 000000000000..bf511e61ec1f
>> --- /dev/null
>> +++ b/tools/testing/selftests/bpf/prog_tests/module_attach_shadow.c
>> @@ -0,0 +1,124 @@
>> +// 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";
>> +
>> +int get_bpf_testmod_btf_fd(void)
> 
> should be static?

Yes, I believe.

> 
>> +{
>> +	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) {
> 
> I was checking how's libbpf doing this and found load_module_btfs,
> which seems similar.. and it has one additional check in here:
> 
>                          if (errno == ENOENT)
>                                  continue; /* expected race: BTF was unloaded */
> 
> I guess it's not likely, but it's better to have it

Sure, will add it. You're right, the implementation is mostly taken from
libbpf's load_module_btfs.

> 
> 
> SNIP
> 
>> +	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;
>> +
>> +		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;
> 
> so IIUC the issue is that without the previous fix this will create
> 2 separate trampolines pointing to single address.. and we can have
> just one trampoline for address.. so the 2nd trampoline update will
> fail, because the trampoline location is already changed/taken ?
> 
> could you please put some description like that in the comment or
> changelog?

The description is already in the commit message and in the series
changelog, although it may be a bit inaccurate (stating that two
programs are attached to the same trampoline rather that two trampolines
attached to the same address).

I'll fix it and add it to the comment, it could be useful to have it
there as well.

Thanks!
Viktor

> 
> thanks,
> jirka
> 
>> +	}
>> +
>> +	err = bpf_prog_test_run_opts(prog_fd[0], &test_opts);
>> +	ASSERT_OK(err, "running test");
>> +
>> +out:
>> +	if (vmlinux_btf)
>> +		btf__free(vmlinux_btf);
>> +	if (mod_btf)
>> +		btf__free(mod_btf);
>> +	for (i = 0; i < 2; i++) {
>> +		if (btf_fd[i])
>> +			close(btf_fd[i]);
>> +		if (prog_fd[i])
>> +			close(prog_fd[i]);
>> +		if (link_fd[i])
>> +			close(link_fd[i]);
>> +	}
>> +}
>> -- 
>> 2.38.1
>>
> 

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

[Alexei Starovoitov]

On Mon, Dec 05, 2022 at 04:26:05PM +0100, Viktor Malik wrote:
> When attaching fentry/fexit/fmod_ret/lsm to a function located in a
> module without specifying the target program, 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.
> 
> This 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.
> 
> Since the attachment must contain the BTF of the program to attach to,
> we may extract the module name from it (if the attach target is a
> module) and search for the function address in the correct module.
> 
> Signed-off-by: Viktor Malik <vmalik@redhat.com>
> Acked-by: Hao Luo <haoluo@google.com>
> ---
>  include/linux/btf.h   | 1 +
>  kernel/bpf/btf.c      | 5 +++++
>  kernel/bpf/verifier.c | 5 ++++-
>  3 files changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/btf.h b/include/linux/btf.h
> index cbd6e4096f8c..b7b791d1f3d6 100644
> --- a/include/linux/btf.h
> +++ b/include/linux/btf.h
> @@ -188,6 +188,7 @@ u32 btf_obj_id(const struct btf *btf);
>  bool btf_is_kernel(const struct btf *btf);
>  bool btf_is_module(const struct btf *btf);
>  struct module *btf_try_get_module(const struct btf *btf);
> +const char *btf_module_name(const struct btf *btf);
>  u32 btf_nr_types(const struct btf *btf);
>  bool btf_member_is_reg_int(const struct btf *btf, const struct btf_type *s,
>  			   const struct btf_member *m,
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index c80bd8709e69..f78e8060efa6 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -7208,6 +7208,11 @@ bool btf_is_module(const struct btf *btf)
>  	return btf->kernel_btf && strcmp(btf->name, "vmlinux") != 0;
>  }
>  
> +const char *btf_module_name(const struct btf *btf)
> +{
> +	return btf->name;
> +}

It feels that btf->name is leaking a bit of implementation detail.
How about doing:

struct module *btf_find_module(const struct btf *btf)
{
        reutrn find_module(btf->name);
}

> +
>  enum {
>  	BTF_MODULE_F_LIVE = (1 << 0),
>  };
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 1d51bd9596da..0c533db51f92 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -16483,7 +16483,10 @@ 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))
> +				addr = kallsyms_lookup_name_in_module(btf_module_name(btf), tname);

and use find_kallsyms_symbol_value() here
(with preempt_disable dance).
There won't be a need for patch 1 too.

wdyt?

> +			else
> +				addr = kallsyms_lookup_name(tname);
>  			if (!addr) {
>  				bpf_log(log,
>  					"The address of function %s cannot be found\n",
> -- 
> 2.38.1
> 

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

[Viktor Malik]

On 12/7/22 01:10, Alexei Starovoitov wrote:
> On Mon, Dec 05, 2022 at 04:26:05PM +0100, Viktor Malik wrote:
>> When attaching fentry/fexit/fmod_ret/lsm to a function located in a
>> module without specifying the target program, 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.
>>
>> This 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.
>>
>> Since the attachment must contain the BTF of the program to attach to,
>> we may extract the module name from it (if the attach target is a
>> module) and search for the function address in the correct module.
>>
>> Signed-off-by: Viktor Malik <vmalik@redhat.com>
>> Acked-by: Hao Luo <haoluo@google.com>
>> ---
>>   include/linux/btf.h   | 1 +
>>   kernel/bpf/btf.c      | 5 +++++
>>   kernel/bpf/verifier.c | 5 ++++-
>>   3 files changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/linux/btf.h b/include/linux/btf.h
>> index cbd6e4096f8c..b7b791d1f3d6 100644
>> --- a/include/linux/btf.h
>> +++ b/include/linux/btf.h
>> @@ -188,6 +188,7 @@ u32 btf_obj_id(const struct btf *btf);
>>   bool btf_is_kernel(const struct btf *btf);
>>   bool btf_is_module(const struct btf *btf);
>>   struct module *btf_try_get_module(const struct btf *btf);
>> +const char *btf_module_name(const struct btf *btf);
>>   u32 btf_nr_types(const struct btf *btf);
>>   bool btf_member_is_reg_int(const struct btf *btf, const struct btf_type *s,
>>   			   const struct btf_member *m,
>> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
>> index c80bd8709e69..f78e8060efa6 100644
>> --- a/kernel/bpf/btf.c
>> +++ b/kernel/bpf/btf.c
>> @@ -7208,6 +7208,11 @@ bool btf_is_module(const struct btf *btf)
>>   	return btf->kernel_btf && strcmp(btf->name, "vmlinux") != 0;
>>   }
>>   
>> +const char *btf_module_name(const struct btf *btf)
>> +{
>> +	return btf->name;
>> +}
> 
> It feels that btf->name is leaking a bit of implementation detail.
> How about doing:
> 
> struct module *btf_find_module(const struct btf *btf)
> {
>          reutrn find_module(btf->name);
> }
> 
>> +
>>   enum {
>>   	BTF_MODULE_F_LIVE = (1 << 0),
>>   };
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index 1d51bd9596da..0c533db51f92 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
>> @@ -16483,7 +16483,10 @@ 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))
>> +				addr = kallsyms_lookup_name_in_module(btf_module_name(btf), tname);
> 
> and use find_kallsyms_symbol_value() here
> (with preempt_disable dance).
> There won't be a need for patch 1 too.
> 
> wdyt?

This makes sense to me maybe more than the current solution.
I'm not sure where it's best to do preempt_disable, though. Would you
rather do it here or inside find_kallsyms_symbol_value? Or should we
create a new wrapper for find_kallsyms_symbol_value, possibly outside of
kernel/module/internal.h?

Thanks!

> 
>> +			else
>> +				addr = kallsyms_lookup_name(tname);
>>   			if (!addr) {
>>   				bpf_log(log,
>>   					"The address of function %s cannot be found\n",
>> -- 
>> 2.38.1
>>
>