[PATCH v2] perf unwind: Fix map reference counts

[PATCH v2] perf unwind: Fix map reference counts

[Ian Rogers]

The result of thread__find_map is the map in the passed in
addr_location. Calling addr_location__exit puts that map and so copies
need to do a map__get. Add in the corresponding map__puts.

Signed-off-by: Ian Rogers <irogers@google.com>

v2. Add missing map__put when dso is missing.
---
 tools/perf/util/unwind-libunwind-local.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/tools/perf/util/unwind-libunwind-local.c b/tools/perf/util/unwind-libunwind-local.c
index 36bf5100bad2..ebfde537b99b 100644
--- a/tools/perf/util/unwind-libunwind-local.c
+++ b/tools/perf/util/unwind-libunwind-local.c
@@ -419,7 +419,8 @@ static struct map *find_map(unw_word_t ip, struct unwind_info *ui)
 	struct map *ret;
 
 	addr_location__init(&al);
-	ret = thread__find_map(ui->thread, PERF_RECORD_MISC_USER, ip, &al);
+	thread__find_map(ui->thread, PERF_RECORD_MISC_USER, ip, &al);
+	ret = map__get(al.map);
 	addr_location__exit(&al);
 	return ret;
 }
@@ -440,8 +441,10 @@ find_proc_info(unw_addr_space_t as, unw_word_t ip, unw_proc_info_t *pi,
 		return -EINVAL;
 
 	dso = map__dso(map);
-	if (!dso)
+	if (!dso) {
+		map__put(map);
 		return -EINVAL;
+	}
 
 	pr_debug("unwind: find_proc_info dso %s\n", dso->name);
 
@@ -476,11 +479,11 @@ find_proc_info(unw_addr_space_t as, unw_word_t ip, unw_proc_info_t *pi,
 
 		memset(&di, 0, sizeof(di));
 		if (dwarf_find_debug_frame(0, &di, ip, base, symfile, start, map__end(map)))
-			return dwarf_search_unwind_table(as, ip, &di, pi,
-							 need_unwind_info, arg);
+			ret = dwarf_search_unwind_table(as, ip, &di, pi,
+							need_unwind_info, arg);
 	}
 #endif
-
+	map__put(map);
 	return ret;
 }
 
@@ -534,12 +537,14 @@ static int access_dso_mem(struct unwind_info *ui, unw_word_t addr,
 
 	dso = map__dso(map);
 
-	if (!dso)
+	if (!dso) {
+		map__put(map);
 		return -1;
+	}
 
 	size = dso__data_read_addr(dso, map, ui->machine,
 				   addr, (u8 *) data, sizeof(*data));
-
+	map__put(map);
 	return !(size == sizeof(*data));
 }
 
-- 
2.41.0.162.gfafddb0af9-goog

Re: [PATCH v2] perf unwind: Fix map reference counts

[Namhyung Kim]

On Thu, Jun 22, 2023 at 9:31 PM Ian Rogers <irogers@google.com> wrote:
>
> The result of thread__find_map is the map in the passed in
> addr_location. Calling addr_location__exit puts that map and so copies
> need to do a map__get. Add in the corresponding map__puts.
>
> Signed-off-by: Ian Rogers <irogers@google.com>

Acked-by: Namhyung Kim <namhyung@kernel.org>

Thanks,
Namhyung


