Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S935249AbcJGCWN convert rfc822-to-8bit (ORCPT ); Thu, 6 Oct 2016 22:22:13 -0400 Received: from LGEAMRELO13.lge.com ([156.147.23.53]:57658 "EHLO lgeamrelo13.lge.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751027AbcJGCWE (ORCPT ); Thu, 6 Oct 2016 22:22:04 -0400 X-Original-SENDERIP: 156.147.1.151 X-Original-MAILFROM: namhyung@kernel.org X-Original-SENDERIP: 165.244.98.76 X-Original-MAILFROM: namhyung@kernel.org X-Original-SENDERIP: 10.177.227.17 X-Original-MAILFROM: namhyung@kernel.org Date: Fri, 7 Oct 2016 11:22:00 +0900 From: Namhyung Kim To: Arnaldo Carvalho de Melo CC: Krister Johansen , Masami Hiramatsu , =?utf-8?B?RnLDqWTDqXJpYw==?= Weisbecker , Subject: Re: callchain map refcounting fixes was Re: [PATCH perf/core] perf script: fix a use after free crash. Message-ID: <20161007022200.GB31113@sejong> References: <20161002031336.GA2635@templeofstupid.com> <20161005114524.GY7143@kernel.org> MIME-Version: 1.0 In-Reply-To: <20161005114524.GY7143@kernel.org> User-Agent: Mutt/1.7.0 (2016-08-17) X-MIMETrack: Itemize by SMTP Server on LGEKRMHUB02/LGE/LG Group(Release 8.5.3FP6|November 21, 2013) at 2016/10/07 11:22:00, Serialize by Router on LGEKRMHUB02/LGE/LG Group(Release 8.5.3FP6|November 21, 2013) at 2016/10/07 11:22:01 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6454 Lines: 198 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(&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. > > 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 = &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); 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.. > > 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? > > > > return 0; > > } > > -- > > 2.7.4