Re: [PATCH] perf ksymbol: fix memory leak: decrease refcount of map and dso

[Arnaldo Carvalho de Melo]

Em Fri, Jun 04, 2021 at 05:16:39PM +0200, Riccardo Mancini escreveu:
> Hi,
> 
> On Fri, 2021-06-04 at 10:22 -0300, Arnaldo Carvalho de Melo wrote:
> > Em Thu, Jun 03, 2021 at 09:26:40PM -0700, Ian Rogers escreveu:
> > > On Wed, Jun 2, 2021 at 4:15 PM Riccardo Mancini <rickyman7@xxxxxxxxx> wrote:
> > > > +++ b/tools/perf/util/machine.c
> > > > @@ -776,6 +776,7 @@ static int machine__process_ksymbol_register(struct
> > > > machine *machine,
> > > >                 if (dso) {
> > > >                         dso->kernel = DSO_SPACE__KERNEL;
> > > >                         map = map__new2(0, dso);
> > > > +                       dso__put(dso);
> > 
> > > Will this cause 2 puts if the map allocation fails? Perhaps this
> > > should be "if (map) dso__put(dso);".
> > 
> > I think its just a matter of removing the put in the error path, i.e.
> > the patch becomes what is at the end of this message.
> > 
> > I.e. if map__new2() fails, we want to drop the dso reference, and if it
> > works, we already have a reference to it, obtained in map__new2().
> 
> Agree.
> I'm sorry for this stupid oversight.
> Should we make it a series including the fix to the issue you pointed out below,
> or should I send you a v2 and fix the other issue in a subsequent patch?

Please send a v2 patch, and then consider starting a new series with the
issues below.
 
> > But looking at this code now I realize that maps__find() should grab a
> > refcount for the map it returns, because in this
> > machine__process_ksymbol_register() function we use reference that 'map'
> > after the if block, i.e. we use it if it came from maps__find() or if we
> > created it machine__process_ksymbol_register, so there is a possible
> > race where other thread removes it from the list and map__put()s it
> > ending up in map__delete() while we still use it in
> > machine__process_ksymbol_register(), right?
> 
> Agree. It should be placed before up_read to avoid races, right?

Yes, we have to grab a refcount while we are sure its not going away,
then return that as the lookup result, whoever receives that refcounted
entry should use it and then drop the refcount.

> Then we would need to see where it's called and add the appropriate map__put.

yes
 
> In addition, having a look at other possible concurrency issues in map.c:

Its good to have new eyes looking at this, exactly at a time we're
discussing further parallelizing perf :-)

>  - maps__for_each_entry should always be called with either read or write lock,
> am I right? It looks like this is not done in certain parts of the code. If such

Right.

> lock is taken, then grabbing the refcount on the looping variable is not needed
> unless we need to return it, right?

Right, returning an entry needs to take a refcount.

>  - maps__first and map__next do not grab a refcount and neither a lock. If
> they're used through a lock-protected loop, it's not a problem, but maybe it's

yes

> worth making explicit that they are not to be used directly (through either a
> comment or adding some underscores in their names).

yes, __ in front means, in kernel style, that it does less than the non
__ prefixed, same name, function.

>  - maps__empty: should probably take a reader lock.

Indeed.

>  - maps__find_symbol: the returned symbol is not protected (the caller does not
> receive a refcount to neither map or dso, so if dso is deleted, his reference to
> the symbol gets invalidated). Depending on how it's being used it might not be a
> problem, but in the general scenario I think it's not thread-safe.

Yes, that function is also problematic.

Thanks for looking into this, please consider sending patches for these
issues,

- Arnaldo