>
> v2. Add missing map__put when dso is missing.
> ---
>  tools/perf/util/unwind-libunwind-local.c | 19 ++++++++++++-------
>  1 file changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/tools/perf/util/unwind-libunwind-local.c b/tools/perf/util/unwind-libunwind-local.c
> index 36bf5100bad2..ebfde537b99b 100644
> --- a/tools/perf/util/unwind-libunwind-local.c
> +++ b/tools/perf/util/unwind-libunwind-local.c
> @@ -419,7 +419,8 @@ static struct map *find_map(unw_word_t ip, struct unwind_info *ui)
>         struct map *ret;
>
>         addr_location__init(&al);
> -       ret = thread__find_map(ui->thread, PERF_RECORD_MISC_USER, ip, &al);
> +       thread__find_map(ui->thread, PERF_RECORD_MISC_USER, ip, &al);
> +       ret = map__get(al.map);
>         addr_location__exit(&al);
>         return ret;
>  }
> @@ -440,8 +441,10 @@ find_proc_info(unw_addr_space_t as, unw_word_t ip, unw_proc_info_t *pi,
>                 return -EINVAL;
>
>         dso = map__dso(map);
> -       if (!dso)
> +       if (!dso) {
> +               map__put(map);
>                 return -EINVAL;
> +       }
>
>         pr_debug("unwind: find_proc_info dso %s\n", dso->name);
>
> @@ -476,11 +479,11 @@ find_proc_info(unw_addr_space_t as, unw_word_t ip, unw_proc_info_t *pi,
>
>                 memset(&di, 0, sizeof(di));
>                 if (dwarf_find_debug_frame(0, &di, ip, base, symfile, start, map__end(map)))
> -                       return dwarf_search_unwind_table(as, ip, &di, pi,
> -                                                        need_unwind_info, arg);
> +                       ret = dwarf_search_unwind_table(as, ip, &di, pi,
> +                                                       need_unwind_info, arg);
>         }
>  #endif
> -
> +       map__put(map);
>         return ret;
>  }
>
> @@ -534,12 +537,14 @@ static int access_dso_mem(struct unwind_info *ui, unw_word_t addr,
>
>         dso = map__dso(map);
>
> -       if (!dso)
> +       if (!dso) {
> +               map__put(map);
>                 return -1;
> +       }
>
>         size = dso__data_read_addr(dso, map, ui->machine,
>                                    addr, (u8 *) data, sizeof(*data));
> -
> +       map__put(map);
>         return !(size == sizeof(*data));
>  }
>
> --
> 2.41.0.162.gfafddb0af9-goog
>

Re: [PATCH v2] perf unwind: Fix map reference counts

[Ian Rogers]

On Fri, Jun 23, 2023 at 9:18 AM Markus Elfring <Markus.Elfring@web.de> wrote:
>
> > v2. Add missing map__put when dso is missing.
> > ---
>
> Please omit such version information from the change suggestion.
>
> See also:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.4-rc7#n698

ah, tldr. Will correct in the future.

>
> How do you think about to add the tag “Fixes”?

In general we've not been adding Fixes as there is a danger a backport
will introduce a use-after-free.

Thanks,
Ian

> Regards,
> Markus

Re: [PATCH v2] perf unwind: Fix map reference counts

[Namhyung Kim]

Hello,

On Fri, Jun 23, 2023 at 10:49 AM Ian Rogers <irogers@google.com> wrote:
>
> On Fri, Jun 23, 2023 at 9:18 AM Markus Elfring <Markus.Elfring@web.de> wrote:
> >
> > > v2. Add missing map__put when dso is missing.
> > > ---
> >
> > Please omit such version information from the change suggestion.
> >
> > See also:
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.4-rc7#n698
>
> ah, tldr. Will correct in the future.

Thanks for taking a look at this.  I moved it above the tag lines
this time.

>
> >
> > How do you think about to add the tag “Fixes”?
>
> In general we've not been adding Fixes as there is a danger a backport
> will introduce a use-after-free.

Right, this change depends on other changes.  Simply cherry-picking
this will result in unmatched ref count IIUC.

Applied to perf-tools-next, thanks!

Re: [PATCH v2] perf unwind: Fix map reference counts

[Dan Carpenter]

On Fri, Jun 23, 2023 at 10:49:36AM -0700, Ian Rogers wrote:
> >
> > How do you think about to add the tag “Fixes”?
> 
> In general we've not been adding Fixes as there is a danger a backport
> will introduce a use-after-free.

I feel like we have been discussing issues around Perf backports
recently.  Wasn't there some build breakage that wasn't detected?  Why
not just ask Sasha to leave perf out of the -stable tree?

Also Sasha has a tag to explain that patch AAA is included because
patch BBB depends on it.  I feel like maybe those tags are backwards,
it would be nicer to tag AAA as depending on BBB.  That way we could
add the dependency tags here.

I think at Linaro we have recently been testing taking the latest Perf
tools and using them on older kernels.  I don't know the details around
why we can't just use the perf that ships with the kernel...

To tell the truth, I also don't really understand the problem for this
patch specifically. From what I can see, the Fixes tag would have been:

