[PATCH] perf report: Fix a memory leak for perf_env on AMD

[PATCH] perf report: Fix a memory leak for perf_env on AMD

[Namhyung Kim]

The env.pmu_mapping can be leaked when it reads data from a pipe on AMD.
For a pipe data, it reads the header data including pmu_mapping from
PERF_RECORD_HEADER_FEATURE runtime.  But it's already set in:

  perf_session__new()
    __perf_session__new()
      evlist__init_trace_event_sample_raw()
        evlist__has_amd_ibs()
          perf_env__nr_pmu_mappings()

Then it'll overwrite that when it processes the HEADER_FEATURE record.
Here's a report from address sanitizer.

  Direct leak of 2689 byte(s) in 1 object(s) allocated from:
    #0 0x7fed8f814596 in realloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:98
    #1 0x5595a7d416b1 in strbuf_grow util/strbuf.c:64
    #2 0x5595a7d414ef in strbuf_init util/strbuf.c:25
    #3 0x5595a7d0f4b7 in perf_env__read_pmu_mappings util/env.c:362
    #4 0x5595a7d12ab7 in perf_env__nr_pmu_mappings util/env.c:517
    #5 0x5595a7d89d2f in evlist__has_amd_ibs util/amd-sample-raw.c:315
    #6 0x5595a7d87fb2 in evlist__init_trace_event_sample_raw util/sample-raw.c:23
    #7 0x5595a7d7f893 in __perf_session__new util/session.c:179
    #8 0x5595a7b79572 in perf_session__new util/session.h:115
    #9 0x5595a7b7e9dc in cmd_report builtin-report.c:1603
    #10 0x5595a7c019eb in run_builtin perf.c:351
    #11 0x5595a7c01c92 in handle_internal_command perf.c:404
    #12 0x5595a7c01deb in run_argv perf.c:448
    #13 0x5595a7c02134 in main perf.c:556
    #14 0x7fed85833d67 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58

Let's free the existing pmu_mapping data if any.

Cc: Ravi Bangoria <ravi.bangoria@amd.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/header.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index 1900965f87527948..e3cdc3b7b4ab2409 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -2770,6 +2770,8 @@ static int process_pmu_mappings(struct feat_fd *ff, void *data __maybe_unused)
 		free(name);
 		pmu_num--;
 	}
+	/* AMD may set it by evlist__has_amd_ibs() from perf_session__new() */
+	free(ff->ph->env.pmu_mappings);
 	ff->ph->env.pmu_mappings = strbuf_detach(&sb, NULL);
 	return 0;
 
-- 
2.49.0.rc0.332.g42c0ae87b1-goog

Re: [PATCH] perf report: Fix a memory leak for perf_env on AMD

[Ian Rogers]

On Mon, Mar 10, 2025 at 5:04 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> The env.pmu_mapping can be leaked when it reads data from a pipe on AMD.
> For a pipe data, it reads the header data including pmu_mapping from
> PERF_RECORD_HEADER_FEATURE runtime.  But it's already set in:
>
>   perf_session__new()
>     __perf_session__new()
>       evlist__init_trace_event_sample_raw()
>         evlist__has_amd_ibs()
>           perf_env__nr_pmu_mappings()
>
> Then it'll overwrite that when it processes the HEADER_FEATURE record.
> Here's a report from address sanitizer.
>
>   Direct leak of 2689 byte(s) in 1 object(s) allocated from:
>     #0 0x7fed8f814596 in realloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:98
>     #1 0x5595a7d416b1 in strbuf_grow util/strbuf.c:64
>     #2 0x5595a7d414ef in strbuf_init util/strbuf.c:25
>     #3 0x5595a7d0f4b7 in perf_env__read_pmu_mappings util/env.c:362
>     #4 0x5595a7d12ab7 in perf_env__nr_pmu_mappings util/env.c:517
>     #5 0x5595a7d89d2f in evlist__has_amd_ibs util/amd-sample-raw.c:315
>     #6 0x5595a7d87fb2 in evlist__init_trace_event_sample_raw util/sample-raw.c:23
>     #7 0x5595a7d7f893 in __perf_session__new util/session.c:179
>     #8 0x5595a7b79572 in perf_session__new util/session.h:115
>     #9 0x5595a7b7e9dc in cmd_report builtin-report.c:1603
>     #10 0x5595a7c019eb in run_builtin perf.c:351
>     #11 0x5595a7c01c92 in handle_internal_command perf.c:404
>     #12 0x5595a7c01deb in run_argv perf.c:448
>     #13 0x5595a7c02134 in main perf.c:556
>     #14 0x7fed85833d67 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
>
> Let's free the existing pmu_mapping data if any.

Can we simplify `evlist__has_amd_ibs` as there seems no reason to be
creating/parsing strings. For example:
```
bool evlist__has_amd_ibs(struct evlist *evlist)
{
   return perf_pmus__find("ibs_op") != NULL ||
perf_pmus__find("ibs_fetch") != NULL;
}
```

Similarly the variables `ibs_fetch_type` and `ibs_op_type` can be
found with `perf_pmus__find("ibs_op")->type` and
`perf_pmus__find("ibs_op")->type` respectively. It probably makes more
sense to just cache access to the PMUs rather than do the finds all
the time.

I wonder there is a problem with the pmus alternative that currently
the pmus are global and not part of say the session, not loaded from
the perf.data file, etc. So use on a different machine than the
recording could be broken by what I suggest. I guess then, just
throwing in the free is simplest.

Thanks,
Ian



