[PATCH] Make kretprobe_trampoline symbol look like a function.

[PATCH] Make kretprobe_trampoline symbol look like a function.

[Thiago Jung Bauermann]

Fixes the following testsuite failure:

$ sudo ./perf test -v kallsyms
 1: vmlinux symtab matches kallsyms                          :
--- start ---
test child forked, pid 12489
Using /proc/kcore for kernel object code
Looking at the vmlinux_path (8 entries long)
Using /boot/vmlinux for symbols
0xc00000000003d300: diff name v: .kretprobe_trampoline_holder k: kretprobe_trampoline
Maps only in vmlinux:
 c00000000086ca38-c000000000879b6c 87ca38 [kernel].text.unlikely
 c000000000879b6c-c000000000bf0000 889b6c [kernel].meminit.text
 c000000000bf0000-c000000000c53264 c00000 [kernel].init.text
 c000000000c53264-d000000004250000 c63264 [kernel].exit.text
 d000000004250000-d000000004450000 0 [libcrc32c]
 d000000004450000-d000000004620000 0 [xfs]
 d000000004620000-d000000004680000 0 [autofs4]
 d000000004680000-d0000000046e0000 0 [x_tables]
 d0000000046e0000-d000000004780000 0 [ip_tables]
 d000000004780000-d0000000047e0000 0 [rng_core]
 d0000000047e0000-ffffffffffffffff 0 [pseries_rng]
Maps in vmlinux with a different name in kallsyms:
Maps only in kallsyms:
 d000000000000000-f000000000000000 1000000000010000 [kernel.kallsyms]
 f000000000000000-ffffffffffffffff 3000000000010000 [kernel.kallsyms]
test child finished with -1
---- end ----
vmlinux symtab matches kallsyms: FAILED!

The problem is that the kretprobe_trampoline symbol looks like this:

$ eu-readelf -s /boot/vmlinux G kretprobe_trampoline
 2431: c000000001302368     24 NOTYPE  LOCAL  DEFAULT       37 kretprobe_trampoline_holder
 2432: c00000000003d300      8 FUNC    LOCAL  DEFAULT        1 .kretprobe_trampoline_holder
97543: c00000000003d300      0 NOTYPE  GLOBAL DEFAULT        1 kretprobe_trampoline

Its type is NOTYPE, and its size is 0, and this is a problem because
symbol-elf.c:dso__load_sym skips function symbols that are not STT_FUNC
or STT_GNU_IFUNC (this is determined by elf_sym__is_function). Even
if the type is changed to STT_FUNC, when dso__load_sym calls
symbols__fixup_duplicate, the kretprobe_trampoline symbol is dropped in
favour of .kretprobe_trampoline_holder because the latter has non-zero
size (as determined by choose_best_symbol).

With this patch, during symbol deduplication kretprobe_trampoline is
used instead of .kretprobe_trampoline_holder, making all vmlinux symbols match
/proc/kallsyms and the testcase passes.

Alternatively, I just noticed that commit c1c355c was merged and it
gets rid of kretprobe_trampoline_holder altogether on x86. I believe that
would fix this problem as well, but I don't know enough about kprobes to
know whether kretprobe_trampoline_holder has any use  on powerpc.

Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
---
 arch/powerpc/kernel/kprobes.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
