[PATCH] powerpc/perf: fix ppc64 perf probe add events failed

[PATCH] powerpc/perf: fix ppc64 perf probe add events failed

[Zechuan Chen]

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@huawei.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;
+
 		/* There can be duplicated symbols in the map */
 		for (i = 0; i < j; i++)
 			if (sym->start == syms[i]->start) {
-- 
2.12.3

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

[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@huawei.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

>  		/* There can be duplicated symbols in the map */
>  		for (i = 0; i < j; i++)
>  			if (sym->start == syms[i]->start) {
> -- 
> 2.12.3

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

[chenzechuan]

在 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@huawei.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@huawei.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.

Fixes: bf794bf52a80 ("powerpc/kprobes: Fix kallsyms lookup across 
powerpc ABIv1 and ABIv2")

Signed-off-by: Zechuan Chen <chenzechuan1@huawei.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

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

[Masami Hiramatsu]

On Wed, 5 Jan 2022 10:56:46 +0800
chenzechuan <chenzechuan1@huawei.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@huawei.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@huawei.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@kernel.org>

Thank you!

> 
> Fixes: bf794bf52a80 ("powerpc/kprobes: Fix kallsyms lookup across 
> powerpc ABIv1 and ABIv2")
> 
> Signed-off-by: Zechuan Chen <chenzechuan1@huawei.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@kernel.org>