Date: Tue, 11 Oct 2016 02:28:25 -0700
From: Krister Johansen <kjlx@templeofstupid.com>
To: Namhyung Kim <namhyung@kernel.org>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>,
        Masami Hiramatsu <mhiramat@kernel.org>,
        =?utf-8?B?RnLDqWTDqXJpYw==?= Weisbecker <fweisbec@gmail.com>,
        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: <20161011092825.GB7837@templeofstupid.com>
References: <20161002031336.GA2635@templeofstupid.com>
 <20161005114524.GY7143@kernel.org>
 <20161007022200.GB31113@sejong>
 <20161009061321.GA2677@templeofstupid.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20161009061321.GA2677@templeofstupid.com>
User-Agent: Mutt/1.5.24 (2015-08-30)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1893
Lines: 41

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

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