Date:   Tue, 4 Jan 2022 22:19:42 -0800
From:   Krister Johansen <kjlx@templeofstupid.com>
To:     Arnaldo Carvalho de Melo <acme@kernel.org>
Cc:     Riccardo Mancini <rickyman7@gmail.com>,
        Ian Rogers <irogers@google.com>,
        Namhyung Kim <namhyung@kernel.org>,
        Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@redhat.com>,
        Mark Rutland <mark.rutland@arm.com>,
        Jiri Olsa <jolsa@redhat.com>, linux-kernel@vger.kernel.org,
        linux-perf-users@vger.kernel.org
Subject: Re: [PATCH 01/20] perf nsinfo: fix refcounting
Message-ID: <20220105061942.GA20464@templeofstupid.com>
References: <cover.1626343282.git.rickyman7@gmail.com>
 <55223bc8821b34ccb01f92ef1401c02b6a32e61f.1626343282.git.rickyman7@gmail.com>
 <YPCGONcQx5SxEKdY@kernel.org>
 <YPCKeDvyLOQzKTln@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YPCKeDvyLOQzKTln@kernel.org>
User-Agent: Mutt/1.5.24 (2015-08-30)
Precedence: bulk

On Thu, Jul 15, 2021 at 04:20:24PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Thu, Jul 15, 2021 at 04:02:16PM -0300, Arnaldo Carvalho de Melo escreveu:
> > Em Thu, Jul 15, 2021 at 06:07:06PM +0200, Riccardo Mancini escreveu:
> > > ASan reports a memory leak of nsinfo during the execution of the perf
> > > test "31: Lookup mmap thread".
> > > The leak is caused by a refcounted variable being replaced without
> > > dropping the refcount.
> 
> > > This patch makes sure that the refcnt of nsinfo is decreased whenever
> > > a refcounted variable is replaced with a new value.
> 
> > So, there are multiple fixes in just one patch, I'll split it into
> > three, no need to resend.
> 
> > I'll try and check if finding Fixes: for the three is easy, that way
> > stable@vger.kernel.org will figure out which of the supported releases
> > need each of them.
> 
> The second is at the end of this message, and looking at it I think
> there is a problem with where this nsinfo reference is stored, it should
> be in the 'struct map', that is per-thread, not in the 'struct dso',
> that may be shared by multiple 'struct map's in different cgroups, so we
> can't set dso->nsinfo in map__new(), right? Kristen?

Apologies for the long delayed reply, and for the hassle that this has
caused.

You're right, Arnaldo, that namespace mapping could differ on a per
thread basis.  If the dso objects are intended to be read by multiple
threads with different views of the target process's namespaces.  With
containers, though, there might be many different versions of the same
object with the same filename, and the only ways we can disambiguate are
through the build-ids or by ensuring that the dso refers to a single
pid and mount namespace.  I may have erred in assuming that all of
perf's threads would be able to access the target namespace via the same
identifiers.  Is that what's going on here?

-K


> commit 280f3933661f9ca9563ed04ce82313fdbcbb8f75
> Author: Riccardo Mancini <rickyman7@gmail.com>
> Date:   Thu Jul 15 18:07:06 2021 +0200
> 
>     perf map: Fix dso->nsinfo refcounting
>     
>     ASan reports a memory leak of nsinfo during the execution of
>     
>       # perf test "31: Lookup mmap thread"
>     
>     The leak is caused by a refcounted variable being replaced without
>     dropping the refcount.
>     
>     This patch makes sure that the refcnt of nsinfo is decreased whenever a
>     refcounted variable is replaced with a new value.
>     
>     Signed-off-by: Riccardo Mancini <rickyman7@gmail.com>
>     Fixes: bf2e710b3cb8445c ("perf maps: Lookup maps in both intitial mountns and inner mountns.")
>     Cc: Ian Rogers <irogers@google.com>
>     Cc: Jiri Olsa <jolsa@redhat.com>
>     Cc: Krister Johansen <kjlx@templeofstupid.com>
>     Cc: Mark Rutland <mark.rutland@arm.com>
>     Cc: Namhyung Kim <namhyung@kernel.org>
>     Cc: Peter Zijlstra <peterz@infradead.org>
>     Link: http://lore.kernel.org/lkml/55223bc8821b34ccb01f92ef1401c02b6a32e61f.1626343282.git.rickyman7@gmail.com
>     Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> 
> diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
> index 8af693d9678cefe0..72e7f3616157ead4 100644
> --- a/tools/perf/util/map.c
> +++ b/tools/perf/util/map.c
> @@ -192,6 +192,8 @@ struct map *map__new(struct machine *machine, u64 start, u64 len,
>  			if (!(prot & PROT_EXEC))
>  				dso__set_loaded(dso);
>  		}
> +
> +		nsinfo__put(dso->nsinfo);
>  		dso->nsinfo = nsi;
>  
>  		if (build_id__is_defined(bid))
>