> Cc: Ravi Bangoria <ravi.bangoria@amd.com>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/util/header.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> index 1900965f87527948..e3cdc3b7b4ab2409 100644
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
> @@ -2770,6 +2770,8 @@ static int process_pmu_mappings(struct feat_fd *ff, void *data __maybe_unused)
>                 free(name);
>                 pmu_num--;
>         }
> +       /* AMD may set it by evlist__has_amd_ibs() from perf_session__new() */
> +       free(ff->ph->env.pmu_mappings);
>         ff->ph->env.pmu_mappings = strbuf_detach(&sb, NULL);
>         return 0;
>
> --
> 2.49.0.rc0.332.g42c0ae87b1-goog
>

Re: [PATCH] perf report: Fix a memory leak for perf_env on AMD

[Namhyung Kim]

On Mon, Mar 10, 2025 at 05:49:48PM -0700, Ian Rogers wrote:
> On Mon, Mar 10, 2025 at 5:04 PM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > The env.pmu_mapping can be leaked when it reads data from a pipe on AMD.
> > For a pipe data, it reads the header data including pmu_mapping from
> > PERF_RECORD_HEADER_FEATURE runtime.  But it's already set in:
> >
> >   perf_session__new()
> >     __perf_session__new()
> >       evlist__init_trace_event_sample_raw()
> >         evlist__has_amd_ibs()
> >           perf_env__nr_pmu_mappings()
> >
> > Then it'll overwrite that when it processes the HEADER_FEATURE record.
> > Here's a report from address sanitizer.
> >
> >   Direct leak of 2689 byte(s) in 1 object(s) allocated from:
> >     #0 0x7fed8f814596 in realloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:98
> >     #1 0x5595a7d416b1 in strbuf_grow util/strbuf.c:64
> >     #2 0x5595a7d414ef in strbuf_init util/strbuf.c:25
> >     #3 0x5595a7d0f4b7 in perf_env__read_pmu_mappings util/env.c:362
> >     #4 0x5595a7d12ab7 in perf_env__nr_pmu_mappings util/env.c:517
> >     #5 0x5595a7d89d2f in evlist__has_amd_ibs util/amd-sample-raw.c:315
> >     #6 0x5595a7d87fb2 in evlist__init_trace_event_sample_raw util/sample-raw.c:23
> >     #7 0x5595a7d7f893 in __perf_session__new util/session.c:179
> >     #8 0x5595a7b79572 in perf_session__new util/session.h:115
> >     #9 0x5595a7b7e9dc in cmd_report builtin-report.c:1603
> >     #10 0x5595a7c019eb in run_builtin perf.c:351
> >     #11 0x5595a7c01c92 in handle_internal_command perf.c:404
> >     #12 0x5595a7c01deb in run_argv perf.c:448
> >     #13 0x5595a7c02134 in main perf.c:556
> >     #14 0x7fed85833d67 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
> >
> > Let's free the existing pmu_mapping data if any.
> 
> Can we simplify `evlist__has_amd_ibs` as there seems no reason to be
> creating/parsing strings. For example:
> ```
> bool evlist__has_amd_ibs(struct evlist *evlist)
> {
>    return perf_pmus__find("ibs_op") != NULL ||
> perf_pmus__find("ibs_fetch") != NULL;
> }
> ```

I think the idea is to find the pmu from the recorded data file not 
at runtime. 


> Similarly the variables `ibs_fetch_type` and `ibs_op_type` can be
> found with `perf_pmus__find("ibs_op")->type` and
> `perf_pmus__find("ibs_op")->type` respectively. It probably makes more
> sense to just cache access to the PMUs rather than do the finds all
> the time.
> 
> I wonder there is a problem with the pmus alternative that currently
> the pmus are global and not part of say the session, not loaded from
> the perf.data file, etc. So use on a different machine than the
> recording could be broken by what I suggest. I guess then, just
> throwing in the free is simplest.

Agreed.  It could be saved in a session or env.

For now, let's free the string to fix the leak quickly.

Thanks,
Namhyung

> 
> 
> > Cc: Ravi Bangoria <ravi.bangoria@amd.com>
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > ---
> >  tools/perf/util/header.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> > index 1900965f87527948..e3cdc3b7b4ab2409 100644
> > --- a/tools/perf/util/header.c
> > +++ b/tools/perf/util/header.c
> > @@ -2770,6 +2770,8 @@ static int process_pmu_mappings(struct feat_fd *ff, void *data __maybe_unused)
> >                 free(name);
> >                 pmu_num--;
> >         }
> > +       /* AMD may set it by evlist__has_amd_ibs() from perf_session__new() */
> > +       free(ff->ph->env.pmu_mappings);
> >         ff->ph->env.pmu_mappings = strbuf_detach(&sb, NULL);
> >         return 0;
> >
> > --
> > 2.49.0.rc0.332.g42c0ae87b1-goog
> >

Re: [PATCH] perf report: Fix a memory leak for perf_env on AMD

[Namhyung Kim]

On Mon, 10 Mar 2025 17:04:16 -0700, Namhyung Kim wrote:
> The env.pmu_mapping can be leaked when it reads data from a pipe on AMD.
> For a pipe data, it reads the header data including pmu_mapping from
> PERF_RECORD_HEADER_FEATURE runtime.  But it's already set in:
> 
>   perf_session__new()
>     __perf_session__new()
>       evlist__init_trace_event_sample_raw()
>         evlist__has_amd_ibs()
>           perf_env__nr_pmu_mappings()
> 
> [...]
Applied to perf-tools-next, thanks!

Best regards,
Namhyung