[GIT PULL 0/2] perf/urgent fixes

[GIT PULL 0/2] perf/urgent fixes

[Arnaldo Carvalho de Melo]

See http://www.infradead.org/rpr.html

Hi Ingo,

	Please consider pulling,

- Arnaldo

The following changes since commit 28fb8a5b6e233fc384fb27f9f91f811b40ba9cf8:

  Merge tag 'perf-urgent-for-mingo' of git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux into perf/urgent (2016-01-30 09:15:49 +0100)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git tags/perf-urgent-for-mingo

for you to fetch changes up to d3a37ea00a9fbc0f4fa91647c9ae66c038ba65e3:

  perf tools: Fix fault in error patch of intel_pt_process_auxtrace_info() (2016-02-01 12:03:35 -0300)

----------------------------------------------------------------
perf/urgent fixes:

- tracepoint_error() can receive e=NULL, robustify it, fixes a problem noticed
  with a very specific combination: Machine with Intel PT (e.g. Broadwell),
  kernel with no perf_event_attr.context_switch feature (e.g. 4.2) and unreadable
  tracefs (for instance !root users), making the fallback from
  perf_event_attr.context_switch to the sched:sched_switch tracepoint to fail
  reading its info from tracefs, fix it. (Adrian Hunter)

- Fix fault in error path of intel_pt_process_auxtrace_info(), obeying
  the existing 'struct thread' lifetime cycle checking expectations, noticed
  for instance, when processing perf.data files with Intel PT data using
  'perf script' (Wang Nan)

Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>

----------------------------------------------------------------
Adrian Hunter (1):
      perf tools: tracepoint_error() can receive e=NULL, robustify it

Wang Nan (1):
      perf tools: Fix fault in error patch of intel_pt_process_auxtrace_info()

 tools/perf/util/intel-pt.c     | 4 ++++
 tools/perf/util/parse-events.c | 3 +++
 2 files changed, 7 insertions(+)

[PATCH 1/2] perf tools: tracepoint_error() can receive e=NULL, robustify it

[Arnaldo Carvalho de Melo]

From: Adrian Hunter <adrian.hunter@intel.com>

