perf script: fix a use after free crash.

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 cursor with the invalid map. Use the existing map refcount mechanism to forestall cleanup of a map until the cursor iterates past the node.

Signed-off-by Krister Johansen <[email protected]>

---
 tools/perf/util/callchain.c | 12 ++++++++++--
 tools/perf/util/callchain.h | 20 ++++++++++++++++++++
 tools/perf/util/hist.c      |  4 ++++
 3 files changed, 34 insertions(+), 2 deletions(-)

tools/perf/util/callchain.c +10 -2

index 07fd30b..15c89b2 100644
@@ -439,7 +439,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);
 		list_add_tail(&call->list, &node->val);
 
 		callchain_cursor_advance(cursor);

@@ -464,6 +464,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);

@@ -732,6 +733,7 @@ merge_chain_branch(struct callchain_cursor *cursor,
 		callchain_cursor_append(cursor, list->ip,
 					list->ms.map, list->ms.sym);
 		list_del(&list->list);
+		map__zput(list->ms.map);
 		free(list);
 	}
 

@@ -780,7 +782,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;
 
 	cursor->nr++;

@@ -830,6 +833,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;

@@ -947,11 +952,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);
 	}
 

@@ -1035,6 +1042,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;

tools/perf/util/callchain.h +20 -0

index 13e7554..0d944ef 100644
@@ -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"

@@ -178,8 +179,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,

@@ -238,12 +244,26 @@ int perf_callchain_config(const char *var, const char *value);
 static inline void callchain_cursor_snapshot(struct callchain_cursor *dest,
 					     struct callchain_cursor *src)
 {
+	struct callchain_cursor_node *node;
+
 	*dest = *src;
 
 	dest->first = src->curr;
 	dest->nr -= src->pos;
+
+	for (node = dest->first; node != NULL; node = node->next)
+		map__get(node->map);
 }
 
+static inline void callchain_cursor_snapshot_rele(struct callchain_cursor *curs)
+{
+	struct callchain_cursor_node *node;
+
+	for (node = curs->first; node != NULL; node = node->next)
+		map__put(node->map);
+}
+
+
 #ifdef HAVE_SKIP_CALLCHAIN_IDX
 int arch_skip_callchain_idx(struct thread *thread, struct ip_callchain *chain);
 #else

tools/perf/util/hist.c +4 -0

index b02992e..f8335e8 100644
@@ -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"

@@ -970,6 +971,8 @@ iter_add_next_cumulative_entry(struct hist_entry_iter *iter,
 
 	if (symbol_conf.use_callchain)
 		callchain_append(he->callchain, &cursor, sample->period);
+	/* Cleanup temporary cursor. */
+	callchain_cursor_snapshot_rele(&cursor);
 	return 0;
 }
 

@@ -979,6 +982,7 @@ iter_finish_cumulative_entry(struct hist_entry_iter *iter,
 {
 	zfree(&iter->priv);
 	iter->he = NULL;
+	map__zput(al->map);
 
 	return 0;
 }

Signature · 1 line

2.7.4

Em Sat, Oct 01, 2016 at 08:13:36PM -0700, Krister Johansen escreveu:

Quoted 5 lines

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 cursor with the invalid map. Use the existing map refcount mechanism to forestall cleanup of a map until the cursor iterates past the node.

Seems ok, thanks for working on this! Can you provide a test case that causes the SEGV so that I can, in addition to reviewing your changes and auditing the code to check if all cases ara plugged, to reproduce the problem?

Frédéric, Namhyung, Ack?

Masami, is this a case that your refcount validator can catch?

- Arnaldo

Quoted 161 lines

Signed-off-by Krister Johansen <[email protected]>

---
 tools/perf/util/callchain.c | 12 ++++++++++--
 tools/perf/util/callchain.h | 20 ++++++++++++++++++++
 tools/perf/util/hist.c      |  4 ++++
 3 files changed, 34 insertions(+), 2 deletions(-)

tools/perf/util/callchain.c +10 -2

index 07fd30b..15c89b2 100644
@@ -439,7 +439,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);
 		list_add_tail(&call->list, &node->val);
 
 		callchain_cursor_advance(cursor);

