Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1750881AbbKKFDv (ORCPT ); Wed, 11 Nov 2015 00:03:51 -0500 Received: from mail-pa0-f45.google.com ([209.85.220.45]:33514 "EHLO mail-pa0-f45.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750714AbbKKFDt (ORCPT ); Wed, 11 Nov 2015 00:03:49 -0500 Subject: Re: [PATCH v5 5/6] arm64: ftrace: add arch-specific stack tracer To: Jungseok Lee References: <1446792285-1154-1-git-send-email-takahiro.akashi@linaro.org> <1446792285-1154-6-git-send-email-takahiro.akashi@linaro.org> <823AA540-A025-4D44-852A-10F4A99A5760@gmail.com> Cc: catalin.marinas@arm.com, will.deacon@arm.com, rostedt@goodmis.org, broonie@kernel.org, david.griego@linaro.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org From: AKASHI Takahiro Message-ID: <5642CC2E.7010405@linaro.org> Date: Wed, 11 Nov 2015 14:03:42 +0900 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.2.0 MIME-Version: 1.0 In-Reply-To: <823AA540-A025-4D44-852A-10F4A99A5760@gmail.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 9567 Lines: 301 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 ' + 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 >> --- >> 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 >> #include >> #include >> #include >> @@ -16,6 +17,7 @@ >> #include >> #include >> #include >> +#include >> >> #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 >> >> +#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: >> + * >> + * 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: >> + * >> + * >> + * 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? -Takahiro AKASHI > Best Regards > Jungseok Lee > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/