From patchwork Tue Aug 11 14:52:00 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jungseok Lee X-Patchwork-Id: 6992671 Return-Path: X-Original-To: patchwork-linux-arm@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork2.web.kernel.org (Postfix) with ESMTP id 399B7C05AC for ; Tue, 11 Aug 2015 14:54:39 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 3821E2038A for ; Tue, 11 Aug 2015 14:54:38 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.9]) (using TLSv1.2 with cipher AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 31FEF205BE for ; Tue, 11 Aug 2015 14:54:37 +0000 (UTC) Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.80.1 #2 (Red Hat Linux)) id 1ZPAue-0000bm-Im; Tue, 11 Aug 2015 14:52:24 +0000 Received: from mail-pd0-x232.google.com ([2607:f8b0:400e:c02::232]) by bombadil.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1ZPAub-0000Xc-QL for linux-arm-kernel@lists.infradead.org; Tue, 11 Aug 2015 14:52:23 +0000 Received: by pdrg1 with SMTP id g1so84463605pdr.2 for ; Tue, 11 Aug 2015 07:52:00 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=subject:mime-version:content-type:from:in-reply-to:date:cc :content-transfer-encoding:message-id:references:to; bh=zq0R3zyP4v/5wfdpwxckBvfTPSsoQNvKcFcbWi5ZdeA=; b=0jBzsVEm9Jqbu0tdLhK7bSjfQWs2cI+ktdNpCAdBDtDhSMD1e0eM/lvKZC6oUJ13fv gByKtG6m7FhcJ7I6POn7cdKPGaMeW49+uifpoNLwaWDoKG41VK7hTgWXjeMGXkmeWnng SO7BUpE39ZW2xId8fWJHaCOsNHRlIYsM7VU0ICUbCH0LODNYIdDvdwiKwOv1iEnDQxh/ CvDqeLpi8LIkOk6r2uLgGAhq3an4a5xMOdPgfPcolx6uBXYaoNqbepit1BbVdV8PC/lY mhDGtDIuhHq3PlIXZUsat1dixBGvMdPdqqcda/WWZVVoPAdfJ+TlCDFUBmnZNrTmI5s8 FdWQ== X-Received: by 10.70.90.193 with SMTP id by1mr57168753pdb.80.1439304720663; Tue, 11 Aug 2015 07:52:00 -0700 (PDT) Received: from [192.168.123.119] ([116.121.77.221]) by smtp.gmail.com with ESMTPSA id bf14sm2986210pdb.28.2015.08.11.07.51.57 (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Tue, 11 Aug 2015 07:52:00 -0700 (PDT) Subject: Re: [RFC v2 0/4] arm64: ftrace: fix incorrect output from stack tracer Mime-Version: 1.0 (Apple Message framework v1283) From: Jungseok Lee In-Reply-To: <1438674249-3447-1-git-send-email-takahiro.akashi@linaro.org> Date: Tue, 11 Aug 2015 23:52:00 +0900 Message-Id: <50B298B0-1978-471B-BCE7-BC433E9993C1@gmail.com> References: <1438674249-3447-1-git-send-email-takahiro.akashi@linaro.org> To: AKASHI Takahiro X-Mailer: Apple Mail (2.1283) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20150811_075221_907658_EA97EE57 X-CRM114-Status: GOOD ( 24.82 ) X-Spam-Score: -2.5 (--) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: catalin.marinas@arm.com, will.deacon@arm.com, linux-kernel@vger.kernel.org, rostedt@goodmis.org, broonie@kernel.org, david.griego@linaro.org, olof@lixom.net, linux-arm-kernel@lists.infradead.org Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org X-Spam-Status: No, score=-4.3 required=5.0 tests=BAYES_00, DKIM_ADSP_CUSTOM_MED, DKIM_SIGNED, FREEMAIL_FROM, RCVD_IN_DNSWL_MED, RP_MATCHES_RCVD, T_DKIM_INVALID, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Aug 4, 2015, at 4:44 PM, AKASHI Takahiro wrote: Hi Akashi, > See the following threads [1],[2] for the background. > > With this patch series, I'm trying to fix several problems I noticed > with stack tracer on arm64. But it is rather experimental, and there still > remained are several issues. > > Patch #1 modifies ftrace framework so that we can provide arm64-specific > stack tracer later. > (Well, I know there is some room for further refactoring, but this is > just a prototype code.) > Patch #2 implements this arch_check_stack() using unwind_stackframe() to > traverse stack frames and identify a stack index for each traced function. > Patch #3 addresses an issue that stack tracer doesn't work well in > conjuction with function graph tracer. > Patch #4 addresses an issue that unwind_stackframe() misses a function > that is the one when an exception happens by setting up a stack frame > for exception handler. > > See below for the result with those patches. > Issues include: > a) Size of gic_handle_irq() is 48 (#13), but should be 64. > b) We cannot identify a location of function which is being returned > and hooked temporarily by return_to_handler() (#18) > c) A meaningless entry (#62) as well as a bogus size for the first > function, el0_svc (#61) > d) Size doesn't contain a function's "dynamically allocated local > variables," if any, but instead is sumed up in child's size. > (See details in [3].) > > I'm afraid that we cannot fix issue b) unless we can do *atomically* > push/pop a return adress in current->ret_stack[], increment/decrement > current->curr_ret_stack and restore lr register. > > We will be able to fix issue d) once we know all the locations in > the list, including b). > > [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-July/354126.html > [2] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-July/355920.html > [3] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-July/358034.html I hope I'm not too late.. This series looks written on top of the hunk in the end of this reply. I've strongly agreed with your opinion as digging out this issue. We need to analyse the first instruction of each function, "stp x29, x30, [sp, #val]!", in order to solve this problem clearly. How about decoding the store-pair instruction? If so, we could record a correct position into stack_dump_index. That is, in your ascii art, top-sp0 and top-sp1 are written into stack_dump_index[i+1] and stack_dump_index[i] respectively. sp2 +-------+ <--------- func-2's stackframe | | | | fp2 +-------+ | fp1 | +-------+ <-- p1 (*p1 == stack_dump_trace[i] == lr1) | lr1 | +-------+ | | | func-2's local variables | | sp1 +-------+ <--------- func-1(lr1)'s stackframe | | (stack_dump_index[i] = top - p1) | func-1's dynamic local variables | | fp1 +-------+ | fp0 | +-------+ <-- p0 (*p0 == stack_dump_trace[i+1] == lr0) | lr0 | +-------+ | | | func-1's local variables | | sp0 +-------+ <--------- func-0(lr0)'s stackframe | | (stack_dump_index[i+1] = top - p0) | | *-------+ top Best Regards Jungseok Lee ----8<---- ----8<---- diff --git a/arch/arm64/include/asm/ftrace.h b/arch/arm64/include/asm/ftrace.h index c5534fa..2b43e20 100644 --- a/arch/arm64/include/asm/ftrace.h +++ b/arch/arm64/include/asm/ftrace.h @@ -13,8 +13,9 @@ #include -#define MCOUNT_ADDR ((unsigned long)_mcount) -#define MCOUNT_INSN_SIZE AARCH64_INSN_SIZE +#define MCOUNT_ADDR ((unsigned long)_mcount) +#define MCOUNT_INSN_SIZE AARCH64_INSN_SIZE +#define FTRACE_STACK_FRAME_OFFSET AARCH64_INSN_SIZE #ifndef __ASSEMBLY__ #include diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c index 407991b..9ab67af 100644 --- a/arch/arm64/kernel/stacktrace.c +++ b/arch/arm64/kernel/stacktrace.c @@ -20,6 +20,7 @@ #include #include +#include #include /* @@ -52,7 +53,7 @@ int notrace unwind_frame(struct stackframe *frame) * -4 here because we care about the PC at time of bl, * not where the return will go. */ - frame->pc = *(unsigned long *)(fp + 8) - 4; + frame->pc = *(unsigned long *)(fp + 8) - AARCH64_INSN_SIZE; return 0; } diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h index 1da6029..6566201 100644 --- a/include/linux/ftrace.h +++ b/include/linux/ftrace.h @@ -260,6 +260,9 @@ static inline void ftrace_kill(void) { } #endif /* CONFIG_FUNCTION_TRACER */ #ifdef CONFIG_STACK_TRACER +#ifndef FTRACE_STACK_FRAME_OFFSET +#define FTRACE_STACK_FRAME_OFFSET 0 +#endif extern int stack_tracer_enabled; int stack_trace_sysctl(struct ctl_table *table, int write, diff --git a/kernel/trace/trace_stack.c b/kernel/trace/trace_stack.c index b746399..30521ea 100644 --- a/kernel/trace/trace_stack.c +++ b/kernel/trace/trace_stack.c @@ -105,7 +105,7 @@ check_stack(unsigned long ip, unsigned long *stack) /* Skip over the overhead of the stack tracer itself */ for (i = 0; i < max_stack_trace.nr_entries; i++) { - if (stack_dump_trace[i] == ip) + if ((stack_dump_trace[i] + FTRACE_STACK_FRAME_OFFSET) == ip) break; } @@ -133,7 +133,8 @@ check_stack(unsigned long ip, unsigned long *stack) for (; p < top && i < max_stack_trace.nr_entries; p++) { if (stack_dump_trace[i] == ULONG_MAX) break; - if (*p == stack_dump_trace[i]) { + if (*p == (stack_dump_trace[i] + + FTRACE_STACK_FRAME_OFFSET)) { stack_dump_trace[x] = stack_dump_trace[i++]; this_size = stack_dump_index[x++] = (top - p) * sizeof(unsigned long);