Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1354433 > unrolled thread

[PATCH 1/5] ftrace perf: Check sample types only for sampling events

Started byJiri Olsa <jolsa@kernel.org>
First post2016-03-09 21:50 +0100
Last post2016-03-15 23:00 +0100
Articles 8 — 4 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 1/5] ftrace perf: Check sample types only for sampling events Jiri Olsa <jolsa@kernel.org> - 2016-03-09 21:50 +0100
    Re: [PATCH 1/5] ftrace perf: Check sample types only for sampling  events Namhyung Kim <namhyung@kernel.org> - 2016-03-10 01:40 +0100
      Re: [PATCH 1/5] ftrace perf: Check sample types only for sampling  events Jiri Olsa <jolsa@redhat.com> - 2016-03-10 08:30 +0100
        Re: [PATCH 1/5] ftrace perf: Check sample types only for sampling  events Jiri Olsa <jolsa@redhat.com> - 2016-03-11 09:40 +0100
          Re: [PATCH 1/5] ftrace perf: Check sample types only for sampling  events Namhyung Kim <namhyung@kernel.org> - 2016-03-11 14:50 +0100
            Re: [PATCH 1/5] ftrace perf: Check sample types only for sampling  events Jiri Olsa <jolsa@redhat.com> - 2016-03-11 19:20 +0100
    Re: [PATCH 1/5] ftrace perf: Check sample types only for sampling  events Steven Rostedt <rostedt@goodmis.org> - 2016-03-15 21:10 +0100
      Re: [PATCH 1/5] ftrace perf: Check sample types only for sampling  events Jiri Olsa <jolsa@redhat.com> - 2016-03-15 23:00 +0100

#1354433 — [PATCH 1/5] ftrace perf: Check sample types only for sampling events

FromJiri Olsa <jolsa@kernel.org>
Date2016-03-09 21:50 +0100
Subject[PATCH 1/5] ftrace perf: Check sample types only for sampling events
Message-ID<raTdn-4Ll-1@gated-at.bofh.it>
Currently we check sample type for ftrace:function event
even if it's not created as sampling event. That prevents
creating ftrace_function event in counting mode.

Making sure we check sample types only for sampling events.

Before:
  $ sudo perf stat -e ftrace:function ls
  ...

   Performance counter stats for 'ls':

     <not supported>      ftrace:function

         0.001983662 seconds time elapsed

After:
  $ sudo perf stat -e ftrace:function ls
  ...

   Performance counter stats for 'ls':

              44,498      ftrace:function

         0.037534722 seconds time elapsed

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 kernel/trace/trace_event_perf.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/trace/trace_event_perf.c b/kernel/trace/trace_event_perf.c
index 00df25fd86ef..a7171ec2c1ca 100644
--- a/kernel/trace/trace_event_perf.c
+++ b/kernel/trace/trace_event_perf.c
@@ -52,14 +52,14 @@ static int perf_trace_event_perm(struct trace_event_call *tp_event,
 		 * event, due to issues with page faults while tracing page
 		 * fault handler and its overall trickiness nature.
 		 */
-		if (!p_event->attr.exclude_callchain_user)
+		if (is_sampling_event(p_event) && !p_event->attr.exclude_callchain_user)
 			return -EINVAL;
 
 		/*
 		 * Same reason to disable user stack dump as for user space
 		 * callchains above.
 		 */
-		if (p_event->attr.sample_type & PERF_SAMPLE_STACK_USER)
+		if (is_sampling_event(p_event) && p_event->attr.sample_type & PERF_SAMPLE_STACK_USER)
 			return -EINVAL;
 	}
 
-- 
2.4.3

[toc] | [next] | [standalone]

#1354713 — Re: [PATCH 1/5] ftrace perf: Check sample types only for sampling events

FromNamhyung Kim <namhyung@kernel.org>
Date2016-03-10 01:40 +0100
SubjectRe: [PATCH 1/5] ftrace perf: Check sample types only for sampling events
Message-ID<raWNY-7y1-9@gated-at.bofh.it>
In reply to#1354433
Hi Jiri,

