2 memory fixes and a build fix

[PATCH v1 2/3] perf probe: Fix libdw memory leak

Posted by Ian Rogers 2 months ago

Add missing dwarf_cfi_end to free memory associated with probe_finder
cfi_eh or cfi_dbg. This addresses leak sanitizer issues seen in:
tools/perf/tests/shell/test_uprobe_from_different_cu.sh

Fixes: 270bde1e76f4 ("perf probe: Search both .eh_frame and .debug_frame sections for probe location")
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/probe-finder.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/tools/perf/util/probe-finder.c b/tools/perf/util/probe-finder.c
index 630e16c54ed5..78f34fa0c391 100644
--- a/tools/perf/util/probe-finder.c
+++ b/tools/perf/util/probe-finder.c
@@ -1379,6 +1379,11 @@ int debuginfo__find_trace_events(struct debuginfo *dbg,
 	if (ret >= 0 && tf.pf.skip_empty_arg)
 		ret = fill_empty_trace_arg(pev, tf.tevs, tf.ntevs);
 
+#if _ELFUTILS_PREREQ(0, 142)
+	dwarf_cfi_end(tf.pf.cfi_eh);
+	dwarf_cfi_end(tf.pf.cfi_dbg);
+#endif
+
 	if (ret < 0 || tf.ntevs == 0) {
 		for (i = 0; i < tf.ntevs; i++)
 			clear_probe_trace_event(&tf.tevs[i]);
-- 
2.46.0.792.g87dc391469-goog

Re: [PATCH v1 2/3] perf probe: Fix libdw memory leak

Posted by Namhyung Kim 1 month, 3 weeks ago

On Mon, Sep 23, 2024 at 5:37 PM Ian Rogers <irogers@google.com> wrote:
>
> Add missing dwarf_cfi_end to free memory associated with probe_finder
> cfi_eh or cfi_dbg. This addresses leak sanitizer issues seen in:
> tools/perf/tests/shell/test_uprobe_from_different_cu.sh
>
> Fixes: 270bde1e76f4 ("perf probe: Search both .eh_frame and .debug_frame sections for probe location")
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/util/probe-finder.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/tools/perf/util/probe-finder.c b/tools/perf/util/probe-finder.c
> index 630e16c54ed5..78f34fa0c391 100644
> --- a/tools/perf/util/probe-finder.c
> +++ b/tools/perf/util/probe-finder.c
> @@ -1379,6 +1379,11 @@ int debuginfo__find_trace_events(struct debuginfo *dbg,
>         if (ret >= 0 && tf.pf.skip_empty_arg)
>                 ret = fill_empty_trace_arg(pev, tf.tevs, tf.ntevs);
>
> +#if _ELFUTILS_PREREQ(0, 142)
> +       dwarf_cfi_end(tf.pf.cfi_eh);
> +       dwarf_cfi_end(tf.pf.cfi_dbg);
> +#endif

This is causing another problem.  Now vfs_getname tests are
failing because perf probe aborts.

Thanks,
Namhyung

> +
>         if (ret < 0 || tf.ntevs == 0) {
>                 for (i = 0; i < tf.ntevs; i++)
>                         clear_probe_trace_event(&tf.tevs[i]);
> --
> 2.46.0.792.g87dc391469-goog
>

Re: [PATCH v1 2/3] perf probe: Fix libdw memory leak

Posted by Ian Rogers 1 month, 3 weeks ago

On Wed, Oct 2, 2024 at 10:44 AM Namhyung Kim <namhyung@gmail.com> wrote:
>
> On Mon, Sep 23, 2024 at 5:37 PM Ian Rogers <irogers@google.com> wrote:
> >
> > Add missing dwarf_cfi_end to free memory associated with probe_finder
> > cfi_eh or cfi_dbg. This addresses leak sanitizer issues seen in:
> > tools/perf/tests/shell/test_uprobe_from_different_cu.sh
> >
> > Fixes: 270bde1e76f4 ("perf probe: Search both .eh_frame and .debug_frame sections for probe location")
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >  tools/perf/util/probe-finder.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/tools/perf/util/probe-finder.c b/tools/perf/util/probe-finder.c
> > index 630e16c54ed5..78f34fa0c391 100644
> > --- a/tools/perf/util/probe-finder.c
> > +++ b/tools/perf/util/probe-finder.c
> > @@ -1379,6 +1379,11 @@ int debuginfo__find_trace_events(struct debuginfo *dbg,
> >         if (ret >= 0 && tf.pf.skip_empty_arg)
> >                 ret = fill_empty_trace_arg(pev, tf.tevs, tf.ntevs);
> >
> > +#if _ELFUTILS_PREREQ(0, 142)
> > +       dwarf_cfi_end(tf.pf.cfi_eh);
> > +       dwarf_cfi_end(tf.pf.cfi_dbg);
> > +#endif
>
> This is causing another problem.  Now vfs_getname tests are
> failing because perf probe aborts.

I wasn't able to reproduce but largely as the test skips. The variable
is out of scope after the function so I'm struggling to see what the
issue is.

Thanks,
Ian

Re: [PATCH v1 2/3] perf probe: Fix libdw memory leak

Posted by Namhyung Kim 1 month, 3 weeks ago

On Wed, Oct 02, 2024 at 12:08:30PM -0700, Ian Rogers wrote:
> On Wed, Oct 2, 2024 at 10:44 AM Namhyung Kim <namhyung@gmail.com> wrote:
> >
> > On Mon, Sep 23, 2024 at 5:37 PM Ian Rogers <irogers@google.com> wrote:
> > >
> > > Add missing dwarf_cfi_end to free memory associated with probe_finder
> > > cfi_eh or cfi_dbg. This addresses leak sanitizer issues seen in:
> > > tools/perf/tests/shell/test_uprobe_from_different_cu.sh
> > >
> > > Fixes: 270bde1e76f4 ("perf probe: Search both .eh_frame and .debug_frame sections for probe location")
> > > Signed-off-by: Ian Rogers <irogers@google.com>
> > > ---
> > >  tools/perf/util/probe-finder.c | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > >
> > > diff --git a/tools/perf/util/probe-finder.c b/tools/perf/util/probe-finder.c
> > > index 630e16c54ed5..78f34fa0c391 100644
> > > --- a/tools/perf/util/probe-finder.c
> > > +++ b/tools/perf/util/probe-finder.c
> > > @@ -1379,6 +1379,11 @@ int debuginfo__find_trace_events(struct debuginfo *dbg,
> > >         if (ret >= 0 && tf.pf.skip_empty_arg)
> > >                 ret = fill_empty_trace_arg(pev, tf.tevs, tf.ntevs);
> > >
> > > +#if _ELFUTILS_PREREQ(0, 142)
> > > +       dwarf_cfi_end(tf.pf.cfi_eh);
> > > +       dwarf_cfi_end(tf.pf.cfi_dbg);
> > > +#endif
> >
> > This is causing another problem.  Now vfs_getname tests are
> > failing because perf probe aborts.
> 
> I wasn't able to reproduce but largely as the test skips. The variable
> is out of scope after the function so I'm struggling to see what the
> issue is.

I'm seeing this.

  $ sudo ./perf test -v vfs
   91: Add vfs_getname probe to get syscall args filenames:
  --- start ---
  test child forked, pid 3013362
  free(): invalid pointer
  linux/tools/perf/tests/shell/lib/probe_vfs_getname.sh: line 13: 3013380 Aborted                 perf probe -q "vfs_getname=getname_flags:${line} pathname=result->name:string"
  free(): invalid pointer
  linux/tools/perf/tests/shell/lib/probe_vfs_getname.sh: line 13: 3013381 Aborted                 perf probe $add_probe_verbose "vfs_getname=getname_flags:${line} pathname=filename:ustring"
  ---- end(-1) ----
   91: Add vfs_getname probe to get syscall args filenames             : FAILED!
   93: Use vfs_getname probe to get syscall args filenames:
  --- start ---
  test child forked, pid 3013479
  free(): invalid pointer
  linux/tools/perf/tests/shell/lib/probe_vfs_getname.sh: line 13: 3013502 Aborted                 perf probe -q "vfs_getname=getname_flags:${line} pathname=result->name:string"
  free(): invalid pointer
  linux/tools/perf/tests/shell/lib/probe_vfs_getname.sh: line 13: 3013514 Aborted                 perf probe $add_probe_verbose "vfs_getname=getname_flags:${line} pathname=filename:ustring"
  ---- end(-1) ----
   93: Use vfs_getname probe to get syscall args filenames             : FAILED!
  127: Check open filename arg using perf trace + vfs_getname:
  --- start ---
  test child forked, pid 3013528
  free(): invalid pointer
  linux/tools/perf/tests/shell/lib/probe_vfs_getname.sh: line 13: 3013547 Aborted                 perf probe -q "vfs_getname=getname_flags:${line} pathname=result->name:string"
  free(): invalid pointer
  linux/tools/perf/tests/shell/lib/probe_vfs_getname.sh: line 13: 3013548 Aborted                 perf probe $add_probe_verbose "vfs_getname=getname_flags:${line} pathname=filename:ustring"
  ---- end(-1) ----
  127: Check open filename arg using perf trace + vfs_getname          : FAILED!
  
Dropping the series from tmp.perf-tools-next for now.

Thanks,
Namhyung

Re: [PATCH v1 2/3] perf probe: Fix libdw memory leak

Posted by James Clark 2 months ago


On 24/09/2024 1:37 am, Ian Rogers wrote:
> Add missing dwarf_cfi_end to free memory associated with probe_finder
> cfi_eh or cfi_dbg. This addresses leak sanitizer issues seen in:
> tools/perf/tests/shell/test_uprobe_from_different_cu.sh
> 
> Fixes: 270bde1e76f4 ("perf probe: Search both .eh_frame and .debug_frame sections for probe location")
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>   tools/perf/util/probe-finder.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/tools/perf/util/probe-finder.c b/tools/perf/util/probe-finder.c
> index 630e16c54ed5..78f34fa0c391 100644
> --- a/tools/perf/util/probe-finder.c
> +++ b/tools/perf/util/probe-finder.c
> @@ -1379,6 +1379,11 @@ int debuginfo__find_trace_events(struct debuginfo *dbg,
>   	if (ret >= 0 && tf.pf.skip_empty_arg)
>   		ret = fill_empty_trace_arg(pev, tf.tevs, tf.ntevs);
>   
> +#if _ELFUTILS_PREREQ(0, 142)
> +	dwarf_cfi_end(tf.pf.cfi_eh);
> +	dwarf_cfi_end(tf.pf.cfi_dbg);
> +#endif
> +

I noticed that c06547d converted an _ELFUTILS_PREREQ(0, 142) into 
HAVE_DWARF_CFI_SUPPORT. But there is still a mixture of both in the code 
(unrelated to this patch). The commit message doesn't say why it is 
better, just that it could be changed, so I'm not sure which one is right.

Re: [PATCH v1 2/3] perf probe: Fix libdw memory leak

Posted by Namhyung Kim 2 months ago

On Tue, Sep 24, 2024 at 10:17:08AM +0100, James Clark wrote:
> 
> 
> On 24/09/2024 1:37 am, Ian Rogers wrote:
> > Add missing dwarf_cfi_end to free memory associated with probe_finder
> > cfi_eh or cfi_dbg. This addresses leak sanitizer issues seen in:
> > tools/perf/tests/shell/test_uprobe_from_different_cu.sh
> > 
> > Fixes: 270bde1e76f4 ("perf probe: Search both .eh_frame and .debug_frame sections for probe location")
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >   tools/perf/util/probe-finder.c | 5 +++++
> >   1 file changed, 5 insertions(+)
> > 
> > diff --git a/tools/perf/util/probe-finder.c b/tools/perf/util/probe-finder.c
> > index 630e16c54ed5..78f34fa0c391 100644
> > --- a/tools/perf/util/probe-finder.c
> > +++ b/tools/perf/util/probe-finder.c
> > @@ -1379,6 +1379,11 @@ int debuginfo__find_trace_events(struct debuginfo *dbg,
> >   	if (ret >= 0 && tf.pf.skip_empty_arg)
> >   		ret = fill_empty_trace_arg(pev, tf.tevs, tf.ntevs);
> > +#if _ELFUTILS_PREREQ(0, 142)
> > +	dwarf_cfi_end(tf.pf.cfi_eh);
> > +	dwarf_cfi_end(tf.pf.cfi_dbg);
> > +#endif
> > +
> 
> I noticed that c06547d converted an _ELFUTILS_PREREQ(0, 142) into
> HAVE_DWARF_CFI_SUPPORT. But there is still a mixture of both in the code
> (unrelated to this patch). The commit message doesn't say why it is better,
> just that it could be changed, so I'm not sure which one is right.

I think HAVE_DWARF_CFI_SUPPORT is better since it reveals the intention
clearly.

Thanks,
Namhyung

Re: [PATCH v1 2/3] perf probe: Fix libdw memory leak

Posted by Ian Rogers 2 months ago

On Tue, Sep 24, 2024 at 11:40 AM Namhyung Kim <namhyung@kernel.org> wrote:
>
> On Tue, Sep 24, 2024 at 10:17:08AM +0100, James Clark wrote:
> >
> >
> > On 24/09/2024 1:37 am, Ian Rogers wrote:
> > > Add missing dwarf_cfi_end to free memory associated with probe_finder
> > > cfi_eh or cfi_dbg. This addresses leak sanitizer issues seen in:
> > > tools/perf/tests/shell/test_uprobe_from_different_cu.sh
> > >
> > > Fixes: 270bde1e76f4 ("perf probe: Search both .eh_frame and .debug_frame sections for probe location")
> > > Signed-off-by: Ian Rogers <irogers@google.com>
> > > ---
> > >   tools/perf/util/probe-finder.c | 5 +++++
> > >   1 file changed, 5 insertions(+)
> > >
> > > diff --git a/tools/perf/util/probe-finder.c b/tools/perf/util/probe-finder.c
> > > index 630e16c54ed5..78f34fa0c391 100644
> > > --- a/tools/perf/util/probe-finder.c
> > > +++ b/tools/perf/util/probe-finder.c
> > > @@ -1379,6 +1379,11 @@ int debuginfo__find_trace_events(struct debuginfo *dbg,
> > >     if (ret >= 0 && tf.pf.skip_empty_arg)
> > >             ret = fill_empty_trace_arg(pev, tf.tevs, tf.ntevs);
> > > +#if _ELFUTILS_PREREQ(0, 142)
> > > +   dwarf_cfi_end(tf.pf.cfi_eh);
> > > +   dwarf_cfi_end(tf.pf.cfi_dbg);
> > > +#endif
> > > +
> >
> > I noticed that c06547d converted an _ELFUTILS_PREREQ(0, 142) into
> > HAVE_DWARF_CFI_SUPPORT. But there is still a mixture of both in the code
> > (unrelated to this patch). The commit message doesn't say why it is better,
> > just that it could be changed, so I'm not sure which one is right.
>
> I think HAVE_DWARF_CFI_SUPPORT is better since it reveals the intention
> clearly.

Let's just nuke the conditional compilation, the CFI support is in
libdw (calling it dwarf is just actively confusing) is 15 years old:
https://lore.kernel.org/lkml/20240924160418.1391100-7-irogers@google.com/
https://lore.kernel.org/lkml/20240924160418.1391100-8-irogers@google.com/

Thanks,
Ian

Re: [PATCH v1 2/3] perf probe: Fix libdw memory leak

Posted by Namhyung Kim 2 months ago

On Tue, Sep 24, 2024 at 12:47:33PM -0700, Ian Rogers wrote:
> On Tue, Sep 24, 2024 at 11:40 AM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > On Tue, Sep 24, 2024 at 10:17:08AM +0100, James Clark wrote:
> > >
> > >
> > > On 24/09/2024 1:37 am, Ian Rogers wrote:
> > > > Add missing dwarf_cfi_end to free memory associated with probe_finder
> > > > cfi_eh or cfi_dbg. This addresses leak sanitizer issues seen in:
> > > > tools/perf/tests/shell/test_uprobe_from_different_cu.sh
> > > >
> > > > Fixes: 270bde1e76f4 ("perf probe: Search both .eh_frame and .debug_frame sections for probe location")
> > > > Signed-off-by: Ian Rogers <irogers@google.com>
> > > > ---
> > > >   tools/perf/util/probe-finder.c | 5 +++++
> > > >   1 file changed, 5 insertions(+)
> > > >
> > > > diff --git a/tools/perf/util/probe-finder.c b/tools/perf/util/probe-finder.c
> > > > index 630e16c54ed5..78f34fa0c391 100644
> > > > --- a/tools/perf/util/probe-finder.c
> > > > +++ b/tools/perf/util/probe-finder.c
> > > > @@ -1379,6 +1379,11 @@ int debuginfo__find_trace_events(struct debuginfo *dbg,
> > > >     if (ret >= 0 && tf.pf.skip_empty_arg)
> > > >             ret = fill_empty_trace_arg(pev, tf.tevs, tf.ntevs);
> > > > +#if _ELFUTILS_PREREQ(0, 142)
> > > > +   dwarf_cfi_end(tf.pf.cfi_eh);
> > > > +   dwarf_cfi_end(tf.pf.cfi_dbg);
> > > > +#endif
> > > > +
> > >
> > > I noticed that c06547d converted an _ELFUTILS_PREREQ(0, 142) into
> > > HAVE_DWARF_CFI_SUPPORT. But there is still a mixture of both in the code
> > > (unrelated to this patch). The commit message doesn't say why it is better,
> > > just that it could be changed, so I'm not sure which one is right.
> >
> > I think HAVE_DWARF_CFI_SUPPORT is better since it reveals the intention
> > clearly.
> 
> Let's just nuke the conditional compilation, the CFI support is in
> libdw (calling it dwarf is just actively confusing) is 15 years old:
> https://lore.kernel.org/lkml/20240924160418.1391100-7-irogers@google.com/
> https://lore.kernel.org/lkml/20240924160418.1391100-8-irogers@google.com/

Sounds good as long as we checks the version or the API in the feature
test.

Thanks,
Namhyung