On 2017/05/17 10:21AM, Masami Hiramatsu wrote:
> On Mon, 15 May 2017 23:35:04 +0530
> "Naveen N. Rao" <naveen.n....@linux.vnet.ibm.com> wrote:
>
> > Fix a circa 2005 FIXME by implementing a check to ensure that we
> > actually got into the jprobe break handler() due to the trap in
> > jprobe_return().
> >
> > Signed-off-by: Naveen N. Rao <naveen.n....@linux.vnet.ibm.com>
> > ---
> > arch/powerpc/kernel/kprobes.c | 20 +++++++++-----------
> > 1 file changed, 9 insertions(+), 11 deletions(-)
> >
> > diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
> > index 19b053475758..1ebeb8c482db 100644
> > --- a/arch/powerpc/kernel/kprobes.c
> > +++ b/arch/powerpc/kernel/kprobes.c
> > @@ -627,25 +627,23 @@ NOKPROBE_SYMBOL(setjmp_pre_handler);
> >
> > void __used jprobe_return(void)
> > {
> > - asm volatile("trap" ::: "memory");
> > + asm volatile("jprobe_return_trap:\n"
> > + "trap\n"
> > + ::: "memory");
> > }
> > NOKPROBE_SYMBOL(jprobe_return);
> >
> > -static void __used jprobe_return_end(void)
> > -{
> > -}
> > -NOKPROBE_SYMBOL(jprobe_return_end);
> > -
> > int longjmp_break_handler(struct kprobe *p, struct pt_regs *regs)
> > {
> > struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
> > unsigned long sp;
> >
> > - /*
> > - * FIXME - we should ideally be validating that we got here 'cos
> > - * of the "trap" in jprobe_return() above, before restoring the
> > - * saved regs...
> > - */
> > + if (regs->nip != ppc_kallsyms_lookup_name("jprobe_return_trap")) {
> > + WARN(1, "longjmp_break_handler NIP (0x%lx) does not match
> > jprobe_return_trap (0x%lx)\n",
> > + regs->nip,
> > ppc_kallsyms_lookup_name("jprobe_return_trap"));
> > + return 0;
>
> If you don't handle this break, you shouldn't warn it, because
> program_check_exception() will continue to find how to handle it
> by notify_die(). IOW, please return silently, or just add a
> debug message.
The only reason this can happen is if we hit a third-party installed
trap while executing a jprobe hook. And given that we run with premption
disabled, I felt that this is an unlikely scenario.
The reason for adding the check was to guard against issues with looking
up the symbol (due to some of the ABI aspects). So, I felt a WARN()
would be good to have and to make it obvious.
I do see your point though. I'm wondering if pr_info() would be better?
Thanks for the review,
- Naveen
