From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-20.2 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,MENTIONS_GIT_HOSTING,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED, USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id A1333C4332E for ; Thu, 18 Mar 2021 18:37:20 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 7559164F40 for ; Thu, 18 Mar 2021 18:37:20 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232684AbhCRSgw (ORCPT ); Thu, 18 Mar 2021 14:36:52 -0400 Received: from mail.kernel.org ([198.145.29.99]:43168 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232523AbhCRSg2 (ORCPT ); Thu, 18 Mar 2021 14:36:28 -0400 Received: by mail.kernel.org (Postfix) with ESMTPSA id BEFF964F30; Thu, 18 Mar 2021 18:36:26 +0000 (UTC) Date: Thu, 18 Mar 2021 18:36:24 +0000 From: Catalin Marinas To: Mark Rutland Cc: Chen Jun , linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, akpm@linux-foundation.org, will@kernel.org, rui.xiang@huawei.com, Mark Brown Subject: Re: [PATCH 2/2] arm64: stacktrace: Add skip when task == current Message-ID: <20210318183623.GB10758@arm.com> References: <20210317142050.57712-1-chenjun102@huawei.com> <20210317142050.57712-3-chenjun102@huawei.com> <20210317183636.GG12269@arm.com> <20210317193416.GB9786@C02TD0UTHF1T.local> <20210318161723.GA10758@arm.com> <20210318171207.GB29466@C02TD0UTHF1T.local> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210318171207.GB29466@C02TD0UTHF1T.local> User-Agent: Mutt/1.10.1 (2018-07-13) Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Mar 18, 2021 at 05:12:07PM +0000, Mark Rutland wrote: > On Thu, Mar 18, 2021 at 04:17:24PM +0000, Catalin Marinas wrote: > > On Wed, Mar 17, 2021 at 07:34:16PM +0000, Mark Rutland wrote: > > > On Wed, Mar 17, 2021 at 06:36:36PM +0000, Catalin Marinas wrote: > > > > On Wed, Mar 17, 2021 at 02:20:50PM +0000, Chen Jun wrote: > > > > > On ARM64, cat /sys/kernel/debug/page_owner, all pages return the same > > > > > stack: > > > > > stack_trace_save+0x4c/0x78 > > > > > register_early_stack+0x34/0x70 > > > > > init_page_owner+0x34/0x230 > > > > > page_ext_init+0x1bc/0x1dc > > > > > > > > > > The reason is that: > > > > > check_recursive_alloc always return 1 because that > > > > > entries[0] is always equal to ip (__set_page_owner+0x3c/0x60). > > > > > > > > > > The root cause is that: > > > > > commit 5fc57df2f6fd ("arm64: stacktrace: Convert to ARCH_STACKWALK") > > > > > make the save_trace save 2 more entries. > > > > > > > > > > Add skip in arch_stack_walk when task == current. > > > > > > > > > > Fixes: 5fc57df2f6fd ("arm64: stacktrace: Convert to ARCH_STACKWALK") > > > > > Signed-off-by: Chen Jun > > > > > --- > > > > > arch/arm64/kernel/stacktrace.c | 5 +++-- > > > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c > > > > > index ad20981..c26b0ac 100644 > > > > > --- a/arch/arm64/kernel/stacktrace.c > > > > > +++ b/arch/arm64/kernel/stacktrace.c > > > > > @@ -201,11 +201,12 @@ void arch_stack_walk(stack_trace_consume_fn consume_entry, void *cookie, > > > > > > > > > > if (regs) > > > > > start_backtrace(&frame, regs->regs[29], regs->pc); > > > > > - else if (task == current) > > > > > + else if (task == current) { > > > > > + ((struct stacktrace_cookie *)cookie)->skip += 2; > > > > > start_backtrace(&frame, > > > > > (unsigned long)__builtin_frame_address(0), > > > > > (unsigned long)arch_stack_walk); > > > > > - else > > > > > + } else > > > > > start_backtrace(&frame, thread_saved_fp(task), > > > > > thread_saved_pc(task)); > > > > > > > > I don't like abusing the cookie here. It's void * as it's meant to be an > > > > opaque type. I'd rather skip the first two frames in walk_stackframe() > > > > instead before invoking fn(). > > > > > > I agree that we shouldn't touch cookie here. > > > > > > I don't think that it's right to bodge this inside walk_stackframe(), > > > since that'll add bogus skipping for the case starting with regs in the > > > current task. If we need a bodge, it has to live in arch_stack_walk() > > > where we set up the initial unwinding state. > > > > Good point. However, instead of relying on __builtin_frame_address(1), > > can we add a 'skip' value to struct stackframe via arch_stack_walk() -> > > start_backtrace() that is consumed by walk_stackframe()? > > We could, but I'd strongly prefer to use __builtin_frame_address(1) if > we can, as it's much simpler to read and keeps the logic constrained to > the starting function. I'd already hacked that up at: > > https://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git/commit/?h=arm64/unwind&id=5811a76c1be1dcea7104a9a771fc2604bc2a90ef > > ... and I'm fairly confident that this works on arm64. If it works with both clang and gcc (and various versions), it's cleaner this way. > If __builtin_frame_address(1) is truly unreliable, then we could just > manually unwind one step within arch_stack_walk() when unwinding > current, which I think is cleaner than spreading this within > walk_stackframe(). > > I can clean up the commit message and post that as a real patch, if you > like? Yes, please. Either variant is fine by me, with a preference for __builtin_frame_address(1) (if we know it works). > > > In another thread, we came to the conclusion that arch_stack_walk() > > > should start at its parent, and its parent should add any skipping it > > > requires. > > > > This makes sense. > > > > > Currently, arch_stack_walk() is off-by-one, and we can bodge that by > > > using __builtin_frame_address(1), though I'm waiting for some compiler > > > folk to confirm that's sound. Otherwise we need to add an assembly > > > trampoline to snapshot the FP, which is unfortunastely convoluted. > > > > > > This report suggests that a caller of arch_stack_walk() is off-by-one > > > too, which suggests a larger cross-architecture semantic issue. I'll try > > > to take a look tomorrow. > > > > I don't think the caller is off by one, at least not by the final skip > > value. __set_page_owner() wants the trace to start at its caller. The > > callee save_stack() in the same file adds a skip of 2. > > save_stack_trace() increments the skip before invoking > > arch_stack_walk(). So far, this assumes that arch_stack_walk() starts at > > its parent, i.e. save_stack_trace(). > > FWIW, I had only assumed the caller was also off-by-one because the > commit message for this patch said the conversion to ARCH_STACKWALK > added two entries. Have I misunderstood, or is that incorrect? I think the commit log is incorrect. Prior to the ARCH_STACKWALK conversion, __save_stack_trace() was skipping 2 since it was creating the initial stack_trace_data and called from save_stack_trace(). After the conversion, the start frame is initialised by arch_stack_walk() which doesn't have any other arch-specific caller it needs to skip. -- Catalin From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-20.5 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,MENTIONS_GIT_HOSTING,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 3CFA9C433E0 for ; Thu, 18 Mar 2021 18:38:07 +0000 (UTC) Received: from desiato.infradead.org (desiato.infradead.org [90.155.92.199]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id C48D864F1F for ; Thu, 18 Mar 2021 18:38:06 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org C48D864F1F Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=arm.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=desiato.20200630; h=Sender:Content-Transfer-Encoding :Content-Type:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References:Message-ID: Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=2ySAtCaNll/PMgu7RKhGhXuLpb3MFnVGqDmTE30P5AY=; b=eBkovPhue4MacChWTa6ej/q3u qXCB1L1AYekmikno5F6q4nhebBRut2/+4eF9lIS/smCmQ4VP24mg6EDoYvRT41D672qdYru3XCLlc Ef6YhUvZf4HVv79CCdD5iRRF1tv0oLcYwyUVh+6A1GTadCWYgcaVZjeGVBnrvSK1I/arTmSAHvem5 RcFCq+RGuNbzHkaLYT79gCZxH1kGZLqEpmuoOUdzZHVseltaWRdrNFepRgttdWIdIx5dxtHBcsNn7 OT/jCs0oBdlpItRGngNTECy1Yxnfk2ArU9x5SyTgVeqm3AIGP4ApNCDFQ38oIc1V06yTreQlO3V8f 0N8lD5gYg==; Received: from localhost ([::1] helo=desiato.infradead.org) by desiato.infradead.org with esmtp (Exim 4.94 #2 (Red Hat Linux)) id 1lMxVb-005rCn-NV; Thu, 18 Mar 2021 18:36:35 +0000 Received: from mail.kernel.org ([198.145.29.99]) by desiato.infradead.org with esmtps (Exim 4.94 #2 (Red Hat Linux)) id 1lMxVV-005rCV-L5 for linux-arm-kernel@lists.infradead.org; Thu, 18 Mar 2021 18:36:32 +0000 Received: by mail.kernel.org (Postfix) with ESMTPSA id BEFF964F30; Thu, 18 Mar 2021 18:36:26 +0000 (UTC) Date: Thu, 18 Mar 2021 18:36:24 +0000 From: Catalin Marinas To: Mark Rutland Cc: Chen Jun , linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, akpm@linux-foundation.org, will@kernel.org, rui.xiang@huawei.com, Mark Brown Subject: Re: [PATCH 2/2] arm64: stacktrace: Add skip when task == current Message-ID: <20210318183623.GB10758@arm.com> References: <20210317142050.57712-1-chenjun102@huawei.com> <20210317142050.57712-3-chenjun102@huawei.com> <20210317183636.GG12269@arm.com> <20210317193416.GB9786@C02TD0UTHF1T.local> <20210318161723.GA10758@arm.com> <20210318171207.GB29466@C02TD0UTHF1T.local> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20210318171207.GB29466@C02TD0UTHF1T.local> User-Agent: Mutt/1.10.1 (2018-07-13) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210318_183630_182465_9536FCD4 X-CRM114-Status: GOOD ( 45.27 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Thu, Mar 18, 2021 at 05:12:07PM +0000, Mark Rutland wrote: > On Thu, Mar 18, 2021 at 04:17:24PM +0000, Catalin Marinas wrote: > > On Wed, Mar 17, 2021 at 07:34:16PM +0000, Mark Rutland wrote: > > > On Wed, Mar 17, 2021 at 06:36:36PM +0000, Catalin Marinas wrote: > > > > On Wed, Mar 17, 2021 at 02:20:50PM +0000, Chen Jun wrote: > > > > > On ARM64, cat /sys/kernel/debug/page_owner, all pages return the same > > > > > stack: > > > > > stack_trace_save+0x4c/0x78 > > > > > register_early_stack+0x34/0x70 > > > > > init_page_owner+0x34/0x230 > > > > > page_ext_init+0x1bc/0x1dc > > > > > > > > > > The reason is that: > > > > > check_recursive_alloc always return 1 because that > > > > > entries[0] is always equal to ip (__set_page_owner+0x3c/0x60). > > > > > > > > > > The root cause is that: > > > > > commit 5fc57df2f6fd ("arm64: stacktrace: Convert to ARCH_STACKWALK") > > > > > make the save_trace save 2 more entries. > > > > > > > > > > Add skip in arch_stack_walk when task == current. > > > > > > > > > > Fixes: 5fc57df2f6fd ("arm64: stacktrace: Convert to ARCH_STACKWALK") > > > > > Signed-off-by: Chen Jun > > > > > --- > > > > > arch/arm64/kernel/stacktrace.c | 5 +++-- > > > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c > > > > > index ad20981..c26b0ac 100644 > > > > > --- a/arch/arm64/kernel/stacktrace.c > > > > > +++ b/arch/arm64/kernel/stacktrace.c > > > > > @@ -201,11 +201,12 @@ void arch_stack_walk(stack_trace_consume_fn consume_entry, void *cookie, > > > > > > > > > > if (regs) > > > > > start_backtrace(&frame, regs->regs[29], regs->pc); > > > > > - else if (task == current) > > > > > + else if (task == current) { > > > > > + ((struct stacktrace_cookie *)cookie)->skip += 2; > > > > > start_backtrace(&frame, > > > > > (unsigned long)__builtin_frame_address(0), > > > > > (unsigned long)arch_stack_walk); > > > > > - else > > > > > + } else > > > > > start_backtrace(&frame, thread_saved_fp(task), > > > > > thread_saved_pc(task)); > > > > > > > > I don't like abusing the cookie here. It's void * as it's meant to be an > > > > opaque type. I'd rather skip the first two frames in walk_stackframe() > > > > instead before invoking fn(). > > > > > > I agree that we shouldn't touch cookie here. > > > > > > I don't think that it's right to bodge this inside walk_stackframe(), > > > since that'll add bogus skipping for the case starting with regs in the > > > current task. If we need a bodge, it has to live in arch_stack_walk() > > > where we set up the initial unwinding state. > > > > Good point. However, instead of relying on __builtin_frame_address(1), > > can we add a 'skip' value to struct stackframe via arch_stack_walk() -> > > start_backtrace() that is consumed by walk_stackframe()? > > We could, but I'd strongly prefer to use __builtin_frame_address(1) if > we can, as it's much simpler to read and keeps the logic constrained to > the starting function. I'd already hacked that up at: > > https://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git/commit/?h=arm64/unwind&id=5811a76c1be1dcea7104a9a771fc2604bc2a90ef > > ... and I'm fairly confident that this works on arm64. If it works with both clang and gcc (and various versions), it's cleaner this way. > If __builtin_frame_address(1) is truly unreliable, then we could just > manually unwind one step within arch_stack_walk() when unwinding > current, which I think is cleaner than spreading this within > walk_stackframe(). > > I can clean up the commit message and post that as a real patch, if you > like? Yes, please. Either variant is fine by me, with a preference for __builtin_frame_address(1) (if we know it works). > > > In another thread, we came to the conclusion that arch_stack_walk() > > > should start at its parent, and its parent should add any skipping it > > > requires. > > > > This makes sense. > > > > > Currently, arch_stack_walk() is off-by-one, and we can bodge that by > > > using __builtin_frame_address(1), though I'm waiting for some compiler > > > folk to confirm that's sound. Otherwise we need to add an assembly > > > trampoline to snapshot the FP, which is unfortunastely convoluted. > > > > > > This report suggests that a caller of arch_stack_walk() is off-by-one > > > too, which suggests a larger cross-architecture semantic issue. I'll try > > > to take a look tomorrow. > > > > I don't think the caller is off by one, at least not by the final skip > > value. __set_page_owner() wants the trace to start at its caller. The > > callee save_stack() in the same file adds a skip of 2. > > save_stack_trace() increments the skip before invoking > > arch_stack_walk(). So far, this assumes that arch_stack_walk() starts at > > its parent, i.e. save_stack_trace(). > > FWIW, I had only assumed the caller was also off-by-one because the > commit message for this patch said the conversion to ARCH_STACKWALK > added two entries. Have I misunderstood, or is that incorrect? I think the commit log is incorrect. Prior to the ARCH_STACKWALK conversion, __save_stack_trace() was skipping 2 since it was creating the initial stack_trace_data and called from save_stack_trace(). After the conversion, the start frame is initialised by arch_stack_walk() which doesn't have any other arch-specific caller it needs to skip. -- Catalin _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel