All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] perf stat: force error in fallback on :k events
@ 2020-04-14 16:15 Ian Rogers
  2020-04-15  9:18 ` Jiri Olsa
                   ` (2 more replies)
  0 siblings, 3 replies; 4+ messages in thread
From: Ian Rogers @ 2020-04-14 16:15 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	linux-kernel
  Cc: Stephane Eranian, Ian Rogers

From: Stephane Eranian <eranian@google.com>

When it is not possible for a non-privilege perf command
to monitor at the kernel level (:k), the fallback code forces
a :u. That works if the event was previously monitoring both levels.
But if the event was already constrained to kernel only, then it does
not make sense to restrict it to user only.
Given the code works by exclusion, a kernel only event would have:
attr->exclude_user = 1
The fallback code would add:
attr->exclude_kernel = 1;

In the end the end would not monitor in either the user level or kernel
level. In other words, it would count nothing.

An event programmed to monitor kernel only cannot be switched to user only
without seriously warning the user.

This patch forces an error in this case to make it clear the request
cannot really be satisfied.

Behavior with paranoid 1:
$ sudo bash -c "echo 1 > /proc/sys/kernel/perf_event_paranoid"
$ perf stat -e cycles:k sleep 1

 Performance counter stats for 'sleep 1':

         1,520,413      cycles:k

       1.002361664 seconds time elapsed

       0.002480000 seconds user
       0.000000000 seconds sys

Old behavior with paranoid 2:
$ sudo bash -c "echo 2 > /proc/sys/kernel/perf_event_paranoid"
$ perf stat -e cycles:k sleep 1
 Performance counter stats for 'sleep 1':

                 0      cycles:ku

       1.002358127 seconds time elapsed

       0.002384000 seconds user
       0.000000000 seconds sys

New behavior with paranoid 2:
$ sudo bash -c "echo 2 > /proc/sys/kernel/perf_event_paranoid"
$ perf stat -e cycles:k sleep 1
Error:
You may not have permission to collect stats.

Consider tweaking /proc/sys/kernel/perf_event_paranoid,
which controls use of the performance events system by
unprivileged users (without CAP_PERFMON or CAP_SYS_ADMIN).

The current value is 2:

  -1: Allow use of (almost) all events by all users
      Ignore mlock limit after perf_event_mlock_kb without CAP_IPC_LOCK
>= 0: Disallow ftrace function tracepoint by users without CAP_PERFMON or CAP_SYS_ADMIN
      Disallow raw tracepoint access by users without CAP_SYS_PERFMON or CAP_SYS_ADMIN
>= 1: Disallow CPU event access by users without CAP_PERFMON or CAP_SYS_ADMIN
>= 2: Disallow kernel profiling by users without CAP_PERFMON or CAP_SYS_ADMIN

To make this setting permanent, edit /etc/sysctl.conf too, e.g.:

        kernel.perf_event_paranoid = -1

v2 of this patch addresses the review feedback from jolsa@redhat.com.

Signed-off-by: Stephane Eranian <eranian@google.com>
Reviewed-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/evsel.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index d23db6755f51..b4e8bcb5ab05 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -2446,6 +2446,10 @@ bool perf_evsel__fallback(struct evsel *evsel, int err,
 		char *new_name;
 		const char *sep = ":";
 
+		/* If event has exclude user then don't exclude kernel. */
+		if (evsel->core.attr.exclude_user)
+			return false;
+
 		/* Is there already the separator in the name. */
 		if (strchr(name, '/') ||
 		    strchr(name, ':'))
-- 
2.26.0.110.g2183baf09c-goog


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH v2] perf stat: force error in fallback on :k events
  2020-04-14 16:15 [PATCH v2] perf stat: force error in fallback on :k events Ian Rogers
