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

Arnaldo Carvalho de Melo
2021-06-04T13:23:03Z

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

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?

- Arnaldo

> > }

> > if (!dso || !map) {
> > @@ -792,6 +793,7 @@ static int machine__process_ksymbol_register(struct machine *machine,
> > map->start = event->ksymbol.addr;
> > map->end = map->start + event->ksymbol.len;
> > maps__insert(&machine->kmaps, map);
> > + map__put(map);
> > dso__set_loaded(dso);

> > if (is_bpf_image(event->ksymbol.name)) {

diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index 3ff4936a15a42f74..da19be7da284c250 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -776,10 +776,10 @@ static int machine__process_ksymbol_register(struct machine *machine,
if (dso) {
dso->kernel = DSO_SPACE__KERNEL;
map = map__new2(0, dso);
+ dso__put(dso);
}

if (!dso || !map) {
- dso__put(dso);
return -ENOMEM;
}

@@ -792,6 +792,7 @@ static int machine__process_ksymbol_register(struct machine *machine,
map->start = event->ksymbol.addr;
map->end = map->start + event->ksymbol.len;
maps__insert(&machine->kmaps, map);
+ map__put(map);
dso__set_loaded(dso);

if (is_bpf_image(event->ksymbol.name)) {