* [PATCH v4 0/7] powerpc: build out kprobes blacklist -- series 3
@ 2017-06-29 10:41 Naveen N. Rao
2017-06-29 10:41 ` [PATCH v4 1/7] powerpc64/elfv1: Only dereference function descriptor for non-text symbols Naveen N. Rao
` (6 more replies)
0 siblings, 7 replies; 17+ messages in thread
From: Naveen N. Rao @ 2017-06-29 10:41 UTC (permalink / raw)
To: Michael Ellerman
Cc: Nicholas Piggin, Ananth N Mavinakayanahalli, Masami Hiramatsu,
Anton Blanchard, 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 v3:
- Patch 1 now implements a different approach by checking if the function
pointer points to .text or not.
- Patch 4 is new, as suggested by Nick.
- Patch 6 (previously 5) changed to leave 'restore' symbol alone.
- Patch 7 (previously 6) moves __rtas_enter after the mtmsr.
- Patches 2, 3 and 5 (previously numbered 4) are unchanged
v3:
https://www.mail-archive.com/linuxppc-dev@lists.ozlabs.org/msg119596.html
(*) series 2:
https://www.mail-archive.com/linuxppc-dev@lists.ozlabs.org/msg117562.html
- Naveen
Naveen N. Rao (7):
powerpc64/elfv1: Only dereference function descriptor for non-text
symbols
powerpc/64s: Convert .L__replay_interrupt_return to a local label
powerpc/64s: Blacklist system_call() and system_call_common() from
kprobes
powerpc/64s: Move system_call() symbol to just after setting MSR_EE
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 | 75 +++++++++++++++++++-------------
arch/powerpc/kernel/exceptions-64s.S | 6 ++-
arch/powerpc/kernel/traps.c | 3 ++
4 files changed, 61 insertions(+), 33 deletions(-)
--
2.13.1
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v4 1/7] powerpc64/elfv1: Only dereference function descriptor for non-text symbols
2017-06-29 10:41 [PATCH v4 0/7] powerpc: build out kprobes blacklist -- series 3 Naveen N. Rao
@ 2017-06-29 10:41 ` Naveen N. Rao
2017-06-29 10:49 ` Nicholas Piggin
2017-06-29 10:41 ` [PATCH v4 2/7] powerpc/64s: Convert .L__replay_interrupt_return to a local label Naveen N. Rao
` (5 subsequent siblings)
6 siblings, 1 reply; 17+ messages in thread
From: Naveen N. Rao @ 2017-06-29 10:41 UTC (permalink / raw)
To: Michael Ellerman
Cc: Nicholas Piggin, Ananth N Mavinakayanahalli, Masami Hiramatsu,
Anton Blanchard, 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 function pointer we receive in
ppc_function_entry() already points to kernel text. If so, we just
return it as is. If not, we assume that this is a function descriptor
and proceed to dereference it.
Suggested-by: Nicholas Piggin <npiggin@gmail.com>
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..5482928eea1b 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 also receive pointer to an assembly symbol. To
+ * detect that, we first check if the function pointer we receive
+ * already points to kernel/module text and we only dereference it
+ * if it doesn't.
*/
- return ((func_descr_t *)func)->entry;
+ if (kernel_text_address((unsigned long)func))
+ return (unsigned long)func;
+ else
+ return ((func_descr_t *)func)->entry;
#else
return (unsigned long)func;
#endif
--
2.13.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v4 2/7] powerpc/64s: Convert .L__replay_interrupt_return to a local label
2017-06-29 10:41 [PATCH v4 0/7] powerpc: build out kprobes blacklist -- series 3 Naveen N. Rao
2017-06-29 10:41 ` [PATCH v4 1/7] powerpc64/elfv1: Only dereference function descriptor for non-text symbols Naveen N. Rao
@ 2017-06-29 10:41 ` Naveen N. Rao
2017-06-29 10:41 ` [PATCH v4 3/7] powerpc/64s: Blacklist system_call() and system_call_common() from kprobes Naveen N. Rao
` (4 subsequent siblings)
6 siblings, 0 replies; 17+ messages in thread
From: Naveen N. Rao @ 2017-06-29 10:41 UTC (permalink / raw)
To: Michael Ellerman
Cc: Nicholas Piggin, Ananth N Mavinakayanahalli, Masami Hiramatsu,
Anton Blanchard, 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>
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 02a82777fd5b..3262e447b895 100644
--- a/arch/powerpc/kernel/exceptions-64s.S
+++ b/arch/powerpc/kernel/exceptions-64s.S
@@ -1631,7 +1631,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
@@ -1649,6 +1649,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] 17+ messages in thread
* [PATCH v4 3/7] powerpc/64s: Blacklist system_call() and system_call_common() from kprobes
2017-06-29 10:41 [PATCH v4 0/7] powerpc: build out kprobes blacklist -- series 3 Naveen N. Rao
2017-06-29 10:41 ` [PATCH v4 1/7] powerpc64/elfv1: Only dereference function descriptor for non-text symbols Naveen N. Rao
2017-06-29 10:41 ` [PATCH v4 2/7] powerpc/64s: Convert .L__replay_interrupt_return to a local label Naveen N. Rao
@ 2017-06-29 10:41 ` Naveen N. Rao
2017-06-29 10:41 ` [PATCH v4 4/7] powerpc/64s: Move system_call() symbol to just after setting MSR_EE Naveen N. Rao
` (3 subsequent siblings)
6 siblings, 0 replies; 17+ messages in thread
From: Naveen N. Rao @ 2017-06-29 10:41 UTC (permalink / raw)
To: Michael Ellerman
Cc: Nicholas Piggin, Ananth N Mavinakayanahalli, Masami Hiramatsu,
Anton Blanchard, 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>
Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
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] 17+ messages in thread
* [PATCH v4 4/7] powerpc/64s: Move system_call() symbol to just after setting MSR_EE
2017-06-29 10:41 [PATCH v4 0/7] powerpc: build out kprobes blacklist -- series 3 Naveen N. Rao
` (2 preceding siblings ...)
2017-06-29 10:41 ` [PATCH v4 3/7] powerpc/64s: Blacklist system_call() and system_call_common() from kprobes Naveen N. Rao
@ 2017-06-29 10:41 ` Naveen N. Rao
2017-06-29 11:44 ` Nicholas Piggin
2017-06-29 10:41 ` [PATCH v4 5/7] powerpc/64s: Un-blacklist system_call() from kprobes Naveen N. Rao
` (2 subsequent siblings)
6 siblings, 1 reply; 17+ messages in thread
From: Naveen N. Rao @ 2017-06-29 10:41 UTC (permalink / raw)
To: Michael Ellerman
Cc: Nicholas Piggin, Ananth N Mavinakayanahalli, Masami Hiramatsu,
Anton Blanchard, linuxppc-dev
It is common to get a PMU interrupt right after the mtmsr instruction that
enables interrupts. Due to this, the stack trace profile gets needlessly split
across system_call_common() and system_call().
Previously, system_call() symbol was at the current place to hide a few
earlier symbols which have since been made private or removed entirely.
So, let's move system_call() slightly higher up, right after the mtmsr
instruction that enables interrupts. Convert existing references to
system_call to a local syscall symbol.
Suggested-by: Nicholas Piggin <npiggin@gmail.com>
Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
arch/powerpc/kernel/entry_64.S | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
index ef8e6615b8ba..c39436706555 100644
--- a/arch/powerpc/kernel/entry_64.S
+++ b/arch/powerpc/kernel/entry_64.S
@@ -142,6 +142,7 @@ END_FW_FTR_SECTION_IFSET(FW_FEATURE_SPLPAR)
mtmsrd r11,1
#endif /* CONFIG_PPC_BOOK3E */
+system_call: /* label this so stack traces look sane */
/* We do need to set SOFTE in the stack frame or the return
* from interrupt will be painful
*/
@@ -155,7 +156,7 @@ END_FW_FTR_SECTION_IFSET(FW_FEATURE_SPLPAR)
cmpldi 0,r0,NR_syscalls
bge- .Lsyscall_enosys
-system_call: /* label this so stack traces look sane */
+.Lsyscall:
/*
* Need to vector to 32 Bit or default sys_call_table here,
* based on caller's run-mode / personality.
@@ -309,13 +310,13 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
ld r7,GPR7(r1)
ld r8,GPR8(r1)
- /* Repopulate r9 and r10 for the system_call path */
+ /* Repopulate r9 and r10 for the syscall path */
addi r9,r1,STACK_FRAME_OVERHEAD
CURRENT_THREAD_INFO(r10, r1)
ld r10,TI_FLAGS(r10)
cmpldi r0,NR_syscalls
- blt+ system_call
+ blt+ .Lsyscall
/* Return code is already in r3 thanks to do_syscall_trace_enter() */
b .Lsyscall_exit
--
2.13.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v4 5/7] powerpc/64s: Un-blacklist system_call() from kprobes
2017-06-29 10:41 [PATCH v4 0/7] powerpc: build out kprobes blacklist -- series 3 Naveen N. Rao
` (3 preceding siblings ...)
2017-06-29 10:41 ` [PATCH v4 4/7] powerpc/64s: Move system_call() symbol to just after setting MSR_EE Naveen N. Rao
@ 2017-06-29 10:41 ` Naveen N. Rao
2017-06-29 10:55 ` Nicholas Piggin
2017-06-29 10:41 ` [PATCH v4 6/7] powerpc/64s: Blacklist functions invoked on a trap Naveen N. Rao
2017-06-29 10:41 ` [PATCH v4 7/7] powerpc/64s: Blacklist rtas entry/exit from kprobes Naveen N. Rao
6 siblings, 1 reply; 17+ messages in thread
From: Naveen N. Rao @ 2017-06-29 10:41 UTC (permalink / raw)
To: Michael Ellerman
Cc: Nicholas Piggin, Ananth N Mavinakayanahalli, Masami Hiramatsu,
Anton Blanchard, 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 c39436706555..7a87427a67cd 100644
--- a/arch/powerpc/kernel/entry_64.S
+++ b/arch/powerpc/kernel/entry_64.S
@@ -205,6 +205,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)
@@ -413,7 +414,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] 17+ messages in thread
* [PATCH v4 6/7] powerpc/64s: Blacklist functions invoked on a trap
2017-06-29 10:41 [PATCH v4 0/7] powerpc: build out kprobes blacklist -- series 3 Naveen N. Rao
` (4 preceding siblings ...)
2017-06-29 10:41 ` [PATCH v4 5/7] powerpc/64s: Un-blacklist system_call() from kprobes Naveen N. Rao
@ 2017-06-29 10:41 ` Naveen N. Rao
2017-06-29 10:41 ` [PATCH v4 7/7] powerpc/64s: Blacklist rtas entry/exit from kprobes Naveen N. Rao
6 siblings, 0 replies; 17+ messages in thread
From: Naveen N. Rao @ 2017-06-29 10:41 UTC (permalink / raw)
To: Michael Ellerman
Cc: Nicholas Piggin, Ananth N Mavinakayanahalli, Masami Hiramatsu,
Anton Blanchard, linuxppc-dev
Blacklist all functions involved while handling a trap. We:
- convert some of the symbols into private symbols, 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 | 35 ++++++++++++++++++++++-------------
arch/powerpc/kernel/exceptions-64s.S | 2 ++
arch/powerpc/kernel/traps.c | 3 +++
3 files changed, 27 insertions(+), 13 deletions(-)
diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
index 7a87427a67cd..0c27084800b6 100644
--- a/arch/powerpc/kernel/entry_64.S
+++ b/arch/powerpc/kernel/entry_64.S
@@ -185,7 +185,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,
@@ -425,6 +425,7 @@ _GLOBAL(save_nvgprs)
clrrdi r0,r11,1
std r0,_TRAP(r1)
blr
+_ASM_NOKPROBE_SYMBOL(save_nvgprs);
/*
@@ -795,11 +796,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
@@ -808,14 +809,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);
@@ -823,7 +824,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
@@ -857,7 +858,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
@@ -915,7 +916,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
@@ -925,13 +926,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
@@ -941,7 +942,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
@@ -990,10 +991,18 @@ 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(restore);
+_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 3262e447b895..20318ceeea6a 100644
--- a/arch/powerpc/kernel/exceptions-64s.S
+++ b/arch/powerpc/kernel/exceptions-64s.S
@@ -1591,6 +1591,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
@@ -1652,3 +1653,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] 17+ messages in thread
* [PATCH v4 7/7] powerpc/64s: Blacklist rtas entry/exit from kprobes
2017-06-29 10:41 [PATCH v4 0/7] powerpc: build out kprobes blacklist -- series 3 Naveen N. Rao
` (5 preceding siblings ...)
2017-06-29 10:41 ` [PATCH v4 6/7] powerpc/64s: Blacklist functions invoked on a trap Naveen N. Rao
@ 2017-06-29 10:41 ` Naveen N. Rao
2017-06-29 11:01 ` Nicholas Piggin
6 siblings, 1 reply; 17+ messages in thread
From: Naveen N. Rao @ 2017-06-29 10:41 UTC (permalink / raw)
To: Michael Ellerman
Cc: Nicholas Piggin, Ananth N Mavinakayanahalli, Masami Hiramatsu,
Anton Blanchard, 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 | 3 +++
1 file changed, 3 insertions(+)
diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
index 0c27084800b6..16f4c4a1a294 100644
--- a/arch/powerpc/kernel/entry_64.S
+++ b/arch/powerpc/kernel/entry_64.S
@@ -1082,6 +1082,7 @@ _GLOBAL(enter_rtas)
sync /* disable interrupts so SRR0/1 */
mtmsrd r0 /* don't get trashed */
+__enter_rtas:
LOAD_REG_ADDR(r4, rtas)
ld r5,RTASENTRY(r4) /* get the rtas->entry value */
ld r4,RTASBASE(r4) /* get the rtas->base value */
@@ -1115,6 +1116,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] 17+ messages in thread
* Re: [PATCH v4 1/7] powerpc64/elfv1: Only dereference function descriptor for non-text symbols
2017-06-29 10:41 ` [PATCH v4 1/7] powerpc64/elfv1: Only dereference function descriptor for non-text symbols Naveen N. Rao
@ 2017-06-29 10:49 ` Nicholas Piggin
2017-06-29 11:48 ` Naveen N. Rao
0 siblings, 1 reply; 17+ messages in thread
From: Nicholas Piggin @ 2017-06-29 10:49 UTC (permalink / raw)
To: Naveen N. Rao
Cc: Michael Ellerman, Ananth N Mavinakayanahalli, Masami Hiramatsu,
Anton Blanchard, linuxppc-dev
On Thu, 29 Jun 2017 16:11:04 +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 function pointer we receive in
> ppc_function_entry() already points to kernel text. If so, we just
> return it as is. If not, we assume that this is a function descriptor
> and proceed to dereference it.
>
> Suggested-by: Nicholas Piggin <npiggin@gmail.com>
> 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..5482928eea1b 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 also receive pointer to an assembly symbol. To
> + * detect that, we first check if the function pointer we receive
> + * already points to kernel/module text and we only dereference it
> + * if it doesn't.
> */
> - return ((func_descr_t *)func)->entry;
> + if (kernel_text_address((unsigned long)func))
> + return (unsigned long)func;
> + else
> + return ((func_descr_t *)func)->entry;
This seems good to me now. I guess it should do the right thing with
modules too, looking at kernel_text_address implementation.
Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 5/7] powerpc/64s: Un-blacklist system_call() from kprobes
2017-06-29 10:41 ` [PATCH v4 5/7] powerpc/64s: Un-blacklist system_call() from kprobes Naveen N. Rao
@ 2017-06-29 10:55 ` Nicholas Piggin
2017-06-29 11:51 ` Naveen N. Rao
0 siblings, 1 reply; 17+ messages in thread
From: Nicholas Piggin @ 2017-06-29 10:55 UTC (permalink / raw)
To: Naveen N. Rao
Cc: Michael Ellerman, Ananth N Mavinakayanahalli, Masami Hiramatsu,
Anton Blanchard, linuxppc-dev
On Thu, 29 Jun 2017 16:11:08 +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 perhaps add a small comment to explain the label
(and why it's safe to have after the mtmsrd). It could be
a bit confusing to read if you don't have that detail of
the tracer in your mind.
Other than that
Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
>
> 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 c39436706555..7a87427a67cd 100644
> --- a/arch/powerpc/kernel/entry_64.S
> +++ b/arch/powerpc/kernel/entry_64.S
> @@ -205,6 +205,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)
> @@ -413,7 +414,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] 17+ messages in thread
* Re: [PATCH v4 7/7] powerpc/64s: Blacklist rtas entry/exit from kprobes
2017-06-29 10:41 ` [PATCH v4 7/7] powerpc/64s: Blacklist rtas entry/exit from kprobes Naveen N. Rao
@ 2017-06-29 11:01 ` Nicholas Piggin
2017-06-29 11:54 ` Naveen N. Rao
0 siblings, 1 reply; 17+ messages in thread
From: Nicholas Piggin @ 2017-06-29 11:01 UTC (permalink / raw)
To: Naveen N. Rao
Cc: Michael Ellerman, Ananth N Mavinakayanahalli, Masami Hiramatsu,
Anton Blanchard, linuxppc-dev
On Thu, 29 Jun 2017 16:11:10 +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 | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
> index 0c27084800b6..16f4c4a1a294 100644
> --- a/arch/powerpc/kernel/entry_64.S
> +++ b/arch/powerpc/kernel/entry_64.S
> @@ -1082,6 +1082,7 @@ _GLOBAL(enter_rtas)
> sync /* disable interrupts so SRR0/1 */
> mtmsrd r0 /* don't get trashed */
>
> +__enter_rtas:
Hmm, am I missing something, or is there a reason to put these labels
after the mtmsr? Even if kprobes does the right thing, I think it's
easier to read the code if you cover the mtmsr as well.
Thanks,
Nick
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 4/7] powerpc/64s: Move system_call() symbol to just after setting MSR_EE
2017-06-29 10:41 ` [PATCH v4 4/7] powerpc/64s: Move system_call() symbol to just after setting MSR_EE Naveen N. Rao
@ 2017-06-29 11:44 ` Nicholas Piggin
0 siblings, 0 replies; 17+ messages in thread
From: Nicholas Piggin @ 2017-06-29 11:44 UTC (permalink / raw)
To: Naveen N. Rao
Cc: Michael Ellerman, Ananth N Mavinakayanahalli, Masami Hiramatsu,
Anton Blanchard, linuxppc-dev
On Thu, 29 Jun 2017 16:11:07 +0530
"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> It is common to get a PMU interrupt right after the mtmsr instruction that
> enables interrupts. Due to this, the stack trace profile gets needlessly split
> across system_call_common() and system_call().
>
> Previously, system_call() symbol was at the current place to hide a few
> earlier symbols which have since been made private or removed entirely.
>
> So, let's move system_call() slightly higher up, right after the mtmsr
> instruction that enables interrupts. Convert existing references to
> system_call to a local syscall symbol.
>
> Suggested-by: Nicholas Piggin <npiggin@gmail.com>
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
Ah, I didn't realize the label is actually used. Still, I like the
patch myself.
Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 1/7] powerpc64/elfv1: Only dereference function descriptor for non-text symbols
2017-06-29 10:49 ` Nicholas Piggin
@ 2017-06-29 11:48 ` Naveen N. Rao
0 siblings, 0 replies; 17+ messages in thread
From: Naveen N. Rao @ 2017-06-29 11:48 UTC (permalink / raw)
To: Nicholas Piggin
Cc: Michael Ellerman, Ananth N Mavinakayanahalli, Masami Hiramatsu,
Anton Blanchard, linuxppc-dev
On 2017/06/29 08:49PM, Nicholas Piggin wrote:
> On Thu, 29 Jun 2017 16:11:04 +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 function pointer we receive in
> > ppc_function_entry() already points to kernel text. If so, we just
> > return it as is. If not, we assume that this is a function descriptor
> > and proceed to dereference it.
> >
> > Suggested-by: Nicholas Piggin <npiggin@gmail.com>
> > 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..5482928eea1b 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 also receive pointer to an assembly symbol. To
> > + * detect that, we first check if the function pointer we receive
> > + * already points to kernel/module text and we only dereference it
> > + * if it doesn't.
> > */
> > - return ((func_descr_t *)func)->entry;
> > + if (kernel_text_address((unsigned long)func))
> > + return (unsigned long)func;
> > + else
> > + return ((func_descr_t *)func)->entry;
>
> This seems good to me now. I guess it should do the right thing with
> modules too, looking at kernel_text_address implementation.
Yes, I've tested that by exercizing the jprobe sample module. We use
this to lookup the address of the jprobe handler which happens to be in
the jprobe module and it is working properly:
$ sudo modprobe jprobe_example
$ dmesg | grep Planted
[ 4817.649072] Planted jprobe at c0000000000ed7b0, handler addr d0000000074b03e8
$ sudo grep j_do_fork /proc/kallsyms
d0000000074b03e8 d j_do_fork [jprobe_example]
d0000000074b0000 t .j_do_fork [jprobe_example]
>
> Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
>
Thanks for the review,
- Naveen
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 5/7] powerpc/64s: Un-blacklist system_call() from kprobes
2017-06-29 10:55 ` Nicholas Piggin
@ 2017-06-29 11:51 ` Naveen N. Rao
0 siblings, 0 replies; 17+ messages in thread
From: Naveen N. Rao @ 2017-06-29 11:51 UTC (permalink / raw)
To: Nicholas Piggin
Cc: Michael Ellerman, Ananth N Mavinakayanahalli, Masami Hiramatsu,
Anton Blanchard, linuxppc-dev
On 2017/06/29 08:55PM, Nicholas Piggin wrote:
> On Thu, 29 Jun 2017 16:11:08 +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 perhaps add a small comment to explain the label
> (and why it's safe to have after the mtmsrd). It could be
> a bit confusing to read if you don't have that detail of
> the tracer in your mind.
Ah, I see. You did ask for this previously, but I thought you were just
asking for a comment that this is blacklisted, which can be found out by
looking for the _ASM_NOKPROBE_SYMBOL() further below.
I will add a comment.
Thanks,
Naveen
>
> Other than that
>
> Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
>
>
> >
> > 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 c39436706555..7a87427a67cd 100644
> > --- a/arch/powerpc/kernel/entry_64.S
> > +++ b/arch/powerpc/kernel/entry_64.S
> > @@ -205,6 +205,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)
> > @@ -413,7 +414,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] 17+ messages in thread
* Re: [PATCH v4 7/7] powerpc/64s: Blacklist rtas entry/exit from kprobes
2017-06-29 11:01 ` Nicholas Piggin
@ 2017-06-29 11:54 ` Naveen N. Rao
2017-06-29 12:13 ` Nicholas Piggin
0 siblings, 1 reply; 17+ messages in thread
From: Naveen N. Rao @ 2017-06-29 11:54 UTC (permalink / raw)
To: Nicholas Piggin
Cc: Michael Ellerman, Ananth N Mavinakayanahalli, Masami Hiramatsu,
Anton Blanchard, linuxppc-dev
On 2017/06/29 09:01PM, Nicholas Piggin wrote:
> On Thu, 29 Jun 2017 16:11:10 +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 | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
> > index 0c27084800b6..16f4c4a1a294 100644
> > --- a/arch/powerpc/kernel/entry_64.S
> > +++ b/arch/powerpc/kernel/entry_64.S
> > @@ -1082,6 +1082,7 @@ _GLOBAL(enter_rtas)
> > sync /* disable interrupts so SRR0/1 */
> > mtmsrd r0 /* don't get trashed */
> >
> > +__enter_rtas:
>
> Hmm, am I missing something, or is there a reason to put these labels
> after the mtmsr? Even if kprobes does the right thing, I think it's
> easier to read the code if you cover the mtmsr as well.
I thought you asked for this, per your previous review comment:
https://www.mail-archive.com/linuxppc-dev@lists.ozlabs.org/msg119667.html
Or, did I get that wrong?
Thanks,
Naveen
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 7/7] powerpc/64s: Blacklist rtas entry/exit from kprobes
2017-06-29 11:54 ` Naveen N. Rao
@ 2017-06-29 12:13 ` Nicholas Piggin
2017-06-29 16:51 ` Naveen N. Rao
0 siblings, 1 reply; 17+ messages in thread
From: Nicholas Piggin @ 2017-06-29 12:13 UTC (permalink / raw)
To: Naveen N. Rao
Cc: Michael Ellerman, Ananth N Mavinakayanahalli, Masami Hiramatsu,
Anton Blanchard, linuxppc-dev
On Thu, 29 Jun 2017 17:24:14 +0530
"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> On 2017/06/29 09:01PM, Nicholas Piggin wrote:
> > On Thu, 29 Jun 2017 16:11:10 +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 | 3 +++
> > > 1 file changed, 3 insertions(+)
> > >
> > > diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
> > > index 0c27084800b6..16f4c4a1a294 100644
> > > --- a/arch/powerpc/kernel/entry_64.S
> > > +++ b/arch/powerpc/kernel/entry_64.S
> > > @@ -1082,6 +1082,7 @@ _GLOBAL(enter_rtas)
> > > sync /* disable interrupts so SRR0/1 */
> > > mtmsrd r0 /* don't get trashed */
> > >
> > > +__enter_rtas:
> >
> > Hmm, am I missing something, or is there a reason to put these labels
> > after the mtmsr? Even if kprobes does the right thing, I think it's
> > easier to read the code if you cover the mtmsr as well.
>
> I thought you asked for this, per your previous review comment:
> https://www.mail-archive.com/linuxppc-dev@lists.ozlabs.org/msg119667.html
>
> Or, did I get that wrong?
No you're right, I'm contradicting myself. Let me start again.
I think we'd like to put the label before the mtmsrd if possible. So
in that case, should we adjust the system call code instead (then you
wouldn't have to add a comment for it).
And then you could move this label back above the mtmsrd. Sorry for
the confusion.
Thanks,
Nick
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 7/7] powerpc/64s: Blacklist rtas entry/exit from kprobes
2017-06-29 12:13 ` Nicholas Piggin
@ 2017-06-29 16:51 ` Naveen N. Rao
0 siblings, 0 replies; 17+ messages in thread
From: Naveen N. Rao @ 2017-06-29 16:51 UTC (permalink / raw)
To: Nicholas Piggin
Cc: Michael Ellerman, Ananth N Mavinakayanahalli, Masami Hiramatsu,
Anton Blanchard, linuxppc-dev
On 2017/06/29 10:13PM, Nicholas Piggin wrote:
> On Thu, 29 Jun 2017 17:24:14 +0530
> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
>
> > On 2017/06/29 09:01PM, Nicholas Piggin wrote:
> > > On Thu, 29 Jun 2017 16:11:10 +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 | 3 +++
> > > > 1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
> > > > index 0c27084800b6..16f4c4a1a294 100644
> > > > --- a/arch/powerpc/kernel/entry_64.S
> > > > +++ b/arch/powerpc/kernel/entry_64.S
> > > > @@ -1082,6 +1082,7 @@ _GLOBAL(enter_rtas)
> > > > sync /* disable interrupts so SRR0/1 */
> > > > mtmsrd r0 /* don't get trashed */
> > > >
> > > > +__enter_rtas:
> > >
> > > Hmm, am I missing something, or is there a reason to put these labels
> > > after the mtmsr? Even if kprobes does the right thing, I think it's
> > > easier to read the code if you cover the mtmsr as well.
> >
> > I thought you asked for this, per your previous review comment:
> > https://www.mail-archive.com/linuxppc-dev@lists.ozlabs.org/msg119667.html
> >
> > Or, did I get that wrong?
>
> No you're right, I'm contradicting myself. Let me start again.
>
> I think we'd like to put the label before the mtmsrd if possible. So
> in that case, should we adjust the system call code instead (then you
> wouldn't have to add a comment for it).
>
> And then you could move this label back above the mtmsrd. Sorry for
> the confusion.
Sure - I now get why you were insisting on a comment with the
system_call_exit symbol. v5 enroute...
Thanks,
Naveen
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2017-06-29 17:10 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-06-29 10:41 [PATCH v4 0/7] powerpc: build out kprobes blacklist -- series 3 Naveen N. Rao
2017-06-29 10:41 ` [PATCH v4 1/7] powerpc64/elfv1: Only dereference function descriptor for non-text symbols Naveen N. Rao
2017-06-29 10:49 ` Nicholas Piggin
2017-06-29 11:48 ` Naveen N. Rao
2017-06-29 10:41 ` [PATCH v4 2/7] powerpc/64s: Convert .L__replay_interrupt_return to a local label Naveen N. Rao
2017-06-29 10:41 ` [PATCH v4 3/7] powerpc/64s: Blacklist system_call() and system_call_common() from kprobes Naveen N. Rao
2017-06-29 10:41 ` [PATCH v4 4/7] powerpc/64s: Move system_call() symbol to just after setting MSR_EE Naveen N. Rao
2017-06-29 11:44 ` Nicholas Piggin
2017-06-29 10:41 ` [PATCH v4 5/7] powerpc/64s: Un-blacklist system_call() from kprobes Naveen N. Rao
2017-06-29 10:55 ` Nicholas Piggin
2017-06-29 11:51 ` Naveen N. Rao
2017-06-29 10:41 ` [PATCH v4 6/7] powerpc/64s: Blacklist functions invoked on a trap Naveen N. Rao
2017-06-29 10:41 ` [PATCH v4 7/7] powerpc/64s: Blacklist rtas entry/exit from kprobes Naveen N. Rao
2017-06-29 11:01 ` Nicholas Piggin
2017-06-29 11:54 ` Naveen N. Rao
2017-06-29 12:13 ` Nicholas Piggin
2017-06-29 16:51 ` 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.