Message-ID: <CAP-5=fVXvus+7PMO=WU+V3CnVNSboGgwS5EcPgXhJKb5MtRAWw@mail.gmail.com>
Date: Tue, 22 Oct 2024 09:34:59 -0700
From: Ian Rogers <irogers@...gle.com>
To: Adrian Hunter <adrian.hunter@...el.com>
Cc: Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>, 
	Arnaldo Carvalho de Melo <acme@...nel.org>, Namhyung Kim <namhyung@...nel.org>, 
	Mark Rutland <mark.rutland@....com>, 
	Alexander Shishkin <alexander.shishkin@...ux.intel.com>, Jiri Olsa <jolsa@...nel.org>, 
	Kan Liang <kan.liang@...ux.intel.com>, James Clark <james.clark@....com>, 
	Oliver Upton <oliver.upton@...ux.dev>, Leo Yan <leo.yan@...ux.dev>, 
	Changbin Du <changbin.du@...wei.com>, Athira Rajeev <atrajeev@...ux.vnet.ibm.com>, 
	linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] perf cap: Tidy up and improve capability testing

On Tue, Oct 22, 2024 at 6:56 AM Adrian Hunter <adrian.hunter@...el.com> wrote:
>
> On 7/08/24 01:06, Ian Rogers wrote:
> > Remove dependence on libcap. libcap is only used to query whether a
> > capability is supported, which is just 1 capget system call.
> >
> > If the capget system call fails, fall back on root permission
> > checking. Previously if libcap fails then the permission is assumed
> > not present which may be pessimistic/wrong.
> >
> > Add a used_root out argument to perf_cap__capable to say whether the
> > fall back root check was used. This allows the correct error message,
> > "root" vs "users with the CAP_PERFMON or CAP_SYS_ADMIN capability", to
> > be selected.
> >
> > Tidy uses of perf_cap__capable so that tests aren't repeated if capget
> > isn't supported.
> >
> > Signed-off-by: Ian Rogers <irogers@...gle.com>
>
> I find I need to revert this.  E.g.
>
> $ install-perf.sh
> $ perf record -e intel_pt//k uname
> WARNING: Kernel address maps (/proc/{kallsyms,modules}) are restricted,
> check /proc/sys/kernel/kptr_restrict and
> /proc/sys/kernel/perf_event_paranoid.
>
> Samples in kernel functions may not be resolved if a suitable vmlinux
> file is not found in the buildid cache or in the vmlinux path.
>
> Samples in kernel modules won't be resolved at all.
>
> If some relocation was applied (e.g. kexec) symbols may be misresolved
> even with a suitable vmlinux or kallsyms file.
>
> Couldn't record kernel reference relocation symbol
> Symbol resolution may be skewed if relocation was used (e.g. kexec).
> Check /proc/kallsyms permission or run as root.
> Linux
> [ perf record: Woken up 1 times to write data ]
> [ perf record: Captured and wrote 0.061 MB perf.data ]
> $ git revert e25ebda78e230283bf707ae3e9655270ff40a7f9
> Auto-merging tools/perf/Makefile.config
> Auto-merging tools/perf/builtin-ftrace.c
> Auto-merging tools/perf/util/Build
> Auto-merging tools/perf/util/symbol.c
> [master 752707588a700] Revert "perf cap: Tidy up and improve capability
> testing"
>  7 files changed, 70 insertions(+), 73 deletions(-)
> $ install-perf.sh
> $ perf record -e intel_pt//k uname
> Linux
> [ perf record: Woken up 1 times to write data ]
> [ perf record: Captured and wrote 0.065 MB perf.data ]

Sorry for this. I couldn't immediately see a code path that should
impact `perf record` and the reproducer works for me without the
revert. Running your command under gdb looking for calls to
perf_cap__capable:

1)
```
#0  0x00005555557f7abe in symbol__read_kptr_restrict () at util/symbol.c:2429
#1  0x00005555557f759f in symbol__init (env=0x0) at util/symbol.c:2523
#2  0x00005555555e7ca3 in cmd_record (argc=1, argv=0x7fffffffe390) at
builtin-record.c:4114
#3  0x000055555569775f in run_builtin (p=0x5555560bda50
<commands+288>, argc=4,
   argv=0x7fffffffe390) at perf.c:351
#4  0x0000555555696e13 in handle_internal_command (argc=4,
argv=0x7fffffffe390) at perf.c:404
#5  0x000055555569762f in run_argv (argcp=0x7fffffffe1dc,
argv=0x7fffffffe1d0) at perf.c:448
#6  0x0000555555696a74 in main (argc=4, argv=0x7fffffffe390) at perf.c:562
```
https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/builtin-record.c?h=perf-tools-next#n4108
```
/*
* Allow aliases to facilitate the lookup of symbols for address
* filters. Refer to auxtrace_parse_filters().
*/
symbol_conf.allow_aliases = true;

symbol__init(NULL);
```
I don't think this is the cause but it seems unfortunate we're
enabling symbols in perf record unconditionally.

2)
```
#0  perf_cap__capable (cap=21, used_root=0x7fffffffbd87) at util/cap.c:22
#1  0x0000555555878236 in perf_event_paranoid_check (max_level=-1) at
util/util.c:330
#2  0x00005555559574ba in intel_pt_recording_options
(itr=0x55555610ada0, evlist=0x5555560f36f0,
   opts=0x5555560b4818 <record+320>) at arch/x86/util/intel-pt.c:619
#3  0x00005555558a46ad in auxtrace_record__options
(itr=0x55555610ada0, evlist=0x5555560f36f0,
   opts=0x5555560b4818 <record+320>) at util/auxtrace.c:619
#4  0x00005555555e8139 in cmd_record (argc=1, argv=0x7fffffffe390) at
builtin-record.c:4206
#5  0x000055555569775f in run_builtin (p=0x5555560bda50
<commands+288>, argc=4,
   argv=0x7fffffffe390) at perf.c:351
#6  0x0000555555696e13 in handle_internal_command (argc=4,
argv=0x7fffffffe390) at perf.c:404
#7  0x000055555569762f in run_argv (argcp=0x7fffffffe1dc,
argv=0x7fffffffe1d0) at perf.c:448
#8  0x0000555555696a74 in main (argc=4, argv=0x7fffffffe390) at perf.c:562
```
So this looks like the cause.

Would it be possible for you to trace (strace/gdb/perf trace) what's
happening in perf_cap__capable? I suspect the system call capget and
its interpretation are the source of the problem, but I can't eyeball
an issue in:
https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/cap.c?h=perf-tools-next#n53

I'd prefer a fix to a revert but I'll leave that to others to decide.

Thanks,
Ian

Powered by blists - more mailing lists