@ 2020-04-15  9:18 ` Jiri Olsa
  2020-04-16 15:50 ` Arnaldo Carvalho de Melo
  2020-04-22 12:17 ` [tip: perf/core] perf stat: Force " tip-bot2 for Stephane Eranian
  2 siblings, 0 replies; 4+ messages in thread
From: Jiri Olsa @ 2020-04-15  9:18 UTC (permalink / raw)
  To: Ian Rogers
  Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Namhyung Kim, linux-kernel,
	Stephane Eranian

On Tue, Apr 14, 2020 at 09:15:50AM -0700, Ian Rogers wrote:

SNIP

> New behavior with paranoid 2:
> $ sudo bash -c "echo 2 > /proc/sys/kernel/perf_event_paranoid"
> $ perf stat -e cycles:k sleep 1
> Error:
> You may not have permission to collect stats.
> 
> Consider tweaking /proc/sys/kernel/perf_event_paranoid,
> which controls use of the performance events system by
> unprivileged users (without CAP_PERFMON or CAP_SYS_ADMIN).
> 
> The current value is 2:
> 
>   -1: Allow use of (almost) all events by all users
>       Ignore mlock limit after perf_event_mlock_kb without CAP_IPC_LOCK
> >= 0: Disallow ftrace function tracepoint by users without CAP_PERFMON or CAP_SYS_ADMIN
>       Disallow raw tracepoint access by users without CAP_SYS_PERFMON or CAP_SYS_ADMIN
> >= 1: Disallow CPU event access by users without CAP_PERFMON or CAP_SYS_ADMIN
> >= 2: Disallow kernel profiling by users without CAP_PERFMON or CAP_SYS_ADMIN
> 
> To make this setting permanent, edit /etc/sysctl.conf too, e.g.:
> 
>         kernel.perf_event_paranoid = -1
> 
> v2 of this patch addresses the review feedback from jolsa@redhat.com.
> 
> Signed-off-by: Stephane Eranian <eranian@google.com>
> Reviewed-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/util/evsel.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index d23db6755f51..b4e8bcb5ab05 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -2446,6 +2446,10 @@ bool perf_evsel__fallback(struct evsel *evsel, int err,
>  		char *new_name;
>  		const char *sep = ":";
>  
> +		/* If event has exclude user then don't exclude kernel. */
> +		if (evsel->core.attr.exclude_user)
> +			return false;
> +

nice, it's much simpler

Acked-by: Jiri Olsa <jolsa@redhat.com>

thanks,
jirka

>  		/* Is there already the separator in the name. */
>  		if (strchr(name, '/') ||
>  		    strchr(name, ':'))
> -- 
> 2.26.0.110.g2183baf09c-goog
> 


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v2] perf stat: force error in fallback on :k events
  2020-04-14 16:15 [PATCH v2] perf stat: force error in fallback on :k events Ian Rogers
  2020-04-15  9:18 ` Jiri Olsa
@ 2020-04-16 15:50 ` Arnaldo Carvalho de Melo
  2020-04-22 12:17 ` [tip: perf/core] perf stat: Force " tip-bot2 for Stephane Eranian
  2 siblings, 0 replies; 4+ messages in thread
From: Arnaldo Carvalho de Melo @ 2020-04-16 15:50 UTC (permalink / raw)
  To: Ian Rogers
  Cc: Peter Zijlstra, Ingo Molnar, Mark Rutland, Alexander Shishkin,
	Jiri Olsa, Namhyung Kim, linux-kernel, Stephane Eranian

