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

From: Arnaldo Carvalho de Melo <acme@kernel.org>
To: Ian Rogers <irogers@google.com>
Cc: Riccardo Mancini <rickyman7@gmail.com>,
	Namhyung Kim <namhyung@kernel.org>,
	Peter Zijlstra <peterz@infradead.org>,
	Ingo Molnar <mingo@redhat.com>,
	Mark Rutland <mark.rutland@arm.com>,
	Alexander Shishkin <alexander.shishkin@linux.intel.com>,
	Jiri Olsa <jolsa@redhat.com>,
	Adrian Hunter <adrian.hunter@intel.com>,
	Andi Kleen <ak@linux.intel.com>,
	Tommi Rantala <tommi.t.rantala@nokia.com>,
	linux-perf-users <linux-perf-users@vger.kernel.org>,
	LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] perf ksymbol: fix memory leak: decrease refcount of map and dso
Date: Fri, 4 Jun 2021 10:22:56 -0300	[thread overview]
Message-ID: <YLopMBgLWysdJbkm@kernel.org> (raw)
In-Reply-To: <CAP-5=fVxHUnwGoRypMjCsPSh_yo5PB8Hzbkx5ArA5b0=7S-67g@mail.gmail.com>

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)) {
