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 Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id B12DBC54E58 for ; Wed, 20 Mar 2024 08:45:44 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id F18A46B0082; Wed, 20 Mar 2024 04:45:43 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id EC81E6B0083; Wed, 20 Mar 2024 04:45:43 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id DB7E96B0085; Wed, 20 Mar 2024 04:45:43 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0016.hostedemail.com [216.40.44.16]) by kanga.kvack.org (Postfix) with ESMTP id CC71D6B0082 for ; Wed, 20 Mar 2024 04:45:43 -0400 (EDT) Received: from smtpin12.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay09.hostedemail.com (Postfix) with ESMTP id 91D8C812EE for ; Wed, 20 Mar 2024 08:45:43 +0000 (UTC) X-FDA: 81916784166.12.7C3BF19 Received: from pandora.armlinux.org.uk (pandora.armlinux.org.uk [78.32.30.218]) by imf08.hostedemail.com (Postfix) with ESMTP id 504BD160016 for ; Wed, 20 Mar 2024 08:45:40 +0000 (UTC) Authentication-Results: imf08.hostedemail.com; dkim=pass header.d=armlinux.org.uk header.s=pandora-2019 header.b=iLrI8c0f; spf=none (imf08.hostedemail.com: domain of "linux+linux-mm=kvack.org@armlinux.org.uk" has no SPF policy when checking 78.32.30.218) smtp.mailfrom="linux+linux-mm=kvack.org@armlinux.org.uk"; dmarc=pass (policy=none) header.from=armlinux.org.uk ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1710924341; h=from:from:sender:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=3aU/Zt9LBTw5WsKLQZ2I68GMh3ulAkeZBoPkMlotUXM=; b=6hubFaJoMdefGpz7tlSNdI3KfwViDrVmnQW6TSS1fgbliEs7sE+8mQ0ogU3CElYuspYzQs thyU3yEQ8MVzYEwSvnIjF+B21Fg3Z3WQS8jRtbmGExYa+tkfJHZRYbjx+lpe9eLiV61rs1 55309/S2jrK3d/ZFROhyP6l9ygYPBKo= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1710924341; a=rsa-sha256; cv=none; b=YsNEQk2ZveWuy1S84/njXlyKyerVlaWT0QJmNjnLId2P3sh4seFJ1CC4iUeD3OZzfCoiGB qnogOy2uTMg+VVV+3cCR0nsYNf7J0E35NdYuG1d8Q7zRtZP9x1JBVScyxJPCjdRbDcaILr ovxrQS3JdK+LYfXgItYbu3blMmJHlEw= ARC-Authentication-Results: i=1; imf08.hostedemail.com; dkim=pass header.d=armlinux.org.uk header.s=pandora-2019 header.b=iLrI8c0f; spf=none (imf08.hostedemail.com: domain of "linux+linux-mm=kvack.org@armlinux.org.uk" has no SPF policy when checking 78.32.30.218) smtp.mailfrom="linux+linux-mm=kvack.org@armlinux.org.uk"; dmarc=pass (policy=none) header.from=armlinux.org.uk DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=armlinux.org.uk; s=pandora-2019; h=Sender:In-Reply-To:Content-Type: MIME-Version:References:Message-ID:Subject:Cc:To:From:Date:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Id: List-Help:List-Unsubscribe:List-Subscribe:List-Post:List-Owner:List-Archive; bh=3aU/Zt9LBTw5WsKLQZ2I68GMh3ulAkeZBoPkMlotUXM=; b=iLrI8c0fhZUzbZMNPnBFFo0hkC sITZeKCIgpryXsWbtNZL0XdnM2Oeiw0KZy3I0O7dc3B87ZvETPmV4o7hnEDI2eindh7ioRIK+alP+ 7iMneRZKG4/kfW6gn4UaA0F/XG7I6Ery9m0X8/gYwAZ902Z1NWscuKUSd4x3Sj+PYcPuhZ5I0F48j Dy7iAgrXvIA0BSJZTyeiv6iGKtXLBke54Ue1BVY0kUzSr6wtNvaZ2ljy/Y/WI8iEKaOf2rlVu2FjE Zv8nmVqOI/dlm+rF+BywbpVqlnU4Ibv6Z931EwXehpI9y55m4TwilusEHRCf8RBUO4y8iPRKL21KH owW+QkiA==; Received: from shell.armlinux.org.uk ([fd8f:7570:feb6:1:5054:ff:fe00:4ec]:55760) by pandora.armlinux.org.uk with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.96) (envelope-from ) id 1rmrZO-0006HK-2m; Wed, 20 Mar 2024 08:45:10 +0000 Received: from linux by shell.armlinux.org.uk with local (Exim 4.94.2) (envelope-from ) id 1rmrZH-0002Wj-PF; Wed, 20 Mar 2024 08:45:03 +0000 Date: Wed, 20 Mar 2024 08:45:03 +0000 From: "Russell King (Oracle)" To: Jiangfeng Xiao Cc: arnd@arndb.de, keescook@chromium.org, haibo.li@mediatek.com, angelogioacchino.delregno@collabora.com, amergnat@baylibre.com, akpm@linux-foundation.org, dave.hansen@linux.intel.com, douzhaolei@huawei.com, gustavoars@kernel.org, jpoimboe@kernel.org, kepler.chenxin@huawei.com, kirill.shutemov@linux.intel.com, linux-hardening@vger.kernel.org, linux-kernel@vger.kernel.org, linux-mm@kvack.org, linux-arm-kernel@lists.infradead.org, nixiaoming@huawei.com, peterz@infradead.org, wangbing6@huawei.com, wangfangpeng1@huawei.com, jannh@google.com, willy@infradead.org, David.Laight@aculab.com Subject: Re: [PATCH v2] ARM: unwind: improve unwinders for noreturn case Message-ID: References: <1709516385-7778-1-git-send-email-xiaojiangfeng@huawei.com> <1710906278-23851-1-git-send-email-xiaojiangfeng@huawei.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1710906278-23851-1-git-send-email-xiaojiangfeng@huawei.com> X-Rspamd-Queue-Id: 504BD160016 X-Rspam-User: X-Rspamd-Server: rspam11 X-Stat-Signature: 976o9pt76kjh1ozx84npukh7oncoi8mk X-HE-Tag: 1710924340-95199 X-HE-Meta: U2FsdGVkX18vAUkhONIqlqFkzxXRQjWJOXaiEOVMvAsJp3dlVhAOU1Mwp4elzeQymwrT2H9xmNab2McnaD2l9uXocW5ci9uCLiwEqBmaIrRABZbiPpgvVSy4Q4Mrq16gqr7L7aZg1tWQNbhIy7mOi8g31rfZH4EfLXeUo1a++Q3b5zZDWfEvFCkvhn512lFwB3l2noJw2h4swK57QuXAglo2muEzWL5d6wme9X+GFYizvydp+bs9fH8MHAI6WWuwd0cGw7yLN6nlqxx8b01QQfAAhxNwOI2rNjp02SVEDShLmn0WdQwGy5UVkP2wsFvauGzzr3/5fr3l+LTIsBHUeAw34U9u2EXeQBU8Ry4P0sW1jfSOasxq+lIEZ7tQiy9MoxVnSfVMZdYtPkDNH3yLpjAi4cPmbWCpuzyxS6cXDPnEBaY41VslkRMqYMxWkUIHNaSALYH2PuR8MeFUjcObTaY033igzo32FJuxVIeNFt4yC0QvPXOf8fO9FWeIe3+Ucu87QIpnFhWIQOY7FaE0vZ+Y6uUjpsTS3BUv8M5ZspYUaM96dTnU/RX3Iby1oH6W6heYyo3rS2nHMw7gLUIkAq1ndr3tkCf2QNm5NCnJEf8KkHov98LI7Ei9n2AVJZ1YE7Y+Xla34djkZPHP6uhgJnQm8eL6xwacnvgIN7MtfXqPmFL8E0lbiiiWW5lr5+wlENJ/bRnIhVmoryKoYkSf9p3n0Ufu+1uVICWb83uYHofLBfvohEKiXGXaMbVCjvb7w1qDFNjC3X+hn+5JiuiMiPJfCgryj99a/vABQfBDrOertkkElo1jZLe+uoZFYPxat9hCibcAgx/wYHBAgssMII+ET6DTpf8uWSBrW/kugZN8G45C2G7lAT+3uLzh63U3MMcoAXPjoxy0jpG95XYWNbWeVGHxNE7lJ1zuMXvWpkgYBc3lk0AH5tG5Q+OTkAdLY38gKb1osx67f459SVn hZVS2h09 Q4x0+TW7Rec/DUALYCLfNm+m1Qoe4kzCT+zK1Wra6zw7zvyZupuhTB6+hO99jlqo5pRVz+EQLC5n+uLM= X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: On Wed, Mar 20, 2024 at 11:44:38AM +0800, Jiangfeng Xiao wrote: > This is an off-by-one bug which is common in unwinders, > due to the fact that the address on the stack points > to the return address rather than the call address. > > So, for example, when the last instruction of a function > is a function call (e.g., to a noreturn function), it can > cause the unwinder to incorrectly try to unwind from > the function after the callee. > > foo: > ... > bl bar > ... end of function and thus next function ... > > which results in LR pointing into the next function. > > Fixed this by subtracting 1 from frmae->pc in the call frame > (but not exception frames) like ORC on x86 does. The reason that I'm not accepting this patch is because the above says that it fixes it by subtracting 1 from the PC value, but the patch is *way* more complicated than that and there's no explanation why. For example, the following are unexplained: - Why do we always need ex_frame - What is the purpose of the change in format string for the display of backtraces > > Refer to the unwind_next_frame function in the unwind_orc.c > > Suggested-by: Josh Poimboeuf > Link: https://lkml.kernel.org/lkml/20240305175846.qnyiru7uaa7itqba@treble/ > Signed-off-by: Jiangfeng Xiao > --- > ChangeLog v1->v2 > - stay printk("%s...", loglvl, ...) > --- > arch/arm/include/asm/stacktrace.h | 4 ---- > arch/arm/kernel/stacktrace.c | 2 -- > arch/arm/kernel/traps.c | 4 ++-- > arch/arm/kernel/unwind.c | 18 +++++++++++++++--- > 4 files changed, 17 insertions(+), 11 deletions(-) > > diff --git a/arch/arm/include/asm/stacktrace.h b/arch/arm/include/asm/stacktrace.h > index 360f0d2..07e4c16 100644 > --- a/arch/arm/include/asm/stacktrace.h > +++ b/arch/arm/include/asm/stacktrace.h > @@ -21,9 +21,7 @@ struct stackframe { > struct llist_node *kr_cur; > struct task_struct *tsk; > #endif > -#ifdef CONFIG_UNWINDER_FRAME_POINTER > bool ex_frame; > -#endif > }; > > static __always_inline > @@ -37,9 +35,7 @@ void arm_get_current_stackframe(struct pt_regs *regs, struct stackframe *frame) > frame->kr_cur = NULL; > frame->tsk = current; > #endif > -#ifdef CONFIG_UNWINDER_FRAME_POINTER > frame->ex_frame = in_entry_text(frame->pc); > -#endif > } > > extern int unwind_frame(struct stackframe *frame); > diff --git a/arch/arm/kernel/stacktrace.c b/arch/arm/kernel/stacktrace.c > index 620aa82..1abd4f9 100644 > --- a/arch/arm/kernel/stacktrace.c > +++ b/arch/arm/kernel/stacktrace.c > @@ -154,9 +154,7 @@ static void start_stack_trace(struct stackframe *frame, struct task_struct *task > frame->kr_cur = NULL; > frame->tsk = task; > #endif > -#ifdef CONFIG_UNWINDER_FRAME_POINTER > frame->ex_frame = in_entry_text(frame->pc); > -#endif > } > > void arch_stack_walk(stack_trace_consume_fn consume_entry, void *cookie, > diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c > index 3bad79d..46a5b1e 100644 > --- a/arch/arm/kernel/traps.c > +++ b/arch/arm/kernel/traps.c > @@ -84,10 +84,10 @@ void dump_backtrace_entry(unsigned long where, unsigned long from, > printk("%sFunction entered at [<%08lx>] from [<%08lx>]\n", > loglvl, where, from); > #elif defined CONFIG_BACKTRACE_VERBOSE > - printk("%s[<%08lx>] (%ps) from [<%08lx>] (%pS)\n", > + printk("%s[<%08lx>] (%ps) from [<%08lx>] (%pB)\n", > loglvl, where, (void *)where, from, (void *)from); > #else > - printk("%s %ps from %pS\n", loglvl, (void *)where, (void *)from); > + printk("%s %ps from %pB\n", loglvl, (void *)where, (void *)from); > #endif > > if (in_entry_text(from) && end <= ALIGN(frame, THREAD_SIZE)) > diff --git a/arch/arm/kernel/unwind.c b/arch/arm/kernel/unwind.c > index 9d21921..f2baf92 100644 > --- a/arch/arm/kernel/unwind.c > +++ b/arch/arm/kernel/unwind.c > @@ -30,6 +30,7 @@ > #include > #include > > +#include > #include > #include > #include > @@ -416,8 +417,14 @@ int unwind_frame(struct stackframe *frame) > > pr_debug("%s(pc = %08lx lr = %08lx sp = %08lx)\n", __func__, > frame->pc, frame->lr, frame->sp); > - > - idx = unwind_find_idx(frame->pc); > + /* > + * For a call frame (as opposed to a exception frame), when the last > + * instruction of a function is a function call (e.g., to a noreturn > + * function), it can cause the unwinder incorrectly try to unwind > + * from the function after the callee, fixed this by subtracting 1 > + * from frame->pc in the call frame like ORC on x86 does. > + */ > + idx = unwind_find_idx(frame->ex_frame ? frame->pc : frame->pc - 1); > if (!idx) { > if (frame->pc && kernel_text_address(frame->pc)) { > if (in_module_plt(frame->pc) && frame->pc != frame->lr) { > @@ -427,6 +434,7 @@ int unwind_frame(struct stackframe *frame) > * the state of the stack or the register file > */ > frame->pc = frame->lr; > + frame->ex_frame = in_entry_text(frame->pc); > return URC_OK; > } > pr_warn("unwind: Index not found %08lx\n", frame->pc); > @@ -454,6 +462,7 @@ int unwind_frame(struct stackframe *frame) > if (frame->pc == frame->lr) > return -URC_FAILURE; > frame->pc = frame->lr; > + frame->ex_frame = in_entry_text(frame->pc); > return URC_OK; > } else if ((idx->insn & 0x80000000) == 0) > /* prel31 to the unwind table */ > @@ -515,6 +524,7 @@ int unwind_frame(struct stackframe *frame) > frame->lr = ctrl.vrs[LR]; > frame->pc = ctrl.vrs[PC]; > frame->lr_addr = ctrl.lr_addr; > + frame->ex_frame = in_entry_text(frame->pc); > > return URC_OK; > } > @@ -544,6 +554,7 @@ void unwind_backtrace(struct pt_regs *regs, struct task_struct *tsk, > */ > here: > frame.pc = (unsigned long)&&here; > + frame.ex_frame = false; > } else { > /* task blocked in __switch_to */ > frame.fp = thread_saved_fp(tsk); > @@ -554,11 +565,12 @@ void unwind_backtrace(struct pt_regs *regs, struct task_struct *tsk, > */ > frame.lr = 0; > frame.pc = thread_saved_pc(tsk); > + frame.ex_frame = false; > } > > while (1) { > int urc; > - unsigned long where = frame.pc; > + unsigned long where = frame.ex_frame ? frame.pc : frame.pc - 1; > > urc = unwind_frame(&frame); > if (urc < 0) > -- > 1.8.5.6 > > -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!