| Message ID | 0572aa31dd57bef6f158ac97d0b666a0c545356a.1494871248.git.naveen.n.rao@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
On Mon, 15 May 2017 23:35:03 +0530 "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote: > diff --git a/arch/powerpc/include/asm/kprobes.h b/arch/powerpc/include/asm/kprobes.h > index a83821f33ea3..b6960ef213ac 100644 > --- a/arch/powerpc/include/asm/kprobes.h > +++ b/arch/powerpc/include/asm/kprobes.h > @@ -61,6 +61,15 @@ extern kprobe_opcode_t optprobe_template_end[]; > #define MAX_OPTINSN_SIZE (optprobe_template_end - optprobe_template_entry) > #define RELATIVEJUMP_SIZE sizeof(kprobe_opcode_t) /* 4 bytes */ > > +/* Save upto 16 parameters along with the stack frame header */ > +#define MAX_STACK_SIZE (STACK_FRAME_PARM_SAVE + (16 * sizeof(unsigned long))) > +#define MIN_STACK_SIZE(ADDR) \ > + (((MAX_STACK_SIZE) < (((unsigned long)current_thread_info()) + \ > + THREAD_SIZE - (unsigned long)(ADDR))) \ > + ? (MAX_STACK_SIZE) \ > + : (((unsigned long)current_thread_info()) + \ > + THREAD_SIZE - (unsigned long)(ADDR))) Could you add CUR_STACK_SIZE(addr) as x86 does instead of repeating similar code? Thank you,
"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> writes: > As pointed out in x86 setjmp_pre_handler(), we need to save and restore > the parameter save area since the jprobe hook might overwrite it. Since > there is no easy way to identify the size of the parameter save area, > we choose to save/restore a fixed 16 [double]word-sized area including > the stack frame header. > > We introduce STACK_FRAME_PARM_SAVE to encode the offset of the parameter > save area from the stack frame pointer. Remove the similarly named > PARAMETER_SAVE_AREA_OFFSET in ptrace.c as those are currently not used > anywhere. > > Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com> > --- > Michael, > I've set the limit to 16 parameters as being a "reasonable" number, but > we could very well make this 24 or 32 if we want to be sure. Let me > know what you prefer. That sounds incredibly fragile. Are we really just guessing at the size required? What happens if we under estimate, do we crash, silently corrupt data .. ? cheers
On 2017/05/18 03:22PM, Michael Ellerman wrote: > "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> writes: > > > As pointed out in x86 setjmp_pre_handler(), we need to save and restore > > the parameter save area since the jprobe hook might overwrite it. Since > > there is no easy way to identify the size of the parameter save area, > > we choose to save/restore a fixed 16 [double]word-sized area including > > the stack frame header. > > > > We introduce STACK_FRAME_PARM_SAVE to encode the offset of the parameter > > save area from the stack frame pointer. Remove the similarly named > > PARAMETER_SAVE_AREA_OFFSET in ptrace.c as those are currently not used > > anywhere. > > > > Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com> > > --- > > Michael, > > I've set the limit to 16 parameters as being a "reasonable" number, but > > we could very well make this 24 or 32 if we want to be sure. Let me > > know what you prefer. > > That sounds incredibly fragile. Are we really just guessing at the size > required? What happens if we under estimate, do we crash, silently > corrupt data .. ? There is a _chance_ of crashing/corrupting data, yes. The conditions required for that include: - a function that takes > 8 parameters, hence requiring parameters to be passed on the stack - the jprobe hook replacing that function having to clobber one of those parameters passed on the stack. Assuming C code, I am not sure what conditions could trigger gcc to emit code that may do the latter. However, per the ABI, this is possible as the parameter save area is owned by the callee and the caller can't assume that it'll remain intact. Given the slim probability, I felt that saving/restoring a fixed amount of stack is sufficient to mitigate this. The other option is to save/restore the entire stack frame, but that may be quite large and just a lot of overhead in most cases. Thoughts? - Naveen
On 2017/05/17 10:12AM, Masami Hiramatsu wrote: > On Mon, 15 May 2017 23:35:03 +0530 > "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote: > > diff --git a/arch/powerpc/include/asm/kprobes.h b/arch/powerpc/include/asm/kprobes.h > > index a83821f33ea3..b6960ef213ac 100644 > > --- a/arch/powerpc/include/asm/kprobes.h > > +++ b/arch/powerpc/include/asm/kprobes.h > > @@ -61,6 +61,15 @@ extern kprobe_opcode_t optprobe_template_end[]; > > #define MAX_OPTINSN_SIZE (optprobe_template_end - optprobe_template_entry) > > #define RELATIVEJUMP_SIZE sizeof(kprobe_opcode_t) /* 4 bytes */ > > > > +/* Save upto 16 parameters along with the stack frame header */ > > +#define MAX_STACK_SIZE (STACK_FRAME_PARM_SAVE + (16 * sizeof(unsigned long))) > > +#define MIN_STACK_SIZE(ADDR) \ > > + (((MAX_STACK_SIZE) < (((unsigned long)current_thread_info()) + \ > > + THREAD_SIZE - (unsigned long)(ADDR))) \ > > + ? (MAX_STACK_SIZE) \ > > + : (((unsigned long)current_thread_info()) + \ > > + THREAD_SIZE - (unsigned long)(ADDR))) > > Could you add CUR_STACK_SIZE(addr) as x86 does instead of repeating similar code? Sure, thanks for calling me out on this ;D - Naveen
diff --git a/arch/powerpc/include/asm/kprobes.h b/arch/powerpc/include/asm/kprobes.h index a83821f33ea3..b6960ef213ac 100644 --- a/arch/powerpc/include/asm/kprobes.h +++ b/arch/powerpc/include/asm/kprobes.h @@ -61,6 +61,15 @@ extern kprobe_opcode_t optprobe_template_end[]; #define MAX_OPTINSN_SIZE (optprobe_template_end - optprobe_template_entry) #define RELATIVEJUMP_SIZE sizeof(kprobe_opcode_t) /* 4 bytes */ +/* Save upto 16 parameters along with the stack frame header */ +#define MAX_STACK_SIZE (STACK_FRAME_PARM_SAVE + (16 * sizeof(unsigned long))) +#define MIN_STACK_SIZE(ADDR) \ + (((MAX_STACK_SIZE) < (((unsigned long)current_thread_info()) + \ + THREAD_SIZE - (unsigned long)(ADDR))) \ + ? (MAX_STACK_SIZE) \ + : (((unsigned long)current_thread_info()) + \ + THREAD_SIZE - (unsigned long)(ADDR))) + #define flush_insn_slot(p) do { } while (0) #define kretprobe_blacklist_size 0 @@ -90,6 +99,7 @@ struct kprobe_ctlblk { unsigned long kprobe_saved_msr; struct pt_regs jprobe_saved_regs; struct prev_kprobe prev_kprobe; + u8 jprobes_stack[MAX_STACK_SIZE]; }; struct arch_optimized_insn { diff --git a/arch/powerpc/include/asm/ptrace.h b/arch/powerpc/include/asm/ptrace.h index e4923686e43a..a50d0514a181 100644 --- a/arch/powerpc/include/asm/ptrace.h +++ b/arch/powerpc/include/asm/ptrace.h @@ -49,8 +49,14 @@ #ifdef PPC64_ELF_ABI_v2 #define STACK_FRAME_MIN_SIZE 32 +/* + * The parameter save area on the stack is used to store arguments being passed + * to callee function and is located at fixed offset from stack pointer. + */ +#define STACK_FRAME_PARM_SAVE 32 #else #define STACK_FRAME_MIN_SIZE STACK_FRAME_OVERHEAD +#define STACK_FRAME_PARM_SAVE 48 #endif /* Size of dummy stack frame allocated when calling signal handler. */ @@ -67,6 +73,7 @@ #define STACK_INT_FRAME_SIZE (sizeof(struct pt_regs) + STACK_FRAME_OVERHEAD) #define STACK_FRAME_MARKER 2 #define STACK_FRAME_MIN_SIZE STACK_FRAME_OVERHEAD +#define STACK_FRAME_PARM_SAVE 8 /* Size of stack frame allocated when calling signal handler. */ #define __SIGNAL_FRAMESIZE 64 diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c index 4398ea60b4e0..19b053475758 100644 --- a/arch/powerpc/kernel/kprobes.c +++ b/arch/powerpc/kernel/kprobes.c @@ -37,6 +37,7 @@ #include <asm/sstep.h> #include <asm/sections.h> #include <linux/uaccess.h> +#include <asm/bitops.h> DEFINE_PER_CPU(struct kprobe *, current_kprobe) = NULL; DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk); @@ -598,8 +599,10 @@ int setjmp_pre_handler(struct kprobe *p, struct pt_regs *regs) { struct jprobe *jp = container_of(p, struct jprobe, kp); struct kprobe_ctlblk *kcb = get_kprobe_ctlblk(); + unsigned long sp = kernel_stack_pointer(regs); memcpy(&kcb->jprobe_saved_regs, regs, sizeof(struct pt_regs)); + memcpy(&kcb->jprobes_stack, (void *)sp, MIN_STACK_SIZE(sp)); /* setup return addr to the jprobe handler routine */ regs->nip = arch_deref_entry_point(jp->entry); @@ -636,6 +639,7 @@ 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 @@ -643,6 +647,9 @@ int longjmp_break_handler(struct kprobe *p, struct pt_regs *regs) * saved regs... */ 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)); + /* It's OK to start function graph tracing again */ unpause_graph_tracing(); preempt_enable_no_resched(); diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c index 925a4ef90559..7631318b5f37 100644 --- a/arch/powerpc/kernel/ptrace.c +++ b/arch/powerpc/kernel/ptrace.c @@ -44,16 +44,6 @@ #define CREATE_TRACE_POINTS #include <trace/events/syscalls.h> -/* - * The parameter save area on the stack is used to store arguments being passed - * to callee function and is located at fixed offset from stack pointer. - */ -#ifdef CONFIG_PPC32 -#define PARAMETER_SAVE_AREA_OFFSET 24 /* bytes */ -#else /* CONFIG_PPC32 */ -#define PARAMETER_SAVE_AREA_OFFSET 48 /* bytes */ -#endif - struct pt_regs_offset { const char *name; int offset;
As pointed out in x86 setjmp_pre_handler(), we need to save and restore the parameter save area since the jprobe hook might overwrite it. Since there is no easy way to identify the size of the parameter save area, we choose to save/restore a fixed 16 [double]word-sized area including the stack frame header. We introduce STACK_FRAME_PARM_SAVE to encode the offset of the parameter save area from the stack frame pointer. Remove the similarly named PARAMETER_SAVE_AREA_OFFSET in ptrace.c as those are currently not used anywhere. Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com> --- Michael, I've set the limit to 16 parameters as being a "reasonable" number, but we could very well make this 24 or 32 if we want to be sure. Let me know what you prefer. Thanks, Naveen arch/powerpc/include/asm/kprobes.h | 10 ++++++++++ arch/powerpc/include/asm/ptrace.h | 7 +++++++ arch/powerpc/kernel/kprobes.c | 7 +++++++ arch/powerpc/kernel/ptrace.c | 10 ---------- 4 files changed, 24 insertions(+), 10 deletions(-)