* [PATCH v3 0/6] powerpc: build out kprobes blacklist -- series 3
@ 2017-06-21 18:38 Naveen N. Rao
2017-06-21 18:38 ` [PATCH v3 1/6] powerpc64/elfv1: Validate function pointer address in the function descriptor Naveen N. Rao
` (5 more replies)
0 siblings, 6 replies; 24+ messages in thread
From: Naveen N. Rao @ 2017-06-21 18:38 UTC (permalink / raw)
To: Michael Ellerman
Cc: Ananth N Mavinakayanahalli, Masami Hiramatsu, Nicholas Piggin,
linuxppc-dev
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v3 1/6] powerpc64/elfv1: Validate function pointer address in the function descriptor
2017-06-21 18:38 [PATCH v3 0/6] powerpc: build out kprobes blacklist -- series 3 Naveen N. Rao
@ 2017-06-21 18:38 ` Naveen N. Rao
2017-06-22 3:22 ` Nicholas Piggin
2017-06-21 18:38 ` [PATCH v3 2/6] powerpc/64s: Convert .L__replay_interrupt_return to a local label Naveen N. Rao
` (4 subsequent siblings)
5 siblings, 1 reply; 24+ messages in thread
From: Naveen N. Rao @ 2017-06-21 18:38 UTC (permalink / raw)
To: Michael Ellerman
Cc: Ananth N Mavinakayanahalli, Masami Hiramatsu, Nicholas Piggin,
linuxppc-dev
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 <naveen.n.rao@linux.vnet.ibm.com>
---
arch/powerpc/include/asm/code-patching.h | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/include/asm/code-patching.h b/arch/powerpc/include/asm/code-patching.h
index abef812de7f8..ec54050be585 100644
--- a/arch/powerpc/include/asm/code-patching.h
+++ b/arch/powerpc/include/asm/code-patching.h
@@ -83,8 +83,16 @@ static inline unsigned long ppc_function_entry(void *func)
* On PPC64 ABIv1 the function pointer actually points to the
* function's descriptor. The first entry in the descriptor is the
* address of the function text.
+ *
+ * However, we may have received a pointer to an assembly symbol
+ * that may not be a function descriptor. Validate that the entry
+ * points to a valid kernel address and if not, return the pointer
+ * we received as is.
*/
- return ((func_descr_t *)func)->entry;
+ if (kernel_text_address(((func_descr_t *)func)->entry))
+ return ((func_descr_t *)func)->entry;
+ else
+ return (unsigned long)func;
#else
return (unsigned long)func;
#endif
--
2.13.1
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v3 2/6] powerpc/64s: Convert .L__replay_interrupt_return to a local label
2017-06-21 18:38 [PATCH v3 0/6] powerpc: build out kprobes blacklist -- series 3 Naveen N. Rao
2017-06-21 18:38 ` [PATCH v3 1/6] powerpc64/elfv1: Validate function pointer address in the function descriptor Naveen N. Rao
@ 2017-06-21 18:38 ` Naveen N. Rao
2017-06-22 3:23 ` Nicholas Piggin
2017-06-21 18:38 ` [PATCH v3 3/6] powerpc/64s: Blacklist system_call() and system_call_common() from kprobes Naveen N. Rao
` (3 subsequent siblings)
5 siblings, 1 reply; 24+ messages in thread
From: Naveen N. Rao @ 2017-06-21 18:38 UTC (permalink / raw)
To: Michael Ellerman
Cc: Ananth N Mavinakayanahalli, Masami Hiramatsu, Nicholas Piggin,
linuxppc-dev
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 <naveen.n.rao@linux.vnet.ibm.com>
---
arch/powerpc/kernel/exceptions-64s.S | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
index 07b79c2c70f8..2df6d7b3070f 100644
--- a/arch/powerpc/kernel/exceptions-64s.S
+++ b/arch/powerpc/kernel/exceptions-64s.S
@@ -1629,7 +1629,7 @@ _GLOBAL(__replay_interrupt)
* we don't give a damn about, so we don't bother storing them.
*/
mfmsr r12
- LOAD_REG_ADDR(r11, .L__replay_interrupt_return)
+ LOAD_REG_ADDR(r11, 1f)
mfcr r9
ori r12,r12,MSR_EE
cmpwi r3,0x900
@@ -1647,6 +1647,6 @@ FTR_SECTION_ELSE
cmpwi r3,0xa00
beq doorbell_super_common_msgclr
ALT_FTR_SECTION_END_IFSET(CPU_FTR_HVMODE)
-.L__replay_interrupt_return:
+1:
blr
--
2.13.1
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v3 3/6] powerpc/64s: Blacklist system_call() and system_call_common() from kprobes
2017-06-21 18:38 [PATCH v3 0/6] powerpc: build out kprobes blacklist -- series 3 Naveen N. Rao
2017-06-21 18:38 ` [PATCH v3 1/6] powerpc64/elfv1: Validate function pointer address in the function descriptor Naveen N. Rao
2017-06-21 18:38 ` [PATCH v3 2/6] powerpc/64s: Convert .L__replay_interrupt_return to a local label Naveen N. Rao
@ 2017-06-21 18:38 ` Naveen N. Rao
2017-06-22 3:36 ` Nicholas Piggin
2017-06-21 18:38 ` [PATCH v3 4/6] powerpc/64s: Un-blacklist system_call() " Naveen N. Rao
` (2 subsequent siblings)
5 siblings, 1 reply; 24+ messages in thread
From: Naveen N. Rao @ 2017-06-21 18:38 UTC (permalink / raw)
To: Michael Ellerman
Cc: Ananth N Mavinakayanahalli, Masami Hiramatsu, Nicholas Piggin,
linuxppc-dev
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 <naveen.n.rao@linux.vnet.ibm.com>
---
arch/powerpc/kernel/entry_64.S | 29 +++++++++++++++--------------
1 file changed, 15 insertions(+), 14 deletions(-)
diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
index da9486e2fd89..ef8e6615b8ba 100644
--- a/arch/powerpc/kernel/entry_64.S
+++ b/arch/powerpc/kernel/entry_64.S
@@ -52,12 +52,11 @@ exception_marker:
.section ".text"
.align 7
- .globl system_call_common
-system_call_common:
+_GLOBAL(system_call_common)
#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
BEGIN_FTR_SECTION
extrdi. r10, r12, 1, (63-MSR_TS_T_LG) /* transaction active? */
- bne tabort_syscall
+ bne .Ltabort_syscall
END_FTR_SECTION_IFSET(CPU_FTR_TM)
#endif
andi. r10,r12,MSR_PR
@@ -152,9 +151,9 @@ END_FW_FTR_SECTION_IFSET(FW_FEATURE_SPLPAR)
CURRENT_THREAD_INFO(r11, r1)
ld r10,TI_FLAGS(r11)
andi. r11,r10,_TIF_SYSCALL_DOTRACE
- bne syscall_dotrace /* does not return */
+ bne .Lsyscall_dotrace /* does not return */
cmpldi 0,r0,NR_syscalls
- bge- syscall_enosys
+ bge- .Lsyscall_enosys
system_call: /* label this so stack traces look sane */
/*
@@ -208,7 +207,7 @@ system_call: /* label this so stack traces look sane */
ld r9,TI_FLAGS(r12)
li r11,-MAX_ERRNO
andi. r0,r9,(_TIF_SYSCALL_DOTRACE|_TIF_SINGLESTEP|_TIF_USER_WORK_MASK|_TIF_PERSYSCALL_MASK)
- bne- syscall_exit_work
+ bne- .Lsyscall_exit_work
/* If MSR_FP and MSR_VEC are set in user msr, then no need to restore */
li r7,MSR_FP
@@ -217,12 +216,12 @@ system_call: /* label this so stack traces look sane */
#endif
and r0,r8,r7
cmpd r0,r7
- bne syscall_restore_math
+ bne .Lsyscall_restore_math
.Lsyscall_restore_math_cont:
cmpld r3,r11
ld r5,_CCR(r1)
- bge- syscall_error
+ bge- .Lsyscall_error
.Lsyscall_error_cont:
ld r7,_NIP(r1)
BEGIN_FTR_SECTION
@@ -248,13 +247,13 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
RFI
b . /* prevent speculative execution */
-syscall_error:
+.Lsyscall_error:
oris r5,r5,0x1000 /* Set SO bit in CR */
neg r3,r3
std r5,_CCR(r1)
b .Lsyscall_error_cont
-syscall_restore_math:
+.Lsyscall_restore_math:
/*
* Some initial tests from restore_math to avoid the heavyweight
* C code entry and MSR manipulations.
@@ -289,7 +288,7 @@ syscall_restore_math:
b .Lsyscall_restore_math_cont
/* Traced system call support */
-syscall_dotrace:
+.Lsyscall_dotrace:
bl save_nvgprs
addi r3,r1,STACK_FRAME_OVERHEAD
bl do_syscall_trace_enter
@@ -322,11 +321,11 @@ syscall_dotrace:
b .Lsyscall_exit
-syscall_enosys:
+.Lsyscall_enosys:
li r3,-ENOSYS
b .Lsyscall_exit
-syscall_exit_work:
+.Lsyscall_exit_work:
#ifdef CONFIG_PPC_BOOK3S
li r10,MSR_RI
mtmsrd r10,1 /* Restore RI */
@@ -386,7 +385,7 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
b ret_from_except
#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
-tabort_syscall:
+.Ltabort_syscall:
/* Firstly we need to enable TM in the kernel */
mfmsr r10
li r9, 1
@@ -412,6 +411,8 @@ tabort_syscall:
rfid
b . /* prevent speculative execution */
#endif
+_ASM_NOKPROBE_SYMBOL(system_call_common);
+_ASM_NOKPROBE_SYMBOL(system_call);
/* Save non-volatile GPRs, if not already saved. */
_GLOBAL(save_nvgprs)
--
2.13.1
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v3 4/6] powerpc/64s: Un-blacklist system_call() from kprobes
2017-06-21 18:38 [PATCH v3 0/6] powerpc: build out kprobes blacklist -- series 3 Naveen N. Rao
` (2 preceding siblings ...)
2017-06-21 18:38 ` [PATCH v3 3/6] powerpc/64s: Blacklist system_call() and system_call_common() from kprobes Naveen N. Rao
@ 2017-06-21 18:38 ` Naveen N. Rao
2017-06-22 3:41 ` Nicholas Piggin
2017-06-21 18:38 ` [PATCH v3 5/6] powerpc/64s: Blacklist functions invoked on a trap Naveen N. Rao
2017-06-21 18:38 ` [PATCH v3 6/6] powerpc/64s: Blacklist rtas entry/exit from kprobes Naveen N. Rao
5 siblings, 1 reply; 24+ messages in thread
From: Naveen N. Rao @ 2017-06-21 18:38 UTC (permalink / raw)
To: Michael Ellerman
Cc: Ananth N Mavinakayanahalli, Masami Hiramatsu, Nicholas Piggin,
linuxppc-dev
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 <naveen.n.rao@linux.vnet.ibm.com>
---
arch/powerpc/kernel/entry_64.S | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
index ef8e6615b8ba..feeeadc9aa71 100644
--- a/arch/powerpc/kernel/entry_64.S
+++ b/arch/powerpc/kernel/entry_64.S
@@ -204,6 +204,7 @@ system_call: /* label this so stack traces look sane */
mtmsrd r11,1
#endif /* CONFIG_PPC_BOOK3E */
+system_call_exit:
ld r9,TI_FLAGS(r12)
li r11,-MAX_ERRNO
andi. r0,r9,(_TIF_SYSCALL_DOTRACE|_TIF_SINGLESTEP|_TIF_USER_WORK_MASK|_TIF_PERSYSCALL_MASK)
@@ -412,7 +413,7 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
b . /* prevent speculative execution */
#endif
_ASM_NOKPROBE_SYMBOL(system_call_common);
-_ASM_NOKPROBE_SYMBOL(system_call);
+_ASM_NOKPROBE_SYMBOL(system_call_exit);
/* Save non-volatile GPRs, if not already saved. */
_GLOBAL(save_nvgprs)
--
2.13.1
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v3 5/6] powerpc/64s: Blacklist functions invoked on a trap
2017-06-21 18:38 [PATCH v3 0/6] powerpc: build out kprobes blacklist -- series 3 Naveen N. Rao
` (3 preceding siblings ...)
2017-06-21 18:38 ` [PATCH v3 4/6] powerpc/64s: Un-blacklist system_call() " Naveen N. Rao
@ 2017-06-21 18:38 ` Naveen N. Rao
2017-06-22 3:44 ` Nicholas Piggin
2017-06-21 18:38 ` [PATCH v3 6/6] powerpc/64s: Blacklist rtas entry/exit from kprobes Naveen N. Rao
5 siblings, 1 reply; 24+ messages in thread
From: Naveen N. Rao @ 2017-06-21 18:38 UTC (permalink / raw)
To: Michael Ellerman
Cc: Ananth N Mavinakayanahalli, Masami Hiramatsu, Nicholas Piggin,
linuxppc-dev
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 <naveen.n.rao@linux.vnet.ibm.com>
---
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(-)
diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
index feeeadc9aa71..d376f07153d7 100644
--- a/arch/powerpc/kernel/entry_64.S
+++ b/arch/powerpc/kernel/entry_64.S
@@ -184,7 +184,7 @@ system_call: /* label this so stack traces look sane */
#ifdef CONFIG_PPC_BOOK3S
/* No MSR:RI on BookE */
andi. r10,r8,MSR_RI
- beq- unrecov_restore
+ beq- .Lunrecov_restore
#endif
/*
* Disable interrupts so current_thread_info()->flags can't change,
@@ -424,6 +424,7 @@ _GLOBAL(save_nvgprs)
clrrdi r0,r11,1
std r0,_TRAP(r1)
blr
+_ASM_NOKPROBE_SYMBOL(save_nvgprs);
/*
@@ -672,18 +673,18 @@ _GLOBAL(ret_from_except_lite)
* Use the internal debug mode bit to do this.
*/
andis. r0,r3,DBCR0_IDM@h
- beq restore
+ beq fast_exc_return_irq
mfmsr r0
rlwinm r0,r0,0,~MSR_DE /* Clear MSR.DE */
mtmsr r0
mtspr SPRN_DBCR0,r3
li r10, -1
mtspr SPRN_DBSR,r10
- b restore
+ b fast_exc_return_irq
#else
addi r3,r1,STACK_FRAME_OVERHEAD
bl restore_math
- b restore
+ b fast_exc_return_irq
#endif
1: andi. r0,r4,_TIF_NEED_RESCHED
beq 2f
@@ -696,7 +697,7 @@ _GLOBAL(ret_from_except_lite)
bne 3f /* only restore TM if nothing else to do */
addi r3,r1,STACK_FRAME_OVERHEAD
bl restore_tm_state
- b restore
+ b fast_exc_return_irq
3:
#endif
bl save_nvgprs
@@ -748,14 +749,14 @@ resume_kernel:
#ifdef CONFIG_PREEMPT
/* Check if we need to preempt */
andi. r0,r4,_TIF_NEED_RESCHED
- beq+ restore
+ beq+ fast_exc_return_irq
/* Check that preempt_count() == 0 and interrupts are enabled */
lwz r8,TI_PREEMPT(r9)
cmpwi cr1,r8,0
ld r0,SOFTE(r1)
cmpdi r0,0
crandc eq,cr1*4+eq,eq
- bne restore
+ bne fast_exc_return_irq
/*
* Here we are preempting the current task. We want to make
@@ -786,7 +787,6 @@ resume_kernel:
.globl fast_exc_return_irq
fast_exc_return_irq:
-restore:
/*
* This is the main kernel exit path. First we check if we
* are about to re-enable interrupts
@@ -794,11 +794,11 @@ restore:
ld r5,SOFTE(r1)
lbz r6,PACASOFTIRQEN(r13)
cmpwi cr0,r5,0
- beq restore_irq_off
+ beq .Lrestore_irq_off
/* We are enabling, were we already enabled ? Yes, just return */
cmpwi cr0,r6,1
- beq cr0,do_restore
+ beq cr0,.Ldo_restore
/*
* We are about to soft-enable interrupts (we are hard disabled
@@ -807,14 +807,14 @@ restore:
*/
lbz r0,PACAIRQHAPPENED(r13)
cmpwi cr0,r0,0
- bne- restore_check_irq_replay
+ bne- .Lrestore_check_irq_replay
/*
* Get here when nothing happened while soft-disabled, just
* soft-enable and move-on. We will hard-enable as a side
* effect of rfi
*/
-restore_no_replay:
+.Lrestore_no_replay:
TRACE_ENABLE_INTS
li r0,1
stb r0,PACASOFTIRQEN(r13);
@@ -822,7 +822,7 @@ restore_no_replay:
/*
* Final return path. BookE is handled in a different file
*/
-do_restore:
+.Ldo_restore:
#ifdef CONFIG_PPC_BOOK3E
b exception_return_book3e
#else
@@ -856,7 +856,7 @@ fast_exception_return:
REST_8GPRS(5, r1)
andi. r0,r3,MSR_RI
- beq- unrecov_restore
+ beq- .Lunrecov_restore
/* Load PPR from thread struct before we clear MSR:RI */
BEGIN_FTR_SECTION
@@ -914,7 +914,7 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
* make sure that in this case, we also clear PACA_IRQ_HARD_DIS
* or that bit can get out of sync and bad things will happen
*/
-restore_irq_off:
+.Lrestore_irq_off:
ld r3,_MSR(r1)
lbz r7,PACAIRQHAPPENED(r13)
andi. r0,r3,MSR_EE
@@ -924,13 +924,13 @@ restore_irq_off:
1: li r0,0
stb r0,PACASOFTIRQEN(r13);
TRACE_DISABLE_INTS
- b do_restore
+ b .Ldo_restore
/*
* Something did happen, check if a re-emit is needed
* (this also clears paca->irq_happened)
*/
-restore_check_irq_replay:
+.Lrestore_check_irq_replay:
/* XXX: We could implement a fast path here where we check
* for irq_happened being just 0x01, in which case we can
* clear it and return. That means that we would potentially
@@ -940,7 +940,7 @@ restore_check_irq_replay:
*/
bl __check_irq_replay
cmpwi cr0,r3,0
- beq restore_no_replay
+ beq .Lrestore_no_replay
/*
* We need to re-emit an interrupt. We do so by re-using our
@@ -989,10 +989,17 @@ restore_check_irq_replay:
#endif /* CONFIG_PPC_DOORBELL */
1: b ret_from_except /* What else to do here ? */
-unrecov_restore:
+.Lunrecov_restore:
addi r3,r1,STACK_FRAME_OVERHEAD
bl unrecoverable_exception
- b unrecov_restore
+ b .Lunrecov_restore
+
+_ASM_NOKPROBE_SYMBOL(ret_from_except);
+_ASM_NOKPROBE_SYMBOL(ret_from_except_lite);
+_ASM_NOKPROBE_SYMBOL(resume_kernel);
+_ASM_NOKPROBE_SYMBOL(fast_exc_return_irq);
+_ASM_NOKPROBE_SYMBOL(fast_exception_return);
+
#ifdef CONFIG_PPC_RTAS
/*
diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
index 2df6d7b3070f..0d025dfb52d8 100644
--- a/arch/powerpc/kernel/exceptions-64s.S
+++ b/arch/powerpc/kernel/exceptions-64s.S
@@ -1589,6 +1589,7 @@ END_FTR_SECTION_IFSET(CPU_FTR_CFAR)
1: addi r3,r1,STACK_FRAME_OVERHEAD
bl kernel_bad_stack
b 1b
+_ASM_NOKPROBE_SYMBOL(bad_stack);
/*
* When doorbell is triggered from system reset wakeup, the message is
@@ -1650,3 +1651,4 @@ ALT_FTR_SECTION_END_IFSET(CPU_FTR_HVMODE)
1:
blr
+_ASM_NOKPROBE_SYMBOL(__replay_interrupt)
diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index d4e545d27ef9..bfcfd9ef09f2 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -237,6 +237,7 @@ void die(const char *str, struct pt_regs *regs, long err)
err = 0;
oops_end(flags, regs, err);
}
+NOKPROBE_SYMBOL(die);
void user_single_step_siginfo(struct task_struct *tsk,
struct pt_regs *regs, siginfo_t *info)
@@ -1968,6 +1969,7 @@ void unrecoverable_exception(struct pt_regs *regs)
regs->trap, regs->nip);
die("Unrecoverable exception", regs, SIGABRT);
}
+NOKPROBE_SYMBOL(unrecoverable_exception);
#if defined(CONFIG_BOOKE_WDT) || defined(CONFIG_40x)
/*
@@ -1998,6 +2000,7 @@ void kernel_bad_stack(struct pt_regs *regs)
regs->gpr[1], regs->nip);
die("Bad kernel stack pointer", regs, SIGABRT);
}
+NOKPROBE_SYMBOL(kernel_bad_stack);
void __init trap_init(void)
{
--
2.13.1
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v3 6/6] powerpc/64s: Blacklist rtas entry/exit from kprobes
2017-06-21 18:38 [PATCH v3 0/6] powerpc: build out kprobes blacklist -- series 3 Naveen N. Rao
` (4 preceding siblings ...)
2017-06-21 18:38 ` [PATCH v3 5/6] powerpc/64s: Blacklist functions invoked on a trap Naveen N. Rao
@ 2017-06-21 18:38 ` Naveen N. Rao
2017-06-22 3:48 ` Nicholas Piggin
5 siblings, 1 reply; 24+ messages in thread
From: Naveen N. Rao @ 2017-06-21 18:38 UTC (permalink / raw)
To: Michael Ellerman
Cc: Ananth N Mavinakayanahalli, Masami Hiramatsu, Nicholas Piggin,
linuxppc-dev
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 <naveen.n.rao@linux.vnet.ibm.com>
---
arch/powerpc/kernel/entry_64.S | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
index d376f07153d7..49c35450f399 100644
--- a/arch/powerpc/kernel/entry_64.S
+++ b/arch/powerpc/kernel/entry_64.S
@@ -1076,6 +1076,8 @@ _GLOBAL(enter_rtas)
rldicr r9,r9,MSR_SF_LG,(63-MSR_SF_LG)
ori r9,r9,MSR_IR|MSR_DR|MSR_FE0|MSR_FE1|MSR_FP|MSR_RI|MSR_LE
andc r6,r0,r9
+
+__enter_rtas:
sync /* disable interrupts so SRR0/1 */
mtmsrd r0 /* don't get trashed */
@@ -1112,6 +1114,8 @@ rtas_return_loc:
mtspr SPRN_SRR1,r4
rfid
b . /* prevent speculative execution */
+_ASM_NOKPROBE_SYMBOL(__enter_rtas)
+_ASM_NOKPROBE_SYMBOL(rtas_return_loc)
.align 3
1: .llong rtas_restore_regs
--
2.13.1
^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH v3 1/6] powerpc64/elfv1: Validate function pointer address in the function descriptor
2017-06-21 18:38 ` [PATCH v3 1/6] powerpc64/elfv1: Validate function pointer address in the function descriptor Naveen N. Rao
@ 2017-06-22 3:22 ` Nicholas Piggin
2017-06-22 10:59 ` Michael Ellerman
0 siblings, 1 reply; 24+ messages in thread
From: Nicholas Piggin @ 2017-06-22 3:22 UTC (permalink / raw)
To: Naveen N. Rao
Cc: Michael Ellerman, Ananth N Mavinakayanahalli, Masami Hiramatsu,
linuxppc-dev
On Thu, 22 Jun 2017 00:08:37 +0530
"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> 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 <naveen.n.rao@linux.vnet.ibm.com>
> ---
> arch/powerpc/include/asm/code-patching.h | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/include/asm/code-patching.h b/arch/powerpc/include/asm/code-patching.h
> index abef812de7f8..ec54050be585 100644
> --- a/arch/powerpc/include/asm/code-patching.h
> +++ b/arch/powerpc/include/asm/code-patching.h
> @@ -83,8 +83,16 @@ static inline unsigned long ppc_function_entry(void *func)
> * On PPC64 ABIv1 the function pointer actually points to the
> * function's descriptor. The first entry in the descriptor is the
> * address of the function text.
> + *
> + * However, we may have received a pointer to an assembly symbol
> + * that may not be a function descriptor. Validate that the entry
> + * points to a valid kernel address and if not, return the pointer
> + * we received as is.
> */
> - return ((func_descr_t *)func)->entry;
> + if (kernel_text_address(((func_descr_t *)func)->entry))
> + return ((func_descr_t *)func)->entry;
> + else
> + return (unsigned long)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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 2/6] powerpc/64s: Convert .L__replay_interrupt_return to a local label
2017-06-21 18:38 ` [PATCH v3 2/6] powerpc/64s: Convert .L__replay_interrupt_return to a local label Naveen N. Rao
@ 2017-06-22 3:23 ` Nicholas Piggin
0 siblings, 0 replies; 24+ messages in thread
From: Nicholas Piggin @ 2017-06-22 3:23 UTC (permalink / raw)
To: Naveen N. Rao
Cc: Michael Ellerman, Ananth N Mavinakayanahalli, Masami Hiramatsu,
linuxppc-dev
On Thu, 22 Jun 2017 00:08:38 +0530
"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> 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>
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
> arch/powerpc/kernel/exceptions-64s.S | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
> index 07b79c2c70f8..2df6d7b3070f 100644
> --- a/arch/powerpc/kernel/exceptions-64s.S
> +++ b/arch/powerpc/kernel/exceptions-64s.S
> @@ -1629,7 +1629,7 @@ _GLOBAL(__replay_interrupt)
> * we don't give a damn about, so we don't bother storing them.
> */
> mfmsr r12
> - LOAD_REG_ADDR(r11, .L__replay_interrupt_return)
> + LOAD_REG_ADDR(r11, 1f)
> mfcr r9
> ori r12,r12,MSR_EE
> cmpwi r3,0x900
> @@ -1647,6 +1647,6 @@ FTR_SECTION_ELSE
> cmpwi r3,0xa00
> beq doorbell_super_common_msgclr
> ALT_FTR_SECTION_END_IFSET(CPU_FTR_HVMODE)
> -.L__replay_interrupt_return:
> +1:
> blr
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 3/6] powerpc/64s: Blacklist system_call() and system_call_common() from kprobes
2017-06-21 18:38 ` [PATCH v3 3/6] powerpc/64s: Blacklist system_call() and system_call_common() from kprobes Naveen N. Rao
@ 2017-06-22 3:36 ` Nicholas Piggin
2017-06-22 11:07 ` Michael Ellerman
0 siblings, 1 reply; 24+ messages in thread
From: Nicholas Piggin @ 2017-06-22 3:36 UTC (permalink / raw)
To: Naveen N. Rao
Cc: Michael Ellerman, Ananth N Mavinakayanahalli, Masami Hiramatsu,
linuxppc-dev
On Thu, 22 Jun 2017 00:08:39 +0530
"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> 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 <naveen.n.rao@linux.vnet.ibm.com>
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 4/6] powerpc/64s: Un-blacklist system_call() from kprobes
2017-06-21 18:38 ` [PATCH v3 4/6] powerpc/64s: Un-blacklist system_call() " Naveen N. Rao
@ 2017-06-22 3:41 ` Nicholas Piggin
2017-06-22 11:14 ` Michael Ellerman
0 siblings, 1 reply; 24+ messages in thread
From: Nicholas Piggin @ 2017-06-22 3:41 UTC (permalink / raw)
To: Naveen N. Rao
Cc: Michael Ellerman, Ananth N Mavinakayanahalli, Masami Hiramatsu,
linuxppc-dev
On Thu, 22 Jun 2017 00:08:40 +0530
"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> 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
>
> Suggested-by: Michael Ellerman <mpe@ellerman.id.au>
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
> arch/powerpc/kernel/entry_64.S | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
> index ef8e6615b8ba..feeeadc9aa71 100644
> --- a/arch/powerpc/kernel/entry_64.S
> +++ b/arch/powerpc/kernel/entry_64.S
> @@ -204,6 +204,7 @@ system_call: /* label this so stack traces look sane */
> mtmsrd r11,1
> #endif /* CONFIG_PPC_BOOK3E */
>
> +system_call_exit:
> ld r9,TI_FLAGS(r12)
> li r11,-MAX_ERRNO
> andi. r0,r9,(_TIF_SYSCALL_DOTRACE|_TIF_SINGLESTEP|_TIF_USER_WORK_MASK|_TIF_PERSYSCALL_MASK)
> @@ -412,7 +413,7 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
> b . /* prevent speculative execution */
> #endif
> _ASM_NOKPROBE_SYMBOL(system_call_common);
> -_ASM_NOKPROBE_SYMBOL(system_call);
> +_ASM_NOKPROBE_SYMBOL(system_call_exit);
>
> /* Save non-volatile GPRs, if not already saved. */
> _GLOBAL(save_nvgprs)
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 5/6] powerpc/64s: Blacklist functions invoked on a trap
2017-06-21 18:38 ` [PATCH v3 5/6] powerpc/64s: Blacklist functions invoked on a trap Naveen N. Rao
@ 2017-06-22 3:44 ` Nicholas Piggin
2017-06-22 11:12 ` Michael Ellerman
0 siblings, 1 reply; 24+ messages in thread
From: Nicholas Piggin @ 2017-06-22 3:44 UTC (permalink / raw)
To: Naveen N. Rao
Cc: Michael Ellerman, Ananth N Mavinakayanahalli, Masami Hiramatsu,
linuxppc-dev
On Thu, 22 Jun 2017 00:08:41 +0530
"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> 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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 6/6] powerpc/64s: Blacklist rtas entry/exit from kprobes
2017-06-21 18:38 ` [PATCH v3 6/6] powerpc/64s: Blacklist rtas entry/exit from kprobes Naveen N. Rao
@ 2017-06-22 3:48 ` Nicholas Piggin
2017-06-22 16:52 ` Naveen N. Rao
0 siblings, 1 reply; 24+ messages in thread
From: Nicholas Piggin @ 2017-06-22 3:48 UTC (permalink / raw)
To: Naveen N. Rao
Cc: Michael Ellerman, Ananth N Mavinakayanahalli, Masami Hiramatsu,
linuxppc-dev
On Thu, 22 Jun 2017 00:08:42 +0530
"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> 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 <naveen.n.rao@linux.vnet.ibm.com>
> ---
> arch/powerpc/kernel/entry_64.S | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
> index d376f07153d7..49c35450f399 100644
> --- a/arch/powerpc/kernel/entry_64.S
> +++ b/arch/powerpc/kernel/entry_64.S
> @@ -1076,6 +1076,8 @@ _GLOBAL(enter_rtas)
> rldicr r9,r9,MSR_SF_LG,(63-MSR_SF_LG)
> ori r9,r9,MSR_IR|MSR_DR|MSR_FE0|MSR_FE1|MSR_FP|MSR_RI|MSR_LE
> andc r6,r0,r9
> +
> +__enter_rtas:
> sync /* disable interrupts so SRR0/1 */
> mtmsrd r0 /* 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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 1/6] powerpc64/elfv1: Validate function pointer address in the function descriptor
2017-06-22 3:22 ` Nicholas Piggin
@ 2017-06-22 10:59 ` Michael Ellerman
2017-06-22 13:06 ` Nicholas Piggin
0 siblings, 1 reply; 24+ messages in thread
From: Michael Ellerman @ 2017-06-22 10:59 UTC (permalink / raw)
To: Nicholas Piggin, Naveen N. Rao
Cc: Ananth N Mavinakayanahalli, Masami Hiramatsu, linuxppc-dev
Nicholas Piggin <npiggin@gmail.com> writes:
> On Thu, 22 Jun 2017 00:08:37 +0530
> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
>
>> 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 <naveen.n.rao@linux.vnet.ibm.com>
>> ---
>> arch/powerpc/include/asm/code-patching.h | 10 +++++++++-
>> 1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/powerpc/include/asm/code-patching.h b/arch/powerpc/include/asm/code-patching.h
>> index abef812de7f8..ec54050be585 100644
>> --- a/arch/powerpc/include/asm/code-patching.h
>> +++ b/arch/powerpc/include/asm/code-patching.h
>> @@ -83,8 +83,16 @@ static inline unsigned long ppc_function_entry(void *func)
>> * On PPC64 ABIv1 the function pointer actually points to the
>> * function's descriptor. The first entry in the descriptor is the
>> * address of the function text.
>> + *
>> + * However, we may have received a pointer to an assembly symbol
>> + * that may not be a function descriptor. Validate that the entry
>> + * points to a valid kernel address and if not, return the pointer
>> + * we received as is.
>> */
>> - return ((func_descr_t *)func)->entry;
>> + if (kernel_text_address(((func_descr_t *)func)->entry))
>> + return ((func_descr_t *)func)->entry;
>> + else
>> + return (unsigned long)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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 3/6] powerpc/64s: Blacklist system_call() and system_call_common() from kprobes
2017-06-22 3:36 ` Nicholas Piggin
@ 2017-06-22 11:07 ` Michael Ellerman
2017-06-22 13:08 ` Nicholas Piggin
0 siblings, 1 reply; 24+ messages in thread
From: Michael Ellerman @ 2017-06-22 11:07 UTC (permalink / raw)
To: Nicholas Piggin, Naveen N. Rao
Cc: Ananth N Mavinakayanahalli, Masami Hiramatsu, linuxppc-dev
Nicholas Piggin <npiggin@gmail.com> writes:
> On Thu, 22 Jun 2017 00:08:39 +0530
> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> 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 <naveen.n.rao@linux.vnet.ibm.com>
>
> 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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 5/6] powerpc/64s: Blacklist functions invoked on a trap
2017-06-22 3:44 ` Nicholas Piggin
@ 2017-06-22 11:12 ` Michael Ellerman
0 siblings, 0 replies; 24+ messages in thread
From: Michael Ellerman @ 2017-06-22 11:12 UTC (permalink / raw)
To: Nicholas Piggin, Naveen N. Rao
Cc: Ananth N Mavinakayanahalli, Masami Hiramatsu, linuxppc-dev
Nicholas Piggin <npiggin@gmail.com> writes:
> On Thu, 22 Jun 2017 00:08:41 +0530
> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> 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.
Yeah it bloats the patch needlessly, we can do a rename patch later.
cheers
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 4/6] powerpc/64s: Un-blacklist system_call() from kprobes
2017-06-22 3:41 ` Nicholas Piggin
@ 2017-06-22 11:14 ` Michael Ellerman
2017-06-22 13:14 ` Nicholas Piggin
0 siblings, 1 reply; 24+ messages in thread
From: Michael Ellerman @ 2017-06-22 11:14 UTC (permalink / raw)
To: Nicholas Piggin, Naveen N. Rao
Cc: Ananth N Mavinakayanahalli, Masami Hiramatsu, linuxppc-dev
Nicholas Piggin <npiggin@gmail.com> writes:
> On Thu, 22 Jun 2017 00:08:40 +0530
> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> 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).
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 1/6] powerpc64/elfv1: Validate function pointer address in the function descriptor
2017-06-22 10:59 ` Michael Ellerman
@ 2017-06-22 13:06 ` Nicholas Piggin
2017-06-22 14:01 ` Naveen N. Rao
0 siblings, 1 reply; 24+ messages in thread
From: Nicholas Piggin @ 2017-06-22 13:06 UTC (permalink / raw)
To: Michael Ellerman
Cc: Naveen N. Rao, Ananth N Mavinakayanahalli, Masami Hiramatsu,
linuxppc-dev
On Thu, 22 Jun 2017 20:59:49 +1000
Michael Ellerman <mpe@ellerman.id.au> wrote:
> Nicholas Piggin <npiggin@gmail.com> writes:
>
> > On Thu, 22 Jun 2017 00:08:37 +0530
> > "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> >
> >> 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 <naveen.n.rao@linux.vnet.ibm.com>
> >> ---
> >> arch/powerpc/include/asm/code-patching.h | 10 +++++++++-
> >> 1 file changed, 9 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/arch/powerpc/include/asm/code-patching.h b/arch/powerpc/include/asm/code-patching.h
> >> index abef812de7f8..ec54050be585 100644
> >> --- a/arch/powerpc/include/asm/code-patching.h
> >> +++ b/arch/powerpc/include/asm/code-patching.h
> >> @@ -83,8 +83,16 @@ static inline unsigned long ppc_function_entry(void *func)
> >> * On PPC64 ABIv1 the function pointer actually points to the
> >> * function's descriptor. The first entry in the descriptor is the
> >> * address of the function text.
> >> + *
> >> + * However, we may have received a pointer to an assembly symbol
> >> + * that may not be a function descriptor. Validate that the entry
> >> + * points to a valid kernel address and if not, return the pointer
> >> + * we received as is.
> >> */
> >> - return ((func_descr_t *)func)->entry;
> >> + if (kernel_text_address(((func_descr_t *)func)->entry))
> >> + return ((func_descr_t *)func)->entry;
> >> + else
> >> + return (unsigned long)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.
kernel_text_address() seems to check for module text as well, so it
might work I think?
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 3/6] powerpc/64s: Blacklist system_call() and system_call_common() from kprobes
2017-06-22 11:07 ` Michael Ellerman
@ 2017-06-22 13:08 ` Nicholas Piggin
2017-06-22 14:34 ` Naveen N. Rao
0 siblings, 1 reply; 24+ messages in thread
From: Nicholas Piggin @ 2017-06-22 13:08 UTC (permalink / raw)
To: Michael Ellerman
Cc: Naveen N. Rao, Ananth N Mavinakayanahalli, Masami Hiramatsu,
linuxppc-dev
On Thu, 22 Jun 2017 21:07:46 +1000
Michael Ellerman <mpe@ellerman.id.au> wrote:
> Nicholas Piggin <npiggin@gmail.com> writes:
>
> > On Thu, 22 Jun 2017 00:08:39 +0530
> > "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> 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 <naveen.n.rao@linux.vnet.ibm.com>
> >
> > 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.
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 4/6] powerpc/64s: Un-blacklist system_call() from kprobes
2017-06-22 11:14 ` Michael Ellerman
@ 2017-06-22 13:14 ` Nicholas Piggin
2017-06-22 15:43 ` Naveen N. Rao
0 siblings, 1 reply; 24+ messages in thread
From: Nicholas Piggin @ 2017-06-22 13:14 UTC (permalink / raw)
To: Michael Ellerman
Cc: Naveen N. Rao, Ananth N Mavinakayanahalli, Masami Hiramatsu,
linuxppc-dev
On Thu, 22 Jun 2017 21:14:21 +1000
Michael Ellerman <mpe@ellerman.id.au> wrote:
> Nicholas Piggin <npiggin@gmail.com> writes:
>
> > On Thu, 22 Jun 2017 00:08:40 +0530
> > "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> 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).
>
> 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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 1/6] powerpc64/elfv1: Validate function pointer address in the function descriptor
2017-06-22 13:06 ` Nicholas Piggin
@ 2017-06-22 14:01 ` Naveen N. Rao
0 siblings, 0 replies; 24+ messages in thread
From: Naveen N. Rao @ 2017-06-22 14:01 UTC (permalink / raw)
To: Nicholas Piggin
Cc: Michael Ellerman, Ananth N Mavinakayanahalli, Masami Hiramatsu,
linuxppc-dev
On 2017/06/22 11:06PM, Nicholas Piggin wrote:
> On Thu, 22 Jun 2017 20:59:49 +1000
> Michael Ellerman <mpe@ellerman.id.au> wrote:
>
> > Nicholas Piggin <npiggin@gmail.com> writes:
> >
> > > On Thu, 22 Jun 2017 00:08:37 +0530
> > > "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> > >
> > >> 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 <naveen.n.rao@linux.vnet.ibm.com>
> > >> ---
> > >> arch/powerpc/include/asm/code-patching.h | 10 +++++++++-
> > >> 1 file changed, 9 insertions(+), 1 deletion(-)
> > >>
> > >> diff --git a/arch/powerpc/include/asm/code-patching.h b/arch/powerpc/include/asm/code-patching.h
> > >> index abef812de7f8..ec54050be585 100644
> > >> --- a/arch/powerpc/include/asm/code-patching.h
> > >> +++ b/arch/powerpc/include/asm/code-patching.h
> > >> @@ -83,8 +83,16 @@ static inline unsigned long ppc_function_entry(void *func)
> > >> * On PPC64 ABIv1 the function pointer actually points to the
> > >> * function's descriptor. The first entry in the descriptor is the
> > >> * address of the function text.
> > >> + *
> > >> + * However, we may have received a pointer to an assembly symbol
> > >> + * that may not be a function descriptor. Validate that the entry
> > >> + * points to a valid kernel address and if not, return the pointer
> > >> + * we received as is.
> > >> */
> > >> - return ((func_descr_t *)func)->entry;
> > >> + if (kernel_text_address(((func_descr_t *)func)->entry))
> > >> + return ((func_descr_t *)func)->entry;
> > >> + else
> > >> + return (unsigned long)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.
> >
> > > 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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 3/6] powerpc/64s: Blacklist system_call() and system_call_common() from kprobes
2017-06-22 13:08 ` Nicholas Piggin
@ 2017-06-22 14:34 ` Naveen N. Rao
0 siblings, 0 replies; 24+ messages in thread
From: Naveen N. Rao @ 2017-06-22 14:34 UTC (permalink / raw)
To: Nicholas Piggin
Cc: Michael Ellerman, Ananth N Mavinakayanahalli, Masami Hiramatsu,
linuxppc-dev
On 2017/06/22 11:08PM, Nicholas Piggin wrote:
> On Thu, 22 Jun 2017 21:07:46 +1000
> Michael Ellerman <mpe@ellerman.id.au> wrote:
>
> > Nicholas Piggin <npiggin@gmail.com> writes:
> >
> > > On Thu, 22 Jun 2017 00:08:39 +0530
> > > "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> 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 <naveen.n.rao@linux.vnet.ibm.com>
> > >
> > > 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.
> > >
> > > 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.
>
> 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.
Sure, I'll add a patch that does that.
- Naveen
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 4/6] powerpc/64s: Un-blacklist system_call() from kprobes
2017-06-22 13:14 ` Nicholas Piggin
@ 2017-06-22 15:43 ` Naveen N. Rao
0 siblings, 0 replies; 24+ messages in thread
From: Naveen N. Rao @ 2017-06-22 15:43 UTC (permalink / raw)
To: Nicholas Piggin
Cc: Michael Ellerman, Ananth N Mavinakayanahalli, Masami Hiramatsu,
linuxppc-dev
On 2017/06/22 11:14PM, Nicholas Piggin wrote:
> On Thu, 22 Jun 2017 21:14:21 +1000
> Michael Ellerman <mpe@ellerman.id.au> wrote:
>
> > Nicholas Piggin <npiggin@gmail.com> writes:
> >
> > > On Thu, 22 Jun 2017 00:08:40 +0530
> > > "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> 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?
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 (*)
> > >
> > > 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.
>
> > 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):
--- a/arch/powerpc/kernel/entry_64.S
+++ b/arch/powerpc/kernel/entry_64.S
@@ -175,8 +175,9 @@ system_call: /* label this so stack traces look sane */
ldx r12,r11,r0 /* Fetch system call handler [ptr] */
mtctr r12
bctrl /* Call handler */
+ nop
-.Lsyscall_exit:
+system_call_exit:
std r3,RESULT(r1)
CURRENT_THREAD_INFO(r12, r1)
Thanks,
Naveen
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 6/6] powerpc/64s: Blacklist rtas entry/exit from kprobes
2017-06-22 3:48 ` Nicholas Piggin
@ 2017-06-22 16:52 ` Naveen N. Rao
0 siblings, 0 replies; 24+ messages in thread
From: Naveen N. Rao @ 2017-06-22 16:52 UTC (permalink / raw)
To: Nicholas Piggin
Cc: Michael Ellerman, Ananth N Mavinakayanahalli, Masami Hiramatsu,
linuxppc-dev
On 2017/06/22 01:48PM, Nicholas Piggin wrote:
> On Thu, 22 Jun 2017 00:08:42 +0530
> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
>
> > 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 <naveen.n.rao@linux.vnet.ibm.com>
> > ---
> > arch/powerpc/kernel/entry_64.S | 4 ++++
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
> > index d376f07153d7..49c35450f399 100644
> > --- a/arch/powerpc/kernel/entry_64.S
> > +++ b/arch/powerpc/kernel/entry_64.S
> > @@ -1076,6 +1076,8 @@ _GLOBAL(enter_rtas)
> > rldicr r9,r9,MSR_SF_LG,(63-MSR_SF_LG)
> > ori r9,r9,MSR_IR|MSR_DR|MSR_FE0|MSR_FE1|MSR_FP|MSR_RI|MSR_LE
> > andc r6,r0,r9
> > +
> > +__enter_rtas:
> > sync /* disable interrupts so SRR0/1 */
> > mtmsrd r0 /* 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
^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2017-06-22 16:53 UTC | newest]
Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-06-21 18:38 [PATCH v3 0/6] powerpc: build out kprobes blacklist -- series 3 Naveen N. Rao
2017-06-21 18:38 ` [PATCH v3 1/6] powerpc64/elfv1: Validate function pointer address in the function descriptor Naveen N. Rao
2017-06-22 3:22 ` Nicholas Piggin
2017-06-22 10:59 ` Michael Ellerman
2017-06-22 13:06 ` Nicholas Piggin
2017-06-22 14:01 ` Naveen N. Rao
2017-06-21 18:38 ` [PATCH v3 2/6] powerpc/64s: Convert .L__replay_interrupt_return to a local label Naveen N. Rao
2017-06-22 3:23 ` Nicholas Piggin
2017-06-21 18:38 ` [PATCH v3 3/6] powerpc/64s: Blacklist system_call() and system_call_common() from kprobes Naveen N. Rao
2017-06-22 3:36 ` Nicholas Piggin
2017-06-22 11:07 ` Michael Ellerman
2017-06-22 13:08 ` Nicholas Piggin
2017-06-22 14:34 ` Naveen N. Rao
2017-06-21 18:38 ` [PATCH v3 4/6] powerpc/64s: Un-blacklist system_call() " Naveen N. Rao
2017-06-22 3:41 ` Nicholas Piggin
2017-06-22 11:14 ` Michael Ellerman
2017-06-22 13:14 ` Nicholas Piggin
2017-06-22 15:43 ` Naveen N. Rao
2017-06-21 18:38 ` [PATCH v3 5/6] powerpc/64s: Blacklist functions invoked on a trap Naveen N. Rao
2017-06-22 3:44 ` Nicholas Piggin
2017-06-22 11:12 ` Michael Ellerman
2017-06-21 18:38 ` [PATCH v3 6/6] powerpc/64s: Blacklist rtas entry/exit from kprobes Naveen N. Rao
2017-06-22 3:48 ` Nicholas Piggin
2017-06-22 16:52 ` Naveen N. Rao
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.