index 7c053f2..068f2f9 100644
--- a/arch/powerpc/kernel/kprobes.c
+++ b/arch/powerpc/kernel/kprobes.c
@@ -281,8 +281,10 @@ no_kprobe:
 static void __used kretprobe_trampoline_holder(void)
 {
 	asm volatile(".global kretprobe_trampoline\n"
+			".type kretprobe_trampoline, @function\n"
 			"kretprobe_trampoline:\n"
-			"nop\n");
+			"nop\n"
+			".size kretprobe_trampoline, .-kretprobe_trampoline\n");
 }
 
 /*
-- 
1.9.1

Re: Make kretprobe_trampoline symbol look like a function.

[Michael Ellerman]

On Thu, 2016-24-03 at 17:17:04 UTC, Thiago Jung Bauermann wrote:
> Fixes the following testsuite failure:
> 
> $ sudo ./perf test -v kallsyms
>  1: vmlinux symtab matches kallsyms                          :
> --- start ---
> test child forked, pid 12489
> Using /proc/kcore for kernel object code
> Looking at the vmlinux_path (8 entries long)
> Using /boot/vmlinux for symbols
> 0xc00000000003d300: diff name v: .kretprobe_trampoline_holder k: kretprobe_trampoline
> Maps only in vmlinux:
>  c00000000086ca38-c000000000879b6c 87ca38 [kernel].text.unlikely
>  c000000000879b6c-c000000000bf0000 889b6c [kernel].meminit.text
>  c000000000bf0000-c000000000c53264 c00000 [kernel].init.text
>  c000000000c53264-d000000004250000 c63264 [kernel].exit.text
>  d000000004250000-d000000004450000 0 [libcrc32c]
>  d000000004450000-d000000004620000 0 [xfs]
>  d000000004620000-d000000004680000 0 [autofs4]
>  d000000004680000-d0000000046e0000 0 [x_tables]
>  d0000000046e0000-d000000004780000 0 [ip_tables]
>  d000000004780000-d0000000047e0000 0 [rng_core]
>  d0000000047e0000-ffffffffffffffff 0 [pseries_rng]
> Maps in vmlinux with a different name in kallsyms:
> Maps only in kallsyms:
>  d000000000000000-f000000000000000 1000000000010000 [kernel.kallsyms]
>  f000000000000000-ffffffffffffffff 3000000000010000 [kernel.kallsyms]
> test child finished with -1
> ---- end ----
> vmlinux symtab matches kallsyms: FAILED!
> 
> The problem is that the kretprobe_trampoline symbol looks like this:
> 
> $ eu-readelf -s /boot/vmlinux G kretprobe_trampoline
>  2431: c000000001302368     24 NOTYPE  LOCAL  DEFAULT       37 kretprobe_trampoline_holder
>  2432: c00000000003d300      8 FUNC    LOCAL  DEFAULT        1 .kretprobe_trampoline_holder
> 97543: c00000000003d300      0 NOTYPE  GLOBAL DEFAULT        1 kretprobe_trampoline
> 
> Its type is NOTYPE, and its size is 0, and this is a problem because
> symbol-elf.c:dso__load_sym skips function symbols that are not STT_FUNC
> or STT_GNU_IFUNC (this is determined by elf_sym__is_function). Even
> if the type is changed to STT_FUNC, when dso__load_sym calls
> symbols__fixup_duplicate, the kretprobe_trampoline symbol is dropped in
> favour of .kretprobe_trampoline_holder because the latter has non-zero
> size (as determined by choose_best_symbol).
> 
> With this patch, during symbol deduplication kretprobe_trampoline is
> used instead of .kretprobe_trampoline_holder, making all vmlinux symbols match
> /proc/kallsyms and the testcase passes.

Thanks, good change log.

> Alternatively, I just noticed that commit c1c355c was merged and it
> gets rid of kretprobe_trampoline_holder altogether on x86. I believe that
> would fix this problem as well, but I don't know enough about kprobes to
> know whether kretprobe_trampoline_holder has any use  on powerpc.

I don't see any reason why we need it on powerpc.

So can you try removing kretprobe_trampoline_holder and check that kretprobes
still work. And then confirm that it also fixes this bug?

cheers

[PATCH] Remove kretprobe_trampoline_holder.

[Thiago Jung Bauermann]

Fixes the following testsuite failure:

$ sudo ./perf test -v kallsyms
 1: vmlinux symtab matches kallsyms                          :
--- start ---
test child forked, pid 12489
Using /proc/kcore for kernel object code
Looking at the vmlinux_path (8 entries long)
Using /boot/vmlinux for symbols
0xc00000000003d300: diff name v: .kretprobe_trampoline_holder k: kretprobe_trampoline
Maps only in vmlinux:
 c00000000086ca38-c000000000879b6c 87ca38 [kernel].text.unlikely
 c000000000879b6c-c000000000bf0000 889b6c [kernel].meminit.text
 c000000000bf0000-c000000000c53264 c00000 [kernel].init.text
 c000000000c53264-d000000004250000 c63264 [kernel].exit.text
 d000000004250000-d000000004450000 0 [libcrc32c]
 d000000004450000-d000000004620000 0 [xfs]
 d000000004620000-d000000004680000 0 [autofs4]
 d000000004680000-d0000000046e0000 0 [x_tables]
 d0000000046e0000-d000000004780000 0 [ip_tables]
 d000000004780000-d0000000047e0000 0 [rng_core]
 d0000000047e0000-ffffffffffffffff 0 [pseries_rng]
Maps in vmlinux with a different name in kallsyms:
Maps only in kallsyms:
 d000000000000000-f000000000000000 1000000000010000 [kernel.kallsyms]
 f000000000000000-ffffffffffffffff 3000000000010000 [kernel.kallsyms]
test child finished with -1
---- end ----
vmlinux symtab matches kallsyms: FAILED!

The problem is that the kretprobe_trampoline symbol looks like this:

$ eu-readelf -s /boot/vmlinux G kretprobe_trampoline
 2431: c000000001302368     24 NOTYPE  LOCAL  DEFAULT       37 kretprobe_trampoline_holder
 2432: c00000000003d300      8 FUNC    LOCAL  DEFAULT        1 .kretprobe_trampoline_holder
97543: c00000000003d300      0 NOTYPE  GLOBAL DEFAULT        1 kretprobe_trampoline

Its type is NOTYPE, and its size is 0, and this is a problem because
symbol-elf.c:dso__load_sym skips function symbols that are not STT_FUNC
or STT_GNU_IFUNC (this is determined by elf_sym__is_function). Even
if the type is changed to STT_FUNC, when dso__load_sym calls
symbols__fixup_duplicate, the kretprobe_trampoline symbol is dropped in
favour of .kretprobe_trampoline_holder because the latter has non-zero
size (as determined by choose_best_symbol).

With this patch, all vmlinux symbols match /proc/kallsyms and the
testcase passes.

Commit c1c355c gets rid of kretprobe_trampoline_holder altogether on
x86. This commit does the same on powerpc.

Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
---

Notes:
    With this patch, all ftrace startup tests still pass.  There's one ftrace
    testcase failure:
    
    [4] Kprobe dynamic event with function tracer   [FAIL]
    execute:
    /home/bauermann/src/linux/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_ftrace.tc
    + .
    /home/bauermann/src/linux/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_ftrace.tc
    ++ '[' -f kprobe_events ']'
    ++ grep function available_tracers
    blk function_graph wakeup_dl wakeup_rt wakeup function nop
    ++ echo nop
    ++ echo _do_fork
    /home/bauermann/src/linux/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_ftrace.tc:
    line 9: echo: write error: Invalid argument
    
    But even without my patch that testcase fails in the same way, so this
    is not a problem introduced by the patch.

 arch/powerpc/kernel/kprobes.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
index 7c053f2..417c0ea 100644
--- a/arch/powerpc/kernel/kprobes.c
+++ b/arch/powerpc/kernel/kprobes.c
@@ -278,12 +278,11 @@ no_kprobe:
  * 	- When the probed function returns, this probe
  * 		causes the handlers to fire
  */