Em Tue, Apr 14, 2020 at 09:15:50AM -0700, Ian Rogers escreveu:
> From: Stephane Eranian <eranian@google.com>
> 
> When it is not possible for a non-privilege perf command
> to monitor at the kernel level (:k), the fallback code forces
> a :u. That works if the event was previously monitoring both levels.
> But if the event was already constrained to kernel only, then it does
> not make sense to restrict it to user only.
> Given the code works by exclusion, a kernel only event would have:
> attr->exclude_user = 1
> The fallback code would add:
> attr->exclude_kernel = 1;
> 
> In the end the end would not monitor in either the user level or kernel
> level. In other words, it would count nothing.
> 
> An event programmed to monitor kernel only cannot be switched to user only
> without seriously warning the user.
> 
> This patch forces an error in this case to make it clear the request
> cannot really be satisfied.
> 
> Behavior with paranoid 1:
> $ sudo bash -c "echo 1 > /proc/sys/kernel/perf_event_paranoid"
> $ perf stat -e cycles:k sleep 1
> 
>  Performance counter stats for 'sleep 1':
> 
>          1,520,413      cycles:k
> 
>        1.002361664 seconds time elapsed
> 
>        0.002480000 seconds user
>        0.000000000 seconds sys
> 
> Old behavior with paranoid 2:
> $ sudo bash -c "echo 2 > /proc/sys/kernel/perf_event_paranoid"
> $ perf stat -e cycles:k sleep 1
>  Performance counter stats for 'sleep 1':
> 
>                  0      cycles:ku
> 
>        1.002358127 seconds time elapsed
> 
>        0.002384000 seconds user
>        0.000000000 seconds sys
> 
> New behavior with paranoid 2:
> $ sudo bash -c "echo 2 > /proc/sys/kernel/perf_event_paranoid"
> $ perf stat -e cycles:k sleep 1
> Error:
> You may not have permission to collect stats.
> 
> Consider tweaking /proc/sys/kernel/perf_event_paranoid,
> which controls use of the performance events system by
> unprivileged users (without CAP_PERFMON or CAP_SYS_ADMIN).
> 
> The current value is 2:
> 
>   -1: Allow use of (almost) all events by all users
>       Ignore mlock limit after perf_event_mlock_kb without CAP_IPC_LOCK
> >= 0: Disallow ftrace function tracepoint by users without CAP_PERFMON or CAP_SYS_ADMIN
>       Disallow raw tracepoint access by users without CAP_SYS_PERFMON or CAP_SYS_ADMIN
> >= 1: Disallow CPU event access by users without CAP_PERFMON or CAP_SYS_ADMIN
> >= 2: Disallow kernel profiling by users without CAP_PERFMON or CAP_SYS_ADMIN
> 
> To make this setting permanent, edit /etc/sysctl.conf too, e.g.:
> 
>         kernel.perf_event_paranoid = -1
> 
> v2 of this patch addresses the review feedback from jolsa@redhat.com.

Hey, thanks _a lot_ for providing the before/after output, this allows
me to review the code and then just go mechanically follow your steps to
reproduce the results, awesome, keep doing it that way.

And Jiri is right, one-liners are fantastic, this one came close enough
:-)

I did some reflowing of the commit text, check if you're curious when
this goes upstream, no original content changed, just added some spaces,
separator blank lines and moved the examples two spaces to the right.

Thanks,

- Arnaldo
 