On Wed, Mar 09, 2016 at 09:46:41PM +0100, Jiri Olsa wrote:
> Currently we check sample type for ftrace:function event
> even if it's not created as sampling event. That prevents
> creating ftrace_function event in counting mode.
> 
> Making sure we check sample types only for sampling events.
> 
> Before:
>   $ sudo perf stat -e ftrace:function ls
>   ...
> 
>    Performance counter stats for 'ls':
> 
>      <not supported>      ftrace:function
> 
>          0.001983662 seconds time elapsed
> 
> After:
>   $ sudo perf stat -e ftrace:function ls
>   ...
> 
>    Performance counter stats for 'ls':
> 
>               44,498      ftrace:function
> 
>          0.037534722 seconds time elapsed
> 
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  kernel/trace/trace_event_perf.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/trace/trace_event_perf.c b/kernel/trace/trace_event_perf.c
> index 00df25fd86ef..a7171ec2c1ca 100644
> --- a/kernel/trace/trace_event_perf.c
> +++ b/kernel/trace/trace_event_perf.c
> @@ -52,14 +52,14 @@ static int perf_trace_event_perm(struct trace_event_call *tp_event,
>  		 * event, due to issues with page faults while tracing page
>  		 * fault handler and its overall trickiness nature.
>  		 */
> -		if (!p_event->attr.exclude_callchain_user)
> +		if (is_sampling_event(p_event) && !p_event->attr.exclude_callchain_user)
>  			return -EINVAL;
>  
>  		/*
>  		 * Same reason to disable user stack dump as for user space
>  		 * callchains above.
>  		 */
> -		if (p_event->attr.sample_type & PERF_SAMPLE_STACK_USER)
> +		if (is_sampling_event(p_event) && p_event->attr.sample_type & PERF_SAMPLE_STACK_USER)
>  			return -EINVAL;
>  	}
>  

What about checking is_sampling_event() first and goto the last
paranoid_tracepoint_raw check instead?  This way we can remove the
same check in the function trace case.

Thanks,
Namhyung

[toc] | [prev] | [next] | [standalone]

#1354896 — Re: [PATCH 1/5] ftrace perf: Check sample types only for sampling events

FromJiri Olsa <jolsa@redhat.com>
Date2016-03-10 08:30 +0100
SubjectRe: [PATCH 1/5] ftrace perf: Check sample types only for sampling events
Message-ID<rb3cK-3AY-1@gated-at.bofh.it>
In reply to#1354713
On Thu, Mar 10, 2016 at 09:36:37AM +0900, Namhyung Kim wrote:
> Hi Jiri,
> 
> On Wed, Mar 09, 2016 at 09:46:41PM +0100, Jiri Olsa wrote:
> > Currently we check sample type for ftrace:function event
> > even if it's not created as sampling event. That prevents
> > creating ftrace_function event in counting mode.
> > 
> > Making sure we check sample types only for sampling events.
> > 
> > Before:
> >   $ sudo perf stat -e ftrace:function ls
> >   ...
> > 
> >    Performance counter stats for 'ls':
> > 
> >      <not supported>      ftrace:function
> > 
> >          0.001983662 seconds time elapsed
> > 
> > After:
> >   $ sudo perf stat -e ftrace:function ls
> >   ...
> > 
> >    Performance counter stats for 'ls':
> > 
> >               44,498      ftrace:function
> > 
> >          0.037534722 seconds time elapsed
> > 
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  kernel/trace/trace_event_perf.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/kernel/trace/trace_event_perf.c b/kernel/trace/trace_event_perf.c
> > index 00df25fd86ef..a7171ec2c1ca 100644
> > --- a/kernel/trace/trace_event_perf.c
> > +++ b/kernel/trace/trace_event_perf.c
> > @@ -52,14 +52,14 @@ static int perf_trace_event_perm(struct trace_event_call *tp_event,
> >  		 * event, due to issues with page faults while tracing page
> >  		 * fault handler and its overall trickiness nature.
> >  		 */
> > -		if (!p_event->attr.exclude_callchain_user)
> > +		if (is_sampling_event(p_event) && !p_event->attr.exclude_callchain_user)
> >  			return -EINVAL;
> >  
> >  		/*
> >  		 * Same reason to disable user stack dump as for user space
> >  		 * callchains above.
> >  		 */
> > -		if (p_event->attr.sample_type & PERF_SAMPLE_STACK_USER)
> > +		if (is_sampling_event(p_event) && p_event->attr.sample_type & PERF_SAMPLE_STACK_USER)
> >  			return -EINVAL;
> >  	}
> >  
> 
> What about checking is_sampling_event() first and goto the last
> paranoid_tracepoint_raw check instead?  This way we can remove the
> same check in the function trace case.

