| Message ID | d6b0691226623b3499d43dff1e31971ea46df9f9.1494871248.git.naveen.n.rao@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Mon, 15 May 2017 23:35:04 +0530 "Naveen N. Rao" <naveen.n.rao@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.rao@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. Thank you, > + } > + > memcpy(regs, &kcb->jprobe_saved_regs, sizeof(struct pt_regs)); > sp = kernel_stack_pointer(regs); > memcpy((void *)sp, &kcb->jprobes_stack, MIN_STACK_SIZE(sp)); > -- > 2.12.2 >
On 2017/05/17 10:21AM, Masami Hiramatsu wrote: > On Mon, 15 May 2017 23:35:04 +0530 > "Naveen N. Rao" <naveen.n.rao@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.rao@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
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; + } + memcpy(regs, &kcb->jprobe_saved_regs, sizeof(struct pt_regs)); sp = kernel_stack_pointer(regs); memcpy((void *)sp, &kcb->jprobes_stack, MIN_STACK_SIZE(sp));
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.rao@linux.vnet.ibm.com> --- arch/powerpc/kernel/kprobes.c | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-)