On Thu, 1 Jun 2017 16:18:17 +0530
"Naveen N. Rao" <naveen.n....@linux.vnet.ibm.com> wrote:
> ftrace_caller() depends on a modified regs->nip to detect if a certain
> function has been livepatched. However, with KPROBES_ON_FTRACE, it is
> possible for regs->nip to have been modified by the kprobes pre_handler
> (jprobes, for instance). In this case, we do not want to invoke the
> livepatch_handler so as not to consume the livepatch stack.
>
> To distinguish between the two (kprobes and livepatch), we check if
> there is an active kprobe on the current function. If there is, then we
> know for sure that it must have modified the NIP as we don't support
> livepatching a kprobe'd function. In this case, we simply skip the
> livepatch_handler and branch to the new NIP. Otherwise, the
> livepatch_handler is invoked.
OK, this logic seems good to me, but I weould like to leave it for
PPC64 maintainer too.
Reviewed-by: Masami Hiramatsu <mhira...@kernel.org>
Thanks!
>
> Fixes: ead514d5fb30a ("powerpc/kprobes: Add support for
> KPROBES_ON_FTRACE")
> Signed-off-by: Naveen N. Rao <naveen.n....@linux.vnet.ibm.com>
> ---
> v2: Changed to use current_kprobe->addr to determine jprobe vs.
> livepatch.
>
> arch/powerpc/include/asm/kprobes.h | 1 +
> arch/powerpc/kernel/kprobes.c | 6 +++++
> arch/powerpc/kernel/trace/ftrace_64_mprofile.S | 32
> ++++++++++++++++++++++----
> 3 files changed, 35 insertions(+), 4 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/kprobes.h
> b/arch/powerpc/include/asm/kprobes.h
> index a83821f33ea3..8814a7249ceb 100644
> --- a/arch/powerpc/include/asm/kprobes.h
> +++ b/arch/powerpc/include/asm/kprobes.h
> @@ -103,6 +103,7 @@ 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);
> +extern int is_current_kprobe_addr(unsigned long addr);
> #ifdef CONFIG_KPROBES_ON_FTRACE
> extern int skip_singlestep(struct kprobe *p, struct pt_regs *regs,
> struct kprobe_ctlblk *kcb);
> diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
> index 0554d6e66194..ec9d94c82fbd 100644
> --- a/arch/powerpc/kernel/kprobes.c
> +++ b/arch/powerpc/kernel/kprobes.c
> @@ -43,6 +43,12 @@ DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk);
>
> struct kretprobe_blackpoint kretprobe_blacklist[] = {{NULL, NULL}};
>
> +int is_current_kprobe_addr(unsigned long addr)
> +{
> + struct kprobe *p = kprobe_running();
> + return (p && (unsigned long)p->addr == addr) ? 1 : 0;
> +}
> +
> bool arch_within_kprobe_blacklist(unsigned long addr)
> {
> return (addr >= (unsigned long)__kprobes_text_start &&
> diff --git a/arch/powerpc/kernel/trace/ftrace_64_mprofile.S
> b/arch/powerpc/kernel/trace/ftrace_64_mprofile.S
> index fa0921410fa4..e6837e85ec28 100644
> --- a/arch/powerpc/kernel/trace/ftrace_64_mprofile.S
> +++ b/arch/powerpc/kernel/trace/ftrace_64_mprofile.S
> @@ -99,13 +99,37 @@ ftrace_call:
> bl ftrace_stub
> nop
>
> - /* Load ctr with the possibly modified NIP */
> - ld r3, _NIP(r1)
> - mtctr r3
> + /* Load the possibly modified NIP */
> + ld r15, _NIP(r1)
> +
> #ifdef CONFIG_LIVEPATCH
> - cmpd r14,r3 /* has NIP been altered? */
> + cmpd r14, r15 /* has NIP been altered? */
> +#endif
> +
> +#if defined(CONFIG_LIVEPATCH) && defined(CONFIG_KPROBES_ON_FTRACE)
> + beq 1f
> +
> + /* Check if there is an active kprobe on us */
> + subi r3, r14, 4
> + bl is_current_kprobe_addr
> + nop
> +
> + /*
> + * If r3 == 1, then this is a kprobe/jprobe.
> + * else, this is livepatched function.
> + *
> + * The subsequent conditional branch for livepatch_handler
> + * will use the result of this compare. For kprobe/jprobe,
> + * we just need to branch to the new NIP, so nothing special
> + * is needed.
> + */
> + cmpdi r3, 1
> +1:
> #endif
>
> + /* Load CTR with the possibly modified NIP */
> + mtctr r15
> +
> /* Restore gprs */
> REST_GPR(0,r1)
> REST_10GPRS(2,r1)
> --
> 2.12.2
>
--
Masami Hiramatsu <mhira...@kernel.org>