-static void __used kretprobe_trampoline_holder(void)
-{
-	asm volatile(".global kretprobe_trampoline\n"
-			"kretprobe_trampoline:\n"
-			"nop\n");
-}
+asm(".global kretprobe_trampoline\n"
+	".type kretprobe_trampoline, @function\n"
+	"kretprobe_trampoline:\n"
+	"nop\n"
+	".size kretprobe_trampoline, .-kretprobe_trampoline\n");
 
 /*
  * Called when the probe at kretprobe trampoline is hit
-- 
1.9.1

Re: [PATCH] Remove kretprobe_trampoline_holder.

[Thiago Jung Bauermann]

Am Montag, 28 M=E4rz 2016, 17:06:32 schrieb Thiago Jung Bauermann:
> /home/bauermann/src/linux/tools/testing/selftests/ftrace/test.d/kprob=
e/kp
> robe_ftrace.tc: line 9: echo: write error: Invalid argument
>=20
>     But even without my patch that testcase fails in the same way, so=
 this
> is not a problem introduced by the patch.

That failure is on this line of kprobe_ftrace.tc:

echo _do_fork > set_ftrace_filter

This fails because on powerpc the function symbol has a dot prepended t=
o its=20
name:

# cat available_filter_functions | grep _do_fork
._do_fork

If I do s/_do_fork/._do_fork/ in kprobe_ftrace.tc then all ftrace kprob=
e=20
tests pass:

$ sudo ./ftracetest test.d/kprobe/
=3D=3D=3D Ftrace unit tests =3D=3D=3D
[1] Kprobe dynamic event - adding and removing  [PASS]
[2] Kprobe dynamic event - busy event check     [PASS]
[3] Kprobe dynamic event with arguments [PASS]
[4] Kprobe dynamic event with function tracer   [PASS]
[5] Kretprobe dynamic event with arguments      [PASS]

# of passed:  5
# of failed:  0
# of unresolved:  0
# of untested:  0
# of unsupported:  0
# of xfailed:  0
# of undefined(test bug):  0

--=20
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center

Re: [PATCH] Remove kretprobe_trampoline_holder.

[Michael Ellerman]

On Mon, 2016-03-28 at 17:29 -0300, Thiago Jung Bauermann wrote:

> Am Montag, 28 März 2016, 17:06:32 schrieb Thiago Jung Bauermann:

> > /home/bauermann/src/linux/tools/testing/selftests/ftrace/test.d/kprobe/kp
> > robe_ftrace.tc: line 9: echo: write error: Invalid argument
> >
> >     But even without my patch that testcase fails in the same way, so this
> > is not a problem introduced by the patch.
>
> That failure is on this line of kprobe_ftrace.tc:
>
> echo _do_fork > set_ftrace_filter
>
> This fails because on powerpc the function symbol has a dot prepended to its
> name:

Only on big endian powerpc. (Or actually ABI < ELFv2, but in practice that
means BE)

> # cat available_filter_functions | grep _do_fork
> ._do_fork
>
> If I do s/_do_fork/._do_fork/ in kprobe_ftrace.tc then all ftrace kprobe
> tests pass:
>
> $ sudo ./ftracetest test.d/kprobe/
> === Ftrace unit tests ===
> [1] Kprobe dynamic event - adding and removing  [PASS]
> [2] Kprobe dynamic event - busy event check     [PASS]
> [3] Kprobe dynamic event with arguments [PASS]
> [4] Kprobe dynamic event with function tracer   [PASS]
> [5] Kretprobe dynamic event with arguments      [PASS]

OK. We fixed that in 'perf probe', but not if you're using the sysfs file
directly.

Do you want to write a patch for ftracetest to try and handle it? I guess you'd
try "_do_fork" and if that fails then try "._do_fork", and maybe only if uname -m
says you're running on ppc64?

cheers

Re: [PATCH] Remove kretprobe_trampoline_holder.

[Thiago Jung Bauermann]

Am Dienstag, 29 M=C3=A4rz 2016, 10:45:57 schrieb Michael Ellerman:
> On Mon, 2016-03-28 at 17:29 -0300, Thiago Jung Bauermann wrote:
> > If I do s/_do_fork/._do_fork/ in kprobe_ftrace.tc then all ftrace k=
probe
> > tests pass:
>=20
> OK. We fixed that in 'perf probe', but not if you're using the sysfs =
file
> directly.
>=20
> Do you want to write a patch for ftracetest to try and handle it? I g=
uess
> you'd try "_do_fork" and if that fails then try "._do_fork", and mayb=
e
> only if uname -m says you're running on ppc64?

I did write a patch yesterday (included below for reference), but then =
I
noticed that the other ftrace tests use _do_fork and they work fine (I =
guess
because of the fix you mentioned). I think that ideally the ftrace filt=
er
mechanism should work with dot symbols as well as regular symbols.

I think this could work by creating a mechanism analogous to the
ARCH_HAS_SYSCALL_MATCH_SYM_NAME one in trace_syscalls.c. Ftrace_match_r=
ecord
could call it to adjust the symbol name (like kprobe_lookup_name) befor=
e
calling ftrace_match.

But I=E2=80=99m wondering if it=E2=80=99s really worth the effort and m=
aybe patching the
testcase is enough? Also, I don=E2=80=99t know whether my idea would ha=
ve any
side effects.

--=20
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center


>From 2ea9b3545906932b9aa213506ec0dff03cffdbe5 Mon Sep 17 00:00:00 2001
From: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
Date: Mon, 28 Mar 2016 18:39:56 -0300
Subject: [PATCH] selftests: kprobe: Fix kprobe_ftrace.tc on ppc64

On ppc64 (but not ppc64le), symbols for function entry points start wit=
h
a dot. There's no _do_fork but there is a ._do_fork.

Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
---
 .../selftests/ftrace/test.d/kprobe/kprobe_ftrace.tc  | 20 ++++++++++++=
+-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_ftrace=
.tc b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_ftrace.tc
index d6f2f49..c3ec5c2 100644
--- a/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_ftrace.tc
+++ b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_ftrace.tc
@@ -4,33 +4,39 @@
 [ -f kprobe_events ] || exit_unsupported # this is configurable
 grep function available_tracers || exit_unsupported # this is configur=
able
=20
+if [ "$(uname -m)" =3D "ppc64" ]; then
+    FILTER_FUNCTION=3D"._do_fork"
+else
+    FILTER_FUNCTION=3D"_do_fork"
+fi
+
 # prepare
 echo nop > current_tracer
-echo _do_fork > set_ftrace_filter
+echo $FILTER_FUNCTION > set_ftrace_filter
 echo 0 > events/enable
 echo > kprobe_events
-echo 'p:testprobe _do_fork' > kprobe_events
+echo "p:testprobe $FILTER_FUNCTION" > kprobe_events
=20
 # kprobe on / ftrace off
 echo 1 > events/kprobes/testprobe/enable
 echo > trace
 ( echo "forked")
 grep testprobe trace
-! grep '_do_fork <-' trace
+! grep "$FILTER_FUNCTION <-" trace
=20
 # kprobe on / ftrace on
 echo function > current_tracer
 echo > trace
 ( echo "forked")
 grep testprobe trace
-grep '_do_fork <-' trace
+grep "$FILTER_FUNCTION <-" trace
=20
 # kprobe off / ftrace on
 echo 0 > events/kprobes/testprobe/enable
 echo > trace
 ( echo "forked")
 ! grep testprobe trace
-grep '_do_fork <-' trace
+grep "$FILTER_FUNCTION <-" trace
=20
 # kprobe on / ftrace on
 echo 1 > events/kprobes/testprobe/enable
@@ -38,14 +44,14 @@ echo function > current_tracer
 echo > trace
 ( echo "forked")
 grep testprobe trace
-grep '_do_fork <-' trace
+grep "$FILTER_FUNCTION <-" trace
=20
 # kprobe on / ftrace off
 echo nop > current_tracer
 echo > trace
 ( echo "forked")
 grep testprobe trace
-! grep '_do_fork <-' trace
+! grep "$FILTER_FUNCTION <-" trace
=20
 # cleanup
 echo nop > current_tracer
--=20
1.9.1

Re: [PATCH] Remove kretprobe_trampoline_holder.

[Michael Ellerman]

On Tue, 2016-03-29 at 15:34 -0300, Thiago Jung Bauermann wrote:
> Am Dienstag, 29 März 2016, 10:45:57 schrieb Michael Ellerman:
> > On Mon, 2016-03-28 at 17:29 -0300, Thiago Jung Bauermann wrote:
> > > If I do s/_do_fork/._do_fork/ in kprobe_ftrace.tc then all ftrace kprobe
> > > tests pass:
> > 
> > OK. We fixed that in 'perf probe', but not if you're using the sysfs file
> > directly.
> > 
> > Do you want to write a patch for ftracetest to try and handle it? I guess
> > you'd try "_do_fork" and if that fails then try "._do_fork", and maybe
> > only if uname -m says you're running on ppc64?
> 
> I did write a patch yesterday (included below for reference), but then I
> noticed that the other ftrace tests use _do_fork and they work fine (I guess
> because of the fix you mentioned). I think that ideally the ftrace filter
> mechanism should work with dot symbols as well as regular symbols.
> 
> I think this could work by creating a mechanism analogous to the
> ARCH_HAS_SYSCALL_MATCH_SYM_NAME one in trace_syscalls.c. Ftrace_match_record
> could call it to adjust the symbol name (like kprobe_lookup_name) before
> calling ftrace_match.
> 
> But I’m wondering if it’s really worth the effort and maybe patching the
> testcase is enough? Also, I don’t know whether my idea would have any
> side effects.

It'd be nice if it worked properly. Reusing kprobe_lookup_name() looks like it
would be the right fix, given this is "kprobe events".

cheers

Re: [PATCH] Remove kretprobe_trampoline_holder.

[Michael Ellerman]

On Mon, 2016-03-28 at 17:06 -0300, Thiago Jung Bauermann wrote:

> Fixes the following testsuite failure:
> 
> $ sudo ./perf test -v kallsyms
>  1: vmlinux symtab matches kallsyms                          :
> --- start ---
> test child forked, pid 12489
> Using /proc/kcore for kernel object code
> Looking at the vmlinux_path (8 entries long)
> Using /boot/vmlinux for symbols
> 0xc00000000003d300: diff name v: .kretprobe_trampoline_holder k: kretprobe_trampoline
> Maps only in vmlinux:
>  c00000000086ca38-c000000000879b6c 87ca38 [kernel].text.unlikely
>  c000000000879b6c-c000000000bf0000 889b6c [kernel].meminit.text
>  c000000000bf0000-c000000000c53264 c00000 [kernel].init.text
>  c000000000c53264-d000000004250000 c63264 [kernel].exit.text
>  d000000004250000-d000000004450000 0 [libcrc32c]
>  d000000004450000-d000000004620000 0 [xfs]
>  d000000004620000-d000000004680000 0 [autofs4]
>  d000000004680000-d0000000046e0000 0 [x_tables]
>  d0000000046e0000-d000000004780000 0 [ip_tables]
>  d000000004780000-d0000000047e0000 0 [rng_core]
>  d0000000047e0000-ffffffffffffffff 0 [pseries_rng]
> Maps in vmlinux with a different name in kallsyms:
> Maps only in kallsyms:
>  d000000000000000-f000000000000000 1000000000010000 [kernel.kallsyms]
>  f000000000000000-ffffffffffffffff 3000000000010000 [kernel.kallsyms]
> test child finished with -1
> ---- end ----
> vmlinux symtab matches kallsyms: FAILED!
> 
> The problem is that the kretprobe_trampoline symbol looks like this:
> 
> $ eu-readelf -s /boot/vmlinux G kretprobe_trampoline
>  2431: c000000001302368     24 NOTYPE  LOCAL  DEFAULT       37 kretprobe_trampoline_holder
>  2432: c00000000003d300      8 FUNC    LOCAL  DEFAULT        1 .kretprobe_trampoline_holder
> 97543: c00000000003d300      0 NOTYPE  GLOBAL DEFAULT        1 kretprobe_trampoline
> 
> Its type is NOTYPE, and its size is 0, and this is a problem because
> symbol-elf.c:dso__load_sym skips function symbols that are not STT_FUNC
> or STT_GNU_IFUNC (this is determined by elf_sym__is_function). Even
> if the type is changed to STT_FUNC, when dso__load_sym calls
> symbols__fixup_duplicate, the kretprobe_trampoline symbol is dropped in
> favour of .kretprobe_trampoline_holder because the latter has non-zero
> size (as determined by choose_best_symbol).
> 
> With this patch, all vmlinux symbols match /proc/kallsyms and the
> testcase passes.

Have you tested this on an LE system? I did a quick test and it appears to be
completely broken. Basically every symbol is not found, eg:

 1: vmlinux symtab matches kallsyms                          :
--- start ---
test child forked, pid 16222
Using /proc/kcore for kernel object code
Looking at the vmlinux_path (8 entries long)
Using /boot/vmlinux-4.5.0-11318-gdf01bc5cf4ea for symbols
0xc00000000000b428: run_init_process not on kallsyms
0xc00000000000b478: try_to_run_init_process not on kallsyms
0xc00000000000b4f8: do_one_initcall not on kallsyms
0xc00000000000b768: match_dev_by_uuid not on kallsyms
...
0xc0000000009846b8: write_mem not on kallsyms
0xc000000000984728: do_fp_store not on kallsyms
0xc000000000984828: emulate_step not on kallsyms

$ sudo grep emulate_step /proc/kallsyms
c000000000984820 T emulate_step


Notice it's off by 8. That's because of the local/global entry point on LE. So
that will need to be fixed somewhere.

cheers

Re: [PATCH] Remove kretprobe_trampoline_holder.

[Thiago Jung Bauermann]

Am Dienstag, 29 M=C3=A4rz 2016, 14:31:34 schrieb Michael Ellerman:
> On Mon, 2016-03-28 at 17:06 -0300, Thiago Jung Bauermann wrote:
> > With this patch, all vmlinux symbols match /proc/kallsyms and the
> > testcase passes.
>=20
> Have you tested this on an LE system?

No, I was focusing on ppc64 BE.

> I did a quick test and it appears to
> be completely broken. Basically every symbol is not found, eg:
>=20
>  1: vmlinux symtab matches kallsyms                          :
> --- start ---
> test child forked, pid 16222
> Using /proc/kcore for kernel object code
> Looking at the vmlinux_path (8 entries long)
> Using /boot/vmlinux-4.5.0-11318-gdf01bc5cf4ea for symbols
> 0xc00000000000b428: run_init_process not on kallsyms
> 0xc00000000000b478: try_to_run_init_process not on kallsyms
> 0xc00000000000b4f8: do_one_initcall not on kallsyms
> 0xc00000000000b768: match_dev_by_uuid not on kallsyms
> ...
> 0xc0000000009846b8: write_mem not on kallsyms
> 0xc000000000984728: do_fp_store not on kallsyms
> 0xc000000000984828: emulate_step not on kallsyms
>=20
> $ sudo grep emulate_step /proc/kallsyms
> c000000000984820 T emulate_step
>=20
>=20
> Notice it's off by 8. That's because of the local/global entry point =
on
> LE. So that will need to be fixed somewhere.

Symbols loaded from the vmlinux file get adjusted to the local entry po=
int=20
because symbol-elf.c:dso__load_sym calls arch__elf_sym_adjust for each=20=

function symbol, which on ppc64le adjusts the address to the local entr=
y=20
point. On the other hand, when symbols are loaded from /proc/kallsyms t=
hey=20
are used as-is (i.e., with the global entry point address).

This seems inconsistent: dso__load_kernel_sym can end up loading symbol=
s=20
from either vmlinux or /proc/kallsyms, so it seems symbols should look =
the=20
same wherever they are loaded from. I am still trying to understand wha=
t the=20
rest of the perf code expects.

If I comment out the call to arch__elf_sym_adjust in dso__load_sym, the=
n all=20
symbols are found in the vmlinux-kallsyms.c test (the test still fails=20=

because two symbols have different names. I haven=E2=80=99t checked why=
). Also, the=20
code-reading.c test is able to read object code for a lot (but not all)=
 of=20
the sample events, whereas in the adjusted symbols case it can=E2=80=99=
t read object=20
code for any sample event. The other perf tests are unaffected by the=20=

change.

I thought that if there=E2=80=99s no vmlinux available, then perf would=
 have to rely=20
only on /proc/kallsyms and I would see some difference in the test resu=
lts=20
compared to when perf uses vmlinux and adjusts the symbols to the local=
=20
entry point, but I saw no difference at all.

So at first glance, it looks like perf is better off using symbols that=
=20
point to the global entry point...
--=20
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center

Re: [PATCH] Remove kretprobe_trampoline_holder.

[Naveen N. Rao]

On 2016/03/29 08:35PM, Thiago Jung Bauermann wrote:
> Am Dienstag, 29 März 2016, 14:31:34 schrieb Michael Ellerman:
> > On Mon, 2016-03-28 at 17:06 -0300, Thiago Jung Bauermann wrote:
> > > With this patch, all vmlinux symbols match /proc/kallsyms and the
> > > testcase passes.
> > 
> > Have you tested this on an LE system?
> 
> No, I was focusing on ppc64 BE.

Which kernel did you use? I don't see this with the latest 4.6.0-rc1.

> 
> > I did a quick test and it appears to
> > be completely broken. Basically every symbol is not found, eg:
> > 
> >  1: vmlinux symtab matches kallsyms                          :
> > --- start ---
> > test child forked, pid 16222
> > Using /proc/kcore for kernel object code
> > Looking at the vmlinux_path (8 entries long)
> > Using /boot/vmlinux-4.5.0-11318-gdf01bc5cf4ea for symbols
> > 0xc00000000000b428: run_init_process not on kallsyms
> > 0xc00000000000b478: try_to_run_init_process not on kallsyms
> > 0xc00000000000b4f8: do_one_initcall not on kallsyms
> > 0xc00000000000b768: match_dev_by_uuid not on kallsyms
> > ...
> > 0xc0000000009846b8: write_mem not on kallsyms
> > 0xc000000000984728: do_fp_store not on kallsyms
> > 0xc000000000984828: emulate_step not on kallsyms
> > 
> > $ sudo grep emulate_step /proc/kallsyms
> > c000000000984820 T emulate_step
> > 
> > 
> > Notice it's off by 8. That's because of the local/global entry point on
> > LE. So that will need to be fixed somewhere.
> 
> Symbols loaded from the vmlinux file get adjusted to the local entry point 
> because symbol-elf.c:dso__load_sym calls arch__elf_sym_adjust for each 
> function symbol, which on ppc64le adjusts the address to the local entry 
> point. On the other hand, when symbols are loaded from /proc/kallsyms 
> they are used as-is (i.e., with the global entry point address).
> 
> This seems inconsistent: dso__load_kernel_sym can end up loading symbols 
> from either vmlinux or /proc/kallsyms, so it seems symbols should look the 
> same wherever they are loaded from. I am still trying to understand what the 
> rest of the perf code expects.

The LEP is used since probing on that catches calls to both the GEP and 
the LEP. I did fix 'perf probe' to fixup the kallsyms address as well.  
See commit 7b6ff0bdb.

> 
> If I comment out the call to arch__elf_sym_adjust in dso__load_sym, then all 
> symbols are found in the vmlinux-kallsyms.c test (the test still fails 
> because two symbols have different names. I haven’t checked why). Also, the 
> code-reading.c test is able to read object code for a lot (but not all) of 
> the sample events, whereas in the adjusted symbols case it can’t read object 
> code for any sample event. The other perf tests are unaffected by the 
> change.
> 
> I thought that if there’s no vmlinux available, then perf would have to rely 
> only on /proc/kallsyms and I would see some difference in the test results 
> compared to when perf uses vmlinux and adjusts the symbols to the local 
> entry point, but I saw no difference at all.
> 
> So at first glance, it looks like perf is better off using symbols that 
> point to the global entry point...

See above - we need to use the LEP. I suspect we don't fixup all the 
symbols in kallsyms -- just the ones being probed which explains why 
there is a mismatch. Let me take a look.


- Naveen

Re: [PATCH] Remove kretprobe_trampoline_holder.

[Naveen N. Rao]

On 2016/03/30 01:34PM, Naveen N Rao wrote:
> On 2016/03/29 08:35PM, Thiago Jung Bauermann wrote:
> > Am Dienstag, 29 März 2016, 14:31:34 schrieb Michael Ellerman:
> > > On Mon, 2016-03-28 at 17:06 -0300, Thiago Jung Bauermann wrote:
> > So at first glance, it looks like perf is better off using symbols 
> > that point to the global entry point...
> 
> See above - we need to use the LEP. I suspect we don't fixup all the 
> symbols in kallsyms -- just the ones being probed which explains why 
> there is a mismatch. Let me take a look.

Indeed. With vmlinux, we use the symbol table to identify symbols with a 
LEP, and we adjust those symbols during load time. However, with 
kallsyms, we don't have any data around which symbols have a LEP and 
which don't. So, we can't fixup all the entries globally.

- Naveen

Re: [PATCH] Remove kretprobe_trampoline_holder.

[Michael Ellerman]

On Wed, 2016-03-30 at 13:34 +0530, Naveen N. Rao wrote:
> On 2016/03/29 08:35PM, Thiago Jung Bauermann wrote:
> > Am Dienstag, 29 März 2016, 14:31:34 schrieb Michael Ellerman:
> > > On Mon, 2016-03-28 at 17:06 -0300, Thiago Jung Bauermann wrote:
> > > > With this patch, all vmlinux symbols match /proc/kallsyms and the
> > > > testcase passes.
> > >
> > > Have you tested this on an LE system?
> >
> > No, I was focusing on ppc64 BE.
>
> Which kernel did you use? I don't see this with the latest 4.6.0-rc1.

Yeah I can't reproduce the perf test failure either.

Not sure what's going on?

cheers

Re: [PATCH] Remove kretprobe_trampoline_holder.

[Thiago Jung Bauermann]

Am Mittwoch, 30 M=E4rz 2016, 20:09:36 schrieb Michael Ellerman:
> On Wed, 2016-03-30 at 13:34 +0530, Naveen N. Rao wrote:
> > On 2016/03/29 08:35PM, Thiago Jung Bauermann wrote:
> > > Am Dienstag, 29 M=E4rz 2016, 14:31:34 schrieb Michael Ellerman:
> > > > On Mon, 2016-03-28 at 17:06 -0300, Thiago Jung Bauermann wrote:=

> > > > > With this patch, all vmlinux symbols match /proc/kallsyms and=
 the
> > > > > testcase passes.
> > > >=20
> > > > Have you tested this on an LE system?
> > >=20
> > > No, I was focusing on ppc64 BE.
> >=20
> > Which kernel did you use? I don't see this with the latest 4.6.0-rc=
1.
>=20
> Yeah I can't reproduce the perf test failure either.

I can still reproduce on 4.6.0-rc1.

> Not sure what's going on?

Try disabling CONFIG_FUNCTION_TRACER. The test failure happens when .kr=
etprobe_trampoline_holder and
kretprobe_trampoline have the same address. If that option is enabled, =
the symbols have different addresses:

$ eu-readelf -s /boot/vmlinux-4.6.0-rc1-dirty | grep kretprobe_tramp   =
                                                =20
 2479: c000000000ef1d68     24 NOTYPE  LOCAL  DEFAULT       37 kretprob=
e_trampoline_holder
 2480: c000000000043ab0     40 FUNC    LOCAL  DEFAULT        1 .kretpro=
be_trampoline_holder
86903: c000000000043ac4      0 NOTYPE  GLOBAL DEFAULT        1 kretprob=
e_trampoline
$ grep kretprobe_tramp /proc/kallsyms
c000000000043ab0 t .kretprobe_trampoline_holder
c000000000043ac4 T kretprobe_trampoline
c000000000ef1d68 d kretprobe_trampoline_holder

--=20
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center

Re: [PATCH] Remove kretprobe_trampoline_holder.

[Naveen N. Rao]

On 2016/03/28 05:06PM, Thiago Jung Bauermann wrote:
> Fixes the following testsuite failure:
> 
> $ sudo ./perf test -v kallsyms
>  1: vmlinux symtab matches kallsyms                          :
> --- start ---
> test child forked, pid 12489
> Using /proc/kcore for kernel object code
> Looking at the vmlinux_path (8 entries long)
> Using /boot/vmlinux for symbols
> 0xc00000000003d300: diff name v: .kretprobe_trampoline_holder k: kretprobe_trampoline
> Maps only in vmlinux:
>  c00000000086ca38-c000000000879b6c 87ca38 [kernel].text.unlikely
>  c000000000879b6c-c000000000bf0000 889b6c [kernel].meminit.text
>  c000000000bf0000-c000000000c53264 c00000 [kernel].init.text
>  c000000000c53264-d000000004250000 c63264 [kernel].exit.text
>  d000000004250000-d000000004450000 0 [libcrc32c]
>  d000000004450000-d000000004620000 0 [xfs]
>  d000000004620000-d000000004680000 0 [autofs4]
>  d000000004680000-d0000000046e0000 0 [x_tables]
>  d0000000046e0000-d000000004780000 0 [ip_tables]
>  d000000004780000-d0000000047e0000 0 [rng_core]
>  d0000000047e0000-ffffffffffffffff 0 [pseries_rng]
> Maps in vmlinux with a different name in kallsyms:
> Maps only in kallsyms:
>  d000000000000000-f000000000000000 1000000000010000 [kernel.kallsyms]
>  f000000000000000-ffffffffffffffff 3000000000010000 [kernel.kallsyms]
> test child finished with -1
> ---- end ----
> vmlinux symtab matches kallsyms: FAILED!

You should indent the above output -- in this specific case, the start 
marker interferes with git am.

Apart from that, I have tested this patch and kretprobe works fine. A 
minor nit I had is that we end up with a non-dot function in .text 
without a corresponding function descriptor for kretprobe_trampoline.  
But, since this is a trampoline, I think that is good. So, for this 
patch:
Reviewed-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>


- Naveen

Re: [PATCH] Remove kretprobe_trampoline_holder.

[Thiago Jung Bauermann]

Am Donnerstag, 31 M=C3=A4rz 2016, 13:53:11 schrieb Naveen N. Rao:
> You should indent the above output -- in this specific case, the star=
t
> marker interferes with git am.
>=20
> Apart from that, I have tested this patch and kretprobe works fine. A=

> minor nit I had is that we end up with a non-dot function in .text
> without a corresponding function descriptor for kretprobe_trampoline.=

> But, since this is a trampoline, I think that is good. So, for this
> patch:
> Reviewed-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>

Thanks for the tips and the review. I fixed the issue you mentioned and=
 sent=20
a new patch with your Reviewed-by.

Also thanks for fixing the vmlinux-kallsyms issue in ppc64le. I didn=E2=
=80=99t=20
comment on the patches because I=E2=80=99m not very familiar with the c=
ode in=20
question.

--=20
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center