[PATCHv5] perf tool: fix dereferencing NULL al->maps

[PATCHv5] perf tool: fix dereferencing NULL al->maps

[Casey Chen]

With 0dd5041c9a0e ("perf addr_location: Add init/exit/copy functions"),
when cpumode is 3 (macro PERF_RECORD_MISC_HYPERVISOR),
thread__find_map() could return with al->maps being NULL.

The path below could add a callchain_cursor_node with NULL ms.maps.

add_callchain_ip()
  thread__find_symbol(.., &al)
    thread__find_map(.., &al)   // al->maps becomes NULL
  ms.maps = maps__get(al.maps)
  callchain_cursor_append(..., &ms, ...)
    node->ms.maps = maps__get(ms->maps)

Then the path below would dereference NULL maps and get segfault.

fill_callchain_info()
  maps__machine(node->ms.maps);

Fix it by checking if maps is NULL in fill_callchain_info().

Signed-off-by: Casey Chen <cachen@purestorage.com>
Reviewed-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/callchain.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/util/callchain.c b/tools/perf/util/callchain.c
index 1730b852a947..6d075648d2cc 100644
--- a/tools/perf/util/callchain.c
+++ b/tools/perf/util/callchain.c
@@ -1141,7 +1141,7 @@ int hist_entry__append_callchain(struct hist_entry *he, struct perf_sample *samp
 int fill_callchain_info(struct addr_location *al, struct callchain_cursor_node *node,
 			bool hide_unresolved)
 {
-	struct machine *machine = maps__machine(node->ms.maps);
+	struct machine *machine = node->ms.maps ? maps__machine(node->ms.maps) : NULL;
 
 	maps__put(al->maps);
 	al->maps = maps__get(node->ms.maps);
-- 
2.45.2

Re: [PATCHv5] perf tool: fix dereferencing NULL al->maps

[Casey Chen]

Ian / Namhyung,

Could you take a look at the latest diff PATCHv5 ?

Thanks,
Casey

On Mon, Jul 22, 2024 at 2:15 PM Casey Chen <cachen@purestorage.com> wrote:
>
> With 0dd5041c9a0e ("perf addr_location: Add init/exit/copy functions"),
> when cpumode is 3 (macro PERF_RECORD_MISC_HYPERVISOR),
> thread__find_map() could return with al->maps being NULL.
>
> The path below could add a callchain_cursor_node with NULL ms.maps.
>
> add_callchain_ip()
>   thread__find_symbol(.., &al)
>     thread__find_map(.., &al)   // al->maps becomes NULL
>   ms.maps = maps__get(al.maps)
>   callchain_cursor_append(..., &ms, ...)
>     node->ms.maps = maps__get(ms->maps)
>
> Then the path below would dereference NULL maps and get segfault.
>
> fill_callchain_info()
>   maps__machine(node->ms.maps);
>
> Fix it by checking if maps is NULL in fill_callchain_info().
>
> Signed-off-by: Casey Chen <cachen@purestorage.com>
> Reviewed-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/util/callchain.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/perf/util/callchain.c b/tools/perf/util/callchain.c
> index 1730b852a947..6d075648d2cc 100644
> --- a/tools/perf/util/callchain.c
> +++ b/tools/perf/util/callchain.c
> @@ -1141,7 +1141,7 @@ int hist_entry__append_callchain(struct hist_entry *he, struct perf_sample *samp
>  int fill_callchain_info(struct addr_location *al, struct callchain_cursor_node *node,
>                         bool hide_unresolved)
>  {
> -       struct machine *machine = maps__machine(node->ms.maps);
> +       struct machine *machine = node->ms.maps ? maps__machine(node->ms.maps) : NULL;
>
>         maps__put(al->maps);
>         al->maps = maps__get(node->ms.maps);
> --
> 2.45.2
>

Re: [PATCHv5] perf tool: fix dereferencing NULL al->maps

[Namhyung Kim]

Hello,

On Tue, Jul 23, 2024 at 6:01 PM Casey Chen <cachen@purestorage.com> wrote:
>
> Ian / Namhyung,
>
> Could you take a look at the latest diff PATCHv5 ?
>
> Thanks,
> Casey
>
> On Mon, Jul 22, 2024 at 2:15 PM Casey Chen <cachen@purestorage.com> wrote:
> >
> > With 0dd5041c9a0e ("perf addr_location: Add init/exit/copy functions"),
> > when cpumode is 3 (macro PERF_RECORD_MISC_HYPERVISOR),
> > thread__find_map() could return with al->maps being NULL.
> >
> > The path below could add a callchain_cursor_node with NULL ms.maps.
> >
> > add_callchain_ip()
> >   thread__find_symbol(.., &al)
> >     thread__find_map(.., &al)   // al->maps becomes NULL
> >   ms.maps = maps__get(al.maps)
> >   callchain_cursor_append(..., &ms, ...)
> >     node->ms.maps = maps__get(ms->maps)
> >
> > Then the path below would dereference NULL maps and get segfault.
> >
> > fill_callchain_info()
> >   maps__machine(node->ms.maps);
> >
> > Fix it by checking if maps is NULL in fill_callchain_info().
> >
> > Signed-off-by: Casey Chen <cachen@purestorage.com>
> > Reviewed-by: Ian Rogers <irogers@google.com>

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

Thanks,
Namhyung


> > ---
> >  tools/perf/util/callchain.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/tools/perf/util/callchain.c b/tools/perf/util/callchain.c
> > index 1730b852a947..6d075648d2cc 100644
> > --- a/tools/perf/util/callchain.c
> > +++ b/tools/perf/util/callchain.c
> > @@ -1141,7 +1141,7 @@ int hist_entry__append_callchain(struct hist_entry *he, struct perf_sample *samp
> >  int fill_callchain_info(struct addr_location *al, struct callchain_cursor_node *node,
> >                         bool hide_unresolved)
> >  {
> > -       struct machine *machine = maps__machine(node->ms.maps);
> > +       struct machine *machine = node->ms.maps ? maps__machine(node->ms.maps) : NULL;
> >
> >         maps__put(al->maps);
> >         al->maps = maps__get(node->ms.maps);
> > --
> > 2.45.2
> >

Re: [PATCHv5] perf tool: fix dereferencing NULL al->maps

[Casey Chen]

On Wed, Jul 24, 2024 at 9:19 AM Namhyung Kim <namhyung@kernel.org> wrote:
>
> Hello,
>
> On Tue, Jul 23, 2024 at 6:01 PM Casey Chen <cachen@purestorage.com> wrote:
> >
> > Ian / Namhyung,
> >
> > Could you take a look at the latest diff PATCHv5 ?
> >
> > Thanks,
> > Casey
> >
> > On Mon, Jul 22, 2024 at 2:15 PM Casey Chen <cachen@purestorage.com> wrote:
> > >
> > > With 0dd5041c9a0e ("perf addr_location: Add init/exit/copy functions"),
> > > when cpumode is 3 (macro PERF_RECORD_MISC_HYPERVISOR),
> > > thread__find_map() could return with al->maps being NULL.
> > >
> > > The path below could add a callchain_cursor_node with NULL ms.maps.
> > >
> > > add_callchain_ip()
> > >   thread__find_symbol(.., &al)
> > >     thread__find_map(.., &al)   // al->maps becomes NULL
> > >   ms.maps = maps__get(al.maps)
> > >   callchain_cursor_append(..., &ms, ...)
> > >     node->ms.maps = maps__get(ms->maps)
> > >
> > > Then the path below would dereference NULL maps and get segfault.
> > >
> > > fill_callchain_info()
> > >   maps__machine(node->ms.maps);
> > >
> > > Fix it by checking if maps is NULL in fill_callchain_info().
> > >
> > > Signed-off-by: Casey Chen <cachen@purestorage.com>
> > > Reviewed-by: Ian Rogers <irogers@google.com>
>
> Acked-by: Namhyung Kim <namhyung@kernel.org>
>
> Thanks,
> Namhyung
>
>
> > > ---
> > >  tools/perf/util/callchain.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/tools/perf/util/callchain.c b/tools/perf/util/callchain.c
> > > index 1730b852a947..6d075648d2cc 100644
> > > --- a/tools/perf/util/callchain.c
> > > +++ b/tools/perf/util/callchain.c
> > > @@ -1141,7 +1141,7 @@ int hist_entry__append_callchain(struct hist_entry *he, struct perf_sample *samp
> > >  int fill_callchain_info(struct addr_location *al, struct callchain_cursor_node *node,
> > >                         bool hide_unresolved)
> > >  {
> > > -       struct machine *machine = maps__machine(node->ms.maps);
> > > +       struct machine *machine = node->ms.maps ? maps__machine(node->ms.maps) : NULL;
> > >
> > >         maps__put(al->maps);
> > >         al->maps = maps__get(node->ms.maps);
> > > --
> > > 2.45.2
> > >

Thanks Namhyung.
I have another question. When will this patch get merged into master
branch or 6.6 release line ? Our benchmark systems depend on this fix
to do performance analysis. Currently, both our kernel and perf are on
6.6.9 and they build separately. We want to update perf hash without
patching it locally.

Casey

Re: [PATCHv5] perf tool: fix dereferencing NULL al->maps

[Namhyung Kim]

On Wed, Jul 24, 2024 at 11:51:44AM -0700, Casey Chen wrote:
> On Wed, Jul 24, 2024 at 9:19 AM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > Hello,
> >
> > On Tue, Jul 23, 2024 at 6:01 PM Casey Chen <cachen@purestorage.com> wrote:
> > >
> > > Ian / Namhyung,
> > >
> > > Could you take a look at the latest diff PATCHv5 ?
> > >
> > > Thanks,
> > > Casey
> > >
> > > On Mon, Jul 22, 2024 at 2:15 PM Casey Chen <cachen@purestorage.com> wrote:
> > > >
> > > > With 0dd5041c9a0e ("perf addr_location: Add init/exit/copy functions"),
> > > > when cpumode is 3 (macro PERF_RECORD_MISC_HYPERVISOR),
> > > > thread__find_map() could return with al->maps being NULL.
> > > >
> > > > The path below could add a callchain_cursor_node with NULL ms.maps.
> > > >
> > > > add_callchain_ip()
> > > >   thread__find_symbol(.., &al)
> > > >     thread__find_map(.., &al)   // al->maps becomes NULL
> > > >   ms.maps = maps__get(al.maps)
> > > >   callchain_cursor_append(..., &ms, ...)
> > > >     node->ms.maps = maps__get(ms->maps)
> > > >
> > > > Then the path below would dereference NULL maps and get segfault.
> > > >
> > > > fill_callchain_info()
> > > >   maps__machine(node->ms.maps);
> > > >
> > > > Fix it by checking if maps is NULL in fill_callchain_info().
> > > >
> > > > Signed-off-by: Casey Chen <cachen@purestorage.com>
> > > > Reviewed-by: Ian Rogers <irogers@google.com>
> >
> > Acked-by: Namhyung Kim <namhyung@kernel.org>
> >
> > Thanks,
> > Namhyung
> >
> >
> > > > ---
> > > >  tools/perf/util/callchain.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/tools/perf/util/callchain.c b/tools/perf/util/callchain.c
> > > > index 1730b852a947..6d075648d2cc 100644
> > > > --- a/tools/perf/util/callchain.c
> > > > +++ b/tools/perf/util/callchain.c
> > > > @@ -1141,7 +1141,7 @@ int hist_entry__append_callchain(struct hist_entry *he, struct perf_sample *samp
> > > >  int fill_callchain_info(struct addr_location *al, struct callchain_cursor_node *node,
> > > >                         bool hide_unresolved)
> > > >  {
> > > > -       struct machine *machine = maps__machine(node->ms.maps);
> > > > +       struct machine *machine = node->ms.maps ? maps__machine(node->ms.maps) : NULL;
> > > >
> > > >         maps__put(al->maps);
> > > >         al->maps = maps__get(node->ms.maps);
> > > > --
> > > > 2.45.2
> > > >
> 
> Thanks Namhyung.
> I have another question. When will this patch get merged into master
> branch or 6.6 release line ? Our benchmark systems depend on this fix
> to do performance analysis. Currently, both our kernel and perf are on
> 6.6.9 and they build separately. We want to update perf hash without
> patching it locally.

I'll route it to v6.11 through perf-tools tree.  Hopefully it'd get
backported to stable kernels later.

Thanks,
Namhyung

Re: [PATCHv5] perf tool: fix dereferencing NULL al->maps

[Arnaldo Carvalho de Melo]

On Mon, Jul 22, 2024 at 03:15:48PM -0600, Casey Chen wrote:
> With 0dd5041c9a0e ("perf addr_location: Add init/exit/copy functions"),
> when cpumode is 3 (macro PERF_RECORD_MISC_HYPERVISOR),
> thread__find_map() could return with al->maps being NULL.
> 
> The path below could add a callchain_cursor_node with NULL ms.maps.
> 
> add_callchain_ip()
>   thread__find_symbol(.., &al)
>     thread__find_map(.., &al)   // al->maps becomes NULL
>   ms.maps = maps__get(al.maps)
>   callchain_cursor_append(..., &ms, ...)
>     node->ms.maps = maps__get(ms->maps)
> 
> Then the path below would dereference NULL maps and get segfault.
> 
> fill_callchain_info()
>   maps__machine(node->ms.maps);
> 
> Fix it by checking if maps is NULL in fill_callchain_info().
> 
> Signed-off-by: Casey Chen <cachen@purestorage.com>
> Reviewed-by: Ian Rogers <irogers@google.com>

Reviewed-by: Arnaldo Carvalho de Melo <acme@redhat.com>

- Arnaldo

> ---
>  tools/perf/util/callchain.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/callchain.c b/tools/perf/util/callchain.c
> index 1730b852a947..6d075648d2cc 100644
> --- a/tools/perf/util/callchain.c
> +++ b/tools/perf/util/callchain.c
> @@ -1141,7 +1141,7 @@ int hist_entry__append_callchain(struct hist_entry *he, struct perf_sample *samp
>  int fill_callchain_info(struct addr_location *al, struct callchain_cursor_node *node,
>  			bool hide_unresolved)
>  {
> -	struct machine *machine = maps__machine(node->ms.maps);
> +	struct machine *machine = node->ms.maps ? maps__machine(node->ms.maps) : NULL;
>  
>  	maps__put(al->maps);
>  	al->maps = maps__get(node->ms.maps);
> -- 
> 2.45.2
>