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 lists.ozlabs.org (lists.ozlabs.org [112.213.38.117]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id AC70AC43334 for ; Sat, 18 Jun 2022 12:53:30 +0000 (UTC) Received: from boromir.ozlabs.org (localhost [IPv6:::1]) by lists.ozlabs.org (Postfix) with ESMTP id 4LQG6x1kt1z3cjG for ; Sat, 18 Jun 2022 22:53:29 +1000 (AEST) Authentication-Results: lists.ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=arm.com (client-ip=217.140.110.172; helo=foss.arm.com; envelope-from=mark.rutland@arm.com; receiver=) Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by lists.ozlabs.org (Postfix) with ESMTP id 4LQG6T3JmGz300x for ; Sat, 18 Jun 2022 22:53:04 +1000 (AEST) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id B43C5113E; Sat, 18 Jun 2022 05:52:33 -0700 (PDT) Received: from FVFF77S0Q05N (unknown [10.57.35.139]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 1F5253F792; Sat, 18 Jun 2022 05:52:28 -0700 (PDT) Date: Sat, 18 Jun 2022 13:52:24 +0100 From: Mark Rutland To: Tong Tiangen Subject: Re: [PATCH -next v5 6/8] arm64: add support for machine check error safe Message-ID: References: <20220528065056.1034168-1-tongtiangen@huawei.com> <20220528065056.1034168-7-tongtiangen@huawei.com> <4aa8b109-c79b-8da0-db89-85ca128f1049@huawei.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <4aa8b109-c79b-8da0-db89-85ca128f1049@huawei.com> X-BeenThere: linuxppc-dev@lists.ozlabs.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Kefeng Wang , Dave Hansen , linux-mm@kvack.org, Paul Mackerras , Guohanjun , Will Deacon , "H . Peter Anvin" , x86@kernel.org, Ingo Molnar , Catalin Marinas , Xie XiuQi , Borislav Petkov , Alexander Viro , Thomas Gleixner , linux-arm-kernel@lists.infradead.org, Robin Murphy , linux-kernel@vger.kernel.org, James Morse , Andrew Morton , linuxppc-dev@lists.ozlabs.org Errors-To: linuxppc-dev-bounces+linuxppc-dev=archiver.kernel.org@lists.ozlabs.org Sender: "Linuxppc-dev" On Sat, Jun 18, 2022 at 05:18:55PM +0800, Tong Tiangen wrote: > 在 2022/6/17 16:55, Mark Rutland 写道: > > On Sat, May 28, 2022 at 06:50:54AM +0000, Tong Tiangen wrote: > > > +static bool arm64_do_kernel_sea(unsigned long addr, unsigned int esr, > > > + struct pt_regs *regs, int sig, int code) > > > +{ > > > + if (!IS_ENABLED(CONFIG_ARCH_HAS_COPY_MC)) > > > + return false; > > > + > > > + if (user_mode(regs) || !current->mm) > > > + return false; > > > > What's the `!current->mm` check for? > > At first, I considered that only user processes have the opportunity to > recover when they trigger memory error. > > But it seems that this restriction is unreasonable. When the kernel thread > triggers memory error, it can also be recovered. for instance: > > https://lore.kernel.org/linux-mm/20220527190731.322722-1-jiaqiyan@google.com/ > > And i think if(!current->mm) shoud be added below: > > if(!current->mm) { > set_thread_esr(0, esr); > arm64_force_sig_fault(...); > } > return true; Why does 'current->mm' have anything to do with this, though? There can be kernel threads with `current->mm` set in unusual circumstances (and there's a lot of kernel code out there which handles that wrong), so if you want to treat user tasks differently, we should be doing something like checking PF_KTHREAD, or adding something like an is_user_task() helper. [...] > > > + > > > + if (apei_claim_sea(regs) < 0) > > > + return false; > > > + > > > + if (!fixup_exception_mc(regs)) > > > + return false; > > > > I thought we still wanted to signal the task in this case? Or do you expect to > > add that into `fixup_exception_mc()` ? > > Yeah, here return false and will signal to task in do_sea() -> > arm64_notify_die(). I mean when we do the fixup. I thought the idea was to apply the fixup (to stop the kernel from crashing), but still to deliver a fatal signal to the user task since we can't do what the user task asked us to. > > > + > > > + set_thread_esr(0, esr); > > > > Why are we not setting the address? Is that deliberate, or an oversight? > > Here set fault_address to 0, i refer to the logic of arm64_notify_die(). > > void arm64_notify_die(...) > { > if (user_mode(regs)) { > WARN_ON(regs != current_pt_regs()); > current->thread.fault_address = 0; > current->thread.fault_code = err; > > arm64_force_sig_fault(signo, sicode, far, str); > } else { > die(str, regs, err); > } > } > > I don't know exactly why and do you know why arm64_notify_die() did this? :) To be honest, I don't know, and that looks equally suspicious to me. Looking at the git history, that was added in commit: 9141300a5884b57c ("arm64: Provide read/write fault information in compat signal handlers") ... so maybe Catalin recalls why. Perhaps the assumption is just that this will be fatal and so unimportant? ... but in that case the same logic would apply to the ESR value, so it's not clear to me. Mark.