Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <3b8c7c2c5de492c7fbf86df73c43cdb0fbb453df.camel@gmail.com>
Subject: Re: [PATCH] perf ksymbol: fix memory leak: decrease refcount of map
 and dso
From:   Riccardo Mancini <rickyman7@gmail.com>
To:     Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com>,
        Ian Rogers <irogers@google.com>
Cc:     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>
Date:   Fri, 04 Jun 2021 17:16:39 +0200
In-Reply-To: <YLopMBgLWysdJbkm@kernel.org>
References: <20210602231052.317048-1-rickyman7@gmail.com>
         <CAP-5=fVxHUnwGoRypMjCsPSh_yo5PB8Hzbkx5ArA5b0=7S-67g@mail.gmail.com>
         <YLopMBgLWysdJbkm@kernel.org>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.40.1 (3.40.1-1.fc34) 
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

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?

> 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?
Then we would need to see where it's called and add the appropriate map__put.

In addition, having a look at other possible concurrency issues in map.c:
 - 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
lock is taken, then grabbing the refcount on the looping variable is not needed
unless we need to return it, right?
 - 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
worth making explicit that they are not to be used directly (through either a
comment or adding some underscores in their names).
 - maps__empty: should probably take a reader lock.
 - 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.

Riccardo


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