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=-12.2 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,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 2E1BBC433DF for ; Wed, 26 Aug 2020 15:33:58 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (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 EBC432078D for ; Wed, 26 Aug 2020 15:33:57 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="Pp7137JK" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org EBC432078D 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+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=merlin.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=pUmw4UVdMQSx/aS9taXgPEiOHjOASy6+Wcqja2vMly8=; b=Pp7137JKx0eD5FrPydkfNLobI U9GgmVsgS5HGXgIJzc2jzaKWsCxJ4w5q5hmRRDl6gHCerF+1pyd5lDoWMVTwgYmEyXkdBBITWfMmJ gaIj7LP9YZf4xCekZKSLj1gtbIqKAburIpWBsn+osX1oNnEVCXJO6/larw0ec12Vc/auQw1F0+0xV pNvzpLcp/PQeXO2QQzN0nTmHIavOf8Ns1kLT/uleUKkSyTXo2xiWD8bUtjG7D9PCpeYH7BZoybLgc 76B3WhqMbVj6HI+cbJR2YyH76t+2305LWcY9lscpl6QMeyPCeei2YDHTfsh7gnPvJ6l3ID99q4iwJ No3Qh/naA==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kAxPb-0005A0-Vh; Wed, 26 Aug 2020 15:32:32 +0000 Received: from foss.arm.com ([217.140.110.172]) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kAxPZ-00058v-1j for linux-arm-kernel@lists.infradead.org; Wed, 26 Aug 2020 15:32:30 +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 6317B31B; Wed, 26 Aug 2020 08:32:25 -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 81B6E3F68F; Wed, 26 Aug 2020 08:32:23 -0700 (PDT) Date: Wed, 26 Aug 2020 16:32:21 +0100 From: Dave Martin To: Peter Collingbourne Subject: Re: [PATCH v9 6/6] arm64: expose FAR_EL1 tag bits in siginfo Message-ID: <20200826153218.GT6642@arm.com> References: <9df0de08df310052df01d63bc8bddc5dd71c2bdb.1597720138.git.pcc@google.com> <20200819155650.GI6642@arm.com> <20200824142348.GN6642@arm.com> <20200825150204.GS6642@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-20200826_113229_218582_3A4C501C X-CRM114-Status: GOOD ( 51.53 ) 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: Parisc List , Catalin Marinas , Kevin Brodsky , Oleg Nesterov , Evgenii Stepanov , "James E.J. Bottomley" , Kostya Serebryany , "Eric W. Biederman" , Andrey Konovalov , David Spickett , 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+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Tue, Aug 25, 2020 at 03:06:39PM -0700, Peter Collingbourne wrote: > On Tue, Aug 25, 2020 at 8:02 AM Dave Martin wrote: > > > > On Mon, Aug 24, 2020 at 07:18:19PM -0700, Peter Collingbourne wrote: > > > On Mon, Aug 24, 2020 at 7:23 AM Dave Martin wrote: > > > > > > > > On Wed, Aug 19, 2020 at 06:49:01PM -0700, Peter Collingbourne wrote: > > > > > On Wed, Aug 19, 2020 at 8:56 AM Dave Martin wrote: > > > > > > > > > > > > On Mon, Aug 17, 2020 at 08:33:51PM -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 new pair of > > > > > > > union fields in siginfo._sigfault, and store the tag bits of FAR_EL1 > > > > > > > there, together with a mask specifying which bits are valid. > > > > > > > > > > > > > > A flag is added to si_xflags to allow userspace to determine whether > > > > > > > the values in the fields are valid. > > > > > > > > > > > > > > [1] http://clang.llvm.org/docs/HardwareAssistedAddressSanitizerDesign.html > > > > > > > > > > > > > > Signed-off-by: Peter Collingbourne > > > > > > > --- > > > > > > > > [...] > > > > > > > > > > > diff --git a/kernel/signal.c b/kernel/signal.c > > > > > > > index 72182eed1b8d..1f1e42adc57d 100644 > > > > > > > --- a/kernel/signal.c > > > > > > > +++ b/kernel/signal.c > > > > > > > > [...] > > > > > > > > > > > @@ -1706,7 +1722,9 @@ int send_sig_fault(int sig, int code, void __user *addr > > > > > > > return send_sig_info(info.si_signo, &info, t); > > > > > > > } > > > > > > > > > > > > > > -int force_sig_mceerr(int code, void __user *addr, short lsb) > > > > > > > +int force_sig_mceerr(int code, void __user *addr, short lsb, > > > > > > > + unsigned long addr_ignored_bits, > > > > > > > + unsigned long addr_ignored_bits_mask) > > > > > > > > > > > > Rather than having to pass these extra arguments all over the place, can > > > > > > we just pass the far for addr, and have an arch-specific hook for > > > > > > splitting the ignored from non-ignored bits. For now at least, I expect > > > > > > that ignored bits mask to be constant for a given platform and kernel. > > > > > > > > > > That sounds like a good idea. I think that for MTE we will want to > > > > > make it conditional on the si_code (so SEGV_MTESERR would get 0xf << > > > > > 56 while everything else would get 0xff << 56) so I can hook that up > > > > > at the same time. > > > > > > > > OK, I think that's reasonable. > > > > > > > > Mind you, we seem to have 3 kinds of bits here, so I'm starting to > > > > wonder whether this is really sufficient: > > > > > > > > 1) address bits used in the faulted access > > > > 2) attribute/permission bits used in the faulted access > > > > 3) unavailable bits. > > > > > > > > si_addr contains (1). > > > > > > > > si_addr_ignored_bits contains (2). > > > > > > > > The tag bits from non-MTE faults seem to fall under case (3). Code that > > > > looks for these bits in si_addr_ignored_bits won't find them. > > > > > > I'm reasonably sure that the tag bits are available for non-MTE > > > faults. From https://developer.arm.com/docs/ddi0595/h/aarch64-system-registers/far_el1 > > > : > > > "For a Data Abort or Watchpoint exception, if address tagging is > > > enabled for the address accessed by the data access that caused the > > > exception, then this field includes the tag." > > > > Right, but I wonder whether it would still be good idea to have a way to > > tell userspace which bits are valid. > > I'm a bit confused by this. si_addr_ignored_bits_mask is exactly the > mechanism for telling userspace which bits are valid. Or maybe you're > arguing that we should consider *not* having the mask of valid bits in > siginfo? > > > Collecting and synchronising all the correct information for reporting a > > fault is notoriously easy to mess up in the implementation, and > > misreporting of the tag bits might be regarded as a tolerable fail. > > It really depends. Imagine that a future change to the architecture > exposes bits 60-63 in FAR_EL1 in tag fault errors (we have a number of > ideas for how to use these bits to distinguish between different > use-after-frees in error reports). It would be nice for userspace to > be able to distinguish between the situation where bits 60-63 are 0 > and the situation where the bits are unknown, in order to avoid > producing an incorrect/misleading report. > > > We also don't get tag bits for prefetch aborts (which may be reported > > via SIGSEGV). Arguably the architecture doesn't allow a nonzero tag > > (BR etc. likely just throw those bits on the floor). But it might be > > nice to be explicit about this. > > If we view the PC as being a 64-bit value where the architecture does > not allow setting bits 56-63, I think it would be correct to claim > that addresses derived from the PC have bits 56-63 clear. > > > Other architectures may also have other reasons why the additional bits > > are sometimes available, sometimes not. > > If this is the case for an architecture, it can always report the bits > to be unavailable until it can figure out in which cases the bits are > available. > > > > This language applies to non-tag-check-fault data aborts but is > > > superseded by the following paragraph for tag check faults: > > > "For a synchronous Tag Check Fault abort, bits[63:60] are UNKNOWN." > > > > Right, so in this case we should squash those bits and not report them > > in the mask. Currently are you implying that these are address bits, > > because you exclude them from si_addr_ignored_mask? > > My intent was that these are implied to be unavailable bits, as they > are not set in the architecturally-defined si_addr mask ~(0xff << 56) > nor in si_addr_ignored_mask. OK, I think part of my confusion here is coming from the "ignored_bits" naming. These really aren't ignored, but rather they are meaningful -- that's why you're implementing this extension. True, they're ignored for addressing purposes (i.e., these bits can never distinguish a memory location from a second, distinct, memory location). So for backwards compatibility we mask them out from si_addr. In the interests of moving on to reviewing the actual code and avoiding the discussion from getting too fragmented, can I suggest that you don't reply in detail to this: I'll reflect, and then reiterate my comments on the v10/v11 thread if I still have concerns. I may not get to it this week -- apologies for that -- but if I can start looking at the updated series today I will. Cheers ---Dave _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel