Messages in this thread | | | Subject | Re: [PATCH v5 5/6] arm64: ftrace: add arch-specific stack tracer | From | Jungseok Lee <> | Date | Thu, 12 Nov 2015 07:56:59 +0900 |
| |
On Nov 11, 2015, at 2:03 PM, AKASHI Takahiro wrote: > Jungseok, > > On 11/10/2015 11:05 PM, Jungseok Lee wrote: >> On Nov 6, 2015, at 3:44 PM, AKASHI Takahiro wrote: >> >> Hi Akashi, >> >>> Stack tracer on arm64, check_stack(), is uniqeue in the following >>> points: >>> * analyze a function prologue of a traced function to estimate a more >>> accurate stack pointer value, replacing naive '<child's fp> + 0x10.' >>> * use walk_stackframe(), instead of slurping stack contents as orignal >>> check_stack() does, to identify a stack frame and a stack index (height) >>> for every callsite. >>> >>> Regarding a function prologue analyzer, there is no guarantee that we can >>> handle all the possible patterns of function prologue as gcc does not use >>> any fixed templates to generate them. 'Instruction scheduling' is another >>> issue here. >>> Nevertheless, the current version will surely cover almost all the cases >>> in the kernel image and give us useful information on stack pointers. >>> >>> So this patch utilizes a function prologue only for stack tracer, and >>> does not affect the behaviors of existing stacktrace functions. >>> >>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> >>> --- >>> arch/arm64/include/asm/stacktrace.h | 4 + >>> arch/arm64/kernel/ftrace.c | 64 ++++++++++++ >>> arch/arm64/kernel/stacktrace.c | 185 ++++++++++++++++++++++++++++++++++- >>> 3 files changed, 250 insertions(+), 3 deletions(-) >>> >>> diff --git a/arch/arm64/include/asm/stacktrace.h b/arch/arm64/include/asm/stacktrace.h >>> index 7318f6d..47b4832 100644 >>> --- a/arch/arm64/include/asm/stacktrace.h >>> +++ b/arch/arm64/include/asm/stacktrace.h >>> @@ -25,5 +25,9 @@ struct stackframe { >>> extern int unwind_frame(struct stackframe *frame); >>> extern void walk_stackframe(struct stackframe *frame, >>> int (*fn)(struct stackframe *, void *), void *data); >>> +#ifdef CONFIG_STACK_TRACER >>> +struct stack_trace; >>> +extern void save_stack_trace_sp(struct stack_trace *trace, unsigned long *sp); >>> +#endif >>> >>> #endif /* __ASM_STACKTRACE_H */ >>> diff --git a/arch/arm64/kernel/ftrace.c b/arch/arm64/kernel/ftrace.c >>> index 314f82d..46bfe37 100644 >>> --- a/arch/arm64/kernel/ftrace.c >>> +++ b/arch/arm64/kernel/ftrace.c >>> @@ -9,6 +9,7 @@ >>> * published by the Free Software Foundation. >>> */ >>> >>> +#include <linux/bug.h> >>> #include <linux/ftrace.h> >>> #include <linux/swab.h> >>> #include <linux/uaccess.h> >>> @@ -16,6 +17,7 @@ >>> #include <asm/cacheflush.h> >>> #include <asm/ftrace.h> >>> #include <asm/insn.h> >>> +#include <asm/stacktrace.h> >>> >>> #ifdef CONFIG_DYNAMIC_FTRACE >>> /* >>> @@ -173,3 +175,65 @@ int ftrace_disable_ftrace_graph_caller(void) >>> } >>> #endif /* CONFIG_DYNAMIC_FTRACE */ >>> #endif /* CONFIG_FUNCTION_GRAPH_TRACER */ >>> + >>> +#ifdef CONFIG_STACK_TRACER >>> +static unsigned long stack_trace_sp[STACK_TRACE_ENTRIES]; >>> +static unsigned long raw_stack_trace_max_size; >>> + >>> +void check_stack(unsigned long ip, unsigned long *stack) >>> +{ >>> + unsigned long this_size, flags; >>> + unsigned long top; >>> + int i, j; >>> + >>> + this_size = ((unsigned long)stack) & (THREAD_SIZE-1); >>> + this_size = THREAD_SIZE - this_size; >>> + >>> + if (this_size <= raw_stack_trace_max_size) >>> + return; >>> + >>> + /* we do not handle an interrupt stack yet */ >>> + if (!object_is_on_stack(stack)) >>> + return; >>> + >>> + local_irq_save(flags); >>> + arch_spin_lock(&max_stack_lock); >>> + >>> + /* check again */ >>> + if (this_size <= raw_stack_trace_max_size) >>> + goto out; >>> + >>> + /* find out stack frames */ >>> + stack_trace_max.nr_entries = 0; >>> + stack_trace_max.skip = 0; >>> + save_stack_trace_sp(&stack_trace_max, stack_trace_sp); >>> + stack_trace_max.nr_entries--; /* for the last entry ('-1') */ >>> + >>> + /* calculate a stack index for each function */ >>> + top = ((unsigned long)stack & ~(THREAD_SIZE-1)) + THREAD_SIZE; >>> + for (i = 0; i < stack_trace_max.nr_entries; i++) >>> + stack_trace_index[i] = top - stack_trace_sp[i]; >>> + raw_stack_trace_max_size = this_size; >>> + >>> + /* Skip over the overhead of the stack tracer itself */ >>> + for (i = 0; i < stack_trace_max.nr_entries; i++) >>> + if (stack_trace_max.entries[i] == ip) >>> + break; >>> + >>> + stack_trace_max.nr_entries -= i; >>> + for (j = 0; j < stack_trace_max.nr_entries; j++) { >>> + stack_trace_index[j] = stack_trace_index[j + i]; >>> + stack_trace_max.entries[j] = stack_trace_max.entries[j + i]; >>> + } >>> + stack_trace_max_size = stack_trace_index[0]; >>> + >>> + if (task_stack_end_corrupted(current)) { >>> + WARN(1, "task stack is corrupted.\n"); >>> + stack_trace_print(); >>> + } >>> + >>> + out: >>> + arch_spin_unlock(&max_stack_lock); >>> + local_irq_restore(flags); >>> +} >>> +#endif /* CONFIG_STACK_TRACER */ >>> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c >>> index 5fd3477..4ee052a 100644 >>> --- a/arch/arm64/kernel/stacktrace.c >>> +++ b/arch/arm64/kernel/stacktrace.c >>> @@ -23,6 +23,144 @@ >>> >>> #include <asm/stacktrace.h> >>> >>> +#ifdef CONFIG_STACK_TRACER >>> +/* >>> + * This function parses a function prologue of a traced function and >>> + * determines its stack size. >>> + * A return value indicates a location of @pc in a function prologue. >>> + * @return value: >>> + * <case 1> <case 1'> >>> + * 1: >>> + * sub sp, sp, #XX sub sp, sp, #XX >>> + * 2: >>> + * stp x29, x30, [sp, #YY] stp x29, x30, [sp, #--ZZ]! >>> + * 3: >>> + * add x29, sp, #YY mov x29, sp >>> + * 0: >>> + * >>> + * <case 2> >>> + * 1: >>> + * stp x29, x30, [sp, #-XX]! >>> + * 3: >>> + * mov x29, sp >>> + * 0: >>> + * >>> + * @size: sp offset from calller's sp (XX or XX + ZZ) >>> + * @size2: fp offset from new sp (YY or 0) >>> + */ >>> +static int analyze_function_prologue(unsigned long pc, >>> + unsigned long *size, unsigned long *size2) >>> +{ >>> + unsigned long offset; >>> + u32 *addr, insn; >>> + int pos = -1; >>> + enum aarch64_insn_register src, dst, reg1, reg2, base; >>> + int imm; >>> + enum aarch64_insn_variant variant; >>> + enum aarch64_insn_adsb_type adsb_type; >>> + enum aarch64_insn_ldst_type ldst_type; >>> + >>> + *size = *size2 = 0; >>> + >>> + if (!pc) >>> + goto out; >>> + >>> + if (unlikely(!kallsyms_lookup_size_offset(pc, NULL, &offset))) >>> + goto out; >>> + >>> + addr = (u32 *)(pc - offset); >>> +#ifdef CONFIG_DYNAMIC_FTRACE >>> + if (addr == (u32 *)ftrace_call) >>> + addr = (u32 *)ftrace_caller; >>> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER >>> + else if (addr == (u32 *)ftrace_graph_caller) >>> +#ifdef CONFIG_DYNAMIC_FTRACE >>> + addr = (u32 *)ftrace_caller; >>> +#else >>> + addr = (u32 *)_mcount; >>> +#endif >>> +#endif >>> +#endif >>> + >>> + insn = *addr; >>> + pos = 1; >>> + >>> + /* analyze a function prologue */ >>> + while ((unsigned long)addr < pc) { >>> + if (aarch64_insn_is_branch_imm(insn) || >>> + aarch64_insn_is_br(insn) || >>> + aarch64_insn_is_blr(insn) || >>> + aarch64_insn_is_ret(insn) || >>> + aarch64_insn_is_eret(insn)) >>> + /* exiting a basic block */ >>> + goto out; >>> + >>> + if (aarch64_insn_decode_add_sub_imm(insn, &dst, &src, >>> + &imm, &variant, &adsb_type)) { >>> + if ((adsb_type == AARCH64_INSN_ADSB_SUB) && >>> + (dst == AARCH64_INSN_REG_SP) && >>> + (src == AARCH64_INSN_REG_SP)) { >>> + /* >>> + * Starting the following sequence: >>> + * sub sp, sp, #xx >>> + * stp x29, x30, [sp, #yy] >>> + * add x29, sp, #yy >>> + */ >>> + WARN_ON(pos != 1); >>> + pos = 2; >>> + *size += imm; >>> + } else if ((adsb_type == AARCH64_INSN_ADSB_ADD) && >>> + (dst == AARCH64_INSN_REG_29) && >>> + (src == AARCH64_INSN_REG_SP)) { >>> + /* >>> + * add x29, sp, #yy >>> + * or >>> + * mov x29, sp >>> + */ >>> + WARN_ON(pos != 3); >>> + pos = 0; >>> + *size2 = imm; >>> + >>> + break; >>> + } >>> + } else if (aarch64_insn_decode_load_store_pair(insn, >>> + ®1, ®2, &base, &imm, >>> + &variant, &ldst_type)) { >>> + if ((ldst_type == >>> + AARCH64_INSN_LDST_STORE_PAIR_PRE_INDEX) && >>> + (reg1 == AARCH64_INSN_REG_29) && >>> + (reg2 == AARCH64_INSN_REG_30) && >>> + (base == AARCH64_INSN_REG_SP)) { >>> + /* >>> + * Starting the following sequence: >>> + * stp x29, x30, [sp, #-xx]! >>> + * mov x29, sp >>> + */ >>> + WARN_ON(!((pos == 1) || (pos == 2))); >>> + pos = 3; >>> + *size += -imm; >>> + } else if ((ldst_type == >>> + AARCH64_INSN_LDST_STORE_PAIR) && >>> + (reg1 == AARCH64_INSN_REG_29) && >>> + (reg2 == AARCH64_INSN_REG_30) && >>> + (base == AARCH64_INSN_REG_SP)) { >>> + /* >>> + * stp x29, x30, [sp, #yy] >>> + */ >>> + WARN_ON(pos != 2); >>> + pos = 3; >>> + } >>> + } >>> + >>> + addr++; >>> + insn = *addr; >>> + } >>> + >>> +out: >>> + return pos; >>> +} >>> +#endif >>> + >> >> A small instruction decoder in software! >> >> From a high level point of view, it makes sense to deal with only three main >> cases. Since it is tough to deal with all function prologues in perspective >> of computational overhead and implementation complexity, the above change looks >> like a pretty good tradeoff. This is what I can say now. I'm also interested >> in feedbacks from other folks. > > I really appreciate your reviews and comments on the whole series of my patches > (#2 to #5). > May I add Reviewed-by (and Tested-by?) with your name if you like?
Sure. Tested-by is also okay.
Regarding patch6, the option would be useful if this analyzer is accepted. So, like this patch5, I'm waiting for other folk's response.
How about adding your document [1] to this series? I think it explains all backgrounds, such as motivation, issue, approach, and TODO, on this series.
[1] http://www.spinics.net/lists/arm-kernel/msg444300.html
Best Regards Jungseok Lee
| |