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 CAA52C433E1 for ; Mon, 24 Aug 2020 14:25:24 +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 76EC8206F0 for ; Mon, 24 Aug 2020 14:25:24 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="hwoFPB97" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 76EC8206F0 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=xUZaV8secsclGnCbN98pmTesXlgwtLemLrm+tKD5JZ0=; b=hwoFPB97ncsEdfI8OhxywKVzx 99jVN2M62G+hxtBX15wNmB1SUAM5Cn8FIhVYmTJ7HmcjNym8MYkrLDXf3OAB5znmj+ZfbwRRLEeA9 8Ke8w3Ty7cnIV0Tf0P/wCFIK/OLfsfJEJsrPAiSFOdlhTza9qTVMGcS5IW0FwUbcP2zN5UPmMDl1D SshNoe3AsicgSoQs97nhoVqL2I3nrCoUamhd2+/uRaoqg1ph4dHPGO7yM6AtHGv2xvbm5jLWTnRrm 9RNvLy38WG355S23hc7TLHEwCvdLEwxoHUjdDa2d4xESDFTScFuVqYRCQ4Qg/8PQ/GizE1gOxz7we EHIeJ24dg==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kADOC-0000yj-BT; Mon, 24 Aug 2020 14:24:00 +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 1kADO9-0000yC-Pp for linux-arm-kernel@lists.infradead.org; Mon, 24 Aug 2020 14:23:58 +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 AAE2A1FB; Mon, 24 Aug 2020 07:23:53 -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 C995F3F71F; Mon, 24 Aug 2020 07:23:51 -0700 (PDT) Date: Mon, 24 Aug 2020 15:23:49 +0100 From: Dave Martin To: Peter Collingbourne Subject: Re: [PATCH v9 6/6] arm64: expose FAR_EL1 tag bits in siginfo Message-ID: <20200824142348.GN6642@arm.com> References: <9df0de08df310052df01d63bc8bddc5dd71c2bdb.1597720138.git.pcc@google.com> <20200819155650.GI6642@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-20200824_102358_013215_D24A4604 X-CRM114-Status: GOOD ( 37.41 ) 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 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. So perhaps it would make sense to amend the design a bit. We'd have si_addr = address bits only si_attr = attribute bits only si_attr_mask = mask of valid bits in si_addr Thinking about it, I've just shortened/changed some named and changed the meaning of the mask, so it's very similar to what you already have. The mask of valid bits in si_addr is decided by architectural convention (i.e., ~0xffUL << 56), but we could also expose si_addr_mask = mask of valid bits in si_addr This is a bit redundant though. I think people would already assume that all bits required for classifying the faulting location accurately must already be provided there. > > > > { > > > struct kernel_siginfo info; > > > > > > @@ -1717,7 +1735,9 @@ int force_sig_mceerr(int code, void __user *addr, short lsb) > > > info.si_code = code; > > > info.si_addr = addr; > > > info.si_addr_lsb = lsb; > > > - info.si_xflags = 0; > > > + info.si_xflags = SI_XFLAG_IGNORED_BITS; > > > > Maybe drop the first _, so that this becomes > > > > SIXFLAG_IGNORED_BITS > > > > ? This may help to avoid anyone thinking this might be an si_code > > value. Maybe ..._IGNORED_ADDR_BITS would also help make it clearer what > > this bit is referring to. > > Yes, it should have been spelled the same way as the field name (i.e. > SIXFLAG_ADDR_IGNORED_BITS), that was my mistake. I don't have a strong > opinion on whether to keep the underscore in the middle or not. Fair enough (modulo possible changes above). [...] > > > case SIL_FAULT_BNDERR: > > > to->si_addr = compat_ptr(from->si_addr); > > > @@ -3399,6 +3431,8 @@ static int post_copy_siginfo_from_user32(kernel_siginfo_t *to, > > > to->si_lower = compat_ptr(from->si_lower); > > > to->si_upper = compat_ptr(from->si_upper); > > > to->si_xflags = from->si_xflags; > > > + to->si_addr_ignored_bits = from->si_addr_ignored_bits; > > > + to->si_addr_ignored_bits_mask = from->si_addr_ignored_bits_mask; > > > break; > > > case SIL_FAULT_PKUERR: > > > to->si_addr = compat_ptr(from->si_addr); > > > @@ -3407,6 +3441,8 @@ static int post_copy_siginfo_from_user32(kernel_siginfo_t *to, > > > #endif > > > to->si_pkey = from->si_pkey; > > > to->si_xflags = from->si_xflags; > > > + to->si_addr_ignored_bits = from->si_addr_ignored_bits; > > > + to->si_addr_ignored_bits_mask = from->si_addr_ignored_bits_mask; > > > > Again, can you justify why this is exhaustive? If there any way to > > factor this so as to reduce the number of places we need to hack this > > in? > > Addressed on the other patch. Once I've factored the various > SIL_FAULT* cases there should be only a handful of places requiring > changes. And that looked like a reasonable justification. Cheers ---Dave _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel