Re: [PATCH] powerpc: kprobes: convert __kprobes to NOKPROBE_SYMBOL()

2017-03-08 Thread Naveen N. Rao 
On 2017/03/08 09:42AM, Masami Hiramatsu wrote:
> On Wed,  8 Mar 2017 02:09:29 +0530
> "Naveen N. Rao"  wrote:
> 
> > Along similar lines as commit 9326638cbee2 ("kprobes, x86: Use
> > NOKPROBE_SYMBOL() instead of __kprobes annotation"), convert __kprobes
> > annotation to either NOKPROBE_SYMBOL() or nokprobe_inline. The latter
> > forces inlining, in which case the caller needs to be added to
> > NOKPROBE_SYMBOL().
> 
> Acked-by: Masami Hiramatsu 
> 
> OK, this is a good starting point so far. please consider that those
> functions really have to be protected by kprobes. As you can see on x86,

Yes, I've only converted static functions to nokprobe_inline and 
confirmed that their callers are protected. I also verified that none of 
these show up in kallsyms.

> I allowed some functions to be kprobed by commit 7ec8a97a990d ("kprobes/x86:
> Allow probe on some kprobe preparation functions").
> Maybe on powerpc, those arch_* functions also be probed safely, since
> those are not kicked in the context of breakpoint interruption.

Nice, thanks for the pointer. I will consider that as part of the kprobe 
blacklisting updates I have for powerpc.

Regards,
Naveen

Re: [PATCH] powerpc: kprobes: convert __kprobes to NOKPROBE_SYMBOL()

2017-03-08 Thread Masami Hiramatsu 
On Wed,  8 Mar 2017 02:09:29 +0530
"Naveen N. Rao"  wrote:

> Along similar lines as commit 9326638cbee2 ("kprobes, x86: Use
> NOKPROBE_SYMBOL() instead of __kprobes annotation"), convert __kprobes
> annotation to either NOKPROBE_SYMBOL() or nokprobe_inline. The latter
> forces inlining, in which case the caller needs to be added to
> NOKPROBE_SYMBOL().

Acked-by: Masami Hiramatsu 

OK, this is a good starting point so far. please consider that those
functions really have to be protected by kprobes. As you can see on x86,
I allowed some functions to be kprobed by commit 7ec8a97a990d ("kprobes/x86:
Allow probe on some kprobe preparation functions").
Maybe on powerpc, those arch_* functions also be probed safely, since
those are not kicked in the context of breakpoint interruption.

Thank you,

> 
> Also:
> - blacklist kretprobe_trampoline
> - blacklist arch_deref_entry_point, and
> - convert a few regular inlines to nokprobe_inline in lib/sstep.c
> 
> A key benefit is the ability to detect such symbols as being
> blacklisted. Before this patch:
> 
>   naveen@ubuntu:~/linux/tools/perf$ sudo cat 
> /sys/kernel/debug/kprobes/blacklist | grep read_mem
>   naveen@ubuntu:~/linux/tools/perf$ sudo ./perf probe read_mem
>   Failed to write event: Invalid argument
> Error: Failed to add events.
>   naveen@ubuntu:~/linux/tools/perf$ dmesg | tail -1
>   [ 3736.112815] Could not insert probe at _text+10014968: -22
> 
> After patch:
>   naveen@ubuntu:~/linux/tools/perf$ sudo cat 
> /sys/kernel/debug/kprobes/blacklist | grep read_mem
>   0xc0072b50-0xc0072d20   read_mem
>   naveen@ubuntu:~/linux/tools/perf$ sudo ./perf probe read_mem
>   read_mem is blacklisted function, skip it.
>   Added new events:
> (null):(null)(on read_mem)
> probe:read_mem   (on read_mem)
> 
>   You can now use it in all perf tools, such as:
> 
> perf record -e probe:read_mem -aR sleep 1
> 
>   naveen@ubuntu:~/linux/tools/perf$ sudo grep " read_mem" /proc/kallsyms
>   c0072b50 t read_mem
>   c05f3b40 t read_mem
>   naveen@ubuntu:~/linux/tools/perf$ sudo cat /sys/kernel/debug/kprobes/list
>   c05f3b48  k  read_mem+0x8[DISABLED]
> 
> Signed-off-by: Naveen N. Rao 
> ---
>  arch/powerpc/kernel/kprobes.c| 56 +--
>  arch/powerpc/lib/code-patching.c |  4 +-
>  arch/powerpc/lib/sstep.c | 82 
> +---
>  3 files changed, 82 insertions(+), 60 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
> index fce05a38851c..6d2d464900c4 100644
> --- a/arch/powerpc/kernel/kprobes.c
> +++ b/arch/powerpc/kernel/kprobes.c
> @@ -42,7 +42,7 @@ DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk);
>  
>  struct kretprobe_blackpoint kretprobe_blacklist[] = {{NULL, NULL}};
>  
> -int __kprobes arch_prepare_kprobe(struct kprobe *p)
> +int arch_prepare_kprobe(struct kprobe *p)
>  {
>   int ret = 0;
>   kprobe_opcode_t insn = *p->addr;
> @@ -74,30 +74,34 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p)
>   p->ainsn.boostable = 0;
>   return ret;
>  }
> +NOKPROBE_SYMBOL(arch_prepare_kprobe);
>  
> -void __kprobes arch_arm_kprobe(struct kprobe *p)
> +void arch_arm_kprobe(struct kprobe *p)
>  {
>   *p->addr = BREAKPOINT_INSTRUCTION;
>   flush_icache_range((unsigned long) p->addr,
>  (unsigned long) p->addr + sizeof(kprobe_opcode_t));
>  }
> +NOKPROBE_SYMBOL(arch_arm_kprobe);
>  
> -void __kprobes arch_disarm_kprobe(struct kprobe *p)
> +void arch_disarm_kprobe(struct kprobe *p)
>  {
>   *p->addr = p->opcode;
>   flush_icache_range((unsigned long) p->addr,
>  (unsigned long) p->addr + sizeof(kprobe_opcode_t));
>  }
> +NOKPROBE_SYMBOL(arch_disarm_kprobe);
>  
> -void __kprobes arch_remove_kprobe(struct kprobe *p)
> +void arch_remove_kprobe(struct kprobe *p)
>  {
>   if (p->ainsn.insn) {
>   free_insn_slot(p->ainsn.insn, 0);
>   p->ainsn.insn = NULL;
>   }
>  }
> +NOKPROBE_SYMBOL(arch_remove_kprobe);
>  
> -static void __kprobes prepare_singlestep(struct kprobe *p, struct pt_regs 
> *regs)
> +static nokprobe_inline void prepare_singlestep(struct kprobe *p, struct 
> pt_regs *regs)
>  {
>   enable_single_step(regs);
>  
> @@ -110,37 +114,37 @@ static void __kprobes prepare_singlestep(struct kprobe 
> *p, struct pt_regs *regs)
>   regs->nip = (unsigned long)p->ainsn.insn;
>  }
>  
> -static void __kprobes save_previous_kprobe(struct kprobe_ctlblk *kcb)
> +static nokprobe_inline void save_previous_kprobe(struct kprobe_ctlblk *kcb)
>  {
>   kcb->prev_kprobe.kp = kprobe_running();
>   kcb->prev_kprobe.status = kcb->kprobe_status;
>   kcb->prev_kprobe.saved_msr = kcb->kprobe_saved_msr;
>  }
>  
> -static void __kprobes restore_previous_kprobe(struct kprobe_ctlblk *kcb)
> +static

[PATCH] powerpc: kprobes: convert __kprobes to NOKPROBE_SYMBOL()

2017-03-07 Thread Naveen N. Rao 
Along similar lines as commit 9326638cbee2 ("kprobes, x86: Use
NOKPROBE_SYMBOL() instead of __kprobes annotation"), convert __kprobes
annotation to either NOKPROBE_SYMBOL() or nokprobe_inline. The latter
forces inlining, in which case the caller needs to be added to
NOKPROBE_SYMBOL().

Also:
- blacklist kretprobe_trampoline
- blacklist arch_deref_entry_point, and
- convert a few regular inlines to nokprobe_inline in lib/sstep.c

A key benefit is the ability to detect such symbols as being
blacklisted. Before this patch:

  naveen@ubuntu:~/linux/tools/perf$ sudo cat 
/sys/kernel/debug/kprobes/blacklist | grep read_mem
  naveen@ubuntu:~/linux/tools/perf$ sudo ./perf probe read_mem
  Failed to write event: Invalid argument
Error: Failed to add events.
  naveen@ubuntu:~/linux/tools/perf$ dmesg | tail -1
  [ 3736.112815] Could not insert probe at _text+10014968: -22

After patch:
  naveen@ubuntu:~/linux/tools/perf$ sudo cat 
/sys/kernel/debug/kprobes/blacklist | grep read_mem
  0xc0072b50-0xc0072d20 read_mem
  naveen@ubuntu:~/linux/tools/perf$ sudo ./perf probe read_mem
  read_mem is blacklisted function, skip it.
  Added new events:
(null):(null)(on read_mem)
probe:read_mem   (on read_mem)

  You can now use it in all perf tools, such as:

  perf record -e probe:read_mem -aR sleep 1

  naveen@ubuntu:~/linux/tools/perf$ sudo grep " read_mem" /proc/kallsyms
  c0072b50 t read_mem
  c05f3b40 t read_mem
  naveen@ubuntu:~/linux/tools/perf$ sudo cat /sys/kernel/debug/kprobes/list
  c05f3b48  k  read_mem+0x8[DISABLED]

Signed-off-by: Naveen N. Rao 
---
 arch/powerpc/kernel/kprobes.c| 56 +--
 arch/powerpc/lib/code-patching.c |  4 +-
 arch/powerpc/lib/sstep.c | 82 +---
 3 files changed, 82 insertions(+), 60 deletions(-)

diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
index fce05a38851c..6d2d464900c4 100644
--- a/arch/powerpc/kernel/kprobes.c
+++ b/arch/powerpc/kernel/kprobes.c
@@ -42,7 +42,7 @@ DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk);
 
 struct kretprobe_blackpoint kretprobe_blacklist[] = {{NULL, NULL}};
 
-int __kprobes arch_prepare_kprobe(struct kprobe *p)
+int arch_prepare_kprobe(struct kprobe *p)
 {
int ret = 0;
kprobe_opcode_t insn = *p->addr;
@@ -74,30 +74,34 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p)
p->ainsn.boostable = 0;
return ret;
 }
+NOKPROBE_SYMBOL(arch_prepare_kprobe);
 
-void __kprobes arch_arm_kprobe(struct kprobe *p)
+void arch_arm_kprobe(struct kprobe *p)
 {
*p->addr = BREAKPOINT_INSTRUCTION;
flush_icache_range((unsigned long) p->addr,
   (unsigned long) p->addr + sizeof(kprobe_opcode_t));
 }
+NOKPROBE_SYMBOL(arch_arm_kprobe);
 
-void __kprobes arch_disarm_kprobe(struct kprobe *p)
+void arch_disarm_kprobe(struct kprobe *p)
 {
*p->addr = p->opcode;
flush_icache_range((unsigned long) p->addr,
   (unsigned long) p->addr + sizeof(kprobe_opcode_t));
 }
+NOKPROBE_SYMBOL(arch_disarm_kprobe);
 
-void __kprobes arch_remove_kprobe(struct kprobe *p)
+void arch_remove_kprobe(struct kprobe *p)
 {
if (p->ainsn.insn) {
free_insn_slot(p->ainsn.insn, 0);
p->ainsn.insn = NULL;
}
 }
+NOKPROBE_SYMBOL(arch_remove_kprobe);
 
-static void __kprobes prepare_singlestep(struct kprobe *p, struct pt_regs 
*regs)
+static nokprobe_inline void prepare_singlestep(struct kprobe *p, struct 
pt_regs *regs)
 {
enable_single_step(regs);
 
@@ -110,37 +114,37 @@ static void __kprobes prepare_singlestep(struct kprobe 
*p, struct pt_regs *regs)
regs->nip = (unsigned long)p->ainsn.insn;
 }
 
-static void __kprobes save_previous_kprobe(struct kprobe_ctlblk *kcb)
+static nokprobe_inline void save_previous_kprobe(struct kprobe_ctlblk *kcb)
 {
kcb->prev_kprobe.kp = kprobe_running();
kcb->prev_kprobe.status = kcb->kprobe_status;
kcb->prev_kprobe.saved_msr = kcb->kprobe_saved_msr;
 }
 
-static void __kprobes restore_previous_kprobe(struct kprobe_ctlblk *kcb)
+static nokprobe_inline void restore_previous_kprobe(struct kprobe_ctlblk *kcb)
 {
__this_cpu_write(current_kprobe, kcb->prev_kprobe.kp);
kcb->kprobe_status = kcb->prev_kprobe.status;
kcb->kprobe_saved_msr = kcb->prev_kprobe.saved_msr;
 }
 
-static void __kprobes set_current_kprobe(struct kprobe *p, struct pt_regs 
*regs,
+static nokprobe_inline void set_current_kprobe(struct kprobe *p, struct 
pt_regs *regs,
struct kprobe_ctlblk *kcb)
 {
__this_cpu_write(current_kprobe, p);
kcb->kprobe_saved_msr = regs->msr;
 }
 
-void __kprobes arch_prepare_kretprobe(struct kretprobe_instance *ri,
- struct pt_regs *regs)
+void arch_prepare_kretprobe(struct kretprobe_instance