Fixes segmentation fault using, for instance:

  (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.
  0 x00000000004b9ea5 in tracepoint_error (e=0x0, err=13, sys=0x19b1370 "sched", name=0x19a5d00 "sched_switch") at util/parse-events.c:410
  (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)

Intel PT attempts to find the sched:sched_switch tracepoint but that seg
faults if tracefs is not readable, because the error reporting structure
is null, as errors are not reported when automatically adding
tracepoints.  Fix by checking before using.

Committer note:

This doesn't take place in a kernel that supports
perf_event_attr.context_switch, that is the default way that will be
used for tracking context switches, only in older kernels, like 4.2, in
a machine with Intel PT (e.g. Broadwell) for non-priviledged users.

Further info from a similar patch by Wang:

The error is in tracepoint_error: it assumes the 'e' parameter 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'

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Tong Zhang <ztong@vt.edu>
Cc: Wang Nan <wangnan0@huawei.com>
Cc: stable@vger.kernel.org # v4.4+
Fixes: 196581717d85 ("perf tools: Enhance parsing events tracepoint error output")
Link: http://lkml.kernel.org/r/1453809921-24596-2-git-send-email-adrian.hunter@intel.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@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 4f7b0efdde2f..813d9b272c81 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).
-- 
2.5.0

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

[Arnaldo Carvalho de Melo]

From: Wang Nan <wangnan0@huawei.com>

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 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 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>
Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Adrian Hunter <adrian.hunter@intel.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 | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tools/perf/util/intel-pt.c b/tools/perf/util/intel-pt.c
index 81a2eb77ba7f..e2add6376fec 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();
-- 
2.5.0

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

[Adrian Hunter]

This patch does not fix the problem because the thread__zput() will still
segfault later if the error path is not taken.

Sorry, I didn't look closely at this patch because I was not expecting it
to be taken because of the fix I had already sent:

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

However if you want to keep the struct thread rbtree / list union, the
simple fix would be to reinstate the list initialization in this particular
case i.e.:


diff --git a/tools/perf/util/intel-pt.c b/tools/perf/util/intel-pt.c
index 81a2eb7..18245b8 100644
--- a/tools/perf/util/intel-pt.c
+++ b/tools/perf/util/intel-pt.c
@@ -2068,6 +2068,7 @@ int intel_pt_process_auxtrace_info(union perf_event
                err = -ENOMEM;
                goto err_free_queues;
        }
+       INIT_LIST_HEAD(&pt->unknown_thread->node);
        err = thread__set_comm(pt->unknown_thread, "unknown", 0);
        if (err)
                goto err_delete_thread;



On 01/02/16 19:13, Arnaldo Carvalho de Melo wrote:
> From: Wang Nan <wangnan0@huawei.com>
> 
> 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 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 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>
> Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> Cc: Adrian Hunter <adrian.hunter@intel.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 | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/tools/perf/util/intel-pt.c b/tools/perf/util/intel-pt.c
> index 81a2eb77ba7f..e2add6376fec 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();
> 

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

[Arnaldo Carvalho de Melo]

Em Tue, Feb 02, 2016 at 12:24:19PM +0200, Adrian Hunter escreveu:
> This patch does not fix the problem because the thread__zput() will still
> segfault later if the error path is not taken.
> 
> Sorry, I didn't look closely at this patch because I was not expecting it
> to be taken because of the fix I had already sent:
> 
> 	http://marc.info/?l=linux-kernel&m=145431692623940
> 
> However if you want to keep the struct thread rbtree / list union, the
> simple fix would be to reinstate the list initialization in this particular
> case i.e.:

Right, so it keeps the existing thread lifetime assumptions both in the
error path, as Wang did, and in the normal pt->unknown_thread end of
life at intel_pt_free(), I'm replacing the patch...

- Arnaldo
 
> 
> diff --git a/tools/perf/util/intel-pt.c b/tools/perf/util/intel-pt.c
> index 81a2eb7..18245b8 100644
> --- a/tools/perf/util/intel-pt.c
> +++ b/tools/perf/util/intel-pt.c
> @@ -2068,6 +2068,7 @@ int intel_pt_process_auxtrace_info(union perf_event
>                 err = -ENOMEM;
>                 goto err_free_queues;
>         }
> +       INIT_LIST_HEAD(&pt->unknown_thread->node);
>         err = thread__set_comm(pt->unknown_thread, "unknown", 0);
>         if (err)
>                 goto err_delete_thread;
> 
> 
> 
> On 01/02/16 19:13, Arnaldo Carvalho de Melo wrote:
> > From: Wang Nan <wangnan0@huawei.com>
> > 
> > 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 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 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>
> > Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> > Cc: Adrian Hunter <adrian.hunter@intel.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 | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/tools/perf/util/intel-pt.c b/tools/perf/util/intel-pt.c
> > index 81a2eb77ba7f..e2add6376fec 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();
> > 

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

[Arnaldo Carvalho de Melo]

Em Tue, Feb 02, 2016 at 12:24:19PM +0200, Adrian Hunter escreveu:
> This patch does not fix the problem because the thread__zput() will still
> segfault later if the error path is not taken.
> 
> Sorry, I didn't look closely at this patch because I was not expecting it
> to be taken because of the fix I had already sent:
> 
> 	http://marc.info/?l=linux-kernel&m=145431692623940
> 
> However if you want to keep the struct thread rbtree / list union, the
> simple fix would be to reinstate the list initialization in this particular
> case i.e.:

So, can I go with the following patch+description+authorship?

>From 3a4acda1ecbd290973de08250d7dcdfaf5b2fe0f Mon Sep 17 00:00:00 2001
From: Adrian Hunter <adrian.hunter@intel.com>
Date: Mon, 1 Feb 2016 03:21:04 +0000
Subject: [PATCH 1/1] 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 81a2eb77ba7f..05d815851be1 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;
-- 
2.5.0

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

[Arnaldo Carvalho de Melo]

Em Tue, Feb 02, 2016 at 12:52:25PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Tue, Feb 02, 2016 at 12:24:19PM +0200, Adrian Hunter escreveu:
> > This patch does not fix the problem because the thread__zput() will still
> > segfault later if the error path is not taken.
> > 
> > Sorry, I didn't look closely at this patch because I was not expecting it
> > to be taken because of the fix I had already sent:
> > 
> > 	http://marc.info/?l=linux-kernel&m=145431692623940
> > 
> > However if you want to keep the struct thread rbtree / list union, the
> > simple fix would be to reinstate the list initialization in this particular
> > case i.e.:
> 
> So, can I go with the following patch+description+authorship?

BTW, I just noticed it while testing some unrelated patch (Jiri's hpp
hists stuff) that with a perf.data file with intel pt data, I get this
at the end of a TUI session, i.e. the segfault at tool exit:


Available samples
0 intel_pt/tsc=1,noretcomp=1/u                                                                                                                                              ◆
29 sched:sched_switch                                                                                                                                                       ▒
0 dummy:u                                                                                                                                                                   ▒
6 instructions:u                                                                                                                                                            ▒
0 transactions                                                                                                                                                              ▒
                                                                                                                                                                            Program received signal SIGSEGV, Segmentation fault.                                                                                                                         ▒
                                                   0x00000000004ec5c5 in __write_once_size (size=8, res=0x7fffffffc2e0, p=0x0) at /home/acme/git/linux/tools/include/linux/compiler.h:82                                                                                                                                                                 ▒
82              case 8: *(volatile __u64_alias_t *) p = *(__u64_alias_t *) res; break;                                                                                      ▒
Missing separate debuginfos, use: dnf debuginfo-install audit-libs-2.4.5-1.fc23.x86_64 bzip2-libs-1.0.6-19.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  0x00000000004ec5c5 in __write_once_size (size=8, res=0x7fffffffc2e0, p=0x0) at /home/acme/git/linux/tools/include/linux/compiler.h:82                                   ▒
#1  __list_del (prev=0x0, next=0x1a2e970) at /home/acme/git/linux/tools/include/linux/list.h:89                                                                             ▒
#2  0x00000000004ec631 in __list_del_entry (entry=0x1a2e970) at /home/acme/git/linux/tools/include/linux/list.h:101                                                         ▒
#3  0x00000000004ec6d2 in list_del_init (entry=0x1a2e970) at /home/acme/git/linux/tools/include/linux/list.h:144                                                            ▒
#4  0x00000000004ecd2a in thread__put (thread=0x1a2e970) at util/thread.c:104                                                                                               ▒
#5  0x000000000052de51 in intel_pt_free (session=0x19a3b90) at util/intel-pt.c:1747                                                                                         ▒
#6  0x00000000004e4e3d in auxtrace__free (session=0x19a3b90) at util/auxtrace.h:513                                                                                         ▒
#7  0x00000000004e5603 in perf_session__delete (session=0x19a3b90) at util/session.c:181                                                                                    ▒
#8  0x00000000004363f3 in cmd_report (argc=0, argv=0x7fffffffdeb0, prefix=0x0) at builtin-report.c:984                                                                      ▒
#9  0x000000000049bea3 in run_builtin (p=0x8fa2c0 <commands+192>, argc=1, argv=0x7fffffffdeb0) at perf.c:390                                                                ▒
#10 0x000000000049c10b in handle_internal_command (argc=1, argv=0x7fffffffdeb0) at perf.c:451                                                                               ▒
#11 0x000000000049c259 in run_argv (argcp=0x7fffffffdd0c, argv=0x7fffffffdd00) at perf.c:495                                                                                ▒
#12 0x000000000049c5eb in main (argc=1, argv=0x7fffffffdeb0) at perf.c:618                                                                                                  ▒
(gdb) 
 
> From 3a4acda1ecbd290973de08250d7dcdfaf5b2fe0f Mon Sep 17 00:00:00 2001
> From: Adrian Hunter <adrian.hunter@intel.com>
> Date: Mon, 1 Feb 2016 03:21:04 +0000
> Subject: [PATCH 1/1] 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 81a2eb77ba7f..05d815851be1 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;
> -- 
> 2.5.0
> 

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

[Wangnan (F)]

On 2016/2/2 23:52, Arnaldo Carvalho de Melo wrote:
> Em Tue, Feb 02, 2016 at 12:24:19PM +0200, Adrian Hunter escreveu:
>> This patch does not fix the problem because the thread__zput() will still
>> segfault later if the error path is not taken.
>>
>> Sorry, I didn't look closely at this patch because I was not expecting it
>> to be taken because of the fix I had already sent:
>>
>> 	http://marc.info/?l=linux-kernel&m=145431692623940
>>
>> However if you want to keep the struct thread rbtree / list union, the
>> simple fix would be to reinstate the list initialization in this particular
>> case i.e.:
> So, can I go with the following patch+description+authorship?

I didn't really understand the lifecycle of unknown_thread, and though
it would go to an rbtree in normal path. I think the patch you posted
is good.

Thank you.

>  From 3a4acda1ecbd290973de08250d7dcdfaf5b2fe0f Mon Sep 17 00:00:00 2001
> From: Adrian Hunter <adrian.hunter@intel.com>
> Date: Mon, 1 Feb 2016 03:21:04 +0000
> Subject: [PATCH 1/1] 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 81a2eb77ba7f..05d815851be1 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;

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

[Adrian Hunter]

On 2/02/2016 5:52 p.m., Arnaldo Carvalho de Melo wrote:
> Em Tue, Feb 02, 2016 at 12:24:19PM +0200, Adrian Hunter escreveu:
>> This patch does not fix the problem because the thread__zput() will still
>> segfault later if the error path is not taken.
>>
>> Sorry, I didn't look closely at this patch because I was not expecting it
>> to be taken because of the fix I had already sent:
>>
>> 	http://marc.info/?l=linux-kernel&m=145431692623940
>>
>> However if you want to keep the struct thread rbtree / list union, the
>> simple fix would be to reinstate the list initialization in this particular
>> case i.e.:
>
> So, can I go with the following patch+description+authorship?

Fine by me.  Thank you!

>
>  From 3a4acda1ecbd290973de08250d7dcdfaf5b2fe0f Mon Sep 17 00:00:00 2001
> From: Adrian Hunter <adrian.hunter@intel.com>
> Date: Mon, 1 Feb 2016 03:21:04 +0000
> Subject: [PATCH 1/1] 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 81a2eb77ba7f..05d815851be1 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;
>