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

[Krister Johansen]

On Sat, Oct 08, 2016 at 11:13:21PM -0700, Krister Johansen wrote:
> On Fri, Oct 07, 2016 at 11:22:00AM +0900, Namhyung Kim wrote:
> > On Wed, Oct 05, 2016 at 08:45:24AM -0300, Arnaldo Carvalho de Melo wrote:
> > > Em Sat, Oct 01, 2016 at 08:13:36PM -0700, Krister Johansen escreveu:
> > > > 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
> > > > @@ -979,6 +982,7 @@ iter_finish_cumulative_entry(struct hist_entry_iter 
> > > > *iter,
> > > >  {
> > > > zfree(>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?

I gave the addr_location__put() approach a try, but it caused me to
remember why I had kept this small.  There are certain circumstances
where callers that lookup maps don't take references, seemingly because
the map is already contained within a map group.  If I move this to
addr_location__put(), then I need to expand the scope of this change.
Right now, it's just focusing on making sure that any map that gets
embedded into a callchain cursor, or retrieved from one and held onto,
gets referenced.

In other words, moving this to addr_location__put() frees a bunch of
maps that are acquired from elsewhere without their reference counts
incremented.

I made the rest of the modifications we discussed.  I'll send a v2 patch
in a separate e-mail.

-K

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

[Krister Johansen]

On Sat, Oct 08, 2016 at 11:13:21PM -0700, Krister Johansen wrote:
> On Fri, Oct 07, 2016 at 11:22:00AM +0900, Namhyung Kim wrote:
> > On Wed, Oct 05, 2016 at 08:45:24AM -0300, Arnaldo Carvalho de Melo wrote:
> > > Em Sat, Oct 01, 2016 at 08:13:36PM -0700, Krister Johansen escreveu:
> > > > 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
> > > > @@ -979,6 +982,7 @@ iter_finish_cumulative_entry(struct hist_entry_iter 
> > > > *iter,
> > > >  {
> > > > zfree(>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?

I gave the addr_location__put() approach a try, but it caused me to
remember why I had kept this small.  There are certain circumstances
where callers that lookup maps don't take references, seemingly because
the map is already contained within a map group.  If I move this to
addr_location__put(), then I need to expand the scope of this change.
Right now, it's just focusing on making sure that any map that gets
embedded into a callchain cursor, or retrieved from one and held onto,
gets referenced.

In other words, moving this to addr_location__put() frees a bunch of
maps that are acquired from elsewhere without their reference counts
incremented.

I made the rest of the modifications we discussed.  I'll send a v2 patch
in a separate e-mail.

-K

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

[Krister Johansen]

Hi Namhyung,

Thanks for looking this over.

On Fri, Oct 07, 2016 at 11:22:00AM +0900, Namhyung Kim wrote:
> On Wed, Oct 05, 2016 at 08:45:24AM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Sat, Oct 01, 2016 at 08:13:36PM -0700, Krister Johansen escreveu:

> > > 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(>list, >val);
> > >  
> > >   callchain_cursor_advance(cursor);
> > > @@ -464,6 +464,7 @@ add_child(struct callchain_node *parent,
> > >  
> > >   list_for_each_entry_safe(call, tmp, >val, list) {
> > >   list_del(>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);
> > > + 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 == >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, >parent_val, list) {
> > >   list_del(>list);
> > > + map__zput(list->ms.map);
> > >   free(list);
> > >   }
> > >  
> > >   list_for_each_entry_safe(list, tmp, >val, list) {
> > >   list_del(>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, , list) {
> > >   list_del(>list);
> > > + map__zput(chain->ms.map);
> 
> I think you need to grab the refcnt in the "while (parent)" loop above.


Thanks; good catch.  I'll fix this.

> > > 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, , sample->period);
> > > + /* Cleanup temporary cursor. */
> > > + callchain_cursor_snapshot_rele();
> 
> 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..

Ok. I'll remove the get/put from the snapshot stuff, and will excise the
rele function.

> > >   return 0;
> > >  }
> > >  
> > > @@ -979,6 +982,7 @@ iter_finish_cumulative_entry(struct hist_entry_iter 
> > > *iter,
> > >  {
> > >   zfree(>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?

Thanks for the feedback.  I'll send out a v2 once I get your changes
incorporated and tested.

-K

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

[Krister Johansen]

Hi Namhyung,

Thanks for looking this over.

On Fri, Oct 07, 2016 at 11:22:00AM +0900, Namhyung Kim wrote:
> On Wed, Oct 05, 2016 at 08:45:24AM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Sat, Oct 01, 2016 at 08:13:36PM -0700, Krister Johansen escreveu:

> > > 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(>list, >val);
> > >  
> > >   callchain_cursor_advance(cursor);
> > > @@ -464,6 +464,7 @@ add_child(struct callchain_node *parent,
> > >  
> > >   list_for_each_entry_safe(call, tmp, >val, list) {
> > >   list_del(>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);
> > > + 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 == >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, >parent_val, list) {
> > >   list_del(>list);
> > > + map__zput(list->ms.map);
> > >   free(list);
> > >   }
> > >  
> > >   list_for_each_entry_safe(list, tmp, >val, list) {
> > >   list_del(>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, , list) {
> > >   list_del(>list);
> > > + map__zput(chain->ms.map);
> 
> I think you need to grab the refcnt in the "while (parent)" loop above.


Thanks; good catch.  I'll fix this.

> > > 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, , sample->period);
> > > + /* Cleanup temporary cursor. */
> > > + callchain_cursor_snapshot_rele();
> 
> 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..

Ok. I'll remove the get/put from the snapshot stuff, and will excise the
rele function.

> > >   return 0;
> > >  }
> > >  
> > > @@ -979,6 +982,7 @@ iter_finish_cumulative_entry(struct hist_entry_iter 
> > > *iter,
> > >  {
> > >   zfree(>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?

Thanks for the feedback.  I'll send out a v2 once I get your changes
incorporated and tested.

-K

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

[Namhyung Kim]

Hi Arnaldo and Krister,

On Wed, Oct 05, 2016 at 08:45:24AM -0300, Arnaldo Carvalho de Melo wrote:
> 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 
> > ---
> >  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(>list, >val);
> >  
> > callchain_cursor_advance(cursor);
> > @@ -464,6 +464,7 @@ add_child(struct callchain_node *parent,
> >  
> > list_for_each_entry_safe(call, tmp, >val, list) {
> > list_del(>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);
> > +   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 == >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, >parent_val, list) {
> > list_del(>list);
> > +   map__zput(list->ms.map);
> > free(list);
> > }
> >  
> > list_for_each_entry_safe(list, tmp, >val, list) {
> > list_del(>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, , list) {
> > list_del(>list);
> > +   map__zput(chain->ms.map);

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


> > 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 
> >  #include 
> >  #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 = >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 = 

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

[Namhyung Kim]

Hi Arnaldo and Krister,

On Wed, Oct 05, 2016 at 08:45:24AM -0300, Arnaldo Carvalho de Melo wrote:
> 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 
> > ---
> >  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(>list, >val);
> >  
> > callchain_cursor_advance(cursor);
> > @@ -464,6 +464,7 @@ add_child(struct callchain_node *parent,
> >  
> > list_for_each_entry_safe(call, tmp, >val, list) {
> > list_del(>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);
> > +   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 == >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, >parent_val, list) {
> > list_del(>list);
> > +   map__zput(list->ms.map);
> > free(list);
> > }
> >  
> > list_for_each_entry_safe(list, tmp, >val, list) {
> > list_del(>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, , list) {
> > list_del(>list);
> > +   map__zput(chain->ms.map);

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


> > 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 
> >  #include 
> >  #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 = >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;
> >  
> > 

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

[Krister Johansen]

On Wed, Oct 05, 2016 at 08:45:24AM -0300, Arnaldo Carvalho de Melo wrote:
> 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?

Absolutely.  Thanks for taking the time to look it over.

AFIACT, this occurs when you're probing a .ko with its own debug
information, but when the kernel has none.  The kernel and all of the
in-tree modules were built through an RPM build that strips out
debuginfo into a separate package.  On this particular system, the
kernel debuginfo packages were not installed.  However, I had recently
changed a dkms build of an external module to use -g and to not strip.
We had one lonely .ko with all of its debug information inside, and a
kernel that perf was going to need to use kallsyms.  I was able to
insert the kprobe into the module and record events.  It was just script
and report that caused the core.

It should be possible to reproduce this by running script against a
trace that was recorded from kprobes in a module that has debug
inforation, but while running a kernel that does not.  I didn't specify
any special options for lookup of vmlinux.  I just let the tool figure
it out.

If you think it'd be useful, I can send along the notes that I took when
I debugged this.  They're about 15k, which is why I would hesitate to
send it to the entire list unsolicited.

Thanks again,

-K

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

[Krister Johansen]

On Wed, Oct 05, 2016 at 08:45:24AM -0300, Arnaldo Carvalho de Melo wrote:
> 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?

Absolutely.  Thanks for taking the time to look it over.

AFIACT, this occurs when you're probing a .ko with its own debug
information, but when the kernel has none.  The kernel and all of the
in-tree modules were built through an RPM build that strips out
debuginfo into a separate package.  On this particular system, the
kernel debuginfo packages were not installed.  However, I had recently
changed a dkms build of an external module to use -g and to not strip.
We had one lonely .ko with all of its debug information inside, and a
kernel that perf was going to need to use kallsyms.  I was able to
insert the kprobe into the module and record events.  It was just script
and report that caused the core.

It should be possible to reproduce this by running script against a
trace that was recorded from kprobes in a module that has debug
inforation, but while running a kernel that does not.  I didn't specify
any special options for lookup of vmlinux.  I just let the tool figure
it out.

If you think it'd be useful, I can send along the notes that I took when
I debugged this.  They're about 15k, which is why I would hesitate to
send it to the entire list unsolicited.

Thanks again,

-K

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

[Masami Hiramatsu]

On Wed, 5 Oct 2016 08:45:24 -0300
Arnaldo Carvalho de Melo  wrote:

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

Yes, I think so, it is able to be founded by refcnt debugger.
In case of getting SIGSEGV, we can enhance it to handle the
signal and dump traced data.

Thanks,

> 
> - Arnaldo
>  
> > Signed-off-by: Krister Johansen 
> > ---
> >  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(>list, >val);
> >  
> > callchain_cursor_advance(cursor);
> > @@ -464,6 +464,7 @@ add_child(struct callchain_node *parent,
> >  
> > list_for_each_entry_safe(call, tmp, >val, list) {
> > list_del(>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);
> > +   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 == >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, >parent_val, list) {
> > list_del(>list);
> > +   map__zput(list->ms.map);
> > free(list);
> > }
> >  
> > list_for_each_entry_safe(list, tmp, >val, list) {
> > list_del(>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, , list) {
> > list_del(>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 
> >  #include 
> >  #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 = >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)
> > 

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

[Masami Hiramatsu]

On Wed, 5 Oct 2016 08:45:24 -0300
Arnaldo Carvalho de Melo  wrote:

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

Yes, I think so, it is able to be founded by refcnt debugger.
In case of getting SIGSEGV, we can enhance it to handle the
signal and dump traced data.

Thanks,

> 
> - Arnaldo
>  
> > Signed-off-by: Krister Johansen 
> > ---
> >  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(>list, >val);
> >  
> > callchain_cursor_advance(cursor);
> > @@ -464,6 +464,7 @@ add_child(struct callchain_node *parent,
> >  
> > list_for_each_entry_safe(call, tmp, >val, list) {
> > list_del(>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);
> > +   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 == >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, >parent_val, list) {
> > list_del(>list);
> > +   map__zput(list->ms.map);
> > free(list);
> > }
> >  
> > list_for_each_entry_safe(list, tmp, >val, list) {
> > list_del(>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, , list) {
> > list_del(>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 
> >  #include 
> >  #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 = >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 

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

[Arnaldo Carvalho de Melo]

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 
> ---
>  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(>list, >val);
>  
>   callchain_cursor_advance(cursor);
> @@ -464,6 +464,7 @@ add_child(struct callchain_node *parent,
>  
>   list_for_each_entry_safe(call, tmp, >val, list) {
>   list_del(>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);
> + 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 == >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, >parent_val, list) {
>   list_del(>list);
> + map__zput(list->ms.map);
>   free(list);
>   }
>  
>   list_for_each_entry_safe(list, tmp, >val, list) {
>   list_del(>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, , list) {
>   list_del(>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 
>  #include 
>  #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 = >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;
> +
> +  

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

[Arnaldo Carvalho de Melo]

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 
> ---
>  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(>list, >val);
>  
>   callchain_cursor_advance(cursor);
> @@ -464,6 +464,7 @@ add_child(struct callchain_node *parent,
>  
>   list_for_each_entry_safe(call, tmp, >val, list) {
>   list_del(>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);
> + 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 == >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, >parent_val, list) {
>   list_del(>list);
> + map__zput(list->ms.map);
>   free(list);
>   }
>  
>   list_for_each_entry_safe(list, tmp, >val, list) {
>   list_del(>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, , list) {
>   list_del(>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 
>  #include 
>  #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 = >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 =