* [PATCH bpf-next 0/3] bpf: Fix cookie values for kprobe multi
@ 2022-05-27 20:56 Jiri Olsa
2022-05-27 20:56 ` [PATCH bpf-next 1/3] selftests/bpf: Shuffle cookies symbols in kprobe multi test Jiri Olsa
` (2 more replies)
0 siblings, 3 replies; 20+ messages in thread
From: Jiri Olsa @ 2022-05-27 20:56 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
Cc: netdev, bpf, lkml, Martin KaFai Lau, Song Liu, Yonghong Song,
John Fastabend, KP Singh, Steven Rostedt, Masami Hiramatsu
hi,
there's bug in kprobe_multi link that makes cookies misplaced when
using symbols to attach. The reason is that we sort symbols by name
but not adjacent cookie values. Current test did not find it because
bpf_fentry_test* are already sorted by name.
thanks,
jirka
---
Jiri Olsa (3):
selftests/bpf: Shuffle cookies symbols in kprobe multi test
ftrace: Keep address offset in ftrace_lookup_symbols
bpf: Force cookies array to follow symbols sorting
kernel/trace/bpf_trace.c | 65 ++++++++++++++++++++++++++++++++++++++++++++++++++---------------
kernel/trace/ftrace.c | 13 +++++++++++--
tools/testing/selftests/bpf/prog_tests/bpf_cookie.c | 78 +++++++++++++++++++++++++++++++++++++++---------------------------------------
tools/testing/selftests/bpf/progs/kprobe_multi.c | 24 ++++++++++++------------
4 files changed, 112 insertions(+), 68 deletions(-)
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH bpf-next 1/3] selftests/bpf: Shuffle cookies symbols in kprobe multi test
2022-05-27 20:56 [PATCH bpf-next 0/3] bpf: Fix cookie values for kprobe multi Jiri Olsa
@ 2022-05-27 20:56 ` Jiri Olsa
2022-05-30 5:37 ` Song Liu
2022-05-27 20:56 ` [PATCH bpf-next 2/3] ftrace: Keep address offset in ftrace_lookup_symbols Jiri Olsa
2022-05-27 20:56 ` [PATCH bpf-next 3/3] bpf: Force cookies array to follow symbols sorting Jiri Olsa
2 siblings, 1 reply; 20+ messages in thread
From: Jiri Olsa @ 2022-05-27 20:56 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
Cc: netdev, bpf, lkml, Martin KaFai Lau, Song Liu, Yonghong Song,
John Fastabend, KP Singh, Steven Rostedt, Masami Hiramatsu
There's a kernel bug that causes cookies to be misplaced and
the reason we did not catch this with this test is that we
provide bpf_fentry_test* functions already sorted by name.
Shuffling function bpf_fentry_test2 deeper in the list and
keeping the current cookie values as before will trigger
the bug.
The kernel fix is coming in following changes.
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
.../selftests/bpf/prog_tests/bpf_cookie.c | 78 +++++++++----------
.../selftests/bpf/progs/kprobe_multi.c | 24 +++---
2 files changed, 51 insertions(+), 51 deletions(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c b/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c
index 83ef55e3caa4..2974b44f80fa 100644
--- a/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c
+++ b/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c
@@ -121,24 +121,24 @@ static void kprobe_multi_link_api_subtest(void)
})
GET_ADDR("bpf_fentry_test1", addrs[0]);
- GET_ADDR("bpf_fentry_test2", addrs[1]);
- GET_ADDR("bpf_fentry_test3", addrs[2]);
- GET_ADDR("bpf_fentry_test4", addrs[3]);
- GET_ADDR("bpf_fentry_test5", addrs[4]);
- GET_ADDR("bpf_fentry_test6", addrs[5]);
- GET_ADDR("bpf_fentry_test7", addrs[6]);
+ GET_ADDR("bpf_fentry_test3", addrs[1]);
+ GET_ADDR("bpf_fentry_test4", addrs[2]);
+ GET_ADDR("bpf_fentry_test5", addrs[3]);
+ GET_ADDR("bpf_fentry_test6", addrs[4]);
+ GET_ADDR("bpf_fentry_test7", addrs[5]);
+ GET_ADDR("bpf_fentry_test2", addrs[6]);
GET_ADDR("bpf_fentry_test8", addrs[7]);
#undef GET_ADDR
- cookies[0] = 1;
- cookies[1] = 2;
- cookies[2] = 3;
- cookies[3] = 4;
- cookies[4] = 5;
- cookies[5] = 6;
- cookies[6] = 7;
- cookies[7] = 8;
+ cookies[0] = 1; /* bpf_fentry_test1 */
+ cookies[1] = 2; /* bpf_fentry_test3 */
+ cookies[2] = 3; /* bpf_fentry_test4 */
+ cookies[3] = 4; /* bpf_fentry_test5 */
+ cookies[4] = 5; /* bpf_fentry_test6 */
+ cookies[5] = 6; /* bpf_fentry_test7 */
+ cookies[6] = 7; /* bpf_fentry_test2 */
+ cookies[7] = 8; /* bpf_fentry_test8 */
opts.kprobe_multi.addrs = (const unsigned long *) &addrs;
opts.kprobe_multi.cnt = ARRAY_SIZE(addrs);
@@ -149,14 +149,14 @@ static void kprobe_multi_link_api_subtest(void)
if (!ASSERT_GE(link1_fd, 0, "link1_fd"))
goto cleanup;
- cookies[0] = 8;
- cookies[1] = 7;
- cookies[2] = 6;
- cookies[3] = 5;
- cookies[4] = 4;
- cookies[5] = 3;
- cookies[6] = 2;
- cookies[7] = 1;
+ cookies[0] = 8; /* bpf_fentry_test1 */
+ cookies[1] = 7; /* bpf_fentry_test3 */
+ cookies[2] = 6; /* bpf_fentry_test4 */
+ cookies[3] = 5; /* bpf_fentry_test5 */
+ cookies[4] = 4; /* bpf_fentry_test6 */
+ cookies[5] = 3; /* bpf_fentry_test7 */
+ cookies[6] = 2; /* bpf_fentry_test2 */
+ cookies[7] = 1; /* bpf_fentry_test8 */
opts.kprobe_multi.flags = BPF_F_KPROBE_MULTI_RETURN;
prog_fd = bpf_program__fd(skel->progs.test_kretprobe);
@@ -181,12 +181,12 @@ static void kprobe_multi_attach_api_subtest(void)
struct kprobe_multi *skel = NULL;
const char *syms[8] = {
"bpf_fentry_test1",
- "bpf_fentry_test2",
"bpf_fentry_test3",
"bpf_fentry_test4",
"bpf_fentry_test5",
"bpf_fentry_test6",
"bpf_fentry_test7",
+ "bpf_fentry_test2",
"bpf_fentry_test8",
};
__u64 cookies[8];
@@ -198,14 +198,14 @@ static void kprobe_multi_attach_api_subtest(void)
skel->bss->pid = getpid();
skel->bss->test_cookie = true;
- cookies[0] = 1;
- cookies[1] = 2;
- cookies[2] = 3;
- cookies[3] = 4;
- cookies[4] = 5;
- cookies[5] = 6;
- cookies[6] = 7;
- cookies[7] = 8;
+ cookies[0] = 1; /* bpf_fentry_test1 */
+ cookies[1] = 2; /* bpf_fentry_test3 */
+ cookies[2] = 3; /* bpf_fentry_test4 */
+ cookies[3] = 4; /* bpf_fentry_test5 */
+ cookies[4] = 5; /* bpf_fentry_test6 */
+ cookies[5] = 6; /* bpf_fentry_test7 */
+ cookies[6] = 7; /* bpf_fentry_test2 */
+ cookies[7] = 8; /* bpf_fentry_test8 */
opts.syms = syms;
opts.cnt = ARRAY_SIZE(syms);
@@ -216,14 +216,14 @@ static void kprobe_multi_attach_api_subtest(void)
if (!ASSERT_OK_PTR(link1, "bpf_program__attach_kprobe_multi_opts"))
goto cleanup;
- cookies[0] = 8;
- cookies[1] = 7;
- cookies[2] = 6;
- cookies[3] = 5;
- cookies[4] = 4;
- cookies[5] = 3;
- cookies[6] = 2;
- cookies[7] = 1;
+ cookies[0] = 8; /* bpf_fentry_test1 */
+ cookies[1] = 7; /* bpf_fentry_test3 */
+ cookies[2] = 6; /* bpf_fentry_test4 */
+ cookies[3] = 5; /* bpf_fentry_test5 */
+ cookies[4] = 4; /* bpf_fentry_test6 */
+ cookies[5] = 3; /* bpf_fentry_test7 */
+ cookies[6] = 2; /* bpf_fentry_test2 */
+ cookies[7] = 1; /* bpf_fentry_test8 */
opts.retprobe = true;
diff --git a/tools/testing/selftests/bpf/progs/kprobe_multi.c b/tools/testing/selftests/bpf/progs/kprobe_multi.c
index 93510f4f0f3a..08f95a8155d1 100644
--- a/tools/testing/selftests/bpf/progs/kprobe_multi.c
+++ b/tools/testing/selftests/bpf/progs/kprobe_multi.c
@@ -54,21 +54,21 @@ static void kprobe_multi_check(void *ctx, bool is_return)
if (is_return) {
SET(kretprobe_test1_result, &bpf_fentry_test1, 8);
- SET(kretprobe_test2_result, &bpf_fentry_test2, 7);
- SET(kretprobe_test3_result, &bpf_fentry_test3, 6);
- SET(kretprobe_test4_result, &bpf_fentry_test4, 5);
- SET(kretprobe_test5_result, &bpf_fentry_test5, 4);
- SET(kretprobe_test6_result, &bpf_fentry_test6, 3);
- SET(kretprobe_test7_result, &bpf_fentry_test7, 2);
+ SET(kretprobe_test2_result, &bpf_fentry_test2, 2);
+ SET(kretprobe_test3_result, &bpf_fentry_test3, 7);
+ SET(kretprobe_test4_result, &bpf_fentry_test4, 6);
+ SET(kretprobe_test5_result, &bpf_fentry_test5, 5);
+ SET(kretprobe_test6_result, &bpf_fentry_test6, 4);
+ SET(kretprobe_test7_result, &bpf_fentry_test7, 3);
SET(kretprobe_test8_result, &bpf_fentry_test8, 1);
} else {
SET(kprobe_test1_result, &bpf_fentry_test1, 1);
- SET(kprobe_test2_result, &bpf_fentry_test2, 2);
- SET(kprobe_test3_result, &bpf_fentry_test3, 3);
- SET(kprobe_test4_result, &bpf_fentry_test4, 4);
- SET(kprobe_test5_result, &bpf_fentry_test5, 5);
- SET(kprobe_test6_result, &bpf_fentry_test6, 6);
- SET(kprobe_test7_result, &bpf_fentry_test7, 7);
+ SET(kprobe_test2_result, &bpf_fentry_test2, 7);
+ SET(kprobe_test3_result, &bpf_fentry_test3, 2);
+ SET(kprobe_test4_result, &bpf_fentry_test4, 3);
+ SET(kprobe_test5_result, &bpf_fentry_test5, 4);
+ SET(kprobe_test6_result, &bpf_fentry_test6, 5);
+ SET(kprobe_test7_result, &bpf_fentry_test7, 6);
SET(kprobe_test8_result, &bpf_fentry_test8, 8);
}
--
2.35.3
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH bpf-next 2/3] ftrace: Keep address offset in ftrace_lookup_symbols
2022-05-27 20:56 [PATCH bpf-next 0/3] bpf: Fix cookie values for kprobe multi Jiri Olsa
2022-05-27 20:56 ` [PATCH bpf-next 1/3] selftests/bpf: Shuffle cookies symbols in kprobe multi test Jiri Olsa
@ 2022-05-27 20:56 ` Jiri Olsa
2022-05-30 5:37 ` Song Liu
` (3 more replies)
2022-05-27 20:56 ` [PATCH bpf-next 3/3] bpf: Force cookies array to follow symbols sorting Jiri Olsa
2 siblings, 4 replies; 20+ messages in thread
From: Jiri Olsa @ 2022-05-27 20:56 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
Cc: netdev, bpf, lkml, Martin KaFai Lau, Song Liu, Yonghong Song,
John Fastabend, KP Singh, Steven Rostedt, Masami Hiramatsu
We want to store the resolved address on the same index as
the symbol string, because that's the user (bpf kprobe link)
code assumption.
Also making sure we don't store duplicates that might be
present in kallsyms.
Fixes: bed0d9a50dac ("ftrace: Add ftrace_lookup_symbols function")
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
kernel/trace/ftrace.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 674add0aafb3..00d0ba6397ed 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -7984,15 +7984,23 @@ static int kallsyms_callback(void *data, const char *name,
struct module *mod, unsigned long addr)
{
struct kallsyms_data *args = data;
+ const char **sym;
+ int idx;
- if (!bsearch(&name, args->syms, args->cnt, sizeof(*args->syms), symbols_cmp))
+ sym = bsearch(&name, args->syms, args->cnt, sizeof(*args->syms), symbols_cmp);
+ if (!sym)
+ return 0;
+
+ idx = sym - args->syms;
+ if (args->addrs[idx])
return 0;
addr = ftrace_location(addr);
if (!addr)
return 0;
- args->addrs[args->found++] = addr;
+ args->addrs[idx] = addr;
+ args->found++;
return args->found == args->cnt ? 1 : 0;
}
@@ -8017,6 +8025,7 @@ int ftrace_lookup_symbols(const char **sorted_syms, size_t cnt, unsigned long *a
struct kallsyms_data args;
int err;
+ memset(addrs, 0x0, sizeof(*addrs) * cnt);
args.addrs = addrs;
args.syms = sorted_syms;
args.cnt = cnt;
--
2.35.3
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH bpf-next 3/3] bpf: Force cookies array to follow symbols sorting
2022-05-27 20:56 [PATCH bpf-next 0/3] bpf: Fix cookie values for kprobe multi Jiri Olsa
2022-05-27 20:56 ` [PATCH bpf-next 1/3] selftests/bpf: Shuffle cookies symbols in kprobe multi test Jiri Olsa
2022-05-27 20:56 ` [PATCH bpf-next 2/3] ftrace: Keep address offset in ftrace_lookup_symbols Jiri Olsa
@ 2022-05-27 20:56 ` Jiri Olsa
2022-05-30 5:40 ` Song Liu
2022-06-02 23:01 ` Andrii Nakryiko
2 siblings, 2 replies; 20+ messages in thread
From: Jiri Olsa @ 2022-05-27 20:56 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
Cc: netdev, bpf, lkml, Martin KaFai Lau, Song Liu, Yonghong Song,
John Fastabend, KP Singh, Steven Rostedt, Masami Hiramatsu
When user specifies symbols and cookies for kprobe_multi link
interface it's very likely the cookies will be misplaced and
returned to wrong functions (via get_attach_cookie helper).
The reason is that to resolve the provided functions we sort
them before passing them to ftrace_lookup_symbols, but we do
not do the same sort on the cookie values.
Fixing this by using sort_r function with custom swap callback
that swaps cookie values as well.
Fixes: 0236fec57a15 ("bpf: Resolve symbols with ftrace_lookup_symbols for kprobe multi link")
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
kernel/trace/bpf_trace.c | 65 ++++++++++++++++++++++++++++++----------
1 file changed, 50 insertions(+), 15 deletions(-)
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 10b157a6d73e..e5c423b835ab 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -2423,7 +2423,12 @@ kprobe_multi_link_handler(struct fprobe *fp, unsigned long entry_ip,
kprobe_multi_link_prog_run(link, entry_ip, regs);
}
-static int symbols_cmp(const void *a, const void *b)
+struct multi_symbols_sort {
+ const char **funcs;
+ u64 *cookies;
+};
+
+static int symbols_cmp_r(const void *a, const void *b, const void *priv)
{
const char **str_a = (const char **) a;
const char **str_b = (const char **) b;
@@ -2431,6 +2436,25 @@ static int symbols_cmp(const void *a, const void *b)
return strcmp(*str_a, *str_b);
}
+static void symbols_swap_r(void *a, void *b, int size, const void *priv)
+{
+ const struct multi_symbols_sort *data = priv;
+ const char **name_a = a, **name_b = b;
+ u64 *cookie_a, *cookie_b;
+
+ cookie_a = data->cookies + (name_a - data->funcs);
+ cookie_b = data->cookies + (name_b - data->funcs);
+
+ /* swap name_a/name_b and cookie_a/cookie_b values */
+ swap(*name_a, *name_b);
+ swap(*cookie_a, *cookie_b);
+}
+
+static int symbols_cmp(const void *a, const void *b)
+{
+ return symbols_cmp_r(a, b, NULL);
+}
+
int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *prog)
{
struct bpf_kprobe_multi_link *link = NULL;
@@ -2468,6 +2492,19 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
if (!addrs)
return -ENOMEM;
+ ucookies = u64_to_user_ptr(attr->link_create.kprobe_multi.cookies);
+ if (ucookies) {
+ cookies = kvmalloc(size, GFP_KERNEL);
+ if (!cookies) {
+ err = -ENOMEM;
+ goto error;
+ }
+ if (copy_from_user(cookies, ucookies, size)) {
+ err = -EFAULT;
+ goto error;
+ }
+ }
+
if (uaddrs) {
if (copy_from_user(addrs, uaddrs, size)) {
err = -EFAULT;
@@ -2480,26 +2517,24 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
if (err)
goto error;
- sort(us.syms, cnt, sizeof(*us.syms), symbols_cmp, NULL);
+ if (cookies) {
+ struct multi_symbols_sort data = {
+ .cookies = cookies,
+ .funcs = us.syms,
+ };
+
+ sort_r(us.syms, cnt, sizeof(*us.syms), symbols_cmp_r,
+ symbols_swap_r, &data);
+ } else {
+ sort(us.syms, cnt, sizeof(*us.syms), symbols_cmp, NULL);
+ }
+
err = ftrace_lookup_symbols(us.syms, cnt, addrs);
free_user_syms(&us);
if (err)
goto error;
}
- ucookies = u64_to_user_ptr(attr->link_create.kprobe_multi.cookies);
- if (ucookies) {
- cookies = kvmalloc(size, GFP_KERNEL);
- if (!cookies) {
- err = -ENOMEM;
- goto error;
- }
- if (copy_from_user(cookies, ucookies, size)) {
- err = -EFAULT;
- goto error;
- }
- }
-
link = kzalloc(sizeof(*link), GFP_KERNEL);
if (!link) {
err = -ENOMEM;
--
2.35.3
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH bpf-next 1/3] selftests/bpf: Shuffle cookies symbols in kprobe multi test
2022-05-27 20:56 ` [PATCH bpf-next 1/3] selftests/bpf: Shuffle cookies symbols in kprobe multi test Jiri Olsa
@ 2022-05-30 5:37 ` Song Liu
0 siblings, 0 replies; 20+ messages in thread
From: Song Liu @ 2022-05-30 5:37 UTC (permalink / raw)
To: Jiri Olsa
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Networking,
bpf, lkml, Martin Lau, Yonghong Song, John Fastabend, KP Singh,
Steven Rostedt, Masami Hiramatsu
> On May 27, 2022, at 1:56 PM, Jiri Olsa <jolsa@kernel.org> wrote:
>
> There's a kernel bug that causes cookies to be misplaced and
> the reason we did not catch this with this test is that we
> provide bpf_fentry_test* functions already sorted by name.
>
> Shuffling function bpf_fentry_test2 deeper in the list and
> keeping the current cookie values as before will trigger
> the bug.
>
> The kernel fix is coming in following changes.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Acked-by: Song Liu <songliubraving@fb.com>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH bpf-next 2/3] ftrace: Keep address offset in ftrace_lookup_symbols
2022-05-27 20:56 ` [PATCH bpf-next 2/3] ftrace: Keep address offset in ftrace_lookup_symbols Jiri Olsa
@ 2022-05-30 5:37 ` Song Liu
2022-05-30 7:31 ` Jiri Olsa
2022-05-30 20:20 ` Daniel Borkmann
` (2 subsequent siblings)
3 siblings, 1 reply; 20+ messages in thread
From: Song Liu @ 2022-05-30 5:37 UTC (permalink / raw)
To: Jiri Olsa
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, netdev,
bpf, lkml, Martin Lau, Yonghong Song, John Fastabend, KP Singh,
Steven Rostedt, Masami Hiramatsu
> On May 27, 2022, at 1:56 PM, Jiri Olsa <jolsa@kernel.org> wrote:
>
> We want to store the resolved address on the same index as
> the symbol string, because that's the user (bpf kprobe link)
> code assumption.
>
> Also making sure we don't store duplicates that might be
> present in kallsyms.
>
> Fixes: bed0d9a50dac ("ftrace: Add ftrace_lookup_symbols function")
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Acked-by: Song Liu <songliubraving@fb.com>
BTW, I guess this set should apply to bpf tree?
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH bpf-next 3/3] bpf: Force cookies array to follow symbols sorting
2022-05-27 20:56 ` [PATCH bpf-next 3/3] bpf: Force cookies array to follow symbols sorting Jiri Olsa
@ 2022-05-30 5:40 ` Song Liu
2022-06-02 23:01 ` Andrii Nakryiko
1 sibling, 0 replies; 20+ messages in thread
From: Song Liu @ 2022-05-30 5:40 UTC (permalink / raw)
To: Jiri Olsa
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, netdev,
bpf, lkml, Martin Lau, Yonghong Song, John Fastabend, KP Singh,
Steven Rostedt, Masami Hiramatsu
> On May 27, 2022, at 1:56 PM, Jiri Olsa <jolsa@kernel.org> wrote:
>
> When user specifies symbols and cookies for kprobe_multi link
> interface it's very likely the cookies will be misplaced and
> returned to wrong functions (via get_attach_cookie helper).
>
> The reason is that to resolve the provided functions we sort
> them before passing them to ftrace_lookup_symbols, but we do
> not do the same sort on the cookie values.
>
> Fixing this by using sort_r function with custom swap callback
> that swaps cookie values as well.
>
> Fixes: 0236fec57a15 ("bpf: Resolve symbols with ftrace_lookup_symbols for kprobe multi link")
> Signed-off-by: Jiri Olsa <jolsa@kernel.org
Acked-by: Song Liu <songliubraving@fb.com>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH bpf-next 2/3] ftrace: Keep address offset in ftrace_lookup_symbols
2022-05-30 5:37 ` Song Liu
@ 2022-05-30 7:31 ` Jiri Olsa
0 siblings, 0 replies; 20+ messages in thread
From: Jiri Olsa @ 2022-05-30 7:31 UTC (permalink / raw)
To: Song Liu
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, netdev,
bpf, lkml, Martin Lau, Yonghong Song, John Fastabend, KP Singh,
Steven Rostedt, Masami Hiramatsu
On Mon, May 30, 2022 at 05:37:49AM +0000, Song Liu wrote:
>
>
> > On May 27, 2022, at 1:56 PM, Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > We want to store the resolved address on the same index as
> > the symbol string, because that's the user (bpf kprobe link)
> > code assumption.
> >
> > Also making sure we don't store duplicates that might be
> > present in kallsyms.
> >
> > Fixes: bed0d9a50dac ("ftrace: Add ftrace_lookup_symbols function")
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
>
> Acked-by: Song Liu <songliubraving@fb.com>
>
> BTW, I guess this set should apply to bpf tree?
>
ah right, I checked and it still applies on bpf/master,
please let me know if I need to resend without 'bpf-next'
thanks,
jirka
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH bpf-next 2/3] ftrace: Keep address offset in ftrace_lookup_symbols
2022-05-27 20:56 ` [PATCH bpf-next 2/3] ftrace: Keep address offset in ftrace_lookup_symbols Jiri Olsa
2022-05-30 5:37 ` Song Liu
@ 2022-05-30 20:20 ` Daniel Borkmann
2022-06-05 16:55 ` Steven Rostedt
2022-06-02 22:52 ` Andrii Nakryiko
2022-06-05 16:51 ` Steven Rostedt
3 siblings, 1 reply; 20+ messages in thread
From: Daniel Borkmann @ 2022-05-30 20:20 UTC (permalink / raw)
To: Jiri Olsa, Alexei Starovoitov, Andrii Nakryiko
Cc: netdev, bpf, lkml, Martin KaFai Lau, Song Liu, Yonghong Song,
John Fastabend, KP Singh, Steven Rostedt, Masami Hiramatsu
On 5/27/22 10:56 PM, Jiri Olsa wrote:
> We want to store the resolved address on the same index as
> the symbol string, because that's the user (bpf kprobe link)
> code assumption.
>
> Also making sure we don't store duplicates that might be
> present in kallsyms.
>
> Fixes: bed0d9a50dac ("ftrace: Add ftrace_lookup_symbols function")
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
> kernel/trace/ftrace.c | 13 +++++++++++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
Steven / Masami, would be great to get an Ack from one of you before applying.
Thanks,
Daniel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH bpf-next 2/3] ftrace: Keep address offset in ftrace_lookup_symbols
2022-05-27 20:56 ` [PATCH bpf-next 2/3] ftrace: Keep address offset in ftrace_lookup_symbols Jiri Olsa
2022-05-30 5:37 ` Song Liu
2022-05-30 20:20 ` Daniel Borkmann
@ 2022-06-02 22:52 ` Andrii Nakryiko
2022-06-03 10:16 ` Jiri Olsa
2022-06-05 16:51 ` Steven Rostedt
3 siblings, 1 reply; 20+ messages in thread
From: Andrii Nakryiko @ 2022-06-02 22:52 UTC (permalink / raw)
To: Jiri Olsa
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Networking,
bpf, lkml, Martin KaFai Lau, Song Liu, Yonghong Song,
John Fastabend, KP Singh, Steven Rostedt, Masami Hiramatsu
On Fri, May 27, 2022 at 1:56 PM Jiri Olsa <jolsa@kernel.org> wrote:
>
> We want to store the resolved address on the same index as
> the symbol string, because that's the user (bpf kprobe link)
> code assumption.
>
> Also making sure we don't store duplicates that might be
> present in kallsyms.
>
> Fixes: bed0d9a50dac ("ftrace: Add ftrace_lookup_symbols function")
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
> kernel/trace/ftrace.c | 13 +++++++++++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index 674add0aafb3..00d0ba6397ed 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -7984,15 +7984,23 @@ static int kallsyms_callback(void *data, const char *name,
> struct module *mod, unsigned long addr)
> {
> struct kallsyms_data *args = data;
> + const char **sym;
> + int idx;
>
> - if (!bsearch(&name, args->syms, args->cnt, sizeof(*args->syms), symbols_cmp))
> + sym = bsearch(&name, args->syms, args->cnt, sizeof(*args->syms), symbols_cmp);
> + if (!sym)
> + return 0;
> +
> + idx = sym - args->syms;
> + if (args->addrs[idx])
if we have duplicated symbols we won't increment args->found here,
right? So we won't stop early. But we also don't want to increment
args->found here because we use it to check that we don't have
duplicates (in addition to making sure we resolved all the unique
symbols), right?
So I wonder if in this situation should we return some error code to
signify that we encountered symbol duplicate?
> return 0;
>
> addr = ftrace_location(addr);
> if (!addr)
> return 0;
>
> - args->addrs[args->found++] = addr;
> + args->addrs[idx] = addr;
> + args->found++;
> return args->found == args->cnt ? 1 : 0;
> }
>
> @@ -8017,6 +8025,7 @@ int ftrace_lookup_symbols(const char **sorted_syms, size_t cnt, unsigned long *a
> struct kallsyms_data args;
> int err;
>
> + memset(addrs, 0x0, sizeof(*addrs) * cnt);
> args.addrs = addrs;
> args.syms = sorted_syms;
> args.cnt = cnt;
> --
> 2.35.3
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH bpf-next 3/3] bpf: Force cookies array to follow symbols sorting
2022-05-27 20:56 ` [PATCH bpf-next 3/3] bpf: Force cookies array to follow symbols sorting Jiri Olsa
2022-05-30 5:40 ` Song Liu
@ 2022-06-02 23:01 ` Andrii Nakryiko
2022-06-02 23:02 ` Andrii Nakryiko
2022-06-03 10:18 ` Jiri Olsa
1 sibling, 2 replies; 20+ messages in thread
From: Andrii Nakryiko @ 2022-06-02 23:01 UTC (permalink / raw)
To: Jiri Olsa
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Networking,
bpf, lkml, Martin KaFai Lau, Song Liu, Yonghong Song,
John Fastabend, KP Singh, Steven Rostedt, Masami Hiramatsu
On Fri, May 27, 2022 at 1:57 PM Jiri Olsa <jolsa@kernel.org> wrote:
>
> When user specifies symbols and cookies for kprobe_multi link
> interface it's very likely the cookies will be misplaced and
> returned to wrong functions (via get_attach_cookie helper).
>
> The reason is that to resolve the provided functions we sort
> them before passing them to ftrace_lookup_symbols, but we do
> not do the same sort on the cookie values.
>
> Fixing this by using sort_r function with custom swap callback
> that swaps cookie values as well.
>
> Fixes: 0236fec57a15 ("bpf: Resolve symbols with ftrace_lookup_symbols for kprobe multi link")
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
> kernel/trace/bpf_trace.c | 65 ++++++++++++++++++++++++++++++----------
> 1 file changed, 50 insertions(+), 15 deletions(-)
>
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index 10b157a6d73e..e5c423b835ab 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -2423,7 +2423,12 @@ kprobe_multi_link_handler(struct fprobe *fp, unsigned long entry_ip,
> kprobe_multi_link_prog_run(link, entry_ip, regs);
> }
>
> -static int symbols_cmp(const void *a, const void *b)
> +struct multi_symbols_sort {
> + const char **funcs;
> + u64 *cookies;
> +};
> +
> +static int symbols_cmp_r(const void *a, const void *b, const void *priv)
> {
> const char **str_a = (const char **) a;
> const char **str_b = (const char **) b;
> @@ -2431,6 +2436,25 @@ static int symbols_cmp(const void *a, const void *b)
> return strcmp(*str_a, *str_b);
> }
>
> +static void symbols_swap_r(void *a, void *b, int size, const void *priv)
> +{
> + const struct multi_symbols_sort *data = priv;
> + const char **name_a = a, **name_b = b;
> + u64 *cookie_a, *cookie_b;
> +
> + cookie_a = data->cookies + (name_a - data->funcs);
> + cookie_b = data->cookies + (name_b - data->funcs);
> +
> + /* swap name_a/name_b and cookie_a/cookie_b values */
> + swap(*name_a, *name_b);
> + swap(*cookie_a, *cookie_b);
> +}
> +
> +static int symbols_cmp(const void *a, const void *b)
> +{
> + return symbols_cmp_r(a, b, NULL);
> +}
> +
> int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *prog)
> {
> struct bpf_kprobe_multi_link *link = NULL;
> @@ -2468,6 +2492,19 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
> if (!addrs)
> return -ENOMEM;
>
> + ucookies = u64_to_user_ptr(attr->link_create.kprobe_multi.cookies);
> + if (ucookies) {
> + cookies = kvmalloc(size, GFP_KERNEL);
> + if (!cookies) {
> + err = -ENOMEM;
> + goto error;
> + }
> + if (copy_from_user(cookies, ucookies, size)) {
> + err = -EFAULT;
> + goto error;
> + }
> + }
> +
> if (uaddrs) {
> if (copy_from_user(addrs, uaddrs, size)) {
> err = -EFAULT;
> @@ -2480,26 +2517,24 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
> if (err)
> goto error;
>
> - sort(us.syms, cnt, sizeof(*us.syms), symbols_cmp, NULL);
> + if (cookies) {
> + struct multi_symbols_sort data = {
> + .cookies = cookies,
> + .funcs = us.syms,
> + };
> +
> + sort_r(us.syms, cnt, sizeof(*us.syms), symbols_cmp_r,
> + symbols_swap_r, &data);
> + } else {
> + sort(us.syms, cnt, sizeof(*us.syms), symbols_cmp, NULL);
> + }
maybe just always do sort_r, swap callback can just check if cookie
array is NULL and if not, additionally swap cookies? why have all
these different callbacks and complicate the code unnecessarily?
> +
> err = ftrace_lookup_symbols(us.syms, cnt, addrs);
> free_user_syms(&us);
> if (err)
> goto error;
> }
>
> - ucookies = u64_to_user_ptr(attr->link_create.kprobe_multi.cookies);
> - if (ucookies) {
> - cookies = kvmalloc(size, GFP_KERNEL);
> - if (!cookies) {
> - err = -ENOMEM;
> - goto error;
> - }
> - if (copy_from_user(cookies, ucookies, size)) {
> - err = -EFAULT;
> - goto error;
> - }
> - }
> -
> link = kzalloc(sizeof(*link), GFP_KERNEL);
> if (!link) {
> err = -ENOMEM;
> --
> 2.35.3
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH bpf-next 3/3] bpf: Force cookies array to follow symbols sorting
2022-06-02 23:01 ` Andrii Nakryiko
@ 2022-06-02 23:02 ` Andrii Nakryiko
2022-06-03 10:23 ` Jiri Olsa
2022-06-03 10:18 ` Jiri Olsa
1 sibling, 1 reply; 20+ messages in thread
From: Andrii Nakryiko @ 2022-06-02 23:02 UTC (permalink / raw)
To: Jiri Olsa
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Networking,
bpf, lkml, Martin KaFai Lau, Song Liu, Yonghong Song,
John Fastabend, KP Singh, Steven Rostedt, Masami Hiramatsu
On Thu, Jun 2, 2022 at 4:01 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Fri, May 27, 2022 at 1:57 PM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > When user specifies symbols and cookies for kprobe_multi link
> > interface it's very likely the cookies will be misplaced and
> > returned to wrong functions (via get_attach_cookie helper).
> >
> > The reason is that to resolve the provided functions we sort
> > them before passing them to ftrace_lookup_symbols, but we do
> > not do the same sort on the cookie values.
> >
> > Fixing this by using sort_r function with custom swap callback
> > that swaps cookie values as well.
> >
> > Fixes: 0236fec57a15 ("bpf: Resolve symbols with ftrace_lookup_symbols for kprobe multi link")
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> > kernel/trace/bpf_trace.c | 65 ++++++++++++++++++++++++++++++----------
> > 1 file changed, 50 insertions(+), 15 deletions(-)
> >
> > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> > index 10b157a6d73e..e5c423b835ab 100644
> > --- a/kernel/trace/bpf_trace.c
> > +++ b/kernel/trace/bpf_trace.c
> > @@ -2423,7 +2423,12 @@ kprobe_multi_link_handler(struct fprobe *fp, unsigned long entry_ip,
> > kprobe_multi_link_prog_run(link, entry_ip, regs);
> > }
> >
> > -static int symbols_cmp(const void *a, const void *b)
> > +struct multi_symbols_sort {
> > + const char **funcs;
> > + u64 *cookies;
> > +};
> > +
> > +static int symbols_cmp_r(const void *a, const void *b, const void *priv)
> > {
> > const char **str_a = (const char **) a;
> > const char **str_b = (const char **) b;
> > @@ -2431,6 +2436,25 @@ static int symbols_cmp(const void *a, const void *b)
> > return strcmp(*str_a, *str_b);
> > }
> >
> > +static void symbols_swap_r(void *a, void *b, int size, const void *priv)
> > +{
> > + const struct multi_symbols_sort *data = priv;
> > + const char **name_a = a, **name_b = b;
> > + u64 *cookie_a, *cookie_b;
> > +
> > + cookie_a = data->cookies + (name_a - data->funcs);
> > + cookie_b = data->cookies + (name_b - data->funcs);
> > +
> > + /* swap name_a/name_b and cookie_a/cookie_b values */
> > + swap(*name_a, *name_b);
> > + swap(*cookie_a, *cookie_b);
> > +}
> > +
> > +static int symbols_cmp(const void *a, const void *b)
> > +{
> > + return symbols_cmp_r(a, b, NULL);
> > +}
> > +
> > int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *prog)
> > {
> > struct bpf_kprobe_multi_link *link = NULL;
> > @@ -2468,6 +2492,19 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
> > if (!addrs)
> > return -ENOMEM;
> >
> > + ucookies = u64_to_user_ptr(attr->link_create.kprobe_multi.cookies);
> > + if (ucookies) {
> > + cookies = kvmalloc(size, GFP_KERNEL);
oh, and you'll have to rebase anyways after kvmalloc_array patch
> > + if (!cookies) {
> > + err = -ENOMEM;
> > + goto error;
> > + }
> > + if (copy_from_user(cookies, ucookies, size)) {
> > + err = -EFAULT;
> > + goto error;
> > + }
> > + }
> > +
> > if (uaddrs) {
> > if (copy_from_user(addrs, uaddrs, size)) {
> > err = -EFAULT;
> > @@ -2480,26 +2517,24 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
> > if (err)
> > goto error;
> >
> > - sort(us.syms, cnt, sizeof(*us.syms), symbols_cmp, NULL);
> > + if (cookies) {
> > + struct multi_symbols_sort data = {
> > + .cookies = cookies,
> > + .funcs = us.syms,
> > + };
> > +
> > + sort_r(us.syms, cnt, sizeof(*us.syms), symbols_cmp_r,
> > + symbols_swap_r, &data);
> > + } else {
> > + sort(us.syms, cnt, sizeof(*us.syms), symbols_cmp, NULL);
> > + }
>
> maybe just always do sort_r, swap callback can just check if cookie
> array is NULL and if not, additionally swap cookies? why have all
> these different callbacks and complicate the code unnecessarily?
>
> > +
> > err = ftrace_lookup_symbols(us.syms, cnt, addrs);
> > free_user_syms(&us);
> > if (err)
> > goto error;
> > }
> >
> > - ucookies = u64_to_user_ptr(attr->link_create.kprobe_multi.cookies);
> > - if (ucookies) {
> > - cookies = kvmalloc(size, GFP_KERNEL);
> > - if (!cookies) {
> > - err = -ENOMEM;
> > - goto error;
> > - }
> > - if (copy_from_user(cookies, ucookies, size)) {
> > - err = -EFAULT;
> > - goto error;
> > - }
> > - }
> > -
> > link = kzalloc(sizeof(*link), GFP_KERNEL);
> > if (!link) {
> > err = -ENOMEM;
> > --
> > 2.35.3
> >
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH bpf-next 2/3] ftrace: Keep address offset in ftrace_lookup_symbols
2022-06-02 22:52 ` Andrii Nakryiko
@ 2022-06-03 10:16 ` Jiri Olsa
2022-06-03 19:12 ` Andrii Nakryiko
0 siblings, 1 reply; 20+ messages in thread
From: Jiri Olsa @ 2022-06-03 10:16 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Networking,
bpf, lkml, Martin KaFai Lau, Song Liu, Yonghong Song,
John Fastabend, KP Singh, Steven Rostedt, Masami Hiramatsu
On Thu, Jun 02, 2022 at 03:52:03PM -0700, Andrii Nakryiko wrote:
> On Fri, May 27, 2022 at 1:56 PM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > We want to store the resolved address on the same index as
> > the symbol string, because that's the user (bpf kprobe link)
> > code assumption.
> >
> > Also making sure we don't store duplicates that might be
> > present in kallsyms.
> >
> > Fixes: bed0d9a50dac ("ftrace: Add ftrace_lookup_symbols function")
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> > kernel/trace/ftrace.c | 13 +++++++++++--
> > 1 file changed, 11 insertions(+), 2 deletions(-)
> >
> > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> > index 674add0aafb3..00d0ba6397ed 100644
> > --- a/kernel/trace/ftrace.c
> > +++ b/kernel/trace/ftrace.c
> > @@ -7984,15 +7984,23 @@ static int kallsyms_callback(void *data, const char *name,
> > struct module *mod, unsigned long addr)
> > {
> > struct kallsyms_data *args = data;
> > + const char **sym;
> > + int idx;
> >
> > - if (!bsearch(&name, args->syms, args->cnt, sizeof(*args->syms), symbols_cmp))
> > + sym = bsearch(&name, args->syms, args->cnt, sizeof(*args->syms), symbols_cmp);
> > + if (!sym)
> > + return 0;
> > +
> > + idx = sym - args->syms;
> > + if (args->addrs[idx])
>
> if we have duplicated symbols we won't increment args->found here,
> right? So we won't stop early. But we also don't want to increment
> args->found here because we use it to check that we don't have
> duplicates (in addition to making sure we resolved all the unique
> symbols), right?
>
> So I wonder if in this situation should we return some error code to
> signify that we encountered symbol duplicate?
hum, this callback is called for each kallsyms symbol and there
are duplicates in /proc/kallsyms.. so even if we have just single
copy of such symbol in args->syms, bsearch will find this single
symbol for all the duplicates in /proc/kallsyms and we will endup
in here.. and it's still fine, we should continue
jirka
>
>
> > return 0;
> >
> > addr = ftrace_location(addr);
> > if (!addr)
> > return 0;
> >
> > - args->addrs[args->found++] = addr;
> > + args->addrs[idx] = addr;
> > + args->found++;
> > return args->found == args->cnt ? 1 : 0;
> > }
> >
> > @@ -8017,6 +8025,7 @@ int ftrace_lookup_symbols(const char **sorted_syms, size_t cnt, unsigned long *a
> > struct kallsyms_data args;
> > int err;
> >
> > + memset(addrs, 0x0, sizeof(*addrs) * cnt);
> > args.addrs = addrs;
> > args.syms = sorted_syms;
> > args.cnt = cnt;
> > --
> > 2.35.3
> >
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH bpf-next 3/3] bpf: Force cookies array to follow symbols sorting
2022-06-02 23:01 ` Andrii Nakryiko
2022-06-02 23:02 ` Andrii Nakryiko
@ 2022-06-03 10:18 ` Jiri Olsa
1 sibling, 0 replies; 20+ messages in thread
From: Jiri Olsa @ 2022-06-03 10:18 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Networking,
bpf, lkml, Martin KaFai Lau, Song Liu, Yonghong Song,
John Fastabend, KP Singh, Steven Rostedt, Masami Hiramatsu
On Thu, Jun 02, 2022 at 04:01:07PM -0700, Andrii Nakryiko wrote:
> On Fri, May 27, 2022 at 1:57 PM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > When user specifies symbols and cookies for kprobe_multi link
> > interface it's very likely the cookies will be misplaced and
> > returned to wrong functions (via get_attach_cookie helper).
> >
> > The reason is that to resolve the provided functions we sort
> > them before passing them to ftrace_lookup_symbols, but we do
> > not do the same sort on the cookie values.
> >
> > Fixing this by using sort_r function with custom swap callback
> > that swaps cookie values as well.
> >
> > Fixes: 0236fec57a15 ("bpf: Resolve symbols with ftrace_lookup_symbols for kprobe multi link")
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> > kernel/trace/bpf_trace.c | 65 ++++++++++++++++++++++++++++++----------
> > 1 file changed, 50 insertions(+), 15 deletions(-)
> >
> > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> > index 10b157a6d73e..e5c423b835ab 100644
> > --- a/kernel/trace/bpf_trace.c
> > +++ b/kernel/trace/bpf_trace.c
> > @@ -2423,7 +2423,12 @@ kprobe_multi_link_handler(struct fprobe *fp, unsigned long entry_ip,
> > kprobe_multi_link_prog_run(link, entry_ip, regs);
> > }
> >
> > -static int symbols_cmp(const void *a, const void *b)
> > +struct multi_symbols_sort {
> > + const char **funcs;
> > + u64 *cookies;
> > +};
> > +
> > +static int symbols_cmp_r(const void *a, const void *b, const void *priv)
> > {
> > const char **str_a = (const char **) a;
> > const char **str_b = (const char **) b;
> > @@ -2431,6 +2436,25 @@ static int symbols_cmp(const void *a, const void *b)
> > return strcmp(*str_a, *str_b);
> > }
> >
> > +static void symbols_swap_r(void *a, void *b, int size, const void *priv)
> > +{
> > + const struct multi_symbols_sort *data = priv;
> > + const char **name_a = a, **name_b = b;
> > + u64 *cookie_a, *cookie_b;
> > +
> > + cookie_a = data->cookies + (name_a - data->funcs);
> > + cookie_b = data->cookies + (name_b - data->funcs);
> > +
> > + /* swap name_a/name_b and cookie_a/cookie_b values */
> > + swap(*name_a, *name_b);
> > + swap(*cookie_a, *cookie_b);
> > +}
> > +
> > +static int symbols_cmp(const void *a, const void *b)
> > +{
> > + return symbols_cmp_r(a, b, NULL);
> > +}
> > +
> > int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *prog)
> > {
> > struct bpf_kprobe_multi_link *link = NULL;
> > @@ -2468,6 +2492,19 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
> > if (!addrs)
> > return -ENOMEM;
> >
> > + ucookies = u64_to_user_ptr(attr->link_create.kprobe_multi.cookies);
> > + if (ucookies) {
> > + cookies = kvmalloc(size, GFP_KERNEL);
> > + if (!cookies) {
> > + err = -ENOMEM;
> > + goto error;
> > + }
> > + if (copy_from_user(cookies, ucookies, size)) {
> > + err = -EFAULT;
> > + goto error;
> > + }
> > + }
> > +
> > if (uaddrs) {
> > if (copy_from_user(addrs, uaddrs, size)) {
> > err = -EFAULT;
> > @@ -2480,26 +2517,24 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
> > if (err)
> > goto error;
> >
> > - sort(us.syms, cnt, sizeof(*us.syms), symbols_cmp, NULL);
> > + if (cookies) {
> > + struct multi_symbols_sort data = {
> > + .cookies = cookies,
> > + .funcs = us.syms,
> > + };
> > +
> > + sort_r(us.syms, cnt, sizeof(*us.syms), symbols_cmp_r,
> > + symbols_swap_r, &data);
> > + } else {
> > + sort(us.syms, cnt, sizeof(*us.syms), symbols_cmp, NULL);
> > + }
>
> maybe just always do sort_r, swap callback can just check if cookie
> array is NULL and if not, additionally swap cookies? why have all
> these different callbacks and complicate the code unnecessarily?
right, good idea.. will change
thanks,
jirka
>
> > +
> > err = ftrace_lookup_symbols(us.syms, cnt, addrs);
> > free_user_syms(&us);
> > if (err)
> > goto error;
> > }
> >
> > - ucookies = u64_to_user_ptr(attr->link_create.kprobe_multi.cookies);
> > - if (ucookies) {
> > - cookies = kvmalloc(size, GFP_KERNEL);
> > - if (!cookies) {
> > - err = -ENOMEM;
> > - goto error;
> > - }
> > - if (copy_from_user(cookies, ucookies, size)) {
> > - err = -EFAULT;
> > - goto error;
> > - }
> > - }
> > -
> > link = kzalloc(sizeof(*link), GFP_KERNEL);
> > if (!link) {
> > err = -ENOMEM;
> > --
> > 2.35.3
> >
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH bpf-next 3/3] bpf: Force cookies array to follow symbols sorting
2022-06-02 23:02 ` Andrii Nakryiko
@ 2022-06-03 10:23 ` Jiri Olsa
2022-06-03 21:56 ` Andrii Nakryiko
0 siblings, 1 reply; 20+ messages in thread
From: Jiri Olsa @ 2022-06-03 10:23 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Networking,
bpf, lkml, Martin KaFai Lau, Song Liu, Yonghong Song,
John Fastabend, KP Singh, Steven Rostedt, Masami Hiramatsu
On Thu, Jun 02, 2022 at 04:02:28PM -0700, Andrii Nakryiko wrote:
> On Thu, Jun 2, 2022 at 4:01 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Fri, May 27, 2022 at 1:57 PM Jiri Olsa <jolsa@kernel.org> wrote:
> > >
> > > When user specifies symbols and cookies for kprobe_multi link
> > > interface it's very likely the cookies will be misplaced and
> > > returned to wrong functions (via get_attach_cookie helper).
> > >
> > > The reason is that to resolve the provided functions we sort
> > > them before passing them to ftrace_lookup_symbols, but we do
> > > not do the same sort on the cookie values.
> > >
> > > Fixing this by using sort_r function with custom swap callback
> > > that swaps cookie values as well.
> > >
> > > Fixes: 0236fec57a15 ("bpf: Resolve symbols with ftrace_lookup_symbols for kprobe multi link")
> > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > ---
> > > kernel/trace/bpf_trace.c | 65 ++++++++++++++++++++++++++++++----------
> > > 1 file changed, 50 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> > > index 10b157a6d73e..e5c423b835ab 100644
> > > --- a/kernel/trace/bpf_trace.c
> > > +++ b/kernel/trace/bpf_trace.c
> > > @@ -2423,7 +2423,12 @@ kprobe_multi_link_handler(struct fprobe *fp, unsigned long entry_ip,
> > > kprobe_multi_link_prog_run(link, entry_ip, regs);
> > > }
> > >
> > > -static int symbols_cmp(const void *a, const void *b)
> > > +struct multi_symbols_sort {
> > > + const char **funcs;
> > > + u64 *cookies;
> > > +};
> > > +
> > > +static int symbols_cmp_r(const void *a, const void *b, const void *priv)
> > > {
> > > const char **str_a = (const char **) a;
> > > const char **str_b = (const char **) b;
> > > @@ -2431,6 +2436,25 @@ static int symbols_cmp(const void *a, const void *b)
> > > return strcmp(*str_a, *str_b);
> > > }
> > >
> > > +static void symbols_swap_r(void *a, void *b, int size, const void *priv)
> > > +{
> > > + const struct multi_symbols_sort *data = priv;
> > > + const char **name_a = a, **name_b = b;
> > > + u64 *cookie_a, *cookie_b;
> > > +
> > > + cookie_a = data->cookies + (name_a - data->funcs);
> > > + cookie_b = data->cookies + (name_b - data->funcs);
> > > +
> > > + /* swap name_a/name_b and cookie_a/cookie_b values */
> > > + swap(*name_a, *name_b);
> > > + swap(*cookie_a, *cookie_b);
> > > +}
> > > +
> > > +static int symbols_cmp(const void *a, const void *b)
> > > +{
> > > + return symbols_cmp_r(a, b, NULL);
> > > +}
> > > +
> > > int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *prog)
> > > {
> > > struct bpf_kprobe_multi_link *link = NULL;
> > > @@ -2468,6 +2492,19 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
> > > if (!addrs)
> > > return -ENOMEM;
> > >
> > > + ucookies = u64_to_user_ptr(attr->link_create.kprobe_multi.cookies);
> > > + if (ucookies) {
> > > + cookies = kvmalloc(size, GFP_KERNEL);
>
> oh, and you'll have to rebase anyways after kvmalloc_array patch
true, that kvmalloc_array change went to bpf-next/master,
but as Song mentioned this patchset should probably go for bpf/master?
I'm fine either way, let me know ;-)
thanks,
jirka
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH bpf-next 2/3] ftrace: Keep address offset in ftrace_lookup_symbols
2022-06-03 10:16 ` Jiri Olsa
@ 2022-06-03 19:12 ` Andrii Nakryiko
0 siblings, 0 replies; 20+ messages in thread
From: Andrii Nakryiko @ 2022-06-03 19:12 UTC (permalink / raw)
To: Jiri Olsa
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Networking,
bpf, lkml, Martin KaFai Lau, Song Liu, Yonghong Song,
John Fastabend, KP Singh, Steven Rostedt, Masami Hiramatsu
On Fri, Jun 3, 2022 at 3:16 AM Jiri Olsa <olsajiri@gmail.com> wrote:
>
> On Thu, Jun 02, 2022 at 03:52:03PM -0700, Andrii Nakryiko wrote:
> > On Fri, May 27, 2022 at 1:56 PM Jiri Olsa <jolsa@kernel.org> wrote:
> > >
> > > We want to store the resolved address on the same index as
> > > the symbol string, because that's the user (bpf kprobe link)
> > > code assumption.
> > >
> > > Also making sure we don't store duplicates that might be
> > > present in kallsyms.
> > >
> > > Fixes: bed0d9a50dac ("ftrace: Add ftrace_lookup_symbols function")
> > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > ---
> > > kernel/trace/ftrace.c | 13 +++++++++++--
> > > 1 file changed, 11 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> > > index 674add0aafb3..00d0ba6397ed 100644
> > > --- a/kernel/trace/ftrace.c
> > > +++ b/kernel/trace/ftrace.c
> > > @@ -7984,15 +7984,23 @@ static int kallsyms_callback(void *data, const char *name,
> > > struct module *mod, unsigned long addr)
> > > {
> > > struct kallsyms_data *args = data;
> > > + const char **sym;
> > > + int idx;
> > >
> > > - if (!bsearch(&name, args->syms, args->cnt, sizeof(*args->syms), symbols_cmp))
> > > + sym = bsearch(&name, args->syms, args->cnt, sizeof(*args->syms), symbols_cmp);
> > > + if (!sym)
> > > + return 0;
> > > +
> > > + idx = sym - args->syms;
> > > + if (args->addrs[idx])
> >
> > if we have duplicated symbols we won't increment args->found here,
> > right? So we won't stop early. But we also don't want to increment
> > args->found here because we use it to check that we don't have
> > duplicates (in addition to making sure we resolved all the unique
> > symbols), right?
> >
> > So I wonder if in this situation should we return some error code to
> > signify that we encountered symbol duplicate?
>
> hum, this callback is called for each kallsyms symbol and there
> are duplicates in /proc/kallsyms.. so even if we have just single
> copy of such symbol in args->syms, bsearch will find this single
> symbol for all the duplicates in /proc/kallsyms and we will endup
> in here.. and it's still fine, we should continue
>
ah, ok, duplicate kallsyms entries, right, never mind then
> jirka
>
> >
> >
> > > return 0;
> > >
> > > addr = ftrace_location(addr);
> > > if (!addr)
> > > return 0;
> > >
> > > - args->addrs[args->found++] = addr;
> > > + args->addrs[idx] = addr;
> > > + args->found++;
> > > return args->found == args->cnt ? 1 : 0;
> > > }
> > >
> > > @@ -8017,6 +8025,7 @@ int ftrace_lookup_symbols(const char **sorted_syms, size_t cnt, unsigned long *a
> > > struct kallsyms_data args;
> > > int err;
> > >
> > > + memset(addrs, 0x0, sizeof(*addrs) * cnt);
> > > args.addrs = addrs;
> > > args.syms = sorted_syms;
> > > args.cnt = cnt;
> > > --
> > > 2.35.3
> > >
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH bpf-next 3/3] bpf: Force cookies array to follow symbols sorting
2022-06-03 10:23 ` Jiri Olsa
@ 2022-06-03 21:56 ` Andrii Nakryiko
0 siblings, 0 replies; 20+ messages in thread
From: Andrii Nakryiko @ 2022-06-03 21:56 UTC (permalink / raw)
To: Jiri Olsa
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Networking,
bpf, lkml, Martin KaFai Lau, Song Liu, Yonghong Song,
John Fastabend, KP Singh, Steven Rostedt, Masami Hiramatsu
On Fri, Jun 3, 2022 at 3:23 AM Jiri Olsa <olsajiri@gmail.com> wrote:
>
> On Thu, Jun 02, 2022 at 04:02:28PM -0700, Andrii Nakryiko wrote:
> > On Thu, Jun 2, 2022 at 4:01 PM Andrii Nakryiko
> > <andrii.nakryiko@gmail.com> wrote:
> > >
> > > On Fri, May 27, 2022 at 1:57 PM Jiri Olsa <jolsa@kernel.org> wrote:
> > > >
> > > > When user specifies symbols and cookies for kprobe_multi link
> > > > interface it's very likely the cookies will be misplaced and
> > > > returned to wrong functions (via get_attach_cookie helper).
> > > >
> > > > The reason is that to resolve the provided functions we sort
> > > > them before passing them to ftrace_lookup_symbols, but we do
> > > > not do the same sort on the cookie values.
> > > >
> > > > Fixing this by using sort_r function with custom swap callback
> > > > that swaps cookie values as well.
> > > >
> > > > Fixes: 0236fec57a15 ("bpf: Resolve symbols with ftrace_lookup_symbols for kprobe multi link")
> > > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > > ---
> > > > kernel/trace/bpf_trace.c | 65 ++++++++++++++++++++++++++++++----------
> > > > 1 file changed, 50 insertions(+), 15 deletions(-)
> > > >
> > > > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> > > > index 10b157a6d73e..e5c423b835ab 100644
> > > > --- a/kernel/trace/bpf_trace.c
> > > > +++ b/kernel/trace/bpf_trace.c
> > > > @@ -2423,7 +2423,12 @@ kprobe_multi_link_handler(struct fprobe *fp, unsigned long entry_ip,
> > > > kprobe_multi_link_prog_run(link, entry_ip, regs);
> > > > }
> > > >
> > > > -static int symbols_cmp(const void *a, const void *b)
> > > > +struct multi_symbols_sort {
> > > > + const char **funcs;
> > > > + u64 *cookies;
> > > > +};
> > > > +
> > > > +static int symbols_cmp_r(const void *a, const void *b, const void *priv)
> > > > {
> > > > const char **str_a = (const char **) a;
> > > > const char **str_b = (const char **) b;
> > > > @@ -2431,6 +2436,25 @@ static int symbols_cmp(const void *a, const void *b)
> > > > return strcmp(*str_a, *str_b);
> > > > }
> > > >
> > > > +static void symbols_swap_r(void *a, void *b, int size, const void *priv)
> > > > +{
> > > > + const struct multi_symbols_sort *data = priv;
> > > > + const char **name_a = a, **name_b = b;
> > > > + u64 *cookie_a, *cookie_b;
> > > > +
> > > > + cookie_a = data->cookies + (name_a - data->funcs);
> > > > + cookie_b = data->cookies + (name_b - data->funcs);
> > > > +
> > > > + /* swap name_a/name_b and cookie_a/cookie_b values */
> > > > + swap(*name_a, *name_b);
> > > > + swap(*cookie_a, *cookie_b);
> > > > +}
> > > > +
> > > > +static int symbols_cmp(const void *a, const void *b)
> > > > +{
> > > > + return symbols_cmp_r(a, b, NULL);
> > > > +}
> > > > +
> > > > int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *prog)
> > > > {
> > > > struct bpf_kprobe_multi_link *link = NULL;
> > > > @@ -2468,6 +2492,19 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
> > > > if (!addrs)
> > > > return -ENOMEM;
> > > >
> > > > + ucookies = u64_to_user_ptr(attr->link_create.kprobe_multi.cookies);
> > > > + if (ucookies) {
> > > > + cookies = kvmalloc(size, GFP_KERNEL);
> >
> > oh, and you'll have to rebase anyways after kvmalloc_array patch
>
> true, that kvmalloc_array change went to bpf-next/master,
> but as Song mentioned this patchset should probably go for bpf/master?
>
> I'm fine either way, let me know ;-)
>
I've moved kvmalloc_array() fix to bpf tree (it is an actual fix
against potential overflow after all), so please base everything on
bpf tree.
> thanks,
> jirka
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH bpf-next 2/3] ftrace: Keep address offset in ftrace_lookup_symbols
2022-05-27 20:56 ` [PATCH bpf-next 2/3] ftrace: Keep address offset in ftrace_lookup_symbols Jiri Olsa
` (2 preceding siblings ...)
2022-06-02 22:52 ` Andrii Nakryiko
@ 2022-06-05 16:51 ` Steven Rostedt
2022-06-06 17:56 ` Jiri Olsa
3 siblings, 1 reply; 20+ messages in thread
From: Steven Rostedt @ 2022-06-05 16:51 UTC (permalink / raw)
To: Jiri Olsa
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, netdev,
bpf, lkml, Martin KaFai Lau, Song Liu, Yonghong Song,
John Fastabend, KP Singh, Masami Hiramatsu
On Fri, 27 May 2022 22:56:10 +0200
Jiri Olsa <jolsa@kernel.org> wrote:
> @@ -8017,6 +8025,7 @@ int ftrace_lookup_symbols(const char **sorted_syms, size_t cnt, unsigned long *a
> struct kallsyms_data args;
> int err;
>
> + memset(addrs, 0x0, sizeof(*addrs) * cnt);
Nit, but you don't need the "0x".
-- Steve
> args.addrs = addrs;
> args.syms = sorted_syms;
> args.cnt = cnt;
> --
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH bpf-next 2/3] ftrace: Keep address offset in ftrace_lookup_symbols
2022-05-30 20:20 ` Daniel Borkmann
@ 2022-06-05 16:55 ` Steven Rostedt
0 siblings, 0 replies; 20+ messages in thread
From: Steven Rostedt @ 2022-06-05 16:55 UTC (permalink / raw)
To: Daniel Borkmann
Cc: Jiri Olsa, Alexei Starovoitov, Andrii Nakryiko, netdev, bpf,
lkml, Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Masami Hiramatsu
On Mon, 30 May 2022 22:20:12 +0200
Daniel Borkmann <daniel@iogearbox.net> wrote:
> On 5/27/22 10:56 PM, Jiri Olsa wrote:
> > We want to store the resolved address on the same index as
> > the symbol string, because that's the user (bpf kprobe link)
> > code assumption.
> >
> > Also making sure we don't store duplicates that might be
> > present in kallsyms.
> >
> > Fixes: bed0d9a50dac ("ftrace: Add ftrace_lookup_symbols function")
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> > kernel/trace/ftrace.c | 13 +++++++++++--
> > 1 file changed, 11 insertions(+), 2 deletions(-)
>
> Steven / Masami, would be great to get an Ack from one of you before applying.
I just don't care for the unnecessary "0x" in the memset, but other than that:
Acked-by: Steven Rostedt (Google) <rostedt@goodmis.org>
-- Steve
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH bpf-next 2/3] ftrace: Keep address offset in ftrace_lookup_symbols
2022-06-05 16:51 ` Steven Rostedt
@ 2022-06-06 17:56 ` Jiri Olsa
0 siblings, 0 replies; 20+ messages in thread
From: Jiri Olsa @ 2022-06-06 17:56 UTC (permalink / raw)
To: Steven Rostedt
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, netdev,
bpf, lkml, Martin KaFai Lau, Song Liu, Yonghong Song,
John Fastabend, KP Singh, Masami Hiramatsu
On Sun, Jun 05, 2022 at 12:51:22PM -0400, Steven Rostedt wrote:
> On Fri, 27 May 2022 22:56:10 +0200
> Jiri Olsa <jolsa@kernel.org> wrote:
>
> > @@ -8017,6 +8025,7 @@ int ftrace_lookup_symbols(const char **sorted_syms, size_t cnt, unsigned long *a
> > struct kallsyms_data args;
> > int err;
> >
> > + memset(addrs, 0x0, sizeof(*addrs) * cnt);
>
> Nit, but you don't need the "0x".
ok, will remove
thanks,
jirka
>
> -- Steve
>
> > args.addrs = addrs;
> > args.syms = sorted_syms;
> > args.cnt = cnt;
> > --
^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2022-06-06 17:56 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-27 20:56 [PATCH bpf-next 0/3] bpf: Fix cookie values for kprobe multi Jiri Olsa
2022-05-27 20:56 ` [PATCH bpf-next 1/3] selftests/bpf: Shuffle cookies symbols in kprobe multi test Jiri Olsa
2022-05-30 5:37 ` Song Liu
2022-05-27 20:56 ` [PATCH bpf-next 2/3] ftrace: Keep address offset in ftrace_lookup_symbols Jiri Olsa
2022-05-30 5:37 ` Song Liu
2022-05-30 7:31 ` Jiri Olsa
2022-05-30 20:20 ` Daniel Borkmann
2022-06-05 16:55 ` Steven Rostedt
2022-06-02 22:52 ` Andrii Nakryiko
2022-06-03 10:16 ` Jiri Olsa
2022-06-03 19:12 ` Andrii Nakryiko
2022-06-05 16:51 ` Steven Rostedt
2022-06-06 17:56 ` Jiri Olsa
2022-05-27 20:56 ` [PATCH bpf-next 3/3] bpf: Force cookies array to follow symbols sorting Jiri Olsa
2022-05-30 5:40 ` Song Liu
2022-06-02 23:01 ` Andrii Nakryiko
2022-06-02 23:02 ` Andrii Nakryiko
2022-06-03 10:23 ` Jiri Olsa
2022-06-03 21:56 ` Andrii Nakryiko
2022-06-03 10:18 ` Jiri Olsa
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.