> Signed-off-by: Stephane Eranian <eranian@google.com>
> Reviewed-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/util/evsel.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index d23db6755f51..b4e8bcb5ab05 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -2446,6 +2446,10 @@ bool perf_evsel__fallback(struct evsel *evsel, int err,
>  		char *new_name;
>  		const char *sep = ":";
>  
> +		/* If event has exclude user then don't exclude kernel. */
> +		if (evsel->core.attr.exclude_user)
> +			return false;
> +
>  		/* Is there already the separator in the name. */
>  		if (strchr(name, '/') ||
>  		    strchr(name, ':'))
> -- 
> 2.26.0.110.g2183baf09c-goog
> 

-- 

- Arnaldo

^ permalink raw reply	[flat|nested] 4+ messages in thread

* [tip: perf/core] perf stat: Force error in fallback on :k events
  2020-04-14 16:15 [PATCH v2] perf stat: force error in fallback on :k events Ian Rogers
  2020-04-15  9:18 ` Jiri Olsa
  2020-04-16 15:50 ` Arnaldo Carvalho de Melo
@ 2020-04-22 12:17 ` tip-bot2 for Stephane Eranian
  2 siblings, 0 replies; 4+ messages in thread
From: tip-bot2 for Stephane Eranian @ 2020-04-22 12:17 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: Stephane Eranian, Ian Rogers, Jiri Olsa,
	Arnaldo Carvalho de Melo, Alexander Shishkin, Mark Rutland,
	Namhyung Kim, Peter Zijlstra, x86, LKML

The following commit has been merged into the perf/core branch of tip:

Commit-ID:     bec49a9e05db3dbdca696fa07c62c52638fb6371
Gitweb:        https://git.kernel.org/tip/bec49a9e05db3dbdca696fa07c62c52638fb6371
Author:        Stephane Eranian <eranian@google.com>
AuthorDate:    Tue, 14 Apr 2020 09:15:50 -07:00
Committer:     Arnaldo Carvalho de Melo <acme@redhat.com>
CommitterDate: Sat, 18 Apr 2020 09:05:00 -03:00

perf stat: Force error in fallback on :k events

When it is not possible for a non-privilege perf command to monitor at
the kernel level (:k), the fallback code forces a :u. That works if the
event was previously monitoring both levels.  But if the event was
already constrained to kernel only, then it does not make sense to
restrict it to user only.

Given the code works by exclusion, a kernel only event would have:

  attr->exclude_user = 1

The fallback code would add:

  attr->exclude_kernel = 1

In the end the end would not monitor in either the user level or kernel
level. In other words, it would count nothing.

An event programmed to monitor kernel only cannot be switched to user
only without seriously warning the user.

This patch forces an error in this case to make it clear the request
cannot really be satisfied.

Behavior with paranoid 1:

  $ sudo bash -c "echo 1 > /proc/sys/kernel/perf_event_paranoid"
  $ perf stat -e cycles:k sleep 1

   Performance counter stats for 'sleep 1':

           1,520,413      cycles:k

         1.002361664 seconds time elapsed

         0.002480000 seconds user
         0.000000000 seconds sys

Old behavior with paranoid 2:

  $ sudo bash -c "echo 2 > /proc/sys/kernel/perf_event_paranoid"
  $ perf stat -e cycles:k sleep 1
   Performance counter stats for 'sleep 1':

                   0      cycles:ku

         1.002358127 seconds time elapsed

         0.002384000 seconds user
         0.000000000 seconds sys

New behavior with paranoid 2:

  $ sudo bash -c "echo 2 > /proc/sys/kernel/perf_event_paranoid"
  $ perf stat -e cycles:k sleep 1
  Error:
  You may not have permission to collect stats.

  Consider tweaking /proc/sys/kernel/perf_event_paranoid,
  which controls use of the performance events system by
  unprivileged users (without CAP_PERFMON or CAP_SYS_ADMIN).

  The current value is 2:

    -1: Allow use of (almost) all events by all users
        Ignore mlock limit after perf_event_mlock_kb without CAP_IPC_LOCK
  >= 0: Disallow ftrace function tracepoint by users without CAP_PERFMON or CAP_SYS_ADMIN
        Disallow raw tracepoint access by users without CAP_SYS_PERFMON or CAP_SYS_ADMIN
  >= 1: Disallow CPU event access by users without CAP_PERFMON or CAP_SYS_ADMIN
  >= 2: Disallow kernel profiling by users without CAP_PERFMON or CAP_SYS_ADMIN

  To make this setting permanent, edit /etc/sysctl.conf too, e.g.:

          kernel.perf_event_paranoid = -1

v2 of this patch addresses the review feedback from jolsa@redhat.com.

Signed-off-by: Stephane Eranian <eranian@google.com>
Reviewed-by: Ian Rogers <irogers@google.com>
Acked-by: Jiri Olsa <jolsa@redhat.com>
Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Link: http://lore.kernel.org/lkml/20200414161550.225588-1-irogers@google.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/evsel.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 8300e8c..6a571d3 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -2427,6 +2427,10 @@ bool perf_evsel__fallback(struct evsel *evsel, int err,
 		char *new_name;
 		const char *sep = ":";
 
+		/* If event has exclude user then don't exclude kernel. */
+		if (evsel->core.attr.exclude_user)
+			return false;
+
 		/* Is there already the separator in the name. */
 		if (strchr(name, '/') ||
 		    strchr(name, ':'))

^ permalink raw reply related	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2020-04-22 12:22 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-14 16:15 [PATCH v2] perf stat: force error in fallback on :k events Ian Rogers
2020-04-15  9:18 ` Jiri Olsa
2020-04-16 15:50 ` Arnaldo Carvalho de Melo
2020-04-22 12:17 ` [tip: perf/core] perf stat: Force " tip-bot2 for Stephane Eranian

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.