* [PATCH bpf 0/2] fix a verifier bug in check_attach_btf_id()
@ 2019-12-05 1:06 Yonghong Song
2019-12-05 1:06 ` [PATCH bpf 1/2] bpf: fix a bug when getting subprog 0 jited image in check_attach_btf_id Yonghong Song
2019-12-05 1:06 ` [PATCH bpf 2/2] selftests/bpf: add a fexit/bpf2bpf test with target bpf prog no callees Yonghong Song
0 siblings, 2 replies; 5+ messages in thread
From: Yonghong Song @ 2019-12-05 1:06 UTC (permalink / raw)
To: bpf; +Cc: Alexei Starovoitov, Daniel Borkmann, kernel-team
Commit 5b92a28aae4d ("bpf: Support attaching tracing BPF program to
other BPF programs") added support to attach tracing bpf program to
other bpf programs. It had a bug when trying to get the address
of the jited image if the main program does not have any callees,
resulting in the following kernel segfault:
......
[79162.619208] BUG: kernel NULL pointer dereference, address:
0000000000000000
......
[79162.634255] Call Trace:
[79162.634974] ? _cond_resched+0x15/0x30
[79162.635686] ? kmem_cache_alloc_trace+0x162/0x220
[79162.636398] ? selinux_bpf_prog_alloc+0x1f/0x60
[79162.637111] bpf_prog_load+0x3de/0x690
[79162.637809] __do_sys_bpf+0x105/0x1740
[79162.638488] do_syscall_64+0x5b/0x180
[79162.639147] entry_SYSCALL_64_after_hwframe+0x44/0xa9
Patch #1 fixed the problem with more explanation in the commit message.
Patch #2 added a selftest which will fail without this patch.
Yonghong Song (2):
bpf: fix a bug to get subprog 0 jited image in check_attach_btf_id
selftests/bpf: add a fexit/bpf2bpf test with target bpf prog no
callees
kernel/bpf/verifier.c | 5 +-
.../selftests/bpf/prog_tests/fexit_bpf2bpf.c | 70 ++++++++++++++-----
.../bpf/progs/fexit_bpf2bpf_simple.c | 26 +++++++
.../selftests/bpf/progs/test_pkt_md_access.c | 4 +-
4 files changed, 85 insertions(+), 20 deletions(-)
create mode 100644 tools/testing/selftests/bpf/progs/fexit_bpf2bpf_simple.c
--
2.17.1
^ permalink raw reply [flat|nested] 5+ messages in thread* [PATCH bpf 1/2] bpf: fix a bug when getting subprog 0 jited image in check_attach_btf_id 2019-12-05 1:06 [PATCH bpf 0/2] fix a verifier bug in check_attach_btf_id() Yonghong Song @ 2019-12-05 1:06 ` Yonghong Song 2019-12-05 5:33 ` Alexei Starovoitov 2019-12-05 1:06 ` [PATCH bpf 2/2] selftests/bpf: add a fexit/bpf2bpf test with target bpf prog no callees Yonghong Song 1 sibling, 1 reply; 5+ messages in thread From: Yonghong Song @ 2019-12-05 1:06 UTC (permalink / raw) To: bpf; +Cc: Alexei Starovoitov, Daniel Borkmann, kernel-team For jited bpf program, if the subprogram count is 1, i.e., there is no callees in the program, prog->aux->func will be NULL and prog->bpf_func points to image address of the program. If there is more than one subprogram, prog->aux->func is populated, and subprogram 0 can be accessed through either prog->bpf_func or prog->aux->func[0]. Other subprograms should be accessed through prog->aux->func[subprog_id]. This patch fixed a bug in check_attach_btf_id(), where prog->aux->func[subprog_id] is used to access any subprogram which caused a segfault like below: [79162.619208] BUG: kernel NULL pointer dereference, address: 0000000000000000 ...... [79162.634255] Call Trace: [79162.634974] ? _cond_resched+0x15/0x30 [79162.635686] ? kmem_cache_alloc_trace+0x162/0x220 [79162.636398] ? selinux_bpf_prog_alloc+0x1f/0x60 [79162.637111] bpf_prog_load+0x3de/0x690 [79162.637809] __do_sys_bpf+0x105/0x1740 [79162.638488] do_syscall_64+0x5b/0x180 [79162.639147] entry_SYSCALL_64_after_hwframe+0x44/0xa9 ...... Fixes: 5b92a28aae4d ("bpf: Support attaching tracing BPF program to other BPF programs") Reported-by: Eelco Chaudron <echaudro@redhat.com> Signed-off-by: Yonghong Song <yhs@fb.com> --- kernel/bpf/verifier.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index a0482e1c4a77..034ef81f935b 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -9636,7 +9636,10 @@ static int check_attach_btf_id(struct bpf_verifier_env *env) ret = -EINVAL; goto out; } - addr = (long) tgt_prog->aux->func[subprog]->bpf_func; + if (subprog == 0) + addr = (long) tgt_prog->bpf_func; + else + addr = (long) tgt_prog->aux->func[subprog]->bpf_func; } else { addr = kallsyms_lookup_name(tname); if (!addr) { -- 2.17.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH bpf 1/2] bpf: fix a bug when getting subprog 0 jited image in check_attach_btf_id 2019-12-05 1:06 ` [PATCH bpf 1/2] bpf: fix a bug when getting subprog 0 jited image in check_attach_btf_id Yonghong Song @ 2019-12-05 5:33 ` Alexei Starovoitov 0 siblings, 0 replies; 5+ messages in thread From: Alexei Starovoitov @ 2019-12-05 5:33 UTC (permalink / raw) To: Yonghong Song; +Cc: bpf, Alexei Starovoitov, Daniel Borkmann, kernel-team On Wed, Dec 04, 2019 at 05:06:06PM -0800, Yonghong Song wrote: > For jited bpf program, if the subprogram count is 1, i.e., > there is no callees in the program, prog->aux->func will be NULL > and prog->bpf_func points to image address of the program. > > If there is more than one subprogram, prog->aux->func is populated, > and subprogram 0 can be accessed through either prog->bpf_func or > prog->aux->func[0]. Other subprograms should be accessed through > prog->aux->func[subprog_id]. > > This patch fixed a bug in check_attach_btf_id(), where > prog->aux->func[subprog_id] is used to access any subprogram which > caused a segfault like below: > [79162.619208] BUG: kernel NULL pointer dereference, address: > 0000000000000000 > ...... > [79162.634255] Call Trace: > [79162.634974] ? _cond_resched+0x15/0x30 > [79162.635686] ? kmem_cache_alloc_trace+0x162/0x220 > [79162.636398] ? selinux_bpf_prog_alloc+0x1f/0x60 > [79162.637111] bpf_prog_load+0x3de/0x690 > [79162.637809] __do_sys_bpf+0x105/0x1740 > [79162.638488] do_syscall_64+0x5b/0x180 > [79162.639147] entry_SYSCALL_64_after_hwframe+0x44/0xa9 > ...... > > Fixes: 5b92a28aae4d ("bpf: Support attaching tracing BPF program to other BPF programs") > Reported-by: Eelco Chaudron <echaudro@redhat.com> > Signed-off-by: Yonghong Song <yhs@fb.com> > --- > kernel/bpf/verifier.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index a0482e1c4a77..034ef81f935b 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -9636,7 +9636,10 @@ static int check_attach_btf_id(struct bpf_verifier_env *env) > ret = -EINVAL; > goto out; > } > - addr = (long) tgt_prog->aux->func[subprog]->bpf_func; > + if (subprog == 0) > + addr = (long) tgt_prog->bpf_func; > + else > + addr = (long) tgt_prog->aux->func[subprog]->bpf_func; That is exactly the code I had while developing, but then decided to simplify it, since tgt_prog->aux->func[0]->bpf_func == tgt_prog->bpf_func. Oh well. Thanks for the fix! ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH bpf 2/2] selftests/bpf: add a fexit/bpf2bpf test with target bpf prog no callees 2019-12-05 1:06 [PATCH bpf 0/2] fix a verifier bug in check_attach_btf_id() Yonghong Song 2019-12-05 1:06 ` [PATCH bpf 1/2] bpf: fix a bug when getting subprog 0 jited image in check_attach_btf_id Yonghong Song @ 2019-12-05 1:06 ` Yonghong Song 2019-12-05 5:37 ` Alexei Starovoitov 1 sibling, 1 reply; 5+ messages in thread From: Yonghong Song @ 2019-12-05 1:06 UTC (permalink / raw) To: bpf; +Cc: Alexei Starovoitov, Daniel Borkmann, kernel-team The existing fexit_bpf2bpf test covers the target progrm with callees. This patch added a test for the target program without callees. Signed-off-by: Yonghong Song <yhs@fb.com> --- .../selftests/bpf/prog_tests/fexit_bpf2bpf.c | 70 ++++++++++++++----- .../bpf/progs/fexit_bpf2bpf_simple.c | 26 +++++++ .../selftests/bpf/progs/test_pkt_md_access.c | 4 +- 3 files changed, 81 insertions(+), 19 deletions(-) create mode 100644 tools/testing/selftests/bpf/progs/fexit_bpf2bpf_simple.c diff --git a/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c b/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c index 15c7378362dd..5dd37c37b29a 100644 --- a/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c +++ b/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c @@ -2,25 +2,21 @@ /* Copyright (c) 2019 Facebook */ #include <test_progs.h> -#define PROG_CNT 3 - -void test_fexit_bpf2bpf(void) +static void test_fexit_bpf2bpf_common(const char *obj_file, + const char *target_obj_file, + int prog_cnt, + const char **prog_name) { - const char *prog_name[PROG_CNT] = { - "fexit/test_pkt_access", - "fexit/test_pkt_access_subprog1", - "fexit/test_pkt_access_subprog2", - }; struct bpf_object *obj = NULL, *pkt_obj; int err, pkt_fd, i; - struct bpf_link *link[PROG_CNT] = {}; - struct bpf_program *prog[PROG_CNT]; + struct bpf_link **link = NULL; + struct bpf_program **prog = NULL; __u32 duration, retval; struct bpf_map *data_map; const int zero = 0; - u64 result[PROG_CNT]; + u64 *result = NULL; - err = bpf_prog_load("./test_pkt_access.o", BPF_PROG_TYPE_UNSPEC, + err = bpf_prog_load(target_obj_file, BPF_PROG_TYPE_UNSPEC, &pkt_obj, &pkt_fd); if (CHECK(err, "prog_load sched cls", "err %d errno %d\n", err, errno)) return; @@ -28,7 +24,7 @@ void test_fexit_bpf2bpf(void) .attach_prog_fd = pkt_fd, ); - obj = bpf_object__open_file("./fexit_bpf2bpf.o", &opts); + obj = bpf_object__open_file(obj_file, &opts); if (CHECK(IS_ERR_OR_NULL(obj), "obj_open", "failed to open fexit_bpf2bpf: %ld\n", PTR_ERR(obj))) @@ -38,7 +34,14 @@ void test_fexit_bpf2bpf(void) if (CHECK(err, "obj_load", "err %d\n", err)) goto close_prog; - for (i = 0; i < PROG_CNT; i++) { + link = calloc(sizeof(struct bpf_link *), prog_cnt); + prog = calloc(sizeof(struct bpf_program *), prog_cnt); + result = malloc(prog_cnt * sizeof(u64)); + if (CHECK(!link || !prog || !result, "alloc_memory", + "failed to alloc memory")) + goto close_prog; + + for (i = 0; i < prog_cnt; i++) { prog[i] = bpf_object__find_program_by_title(obj, prog_name[i]); if (CHECK(!prog[i], "find_prog", "prog %s not found\n", prog_name[i])) goto close_prog; @@ -56,21 +59,54 @@ void test_fexit_bpf2bpf(void) "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); + 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)) goto close_prog; - for (i = 0; i < PROG_CNT; i++) + for (i = 0; i < prog_cnt; i++) if (CHECK(result[i] != 1, "result", "fexit_bpf2bpf failed err %ld\n", result[i])) goto close_prog; close_prog: - for (i = 0; i < PROG_CNT; i++) + 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); + free(link); + free(prog); + free(result); +} + +static void test_target_no_callees(void) +{ + const char *prog_name[] = { + "fexit/test_pkt_md_access", + }; + test_fexit_bpf2bpf_common("./fexit_bpf2bpf_simple.o", + "./test_pkt_md_access.o", + ARRAY_SIZE(prog_name), + prog_name); +} + +static void test_target_yes_callees(void) +{ + const char *prog_name[] = { + "fexit/test_pkt_access", + "fexit/test_pkt_access_subprog1", + "fexit/test_pkt_access_subprog2", + }; + test_fexit_bpf2bpf_common("./fexit_bpf2bpf.o", + "./test_pkt_access.o", + ARRAY_SIZE(prog_name), + prog_name); +} + +void test_fexit_bpf2bpf(void) +{ + test_target_no_callees(); + test_target_yes_callees(); } diff --git a/tools/testing/selftests/bpf/progs/fexit_bpf2bpf_simple.c b/tools/testing/selftests/bpf/progs/fexit_bpf2bpf_simple.c new file mode 100644 index 000000000000..ebc0ab7f0f5c --- /dev/null +++ b/tools/testing/selftests/bpf/progs/fexit_bpf2bpf_simple.c @@ -0,0 +1,26 @@ +// SPDX-License-Identifier: GPL-2.0 +/* Copyright (c) 2019 Facebook */ +#include <linux/bpf.h> +#include "bpf_helpers.h" +#include "bpf_trace_helpers.h" + +struct sk_buff { + unsigned int len; +}; + +__u64 test_result = 0; +BPF_TRACE_2("fexit/test_pkt_md_access", test_main2, + struct sk_buff *, skb, int, ret) +{ + int len; + + __builtin_preserve_access_index(({ + len = skb->len; + })); + if (len != 74 || ret != 0) + return 0; + + test_result = 1; + return 0; +} +char _license[] SEC("license") = "GPL"; diff --git a/tools/testing/selftests/bpf/progs/test_pkt_md_access.c b/tools/testing/selftests/bpf/progs/test_pkt_md_access.c index 3d039e18bf82..1db2623021ad 100644 --- a/tools/testing/selftests/bpf/progs/test_pkt_md_access.c +++ b/tools/testing/selftests/bpf/progs/test_pkt_md_access.c @@ -27,8 +27,8 @@ int _version SEC("version") = 1; } #endif -SEC("test1") -int process(struct __sk_buff *skb) +SEC("classifier/test_pkt_md_access") +int test_pkt_md_access(struct __sk_buff *skb) { TEST_FIELD(__u8, len, 0xFF); TEST_FIELD(__u16, len, 0xFFFF); -- 2.17.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH bpf 2/2] selftests/bpf: add a fexit/bpf2bpf test with target bpf prog no callees 2019-12-05 1:06 ` [PATCH bpf 2/2] selftests/bpf: add a fexit/bpf2bpf test with target bpf prog no callees Yonghong Song @ 2019-12-05 5:37 ` Alexei Starovoitov 0 siblings, 0 replies; 5+ messages in thread From: Alexei Starovoitov @ 2019-12-05 5:37 UTC (permalink / raw) To: Yonghong Song; +Cc: bpf, Alexei Starovoitov, Daniel Borkmann, kernel-team On Wed, Dec 04, 2019 at 05:06:07PM -0800, Yonghong Song wrote: > > - obj = bpf_object__open_file("./fexit_bpf2bpf.o", &opts); > + obj = bpf_object__open_file(obj_file, &opts); > if (CHECK(IS_ERR_OR_NULL(obj), "obj_open", > "failed to open fexit_bpf2bpf: %ld\n", > PTR_ERR(obj))) > @@ -38,7 +34,14 @@ void test_fexit_bpf2bpf(void) > if (CHECK(err, "obj_load", "err %d\n", err)) > goto close_prog; > > - for (i = 0; i < PROG_CNT; i++) { > + link = calloc(sizeof(struct bpf_link *), prog_cnt); > + prog = calloc(sizeof(struct bpf_program *), prog_cnt); > + result = malloc(prog_cnt * sizeof(u64)); > + if (CHECK(!link || !prog || !result, "alloc_memory", > + "failed to alloc memory")) > + goto close_prog; bpf_object__open_file() can fail when jit is off and for() loop in close_prog will segfault. I fixed it up by moving above 3 mallocs before bpf_object__open_file() and applied both patches. Thanks! ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-12-05 5:37 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-12-05 1:06 [PATCH bpf 0/2] fix a verifier bug in check_attach_btf_id() Yonghong Song 2019-12-05 1:06 ` [PATCH bpf 1/2] bpf: fix a bug when getting subprog 0 jited image in check_attach_btf_id Yonghong Song 2019-12-05 5:33 ` Alexei Starovoitov 2019-12-05 1:06 ` [PATCH bpf 2/2] selftests/bpf: add a fexit/bpf2bpf test with target bpf prog no callees Yonghong Song 2019-12-05 5:37 ` Alexei Starovoitov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).