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=-3.8 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=no 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 D76EAC388F9 for ; Wed, 11 Nov 2020 07:48:57 +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 3DFF0206B5 for ; Wed, 11 Nov 2020 07:48: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="E8KRRwr4" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 3DFF0206B5 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=xmission.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:Subject:MIME-Version:Message-ID:In-Reply-To:Date: References:To:From:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=sEhfmBcw0Vsp8mmK1KqsDvL+rRc3i7pcqdvmVcMkCiE=; b=E8KRRwr4iXsnvTZGQld7diqH+ LwSO7s1HYVCCrx8UmO7qhpYu3mz2k66DAkVPLR66yLpLqQl68GBUZLzqnYlgspjBlxvAKeMWzGLua 15vrFtUvDdpZDpoKDk5s1xg5o+QXCKKwQ9TANxI1S9VtY/lH/+OYnyEosXMaYFwDNN+anmYKSVfsf VxRfZ2tBkZ75gpN3xTSLK4issRm2n1H+7w5VTo2q+pNE6L2kFHeoGCnyPXFBQRx01m5zM6AYtNVy9 klqpKcGuLvfjqGH/nfoGzHRxFR1Utzrj4+XKJStV7qFPd28wbVbpSxVPQn91StBsO0RdirtBmmcxw IX1CwwGlQ==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kckq4-0007Uk-Na; Wed, 11 Nov 2020 07:46:44 +0000 Received: from out01.mta.xmission.com ([166.70.13.231]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1kckpu-0007Ta-Ou for linux-arm-kernel@lists.infradead.org; Wed, 11 Nov 2020 07:46:36 +0000 Received: from in02.mta.xmission.com ([166.70.13.52]) by out01.mta.xmission.com with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.93) (envelope-from ) id 1kckpF-003IjL-Nw; Wed, 11 Nov 2020 00:45:53 -0700 Received: from ip68-227-160-95.om.om.cox.net ([68.227.160.95] helo=x220.xmission.com) by in02.mta.xmission.com with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.93) (envelope-from ) id 1kckpE-00EXHX-2o; Wed, 11 Nov 2020 00:45:53 -0700 From: ebiederm@xmission.com (Eric W. Biederman) To: Peter Collingbourne References: <0ce3d90b5d6a4457b2fe3b0582f61fab70b17dfc.1604523707.git.pcc@google.com> <87eel2ypy3.fsf@x220.int.ebiederm.org> <87blg5tfdt.fsf@x220.int.ebiederm.org> Date: Wed, 11 Nov 2020 01:45:41 -0600 In-Reply-To: (Peter Collingbourne's message of "Tue, 10 Nov 2020 14:06:21 -0800") Message-ID: <875z6cs5ei.fsf@x220.int.ebiederm.org> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) MIME-Version: 1.0 X-XM-SPF: eid=1kckpE-00EXHX-2o; ; ; mid=<875z6cs5ei.fsf@x220.int.ebiederm.org>; ; ; hst=in02.mta.xmission.com; ; ; ip=68.227.160.95; ; ; frm=ebiederm@xmission.com; ; ; spf=neutral X-XM-AID: U2FsdGVkX19cDOuE1rjQCEJ2Ebzd5s8TEXbhokN7ixM= X-SA-Exim-Connect-IP: 68.227.160.95 X-SA-Exim-Mail-From: ebiederm@xmission.com Subject: Re: [PATCH v14 8/8] arm64: expose FAR_EL1 tag bits in siginfo X-SA-Exim-Version: 4.2.1 (built Sat, 08 Feb 2020 21:53:50 +0000) X-SA-Exim-Scanned: Yes (on in02.mta.xmission.com) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20201111_024634_941648_6E52B099 X-CRM114-Status: GOOD ( 55.24 ) 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: Catalin Marinas , Helge Deller , Kevin Brodsky , Oleg Nesterov , Linux API , "James E.J. Bottomley" , Kostya Serebryany , Linux ARM , Andrey Konovalov , David Spickett , Vincenzo Frascino , Will Deacon , Dave Martin , Evgenii Stepanov , 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 Peter Collingbourne writes: > On Tue, Nov 10, 2020 at 7:12 AM Eric W. Biederman wrote: >> >> Peter Collingbourne writes: >> >> > On Mon, Nov 9, 2020 at 5:13 PM Eric W. Biederman wrote: >> >> >> >> Peter Collingbourne writes: >> >> >> >> > 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 >> >> > 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_faultflags to allow userspace to determine whether >> >> > the values in the fields are valid. >> >> >> >> I think I am missing some things: >> >> >> >> Today it is documented that the tag bits are cleared, and so we can't >> >> use the highbits to hold the tag bits by default. >> >> >> >> Why do you need to deliver which tag bits are valid? That feels like an >> >> implementation detail that is needed to setup the tag bits. It feels >> >> like it would be constant per process. So I don't understand why the >> >> siginfo needs to report information the process should already have. >> > >> > It isn't constant as it may vary depending on the specific type of >> > fault being delivered. For example on arm64 the architecture only >> > provides us with bits 56-59 of the tag for tag check faults, while all >> > other data aborts also provide bits 60-63. Now although the user >> > program may distinguish the two cases by checking the si_code, we >> > would also like to allow future architecture extensions to provide >> > bits 60-63 for tag check faults as well and allow distinguishing >> > between "bits 60-63 were zero" and "bits 60-63 were unknown" (which is >> > important when providing error reports). >> >> Does that mean that bits 60-63 are effectively unusable as tag bits >> if the tag check fault won't report them? > > The hardware doesn't support tag checking on bits 60-63, only on bits > 56-59, so in terms of hardware enforced memory tag checking they can't > be used as tag bits. But they are still subject to address tagging aka > top-byte-ignore so they could be used by userspace for other purposes > (e.g. if the allocator adds entropy to all 8 bits instead of just bits > 56-59 they could in theory be used to allow better diagnostics with > more precise matching of an invalid access to a previous allocation). > >> If you can use bits 60-63 as tag bits how does that work if they aren't >> reported? > > It still works but we're limited in how many allocations we can match > accesses to (16 vs 256). > >> > I suppose that you could imagine that, if the "bits 60-63 available" >> > extension ever comes to pass, it could be exposed via a bit in >> > getauxval(AT_HWCAP2) (or maybe something like AT_HWCAP3 depending on >> > how long it takes to arrive) and that would provide a way for >> > userspace to know which bits are valid. But it seems like allowing >> > this to vary per signal is relatively cheap, allows the information to >> > be made available trivially architecture independently and keeps our >> > options open for the future (because we don't know if some future >> > architecture will actually make this a per-signal attribute). >> > >> > That being said, maybe we could solve this problem at the point at >> > which we do encounter such an architecture though. >> >> My goal would be to extend things to the minimum extent necessary to >> handle today's reality well. Usually architectures evolve in >> unanticipated directions so simple and straight forward usually wins for >> handling future evolution. As there is simply less old baggage to carry >> around. >> >> However I don't understand if reporting the valid bits on a signal by >> signal basis has a real advantage today or not. If it helps today we >> will find room for the field. > > Okay, that makes sense. With the architecture as specified today I > don't think we would need it, since you can write a function that > operates on the siginfo and tells you what the si_addr_tag_bits_mask > would be without it. > >> >> Want prevents adding a sigaction sa_flag SA_EXPOSE_TABITS that when set >> >> causes the high bits to be set, and when clear (the default) will have >> >> the signal delivery code clear those bits. >> >> >> >> That should be enough for code that wants the tag bits to ask for them. >> >> As userspace would need to be updated to get the new bits >> >> >> >> Even if you have chained handlers. The chaining mechanism would need to >> >> be updated and it could call the aware handlers first then clear the tag >> >> bits and call the rest of the handlers. >> >> >> >> It feels like always passing the tag bits in the address and then >> >> clearing them in the copy to userspace if the signal handler is >> >> not ready for them would be easier to maintain. >> > >> > I think that approach might work. Although it may make life harder for >> > callers of ptrace(PTRACE_SETSIGINFO) since they may need to know the >> > value of the bit in order to prepare a correct siginfo structure, if >> > we can reasonably expect them to always be delivering an exact copy of >> > a signal that was received before then maybe that is okay. >> >> I think we can reasonably expect callers of PTRACE_SETSIGINFO to be able >> to either deal the full reality of what is going on, or to only generate >> signals that they fully understand. >> >> Other than the use by CRIU it is a debugging facility and it is not >> expected for ordinary usage. The non-CRIU use case would really seem to >> be what happens if I inject arbitrary signal X into process Y. For that >> you need the ability to inject an arbitrary signal. >> >> My real sense with PTRACE_SETSIGINFO is that if we wind up with a >> regression we can deal with it then. > > Okay, that works for me. > >> > Assuming that this is an alternative to introducing >> > si_addr_tag_bits_mask, the userspace code would need to use the flag >> > bit support detection protocol for SA_EXPOSE_TAGBITS in order to be >> > able to distinguish between "no bits valid" and "some bits valid", and >> > then use an architecture-specific mechanism to determine exactly which >> > bits are valid. Is that okay for a generic feature? >> >> Unless I am mistaken setting the bits is already architecture specific >> so having some architecture specific code in there should not be a big >> problem. >> >> But I really don't understand the arm case well enough to know if we can >> get away without si_addr_tag_bits_mask, and in turn without the flags >> field that indicates the si_addr_tag_bits_mask is present. >> >> So I am asking questions so I can understand just what we get from >> si_addr_tag_bits_mask. > > If we allow the derivation of the mask to be architecture-specific > (which I'd be comfortable with) then I don't think we need it. What we > would end up with is: > > - The tag bits mask is constant and architecturally defined (in > arm64's case it would be 0xff << 56). > - The valid tag bits mask specifies which bits of the tag bits mask in > the fault address are propagated to si_addr. It is defined in an > architecture-specific way based on the signal handler's > SA_EXPOSE_TAGBITS bit, the contents of siginfo and possibly global > information such as getauxval(AT_HWCAP*). > - on arm64 this would currently be defined as: > - if SA_EXPOSE_TAGBITS is not set then the mask is 0xff << 56 for > SIGTRAP/TRAP_BRKPT, 0 otherwise > - if SA_EXPOSE_TAGBITS is set then the mask is 0xf << 56 for > SIGSEGV/SEGV_MTESERR, 0xff << 56 otherwise > - If SA_EXPOSE_TAGBITS is set then the bits in the tag bits mask that > are not also in the valid tag bits mask have an undefined value (this > would e.g. allow future expansion of arm64 to expose bits 60-63 on tag > check faults). > - If the kernel does not support SA_EXPOSE_TAGBITS (as determined > using the flag bit support detection protocol) then the behavior is as > if SA_EXPOSE_TAGBITS is not set. > > So I think I'd be fine with dropping it but let me experiment with the > new approach so that I can confirm that it's practical on Android and > I'll get back to you. Sounds like a plan. Eric _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel