linux-kernel - Re: [PATCH] powerpc/perf: fix ppc64 perf probe add events failed

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] [day] [month] [year] [list]
Date:   Thu, 6 Jan 2022 10:56:39 +0900
From:   Masami Hiramatsu <mhiramat@...nel.org>
To:     chenzechuan <chenzechuan1@...wei.com>
Cc:     Arnaldo Carvalho de Melo <acme@...nel.org>,
        "Naveen N. Rao" <naveen.n.rao@...ux.vnet.ibm.com>,
        <peterz@...radead.org>, <mingo@...hat.com>, <mark.rutland@....com>,
        <alexander.shishkin@...ux.intel.com>, <jolsa@...hat.com>,
        <namhyung@...nel.org>, <Jianlin.Lv@....com>,
        <ravi.bangoria@...ux.ibm.com>, <yao.jin@...ux.intel.com>,
        <yangjihong1@...wei.com>, <mpe@...erman.id.au>,
        <linux-perf-users@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <qiuxi1@...wei.com>, <wangbing6@...wei.com>
Subject: Re: [PATCH] powerpc/perf: fix ppc64 perf probe add events failed

On Wed, 5 Jan 2022 10:56:46 +0800
chenzechuan <chenzechuan1@...wei.com> wrote:

> 
> 在 2022/1/2 22:56, Arnaldo Carvalho de Melo 写道:
> > Em Tue, Dec 28, 2021 at 07:13:38PM +0800, Zechuan Chen escreveu:
> >> Because of commit bf794bf52a80 ("powerpc/kprobes: Fix kallsyms lookup
> >> across powerpc ABIv1 and ABIv2"), in ppc64 ABIv1, our perf command
> >> eliminates the need to use the prefix "." at the symbol name. But when
> >> the command "perf probe -a schedule" is executed on ppc64 ABIv1, it
> >> obtains two symbol address information through /proc/kallsyms, for example:
> >>
> >> cat /proc/kallsyms | grep -w schedule
> >> c000000000657020 T .schedule
> >> c000000000d4fdb8 D schedule
> >>
> >> The symbol "D schedule" is not a function symbol, and perf will print:
> >> "p:probe/schedule _text+13958584"Failed to write event: Invalid argument
> >>
> >> Therefore, when searching symbols from map and adding probe point for
> >> them, a symbol type check is added. If the type of symbol is not a
> >> function, skip it.
> >>
> >> Fixes: bf794bf52a80 ("powerpc/kprobes: Fix kallsyms lookup across powerpc ABIv1 and ABIv2")
> >>
> >> Signed-off-by: Zechuan Chen <chenzechuan1@...wei.com>
> >> ---
> >>   tools/perf/util/probe-event.c | 3 +++
> >>   1 file changed, 3 insertions(+)
> >>
> >> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> >> index b2a02c9ab8ea..a834918a0a0d 100644
> >> --- a/tools/perf/util/probe-event.c
> >> +++ b/tools/perf/util/probe-event.c
> >> @@ -3083,6 +3083,9 @@ static int find_probe_trace_events_from_map(struct perf_probe_event *pev,
> >>   	for (j = 0; j < num_matched_functions; j++) {
> >>   		sym = syms[j];
> >>   
> >> +		if (sym->type != STT_FUNC)
> >> +			continue;
> >> +
> > Humm, shouldn't this be handled by find_probe_functions(), i.e. it
> > shoudn't return data symbols, right? Otherwise other places using this
> > function may malfunction as well?
> >
> > Naveen? Masami?
> >
> > - Arnaldo
> 
> Re：Thanks for your review. You are right.
>      1. Only function symbols should be returned here, not function descriptors.
>         I've moved the modification into function "find_probe_functions()",
>         and the new patch is added at the end of this email. I've re-tested it on my environment.
>         Please review it again, thanks.
> 
>      2. Function find_probe_functions only use in "perf probe".
>         Malfunction exist for all export functions, another example:
> 
>      cat /proc/kallsyms | grep -w sys_fork
>      c000000000058100 T .sys_fork
>      c000000000d485e0 D sys_fork
> 
>      perf probe -a sys_fork -v
>      No kprobe blacklist support, ignored
>      Looking at the vmlinux_path (8 entries long)
>      symsrc__init: cannot get elf header.
>      Could not open debuginfo. Try to use symbols.
>      Looking at the vmlinux_path (8 entries long)
>      symsrc__init: cannot get elf header.
>      Using /proc/kcore for kernel data
>      Using /proc/kallsyms for symbols
>      Opening /sys/kernel/tracing//kprobe_events write=1
>      Opening /sys/kernel/tracing//README write=0
>      Writing event: p:probe/sys_fork _text+360704
>      Writing event: p:probe/sys_fork _text+13927904
>      Failed to write event: Invalid argument
>        Error: Failed to add events. Reason: Invalid argument (Code: -22)
> 
> >>   		/* There can be duplicated symbols in the map */
> >>   		for (i = 0; i < j; i++)
> >>   			if (sym->start == syms[i]->start) {
> >> -- 
> >> 2.12.3
> > .
> 
>  From 8d8f5de99d16e6aa8179ecbc33c0692fc887e2af Mon Sep 17 00:00:00 2001
> From: Zechuan Chen <chenzechuan1@...wei.com>
> Date: Mon, 27 Dec 2021 09:55:11 +0800
> Subject: [PATCH] powerpc/perf: fix ppc64 perf probe add events failed
> 
> Because of commit bf794bf52a80 ("powerpc/kprobes: Fix kallsyms lookup
> across powerpc ABIv1 and ABIv2"), in ppc64 ABIv1, our perf command
> eliminates the need to use the prefix "." at the symbol name. But when
> the command "perf probe -a schedule" is executed on ppc64 ABIv1, it
> obtains two symbol address information through /proc/kallsyms, for example:
> 
> cat /proc/kallsyms | grep -w schedule
> c000000000657020 T .schedule
> c000000000d4fdb8 D schedule
> 
> The symbol "D schedule" is not a function symbol, and perf will print:
> "p:probe/schedule _text+13958584"Failed to write event: Invalid argument
> 
> Therefore, when searching symbols from map and adding probe point for
> them, a symbol type check is added. If the type of symbol is not a
> function, skip it.

OK, this looks good to me. I forgot to check the sym->type :(

Acked-by: Masami Hiramatsu <mhiramat@...nel.org>

Thank you!

> 
> Fixes: bf794bf52a80 ("powerpc/kprobes: Fix kallsyms lookup across 
> powerpc ABIv1 and ABIv2")
> 
> Signed-off-by: Zechuan Chen <chenzechuan1@...wei.com>
> ---
>   tools/perf/util/probe-event.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> index b2a02c9ab8ea..4bd6f438d73c 100644
> --- a/tools/perf/util/probe-event.c
> +++ b/tools/perf/util/probe-event.c
> @@ -2959,6 +2959,9 @@ static int find_probe_functions(struct map *map, 
> char *name,
>                  cut_version = false;
> 
>          map__for_each_symbol(map, sym, tmp) {
> +               if (sym->type != STT_FUNC)
> +                       continue;
> +
>                  norm = arch__normalize_symbol_name(sym->name);
>                  if (!norm)
>                          continue;
> --
> 2.12.3
> 


-- 
Masami Hiramatsu <mhiramat@...nel.org>