Messages in this thread | | | Subject | Re: [PATCH] perf ksymbol: fix memory leak: decrease refcount of map and dso | From | Riccardo Mancini <> | Date | Fri, 18 Jun 2021 12:01:28 +0200 |
| |
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
| |