Re: [RFC PATCH -tip v5 18/27] powerpc/kprobes: Don't call the ->break_handler() in arm kprobes code
On Thu, 07 Jun 2018 22:07:26 +0530 "Naveen N. Rao" wrote: > Masami Hiramatsu wrote: > > On Thu, 07 Jun 2018 17:07:00 +0530 > > "Naveen N. Rao" wrote: > > > >> Masami Hiramatsu wrote: > >> > Don't call the ->break_handler() from the arm kprobes code, > >>^^^ powerpc > >> > >> > because it was only used by jprobes which got removed. > >> > > >> > This also makes skip_singlestep() a static function since > >> > only ftrace-kprobe.c is using this function. > >> > > >> > Signed-off-by: Masami Hiramatsu > >> > Cc: Benjamin Herrenschmidt > >> > Cc: Paul Mackerras > >> > Cc: Michael Ellerman > >> > Cc: "Naveen N. Rao" > >> > Cc: linuxppc-dev@lists.ozlabs.org > >> > --- > >> > arch/powerpc/include/asm/kprobes.h | 10 -- > >> > arch/powerpc/kernel/kprobes-ftrace.c | 16 +++- > >> > arch/powerpc/kernel/kprobes.c| 31 > >> > +++ > >> > 3 files changed, 14 insertions(+), 43 deletions(-) > >> > >> With 2 small comments... > > > > 2 ? or 1 ? > > Two, with one in the commit log above :) Oops, sorry I missed it. yeah, the comment above is my mistake. I'll fix it. Thanks! -- Masami Hiramatsu
Re: [RFC PATCH -tip v5 18/27] powerpc/kprobes: Don't call the ->break_handler() in arm kprobes code
Masami Hiramatsu wrote: On Thu, 07 Jun 2018 17:07:00 +0530 "Naveen N. Rao" wrote: Masami Hiramatsu wrote: > Don't call the ->break_handler() from the arm kprobes code, ^^^ powerpc > because it was only used by jprobes which got removed. > > This also makes skip_singlestep() a static function since > only ftrace-kprobe.c is using this function. > > Signed-off-by: Masami Hiramatsu > Cc: Benjamin Herrenschmidt > Cc: Paul Mackerras > Cc: Michael Ellerman > Cc: "Naveen N. Rao" > Cc: linuxppc-dev@lists.ozlabs.org > --- > arch/powerpc/include/asm/kprobes.h | 10 -- > arch/powerpc/kernel/kprobes-ftrace.c | 16 +++- > arch/powerpc/kernel/kprobes.c| 31 +++ > 3 files changed, 14 insertions(+), 43 deletions(-) With 2 small comments... 2 ? or 1 ? Two, with one in the commit log above :) - Naveen
Re: [RFC PATCH -tip v5 18/27] powerpc/kprobes: Don't call the ->break_handler() in arm kprobes code
On Thu, 07 Jun 2018 17:07:00 +0530
"Naveen N. Rao" wrote:
> Masami Hiramatsu wrote:
> > Don't call the ->break_handler() from the arm kprobes code,
> ^^^ powerpc
>
> > because it was only used by jprobes which got removed.
> >
> > This also makes skip_singlestep() a static function since
> > only ftrace-kprobe.c is using this function.
> >
> > Signed-off-by: Masami Hiramatsu
> > Cc: Benjamin Herrenschmidt
> > Cc: Paul Mackerras
> > Cc: Michael Ellerman
> > Cc: "Naveen N. Rao"
> > Cc: linuxppc-dev@lists.ozlabs.org
> > ---
> > arch/powerpc/include/asm/kprobes.h | 10 --
> > arch/powerpc/kernel/kprobes-ftrace.c | 16 +++-
> > arch/powerpc/kernel/kprobes.c| 31 +++
> > 3 files changed, 14 insertions(+), 43 deletions(-)
>
> With 2 small comments...
2 ? or 1 ?
> Acked-by: Naveen N. Rao
>
> - Naveen
>
> >
> > diff --git a/arch/powerpc/include/asm/kprobes.h
> > b/arch/powerpc/include/asm/kprobes.h
> > index 674036db558b..785c464b6588 100644
> > --- a/arch/powerpc/include/asm/kprobes.h
> > +++ b/arch/powerpc/include/asm/kprobes.h
> > @@ -102,16 +102,6 @@ extern int kprobe_exceptions_notify(struct
> > notifier_block *self,
> > extern int kprobe_fault_handler(struct pt_regs *regs, int trapnr);
> > extern int kprobe_handler(struct pt_regs *regs);
> > extern int kprobe_post_handler(struct pt_regs *regs);
> > -#ifdef CONFIG_KPROBES_ON_FTRACE
> > -extern int skip_singlestep(struct kprobe *p, struct pt_regs *regs,
> > - struct kprobe_ctlblk *kcb);
> > -#else
> > -static inline int skip_singlestep(struct kprobe *p, struct pt_regs *regs,
> > - struct kprobe_ctlblk *kcb)
> > -{
> > - return 0;
> > -}
> > -#endif
> > #else
> > static inline int kprobe_handler(struct pt_regs *regs) { return 0; }
> > static inline int kprobe_post_handler(struct pt_regs *regs) { return 0; }
> > diff --git a/arch/powerpc/kernel/kprobes-ftrace.c
> > b/arch/powerpc/kernel/kprobes-ftrace.c
> > index 1b316331c2d9..3869b0e5d5c7 100644
> > --- a/arch/powerpc/kernel/kprobes-ftrace.c
> > +++ b/arch/powerpc/kernel/kprobes-ftrace.c
> > @@ -26,8 +26,8 @@
> > #include
> >
> > static nokprobe_inline
> > -int __skip_singlestep(struct kprobe *p, struct pt_regs *regs,
> > - struct kprobe_ctlblk *kcb, unsigned long orig_nip)
> > +int skip_singlestep(struct kprobe *p, struct pt_regs *regs,
> > + struct kprobe_ctlblk *kcb, unsigned long orig_nip)
> > {
> > /*
> > * Emulate singlestep (and also recover regs->nip)
> > @@ -44,16 +44,6 @@ int __skip_singlestep(struct kprobe *p, struct pt_regs
> > *regs,
> > return 1;
> > }
> >
> > -int skip_singlestep(struct kprobe *p, struct pt_regs *regs,
> > - struct kprobe_ctlblk *kcb)
> > -{
> > - if (kprobe_ftrace(p))
> > - return __skip_singlestep(p, regs, kcb, 0);
> > - else
> > - return 0;
> > -}
> > -NOKPROBE_SYMBOL(skip_singlestep);
> > -
> > /* Ftrace callback handler for kprobes */
> > void kprobe_ftrace_handler(unsigned long nip, unsigned long parent_nip,
> >struct ftrace_ops *ops, struct pt_regs *regs)
> > @@ -82,7 +72,7 @@ void kprobe_ftrace_handler(unsigned long nip, unsigned
> > long parent_nip,
> > __this_cpu_write(current_kprobe, p);
> > kcb->kprobe_status = KPROBE_HIT_ACTIVE;
> > if (!p->pre_handler || !p->pre_handler(p, regs))
> > - __skip_singlestep(p, regs, kcb, orig_nip);
> > + skip_singlestep(p, regs, kcb, orig_nip);
>
> We can probably get rid of skip_singlestep() completely along with
> orig_nip since instructions are always 4 bytes on powerpc. So, the
> changes we do to nip should help to recover the value automatically.
Good point! Yes, skip_singlestep() is no more exported, so we just consolidate
it into kprobe_ftrace_handler() for simplifying operation.
Thank you!
>
> - Naveen
>
>
--
Masami Hiramatsu
Re: [RFC PATCH -tip v5 18/27] powerpc/kprobes: Don't call the ->break_handler() in arm kprobes code
Masami Hiramatsu wrote:
Don't call the ->break_handler() from the arm kprobes code,
^^^ powerpc
because it was only used by jprobes which got removed.
This also makes skip_singlestep() a static function since
only ftrace-kprobe.c is using this function.
Signed-off-by: Masami Hiramatsu
Cc: Benjamin Herrenschmidt
Cc: Paul Mackerras
Cc: Michael Ellerman
Cc: "Naveen N. Rao"
Cc: linuxppc-dev@lists.ozlabs.org
---
arch/powerpc/include/asm/kprobes.h | 10 --
arch/powerpc/kernel/kprobes-ftrace.c | 16 +++-
arch/powerpc/kernel/kprobes.c| 31 +++
3 files changed, 14 insertions(+), 43 deletions(-)
With 2 small comments...
Acked-by: Naveen N. Rao
- Naveen
diff --git a/arch/powerpc/include/asm/kprobes.h
b/arch/powerpc/include/asm/kprobes.h
index 674036db558b..785c464b6588 100644
--- a/arch/powerpc/include/asm/kprobes.h
+++ b/arch/powerpc/include/asm/kprobes.h
@@ -102,16 +102,6 @@ extern int kprobe_exceptions_notify(struct notifier_block
*self,
extern int kprobe_fault_handler(struct pt_regs *regs, int trapnr);
extern int kprobe_handler(struct pt_regs *regs);
extern int kprobe_post_handler(struct pt_regs *regs);
-#ifdef CONFIG_KPROBES_ON_FTRACE
-extern int skip_singlestep(struct kprobe *p, struct pt_regs *regs,
- struct kprobe_ctlblk *kcb);
-#else
-static inline int skip_singlestep(struct kprobe *p, struct pt_regs *regs,
- struct kprobe_ctlblk *kcb)
-{
- return 0;
-}
-#endif
#else
static inline int kprobe_handler(struct pt_regs *regs) { return 0; }
static inline int kprobe_post_handler(struct pt_regs *regs) { return 0; }
diff --git a/arch/powerpc/kernel/kprobes-ftrace.c
b/arch/powerpc/kernel/kprobes-ftrace.c
index 1b316331c2d9..3869b0e5d5c7 100644
--- a/arch/powerpc/kernel/kprobes-ftrace.c
+++ b/arch/powerpc/kernel/kprobes-ftrace.c
@@ -26,8 +26,8 @@
#include
static nokprobe_inline
-int __skip_singlestep(struct kprobe *p, struct pt_regs *regs,
- struct kprobe_ctlblk *kcb, unsigned long orig_nip)
+int skip_singlestep(struct kprobe *p, struct pt_regs *regs,
+ struct kprobe_ctlblk *kcb, unsigned long orig_nip)
{
/*
* Emulate singlestep (and also recover regs->nip)
@@ -44,16 +44,6 @@ int __skip_singlestep(struct kprobe *p, struct pt_regs *regs,
return 1;
}
-int skip_singlestep(struct kprobe *p, struct pt_regs *regs,
- struct kprobe_ctlblk *kcb)
-{
- if (kprobe_ftrace(p))
- return __skip_singlestep(p, regs, kcb, 0);
- else
- return 0;
-}
-NOKPROBE_SYMBOL(skip_singlestep);
-
/* Ftrace callback handler for kprobes */
void kprobe_ftrace_handler(unsigned long nip, unsigned long parent_nip,
struct ftrace_ops *ops, struct pt_regs *regs)
@@ -82,7 +72,7 @@ void kprobe_ftrace_handler(unsigned long nip, unsigned long
parent_nip,
__this_cpu_write(current_kprobe, p);
kcb->kprobe_status = KPROBE_HIT_ACTIVE;
if (!p->pre_handler || !p->pre_handler(p, regs))
- __skip_singlestep(p, regs, kcb, orig_nip);
+ skip_singlestep(p, regs, kcb, orig_nip);
We can probably get rid of skip_singlestep() completely along with
orig_nip since instructions are always 4 bytes on powerpc. So, the
changes we do to nip should help to recover the value automatically.
- Naveen
[RFC PATCH -tip v5 18/27] powerpc/kprobes: Don't call the ->break_handler() in arm kprobes code
Don't call the ->break_handler() from the arm kprobes code,
because it was only used by jprobes which got removed.
This also makes skip_singlestep() a static function since
only ftrace-kprobe.c is using this function.
Signed-off-by: Masami Hiramatsu
Cc: Benjamin Herrenschmidt
Cc: Paul Mackerras
Cc: Michael Ellerman
Cc: "Naveen N. Rao"
Cc: linuxppc-dev@lists.ozlabs.org
---
arch/powerpc/include/asm/kprobes.h | 10 --
arch/powerpc/kernel/kprobes-ftrace.c | 16 +++-
arch/powerpc/kernel/kprobes.c| 31 +++
3 files changed, 14 insertions(+), 43 deletions(-)
diff --git a/arch/powerpc/include/asm/kprobes.h
b/arch/powerpc/include/asm/kprobes.h
index 674036db558b..785c464b6588 100644
--- a/arch/powerpc/include/asm/kprobes.h
+++ b/arch/powerpc/include/asm/kprobes.h
@@ -102,16 +102,6 @@ extern int kprobe_exceptions_notify(struct notifier_block
*self,
extern int kprobe_fault_handler(struct pt_regs *regs, int trapnr);
extern int kprobe_handler(struct pt_regs *regs);
extern int kprobe_post_handler(struct pt_regs *regs);
-#ifdef CONFIG_KPROBES_ON_FTRACE
-extern int skip_singlestep(struct kprobe *p, struct pt_regs *regs,
- struct kprobe_ctlblk *kcb);
-#else
-static inline int skip_singlestep(struct kprobe *p, struct pt_regs *regs,
- struct kprobe_ctlblk *kcb)
-{
- return 0;
-}
-#endif
#else
static inline int kprobe_handler(struct pt_regs *regs) { return 0; }
static inline int kprobe_post_handler(struct pt_regs *regs) { return 0; }
diff --git a/arch/powerpc/kernel/kprobes-ftrace.c
b/arch/powerpc/kernel/kprobes-ftrace.c
index 1b316331c2d9..3869b0e5d5c7 100644
--- a/arch/powerpc/kernel/kprobes-ftrace.c
+++ b/arch/powerpc/kernel/kprobes-ftrace.c
@@ -26,8 +26,8 @@
#include
static nokprobe_inline
-int __skip_singlestep(struct kprobe *p, struct pt_regs *regs,
- struct kprobe_ctlblk *kcb, unsigned long orig_nip)
+int skip_singlestep(struct kprobe *p, struct pt_regs *regs,
+ struct kprobe_ctlblk *kcb, unsigned long orig_nip)
{
/*
* Emulate singlestep (and also recover regs->nip)
@@ -44,16 +44,6 @@ int __skip_singlestep(struct kprobe *p, struct pt_regs *regs,
return 1;
}
-int skip_singlestep(struct kprobe *p, struct pt_regs *regs,
- struct kprobe_ctlblk *kcb)
-{
- if (kprobe_ftrace(p))
- return __skip_singlestep(p, regs, kcb, 0);
- else
- return 0;
-}
-NOKPROBE_SYMBOL(skip_singlestep);
-
/* Ftrace callback handler for kprobes */
void kprobe_ftrace_handler(unsigned long nip, unsigned long parent_nip,
struct ftrace_ops *ops, struct pt_regs *regs)
@@ -82,7 +72,7 @@ void kprobe_ftrace_handler(unsigned long nip, unsigned long
parent_nip,
__this_cpu_write(current_kprobe, p);
kcb->kprobe_status = KPROBE_HIT_ACTIVE;
if (!p->pre_handler || !p->pre_handler(p, regs))
- __skip_singlestep(p, regs, kcb, orig_nip);
+ skip_singlestep(p, regs, kcb, orig_nip);
else {
/*
* If pre_handler returns !0, it sets regs->nip and
diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
index 600678fce0a8..f06747e2e70d 100644
--- a/arch/powerpc/kernel/kprobes.c
+++ b/arch/powerpc/kernel/kprobes.c
@@ -317,25 +317,17 @@ int kprobe_handler(struct pt_regs *regs)
}
prepare_singlestep(p, regs);
return 1;
- } else {
- if (*addr != BREAKPOINT_INSTRUCTION) {
- /* If trap variant, then it belongs not to us */
- kprobe_opcode_t cur_insn = *addr;
- if (is_trap(cur_insn))
- goto no_kprobe;
- /* The breakpoint instruction was removed by
-* another cpu right after we hit, no further
-* handling of this interrupt is appropriate
-*/
- ret = 1;
+ } else if (*addr != BREAKPOINT_INSTRUCTION) {
+ /* If trap variant, then it belongs not to us */
+ kprobe_opcode_t cur_insn = *addr;
+
+ if (is_trap(cur_insn))
goto no_kprobe;
- }
- p = __this_cpu_read(current_kprobe);
- if (p->break_handler && p->break_handler(p, regs)) {
- if (!skip_singlestep(p, regs, kcb))
- goto ss_probe;
- ret = 1;
- }
+ /* The breakpoint
