From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1759061AbdADUpB (ORCPT ); Wed, 4 Jan 2017 15:45:01 -0500 Received: from hapkido.dreamhost.com ([66.33.216.122]:60130 "EHLO hapkido.dreamhost.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751427AbdADUo7 (ORCPT ); Wed, 4 Jan 2017 15:44:59 -0500 Date: Wed, 4 Jan 2017 00:37:39 -0800 From: Krister Johansen To: Arnaldo Carvalho de Melo Cc: Krister Johansen , Namhyung Kim , Masami Hiramatsu , =?utf-8?B?RnLDqWTDqXJpYw==?= Weisbecker , linux-kernel@vger.kernel.org Subject: Re: [PATCH v2 perf/core] perf script: fix a use after free crash. Message-ID: <20170104083739.GA3009@templeofstupid.com> References: <20161026002010.GD2525@templeofstupid.com> <20161026134453.GA4936@kernel.org> <20161111004046.GA2185@templeofstupid.com> <20161122190106.GE5390@kernel.org> <20161229013947.GA2341@templeofstupid.com> <20170102151514.GB21178@kernel.org> <20170102173530.GA27864@kernel.org> <20170102173657.GB27864@kernel.org> <20170102193904.GC27864@kernel.org> <20170103003033.GD27864@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170103003033.GD27864@kernel.org> 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 On Mon, Jan 02, 2017 at 09:30:33PM -0300, Arnaldo Carvalho de Melo wrote: > Em Mon, Jan 02, 2017 at 04:39:04PM -0300, Arnaldo Carvalho de Melo escreveu: > > Em Mon, Jan 02, 2017 at 02:36:57PM -0300, Arnaldo Carvalho de Melo escreveu: > > > Em Mon, Jan 02, 2017 at 02:35:30PM -0300, Arnaldo Carvalho de Melo escreveu: > > > > 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() > > { > > > > 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? > > 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 > > diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c > index 72f5c82798e9..c27bda16e9cd 100644 > --- a/tools/perf/util/hist.c > +++ b/tools/perf/util/hist.c > @@ -980,7 +980,6 @@ iter_finish_cumulative_entry(struct hist_entry_iter *iter, > { > zfree(&iter->priv); > iter->he = NULL; > - map__zput(al->map); > > return 0; > } Thanks for taking the time to debug it this far. I certainly appreciate it. The map_zput() in iter_finish_cumulative_entry() was intended to offset the map_get() in iter_next_cumulative_entry() -> fill_callchain_info(). The call to fill_callchain_info should perform a map__get() on the addr_location that gets passed in. However, it looks like I mistakenly assumed that fill_callcahin_info would always get called from iter_next_cumulative_entry when clearly that doesn't happen if callchain_cursor_current returns a NULL node. Looking back, it's not entirely clear to me that the map__get() in fill_callchain_info is necessary either, since its only caller is the hist_iter_cumulative used by hist_entry_iter__add. 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. Thanks again, and apologies for all of the inconvenience. -K