right, will check

thanks,
jirka

[toc] | [prev] | [next] | [standalone]

#1355689 — Re: [PATCH 1/5] ftrace perf: Check sample types only for sampling events

FromJiri Olsa <jolsa@redhat.com>
Date2016-03-11 09:40 +0100
SubjectRe: [PATCH 1/5] ftrace perf: Check sample types only for sampling events
Message-ID<rbqM1-3qq-7@gated-at.bofh.it>
In reply to#1354896
On Thu, Mar 10, 2016 at 08:25:02AM +0100, Jiri Olsa wrote:
> On Thu, Mar 10, 2016 at 09:36:37AM +0900, Namhyung Kim wrote:
> > Hi Jiri,
> > 
> > On Wed, Mar 09, 2016 at 09:46:41PM +0100, Jiri Olsa wrote:
> > > Currently we check sample type for ftrace:function event
> > > even if it's not created as sampling event. That prevents
> > > creating ftrace_function event in counting mode.
> > > 
> > > Making sure we check sample types only for sampling events.
> > > 
> > > Before:
> > >   $ sudo perf stat -e ftrace:function ls
> > >   ...
> > > 
> > >    Performance counter stats for 'ls':
> > > 
> > >      <not supported>      ftrace:function
> > > 
> > >          0.001983662 seconds time elapsed
> > > 
> > > After:
> > >   $ sudo perf stat -e ftrace:function ls
> > >   ...
> > > 
> > >    Performance counter stats for 'ls':
> > > 
> > >               44,498      ftrace:function
> > > 
> > >          0.037534722 seconds time elapsed
> > > 
> > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > ---
> > >  kernel/trace/trace_event_perf.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/kernel/trace/trace_event_perf.c b/kernel/trace/trace_event_perf.c
> > > index 00df25fd86ef..a7171ec2c1ca 100644
> > > --- a/kernel/trace/trace_event_perf.c
> > > +++ b/kernel/trace/trace_event_perf.c
> > > @@ -52,14 +52,14 @@ static int perf_trace_event_perm(struct trace_event_call *tp_event,
> > >  		 * event, due to issues with page faults while tracing page
> > >  		 * fault handler and its overall trickiness nature.
> > >  		 */
> > > -		if (!p_event->attr.exclude_callchain_user)
> > > +		if (is_sampling_event(p_event) && !p_event->attr.exclude_callchain_user)
> > >  			return -EINVAL;
> > >  
> > >  		/*
> > >  		 * Same reason to disable user stack dump as for user space
> > >  		 * callchains above.
> > >  		 */
> > > -		if (p_event->attr.sample_type & PERF_SAMPLE_STACK_USER)
> > > +		if (is_sampling_event(p_event) && p_event->attr.sample_type & PERF_SAMPLE_STACK_USER)
> > >  			return -EINVAL;
> > >  	}
> > >  
> > 
> > What about checking is_sampling_event() first and goto the last
> > paranoid_tracepoint_raw check instead?  This way we can remove the
> > same check in the function trace case.
> 
> right, will check

hum, did you mean something like this?

I'd rather keep it the original way.. seems more straight

jirka


