[PATCH 1/3] powerpc: kprobes: fix handling of function offsets on ABIv2

[PATCH 1/3] powerpc: kprobes: fix handling of function offsets on ABIv2

[Naveen N. Rao]

commit 239aeba76409 ("perf powerpc: Fix kprobe and kretprobe handling
with kallsyms on ppc64le") changed how we use the offset field in struct
kprobe on ABIv2. perf now offsets from the GEP (Global entry point) if an
offset is specified and otherwise chooses the LEP (Local entry point).

Fix the same in kernel for kprobe API users. We do this by extending
kprobe_lookup_name() to accept an additional parameter to indicate the
offset specified with the kprobe registration. If offset is 0, we return
the local function entry and return the global entry point otherwise.

With:
	# cd /sys/kernel/debug/tracing/
	# echo "p _do_fork" >> kprobe_events
	# echo "p _do_fork+0x10" >> kprobe_events

before this patch:
	# cat ../kprobes/list
	c0000000000d0748  k  _do_fork+0x8    [DISABLED]
	c0000000000d0758  k  _do_fork+0x18    [DISABLED]
	c0000000000412b0  k  kretprobe_trampoline+0x0    [OPTIMIZED]

and after:
	# cat ../kprobes/list
	c0000000000d04c8  k  _do_fork+0x8    [DISABLED]
	c0000000000d04d0  k  _do_fork+0x10    [DISABLED]
	c0000000000412b0  k  kretprobe_trampoline+0x0    [OPTIMIZED]

Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 arch/powerpc/include/asm/kprobes.h | 6 +++---
 arch/powerpc/kernel/optprobes.c    | 4 ++--
 kernel/kprobes.c                   | 6 +++---
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/arch/powerpc/include/asm/kprobes.h b/arch/powerpc/include/asm/kprobes.h
index d821835ade86..e7ada061aa12 100644
--- a/arch/powerpc/include/asm/kprobes.h
+++ b/arch/powerpc/include/asm/kprobes.h
@@ -60,10 +60,10 @@ extern kprobe_opcode_t optprobe_template_end[];
 
 #ifdef PPC64_ELF_ABI_v2
 /* PPC64 ABIv2 needs local entry point */
-#define kprobe_lookup_name(name, addr)					\
+#define kprobe_lookup_name(name, addr, offset)				\
 {									\
 	addr = (kprobe_opcode_t *)kallsyms_lookup_name(name);		\
-	if (addr)							\
+	if (addr && !(offset))						\
 		addr = (kprobe_opcode_t *)ppc_function_entry(addr);	\
 }
 #elif defined(PPC64_ELF_ABI_v1)
@@ -75,7 +75,7 @@ extern kprobe_opcode_t optprobe_template_end[];
  * This ensures we always get to the actual symbol and not the descriptor.
  * Also handle <module:symbol> format.
  */
-#define kprobe_lookup_name(name, addr)					\
+#define kprobe_lookup_name(name, addr, offset)				\
 {									\
 	char dot_name[MODULE_NAME_LEN + 1 + KSYM_NAME_LEN];		\
 	const char *modsym;							\
diff --git a/arch/powerpc/kernel/optprobes.c b/arch/powerpc/kernel/optprobes.c
index 2282bf4e63cd..e51a045f3d3b 100644
--- a/arch/powerpc/kernel/optprobes.c
+++ b/arch/powerpc/kernel/optprobes.c
@@ -243,8 +243,8 @@ int arch_prepare_optimized_kprobe(struct optimized_kprobe *op, struct kprobe *p)
 	/*
 	 * 2. branch to optimized_callback() and emulate_step()
 	 */
-	kprobe_lookup_name("optimized_callback", op_callback_addr);
-	kprobe_lookup_name("emulate_step", emulate_step_addr);
+	kprobe_lookup_name("optimized_callback", op_callback_addr, 0);
+	kprobe_lookup_name("emulate_step", emulate_step_addr, 0);
 	if (!op_callback_addr || !emulate_step_addr) {
 		WARN(1, "kprobe_lookup_name() failed\n");
 		goto error;
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 83ad7e440417..9bc433575d98 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -63,7 +63,7 @@
  * so this must be overridable.
  */
 #ifndef kprobe_lookup_name
-#define kprobe_lookup_name(name, addr) \
+#define kprobe_lookup_name(name, addr, offset) \
 	addr = ((kprobe_opcode_t *)(kallsyms_lookup_name(name)))
 #endif
 
@@ -1365,7 +1365,7 @@ static kprobe_opcode_t *kprobe_addr(struct kprobe *p)
 		goto invalid;
 
 	if (p->symbol_name) {
-		kprobe_lookup_name(p->symbol_name, addr);
+		kprobe_lookup_name(p->symbol_name, addr, p->offset);
 		if (!addr)
 			return ERR_PTR(-ENOENT);
 	}
@@ -2161,7 +2161,7 @@ static int __init init_kprobes(void)
 		/* lookup the function address from its name */
 		for (i = 0; kretprobe_blacklist[i].name != NULL; i++) {
 			kprobe_lookup_name(kretprobe_blacklist[i].name,
-					   kretprobe_blacklist[i].addr);
+					   kretprobe_blacklist[i].addr, 0);
 			if (!kretprobe_blacklist[i].addr)
 				printk("kretprobe: lookup failed: %s\n",
 				       kretprobe_blacklist[i].name);
-- 
2.11.0

[PATCH 2/3] powerpc: kprobes: factor out code to emulate instruction into a helper

[Naveen N. Rao]

This helper will be used in a subsequent patch to emulate instructions
on re-entering the kprobe handler. No functional change.

Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 arch/powerpc/kernel/kprobes.c | 52 ++++++++++++++++++++++++++-----------------
 1 file changed, 31 insertions(+), 21 deletions(-)

diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
index fce05a38851c..9cdf2de31e9e 100644
--- a/arch/powerpc/kernel/kprobes.c
+++ b/arch/powerpc/kernel/kprobes.c
@@ -140,6 +140,35 @@ void __kprobes arch_prepare_kretprobe(struct kretprobe_instance *ri,
 	regs->link = (unsigned long)kretprobe_trampoline;
 }
 
+int __kprobes try_to_emulate(struct kprobe *p, struct pt_regs *regs)
+{
+	int ret;
+	unsigned int insn = *p->ainsn.insn;
+
+	/* regs->nip is also adjusted if emulate_step returns 1 */
+	ret = emulate_step(regs, insn);
+	if (ret > 0) {
+		/*
+		 * Once this instruction has been boosted
+		 * successfully, set the boostable flag
+		 */
+		if (unlikely(p->ainsn.boostable == 0))
+			p->ainsn.boostable = 1;
+	} else if (ret < 0) {
+		/*
+		 * We don't allow kprobes on mtmsr(d)/rfi(d), etc.
+		 * So, we should never get here... but, its still
+		 * good to catch them, just in case...
+		 */
+		printk("Can't step on instruction %x\n", insn);
+		BUG();
+	} else if (ret == 0)
+		/* This instruction can't be boosted */
+		p->ainsn.boostable = -1;
+
+	return ret;
+}
+
 int __kprobes kprobe_handler(struct pt_regs *regs)
 {
 	struct kprobe *p;
@@ -235,18 +264,9 @@ int __kprobes kprobe_handler(struct pt_regs *regs)
 
 ss_probe:
 	if (p->ainsn.boostable >= 0) {
-		unsigned int insn = *p->ainsn.insn;
+		ret = try_to_emulate(p, regs);
 
-		/* regs->nip is also adjusted if emulate_step returns 1 */
-		ret = emulate_step(regs, insn);
 		if (ret > 0) {
-			/*
-			 * Once this instruction has been boosted
-			 * successfully, set the boostable flag
-			 */
-			if (unlikely(p->ainsn.boostable == 0))
-				p->ainsn.boostable = 1;
-
 			if (p->post_handler)
 				p->post_handler(p, regs, 0);
 
@@ -254,17 +274,7 @@ int __kprobes kprobe_handler(struct pt_regs *regs)
 			reset_current_kprobe();
 			preempt_enable_no_resched();
 			return 1;
-		} else if (ret < 0) {
-			/*
-			 * We don't allow kprobes on mtmsr(d)/rfi(d), etc.
-			 * So, we should never get here... but, its still
-			 * good to catch them, just in case...
-			 */
-			printk("Can't step on instruction %x\n", insn);
-			BUG();
-		} else if (ret == 0)
-			/* This instruction can't be boosted */
-			p->ainsn.boostable = -1;
+		}
 	}
 	prepare_singlestep(p, regs);
 	kcb->kprobe_status = KPROBE_HIT_SS;
-- 
2.11.0

[PATCH 3/3] powerpc: kprobes: emulate instructions on kprobe handler re-entry

[Naveen N. Rao]

On kprobe handler re-entry, try to emulate the instruction rather than
single stepping always.

As a related change, remove the duplicate saving of msr as that is
already done in set_current_kprobe()

Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 arch/powerpc/kernel/kprobes.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
index 9cdf2de31e9e..c213637b9d25 100644
--- a/arch/powerpc/kernel/kprobes.c
+++ b/arch/powerpc/kernel/kprobes.c
@@ -206,10 +206,17 @@ int __kprobes kprobe_handler(struct pt_regs *regs)
 			 */
 			save_previous_kprobe(kcb);
 			set_current_kprobe(p, regs, kcb);
-			kcb->kprobe_saved_msr = regs->msr;
 			kprobes_inc_nmissed_count(p);
 			prepare_singlestep(p, regs);
 			kcb->kprobe_status = KPROBE_REENTER;
+			if (p->ainsn.boostable >= 0) {
+				ret = try_to_emulate(p, regs);
+
+				if (ret > 0) {
+					restore_previous_kprobe(kcb);
+					return 1;
+				}
+			}
 			return 1;
 		} else {
 			if (*addr != BREAKPOINT_INSTRUCTION) {
-- 
2.11.0

Re: [PATCH 1/3] powerpc: kprobes: fix handling of function offsets on ABIv2

[Ananth N Mavinakayanahalli]

On Tue, Feb 14, 2017 at 02:08:01PM +0530, Naveen N. Rao wrote:
> commit 239aeba76409 ("perf powerpc: Fix kprobe and kretprobe handling
> with kallsyms on ppc64le") changed how we use the offset field in struct
> kprobe on ABIv2. perf now offsets from the GEP (Global entry point) if an
> offset is specified and otherwise chooses the LEP (Local entry point).
> 
> Fix the same in kernel for kprobe API users. We do this by extending
> kprobe_lookup_name() to accept an additional parameter to indicate the
> offset specified with the kprobe registration. If offset is 0, we return
> the local function entry and return the global entry point otherwise.
> 
> With:
> 	# cd /sys/kernel/debug/tracing/
> 	# echo "p _do_fork" >> kprobe_events
> 	# echo "p _do_fork+0x10" >> kprobe_events
> 
> before this patch:
> 	# cat ../kprobes/list
> 	c0000000000d0748  k  _do_fork+0x8    [DISABLED]
> 	c0000000000d0758  k  _do_fork+0x18    [DISABLED]
> 	c0000000000412b0  k  kretprobe_trampoline+0x0    [OPTIMIZED]
> 
> and after:
> 	# cat ../kprobes/list
> 	c0000000000d04c8  k  _do_fork+0x8    [DISABLED]
> 	c0000000000d04d0  k  _do_fork+0x10    [DISABLED]
> 	c0000000000412b0  k  kretprobe_trampoline+0x0    [OPTIMIZED]
> 
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>

Acked-by: Ananth N Mavinakayanahalli <ananth@linux.vnet.ibm.com>

Re: [PATCH 2/3] powerpc: kprobes: factor out code to emulate instruction into a helper

[Ananth N Mavinakayanahalli]

On Tue, Feb 14, 2017 at 02:08:02PM +0530, Naveen N. Rao wrote:
> This helper will be used in a subsequent patch to emulate instructions
> on re-entering the kprobe handler. No functional change.
> 
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>

Acked-by: Ananth N Mavinakayanahalli <ananth@linux.vnet.ibm.com>

Re: [PATCH 3/3] powerpc: kprobes: emulate instructions on kprobe handler re-entry

[Ananth N Mavinakayanahalli]

On Tue, Feb 14, 2017 at 02:08:03PM +0530, Naveen N. Rao wrote:
> On kprobe handler re-entry, try to emulate the instruction rather than
> single stepping always.
> 
> As a related change, remove the duplicate saving of msr as that is
> already done in set_current_kprobe()
> 
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>

Acked-by: Ananth N Mavinakayanahalli <ananth@linux.vnet.ibm.com>

Re: [PATCH 1/3] powerpc: kprobes: fix handling of function offsets on ABIv2

[Masami Hiramatsu]

On Tue, 14 Feb 2017 14:08:01 +0530
"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:

> commit 239aeba76409 ("perf powerpc: Fix kprobe and kretprobe handling
> with kallsyms on ppc64le") changed how we use the offset field in struct
> kprobe on ABIv2. perf now offsets from the GEP (Global entry point) if an
> offset is specified and otherwise chooses the LEP (Local entry point).
> 
> Fix the same in kernel for kprobe API users. We do this by extending
> kprobe_lookup_name() to accept an additional parameter to indicate the
> offset specified with the kprobe registration. If offset is 0, we return
> the local function entry and return the global entry point otherwise.
> 
> With:
> 	# cd /sys/kernel/debug/tracing/
> 	# echo "p _do_fork" >> kprobe_events
> 	# echo "p _do_fork+0x10" >> kprobe_events
> 
> before this patch:
> 	# cat ../kprobes/list
> 	c0000000000d0748  k  _do_fork+0x8    [DISABLED]
> 	c0000000000d0758  k  _do_fork+0x18    [DISABLED]
> 	c0000000000412b0  k  kretprobe_trampoline+0x0    [OPTIMIZED]
> 
> and after:
> 	# cat ../kprobes/list
> 	c0000000000d04c8  k  _do_fork+0x8    [DISABLED]
> 	c0000000000d04d0  k  _do_fork+0x10    [DISABLED]
> 	c0000000000412b0  k  kretprobe_trampoline+0x0    [OPTIMIZED]
> 
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
>  arch/powerpc/include/asm/kprobes.h | 6 +++---
>  arch/powerpc/kernel/optprobes.c    | 4 ++--
>  kernel/kprobes.c                   | 6 +++---
>  3 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/kprobes.h b/arch/powerpc/include/asm/kprobes.h
> index d821835ade86..e7ada061aa12 100644
> --- a/arch/powerpc/include/asm/kprobes.h
> +++ b/arch/powerpc/include/asm/kprobes.h
> @@ -60,10 +60,10 @@ extern kprobe_opcode_t optprobe_template_end[];
>  
>  #ifdef PPC64_ELF_ABI_v2
>  /* PPC64 ABIv2 needs local entry point */
> -#define kprobe_lookup_name(name, addr)					\
> +#define kprobe_lookup_name(name, addr, offset)				\
>  {									\
>  	addr = (kprobe_opcode_t *)kallsyms_lookup_name(name);		\
> -	if (addr)							\
> +	if (addr && !(offset))						\
>  		addr = (kprobe_opcode_t *)ppc_function_entry(addr);	\
>  }
>  #elif defined(PPC64_ELF_ABI_v1)
> @@ -75,7 +75,7 @@ extern kprobe_opcode_t optprobe_template_end[];
>   * This ensures we always get to the actual symbol and not the descriptor.
>   * Also handle <module:symbol> format.
>   */
> -#define kprobe_lookup_name(name, addr)					\
> +#define kprobe_lookup_name(name, addr, offset)				\
>  {									\
>  	char dot_name[MODULE_NAME_LEN + 1 + KSYM_NAME_LEN];		\
>  	const char *modsym;							\
> diff --git a/arch/powerpc/kernel/optprobes.c b/arch/powerpc/kernel/optprobes.c
> index 2282bf4e63cd..e51a045f3d3b 100644
> --- a/arch/powerpc/kernel/optprobes.c
> +++ b/arch/powerpc/kernel/optprobes.c
> @@ -243,8 +243,8 @@ int arch_prepare_optimized_kprobe(struct optimized_kprobe *op, struct kprobe *p)
>  	/*
>  	 * 2. branch to optimized_callback() and emulate_step()
>  	 */
> -	kprobe_lookup_name("optimized_callback", op_callback_addr);
> -	kprobe_lookup_name("emulate_step", emulate_step_addr);
> +	kprobe_lookup_name("optimized_callback", op_callback_addr, 0);
> +	kprobe_lookup_name("emulate_step", emulate_step_addr, 0);
>  	if (!op_callback_addr || !emulate_step_addr) {
>  		WARN(1, "kprobe_lookup_name() failed\n");
>  		goto error;
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index 83ad7e440417..9bc433575d98 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -63,7 +63,7 @@
>   * so this must be overridable.
>   */
>  #ifndef kprobe_lookup_name
> -#define kprobe_lookup_name(name, addr) \
> +#define kprobe_lookup_name(name, addr, offset) \
>  	addr = ((kprobe_opcode_t *)(kallsyms_lookup_name(name)))
>  #endif

Hmm, it smells no good coding... I would like to use __weak function
instead of this "#ifndef" trick.

Thank you,

>  
> @@ -1365,7 +1365,7 @@ static kprobe_opcode_t *kprobe_addr(struct kprobe *p)
>  		goto invalid;
>  
>  	if (p->symbol_name) {
> -		kprobe_lookup_name(p->symbol_name, addr);
> +		kprobe_lookup_name(p->symbol_name, addr, p->offset);
>  		if (!addr)
>  			return ERR_PTR(-ENOENT);
>  	}
> @@ -2161,7 +2161,7 @@ static int __init init_kprobes(void)
>  		/* lookup the function address from its name */
>  		for (i = 0; kretprobe_blacklist[i].name != NULL; i++) {
>  			kprobe_lookup_name(kretprobe_blacklist[i].name,
> -					   kretprobe_blacklist[i].addr);
> +					   kretprobe_blacklist[i].addr, 0);
>  			if (!kretprobe_blacklist[i].addr)
>  				printk("kretprobe: lookup failed: %s\n",
>  				       kretprobe_blacklist[i].name);
> -- 
> 2.11.0
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH 1/3] powerpc: kprobes: fix handling of function offsets on ABIv2

[Naveen N. Rao]

On 2017/02/16 12:17AM, Masami Hiramatsu wrote:
> On Tue, 14 Feb 2017 14:08:01 +0530
> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> 
> > commit 239aeba76409 ("perf powerpc: Fix kprobe and kretprobe handling
> > with kallsyms on ppc64le") changed how we use the offset field in struct
> > kprobe on ABIv2. perf now offsets from the GEP (Global entry point) if an
> > offset is specified and otherwise chooses the LEP (Local entry point).
> > 
> > Fix the same in kernel for kprobe API users. We do this by extending
> > kprobe_lookup_name() to accept an additional parameter to indicate the
> > offset specified with the kprobe registration. If offset is 0, we return
> > the local function entry and return the global entry point otherwise.
> > 

<snip>

> > diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> > index 83ad7e440417..9bc433575d98 100644
> > --- a/kernel/kprobes.c
> > +++ b/kernel/kprobes.c
> > @@ -63,7 +63,7 @@
> >   * so this must be overridable.
> >   */
> >  #ifndef kprobe_lookup_name
> > -#define kprobe_lookup_name(name, addr) \
> > +#define kprobe_lookup_name(name, addr, offset) \
> >  	addr = ((kprobe_opcode_t *)(kallsyms_lookup_name(name)))
> >  #endif
> 
> Hmm, it smells no good coding... I would like to use __weak function
> instead of this "#ifndef" trick.

Can't say I wasn't tempted. KPROBES_ON_FTRACE makes this worse. I will 
clean this up.

Thanks!
- Naveen