From: Naveen N. Rao <hidden> Date: 2017-06-21 18:39:02
This is the third in the series of patches to build out an appropriate
kprobes blacklist for powerpc. Since posting the second series (*),
there have been related changes to the code and I have brought that
series forward to account for those changes. As such, all patches from
the second series are included in this patchset.
This patchset now ensures that the newly added multiple kprobes test in
the ftrace testsuite passes on powerpc64. Tested on both Elfv1 and
Elfv2.
Changes since series 2 v2:
- Patches 1, 2 and 6 are new.
- Patch 3 now additionally converts syscall_restore_math() to a local
symbol.
- Patch 5 additionally blacklists __replay_interrupt.
(*)
https://www.mail-archive.com/linuxppc-dev@lists.ozlabs.org/msg117562.html
- Naveen
Naveen N. Rao (6):
powerpc64/elfv1: Validate function pointer address in the function
descriptor
powerpc/64s: Convert .L__replay_interrupt_return to a local label
powerpc/64s: Blacklist system_call() and system_call_common() from
kprobes
powerpc/64s: Un-blacklist system_call() from kprobes
powerpc/64s: Blacklist functions invoked on a trap
powerpc/64s: Blacklist rtas entry/exit from kprobes
arch/powerpc/include/asm/code-patching.h | 10 +++-
arch/powerpc/kernel/entry_64.S | 81 ++++++++++++++++++--------------
arch/powerpc/kernel/exceptions-64s.S | 6 ++-
arch/powerpc/kernel/traps.c | 3 ++
4 files changed, 63 insertions(+), 37 deletions(-)
--
2.13.1
From: Naveen N. Rao <hidden> Date: 2017-06-21 18:39:08
Currently, we assume that the function pointer we receive in
ppc_function_entry() points to a function descriptor. However, this is
not always the case. In particular, assembly symbols without the right
annotation do not have an associated function descriptor. Some of these
symbols are added to the kprobe blacklist using _ASM_NOKPROBE_SYMBOL().
When such addresses are subsequently processed through
arch_deref_entry_point() in populate_kprobe_blacklist(), we see the
below errors during bootup:
[ 0.663963] Failed to find blacklist at 7d9b02a648029b6c
[ 0.663970] Failed to find blacklist at a14d03d0394a0001
[ 0.663972] Failed to find blacklist at 7d5302a6f94d0388
[ 0.663973] Failed to find blacklist at 48027d11e8610178
[ 0.663974] Failed to find blacklist at f8010070f8410080
[ 0.663976] Failed to find blacklist at 386100704801f89d
[ 0.663977] Failed to find blacklist at 7d5302a6f94d00b0
Fix this by checking if the address in the function descriptor is
actually a valid kernel address. In the case of assembly symbols, this
will almost always fail as this ends up being powerpc instructions. In
that case, return pointer to the address we received, rather than the
dereferenced value.
Signed-off-by: Naveen N. Rao <redacted>
---
arch/powerpc/include/asm/code-patching.h | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
From: Naveen N. Rao <hidden> Date: 2017-06-21 18:39:10
Commit b48bbb82e2b835 ("powerpc/64s: Don't unbalance the return branch
predictor in __replay_interrupt()") introduced __replay_interrupt_return
symbol with '.L' prefix in hopes of keeping it private. However, due to
the use of LOAD_REG_ADDR(), the assembler kept this symbol visible. Fix
the same by instead using the local label '1'.
Fixes: Commit b48bbb82e2b835 ("powerpc/64s: Don't unbalance the return branch
predictor in __replay_interrupt()")
Suggested-by: Nicholas Piggin <npiggin@gmail.com>
Signed-off-by: Naveen N. Rao <redacted>
---
arch/powerpc/kernel/exceptions-64s.S | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
@@ -1629,7 +1629,7 @@ _GLOBAL(__replay_interrupt)*wedon't give a damn about, so we don'tbotherstoringthem.*/mfmsrr12-LOAD_REG_ADDR(r11,.L__replay_interrupt_return)+LOAD_REG_ADDR(r11,1f)mfcrr9orir12,r12,MSR_EEcmpwir3,0x900
From: Naveen N. Rao <hidden> Date: 2017-06-21 18:39:13
Convert some of the symbols into private symbols and blacklist
system_call_common() and system_call() from kprobes. We can't take a
trap at parts of these functions as either MSR_RI is unset or the kernel
stack pointer is not yet setup.
Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Naveen N. Rao <redacted>
---
arch/powerpc/kernel/entry_64.S | 29 +++++++++++++++--------------
1 file changed, 15 insertions(+), 14 deletions(-)
From: Naveen N. Rao <hidden> Date: 2017-06-21 18:39:15
It is actually safe to probe system_call() in entry_64.S, but only till
we unset MSR_RI. To allow this, add a new symbol system_call_exit()
after the mtmsrd and blacklist that. Though the mtmsrd instruction
itself is now whitelisted, we won't be allowed to probe on it as we
don't allow probing on rfi and mtmsr instructions (checked for in
arch_prepare_kprobe()).
Suggested-by: Michael Ellerman <mpe@ellerman.id.au>
Signed-off-by: Naveen N. Rao <redacted>
---
arch/powerpc/kernel/entry_64.S | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
From: Naveen N. Rao <hidden> Date: 2017-06-21 18:39:19
Blacklist all functions involved while handling a trap. We:
- convert some of the symbols into private symbols,
- remove the duplicate 'restore' symbol, and
- blacklist most functions involved while handling a trap.
Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Naveen N. Rao <redacted>
---
arch/powerpc/kernel/entry_64.S | 47 +++++++++++++++++++++---------------
arch/powerpc/kernel/exceptions-64s.S | 2 ++
arch/powerpc/kernel/traps.c | 3 +++
3 files changed, 32 insertions(+), 20 deletions(-)
From: Naveen N. Rao <hidden> Date: 2017-06-21 18:39:22
We can't take traps with relocation off, so blacklist enter_rtas() and
rtas_return_loc(). However, instead of blacklisting all of enter_rtas(),
introduce a new symbol __enter_rtas from where on we can't take a trap
and blacklist that.
Signed-off-by: Naveen N. Rao <redacted>
---
arch/powerpc/kernel/entry_64.S | 4 ++++
1 file changed, 4 insertions(+)
From: Nicholas Piggin <npiggin@gmail.com> Date: 2017-06-22 03:22:41
On Thu, 22 Jun 2017 00:08:37 +0530
"Naveen N. Rao" [off-list ref] wrote:
quoted hunk
Currently, we assume that the function pointer we receive in
ppc_function_entry() points to a function descriptor. However, this is
not always the case. In particular, assembly symbols without the right
annotation do not have an associated function descriptor. Some of these
symbols are added to the kprobe blacklist using _ASM_NOKPROBE_SYMBOL().
When such addresses are subsequently processed through
arch_deref_entry_point() in populate_kprobe_blacklist(), we see the
below errors during bootup:
[ 0.663963] Failed to find blacklist at 7d9b02a648029b6c
[ 0.663970] Failed to find blacklist at a14d03d0394a0001
[ 0.663972] Failed to find blacklist at 7d5302a6f94d0388
[ 0.663973] Failed to find blacklist at 48027d11e8610178
[ 0.663974] Failed to find blacklist at f8010070f8410080
[ 0.663976] Failed to find blacklist at 386100704801f89d
[ 0.663977] Failed to find blacklist at 7d5302a6f94d00b0
Fix this by checking if the address in the function descriptor is
actually a valid kernel address. In the case of assembly symbols, this
will almost always fail as this ends up being powerpc instructions. In
that case, return pointer to the address we received, rather than the
dereferenced value.
Signed-off-by: Naveen N. Rao <redacted>
---
arch/powerpc/include/asm/code-patching.h | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
@@ -83,8 +83,16 @@ static inline unsigned long ppc_function_entry(void *func)*OnPPC64ABIv1thefunctionpointeractuallypointstothe*function'sdescriptor.Thefirstentryinthedescriptoristhe*addressofthefunctiontext.+*+*However,wemayhavereceivedapointertoanassemblysymbol+*thatmaynotbeafunctiondescriptor.Validatethattheentry+*pointstoavalidkerneladdressandifnot,returnthepointer+*wereceivedasis.*/-return((func_descr_t*)func)->entry;+if(kernel_text_address(((func_descr_t*)func)->entry))+return((func_descr_t*)func)->entry;+else+return(unsignedlong)func;
What if "func" is a text section label (bare asm function)?
Won't func->entry load the random instruction located there
and compare it with a kernel address?
I don't know too much about the v1 ABI, but should we check for
func belonging in the .opd section first and base the check on
that? Alternatively I if "func" is in the kernel text address,
we can recognize it's not in the .opd section... right?
Thanks,
Nick
From: Nicholas Piggin <npiggin@gmail.com> Date: 2017-06-22 03:23:21
On Thu, 22 Jun 2017 00:08:38 +0530
"Naveen N. Rao" [off-list ref] wrote:
Commit b48bbb82e2b835 ("powerpc/64s: Don't unbalance the return branch
predictor in __replay_interrupt()") introduced __replay_interrupt_return
symbol with '.L' prefix in hopes of keeping it private. However, due to
the use of LOAD_REG_ADDR(), the assembler kept this symbol visible. Fix
the same by instead using the local label '1'.
Fixes: Commit b48bbb82e2b835 ("powerpc/64s: Don't unbalance the return branch
predictor in __replay_interrupt()")
Suggested-by: Nicholas Piggin <npiggin@gmail.com>
Thanks, good catch.
Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
@@ -1629,7 +1629,7 @@ _GLOBAL(__replay_interrupt)*wedon't give a damn about, so we don'tbotherstoringthem.*/mfmsrr12-LOAD_REG_ADDR(r11,.L__replay_interrupt_return)+LOAD_REG_ADDR(r11,1f)mfcrr9orir12,r12,MSR_EEcmpwir3,0x900
From: Nicholas Piggin <npiggin@gmail.com> Date: 2017-06-22 03:36:53
On Thu, 22 Jun 2017 00:08:39 +0530
"Naveen N. Rao" [off-list ref] wrote:
Convert some of the symbols into private symbols and blacklist
system_call_common() and system_call() from kprobes. We can't take a
trap at parts of these functions as either MSR_RI is unset or the kernel
stack pointer is not yet setup.
Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Naveen N. Rao <redacted>
I don't have a problem with this bunch of system call labels
going private. They've never added much for me in profiles.
Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
Semi-related question, why is system_call: where it is? Should we
move it up to right after the mtmsrd / wrteei instruction?
(obviously for another patch). It's pretty common to get PMU
interrupts coming in right after mtmsr and this makes profiles split
the syscall into two which is annoying.
Thanks,
Nick
From: Nicholas Piggin <npiggin@gmail.com> Date: 2017-06-22 03:41:24
On Thu, 22 Jun 2017 00:08:40 +0530
"Naveen N. Rao" [off-list ref] wrote:
It is actually safe to probe system_call() in entry_64.S, but only till
we unset MSR_RI. To allow this, add a new symbol system_call_exit()
after the mtmsrd and blacklist that. Though the mtmsrd instruction
itself is now whitelisted, we won't be allowed to probe on it as we
don't allow probing on rfi and mtmsr instructions (checked for in
arch_prepare_kprobe()).
Can you add a little comment to say probes aren't allowed, and it's
located after the mtmsr in order to avoid contaminating traces?
Also I wonder if a slightly different name would be more instructive?
I don't normally care, but the system_call_common code isn't trivial
to follow. system_call_exit might give the impression that it is the
entire exit path (which would pair with system_call for entry).
Perhaps system_call_exit_notrace? No that sucks too :(
Thanks,
Nick
From: Nicholas Piggin <npiggin@gmail.com> Date: 2017-06-22 03:44:59
On Thu, 22 Jun 2017 00:08:41 +0530
"Naveen N. Rao" [off-list ref] wrote:
Blacklist all functions involved while handling a trap. We:
- convert some of the symbols into private symbols,
- remove the duplicate 'restore' symbol, and
- blacklist most functions involved while handling a trap.
I'm not sure removing "restore" makes it better.
fast_exc_return_irq is a relatively specialised case...
I think all these names could be reworked and made a bit
more consistent and descriptive, but for this patch could
you just leave restore in there?
Otherwise it seems okay to me, but I haven't gone through
all the functions involved with trap yet and verified.
Thanks,
Nick
From: Nicholas Piggin <npiggin@gmail.com> Date: 2017-06-22 03:49:11
On Thu, 22 Jun 2017 00:08:42 +0530
"Naveen N. Rao" [off-list ref] wrote:
quoted hunk
We can't take traps with relocation off, so blacklist enter_rtas() and
rtas_return_loc(). However, instead of blacklisting all of enter_rtas(),
introduce a new symbol __enter_rtas from where on we can't take a trap
and blacklist that.
Signed-off-by: Naveen N. Rao <redacted>
---
arch/powerpc/kernel/entry_64.S | 4 ++++
1 file changed, 4 insertions(+)
@@ -1076,6 +1076,8 @@ _GLOBAL(enter_rtas)rldicrr9,r9,MSR_SF_LG,(63-MSR_SF_LG)orir9,r9,MSR_IR|MSR_DR|MSR_FE0|MSR_FE1|MSR_FP|MSR_RI|MSR_LEandcr6,r0,r9++__enter_rtas:sync/*disableinterruptssoSRR0/1*/mtmsrdr0/*don't get trashed */
Along the lines of the system call patch... For consistency, could we
put the __enter_rtas right after mtmsrd? And I wonder if we shoul
come up with a common prefix or postfix naming convention for these
such labels used to control probing?
How do opal calls avoid tracing?
Thanks,
Nick
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2017-06-22 10:59:50
Nicholas Piggin [off-list ref] writes:
On Thu, 22 Jun 2017 00:08:37 +0530
"Naveen N. Rao" [off-list ref] wrote:
quoted
Currently, we assume that the function pointer we receive in
ppc_function_entry() points to a function descriptor. However, this is
not always the case. In particular, assembly symbols without the right
annotation do not have an associated function descriptor. Some of these
symbols are added to the kprobe blacklist using _ASM_NOKPROBE_SYMBOL().
When such addresses are subsequently processed through
arch_deref_entry_point() in populate_kprobe_blacklist(), we see the
below errors during bootup:
[ 0.663963] Failed to find blacklist at 7d9b02a648029b6c
[ 0.663970] Failed to find blacklist at a14d03d0394a0001
[ 0.663972] Failed to find blacklist at 7d5302a6f94d0388
[ 0.663973] Failed to find blacklist at 48027d11e8610178
[ 0.663974] Failed to find blacklist at f8010070f8410080
[ 0.663976] Failed to find blacklist at 386100704801f89d
[ 0.663977] Failed to find blacklist at 7d5302a6f94d00b0
Fix this by checking if the address in the function descriptor is
actually a valid kernel address. In the case of assembly symbols, this
will almost always fail as this ends up being powerpc instructions. In
that case, return pointer to the address we received, rather than the
dereferenced value.
Signed-off-by: Naveen N. Rao <redacted>
---
arch/powerpc/include/asm/code-patching.h | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
@@ -83,8 +83,16 @@ static inline unsigned long ppc_function_entry(void *func)*OnPPC64ABIv1thefunctionpointeractuallypointstothe*function'sdescriptor.Thefirstentryinthedescriptoristhe*addressofthefunctiontext.+*+*However,wemayhavereceivedapointertoanassemblysymbol+*thatmaynotbeafunctiondescriptor.Validatethattheentry+*pointstoavalidkerneladdressandifnot,returnthepointer+*wereceivedasis.*/-return((func_descr_t*)func)->entry;+if(kernel_text_address(((func_descr_t*)func)->entry))+return((func_descr_t*)func)->entry;+else+return(unsignedlong)func;
What if "func" is a text section label (bare asm function)?
Won't func->entry load the random instruction located there
and compare it with a kernel address?
Yes, that's the problem.
I don't know too much about the v1 ABI, but should we check for
func belonging in the .opd section first and base the check on
that? Alternatively I if "func" is in the kernel text address,
we can recognize it's not in the .opd section... right?
That sounds like a more robust solution. But I suspect it won't work for
modules.
Another option might be to canonicalise the blacklist so that it always
points to the text address, not sure how easy that would be.
cheers
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2017-06-22 11:07:46
Nicholas Piggin [off-list ref] writes:
On Thu, 22 Jun 2017 00:08:39 +0530
"Naveen N. Rao" [off-list ref] wrote:
quoted
Convert some of the symbols into private symbols and blacklist
system_call_common() and system_call() from kprobes. We can't take a
trap at parts of these functions as either MSR_RI is unset or the kernel
stack pointer is not yet setup.
Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Naveen N. Rao <redacted>
I don't have a problem with this bunch of system call labels
going private. They've never added much for me in profiles.
Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
Semi-related question, why is system_call: where it is?
Ancient history.
We used to have:
bne syscall_dotrace
syscall_dotrace_cont:
cmpldi 0,r0,NR_syscalls
bge- syscall_enosys
system_call: /* label this so stack traces look sane */
So it was there to hide syscall_dotrace_cont from back traces.
But we made syscall_dotrace_cont local in 2012 and then removed it
entirely in 2015.
Should we move it up to right after the mtmsrd / wrteei instruction?
(obviously for another patch). It's pretty common to get PMU
interrupts coming in right after mtmsr and this makes profiles split
the syscall into two which is annoying.
Move it wherever makes sense and gives good back traces.
cheers
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2017-06-22 11:12:37
Nicholas Piggin [off-list ref] writes:
On Thu, 22 Jun 2017 00:08:41 +0530
"Naveen N. Rao" [off-list ref] wrote:
quoted
Blacklist all functions involved while handling a trap. We:
- convert some of the symbols into private symbols,
- remove the duplicate 'restore' symbol, and
- blacklist most functions involved while handling a trap.
I'm not sure removing "restore" makes it better.
Yeah it bloats the patch needlessly, we can do a rename patch later.
cheers
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2017-06-22 11:14:22
Nicholas Piggin [off-list ref] writes:
On Thu, 22 Jun 2017 00:08:40 +0530
"Naveen N. Rao" [off-list ref] wrote:
quoted
It is actually safe to probe system_call() in entry_64.S, but only till
we unset MSR_RI. To allow this, add a new symbol system_call_exit()
after the mtmsrd and blacklist that. Though the mtmsrd instruction
itself is now whitelisted, we won't be allowed to probe on it as we
don't allow probing on rfi and mtmsr instructions (checked for in
arch_prepare_kprobe()).
Can you add a little comment to say probes aren't allowed, and it's
located after the mtmsr in order to avoid contaminating traces?
Also I wonder if a slightly different name would be more instructive?
I don't normally care, but the system_call_common code isn't trivial
to follow. system_call_exit might give the impression that it is the
entire exit path (which would pair with system_call for entry).
It is the entire path in the happy case isn't it? I'm not sure I know
what you mean.
Perhaps system_call_exit_notrace? No that sucks too :(
A bit :D
If you're tracing etc. then you'll be in syscall_exit_work, isn't that
sufficient to differentiate the two?
cheers
From: Nicholas Piggin <npiggin@gmail.com> Date: 2017-06-22 13:06:23
On Thu, 22 Jun 2017 20:59:49 +1000
Michael Ellerman [off-list ref] wrote:
Nicholas Piggin [off-list ref] writes:
quoted
On Thu, 22 Jun 2017 00:08:37 +0530
"Naveen N. Rao" [off-list ref] wrote:
quoted
Currently, we assume that the function pointer we receive in
ppc_function_entry() points to a function descriptor. However, this is
not always the case. In particular, assembly symbols without the right
annotation do not have an associated function descriptor. Some of these
symbols are added to the kprobe blacklist using _ASM_NOKPROBE_SYMBOL().
When such addresses are subsequently processed through
arch_deref_entry_point() in populate_kprobe_blacklist(), we see the
below errors during bootup:
[ 0.663963] Failed to find blacklist at 7d9b02a648029b6c
[ 0.663970] Failed to find blacklist at a14d03d0394a0001
[ 0.663972] Failed to find blacklist at 7d5302a6f94d0388
[ 0.663973] Failed to find blacklist at 48027d11e8610178
[ 0.663974] Failed to find blacklist at f8010070f8410080
[ 0.663976] Failed to find blacklist at 386100704801f89d
[ 0.663977] Failed to find blacklist at 7d5302a6f94d00b0
Fix this by checking if the address in the function descriptor is
actually a valid kernel address. In the case of assembly symbols, this
will almost always fail as this ends up being powerpc instructions. In
that case, return pointer to the address we received, rather than the
dereferenced value.
Signed-off-by: Naveen N. Rao <redacted>
---
arch/powerpc/include/asm/code-patching.h | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
@@ -83,8 +83,16 @@ static inline unsigned long ppc_function_entry(void *func)*OnPPC64ABIv1thefunctionpointeractuallypointstothe*function'sdescriptor.Thefirstentryinthedescriptoristhe*addressofthefunctiontext.+*+*However,wemayhavereceivedapointertoanassemblysymbol+*thatmaynotbeafunctiondescriptor.Validatethattheentry+*pointstoavalidkerneladdressandifnot,returnthepointer+*wereceivedasis.*/-return((func_descr_t*)func)->entry;+if(kernel_text_address(((func_descr_t*)func)->entry))+return((func_descr_t*)func)->entry;+else+return(unsignedlong)func;
What if "func" is a text section label (bare asm function)?
Won't func->entry load the random instruction located there
and compare it with a kernel address?
Yes, that's the problem.
quoted
I don't know too much about the v1 ABI, but should we check for
func belonging in the .opd section first and base the check on
that? Alternatively I if "func" is in the kernel text address,
we can recognize it's not in the .opd section... right?
That sounds like a more robust solution. But I suspect it won't work for
modules.
kernel_text_address() seems to check for module text as well, so it
might work I think?
From: Nicholas Piggin <npiggin@gmail.com> Date: 2017-06-22 13:08:15
On Thu, 22 Jun 2017 21:07:46 +1000
Michael Ellerman [off-list ref] wrote:
Nicholas Piggin [off-list ref] writes:
quoted
On Thu, 22 Jun 2017 00:08:39 +0530
"Naveen N. Rao" [off-list ref] wrote:
quoted
Convert some of the symbols into private symbols and blacklist
system_call_common() and system_call() from kprobes. We can't take a
trap at parts of these functions as either MSR_RI is unset or the kernel
stack pointer is not yet setup.
Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Naveen N. Rao <redacted>
I don't have a problem with this bunch of system call labels
going private. They've never added much for me in profiles.
Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
Semi-related question, why is system_call: where it is?
Ancient history.
We used to have:
bne syscall_dotrace
syscall_dotrace_cont:
cmpldi 0,r0,NR_syscalls
bge- syscall_enosys
system_call: /* label this so stack traces look sane */
So it was there to hide syscall_dotrace_cont from back traces.
But we made syscall_dotrace_cont local in 2012 and then removed it
entirely in 2015.
quoted
Should we move it up to right after the mtmsrd / wrteei instruction?
(obviously for another patch). It's pretty common to get PMU
interrupts coming in right after mtmsr and this makes profiles split
the syscall into two which is annoying.
Move it wherever makes sense and gives good back traces.
I'd be in favour of moving it to right after the interurpt enable.
I suppose you'd want a separate patch for that though. But we could
put it in this series since we're changing a lot of labels.
Thanks,
Nick
From: Nicholas Piggin <npiggin@gmail.com> Date: 2017-06-22 13:14:25
On Thu, 22 Jun 2017 21:14:21 +1000
Michael Ellerman [off-list ref] wrote:
Nicholas Piggin [off-list ref] writes:
quoted
On Thu, 22 Jun 2017 00:08:40 +0530
"Naveen N. Rao" [off-list ref] wrote:
quoted
It is actually safe to probe system_call() in entry_64.S, but only till
we unset MSR_RI. To allow this, add a new symbol system_call_exit()
after the mtmsrd and blacklist that. Though the mtmsrd instruction
itself is now whitelisted, we won't be allowed to probe on it as we
don't allow probing on rfi and mtmsr instructions (checked for in
arch_prepare_kprobe()).
Can you add a little comment to say probes aren't allowed, and it's
located after the mtmsr in order to avoid contaminating traces?
Also I wonder if a slightly different name would be more instructive?
I don't normally care, but the system_call_common code isn't trivial
to follow. system_call_exit might give the impression that it is the
entire exit path (which would pair with system_call for entry).
It is the entire path in the happy case isn't it? I'm not sure I know
what you mean.
Oh, yes you're right, I thought it was moved down further.
Thanks,
Nick
From: Naveen N. Rao <hidden> Date: 2017-06-22 14:01:23
On 2017/06/22 11:06PM, Nicholas Piggin wrote:
On Thu, 22 Jun 2017 20:59:49 +1000
Michael Ellerman [off-list ref] wrote:
quoted
Nicholas Piggin [off-list ref] writes:
quoted
On Thu, 22 Jun 2017 00:08:37 +0530
"Naveen N. Rao" [off-list ref] wrote:
quoted
Currently, we assume that the function pointer we receive in
ppc_function_entry() points to a function descriptor. However, this is
not always the case. In particular, assembly symbols without the right
annotation do not have an associated function descriptor. Some of these
symbols are added to the kprobe blacklist using _ASM_NOKPROBE_SYMBOL().
When such addresses are subsequently processed through
arch_deref_entry_point() in populate_kprobe_blacklist(), we see the
below errors during bootup:
[ 0.663963] Failed to find blacklist at 7d9b02a648029b6c
[ 0.663970] Failed to find blacklist at a14d03d0394a0001
[ 0.663972] Failed to find blacklist at 7d5302a6f94d0388
[ 0.663973] Failed to find blacklist at 48027d11e8610178
[ 0.663974] Failed to find blacklist at f8010070f8410080
[ 0.663976] Failed to find blacklist at 386100704801f89d
[ 0.663977] Failed to find blacklist at 7d5302a6f94d00b0
Fix this by checking if the address in the function descriptor is
actually a valid kernel address. In the case of assembly symbols, this
will almost always fail as this ends up being powerpc instructions. In
that case, return pointer to the address we received, rather than the
dereferenced value.
Signed-off-by: Naveen N. Rao <redacted>
---
arch/powerpc/include/asm/code-patching.h | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
@@ -83,8 +83,16 @@ static inline unsigned long ppc_function_entry(void *func)*OnPPC64ABIv1thefunctionpointeractuallypointstothe*function'sdescriptor.Thefirstentryinthedescriptoristhe*addressofthefunctiontext.+*+*However,wemayhavereceivedapointertoanassemblysymbol+*thatmaynotbeafunctiondescriptor.Validatethattheentry+*pointstoavalidkerneladdressandifnot,returnthepointer+*wereceivedasis.*/-return((func_descr_t*)func)->entry;+if(kernel_text_address(((func_descr_t*)func)->entry))+return((func_descr_t*)func)->entry;+else+return(unsignedlong)func;
What if "func" is a text section label (bare asm function)?
Won't func->entry load the random instruction located there
and compare it with a kernel address?
Yes, that's the problem.
Yes, we were currently returning those instructions as the function
entry address.
quoted
quoted
I don't know too much about the v1 ABI, but should we check for
func belonging in the .opd section first and base the check on
that? Alternatively I if "func" is in the kernel text address,
we can recognize it's not in the .opd section... right?
That sounds like a more robust solution. But I suspect it won't work for
modules.
kernel_text_address() seems to check for module text as well, so it
might work I think?
Yes, I think that's a very nice idea! I'll check and confirm that it
does what it's supposed to.
Thanks for the review,
- Naveen
From: Naveen N. Rao <hidden> Date: 2017-06-22 14:35:24
On 2017/06/22 11:08PM, Nicholas Piggin wrote:
On Thu, 22 Jun 2017 21:07:46 +1000
Michael Ellerman [off-list ref] wrote:
quoted
Nicholas Piggin [off-list ref] writes:
quoted
On Thu, 22 Jun 2017 00:08:39 +0530
"Naveen N. Rao" [off-list ref] wrote:
quoted
Convert some of the symbols into private symbols and blacklist
system_call_common() and system_call() from kprobes. We can't take a
trap at parts of these functions as either MSR_RI is unset or the kernel
stack pointer is not yet setup.
Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Naveen N. Rao <redacted>
I don't have a problem with this bunch of system call labels
going private. They've never added much for me in profiles.
Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
Thanks for the review.
quoted
quoted
Semi-related question, why is system_call: where it is?
Ancient history.
We used to have:
bne syscall_dotrace
syscall_dotrace_cont:
cmpldi 0,r0,NR_syscalls
bge- syscall_enosys
system_call: /* label this so stack traces look sane */
So it was there to hide syscall_dotrace_cont from back traces.
But we made syscall_dotrace_cont local in 2012 and then removed it
entirely in 2015.
quoted
Should we move it up to right after the mtmsrd / wrteei instruction?
(obviously for another patch). It's pretty common to get PMU
interrupts coming in right after mtmsr and this makes profiles split
the syscall into two which is annoying.
Move it wherever makes sense and gives good back traces.
I'd be in favour of moving it to right after the interurpt enable.
I suppose you'd want a separate patch for that though. But we could
put it in this series since we're changing a lot of labels.
From: Naveen N. Rao <hidden> Date: 2017-06-22 15:43:51
On 2017/06/22 11:14PM, Nicholas Piggin wrote:
On Thu, 22 Jun 2017 21:14:21 +1000
Michael Ellerman [off-list ref] wrote:
quoted
Nicholas Piggin [off-list ref] writes:
quoted
On Thu, 22 Jun 2017 00:08:40 +0530
"Naveen N. Rao" [off-list ref] wrote:
quoted
It is actually safe to probe system_call() in entry_64.S, but only till
we unset MSR_RI. To allow this, add a new symbol system_call_exit()
after the mtmsrd and blacklist that. Though the mtmsrd instruction
itself is now whitelisted, we won't be allowed to probe on it as we
don't allow probing on rfi and mtmsr instructions (checked for in
arch_prepare_kprobe()).
Can you add a little comment to say probes aren't allowed, and it's
located after the mtmsr in order to avoid contaminating traces?
Hmm.. it's actually after the mtmsrd since we can probe until that
point. In v2, I converted .Lsyscall_exit() into a different label and
blacklisted all of it. But Michael prefers to only blacklist what we
must. It is helpful if we ever want to probe after returning from a
syscall. That said, please see further below (*)
quoted
quoted
Also I wonder if a slightly different name would be more instructive?
I don't normally care, but the system_call_common code isn't trivial
to follow. system_call_exit might give the impression that it is the
entire exit path (which would pair with system_call for entry).
It is the entire path in the happy case isn't it? I'm not sure I know
what you mean.
Oh, yes you're right, I thought it was moved down further.
quoted
If you're tracing etc. then you'll be in syscall_exit_work, isn't
that sufficient to differentiate the two?
Not sure how much this matters, but the new symbol (system_call_exit) is
actually a few instructions from after the syscall return
(.Lsyscall_exit). And I have converted syscall_exit_work to a private
symbol as well.
In general, from kprobes standpoint, there are places there where we can
probe safely but doing so may require new symbols to be introduced,
which could make traces look weird, as well as distribute samples among
different symbols.
I'm not sure how best to proceed. In this current series, I pretty much
blacklist all of system_call_common() through to syscall_exit() except
for the small part in between in system_call(). All other symbols have
been made private, so nothing else apart from system_call_common(),
system_call() and system_call_exit() show up in traces.
We could probably retain syscall_dotrace(), syscall_enosys() and parts
of syscall_exit_work() after RI is restored, which will allow those to
be probed, but end up showing these symbols in traces.
What would be preferable?
-----
(*)
The other suggestion Michael had was to just put a nop after the bctrl
and to again make .Lsyscall_exit public and blacklist that (though he
wasn't all for it). I suppose it does solve this issue in a nice way -
the call traces when in a system call show the proper symbol and we do
have a 'nop' instruction on which we can probe to catch returns from
system calls. Is that something we can re-consider? Something like this
(along with converting other .Lsyscall_exit references):
From: Naveen N. Rao <hidden> Date: 2017-06-22 16:53:44
On 2017/06/22 01:48PM, Nicholas Piggin wrote:
On Thu, 22 Jun 2017 00:08:42 +0530
"Naveen N. Rao" [off-list ref] wrote:
quoted
We can't take traps with relocation off, so blacklist enter_rtas() and
rtas_return_loc(). However, instead of blacklisting all of enter_rtas(),
introduce a new symbol __enter_rtas from where on we can't take a trap
and blacklist that.
Signed-off-by: Naveen N. Rao <redacted>
---
arch/powerpc/kernel/entry_64.S | 4 ++++
1 file changed, 4 insertions(+)
@@ -1076,6 +1076,8 @@ _GLOBAL(enter_rtas)rldicrr9,r9,MSR_SF_LG,(63-MSR_SF_LG)orir9,r9,MSR_IR|MSR_DR|MSR_FE0|MSR_FE1|MSR_FP|MSR_RI|MSR_LEandcr6,r0,r9++__enter_rtas:sync/*disableinterruptssoSRR0/1*/mtmsrdr0/*don't get trashed */
Along the lines of the system call patch... For consistency, could we
put the __enter_rtas right after mtmsrd? And I wonder if we shoul
Sure.
come up with a common prefix or postfix naming convention for these
such labels used to control probing?
We could, but I am not sure it will help much. On the other hand, such
symbols may make backtraces pretty distracting.
I'm just using '__' as a prefix to make it less distracting, though it
isn't all that great either. I'm clearly hopeless with names o_O
The other option is to just blacklist entire functions, but we will then
lose the ability to probe in many places where we may have wanted to.
How do opal calls avoid tracing?
It doesn't yet. I'm still going through the initial few symbols and
identifying what needs blacklisting. Opal is further down.
Thanks,
Naveen