| Message ID | 20220527205611.655282-3-jolsa@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | bpf: Fix cookie values for kprobe multi | expand |
> 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?
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
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
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 >
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 > >
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 > > >
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; > --
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
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; > > --
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;
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(-)