@@ -464,6 +464,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);

@@ -732,6 +733,7 @@ merge_chain_branch(struct callchain_cursor *cursor,
 		callchain_cursor_append(cursor, list->ip,
 					list->ms.map, list->ms.sym);
 		list_del(&list->list);
+		map__zput(list->ms.map);
 		free(list);
 	}
 

@@ -780,7 +782,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;
 
 	cursor->nr++;

@@ -830,6 +833,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;

@@ -947,11 +952,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);
 	}
 

@@ -1035,6 +1042,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;

tools/perf/util/callchain.h +20 -0

index 13e7554..0d944ef 100644
@@ -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"

@@ -178,8 +179,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,

@@ -238,12 +244,26 @@ int perf_callchain_config(const char *var, const char *value);
 static inline void callchain_cursor_snapshot(struct callchain_cursor *dest,
 					     struct callchain_cursor *src)
 {
+	struct callchain_cursor_node *node;
+
 	*dest = *src;
 
 	dest->first = src->curr;
 	dest->nr -= src->pos;
+
+	for (node = dest->first; node != NULL; node = node->next)
+		map__get(node->map);
 }
 
+static inline void callchain_cursor_snapshot_rele(struct callchain_cursor *curs)
+{
+	struct callchain_cursor_node *node;
+
+	for (node = curs->first; node != NULL; node = node->next)
+		map__put(node->map);
+}
+
+
 #ifdef HAVE_SKIP_CALLCHAIN_IDX
 int arch_skip_callchain_idx(struct thread *thread, struct ip_callchain *chain);
 #else

tools/perf/util/hist.c +4 -0

index b02992e..f8335e8 100644
@@ -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"

@@ -970,6 +971,8 @@ iter_add_next_cumulative_entry(struct hist_entry_iter *iter,
 
 	if (symbol_conf.use_callchain)
 		callchain_append(he->callchain, &cursor, sample->period);
+	/* Cleanup temporary cursor. */
+	callchain_cursor_snapshot_rele(&cursor);
 	return 0;
 }
 

@@ -979,6 +982,7 @@ iter_finish_cumulative_entry(struct hist_entry_iter *iter,
 {
 	zfree(&iter->priv);
 	iter->he = NULL;
+	map__zput(al->map);
 
 	return 0;
 }

Signature · 1 line

2.7.4

Em Sat, Oct 01, 2016 at 08:13:36PM -0700, Krister Johansen escreveu:

Quoted 5 lines

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 cursor with the invalid map. Use the existing map refcount mechanism to forestall cleanup of a map until the cursor iterates past the node.

Seems ok, thanks for working on this! Can you provide a test case that causes the SEGV so that I can, in addition to reviewing your changes and auditing the code to check if all cases ara plugged, to reproduce the problem?

Em Sat, Oct 01, 2016 at 08:13:36PM -0700, Krister Johansen escreveu:

Quoted 5 lines

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 cursor with the invalid map. Use the existing map refcount mechanism to forestall cleanup of a map until the cursor iterates past the node.

Seems ok, thanks for working on this! Can you provide a test case that causes the SEGV so that I can, in addition to reviewing your changes and auditing the code to check if all cases ara plugged, to reproduce the problem?

Frédéric, Namhyung, Ack?

Masami, is this a case that your refcount validator can catch?

- Arnaldo

Quoted 69 lines

Signed-off-by Krister Johansen <[email protected]>

---
 tools/perf/util/callchain.c | 12 ++++++++++--
 tools/perf/util/callchain.h | 20 ++++++++++++++++++++
 tools/perf/util/hist.c      |  4 ++++
 3 files changed, 34 insertions(+), 2 deletions(-)

tools/perf/util/callchain.c +9 -2

index 07fd30b..15c89b2 100644
@@ -439,7 +439,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);
 		list_add_tail(&call->list, &node->val);
 
 		callchain_cursor_advance(cursor);

