Re: [PATCH 1/2] perf/probe: Report possible permission error for map__load failure

[Arnaldo Carvalho de Melo]

Em Mon, Jun 07, 2021 at 09:00:34AM +0900, Masami Hiramatsu escreveu:
> On Sun, 6 Jun 2021 00:11:31 -0700
> Namhyung Kim <namhyung@xxxxxxxxxx> wrote:
> 
> > Hi Arnaldo and Masami,
> > 
> > On Fri, Jun 4, 2021 at 12:18 PM Arnaldo Carvalho de Melo
> > <acme@xxxxxxxxxx> wrote:
> > >
> > > Em Sat, Jun 05, 2021 at 12:30:58AM +0900, Masami Hiramatsu escreveu:
> > > > Report possible permission error including kptr_restrict setting
> > > > for map__load() failure. This can happen when non-superuser runs
> > > > perf probe.
> > > >
> > > > With this patch, perf probe shows the following message.
> > > >
> > > >  $ perf probe vfs_read
> > > >  Failed to load symbols from /proc/kallsyms
> > > >  Please ensure you can read the /proc/kallsyms symbol addresses.
> > > >  If the /proc/sys/kernel/kptr_restrict is '2', you can not read
> > > >  kernel symbol address even if you are a superuser. Please change
> > > >  it to '1'. If kptr_restrict is '1', the superuser can read the
> > > >  symbol addresses.
> > > >  In that case, please run this command again with sudo.
> > > >    Error: Failed to add events.
> > 
> > Maybe we can read the kptr_restrict file and (e)uid to suggest
> > the specific message for the situation only.
> 
> Agreed, and that should be done in map__load(), since it returns -1
> for any error. Caller needs to estimate the reason.

Then we need to change it to return a errno so that the caller can react
accordingly.

- Arnaldo
 
> If each caller does it, those have to repeat same estimation and
> similar messages in different place. Maybe we need to have a global
> error and hint object so that caller can show it when it detects
> an error.
> 
> Thank you,
> 
> 
> > Thanks,
> > Namhyung
> > 
> > >
> > >
> > > > Signed-off-by: Masami Hiramatsu <mhiramat@xxxxxxxxxx>
> > > > ---
> > > >  tools/perf/util/probe-event.c |   25 ++++++++++++++++++++++---
> > > >  1 file changed, 22 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> > > > index 3a7649835ec9..8fe179d671c3 100644
> > > > --- a/tools/perf/util/probe-event.c
> > > > +++ b/tools/perf/util/probe-event.c
> > > > @@ -2936,7 +2936,7 @@ static int find_probe_functions(struct map *map, char *name,
> > > >       bool cut_version = true;
> > > >
> > > >       if (map__load(map) < 0)
> > > > -             return 0;
> > > > +             return -EACCES; /* Possible permission error to load symbols */
> > > >
> > > >       /* If user gives a version, don't cut off the version from symbols */
> > > >       if (strchr(name, '@'))
> > > > @@ -2975,6 +2975,17 @@ void __weak arch__fix_tev_from_maps(struct perf_probe_event *pev __maybe_unused,
> > > >                               struct map *map __maybe_unused,
> > > >                               struct symbol *sym __maybe_unused) { }
> > > >
> > > > +
> > > > +static void pr_kallsyms_access_error(void)
> > > > +{
> > > > +     pr_err("Please ensure you can read the /proc/kallsyms symbol addresses.\n"
> > > > +            "If the /proc/sys/kernel/kptr_restrict is '2', you can not read\n"
> > > > +            "kernel symbol address even if you are a superuser. Please change\n"
> > > > +            "it to '1'. If kptr_restrict is '1', the superuser can read the\n"
> > > > +            "symbol addresses.\n"
> > > > +            "In that case, please run this command again with sudo.\n");
> > > > +}
> > > > +
> > > >  /*
> > > >   * Find probe function addresses from map.
> > > >   * Return an error or the number of found probe_trace_event
> > > > @@ -3011,8 +3022,16 @@ static int find_probe_trace_events_from_map(struct perf_probe_event *pev,
> > > >        */
> > > >       num_matched_functions = find_probe_functions(map, pp->function, syms);
> > > >       if (num_matched_functions <= 0) {
> > > > -             pr_err("Failed to find symbol %s in %s\n", pp->function,
> > > > -                     pev->target ? : "kernel");
> > > > +             if (num_matched_functions == -EACCES) {
> > > > +                     pr_err("Failed to load symbols from %s\n",
> > > > +                            pev->target ?: "/proc/kallsyms");
> > > > +                     if (pev->target)
> > > > +                             pr_err("Please ensure the file is not stripped.\n");
> > > > +                     else
> > > > +                             pr_kallsyms_access_error();
> > > > +             } else
> > > > +                     pr_err("Failed to find symbol %s in %s\n", pp->function,
> > > > +                             pev->target ? : "kernel");
> > > >               ret = -ENOENT;
> > > >               goto out;
> > > >       } else if (num_matched_functions > probe_conf.max_probes) {
> > > >
> > >
> > > --
> > >
> > > - Arnaldo
> 
> 
> -- 
> Masami Hiramatsu <mhiramat@xxxxxxxxxx>

-- 

- Arnaldo