Fixes: 0dd5041c9a0e ("perf addr_location: Add init/exit/copy functions")

1) Adding a Fixes tag would have automatically prevented any backports.
2) I don't see any possible use after frees.  That probably means I have
identified the wrong Fixes tag?

I'm not going to dig further than that because I don't care.  I'm just
looking at it because Markus added kernel-janitors to the CC list.  But
for subsystems where I'm more involved then I always look at how a bug
is introduced.  That information is essential to me as a reviewer.  So
if I'm writing a patch and even if it's not a bug fix but let's say it
deletes dead code then I often include include the information under the
--- cut off line.

---
This dead code was introduced by commit 23423423 ("blah blah blah").

regards,
dan carpenter

Re: [PATCH v2] perf unwind: Fix map reference counts

[Ian Rogers]

On Mon, Jun 26, 2023 at 5:42 AM Dan Carpenter <dan.carpenter@linaro.org> wrote:
>
> On Fri, Jun 23, 2023 at 10:49:36AM -0700, Ian Rogers wrote:
> > >
> > > How do you think about to add the tag “Fixes”?
> >
> > In general we've not been adding Fixes as there is a danger a backport
> > will introduce a use-after-free.
>
> I feel like we have been discussing issues around Perf backports
> recently.  Wasn't there some build breakage that wasn't detected?  Why
> not just ask Sasha to leave perf out of the -stable tree?
>
> Also Sasha has a tag to explain that patch AAA is included because
> patch BBB depends on it.  I feel like maybe those tags are backwards,
> it would be nicer to tag AAA as depending on BBB.  That way we could
> add the dependency tags here.
>
> I think at Linaro we have recently been testing taking the latest Perf
> tools and using them on older kernels.  I don't know the details around
> why we can't just use the perf that ships with the kernel...

Using the perf tool that ships with the kernel should be fine but the
perf tool is backward compatible with older kernels. There are new
features, such as using BPF for kernel lock analysis, that are
available in the latest perf tool and so it could be nice to have
these available/tested on older kernels.

> To tell the truth, I also don't really understand the problem for this
> patch specifically. From what I can see, the Fixes tag would have been:
>
> Fixes: 0dd5041c9a0e ("perf addr_location: Add init/exit/copy functions")

No, the issue is fixing a long standing problem in the perf tool where
reference counting has been broken. We have implemented a poor man's
RAII using leak analysis and these patches stem from that, but the
errant code pre-dates it. Fwiw, more details on the reference count
checker is here:
https://perf.wiki.kernel.org/index.php/Reference_Count_Checking

> 1) Adding a Fixes tag would have automatically prevented any backports.
> 2) I don't see any possible use after frees.  That probably means I have
> identified the wrong Fixes tag?

You'd need tests that stress libunwind unwinding, such as "perf top
-g". Prior to the reference count checker work the code contained
unnecessary gets to avoid lowering reference counts and causing
use-after-free (a memory leak considered less bad than crashing). By
backporting the code you are taking part of the fix and potentially
creating a new problem.

Thanks,
Ian

> I'm not going to dig further than that because I don't care.  I'm just
> looking at it because Markus added kernel-janitors to the CC list.  But
> for subsystems where I'm more involved then I always look at how a bug
> is introduced.  That information is essential to me as a reviewer.  So
> if I'm writing a patch and even if it's not a bug fix but let's say it
> deletes dead code then I often include include the information under the
> --- cut off line.
>
> ---
> This dead code was introduced by commit 23423423 ("blah blah blah").
>
> regards,
> dan carpenter

Re: [PATCH v2] perf unwind: Fix map reference counts

[Dan Carpenter]

Ah, great.  Thanks, Ian.

The Reference Count Checking page is good information.  There is a bunch
of interest in doing better QC so this stuff is good to know.

So in this case, it's probably difficult to assign a Fixes tag and that's
fine.  Whatever.  But I'm not a huge fan of the "Leave off the Fixes tag"
approach to preventing backports.  There should be a better system for
that.  Maybe there should be a tag for that?  Or possibly that's too
complicated...  I'm not sure.

Anyway, let's leave it for now.  If it's really a problem then we'll run
into it again later and we can think about it at that point.

regards,
dan carpenter