* [PATCH bpf-next v4 0/2] Fix attaching fentry/fexit/fmod_ret/lsm to modules
@ 2022-12-12 12:59 Viktor Malik
2022-12-12 12:59 ` [PATCH bpf-next v4 1/2] bpf: " Viktor Malik
2022-12-12 12:59 ` [PATCH bpf-next v4 2/2] bpf/selftests: Test fentry attachment to shadowed functions Viktor Malik
0 siblings, 2 replies; 7+ messages in thread
From: Viktor Malik @ 2022-12-12 12:59 UTC (permalink / raw)
To: bpf
Cc: Alexei Starovoitov, Daniel Borkmann, John Fastabend,
Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song,
KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, 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 for the target trampoline, attempting
to create two trampolines for the same address (which is prohibited).
[1] https://github.com/iovisor/bpftrace/pull/2315
---
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
kernel/bpf/verifier.c | 16 ++-
net/bpf/test_run.c | 5 +
.../selftests/bpf/bpf_testmod/bpf_testmod.c | 6 +
.../bpf/prog_tests/module_attach_shadow.c | 131 ++++++++++++++++++
4 files changed, 157 insertions(+), 1 deletion(-)
create mode 100644 tools/testing/selftests/bpf/prog_tests/module_attach_shadow.c
--
2.38.1
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH bpf-next v4 1/2] bpf: Fix attaching fentry/fexit/fmod_ret/lsm to modules
2022-12-12 12:59 [PATCH bpf-next v4 0/2] Fix attaching fentry/fexit/fmod_ret/lsm to modules Viktor Malik
@ 2022-12-12 12:59 ` Viktor Malik
2022-12-12 17:08 ` Yonghong Song
2022-12-13 10:27 ` Viktor Malik
2022-12-12 12:59 ` [PATCH bpf-next v4 2/2] bpf/selftests: Test fentry attachment to shadowed functions Viktor Malik
1 sibling, 2 replies; 7+ messages in thread
From: Viktor Malik @ 2022-12-12 12:59 UTC (permalink / raw)
To: bpf
Cc: Alexei Starovoitov, Daniel Borkmann, John Fastabend,
Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song,
KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, 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 from it and search for the function address in
the module.
Signed-off-by: Viktor Malik <vmalik@redhat.com>
---
kernel/bpf/verifier.c | 16 +++++++++++++++-
1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index a5255a0dcbb6..d646c5263bc5 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"
@@ -16478,6 +16479,7 @@ int bpf_check_attach_target(struct bpf_verifier_log *log,
const char *tname;
struct btf *btf;
long addr = 0;
+ struct module *mod;
if (!btf_id) {
bpf_log(log, "Tracing programs must provide btf_id\n");
@@ -16645,7 +16647,19 @@ 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)) {
+ preempt_disable();
+ mod = btf_try_get_module(btf);
+ if (mod) {
+ addr = find_kallsyms_symbol_value(mod, tname);
+ module_put(mod);
+ } else {
+ addr = 0;
+ }
+ preempt_enable();
+ } else {
+ addr = kallsyms_lookup_name(tname);
+ }
if (!addr) {
bpf_log(log,
"The address of function %s cannot be found\n",
--
2.38.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH bpf-next v4 2/2] bpf/selftests: Test fentry attachment to shadowed functions
2022-12-12 12:59 [PATCH bpf-next v4 0/2] Fix attaching fentry/fexit/fmod_ret/lsm to modules Viktor Malik
2022-12-12 12:59 ` [PATCH bpf-next v4 1/2] bpf: " Viktor Malik
@ 2022-12-12 12:59 ` Viktor Malik
1 sibling, 0 replies; 7+ messages in thread
From: Viktor Malik @ 2022-12-12 12:59 UTC (permalink / raw)
To: bpf
Cc: Alexei Starovoitov, Daniel Borkmann, John Fastabend,
Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song,
KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, 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.
Signed-off-by: Viktor Malik <vmalik@redhat.com>
---
net/bpf/test_run.c | 5 +
.../selftests/bpf/bpf_testmod/bpf_testmod.c | 6 +
.../bpf/prog_tests/module_attach_shadow.c | 131 ++++++++++++++++++
3 files changed, 142 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 444736e707a4..5df17dece211 100644
--- a/net/bpf/test_run.c
+++ b/net/bpf/test_run.c
@@ -535,6 +535,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..13760774754e 100644
--- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
+++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
@@ -229,6 +229,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_attach_shadow.c b/tools/testing/selftests/bpf/prog_tests/module_attach_shadow.c
new file mode 100644
index 000000000000..a75d2cdde928
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/module_attach_shadow.c
@@ -0,0 +1,131 @@
+// 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] = {};
+
+ 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;
+
+ // 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], &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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH bpf-next v4 1/2] bpf: Fix attaching fentry/fexit/fmod_ret/lsm to modules
2022-12-12 12:59 ` [PATCH bpf-next v4 1/2] bpf: " Viktor Malik
@ 2022-12-12 17:08 ` Yonghong Song
2022-12-13 10:59 ` Viktor Malik
2022-12-13 10:27 ` Viktor Malik
1 sibling, 1 reply; 7+ messages in thread
From: Yonghong Song @ 2022-12-12 17:08 UTC (permalink / raw)
To: Viktor Malik, bpf
Cc: Alexei Starovoitov, Daniel Borkmann, John Fastabend,
Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song,
KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa
On 12/12/22 4:59 AM, 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 from it and search for the function address in
> the module.
>
> Signed-off-by: Viktor Malik <vmalik@redhat.com>
> ---
> kernel/bpf/verifier.c | 16 +++++++++++++++-
> 1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index a5255a0dcbb6..d646c5263bc5 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"
>
> @@ -16478,6 +16479,7 @@ int bpf_check_attach_target(struct bpf_verifier_log *log,
> const char *tname;
> struct btf *btf;
> long addr = 0;
> + struct module *mod;
>
> if (!btf_id) {
> bpf_log(log, "Tracing programs must provide btf_id\n");
> @@ -16645,7 +16647,19 @@ 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)) {
> + preempt_disable();
> + mod = btf_try_get_module(btf);
> + if (mod) {
> + addr = find_kallsyms_symbol_value(mod, tname);
> + module_put(mod);
> + } else {
> + addr = 0;
> + }
> + preempt_enable();
What if module is unloaded right after preempt_enabled so 'addr' becomes
invalid? Is this a corner case we should consider?
> + } else {
> + addr = kallsyms_lookup_name(tname);
> + }
> if (!addr) {
> bpf_log(log,
> "The address of function %s cannot be found\n",
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH bpf-next v4 1/2] bpf: Fix attaching fentry/fexit/fmod_ret/lsm to modules
2022-12-12 12:59 ` [PATCH bpf-next v4 1/2] bpf: " Viktor Malik
2022-12-12 17:08 ` Yonghong Song
@ 2022-12-13 10:27 ` Viktor Malik
1 sibling, 0 replies; 7+ messages in thread
From: Viktor Malik @ 2022-12-13 10:27 UTC (permalink / raw)
To: bpf
Cc: Alexei Starovoitov, Daniel Borkmann, John Fastabend,
Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song,
KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa
On 12/12/22 13:59, 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 from it and search for the function address in
> the module.
>
> Signed-off-by: Viktor Malik <vmalik@redhat.com>
> ---
> kernel/bpf/verifier.c | 16 +++++++++++++++-
> 1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index a5255a0dcbb6..d646c5263bc5 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"
Looks like there's a number of errors reported by test robot due to
including "../module/internal.h" (mostly unrelated to this patchset).
I'm not sure how to approach those - move find_kallsyms_symbol_value to
include/linux/module.h or just ignore the errors?
For both cases, a trivial find_kallsyms_symbol_value for
!CONFIG_KALLSYMS will be necessary.
>
> #include "disasm.h"
>
> @@ -16478,6 +16479,7 @@ int bpf_check_attach_target(struct bpf_verifier_log *log,
> const char *tname;
> struct btf *btf;
> long addr = 0;
> + struct module *mod;
>
> if (!btf_id) {
> bpf_log(log, "Tracing programs must provide btf_id\n");
> @@ -16645,7 +16647,19 @@ 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)) {
> + preempt_disable();
> + mod = btf_try_get_module(btf);
> + if (mod) {
> + addr = find_kallsyms_symbol_value(mod, tname);
> + module_put(mod);
> + } else {
> + addr = 0;
> + }
> + preempt_enable();
> + } else {
> + addr = kallsyms_lookup_name(tname);
> + }
> if (!addr) {
> bpf_log(log,
> "The address of function %s cannot be found\n",
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH bpf-next v4 1/2] bpf: Fix attaching fentry/fexit/fmod_ret/lsm to modules
2022-12-12 17:08 ` Yonghong Song
@ 2022-12-13 10:59 ` Viktor Malik
2022-12-13 21:06 ` Yonghong Song
0 siblings, 1 reply; 7+ messages in thread
From: Viktor Malik @ 2022-12-13 10:59 UTC (permalink / raw)
To: Yonghong Song, bpf
Cc: Alexei Starovoitov, Daniel Borkmann, John Fastabend,
Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song,
KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa
On 12/12/22 18:08, Yonghong Song wrote:
>
>
> On 12/12/22 4:59 AM, 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 from it and search for the function address in
>> the module.
>>
>> Signed-off-by: Viktor Malik <vmalik@redhat.com>
>> ---
>> kernel/bpf/verifier.c | 16 +++++++++++++++-
>> 1 file changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index a5255a0dcbb6..d646c5263bc5 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"
>> @@ -16478,6 +16479,7 @@ int bpf_check_attach_target(struct
>> bpf_verifier_log *log,
>> const char *tname;
>> struct btf *btf;
>> long addr = 0;
>> + struct module *mod;
>> if (!btf_id) {
>> bpf_log(log, "Tracing programs must provide btf_id\n");
>> @@ -16645,7 +16647,19 @@ 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)) {
>> + preempt_disable();
>> + mod = btf_try_get_module(btf);
>> + if (mod) {
>> + addr = find_kallsyms_symbol_value(mod, tname);
>> + module_put(mod);
>> + } else {
>> + addr = 0;
>> + }
>> + preempt_enable();
>
> What if module is unloaded right after preempt_enabled so 'addr' becomes
> invalid? Is this a corner case we should consider?
IIUC, if 'addr' becomes invalid, the attachment will eventually fail.
So I'd say that there's no need to consider that case here, it's not
considered for kallsyms_lookup_name below (which may call
module_kallsyms_lookup_name) either.
>
>> + } else {
>> + addr = kallsyms_lookup_name(tname);
>> + }
>> if (!addr) {
>> bpf_log(log,
>> "The address of function %s cannot be found\n",
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH bpf-next v4 1/2] bpf: Fix attaching fentry/fexit/fmod_ret/lsm to modules
2022-12-13 10:59 ` Viktor Malik
@ 2022-12-13 21:06 ` Yonghong Song
0 siblings, 0 replies; 7+ messages in thread
From: Yonghong Song @ 2022-12-13 21:06 UTC (permalink / raw)
To: Viktor Malik, bpf
Cc: Alexei Starovoitov, Daniel Borkmann, John Fastabend,
Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song,
KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa
On 12/13/22 2:59 AM, Viktor Malik wrote:
> On 12/12/22 18:08, Yonghong Song wrote:
>>
>>
>> On 12/12/22 4:59 AM, 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 from it and search for the function address in
>>> the module.
>>>
>>> Signed-off-by: Viktor Malik <vmalik@redhat.com>
>>> ---
>>> kernel/bpf/verifier.c | 16 +++++++++++++++-
>>> 1 file changed, 15 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>>> index a5255a0dcbb6..d646c5263bc5 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"
>>> @@ -16478,6 +16479,7 @@ int bpf_check_attach_target(struct
>>> bpf_verifier_log *log,
>>> const char *tname;
>>> struct btf *btf;
>>> long addr = 0;
>>> + struct module *mod;
>>> if (!btf_id) {
>>> bpf_log(log, "Tracing programs must provide btf_id\n");
>>> @@ -16645,7 +16647,19 @@ 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)) {
>>> + preempt_disable();
>>> + mod = btf_try_get_module(btf);
>>> + if (mod) {
>>> + addr = find_kallsyms_symbol_value(mod, tname);
>>> + module_put(mod);
>>> + } else {
>>> + addr = 0;
>>> + }
>>> + preempt_enable();
>>
>> What if module is unloaded right after preempt_enabled so 'addr'
>> becomes invalid? Is this a corner case we should consider?
>
> IIUC, if 'addr' becomes invalid, the attachment will eventually fail.
>
> So I'd say that there's no need to consider that case here, it's not
> considered for kallsyms_lookup_name below (which may call
> module_kallsyms_lookup_name) either.
The below kallsyms_lookup_name(tname) works for vmlinux and vmlinux
function won't go away.
The following is what I understand for module address:
bpf_tracing_prog_attach:
...
bpf_check_attach_target (get addr etc. into tgt_info.
...
bpf_trampoline_link_prog
__bpf_trampoline_link_prog
bpf_trampoline_update
register_fentry
bpf_trampoline_module_get
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;
}
We try to grab a module reference here based on the func addr.
But there is a risk that module (m1) might be unloaded and a different
module (m2) might be loaded which occupies the address space of
m1. In such cases, we might have issues.
To resolve this issue, we might want to grab the reference
earlier for find_kallsyms_symbol_value and won't release it
until the program is detached. try_module_get() then will
be unnecessary in this particular case. But care must be
taken for other code paths.
>
>>
>>> + } else {
>>> + addr = kallsyms_lookup_name(tname);
>>> + }
>>> if (!addr) {
>>> bpf_log(log,
>>> "The address of function %s cannot be found\n",
>>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-12-13 21:07 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-12-12 12:59 [PATCH bpf-next v4 0/2] Fix attaching fentry/fexit/fmod_ret/lsm to modules Viktor Malik
2022-12-12 12:59 ` [PATCH bpf-next v4 1/2] bpf: " Viktor Malik
2022-12-12 17:08 ` Yonghong Song
2022-12-13 10:59 ` Viktor Malik
2022-12-13 21:06 ` Yonghong Song
2022-12-13 10:27 ` Viktor Malik
2022-12-12 12:59 ` [PATCH bpf-next v4 2/2] bpf/selftests: Test fentry attachment to shadowed functions Viktor Malik
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox