[PATCH bpf-next 0/3] bpf: Fix cookie values for kprobe multi

[PATCH bpf-next 0/3] bpf: Fix cookie values for kprobe multi

[Jiri Olsa]

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(-)

[PATCH bpf-next 1/3] selftests/bpf: Shuffle cookies symbols in kprobe multi test

[Jiri Olsa]

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

Re: [PATCH bpf-next 1/3] selftests/bpf: Shuffle cookies symbols in kprobe multi test

[Song Liu]

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

[PATCH bpf-next 2/3] ftrace: Keep address offset in ftrace_lookup_symbols

[Jiri Olsa]

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

Re: [PATCH bpf-next 2/3] ftrace: Keep address offset in ftrace_lookup_symbols

[Song Liu]

> 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? 

Re: [PATCH bpf-next 2/3] ftrace: Keep address offset in ftrace_lookup_symbols

[Jiri Olsa]

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

Re: [PATCH bpf-next 2/3] ftrace: Keep address offset in ftrace_lookup_symbols

[Daniel Borkmann]

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

Re: [PATCH bpf-next 2/3] ftrace: Keep address offset in ftrace_lookup_symbols

[Steven Rostedt]

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

Re: [PATCH bpf-next 2/3] ftrace: Keep address offset in ftrace_lookup_symbols

[Andrii Nakryiko]

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
>

Re: [PATCH bpf-next 2/3] ftrace: Keep address offset in ftrace_lookup_symbols

[Jiri Olsa]

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

Re: [PATCH bpf-next 2/3] ftrace: Keep address offset in ftrace_lookup_symbols

[Andrii Nakryiko]

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

Re: [PATCH bpf-next 2/3] ftrace: Keep address offset in ftrace_lookup_symbols

[Steven Rostedt]

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

Re: [PATCH bpf-next 2/3] ftrace: Keep address offset in ftrace_lookup_symbols

[Jiri Olsa]

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

[PATCH bpf-next 3/3] bpf: Force cookies array to follow symbols sorting

[Jiri Olsa]

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

Re: [PATCH bpf-next 3/3] bpf: Force cookies array to follow symbols sorting

[Song Liu]

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

Re: [PATCH bpf-next 3/3] bpf: Force cookies array to follow symbols sorting

[Andrii Nakryiko]

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
>

Re: [PATCH bpf-next 3/3] bpf: Force cookies array to follow symbols sorting

[Andrii Nakryiko]

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

Re: [PATCH bpf-next 3/3] bpf: Force cookies array to follow symbols sorting

[Jiri Olsa]

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

Re: [PATCH bpf-next 3/3] bpf: Force cookies array to follow symbols sorting

[Andrii Nakryiko]

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

Re: [PATCH bpf-next 3/3] bpf: Force cookies array to follow symbols sorting

[Jiri Olsa]

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