---
diff --git a/kernel/trace/trace_event_perf.c b/kernel/trace/trace_event_perf.c
index 00df25fd86ef..7c1edb57c823 100644
--- a/kernel/trace/trace_event_perf.c
+++ b/kernel/trace/trace_event_perf.c
@@ -44,23 +44,22 @@ static int perf_trace_event_perm(struct trace_event_call *tp_event,
 
 	/* The ftrace function trace is allowed only for root. */
 	if (ftrace_event_is_function(tp_event)) {
-		if (perf_paranoid_tracepoint_raw() && !capable(CAP_SYS_ADMIN))
-			return -EPERM;
-
 		/*
 		 * We don't allow user space callchains for  function trace
 		 * event, due to issues with page faults while tracing page
 		 * fault handler and its overall trickiness nature.
 		 */
-		if (!p_event->attr.exclude_callchain_user)
+		if (is_sampling_event(p_event) && !p_event->attr.exclude_callchain_user)
 			return -EINVAL;
 
 		/*
 		 * Same reason to disable user stack dump as for user space
 		 * callchains above.
 		 */
-		if (p_event->attr.sample_type & PERF_SAMPLE_STACK_USER)
+		if (is_sampling_event(p_event) && p_event->attr.sample_type & PERF_SAMPLE_STACK_USER)
 			return -EINVAL;
+
+		goto root_check;
 	}
 
 	/* No tracing, just counting, so no obvious leak */
@@ -73,6 +72,7 @@ static int perf_trace_event_perm(struct trace_event_call *tp_event,
 			return 0;
 	}
 
+root_check:
 	/*
 	 * ...otherwise raw tracepoint data can be a severe data leak,
 	 * only allow root to have these.

[toc] | [prev] | [next] | [standalone]

#1355916 — Re: [PATCH 1/5] ftrace perf: Check sample types only for sampling events

FromNamhyung Kim <namhyung@kernel.org>
Date2016-03-11 14:50 +0100
SubjectRe: [PATCH 1/5] ftrace perf: Check sample types only for sampling events
Message-ID<rbvC3-70o-37@gated-at.bofh.it>
In reply to#1355689
On Fri, Mar 11, 2016 at 09:36:24AM +0100, Jiri Olsa wrote:
> On Thu, Mar 10, 2016 at 08:25:02AM +0100, Jiri Olsa wrote:
> > On Thu, Mar 10, 2016 at 09:36:37AM +0900, Namhyung Kim wrote:
> > > Hi Jiri,
> > > 
> > > On Wed, Mar 09, 2016 at 09:46:41PM +0100, Jiri Olsa wrote:
> > > > Currently we check sample type for ftrace:function event
> > > > even if it's not created as sampling event. That prevents
> > > > creating ftrace_function event in counting mode.
> > > > 
> > > > Making sure we check sample types only for sampling events.
> > > > 
> > > > Before:
> > > >   $ sudo perf stat -e ftrace:function ls
> > > >   ...
> > > > 
> > > >    Performance counter stats for 'ls':
> > > > 
> > > >      <not supported>      ftrace:function
> > > > 
> > > >          0.001983662 seconds time elapsed
> > > > 
> > > > After:
> > > >   $ sudo perf stat -e ftrace:function ls
> > > >   ...
> > > > 
> > > >    Performance counter stats for 'ls':
> > > > 
> > > >               44,498      ftrace:function
> > > > 
> > > >          0.037534722 seconds time elapsed
> > > > 
> > > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > > ---
> > > >  kernel/trace/trace_event_perf.c | 4 ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/kernel/trace/trace_event_perf.c b/kernel/trace/trace_event_perf.c
> > > > index 00df25fd86ef..a7171ec2c1ca 100644
> > > > --- a/kernel/trace/trace_event_perf.c
> > > > +++ b/kernel/trace/trace_event_perf.c
> > > > @@ -52,14 +52,14 @@ static int perf_trace_event_perm(struct trace_event_call *tp_event,
> > > >  		 * event, due to issues with page faults while tracing page
> > > >  		 * fault handler and its overall trickiness nature.
> > > >  		 */
> > > > -		if (!p_event->attr.exclude_callchain_user)
> > > > +		if (is_sampling_event(p_event) && !p_event->attr.exclude_callchain_user)
> > > >  			return -EINVAL;
> > > >  
> > > >  		/*
> > > >  		 * Same reason to disable user stack dump as for user space
> > > >  		 * callchains above.
> > > >  		 */
> > > > -		if (p_event->attr.sample_type & PERF_SAMPLE_STACK_USER)
> > > > +		if (is_sampling_event(p_event) && p_event->attr.sample_type & PERF_SAMPLE_STACK_USER)
> > > >  			return -EINVAL;
> > > >  	}
> > > >  
> > > 
> > > What about checking is_sampling_event() first and goto the last
> > > paranoid_tracepoint_raw check instead?  This way we can remove the
> > > same check in the function trace case.
> > 
> > right, will check
> 
> hum, did you mean something like this?
> 
> I'd rather keep it the original way.. seems more straight

Hmm.. I think I was wrong.  But it seems we can simply return 0 for
non sampling case.  How about this?

