Messages in this thread | | | Date | Fri, 4 Jun 2021 15:29:33 -0300 | From | Arnaldo Carvalho de Melo <> | Subject | Re: [PATCH] perf ksymbol: fix memory leak: decrease refcount of map and dso |
| |
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@gmail.com> 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
| |