Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754874AbcJIGNx (ORCPT ); Sun, 9 Oct 2016 02:13:53 -0400 Received: from sub5.mail.dreamhost.com ([208.113.200.129]:37179 "EHLO homiemail-a121.g.dreamhost.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753342AbcJIGNv (ORCPT ); Sun, 9 Oct 2016 02:13:51 -0400 Date: Sat, 8 Oct 2016 23:13:21 -0700 From: Krister Johansen To: Namhyung Kim Cc: Arnaldo Carvalho de Melo , Masami Hiramatsu , =?utf-8?B?RnLDqWTDqXJpYw==?= Weisbecker , linux-kernel@vger.kernel.org Subject: Re: callchain map refcounting fixes was Re: [PATCH perf/core] perf script: fix a use after free crash. Message-ID: <20161009061321.GA2677@templeofstupid.com> References: <20161002031336.GA2635@templeofstupid.com> <20161005114524.GY7143@kernel.org> <20161007022200.GB31113@sejong> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20161007022200.GB31113@sejong> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4579 Lines: 131 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(&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); > > 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, &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.. 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(&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? Thanks for the feedback. I'll send out a v2 once I get your changes incorporated and tested. -K