callchain map refcounting fixes was Re: [PATCH perf/core] perf script: fix a use after free crash.

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

Em Sat, Oct 01, 2016 at 08:13:36PM -0700, Krister Johansen escreveu:
> 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
 
> Signed-off-by: Krister Johansen <kjlx@templeofstupid.com>
> ---
>  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(-)
> 
> diff --git a/tools/perf/util/callchain.c b/tools/perf/util/callchain.c
> index 07fd30b..15c89b2 100644
> --- a/tools/perf/util/callchain.c
> +++ b/tools/perf/util/callchain.c
> @@ -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;
> diff --git a/tools/perf/util/callchain.h b/tools/perf/util/callchain.h
> index 13e7554..0d944ef 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"
> @@ -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
> diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
> index b02992e..f8335e8 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"
> @@ -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;
>  }
> -- 
> 2.7.4