[PATCH v3] tools lib traceevent: add checks for returned EVENT_ERROR type

[PATCH v3] tools lib traceevent: add checks for returned EVENT_ERROR type

[Dean Nelson]

Running the following perf-stat command on an arm64 system produces the
following result...

  [root@aarch64 ~]# perf stat -e kmem:mm_page_alloc -a sleep 1
    Warning: [kmem:mm_page_alloc] function sizeof not defined
    Warning: Error: expected type 4 but read 0
  Segmentation fault
  [root@aarch64 ~]#

The second warning message and SIGSEGV stem from the issue expressed in the
first warning message, and are the result of ignoring the EVENT_ERROR type
returned back through the call chain.

Dealing with the first warning message is beyond the scope of this patch. But
the second warning is addressed by this patch's first hunk. And the SIGSEGV is
eliminated by its second hunk.

Signed-off-by: Dean Nelson <dnelson@redhat.com>
---
Changes in v3:
  - The desire was that a goto be used, not what I'd done in v2.

Changes in v2:
  - Moved the second hunk's check for EVENT_ERROR to a separate line.

 tools/lib/traceevent/event-parse.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/tools/lib/traceevent/event-parse.c b/tools/lib/traceevent/event-parse.c
index cc25f05..ed765a9 100644
--- a/tools/lib/traceevent/event-parse.c
+++ b/tools/lib/traceevent/event-parse.c
@@ -1680,6 +1680,9 @@ process_cond(struct event_format *event, struct print_arg *top, char **tok)
 	type = process_arg(event, left, &token);
 
  again:
+	if (type == EVENT_ERROR)
+		goto out_free;
+
 	/* Handle other operations in the arguments */
 	if (type == EVENT_OP && strcmp(token, ":") != 0) {
 		type = process_op(event, left, &token);
@@ -1939,6 +1942,12 @@ process_op(struct event_format *event, struct print_arg *arg, char **tok)
 			goto out_warn_free;
 
 		type = process_arg_token(event, right, tok, type);
+		if (type == EVENT_ERROR) {
+			free_arg(right);
+			/* token was freed in process_arg_token() via *tok */
+			token = NULL;
+			goto out_free;
+		}
 
 		if (right->type == PRINT_OP &&
 		    get_op_prio(arg->op.op) < get_op_prio(right->op.op)) {

Re: [PATCH v3] tools lib traceevent: add checks for returned EVENT_ERROR type

[Namhyung Kim]

On Thu, Aug 20, 2015 at 11:16:32AM -0400, Dean Nelson wrote:
> Running the following perf-stat command on an arm64 system produces the
> following result...
> 
>   [root@aarch64 ~]# perf stat -e kmem:mm_page_alloc -a sleep 1
>     Warning: [kmem:mm_page_alloc] function sizeof not defined
>     Warning: Error: expected type 4 but read 0
>   Segmentation fault
>   [root@aarch64 ~]#
> 
> The second warning message and SIGSEGV stem from the issue expressed in the
> first warning message, and are the result of ignoring the EVENT_ERROR type
> returned back through the call chain.
> 
> Dealing with the first warning message is beyond the scope of this patch. But
> the second warning is addressed by this patch's first hunk. And the SIGSEGV is
> eliminated by its second hunk.
> 
> Signed-off-by: Dean Nelson <dnelson@redhat.com>

Acked-by: Namhyung Kim <namhyung@kernel.org>

Thank you for fixing this!
Namhyung


> ---
> Changes in v3:
>   - The desire was that a goto be used, not what I'd done in v2.
> 
> Changes in v2:
>   - Moved the second hunk's check for EVENT_ERROR to a separate line.
> 
>  tools/lib/traceevent/event-parse.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/tools/lib/traceevent/event-parse.c b/tools/lib/traceevent/event-parse.c
> index cc25f05..ed765a9 100644
> --- a/tools/lib/traceevent/event-parse.c
> +++ b/tools/lib/traceevent/event-parse.c
> @@ -1680,6 +1680,9 @@ process_cond(struct event_format *event, struct print_arg *top, char **tok)
>  	type = process_arg(event, left, &token);
>  
>   again:
> +	if (type == EVENT_ERROR)
> +		goto out_free;
> +
>  	/* Handle other operations in the arguments */
>  	if (type == EVENT_OP && strcmp(token, ":") != 0) {
>  		type = process_op(event, left, &token);
> @@ -1939,6 +1942,12 @@ process_op(struct event_format *event, struct print_arg *arg, char **tok)
>  			goto out_warn_free;
>  
>  		type = process_arg_token(event, right, tok, type);
> +		if (type == EVENT_ERROR) {
> +			free_arg(right);
> +			/* token was freed in process_arg_token() via *tok */
> +			token = NULL;
> +			goto out_free;
> +		}
>  
>  		if (right->type == PRINT_OP &&
>  		    get_op_prio(arg->op.op) < get_op_prio(right->op.op)) {

Re: [PATCH v3] tools lib traceevent: add checks for returned EVENT_ERROR type

[Steven Rostedt]

On Thu, 20 Aug 2015 11:16:32 -0400
Dean Nelson <dnelson@redhat.com> wrote:

> Running the following perf-stat command on an arm64 system produces the
> following result...
> 
>   [root@aarch64 ~]# perf stat -e kmem:mm_page_alloc -a sleep 1
>     Warning: [kmem:mm_page_alloc] function sizeof not defined
>     Warning: Error: expected type 4 but read 0
>   Segmentation fault
>   [root@aarch64 ~]#
> 
> The second warning message and SIGSEGV stem from the issue expressed in the
> first warning message, and are the result of ignoring the EVENT_ERROR type
> returned back through the call chain.
> 
> Dealing with the first warning message is beyond the scope of this patch. But
> the second warning is addressed by this patch's first hunk. And the SIGSEGV is
> eliminated by its second hunk.

Patch looks fine, but this change log is lacking. I don't think you
need to resend though. But Arnaldo, can you add more to this change log
to describe the following, and that's only if I got it right ;-) If I
didn't get it right, then the change log definitely needs to be
explained better.

====
The second warning was a result of the first warning not stopping
processing after it detected the issue. That is, code that found the
issue reported the first problem, but because it did not exit out of
the functions smoothly, it caused the other warning to appear and not
only that, it later caused the SIGSEGV.
====

-- Steve

Other than that...

Acked-by: Steven Rostedt <rostedt@goodmis.org.



> 
> Signed-off-by: Dean Nelson <dnelson@redhat.com>

Re: [PATCH v3] tools lib traceevent: add checks for returned EVENT_ERROR type

[Dean Nelson]

On 08/20/2015 12:05 PM, Steven Rostedt wrote:
> On Thu, 20 Aug 2015 11:16:32 -0400
> Dean Nelson <dnelson@redhat.com> wrote:
>
>> Running the following perf-stat command on an arm64 system produces the
>> following result...
>>
>>    [root@aarch64 ~]# perf stat -e kmem:mm_page_alloc -a sleep 1
>>      Warning: [kmem:mm_page_alloc] function sizeof not defined
>>      Warning: Error: expected type 4 but read 0
>>    Segmentation fault
>>    [root@aarch64 ~]#
>>
>> The second warning message and SIGSEGV stem from the issue expressed in the
>> first warning message, and are the result of ignoring the EVENT_ERROR type
>> returned back through the call chain.
>>
>> Dealing with the first warning message is beyond the scope of this patch. But
>> the second warning is addressed by this patch's first hunk. And the SIGSEGV is
>> eliminated by its second hunk.
>
> Patch looks fine, but this change log is lacking. I don't think you
> need to resend though. But Arnaldo, can you add more to this change log
> to describe the following, and that's only if I got it right ;-) If I
> didn't get it right, then the change log definitely needs to be
> explained better.

No you definitely got it right.

I thought that was what I was saying by the paragraph beginning with
"The second warning...", with the notion that the 2nd warning and
SIGSEGV "stem from" the 1st warning. And that the latter two issues "are
the result of ignoring the EVENT_ERROR" encountered by the 1st
warning's issue.

At least that is what that paragraph was intended to be all about.
Obviously I failed to communicate.

Yours is clear to me. So why not just replace my poorly done paragraph
with your good  paragraph...


> ====
> The second warning was a result of the first warning not stopping
> processing after it detected the issue. That is, code that found the
> issue reported the first problem, but because it did not exit out of
> the functions smoothly, it caused the other warning to appear and not
> only that, it later caused the SIGSEGV.
> ====

Thanks for the review.


>
> -- Steve
>
> Other than that...
>
> Acked-by: Steven Rostedt <rostedt@goodmis.org.
>
>
>
>>
>> Signed-off-by: Dean Nelson <dnelson@redhat.com>

Re: [PATCH v3] tools lib traceevent: add checks for returned EVENT_ERROR type

[Arnaldo Carvalho de Melo]

Em Thu, Aug 20, 2015 at 12:56:25PM -0500, Dean Nelson escreveu:
> On 08/20/2015 12:05 PM, Steven Rostedt wrote:
> >On Thu, 20 Aug 2015 11:16:32 -0400
> >Dean Nelson <dnelson@redhat.com> wrote:
> >
> >>Running the following perf-stat command on an arm64 system produces the
> >>following result...
> >>
> >>   [root@aarch64 ~]# perf stat -e kmem:mm_page_alloc -a sleep 1
> >>     Warning: [kmem:mm_page_alloc] function sizeof not defined
> >>     Warning: Error: expected type 4 but read 0
> >>   Segmentation fault
> >>   [root@aarch64 ~]#
> >>
> >>The second warning message and SIGSEGV stem from the issue expressed in the
> >>first warning message, and are the result of ignoring the EVENT_ERROR type
> >>returned back through the call chain.
> >>
> >>Dealing with the first warning message is beyond the scope of this patch. But
> >>the second warning is addressed by this patch's first hunk. And the SIGSEGV is
> >>eliminated by its second hunk.
> >
> >Patch looks fine, but this change log is lacking. I don't think you
> >need to resend though. But Arnaldo, can you add more to this change log
> >to describe the following, and that's only if I got it right ;-) If I
> >didn't get it right, then the change log definitely needs to be
> >explained better.
> 
> No you definitely got it right.
> 
> I thought that was what I was saying by the paragraph beginning with
> "The second warning...", with the notion that the 2nd warning and
> SIGSEGV "stem from" the 1st warning. And that the latter two issues "are
> the result of ignoring the EVENT_ERROR" encountered by the 1st
> warning's issue.
> 
> At least that is what that paragraph was intended to be all about.
> Obviously I failed to communicate.
> 
> Yours is clear to me. So why not just replace my poorly done paragraph
> with your good  paragraph...
> 
> 
> >====
> >The second warning was a result of the first warning not stopping
> >processing after it detected the issue. That is, code that found the
> >issue reported the first problem, but because it did not exit out of
> >the functions smoothly, it caused the other warning to appear and not
> >only that, it later caused the SIGSEGV.
> >====
> 
> Thanks for the review.

Ok, so I'll use Steven's text and will stick a Reviewed-by: Steven, ack?

- Arnaldo

Re: [PATCH v3] tools lib traceevent: add checks for returned EVENT_ERROR type

[Steven Rostedt]

On Thu, 20 Aug 2015 16:05:18 -0300
Arnaldo Carvalho de Melo <acme@kernel.org> wrote:

> Ok, so I'll use Steven's text and will stick a Reviewed-by: Steven, ack?

Sure.

-- Steve

Re: [PATCH v3] tools lib traceevent: add checks for returned EVENT_ERROR type

[Arnaldo Carvalho dn Melo]

Em Thu, Aug 20, 2015 at 11:16:32AM -0400, Dean Nelson escreveu:
> Running the following perf-stat command on an arm64 system produces the
> following result...
 
>   [root@aarch64 ~]# perf stat -e kmem:mm_page_alloc -a sleep 1
>     Warning: [kmem:mm_page_alloc] function sizeof not defined
>     Warning: Error: expected type 4 but read 0
>   Segmentation fault
>   [root@aarch64 ~]#

Jiri, while testing this I noticed that when running as !root I get:

  $ perf stat -e kmem:mm_page_alloc -a sleep 1
  event syntax error: 'kmem:mm_page_alloc'
                       \___ unknown tracepoint
  Run 'perf list' for a list of valid events

 usage: perf stat [<options>] [<command>]

      -e, --event <event>   event selector. use 'perf list' to list available events
  [acme@zoo linux]$ 

Which could be a bit more friendly if it noticed that it may be a
tracepoint event and that if that is the case, the user needs to have
tracefs remounted with suitable permissions to be able to use
tracepoints, something like 'perf trace' does:

  [acme@zoo linux]$ trace ls
  Error:	No permissions to read
  /sys/kernel/debug/tracing/events/raw_syscalls/sys_(enter|exit)
  Hint:	Try 'sudo mount -o remount,mode=755 /sys/kernel/debug'

  [acme@zoo linux]$

Take that as a suggestion as you're more familiar with the event parsing
code :-)

- Arnaldo

Re: [PATCH v3] tools lib traceevent: add checks for returned EVENT_ERROR type

[Jiri Olsa]

On Fri, Aug 21, 2015 at 10:34:41AM -0300, Arnaldo Carvalho dn Melo wrote:
> Em Thu, Aug 20, 2015 at 11:16:32AM -0400, Dean Nelson escreveu:
> > Running the following perf-stat command on an arm64 system produces the
> > following result...
>  
> >   [root@aarch64 ~]# perf stat -e kmem:mm_page_alloc -a sleep 1
> >     Warning: [kmem:mm_page_alloc] function sizeof not defined
> >     Warning: Error: expected type 4 but read 0
> >   Segmentation fault
> >   [root@aarch64 ~]#
> 
> Jiri, while testing this I noticed that when running as !root I get:
> 
>   $ perf stat -e kmem:mm_page_alloc -a sleep 1
>   event syntax error: 'kmem:mm_page_alloc'
>                        \___ unknown tracepoint
>   Run 'perf list' for a list of valid events

yep, we need to hook it in.. I recall we discussed this already ;-)
so would something like this be ok:

  $ perf stat -e kmem:mm_page_alloc -a sleep 1
  event syntax error: 'kmem:mm_page_alloc'
                       \___ No permissions to read
  /sys/kernel/debug/tracing/events/raw_syscalls/sys_(enter|exit)
  Hint:       Try 'sudo mount -o remount,mode=755 /sys/kernel/debug'
  Run 'perf list' for a list of valid events

jirka

[tip:perf/core] tools lib traceevent: Add checks for returned EVENT_ERROR type

[tip-bot for Dean Nelson]

Commit-ID:  6f56e9cf581c6cedcaea3eb69444b169867ccf3d
Gitweb:     http://git.kernel.org/tip/6f56e9cf581c6cedcaea3eb69444b169867ccf3d
Author:     Dean Nelson <dnelson@redhat.com>
AuthorDate: Thu, 20 Aug 2015 11:16:32 -0400
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Fri, 21 Aug 2015 10:35:09 -0300

tools lib traceevent: Add checks for returned EVENT_ERROR type

Running the following perf-stat command on an arm64 system produces the
following result...

  [root@aarch64 ~]# perf stat -e kmem:mm_page_alloc -a sleep 1
    Warning: [kmem:mm_page_alloc] function sizeof not defined
    Warning: Error: expected type 4 but read 0
  Segmentation fault
  [root@aarch64 ~]#

The second warning was a result of the first warning not stopping
processing after it detected the issue.

That is, code that found the issue reported the first problem, but
because it did not exit out of the functions smoothly, it caused the
other warning to appear and not only that, it later caused the SIGSEGV.

Signed-off-by: Dean Nelson <dnelson@redhat.com>
Reviewed-by: Steven Rostedt <rostedt@goodmis.org>
Acked-by: Namhyung Kim <namhyung@kernel.org>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/20150820151632.13927.13791.email-sent-by-dnelson@teal
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/lib/traceevent/event-parse.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/tools/lib/traceevent/event-parse.c b/tools/lib/traceevent/event-parse.c
index fcd8a9e..5c1867a 100644
--- a/tools/lib/traceevent/event-parse.c
+++ b/tools/lib/traceevent/event-parse.c
@@ -1745,6 +1745,9 @@ process_cond(struct event_format *event, struct print_arg *top, char **tok)
 	type = process_arg(event, left, &token);
 
  again:
+	if (type == EVENT_ERROR)
+		goto out_free;
+
 	/* Handle other operations in the arguments */
 	if (type == EVENT_OP && strcmp(token, ":") != 0) {
 		type = process_op(event, left, &token);
@@ -2004,6 +2007,12 @@ process_op(struct event_format *event, struct print_arg *arg, char **tok)
 			goto out_warn_free;
 
 		type = process_arg_token(event, right, tok, type);
+		if (type == EVENT_ERROR) {
+			free_arg(right);
+			/* token was freed in process_arg_token() via *tok */
+			token = NULL;
+			goto out_free;
+		}
 
 		if (right->type == PRINT_OP &&
 		    get_op_prio(arg->op.op) < get_op_prio(right->op.op)) {