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 <[email protected]>
---
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)) {
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 <[email protected]>
Acked-by: Namhyung Kim <[email protected]>
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)) {
On Thu, 20 Aug 2015 11:16:32 -0400
Dean Nelson <[email protected]> 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 <[email protected].
>
> Signed-off-by: Dean Nelson <[email protected]>
On 08/20/2015 12:05 PM, Steven Rostedt wrote:
> On Thu, 20 Aug 2015 11:16:32 -0400
> Dean Nelson <[email protected]> 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 <[email protected].
>
>
>
>>
>> Signed-off-by: Dean Nelson <[email protected]>
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 <[email protected]> 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
On Thu, 20 Aug 2015 16:05:18 -0300
Arnaldo Carvalho de Melo <[email protected]> wrote:
> Ok, so I'll use Steven's text and will stick a Reviewed-by: Steven, ack?
Sure.
-- Steve
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
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
Commit-ID: 6f56e9cf581c6cedcaea3eb69444b169867ccf3d
Gitweb: http://git.kernel.org/tip/6f56e9cf581c6cedcaea3eb69444b169867ccf3d
Author: Dean Nelson <[email protected]>
AuthorDate: Thu, 20 Aug 2015 11:16:32 -0400
Committer: Arnaldo Carvalho de Melo <[email protected]>
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 <[email protected]>
Reviewed-by: Steven Rostedt <[email protected]>
Acked-by: Namhyung Kim <[email protected]>
Cc: Jiri Olsa <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Link: http://lkml.kernel.org/r/20150820151632.13927.13791.email-sent-by-dnelson@teal
Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
---
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)) {