@@ -464,6 +464,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);

@@ -732,6 +733,7 @@ merge_chain_branch(struct callchain_cursor *cursor,
 		callchain_cursor_append(cursor, list->ip,
 					list->ms.map, list->ms.sym);
 		list_del(&list->list);
+		map__zput(list->ms.map);
 		free(list);
 	}
 

@@ -780,7 +782,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;
 
 	cursor->nr++;

@@ -830,6 +833,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;

@@ -947,11 +952,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);
 	}
 

tools/perf/util/callchain.c int callchain_node__make_parent_list(struct callchain_node *node) · line 1045

Quoted 5 lines

tools/perf/util/callchain.c +1 -0

@@ -1035,6 +1042,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);

Quoted 70 lines

tools/perf/util/callchain.c +0 -0

 		free(chain);
 	}
 	return -ENOMEM;

tools/perf/util/callchain.h +20 -0

index 13e7554..0d944ef 100644
@@ -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"

@@ -178,8 +179,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,

@@ -238,12 +244,26 @@ int perf_callchain_config(const char *var, const char *value);
 static inline void callchain_cursor_snapshot(struct callchain_cursor *dest,
 					     struct callchain_cursor *src)
 {
+	struct callchain_cursor_node *node;
+
 	*dest = *src;
 
 	dest->first = src->curr;
 	dest->nr -= src->pos;
+
+	for (node = dest->first; node != NULL; node = node->next)
+		map__get(node->map);
 }
 
+static inline void callchain_cursor_snapshot_rele(struct callchain_cursor *curs)
+{
+	struct callchain_cursor_node *node;
+
+	for (node = curs->first; node != NULL; node = node->next)
+		map__put(node->map);
+}
+
+
 #ifdef HAVE_SKIP_CALLCHAIN_IDX
 int arch_skip_callchain_idx(struct thread *thread, struct ip_callchain *chain);
 #else

tools/perf/util/hist.c +1 -0

index b02992e..f8335e8 100644
@@ -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"

@@ -970,6 +971,8 @@ iter_add_next_cumulative_entry(struct hist_entry_iter *iter,
 

tools/perf/util/hist.c iter_add_next_cumulative_entry(struct hist_entry_iter *iter, · line 975

Quoted 4 lines

tools/perf/util/hist.c +2 -0

 	if (symbol_conf.use_callchain)
 		callchain_append(he->callchain, &cursor, sample->period);
+	/* Cleanup temporary cursor. */
+	callchain_cursor_snapshot_rele(&cursor);

tools/perf/util/hist.c iter_finish_cumulative_entry(struct hist_entry_iter *iter, · line 985

Quoted 8 lines

tools/perf/util/hist.c +1 -0

 	return 0;
 }
 

@@ -979,6 +982,7 @@ iter_finish_cumulative_entry(struct hist_entry_iter *iter,
 {
 	zfree(&iter->priv);
 	iter->he = NULL;
+	map__zput(al->map);

Quoted 5 lines

tools/perf/util/hist.c +0 -0

 	return 0;
 }

Signature · 1 line

2.7.4

Quoted 64 lines · Arnaldo Carvalho de Melo

Em Sat, Oct 01, 2016 at 08:13:36PM -0700, Krister Johansen escreveu:

Quoted 62 lines

tools/perf/util/callchain.c +9 -2

index 07fd30b..15c89b2 100644
@@ -439,7 +439,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);
 		list_add_tail(&call->list, &node->val);
 
 		callchain_cursor_advance(cursor);

@@ -464,6 +464,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);

@@ -732,6 +733,7 @@ merge_chain_branch(struct callchain_cursor *cursor,
 		callchain_cursor_append(cursor, list->ip,
 					list->ms.map, list->ms.sym);
 		list_del(&list->list);
+		map__zput(list->ms.map);
 		free(list);
 	}
 

@@ -780,7 +782,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;
 
 	cursor->nr++;

@@ -830,6 +833,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;

@@ -947,11 +952,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);
 	}
 

