linux-kernel - Re: [PATCH 1/1] perf tool: fix handling NULL al->maps returned from thread__find

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZpGFfjebaW9LGNBo@google.com>
Date: Fri, 12 Jul 2024 14:35:26 -0500
From: Namhyung Kim <namhyung@...nel.org>
To: Casey Chen <cachen@...estorage.com>
Cc: linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org,
	yzhong@...estorage.com
Subject: Re: [PATCH 1/1] perf tool: fix handling NULL al->maps returned from
 thread__find_map

Hello,

On Wed, Jul 10, 2024 at 03:29:27PM -0700, Casey Chen wrote:
> On Mon, Jul 8, 2024 at 10:01 PM Namhyung Kim <namhyung@...nel.org> wrote:
> >
> > Hello,
> >
> > On Mon, Jul 8, 2024 at 4:23 PM Casey Chen <cachen@...estorage.com> wrote:
> > >
> > > With 0dd5041c9a0e ("perf addr_location: Add init/exit/copy functions"),
> > > thread__find_map() would return with al->maps or al->map being NULL
> > > when cpumode is 3 (macro PERF_RECORD_MISC_HYPERVISOR),
> > > later deferencing on it would crash.
> > >
> > > Fix callers of thread__find_map() or thread__find_symbol() to handle
> > > this.
> >
> > It looks like you drop the callchain if it doesn't find a map/symbol.
> > Can we keep the entries with raw hex numbers instead?
> >
> In add_callchain_ip(), my change let it return if either al.maps is
> NULL or al.map is NULL after thread__find_symbol(), I'm not sure what
> else can add_callchain_ip() could do to keep raw hex numbers. If it
> proceeds, al.sym is NULL, the code inside 'if (al.sym != NULL)' would
> skip. callchain_srcline() would return NULL. chain_cursor_append()
> would append a node whose ms.maps/ ms.map are NULL. Later
> dereferencing them would cause trouble. But we could add other
> information to the node, like ip, branch, nr_loop_iter, iter_cycles,
> branch_from, are these information good to have ? but how to avoid
> dereferencing NULL maps/map later.

By checking if it's NULL?  I think it's normal to have NULL map or sym
due to missing events, stripped binaries and so on.  The callchain code
used to print raw ip address when it doesn't have symbols.  And srcline
can/should do the same.

Thanks,
Namhyung

> >
> > > ---
> > >  tools/perf/arch/powerpc/util/skip-callchain-idx.c | 10 ++++++----
> > >  tools/perf/util/machine.c                         |  5 +++++
> > >  tools/perf/util/unwind-libdw.c                    |  6 ++++--
> > >  3 files changed, 15 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/tools/perf/arch/powerpc/util/skip-callchain-idx.c b/tools/perf/arch/powerpc/util/skip-callchain-idx.c
> > > index 5f3edb3004d8..25b0804df4c4 100644
> > > --- a/tools/perf/arch/powerpc/util/skip-callchain-idx.c
> > > +++ b/tools/perf/arch/powerpc/util/skip-callchain-idx.c
> > > @@ -255,13 +255,14 @@ int arch_skip_callchain_idx(struct thread *thread, struct ip_callchain *chain)
> > >
> > >         thread__find_symbol(thread, PERF_RECORD_MISC_USER, ip, &al);
> > >
> > > -       if (al.map)
> > > -               dso = map__dso(al.map);
> > > +       if (!al.map)
> > > +               goto out;
> > > +
> > > +       dso = map__dso(al.map);
> > >
> > >         if (!dso) {
> > >                 pr_debug("%" PRIx64 " dso is NULL\n", ip);
> > > -               addr_location__exit(&al);
> > > -               return skip_slot;
> > > +               goto out;
> > >         }
> > >
> > >         rc = check_return_addr(dso, map__start(al.map), ip);
> > > @@ -282,6 +283,7 @@ int arch_skip_callchain_idx(struct thread *thread, struct ip_callchain *chain)
> > >                 skip_slot = 3;
> > >         }
> > >
> > > +out:
> > >         addr_location__exit(&al);
> > >         return skip_slot;
> > >  }
> > > diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> > > index 8477edefc299..fa4037d7f3d4 100644
> > > --- a/tools/perf/util/machine.c
> > > +++ b/tools/perf/util/machine.c
> > > @@ -2098,7 +2098,12 @@ static int add_callchain_ip(struct thread *thread,
> > >                         }
> > >                         goto out;
> > >                 }
> > > +
> > >                 thread__find_symbol(thread, *cpumode, ip, &al);
> > > +               if (!al.maps || !al.map) {
> > > +                       err = 1;
> > > +                       goto out;
> > > +               }
> > >         }
> > >
> > >         if (al.sym != NULL) {
> > > diff --git a/tools/perf/util/unwind-libdw.c b/tools/perf/util/unwind-libdw.c
> > > index b38d322734b4..fb038ef55be2 100644
> > > --- a/tools/perf/util/unwind-libdw.c
> > > +++ b/tools/perf/util/unwind-libdw.c
> > > @@ -53,8 +53,10 @@ static int __report_module(struct addr_location *al, u64 ip,
> > >          */
> > >         thread__find_symbol(ui->thread, PERF_RECORD_MISC_USER, ip, al);
> > >
> > > -       if (al->map)
> > > -               dso = map__dso(al->map);
> > > +       if (!al->map)
> > > +               return -1;
> > > +
> > > +       dso = map__dso(al->map);
> > >
> > >         if (!dso)
> > >                 return 0;
> > > --
> > > 2.45.2
> > >
> > >