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(-)
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 <[email protected]>
---
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
On Mon, May 30, 2022 at 05:37:49AM +0000, Song Liu wrote:
>
>
> > On May 27, 2022, at 1:56 PM, Jiri Olsa <[email protected]> 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 <[email protected]>
>
> Acked-by: Song Liu <[email protected]>
>
> 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
> On May 27, 2022, at 1:56 PM, Jiri Olsa <[email protected]> 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 <[email protected]>
Acked-by: Song Liu <[email protected]>
BTW, I guess this set should apply to bpf tree?
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 <[email protected]>
> ---
> 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
On Fri, Jun 3, 2022 at 3:16 AM Jiri Olsa <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > ---
> > > 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
> > >
On Fri, May 27, 2022 at 1:56 PM Jiri Olsa <[email protected]> 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 <[email protected]>
> ---
> 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
>
On Thu, Jun 02, 2022 at 03:52:03PM -0700, Andrii Nakryiko wrote:
> On Fri, May 27, 2022 at 1:56 PM Jiri Olsa <[email protected]> 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 <[email protected]>
> > ---
> > 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
> >
On Fri, 27 May 2022 22:56:10 +0200
Jiri Olsa <[email protected]> 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;
> --
On Mon, 30 May 2022 22:20:12 +0200
Daniel Borkmann <[email protected]> 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 <[email protected]>
> > ---
> > 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) <[email protected]>
-- Steve
On Sun, Jun 05, 2022 at 12:51:22PM -0400, Steven Rostedt wrote:
> On Fri, 27 May 2022 22:56:10 +0200
> Jiri Olsa <[email protected]> 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;
> > --