[PATCH 1/2] perf tools: Fix fault in error patch of intel_pt_process_auxtrace_info()

[PATCH 1/2] perf tools: Fix fault in error patch of intel_pt_process_auxtrace_info()

[Wang Nan]

In error processing path of intel_pt_process_auxtrace_info() it calls
thread__zput() to clean and free pt->unknown_thread which is created by
thread__new(). However, when error raise, a segfault happen:

 # perf script -F event,comm,pid,tid,time,addr,ip,sym,dso,iregs
 Samples for 'instructions:u' event do not have IREGS attribute set. Cannot print 'iregs' field.
 intel_pt_synth_events: failed to synthesize 'instructions' event type
 Segmentation fault (core dumped)

The problem is: there's a union in 'struct thread' combines a list_head
and a rb_node. The standard life cycle of a thread is: init rb_node during
creating, inserted into machine->threads rbtree uses rb_node, move to
machine->dead_threads using list_head, clean by thread__put:
list_del_init(&thread->node).

In the above command, it clean a thread before adding it into list,
causes the above segfault.

This patch gives a fake list_head and link the thread into it before
calling thread__zput(), get rid of the segfault.

After this patch:
 # perf script -F event,comm,pid,tid,time,addr,ip,sym,dso,iregs
 Samples for 'instructions:u' event do not have IREGS attribute set. Cannot print 'iregs' field.
 intel_pt_synth_events: failed to synthesize 'instructions' event type
 0x248 [0x88]: failed to process type: 70

Reported-by: Tong Zhang <ztong@vt.edu>
Signed-off-by: Wang Nan <wangnan0@huawei.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
---
 tools/perf/util/intel-pt.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tools/perf/util/intel-pt.c b/tools/perf/util/intel-pt.c
