Re: [PATCH v2 perf/core] perf script: fix a use after free crash.

[Arnaldo Carvalho de Melo <acme@kernel.org>]

Em Thu, Nov 10, 2016 at 04:40:46PM -0800, Krister Johansen escreveu:
> On Wed, Oct 26, 2016 at 11:44:53AM -0200, Arnaldo Carvalho de Melo wrote:
> > Em Tue, Oct 25, 2016 at 05:20:10PM -0700, Krister Johansen escreveu:
> > > On Tue, Oct 11, 2016 at 02:28:39AM -0700, Krister Johansen wrote:
> > > > If dso__load_kcore frees all of the existing maps, but one has already
> > > > been attached to a callchain cursor node, then we can get a SIGSEGV in
> > > > any function that happens to try to use this invalid cursor.  Use the
> > > > existing map refcount mechanism to forestall cleanup of a map until the
> > > > cursor iterates past the node.
> > > 
> > > It has been a couple of weeks since I sent out v2 of this patch.  I
> > > understand that folks here have plenty of irons in the fire, but I
> > > wanted to double-check that nobody was waiting on me for additional
> > > information or changes.
> > 
> > It was a mix of waiting for more people to review it, or for Masami to
> > run its refcount debugger on it, ended up falling thru the cracks.
> > 
> > I'll try to process it now.
> 
> Thanks.  As part of processing this did you run into any problems?
> Would you like me to rebase against the latest perf/core and re-send the
> patch?

Sorry for the overly long delay, trying it now after fixing up a
conflict with a recent patchkit (branch stuff) I tested it by running
'perf top -g' and I'm getting some assertion bugs:


# perf top -g
           1.34% filemap_map_pages
         - 0.59% alloc_pages_vma
              1.20% __alloc_pages_nodemask
-    5.87%     0.45%  [kernel]                            [k] handle_mm_fault
   - 1.94% handle_mm_fault
        1.34% filemap_map_pages
      - 0.59% alloc_pages_vma
           1.22% __alloc_pages_nodemask
+    5.75%     0.03%  perf                                [.] hist_entry_iter__add
+    4.46%     0.00%  [unknown]                           [.] 0000000000000000
-    4.06%     2.74%  libc-2.23.so                        [.] _int_malloc
   - 1.95% 0
        1.94% _int_malloc
-    3.20%     0.23%  perf                                [.] iter_add_next_cumulative_entry
   - 1.49% iter_add_next_cumulative_entry
      - 1.43% __hists__add_entry
     2.58%     0.01%  [kernel]                            [k] return_from_SYSCALL_64
     2.57%     2.55%  libperl.so.5.22.2                   [.] Perl_fbm_instr
-    2.54%     2.51%  liblzma.so.5.2.2                    [.] lzma_decode
   - 2.51% lzma_decode
     2.33%     0.00%  ld-2.23.so                          [.] _dl_sysdep_start
+    2.24%     0.04%  ld-2.23.so                          [.] dl_main
     2.13%     0.03%  [kernel]                            [k] ext4_readdir
     2.09%     0.01%  [kernel]                            [k] sys_newstat
     2.08%     0.04%  [kernel]                            [k] vfs_fstatat
     2.07%     0.02%  [kernel]                            [k] SYSC_newstat
     2.02%     0.01%  [kernel]                            [k] iterate_dir
-    1.96%     0.17%  [kernel]                            [k] __alloc_pages_nodemask
   - 1.37% __alloc_pages_nodemask
