Re: [PATCH bpf-next 2/8] ftrace: Add support to resolve module symbols in ftrace_lookup_symbols

[Jiri Olsa <olsajiri@gmail.com>]

On Tue, Oct 11, 2022 at 12:05:47AM -0700, Song Liu wrote:
> On Sun, Oct 9, 2022 at 3:00 PM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Currently ftrace_lookup_symbols iterates only over core symbols,
> > adding module_kallsyms_on_each_symbol call to check on modules
> > symbols as well.
> >
> > Also removing 'args.found == args.cnt' condition, because it's
> > already checked in kallsyms_callback function.
> >
> > Also removing 'err < 0' check, because both *kallsyms_on_each_symbol
> > functions do not return error.
> >
> > Reported-by: Martynas Pumputis <m@lambda.lt>
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> 
> Acked-by: Song Liu <song@kernel.org>
> 
> > ---
> >  kernel/trace/ftrace.c | 11 ++++++-----
> >  1 file changed, 6 insertions(+), 5 deletions(-)
> >
> > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> > index 447d2e2a8549..6efdba4666f4 100644
> > --- a/kernel/trace/ftrace.c
> > +++ b/kernel/trace/ftrace.c
> > @@ -8292,17 +8292,18 @@ static int kallsyms_callback(void *data, const char *name,
> >  int ftrace_lookup_symbols(const char **sorted_syms, size_t cnt, unsigned long *addrs)
> >  {
> >         struct kallsyms_data args;
> > -       int err;
> > +       int found_all;
> >
> >         memset(addrs, 0, sizeof(*addrs) * cnt);
> >         args.addrs = addrs;
> >         args.syms = sorted_syms;
> >         args.cnt = cnt;
> >         args.found = 0;
> > -       err = kallsyms_on_each_symbol(kallsyms_callback, &args);
> > -       if (err < 0)
> > -               return err;
> > -       return args.found == args.cnt ? 0 : -ESRCH;
> > +       found_all = kallsyms_on_each_symbol(kallsyms_callback, &args);
> > +       if (found_all)
> > +               return 0;
> > +       found_all = module_kallsyms_on_each_symbol(kallsyms_callback, &args);
> > +       return found_all ? 0 : -ESRCH;
> 
> We probably need some comments about kallsym_callback and
> ftrace_lookup_symbols. It took me a while to confirm the logic for
> found_all.

ok, I wrote some info in the changelog, but I'll put it
as comment in the code as well

jirka

> 
> >  }
> >
> >  #ifdef CONFIG_SYSCTL
> > --
> > 2.37.3
> >