index 81a2eb7..e2add63 100644
--- a/tools/perf/util/intel-pt.c
+++ b/tools/perf/util/intel-pt.c
@@ -2013,6 +2013,7 @@ int intel_pt_process_auxtrace_info(union perf_event *event,
 	struct auxtrace_info_event *auxtrace_info = &event->auxtrace_info;
 	size_t min_sz = sizeof(u64) * INTEL_PT_PER_CPU_MMAPS;
 	struct intel_pt *pt;
+	struct list_head dead_thread;
 	int err;
 
 	if (auxtrace_info->header.size < sizeof(struct auxtrace_info_event) +
@@ -2153,6 +2154,9 @@ int intel_pt_process_auxtrace_info(union perf_event *event,
 	return 0;
 
 err_delete_thread:
+	RB_CLEAR_NODE(&pt->unknown_thread->rb_node);
+	INIT_LIST_HEAD(&dead_thread);
+	list_add(&pt->unknown_thread->node, &dead_thread);
 	thread__zput(pt->unknown_thread);
 err_free_queues:
 	intel_pt_log_disable();
-- 
1.8.3.4

[PATCH 2/2] perf tools: Fix fault in tracepoint_error if NULL is passed to parse_event

[Wang Nan]

Following segfault can happen with a non-root user:

 $ ./perf record -I -e intel_pt/tsc=1,noretcomp=1/u /bin/ls
 WARNING: Kernel address maps (/proc/{kallsyms,modules}) are restricted,
 check /proc/sys/kernel/kptr_restrict.

 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.

 Segmentation fault (core dumped)

The error is in tracepoint_error: it assumes 'e' is valid.

However, there are many situation a parse_event can be called without
parse_events_error. See result of
'grep 'parse_events(.*NULL)' ./tools/perf/ -r'.

This patch makes tracepoint_error() directly return when !e.

Signed-off-by: Wang Nan <wangnan0@huawei.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Tong Zhang <ztong@vt.edu>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
---
 tools/perf/util/parse-events.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 4f7b0ef..813d9b2 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -399,6 +399,9 @@ static void tracepoint_error(struct parse_events_error *e, int err,
 {
 	char help[BUFSIZ];
 
+	if (!e)
+		return;
+
 	/*
 	 * We get error directly from syscall errno ( > 0),
 	 * or from encoded pointer's error ( < 0).
-- 
1.8.3.4

Re: [PATCH 2/2] perf tools: Fix fault in tracepoint_error if NULL is passed to parse_event

[Adrian Hunter]

On 01/02/16 05:21, Wang Nan wrote:
> Following segfault can happen with a non-root user:
> 
>  $ ./perf record -I -e intel_pt/tsc=1,noretcomp=1/u /bin/ls
>  WARNING: Kernel address maps (/proc/{kallsyms,modules}) are restricted,
>  check /proc/sys/kernel/kptr_restrict.
> 
>  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.
> 
>  Segmentation fault (core dumped)
> 
> The error is in tracepoint_error: it assumes 'e' is valid.
> 
> However, there are many situation a parse_event can be called without
> parse_events_error. See result of
> 'grep 'parse_events(.*NULL)' ./tools/perf/ -r'.
> 
> This patch makes tracepoint_error() directly return when !e.

I sent the same fix here:

	http://marc.info/?l=linux-kernel&m=145381056111871

> 
> Signed-off-by: Wang Nan <wangnan0@huawei.com>
> Cc: Adrian Hunter <adrian.hunter@intel.com>
> Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
> Cc: Tong Zhang <ztong@vt.edu>
> Cc: Josh Poimboeuf <jpoimboe@redhat.com>
> ---
>  tools/perf/util/parse-events.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index 4f7b0ef..813d9b2 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -399,6 +399,9 @@ static void tracepoint_error(struct parse_events_error *e, int err,
>  {
>  	char help[BUFSIZ];
>  
> +	if (!e)
> +		return;
> +
>  	/*
>  	 * We get error directly from syscall errno ( > 0),
>  	 * or from encoded pointer's error ( < 0).
> 

Re: [PATCH 2/2] perf tools: Fix fault in tracepoint_error if NULL is passed to parse_event

[Arnaldo Carvalho de Melo]

Em Mon, Feb 01, 2016 at 10:53:29AM +0200, Adrian Hunter escreveu:
> On 01/02/16 05:21, Wang Nan wrote:
> > Following segfault can happen with a non-root user:
> > 
> >  $ ./perf record -I -e intel_pt/tsc=1,noretcomp=1/u /bin/ls
> >  WARNING: Kernel address maps (/proc/{kallsyms,modules}) are restricted,
> >  check /proc/sys/kernel/kptr_restrict.
> > 
> >  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.
> > 
> >  Segmentation fault (core dumped)
> > 
> > The error is in tracepoint_error: it assumes 'e' is valid.
> > 
> > However, there are many situation a parse_event can be called without
> > parse_events_error. See result of
> > 'grep 'parse_events(.*NULL)' ./tools/perf/ -r'.
> > 
> > This patch makes tracepoint_error() directly return when !e.
> 
> I sent the same fix here:
> 
> 	http://marc.info/?l=linux-kernel&m=145381056111871

Yeah, I couldn't reproduce it, but we narrowed that down to: machine
with Intel PT, without perf_event_attr.context_switch, non-root user,
i.e. user can't access the debugfs events info, rebooted my new machine
with:

[root@jouet ~]# uname -r
4.2.3-300.fc23.x86_64

And, as root, all works because it can read the debugfs events info, to
get the "sched:sched_switch" infoa:

[root@jouet ~]# perf record -I -e intel_pt/tsc=1,noretcomp=1/u /bin/ls
0  a  anaconda-ks.cfg  bin  GBPCEFwr64.tar-from-deb  perf.data
perf.data.old  perf-f23-bringup.todo
[ perf record: Woken up 1 times to write data ]
[ perf record: Captured and wrote 0.214 MB perf.data ]
[root@jouet ~]# perf evlist
intel_pt/tsc=1,noretcomp=1/u
sched:sched_switch
dummy:u
# Tip: use 'perf evlist --trace-fields' to show fields for tracepoint
# events
[root@jouet ~]#

But not as a non priledged user:

(gdb) run record -I -e intel_pt/tsc=1,noretcomp=1/u /bin/ls
Starting program:  record -I -e intel_pt/tsc=1,noretcomp=1/u /bin/ls
No executable file specified.
Use the "file" or "exec-file" command.
(gdb) file perf
Reading symbols from perf...done.
(gdb) run record -I -e intel_pt/tsc=1,noretcomp=1/u /bin/ls
Starting program: /home/acme/bin/perf record -I -e intel_pt/tsc=1,noretcomp=1/u /bin/ls
Missing separate debuginfos, use: dnf debuginfo-install glibc-2.22-7.fc23.x86_64
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib64/libthread_db.so.1".

Program received signal SIGSEGV, Segmentation fault.
0x00000000004b9ea5 in tracepoint_error (e=0x0, err=13, sys=0x19b1370 "sched", name=0x19a5d00 "sched_switch") at util/parse-events.c:410
410			e->str = strdup("can't access trace events");
Missing separate debuginfos, use: dnf debuginfo-install audit-libs-2.4.5-1.fc23.x86_64 bzip2-libs-1.0.6-18.fc23.x86_64 elfutils-libelf-0.165-2.fc23.x86_64 elfutils-libs-0.165-2.fc23.x86_64 libunwind-1.1-10.fc23.x86_64 nss-softokn-freebl-3.21.0-1.1.fc23.x86_64 numactl-libs-2.0.10-3.fc23.x86_64 perl-libs-5.22.1-350.fc23.x86_64 python-libs-2.7.10-8.fc23.x86_64 slang-2.3.0-4.fc23.x86_64 xz-libs-5.2.1-3.fc23.x86_64 zlib-1.2.8-9.fc23.x86_64
(gdb) bt
#0  0x00000000004b9ea5 in tracepoint_error (e=0x0, err=13, sys=0x19b1370 "sched", name=0x19a5d00 "sched_switch") at util/parse-events.c:410
#1  0x00000000004b9fc5 in add_tracepoint (list=0x19a5d20, idx=0x7fffffffb8c0, sys_name=0x19b1370 "sched", evt_name=0x19a5d00 "sched_switch", err=0x0, head_config=0x0)
    at util/parse-events.c:433
#2  0x00000000004ba334 in add_tracepoint_event (list=0x19a5d20, idx=0x7fffffffb8c0, sys_name=0x19b1370 "sched", evt_name=0x19a5d00 "sched_switch", err=0x0, head_config=0x0)
    at util/parse-events.c:498
#3  0x00000000004bb699 in parse_events_add_tracepoint (list=0x19a5d20, idx=0x7fffffffb8c0, sys=0x19b1370 "sched", event=0x19a5d00 "sched_switch", err=0x0, head_config=0x0)
    at util/parse-events.c:936
#4  0x00000000004f6eda in parse_events_parse (_data=0x7fffffffb8b0, scanner=0x19a49d0) at util/parse-events.y:391
#5  0x00000000004bc8e5 in parse_events__scanner (str=0x663ff2 "sched:sched_switch", data=0x7fffffffb8b0, start_token=258) at util/parse-events.c:1361
#6  0x00000000004bca57 in parse_events (evlist=0x19a5220, str=0x663ff2 "sched:sched_switch", err=0x0) at util/parse-events.c:1401
#7  0x0000000000518d5f in perf_evlist__can_select_event (evlist=0x19a3b90, str=0x663ff2 "sched:sched_switch") at util/record.c:253
#8  0x0000000000553c42 in intel_pt_track_switches (evlist=0x19a3b90) at arch/x86/util/intel-pt.c:364
#9  0x00000000005549d1 in intel_pt_recording_options (itr=0x19a2c40, evlist=0x19a3b90, opts=0x8edf68 <record+232>) at arch/x86/util/intel-pt.c:664
#10 0x000000000051e076 in auxtrace_record__options (itr=0x19a2c40, evlist=0x19a3b90, opts=0x8edf68 <record+232>) at util/auxtrace.c:539
#11 0x0000000000433368 in cmd_record (argc=1, argv=0x7fffffffde60, prefix=0x0) at builtin-record.c:1264
#12 0x000000000049bec2 in run_builtin (p=0x8fa2a8 <commands+168>, argc=5, argv=0x7fffffffde60) at perf.c:390
#13 0x000000000049c12a in handle_internal_command (argc=5, argv=0x7fffffffde60) at perf.c:451
#14 0x000000000049c278 in run_argv (argcp=0x7fffffffdcbc, argv=0x7fffffffdcb0) at perf.c:495
#15 0x000000000049c60a in main (argc=5, argv=0x7fffffffde60) at perf.c:618
(gdb) 

I am applying Adrian's original patch, adding the above explanations and
parts of Wang's, the one about grep showing that that parameter can ben NULL while
the function doesn't check it.

- Arnaldo
 
> > Signed-off-by: Wang Nan <wangnan0@huawei.com>
> > Cc: Adrian Hunter <adrian.hunter@intel.com>
> > Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
> > Cc: Tong Zhang <ztong@vt.edu>

> > Cc: Josh Poimboeuf <jpoimboe@redhat.com>
> > ---
> >  tools/perf/util/parse-events.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> > index 4f7b0ef..813d9b2 100644
> > --- a/tools/perf/util/parse-events.c
> > +++ b/tools/perf/util/parse-events.c
> > @@ -399,6 +399,9 @@ static void tracepoint_error(struct parse_events_error *e, int err,
> >  {
> >  	char help[BUFSIZ];
> >  
> > +	if (!e)
> > +		return;
> > +
> >  	/*
> >  	 * We get error directly from syscall errno ( > 0),
> >  	 * or from encoded pointer's error ( < 0).
> > 

Re: [PATCH 1/2] perf tools: Fix fault in error patch of intel_pt_process_auxtrace_info()

[Adrian Hunter]

On 01/02/16 05:21, Wang Nan wrote:
> In error processing path of intel_pt_process_auxtrace_info() it calls
> thread__zput() to clean and free pt->unknown_thread which is created by
> thread__new(). However, when error raise, a segfault happen:
> 
>  # perf script -F event,comm,pid,tid,time,addr,ip,sym,dso,iregs
>  Samples for 'instructions:u' event do not have IREGS attribute set. Cannot print 'iregs' field.
>  intel_pt_synth_events: failed to synthesize 'instructions' event type
>  Segmentation fault (core dumped)
> 
> The problem is: there's a union in 'struct thread' combines a list_head
> and a rb_node. The standard life cycle of a thread is: init rb_node during
> creating, inserted into machine->threads rbtree uses rb_node, move to
> machine->dead_threads using list_head, clean by thread__put:
> list_del_init(&thread->node).

I sent a different patch for this:

	http://marc.info/?l=linux-kernel&m=145381014011697


> 
> In the above command, it clean a thread before adding it into list,
> causes the above segfault.
> 
> This patch gives a fake list_head and link the thread into it before
> calling thread__zput(), get rid of the segfault.
> 
> After this patch:
>  # perf script -F event,comm,pid,tid,time,addr,ip,sym,dso,iregs
>  Samples for 'instructions:u' event do not have IREGS attribute set. Cannot print 'iregs' field.
>  intel_pt_synth_events: failed to synthesize 'instructions' event type
>  0x248 [0x88]: failed to process type: 70
> 
> Reported-by: Tong Zhang <ztong@vt.edu>
> Signed-off-by: Wang Nan <wangnan0@huawei.com>
> Cc: Adrian Hunter <adrian.hunter@intel.com>
> Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
> Cc: Josh Poimboeuf <jpoimboe@redhat.com>
> ---
>  tools/perf/util/intel-pt.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/tools/perf/util/intel-pt.c b/tools/perf/util/intel-pt.c
> index 81a2eb7..e2add63 100644
> --- a/tools/perf/util/intel-pt.c
> +++ b/tools/perf/util/intel-pt.c
> @@ -2013,6 +2013,7 @@ int intel_pt_process_auxtrace_info(union perf_event *event,
>  	struct auxtrace_info_event *auxtrace_info = &event->auxtrace_info;
>  	size_t min_sz = sizeof(u64) * INTEL_PT_PER_CPU_MMAPS;
>  	struct intel_pt *pt;
> +	struct list_head dead_thread;
>  	int err;
>  
>  	if (auxtrace_info->header.size < sizeof(struct auxtrace_info_event) +
> @@ -2153,6 +2154,9 @@ int intel_pt_process_auxtrace_info(union perf_event *event,
>  	return 0;
>  
>  err_delete_thread:
> +	RB_CLEAR_NODE(&pt->unknown_thread->rb_node);
> +	INIT_LIST_HEAD(&dead_thread);
> +	list_add(&pt->unknown_thread->node, &dead_thread);
>  	thread__zput(pt->unknown_thread);
>  err_free_queues:
>  	intel_pt_log_disable();
> 

[tip:perf/urgent] perf tools: Fix thread lifetime related segfaut in intel_pt

[tip-bot for Adrian Hunter]

Commit-ID:  3a4acda1ecbd290973de08250d7dcdfaf5b2fe0f
Gitweb:     http://git.kernel.org/tip/3a4acda1ecbd290973de08250d7dcdfaf5b2fe0f
Author:     Adrian Hunter <adrian.hunter@intel.com>
AuthorDate: Mon, 1 Feb 2016 03:21:04 +0000
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Tue, 2 Feb 2016 12:51:11 -0300

perf tools: Fix thread lifetime related segfaut in intel_pt

intel_pt_process_auxtrace_info() creates a pt->unknown_thread thread
that eventually needs to be freed by the last thread__put() on it, when
its refcount hits zero, which may happen in
intel_pt_process_auxtrace_info() error handling path and triggers the
following segfault, which would happen as well at intel_pt_free, when
tools using this intel_pt codebase frees up resources:

  # perf record -I -e intel_pt/tsc=1,noretcomp=1/u /bin/ls
  0  a  anaconda-ks.cfg  bin   perf.data	perf.data.old  perf-f23-bringup.todo
  [ perf record: Woken up 1 times to write data ]
  [ perf record: Captured and wrote 0.217 MB perf.data ]
  #
  # perf script -F event,comm,pid,tid,time,addr,ip,sym,dso,iregs
  Samples for 'instructions:u' event do not have IREGS attribute set. Cannot print 'iregs' field.
  intel_pt_synth_events: failed to synthesize 'instructions' event type
  Segmentation fault (core dumped)
  #

The problem is: there's a union in 'struct thread' combines a list_head
and a rb_node. The standard life cycle of a thread is: init rb_node in
the constructor, insert it into machine->threads rbtree using rb_node,
move it to machine->dead_threads using list_head, clean in the last
thread__put: list_del_init(&thread->node).

In the above command, it clean a thread before adding it into list,
causes the above segfault.

Since pt->unknown_thread will never live in an rbtree, initialize its
list node so that when list_del_init() is done on it we don't segfault.

After this patch:

  # perf script -F event,comm,pid,tid,time,addr,ip,sym,dso,iregs
  Samples for 'instructions:u' event do not have IREGS attribute set. Cannot print 'iregs' field.
  intel_pt_synth_events: failed to synthesize 'instructions' event type
  0x248 [0x88]: failed to process type: 70
  #

Reported-by: Tong Zhang <ztong@vt.edu>
Reported-by: Wang Nan <wangnan0@huawei.com>
Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Link: http://lkml.kernel.org/r/1454296865-19749-1-git-send-email-wangnan0@huawei.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/intel-pt.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/tools/perf/util/intel-pt.c b/tools/perf/util/intel-pt.c
index 81a2eb7..05d8158 100644
--- a/tools/perf/util/intel-pt.c
+++ b/tools/perf/util/intel-pt.c
@@ -2068,6 +2068,15 @@ int intel_pt_process_auxtrace_info(union perf_event *event,
 		err = -ENOMEM;
 		goto err_free_queues;
 	}
+
+	/*
+	 * Since this thread will not be kept in any rbtree not in a
+	 * list, initialize its list node so that at thread__put() the
+	 * current thread lifetime assuption is kept and we don't segfault
+	 * at list_del_init().
+	 */
+	INIT_LIST_HEAD(&pt->unknown_thread->node);
+
 	err = thread__set_comm(pt->unknown_thread, "unknown", 0);
 	if (err)
 		goto err_delete_thread;