Quoted 5 lines

tools/perf/util/callchain.c int callchain_node__make_parent_list(struct callchain_node *node) · line 1045

Quoted 5 lines

tools/perf/util/callchain.c +1 -0

@@ -1035,6 +1042,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);

I think you need to grab the refcnt in the "while (parent)" loop above.

Quoted 14 lines

Quoted 14 lines

tools/perf/util/hist.c +1 -0

index b02992e..f8335e8 100644
@@ -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"

@@ -970,6 +971,8 @@ iter_add_next_cumulative_entry(struct hist_entry_iter *iter,
 

Quoted 4 lines

tools/perf/util/hist.c iter_add_next_cumulative_entry(struct hist_entry_iter *iter, · line 975

Quoted 4 lines

tools/perf/util/hist.c +2 -0

 	if (symbol_conf.use_callchain)
 		callchain_append(he->callchain, &cursor, sample->period);
+	/* Cleanup temporary cursor. */
+	callchain_cursor_snapshot_rele(&cursor);

This callchain shotshot is used in a short period of time, and it's guaranteed that the maps in callchains will not freed due to refcnt in the orignal callchain cursor. So I think we can skip to get/put refcnt on the snapshot cursor. Also "rele" seems not a good name..

Quoted 8 lines

tools/perf/util/hist.c iter_finish_cumulative_entry(struct hist_entry_iter *iter, · line 985

Quoted 8 lines

tools/perf/util/hist.c +1 -0

 	return 0;
 }
 

@@ -979,6 +982,7 @@ iter_finish_cumulative_entry(struct hist_entry_iter *iter,
 {
 	zfree(&iter->priv);
 	iter->he = NULL;
+	map__zput(al->map);

What is this needed? Why other places like iter_finish_normal_entry isn't?

Quoted 14 lines · Namhyung Kim

Quoted 10 lines · Arnaldo Carvalho de Melo

Em Sat, Oct 01, 2016 at 08:13:36PM -0700, Krister Johansen escreveu:

tools/perf/util/hist.c iter_finish_cumulative_entry(struct hist_entry_iter *iter, · line 985

Quoted 9 lines

tools/perf/util/hist.c +1 -0

index b02992e..f8335e8 100644
@@ -979,6 +982,7 @@ iter_finish_cumulative_entry(struct hist_entry_iter *iter,
 {
 	zfree(&iter->priv);
 	iter->he = NULL;
+	map__zput(al->map);

What is this needed? Why other places like iter_finish_normal_entry isn't?

I put a map__zput() here because iter_next_cumulative_entry() calls fill_callchain_info(), which takes a reference on the al->map in the struct addr_location that it returns. I had forgotten all of this by the time you asked. When I went back to work out my rationale, I stumbled across addr_location__put(). Think it would make sense to relocate the put of al->map there instead?

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.

Quoted 5 lines

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.

Em Tue, Oct 25, 2016 at 05:20:10PM -0700, Krister Johansen escreveu:

Quoted 11 lines · Krister Johansen

Quoted 5 lines

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.

Quoted 13 lines

Em Tue, Oct 25, 2016 at 05:20:10PM -0700, Krister Johansen escreveu:

Quoted 11 lines · Krister Johansen

Quoted 5 lines

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.

Quoted 4 lines

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?

Em Thu, Nov 10, 2016 at 04:40:46PM -0800, Krister Johansen escreveu:

Quoted 3 lines

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

(quoted diff) +3 -5

-    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.

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

(quoted diff) +3 -5

-    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 <[email protected]>
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 <[email protected]>

Cc Frederic Weisbecker <[email protected]>

Cc Masami Hiramatsu <[email protected]>

Cc Namhyung Kim <[email protected]>

Link http://lkml.kernel.org/r/[email protected] open here

Signed-off-by Arnaldo Carvalho de Melo <[email protected]>

tools/perf/util/callchain.c +11 -2

index 823befd8209a..18bb7caee535 100644
@@ -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;

tools/perf/util/callchain.h +6 -0

index d9c70dccf06a..f551fd2cfe5a 100644
@@ -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,

tools/perf/util/hist.c +1 -0

index e1be4132054d..be4b07145705 100644
@@ -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"

Quoted 4 lines

#include "evlist.h"

tools/perf/util/hist.c +0 -0

@@ -979,6 +980,7 @@ iter_finish_cumulative_entry(struct hist_entry_iter *iter,
 {
 	zfree(&iter->priv);

tools/perf/util/hist.c iter_finish_cumulative_entry(struct hist_entry_iter *iter, · line 983

Quoted 2 lines

tools/perf/util/hist.c +1 -0

 	iter->he = NULL;
+	map__zput(al->map);

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?

Tried it again with what is in Linus' tree + your patch and got the same problem:

[acme@jouet linux]$ git remote -v | grep torvalds.*fetch
torvalds git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git (fetch)
[acme@jouet linux]$ git checkout -b test-branch torvalds/master
Branch test-branch set up to track remote branch master from torvalds. Switched to a new branch 'test-branch'
[acme@jouet linux]$ git cherry-pick f7347a33099dbad7e9fb3c22cea211f238bfd320 [test-branch 7d786f548b62] perf callchain: Fix a use after free crash due to refcounting bug
Author: Krister Johansen <[email protected]>
Date: Mon Jan 2 12:06:55 2017 -0300

 3 files changed, 19 insertions(+), 2 deletions(-)

[acme@jouet linux]$ rm -rf /tmp/build/perf/ ; mkdir -p /tmp/build/perf ; make O=/tmp/build/perf -C tools/perf install-bin
make: Entering directory '/home/acme/git/linux/tools/perf'
BUILD: Doing 'make -j4' parallel build
HOSTCC /tmp/build/perf/fixdep.o
<SNIP>

Then I run it with a higher frequency and no delay in refreshing the screen, to stress the refcounting code:

# perf top -F 10000 -g -d 0

Do it while running something like 'make -j32 allmodconfig' to create lots of short lived processes (or use stress-ng, etc).

Em Mon, Jan 02, 2017 at 12:15:14PM -0300, Arnaldo Carvalho de Melo escreveu:

Quoted 22 lines

Tried it again with what is in Linus' tree + your patch and got the same problem:

[acme@jouet linux]$ git remote -v | grep torvalds.*fetch
torvalds git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git (fetch)
[acme@jouet linux]$ git checkout -b test-branch torvalds/master
Branch test-branch set up to track remote branch master from torvalds. Switched to a new branch 'test-branch'
[acme@jouet linux]$ git cherry-pick f7347a33099dbad7e9fb3c22cea211f238bfd320 [test-branch 7d786f548b62] perf callchain: Fix a use after free crash due to refcounting bug
Author: Krister Johansen <[email protected]>
Date: Mon Jan 2 12:06:55 2017 -0300

 3 files changed, 19 insertions(+), 2 deletions(-)

[acme@jouet linux]$ rm -rf /tmp/build/perf/ ; mkdir -p /tmp/build/perf ; make O=/tmp/build/perf -C tools/perf install-bin
make: Entering directory '/home/acme/git/linux/tools/perf'
BUILD: Doing 'make -j4' parallel build
HOSTCC /tmp/build/perf/fixdep.o
<SNIP>

Then I run it with a higher frequency and no delay in refreshing the screen, to stress the refcounting code:

Quoted 4 lines

# perf top -F 10000 -g -d 0

Do it while running something like 'make -j32 allmodconfig' to create lots of short lived processes (or use stress-ng, etc).

Back to acme/perf/core and with debugging, we're getting a refcount hitting zero while the map is still in a rbtree:

perf: util/map.c:246: map__exit: Assertion `!(!((&map->rb_node)->__rb_parent_color == (unsigned long)(&map->rb_node)))' failed.

Thread 1 "perf" received signal SIGABRT, Aborted.
0x00007ffff522691f in raise () from /lib64/libc.so.6
(gdb) bt

#0  0x00007ffff522691f in raise () from /lib64/libc.so.6
#1  0x00007ffff522851a in abort () from /lib64/libc.so.6
#2  0x00007ffff521eda7 in __assert_fail_base () from /lib64/libc.so.6
#3  0x00007ffff521ee52 in __assert_fail () from /lib64/libc.so.6
#4  0x0000000000504e57 in map__exit (map=0x2393790) at util/map.c:246
#5  0x0000000000504ea5 in map__delete (map=0x2393790) at util/map.c:252
#6  0x0000000000504f0a in map__put (map=0x2393790) at util/map.c:259
#7  0x000000000052fa01 in __map__zput (map=0x7fffffff8230) at util/map.h:161
#8  0x000000000053295b in iter_finish_cumulative_entry (iter=0x7fffffff8260, al=0x7fffffff8220) at util/hist.c:983
#9  0x0000000000532b53 in hist_entry_iter__add (iter=0x7fffffff8260, al=0x7fffffff8220, max_stack_depth=127, arg=0x7fffffffa7b0) at util/hist.c:1059
#10 0x000000000044f5cf in perf_event__process_sample (tool=0x7fffffffa7b0, event=0x7ffff7e24578, evsel=0x21515d0, sample=0x7fffffff8410, machine=0x21b2bf8)
    at builtin-top.c:774

#11 0x000000000044f8ee in perf_top__mmap_read_idx (top=0x7fffffffa7b0, idx=2) at builtin-top.c:840
#12 0x000000000044fa0d in perf_top__mmap_read (top=0x7fffffffa7b0) at builtin-top.c:857
#13 0x0000000000450080 in __cmd_top (top=0x7fffffffa7b0) at builtin-top.c:1002
#14 0x00000000004514e0 in cmd_top (argc=0, argv=0x7fffffffe130, prefix=0x0) at builtin-top.c:1330
#15 0x00000000004b5af5 in run_builtin (p=0xa0baf8 <commands+312>, argc=6, argv=0x7fffffffe130) at perf.c:358
#16 0x00000000004b5d62 in handle_internal_command (argc=6, argv=0x7fffffffe130) at perf.c:420
#17 0x00000000004b5ea7 in run_argv (argcp=0x7fffffffdf8c, argv=0x7fffffffdf80) at perf.c:466
#18 0x00000000004b6290 in main (argc=6, argv=0x7fffffffe130) at perf.c:610
(gdb) fr 4
#4 0x0000000000504e57 in map__exit (map=0x2393790) at util/map.c:246
246 BUG_ON(!RB_EMPTY_NODE(&map->rb_node));
(gdb) p map
$1 = (struct map *) 0x2393790
(gdb) p *map
$2 = {{rb_node = {__rb_parent_color = 37304353, rb_right = 0x0, rb_left = 0x0}, node = {next = 0x2393821, prev = 0x0}}, start = 140434683187200,
end = 140434690723840, type = 0 '\000', erange_warned = false, priv = 0, prot = 5, flags = 2, pgoff = 0, reloc = 0, maj = 253, min = 0, ino = 132875,

  ino_generation = 3472328296227680304, map_ip = 0x504125 <map__map_ip>, unmap_ip = 0x504174 <map__unmap_ip>, dso = 0x22b3890, groups = 0x2385290, refcnt = {
    counter = 0}}

(gdb)

Em Mon, Jan 02, 2017 at 02:35:30PM -0300, Arnaldo Carvalho de Melo escreveu:

Quoted 1 line

Em Mon, Jan 02, 2017 at 12:15:14PM -0300, Arnaldo Carvalho de Melo escreveu:

 {
        zfree(&iter->priv);
        iter->he = NULL;

+ map__zput(al->map);

        return 0;

}

Quoted 36 lines

perf: util/map.c:246: map__exit: Assertion `!(!((&map->rb_node)->__rb_parent_color == (unsigned long)(&map->rb_node)))' failed.

Thread 1 "perf" received signal SIGABRT, Aborted.
0x00007ffff522691f in raise () from /lib64/libc.so.6
(gdb) bt

#0  0x00007ffff522691f in raise () from /lib64/libc.so.6
#1  0x00007ffff522851a in abort () from /lib64/libc.so.6
#2  0x00007ffff521eda7 in __assert_fail_base () from /lib64/libc.so.6
#3  0x00007ffff521ee52 in __assert_fail () from /lib64/libc.so.6
#4  0x0000000000504e57 in map__exit (map=0x2393790) at util/map.c:246
#5  0x0000000000504ea5 in map__delete (map=0x2393790) at util/map.c:252
#6  0x0000000000504f0a in map__put (map=0x2393790) at util/map.c:259
#7  0x000000000052fa01 in __map__zput (map=0x7fffffff8230) at util/map.h:161
#8  0x000000000053295b in iter_finish_cumulative_entry (iter=0x7fffffff8260, al=0x7fffffff8220) at util/hist.c:983
#9  0x0000000000532b53 in hist_entry_iter__add (iter=0x7fffffff8260, al=0x7fffffff8220, max_stack_depth=127, arg=0x7fffffffa7b0) at util/hist.c:1059
#10 0x000000000044f5cf in perf_event__process_sample (tool=0x7fffffffa7b0, event=0x7ffff7e24578, evsel=0x21515d0, sample=0x7fffffff8410, machine=0x21b2bf8)
    at builtin-top.c:774

#11 0x000000000044f8ee in perf_top__mmap_read_idx (top=0x7fffffffa7b0, idx=2) at builtin-top.c:840
#12 0x000000000044fa0d in perf_top__mmap_read (top=0x7fffffffa7b0) at builtin-top.c:857
#13 0x0000000000450080 in __cmd_top (top=0x7fffffffa7b0) at builtin-top.c:1002
#14 0x00000000004514e0 in cmd_top (argc=0, argv=0x7fffffffe130, prefix=0x0) at builtin-top.c:1330
#15 0x00000000004b5af5 in run_builtin (p=0xa0baf8 <commands+312>, argc=6, argv=0x7fffffffe130) at perf.c:358
#16 0x00000000004b5d62 in handle_internal_command (argc=6, argv=0x7fffffffe130) at perf.c:420
#17 0x00000000004b5ea7 in run_argv (argcp=0x7fffffffdf8c, argv=0x7fffffffdf80) at perf.c:466
#18 0x00000000004b6290 in main (argc=6, argv=0x7fffffffe130) at perf.c:610
(gdb) fr 4
#4 0x0000000000504e57 in map__exit (map=0x2393790) at util/map.c:246
246 BUG_ON(!RB_EMPTY_NODE(&map->rb_node));
(gdb) p map
$1 = (struct map *) 0x2393790
(gdb) p *map
$2 = {{rb_node = {__rb_parent_color = 37304353, rb_right = 0x0, rb_left = 0x0}, node = {next = 0x2393821, prev = 0x0}}, start = 140434683187200,
end = 140434690723840, type = 0 '\000', erange_warned = false, priv = 0, prot = 5, flags = 2, pgoff = 0, reloc = 0, maj = 253, min = 0, ino = 132875,

  ino_generation = 3472328296227680304, map_ip = 0x504125 <map__map_ip>, unmap_ip = 0x504174 <map__unmap_ip>, dso = 0x22b3890, groups = 0x2385290, refcnt = {
    counter = 0}}

(gdb)

Em Mon, Jan 02, 2017 at 02:36:57PM -0300, Arnaldo Carvalho de Melo escreveu:

Quoted 6 lines

Em Mon, Jan 02, 2017 at 02:35:30PM -0300, Arnaldo Carvalho de Melo escreveu:

Quoted 1 line

Em Mon, Jan 02, 2017 at 12:15:14PM -0300, Arnaldo Carvalho de Melo escreveu:

 {
        zfree(&iter->priv);
        iter->he = NULL;

+ map__zput(al->map);

What this pairs to? I was expecting that since this is called via:

   hist_entry_iter__add()
   {
           <SNIP>
           err2 = iter->ops->finish_entry(iter, al);
   }

Then it would have to match something done earlier in hist_entry_iter__add(), most likely by some iter->ops->() method, but I couldn'd find anything to that extent, can you clarify?

Em Mon, Jan 02, 2017 at 04:39:04PM -0300, Arnaldo Carvalho de Melo escreveu:

Quoted 16 lines

Em Mon, Jan 02, 2017 at 02:36:57PM -0300, Arnaldo Carvalho de Melo escreveu:

Quoted 6 lines

Em Mon, Jan 02, 2017 at 02:35:30PM -0300, Arnaldo Carvalho de Melo escreveu:

Quoted 1 line

Em Mon, Jan 02, 2017 at 12:15:14PM -0300, Arnaldo Carvalho de Melo escreveu:

 {
        zfree(&iter->priv);
        iter->he = NULL;

+ map__zput(al->map);

What this pairs to? I was expecting that since this is called via:

   hist_entry_iter__add()
   {
           <SNIP>
           err2 = iter->ops->finish_entry(iter, al);
   }

Quoted 3 lines

Then it would have to match something done earlier in hist_entry_iter__add(), most likely by some iter->ops->() method, but I couldn'd find anything to that extent, can you clarify?

With the following patch it has been running all day, care to explain why it is needed? I need to run this on valgrind or with Masami's refcount debugger to get more clues :-\

- Arnaldo

tools/perf/util/hist.c +0 -0

index 72f5c82798e9..c27bda16e9cd 100644

Quoted 17 lines · Arnaldo Carvalho de Melo

Em Mon, Jan 02, 2017 at 04:39:04PM -0300, Arnaldo Carvalho de Melo escreveu:

Quoted 16 lines

Em Mon, Jan 02, 2017 at 02:36:57PM -0300, Arnaldo Carvalho de Melo escreveu:

Quoted 6 lines

Em Mon, Jan 02, 2017 at 02:35:30PM -0300, Arnaldo Carvalho de Melo escreveu:

Quoted 1 line

Em Mon, Jan 02, 2017 at 12:15:14PM -0300, Arnaldo Carvalho de Melo escreveu:

 {
        zfree(&iter->priv);
        iter->he = NULL;

+ map__zput(al->map);

What this pairs to? I was expecting that since this is called via:

   hist_entry_iter__add()
   {
           <SNIP>
           err2 = iter->ops->finish_entry(iter, al);
   }

Quoted 7 lines

Quoted 3 lines

Then it would have to match something done earlier in hist_entry_iter__add(), most likely by some iter->ops->() method, but I couldn'd find anything to that extent, can you clarify?

With the following patch it has been running all day, care to explain why it is needed? I need to run this on valgrind or with Masami's refcount debugger to get more clues :-\

Let me try a version of this patch that dispenses with the code in both fill_callchain_info() and iter_finish_cumulative_entry. However, before I do that I'll make sure I can figure out how to reproduce the core that you were seeing. I don't want to send you yet another patch that doesn't run. The busy system may be the clue I needed. I had been running these on a mostly idle system.

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.