Thanks,
Namhyung


diff --git a/kernel/trace/trace_event_perf.c b/kernel/trace/trace_event_perf.c
index 00df25fd86ef..e11108f1d197 100644
--- a/kernel/trace/trace_event_perf.c
+++ b/kernel/trace/trace_event_perf.c
@@ -47,6 +47,9 @@ static int perf_trace_event_perm(struct trace_event_call *tp_event,
 		if (perf_paranoid_tracepoint_raw() && !capable(CAP_SYS_ADMIN))
 			return -EPERM;
 
+		if (!is_sampling_event(p_event))
+			return 0;
+
 		/*
 		 * We don't allow user space callchains for  function trace
 		 * event, due to issues with page faults while tracing page

[toc] | [prev] | [next] | [standalone]

#1356113 — Re: [PATCH 1/5] ftrace perf: Check sample types only for sampling events

FromJiri Olsa <jolsa@redhat.com>
Date2016-03-11 19:20 +0100
SubjectRe: [PATCH 1/5] ftrace perf: Check sample types only for sampling events
Message-ID<rbzPk-1I3-9@gated-at.bofh.it>
In reply to#1355916
On Fri, Mar 11, 2016 at 10:48:14PM +0900, Namhyung Kim wrote:

SNIP

> > > > What about checking is_sampling_event() first and goto the last
> > > > paranoid_tracepoint_raw check instead?  This way we can remove the
> > > > same check in the function trace case.
> > > 
> > > right, will check
> > 
> > hum, did you mean something like this?
> > 
> > I'd rather keep it the original way.. seems more straight
> 
> Hmm.. I think I was wrong.  But it seems we can simply return 0 for
> non sampling case.  How about this?

yep, that seems better.. will post v2

thanks,
jirka

[toc] | [prev] | [next] | [standalone]

#1358222 — Re: [PATCH 1/5] ftrace perf: Check sample types only for sampling events

FromSteven Rostedt <rostedt@goodmis.org>
Date2016-03-15 21:10 +0100
SubjectRe: [PATCH 1/5] ftrace perf: Check sample types only for sampling events
Message-ID<rd3rX-73e-9@gated-at.bofh.it>
In reply to#1354433
On Wed,  9 Mar 2016 21:46:41 +0100
Jiri Olsa <jolsa@kernel.org> wrote:

> Currently we check sample type for ftrace:function event
> even if it's not created as sampling event. That prevents
> creating ftrace_function event in counting mode.
> 
> Making sure we check sample types only for sampling events.
> 
> Before:
>   $ sudo perf stat -e ftrace:function ls
>   ...
> 
>    Performance counter stats for 'ls':
> 
>      <not supported>      ftrace:function
> 
>          0.001983662 seconds time elapsed
> 
> After:
>   $ sudo perf stat -e ftrace:function ls

I'm assuming you gave yourself admin capabilities, and not any normal
user may sample function tracing, right?

-- Steve

[toc] | [prev] | [next] | [standalone]

#1358282 — Re: [PATCH 1/5] ftrace perf: Check sample types only for sampling events

FromJiri Olsa <jolsa@redhat.com>
Date2016-03-15 23:00 +0100
SubjectRe: [PATCH 1/5] ftrace perf: Check sample types only for sampling events
Message-ID<rd5ap-7XV-13@gated-at.bofh.it>
In reply to#1358222
On Tue, Mar 15, 2016 at 04:06:46PM -0400, Steven Rostedt wrote:
> On Wed,  9 Mar 2016 21:46:41 +0100
> Jiri Olsa <jolsa@kernel.org> wrote:
> 
> > Currently we check sample type for ftrace:function event
> > even if it's not created as sampling event. That prevents
> > creating ftrace_function event in counting mode.
> > 
> > Making sure we check sample types only for sampling events.
> > 
> > Before:
> >   $ sudo perf stat -e ftrace:function ls
> >   ...
> > 
> >    Performance counter stats for 'ls':
> > 
> >      <not supported>      ftrace:function
> > 
> >          0.001983662 seconds time elapsed
> > 
> > After:
> >   $ sudo perf stat -e ftrace:function ls
> 
> I'm assuming you gave yourself admin capabilities, and not any normal
> user may sample function tracing, right?

right ;-)

jirka

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web