[PATCH v3 0/6] powerpc: build out kprobes blacklist -- series 3

[PATCH v3 0/6] powerpc: build out kprobes blacklist -- series 3

[Naveen N. Rao]

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

[PATCH v3 1/6] powerpc64/elfv1: Validate function pointer address in the function descriptor

[Naveen N. Rao]

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

[PATCH v3 2/6] powerpc/64s: Convert .L__replay_interrupt_return to a local label

[Naveen N. Rao]

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

[PATCH v3 3/6] powerpc/64s: Blacklist system_call() and system_call_common() from kprobes

[Naveen N. Rao]

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

[PATCH v3 4/6] powerpc/64s: Un-blacklist system_call() from kprobes

[Naveen N. Rao]

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

[PATCH v3 5/6] powerpc/64s: Blacklist functions invoked on a trap

[Naveen N. Rao]

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

[PATCH v3 6/6] powerpc/64s: Blacklist rtas entry/exit from kprobes

[Naveen N. Rao]

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

Re: [PATCH v3 1/6] powerpc64/elfv1: Validate function pointer address in the function descriptor

[Nicholas Piggin]

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

Re: [PATCH v3 2/6] powerpc/64s: Convert .L__replay_interrupt_return to a local label

[Nicholas Piggin]

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
>  

Re: [PATCH v3 3/6] powerpc/64s: Blacklist system_call() and system_call_common() from kprobes

[Nicholas Piggin]

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

Re: [PATCH v3 4/6] powerpc/64s: Un-blacklist system_call() from kprobes

[Nicholas Piggin]

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)

Re: [PATCH v3 5/6] powerpc/64s: Blacklist functions invoked on a trap

[Nicholas Piggin]

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

Re: [PATCH v3 6/6] powerpc/64s: Blacklist rtas entry/exit from kprobes

[Nicholas Piggin]

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

Re: [PATCH v3 1/6] powerpc64/elfv1: Validate function pointer address in the function descriptor

[Michael Ellerman]

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

Re: [PATCH v3 3/6] powerpc/64s: Blacklist system_call() and system_call_common() from kprobes

[Michael Ellerman]

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

Re: [PATCH v3 5/6] powerpc/64s: Blacklist functions invoked on a trap

[Michael Ellerman]

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

Re: [PATCH v3 4/6] powerpc/64s: Un-blacklist system_call() from kprobes

[Michael Ellerman]

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

Re: [PATCH v3 1/6] powerpc64/elfv1: Validate function pointer address in the function descriptor

[Nicholas Piggin]

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?

Re: [PATCH v3 3/6] powerpc/64s: Blacklist system_call() and system_call_common() from kprobes

[Nicholas Piggin]

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

Re: [PATCH v3 4/6] powerpc/64s: Un-blacklist system_call() from kprobes

[Nicholas Piggin]

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

Re: [PATCH v3 1/6] powerpc64/elfv1: Validate function pointer address in the function descriptor

[Naveen N. Rao]

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

Re: [PATCH v3 3/6] powerpc/64s: Blacklist system_call() and system_call_common() from kprobes

[Naveen N. Rao]

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

Re: [PATCH v3 4/6] powerpc/64s: Un-blacklist system_call() from kprobes

[Naveen N. Rao]

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

Re: [PATCH v3 6/6] powerpc/64s: Blacklist rtas entry/exit from kprobes

[Naveen N. Rao]

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