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=-8.4 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,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 12D88C433DF for ; Wed, 13 May 2020 17:27:55 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (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 7A05C2053B for ; Wed, 13 May 2020 17:27:55 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="qS0v5Yqi" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 7A05C2053B Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=arm.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+infradead-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=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject: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=BqcjJDBxcrlJlUWkir17chcLBeY3xdmGEzLkZAnn+ek=; b=qS0v5Yqip3l/sP GK4xHyoGQE9pDy3GIbewk20ytvOd04FNCjc8RX5L7WCUxHLqQyHKg2NVnDA6zc0rNlrDzXDiOR/1r GWdGYRUhkMfT31M+9keDdafhVXxWpMOvZhCfN3XQUA0b76JwAVptPMVyIM8tDjZFsxxg+koTqzujl x4ZG0BHyFbww3uEshoPAYLMDNcfCNxY57HSeBZsBFDZH0J0Gl69EU1Jp6lXUN7f4R4vxlcuOX2jNX eHqhJF0AQ/lF2iKAz9JHaIdx6zZdZBcyGuRpZ1FFtr7qyNgEQral9yJ8FJIOBKqUuXSOdtUFys7WS ACDpIf978w0ULf6Naohg==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1jYvAg-0005TT-6n; Wed, 13 May 2020 17:27:54 +0000 Received: from foss.arm.com ([217.140.110.172]) by bombadil.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1jYvAd-0005T9-4v for linux-arm-kernel@lists.infradead.org; Wed, 13 May 2020 17:27:52 +0000 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 4AA7F30E; Wed, 13 May 2020 10:27:49 -0700 (PDT) Received: from arm.com (usa-sjc-imap-foss1.foss.arm.com [10.121.207.14]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id EE1453F305; Wed, 13 May 2020 10:27:47 -0700 (PDT) Date: Wed, 13 May 2020 18:27:45 +0100 From: Dave Martin To: Peter Collingbourne Subject: Re: [PATCH v3] arm64: Expose original FAR_EL1 value in sigcontext Message-ID: <20200513172745.GX21779@arm.com> References: <20200325174001.234803-1-pcc@google.com> <20200327191915.257116-1-pcc@google.com> <20200504101930.GG30377@arm.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200513_102751_277246_EA3E254A X-CRM114-Status: GOOD ( 35.44 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Andrey Konovalov , Kevin Brodsky , Kostya Serebryany , Evgenii Stepanov , Catalin Marinas , Vincenzo Frascino , Will Deacon , Linux ARM , Richard Henderson Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Thu, May 07, 2020 at 10:55:02AM -0700, Peter Collingbourne wrote: > On Mon, May 4, 2020 at 3:19 AM Dave Martin wrote: > > > > On Fri, Mar 27, 2020 at 12:19:15PM -0700, Peter Collingbourne wrote: > > > The kernel currently clears the tag bits (i.e. bits 56-63) in the fault > > > address exposed via siginfo.si_addr and sigcontext.fault_address. However, > > > the tag bits may be needed by tools in order to accurately diagnose > > > memory errors, such as HWASan [1] or future tools based on the Memory > > > Tagging Extension (MTE). > > > > > > We should not stop clearing these bits in the existing fault address > > > fields, because there may be existing userspace applications that are > > > expecting the tag bits to be cleared. Instead, create a far_context in > > > sigcontext (similar to the existing esr_context), and store the original > > > value of FAR_EL1 (including the tag bits) there. > > > > > > [1] http://clang.llvm.org/docs/HardwareAssistedAddressSanitizerDesign.html > > > > > > Signed-off-by: Peter Collingbourne > > > --- > > > v3: > > > - add documentation to tagged-pointers.rst > > > - update comments in sigcontext.h > > > > > > v2: > > > - revert changes to hw_breakpoint.c > > > - rename set_thread_esr to set_thread_far_esr > > > > > > Documentation/arm64/tagged-pointers.rst | 17 +++++---- > > > arch/arm64/include/asm/exception.h | 2 +- > > > arch/arm64/include/asm/processor.h | 2 +- > > > arch/arm64/include/uapi/asm/sigcontext.h | 21 +++++++---- > > > arch/arm64/kernel/entry-common.c | 2 -- > > > arch/arm64/kernel/signal.c | 20 ++++++++++- > > > arch/arm64/mm/fault.c | 45 ++++++++++++++---------- > > > 7 files changed, 74 insertions(+), 35 deletions(-) > > > > [...] > > > > > diff --git a/arch/arm64/include/uapi/asm/sigcontext.h b/arch/arm64/include/uapi/asm/sigcontext.h > > > index 8b0ebce92427..6782394633cb 100644 > > > --- a/arch/arm64/include/uapi/asm/sigcontext.h > > > +++ b/arch/arm64/include/uapi/asm/sigcontext.h > > > @@ -44,11 +44,12 @@ struct sigcontext { > > > * > > > * 0x210 fpsimd_context > > > * 0x10 esr_context > > > + * 0x10 far_context > > > * 0x8a0 sve_context (vl <= 64) (optional) > > > * 0x20 extra_context (optional) > > > * 0x10 terminator (null _aarch64_ctx) > > > * > > > - * 0x510 (reserved for future allocation) > > > + * 0x500 (reserved for future allocation) > > > * > > > * New records that can exceed this space need to be opt-in for userspace, so > > > * that an expanded signal frame is not generated unexpectedly. The mechanism > > > @@ -94,17 +95,25 @@ struct esr_context { > > > __u64 esr; > > > }; > > > > > > +/* FAR_EL1 context */ > > > +#define FAR_MAGIC 0x46415201 > > > + > > > +struct far_context { > > > + struct _aarch64_ctx head; > > > + __u64 far; > > > +}; > > > + > > > /* > > > * extra_context: describes extra space in the signal frame for > > > * additional structures that don't fit in sigcontext.__reserved[]. > > > * > > > * Note: > > > * > > > - * 1) fpsimd_context, esr_context and extra_context must be placed in > > > - * sigcontext.__reserved[] if present. They cannot be placed in the > > > - * extra space. Any other record can be placed either in the extra > > > - * space or in sigcontext.__reserved[], unless otherwise specified in > > > - * this file. > > > + * 1) fpsimd_context, esr_context, far_context and extra_context must be > > > + * placed in sigcontext.__reserved[] if present. They cannot be placed > > > + * in the extra space. Any other record can be placed either in the > > > + * extra space or in sigcontext.__reserved[], unless otherwise specified > > > + * in this file. > > > > This is for backwards compatibility only. We don't need this constraint > > for any new field, so you can probably leave the paragraph as-is. > > > > Removing this would mean constraint would mean that userspace must be > > prepared to traverse extra_context when looking for far_context. But > > really we want modern userspace to do this anyway, since it reduces > > backwards compatibilty worries when adding more new records in the > > future. > > My original reason for updating this comment was that I figured that > this record was small enough that we could just always include it in > __reserved. > > But thinking about this a bit more, it doesn't seem that just wanting > userspace to read extra_context will guarantee that it will do so. In > practice, it would be easy to write userspace code that works right > now but doesn't read extra_context correctly (either because > extra_context wasn't considered at all, or because the code purporting > to read the record from extra_context contains a latent bug because it > wasn't exercised). Since we may be practically constrained from moving > the record anyway, we might as well document it and allow the > userspace code to be a little simpler. > > I guess one alternative is that we always place this record in > extra_context, which would force userspace to read it correctly. That > has something of the opposite problem (userspace code could be written > to only expect the record in extra_context), but at least we're less > constrained there, and it's more likely that the code would be parsing > __reserved correctly since it would need to do so in order to find > extra_context. > > Anyway, I've reverted the comment change for now in v4, but let me > know what you think. Apologies for the delay in responding -- I think it does make sense to reserve space in __reserved[] for the new record, the the location you suggested for it is sensible. __reserved[] is a scarce resource, and should only be burned on "small" records, but far_context is small. here's another reason too, which is that we don't want to needlessly block new software from using this field without allocating larger stacks -- not least because they just won't, and the problem won't bite them until much later. Hope that helps clarify things. Cheers ---Dave _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel