Message-ID: <da5b052d2c94db91c0bf8cb794c5cad299f19e57.camel@gmail.com>
Date:   Fri, 18 Jun 2021 12:01:28 +0200
From:   Riccardo Mancini <rickyman7@...il.com>
To:     Arnaldo Carvalho de Melo <arnaldo.melo@...il.com>
Cc:     Ian Rogers <irogers@...gle.com>,
        Namhyung Kim <namhyung@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Mark Rutland <mark.rutland@....com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Jiri Olsa <jolsa@...hat.com>,
        Adrian Hunter <adrian.hunter@...el.com>,
        Andi Kleen <ak@...ux.intel.com>,
        Tommi Rantala <tommi.t.rantala@...ia.com>,
        linux-perf-users <linux-perf-users@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] perf ksymbol: fix memory leak: decrease refcount of map
 and dso

Hi, 

On Fri, 2021-06-04 at 15:29 -0300, Arnaldo Carvalho de Melo wrote:
<SNIP> 
> > > 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

This function has quite a number of callers (direct and indirect) so the the
patch is becoming huge. 

One of these callers is thread__find_map, which returns an addr_location
(actually it's an output argument). This addr_location holds references to map,
maps and thread without getting any refcnt (actually in one function it gets it
on the thread and a comment tells to put it once done). If I'm not wrong, this
addr_location is never malloced (always a local variable) and, is should be
present in parts of the code where there should be a refcnt on the thread.
Therefore, maybe it does not get the refcnts since it assumes that thread (upon
which depends maps and as a consequence map) is always refcnted in its context.
However, I think that it should get all refcnts anyways for clarity and to
prevent possible misuses (if I understood correctly, Ian is of the same
opinion).

My solution would be to add the refcnt grabbing for map, maps and thread in
thread__find_map, releasing them in addr_location__put, and then making sure all
callers call it when no longer in use.

Following the same reasoning, I added refcnt grabbing also to mem_info,
branch_info (map was already refcnted, I added it also to maps for coherency),
map_symbol (as in branch_info, I added it to maps), and in other places in which
I saw a pointer was passed without refcounting.

Most changes are quite trivial, however, the changelog is huge:
48 files changed, 472 insertions(+), 157 deletions(-)
Most of them are just returns converted to goto for calling the __put functions.

Doing so, I managed to remove memory leaks caused by refcounting also in perf-
report (I wanted to try also perf top but I encountered another memory-related
issue). However, the changelog is huge and testing all of it is challenging
(especially since I can test missing puts only with ASan's LeakSanitizer and its
reports are usually full of leaks, which I am trying to fix along the way, I
will send some patches in the following days). How would you go about it? Do you
have any suggestions?

>  
> > 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.

This issue is easier to solve than expected since the map is returned as **mapp,
so it's just a matter of making sure that the caller always passes it and then
puts the refcnt.

Thanks,
Riccardo

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

Powered by blists - more mailing lists