On Tue, Nov 22, 2016 at 04:01:06PM -0300, Arnaldo Carvalho de Melo wrote: > 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 <k...@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 <k...@templeofstupid.com> > Cc: Frederic Weisbecker <fweis...@gmail.com> > Cc: Masami Hiramatsu <mhira...@kernel.org> > Cc: Namhyung Kim <namhy...@kernel.org> > Link: http://lkml.kernel.org/r/20161011092839.gc7...@templeofstupid.com > Signed-off-by: Arnaldo Carvalho de Melo <a...@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; > }
As part of trying to tie up the year-end loose-ends, I went back and re-tested a rebase'd version of this patch against perf/core. I ended up with a merge that's identical to yours, except that I'm not seeing any assertion failures with 'perf top -g', 'perf script', or 'perf report'. Was perf/core the branch that was giving you trouble? -K