perf: util/map.c:246: map__exit: Assertion `!(!((&map->rb_node)->__rb_parent_color == (unsigned long)(&map->rb_node)))' failed.
                                                                                                                               Aborted (core dumped)
[root@jouet ~]# 


I'll try to investigate this further later/tomorrow, find the updated patch below.

- Arnaldo

commit af04d2c4a5d1f6bd7f4971118e4e1153cc7c2506
Author: Krister Johansen <kjlx@templeofstupid.com>
Date:   Tue Oct 11 02:28:39 2016 -0700

    perf callchain: Fix a use after free crash due to refcounting bug
    
    If dso__load_kcore frees all of the existing maps, but one has already
    been attached to a callchain cursor node, then we can get a SIGSEGV in
    any function that happens to try to use this invalid cursor.  Use the
    existing map refcount mechanism to forestall cleanup of a map until the
    cursor iterates past the node.
    
    Signed-off-by: Krister Johansen <kjlx@templeofstupid.com>
    Cc: Frederic Weisbecker <fweisbec@gmail.com>
    Cc: Masami Hiramatsu <mhiramat@kernel.org>
    Cc: Namhyung Kim <namhyung@kernel.org>
    Link: http://lkml.kernel.org/r/20161011092839.GC7837@templeofstupid.com
    Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>

diff --git a/tools/perf/util/callchain.c b/tools/perf/util/callchain.c
index 823befd8209a..18bb7caee535 100644
--- a/tools/perf/util/callchain.c
+++ b/tools/perf/util/callchain.c
@@ -437,7 +437,7 @@ fill_node(struct callchain_node *node, struct callchain_cursor *cursor)
 		}
 		call->ip = cursor_node->ip;
 		call->ms.sym = cursor_node->sym;
-		call->ms.map = cursor_node->map;
+		call->ms.map = map__get(cursor_node->map);
 
 		if (cursor_node->branch) {
 			call->branch_count = 1;
@@ -477,6 +477,7 @@ add_child(struct callchain_node *parent,
 
 		list_for_each_entry_safe(call, tmp, &new->val, list) {
 			list_del(&call->list);
+			map__zput(call->ms.map);
 			free(call);
 		}
 		free(new);
@@ -761,6 +762,7 @@ merge_chain_branch(struct callchain_cursor *cursor,
 					list->ms.map, list->ms.sym,
 					false, NULL, 0, 0);
 		list_del(&list->list);
+		map__zput(list->ms.map);
 		free(list);
 	}
 
@@ -811,7 +813,8 @@ int callchain_cursor_append(struct callchain_cursor *cursor,
 	}
 
 	node->ip = ip;
-	node->map = map;
+	map__zput(node->map);
+	node->map = map__get(map);
 	node->sym = sym;
 	node->branch = branch;
 	node->nr_loop_iter = nr_loop_iter;
@@ -868,6 +871,8 @@ int fill_callchain_info(struct addr_location *al, struct callchain_cursor_node *
 			goto out;
 	}
 
+	map__get(al->map);
+
 	if (al->map->groups == &al->machine->kmaps) {
 		if (machine__is_host(al->machine)) {
 			al->cpumode = PERF_RECORD_MISC_KERNEL;
@@ -1142,11 +1147,13 @@ static void free_callchain_node(struct callchain_node *node)
 
 	list_for_each_entry_safe(list, tmp, &node->parent_val, list) {
 		list_del(&list->list);
+		map__zput(list->ms.map);
 		free(list);
 	}
 
 	list_for_each_entry_safe(list, tmp, &node->val, list) {
 		list_del(&list->list);
+		map__zput(list->ms.map);
 		free(list);
 	}
 
@@ -1210,6 +1217,7 @@ int callchain_node__make_parent_list(struct callchain_node *node)
 				goto out;
 			*new = *chain;
 			new->has_children = false;
+			map__get(new->ms.map);
 			list_add_tail(&new->list, &head);
 		}
 		parent = parent->parent;
@@ -1230,6 +1238,7 @@ int callchain_node__make_parent_list(struct callchain_node *node)
 out:
 	list_for_each_entry_safe(chain, new, &head, list) {
 		list_del(&chain->list);
+		map__zput(chain->ms.map);
 		free(chain);
 	}
 	return -ENOMEM;
diff --git a/tools/perf/util/callchain.h b/tools/perf/util/callchain.h
index d9c70dccf06a..f551fd2cfe5a 100644
--- a/tools/perf/util/callchain.h
+++ b/tools/perf/util/callchain.h
@@ -5,6 +5,7 @@
 #include <linux/list.h>
 #include <linux/rbtree.h>
 #include "event.h"
+#include "map.h"
 #include "symbol.h"
 
 #define HELP_PAD "\t\t\t\t"
@@ -184,8 +185,13 @@ int callchain_merge(struct callchain_cursor *cursor,
  */
 static inline void callchain_cursor_reset(struct callchain_cursor *cursor)
 {
+	struct callchain_cursor_node *node;
+
 	cursor->nr = 0;
 	cursor->last = &cursor->first;
+
+	for (node = cursor->first; node != NULL; node = node->next)
+		map__zput(node->map);
 }
 
 int callchain_cursor_append(struct callchain_cursor *cursor, u64 ip,
diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index e1be4132054d..be4b07145705 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -1,6 +1,7 @@
 #include "util.h"
 #include "build-id.h"
 #include "hist.h"
+#include "map.h"
 #include "session.h"
 #include "sort.h"
 #include "evlist.h"
@@ -979,6 +980,7 @@ iter_finish_cumulative_entry(struct hist_entry_iter *iter,
 {
 	zfree(&iter->priv);
 	iter->he = NULL;
+	map__zput(al->map);
 
 	return 0;
 }