From: Arnaldo Carvalho de Melo <acme@kernel.org>
To: Krister Johansen <kjlx@templeofstupid.com>
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
Date: Thu, 6 Jan 2022 17:34:08 -0300 [thread overview]
Message-ID: <YddSQB8Sztuf053g@kernel.org> (raw)
In-Reply-To: <YddQv1O057Mw5Whv@kernel.org>
Em Thu, Jan 06, 2022 at 05:27:43PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Tue, Jan 04, 2022 at 10:19:42PM -0800, Krister Johansen escreveu:
> > 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?
>
> yeah, I think using build-ids, when available is the way to go, and
> since we can almost take it for granted (haven't checked) that it will
> be present, as I think gcc/clang adds them by default, seems the way to
> go.
>
> That or somehow have the DSO list to be perf namespace.
To be by namespace, sorry.
But having it by build-id seems the more sensible way to go, so that we
can avoid having multiple instances of the same DSO in multiple
namespaces, which should be common.
I.e. continue to have a perf 'struct machine' DSO list instead of moving
to having the namespace hierarchy represented under 'struct machine'
since we have a way to uniquify DSOs, the build id.
One way or the other, nsinfo shouldn't be stored in DSOs, its a map
property, which in turn is a thread property.
I.e. please consider thinking about moving it to 'struct map', would you
try it, please?
- Arnaldo
> - Arnaldo
>
> > -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))
> > >
>
> --
>
> - Arnaldo
--
- Arnaldo
next prev parent reply other threads:[~2022-01-06 20:34 UTC|newest]
Thread overview: 50+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-07-15 16:07 [PATCH 00/20] perf: fix several memory leaks reported by ASan on perf-test Riccardo Mancini
2021-07-15 16:07 ` [PATCH 01/20] perf nsinfo: fix refcounting Riccardo Mancini
2021-07-15 19:02 ` Arnaldo Carvalho de Melo
2021-07-15 19:07 ` Arnaldo Carvalho de Melo
2021-07-15 19:20 ` Arnaldo Carvalho de Melo
2022-01-05 6:19 ` Krister Johansen
2022-01-06 20:27 ` Arnaldo Carvalho de Melo
2022-01-06 20:34 ` Arnaldo Carvalho de Melo [this message]
2021-07-15 19:44 ` Arnaldo Carvalho de Melo
2021-07-15 16:07 ` [PATCH 02/20] perf env: fix sibling_dies memory leak Riccardo Mancini
2021-07-15 19:49 ` Arnaldo Carvalho de Melo
2021-07-16 13:13 ` Riccardo Mancini
2021-07-15 16:07 ` [PATCH 03/20] perf test: session_topology: delete session->evlist Riccardo Mancini
2021-07-15 19:51 ` Arnaldo Carvalho de Melo
2021-07-15 16:07 ` [PATCH 04/20] perf test: event_update: fix memory leak of evlist Riccardo Mancini
2021-07-15 19:54 ` Arnaldo Carvalho de Melo
2021-07-15 16:07 ` [PATCH 05/20] perf test: event_update: fix memory leak of unit Riccardo Mancini
2021-07-15 19:58 ` Arnaldo Carvalho de Melo
2021-07-15 16:07 ` [PATCH 06/20] perf dso: fix memory leak in dso__new_map Riccardo Mancini
2021-07-15 20:01 ` Arnaldo Carvalho de Melo
2021-07-15 20:07 ` Arnaldo Carvalho de Melo
2021-07-15 16:07 ` [PATCH 07/20] perf test: maps__merge_in: fix memory leak of maps Riccardo Mancini
2021-07-15 20:10 ` Arnaldo Carvalho de Melo
2021-07-15 16:07 ` [PATCH 08/20] perf env: fix memory leak of cpu_pmu_caps Riccardo Mancini
2021-07-15 20:12 ` Arnaldo Carvalho de Melo
2021-07-15 16:07 ` [PATCH 09/20] perf report: free generated help strings for sort option Riccardo Mancini
2021-07-15 20:15 ` Arnaldo Carvalho de Melo
2021-07-15 16:07 ` [PATCH 10/20] perf inject: close inject.output Riccardo Mancini
2021-07-15 20:17 ` Arnaldo Carvalho de Melo
2021-07-15 16:07 ` [PATCH 11/20] perf session: cleanup trace_event Riccardo Mancini
2021-07-15 20:21 ` Arnaldo Carvalho de Melo
2021-07-15 16:07 ` [PATCH 12/20] perf script: release zstd data Riccardo Mancini
2021-07-15 20:23 ` Arnaldo Carvalho de Melo
2021-07-15 16:07 ` [PATCH 13/20] perf script: fix memory leaks in perf_script Riccardo Mancini
2021-07-15 20:25 ` Arnaldo Carvalho de Melo
2021-07-15 16:07 ` [PATCH 14/20] perf util/lzma: close lzma stream Riccardo Mancini
2021-07-15 20:31 ` Arnaldo Carvalho de Melo
2021-07-15 16:07 ` [PATCH 15/20] perf trace: free malloc'd trace fields on exit Riccardo Mancini
2021-07-15 20:32 ` Arnaldo Carvalho de Melo
2021-07-15 16:07 ` [PATCH 16/20] perf trace: free syscall->arg_fmt Riccardo Mancini
2021-07-15 20:35 ` Arnaldo Carvalho de Melo
2021-07-15 16:07 ` [PATCH 17/20] perf trace: free syscall tp fields in evsel->priv Riccardo Mancini
2021-07-15 20:35 ` Arnaldo Carvalho de Melo
2021-07-15 16:07 ` [PATCH 18/20] perf trace: free strings in trace__parse_events_option Riccardo Mancini
2021-07-15 20:36 ` Arnaldo Carvalho de Melo
2021-07-15 16:07 ` [PATCH 19/20] perf test: bpf: free obj_buf Riccardo Mancini
2021-07-15 20:39 ` Arnaldo Carvalho de Melo
2021-07-16 16:50 ` Arnaldo Carvalho de Melo
2021-07-15 16:07 ` [PATCH 20/20] perf util/probe-file: delete namelist on error in del_events Riccardo Mancini
2021-07-15 20:41 ` Arnaldo Carvalho de Melo
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YddSQB8Sztuf053g@kernel.org \
--to=acme@kernel.org \
--cc=irogers@google.com \
--cc=jolsa@redhat.com \
--cc=kjlx@templeofstupid.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-perf-users@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=mingo@redhat.com \
--cc=namhyung@kernel.org \
--cc=peterz@infradead.org \
--cc=rickyman7@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).