From patchwork Fri Dec 20 01:48:53 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Peter Collingbourne X-Patchwork-Id: 11304645 Return-Path: Received: from mail.kernel.org (pdx-korg-mail-1.web.codeaurora.org [172.30.200.123]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 9E686109A for ; Fri, 20 Dec 2019 01:49:26 +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 73B122467E for ; Fri, 20 Dec 2019 01:49:26 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="lwi+gIKv"; dkim=fail reason="signature verification failed" (2048-bit key) header.d=google.com header.i=@google.com header.b="QnD2X8ug" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 73B122467E Authentication-Results: mail.kernel.org; dmarc=fail (p=reject dis=none) header.from=google.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.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:To:From:Subject:References:Mime-Version :Message-Id:In-Reply-To:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=i8WxpjhY5q/zQ7JUmDJQUYMT9H09oRm3IY1t5vC4ssQ=; b=lwi+gIKvPTm6z2 /tE5OSqD7wR88h6qMbBIDIozsIDJpfTVAiNBgFhZBQtW/lxfnr810nleNHQM35qyWM6sVACnFVL2f DtXJNcfX9Jfl6la+ZmVCt+77w99VM7d0y75LKd0EHJ/mztkgg3ZzDy4Q1LrRVmB4foW8xfc2YDLaQ bS0pZ4LbjDj5HSMnAJ8ntr3pkxYOzqnjGSruhGxvqOv4xdrFzeCdaiIPiahXXT6c2A+M5Y8dzPQ+2 X0M0EYUyjpI6mYUL8di7OEJWP5T4Bqu1Zi/aTzo35ee783Xw8w8CHZcRFZK0k3NOaklrk2RoVAoE8 3hpI9qqms8K2mRcwRgAg==; 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 1ii7Pw-0007am-U4; Fri, 20 Dec 2019 01:49:24 +0000 Received: from mail-yb1-xb49.google.com ([2607:f8b0:4864:20::b49]) by bombadil.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1ii7Pt-0007aB-Cx for linux-arm-kernel@lists.infradead.org; Fri, 20 Dec 2019 01:49:23 +0000 Received: by mail-yb1-xb49.google.com with SMTP id j82so5619078ybj.16 for ; Thu, 19 Dec 2019 17:49:20 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=date:in-reply-to:message-id:mime-version:references:subject:from:to :cc:content-transfer-encoding; bh=HZMnRQf2RzyKu0EQDfD1pRubT2qoNtln9gX9VZl7+Lw=; b=QnD2X8ugKAM+uICpb7uuNo+E46l8sDMOx9E2HllXNNfZ3iUHNx7MEf5Cn0GjUwfRTd OKLau0tl6dua5duaimUEqxFDkRJkuWXPc1v3yiTigDeXisZZhWPRv1M51ulShTut74/0 ma4/kaNbkejRaCDbNXrYqFSVIMW6S7OmDHydlpWtXpRIAnu59Prcrx0HfLn2aunMypwl V/jocBvh3oyonQOMr0+IlqC4PufPaZsPxYCPTQvKCuf8MX3m8fwh0Yhq6AU39rgKPAN0 9TRM3nrbL70rMxgLnlQHhKSO+5WtC9JO6YIi9gIZROJcL/Yc3wkGv5PENac6siscxMHp TN9Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:in-reply-to:message-id:mime-version :references:subject:from:to:cc:content-transfer-encoding; bh=HZMnRQf2RzyKu0EQDfD1pRubT2qoNtln9gX9VZl7+Lw=; b=K7U4dMF55OGBcQiUK+FhN70NFJSJE6U/f9VHRrWe/WDUXQ9QmirWim+uXt3xa+pNey Zlsu1aaE8KezU0T22fiJ1JDd6o0rySVk3tKJ97fLiSjln1PvdtHRQt1XJhWBmqHjU62N vJ9saCdSTxRrZsRGcq/phGOFKo5xr3aDfoxAUYlhfwoscqgF6PI39+Brd26zkp3KVEkt V2HpLuaP2Qxs84La01FJBxEUnWMAJZZDhbyh22uVwyz3oh6qR+qiOHO8JUpPt+T1CipP dl5SZ6hhIMXppki2sJ6r4aWXfHtIKYBYeFo3PfoKbwLzCMqyUpp/6sKCuajrrr/PZXsK U6lg== X-Gm-Message-State: APjAAAVYKmjyA9LVQmfXh+aWoOZC1opPGNlKWvuzrffw5vabmiAW87VH lR+IIe9W7XmRUw3c4P0eiuFd058= X-Google-Smtp-Source: APXvYqzIKCMr5RBHVty3PxKkUeb+z/z35I8hFPiVh8D2zq2tXjZ8EsxBxsgexdyHh/iBIMSJ8f2vGqE= X-Received: by 2002:a81:618a:: with SMTP id v132mr9055009ywb.388.1576806560029; Thu, 19 Dec 2019 17:49:20 -0800 (PST) Date: Thu, 19 Dec 2019 17:48:53 -0800 In-Reply-To: Message-Id: <20191220014853.223389-1-pcc@google.com> Mime-Version: 1.0 References: X-Mailer: git-send-email 2.24.1.735.g03f4e72817-goog Subject: [PATCH] arm64: mte: Clear SCTLR_EL1.TCF0 on exec From: Peter Collingbourne To: Catalin Marinas , Evgenii Stepanov , Kostya Serebryany X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20191219_174921_476668_FBF23738 X-CRM114-Status: GOOD ( 22.90 ) X-Spam-Score: -7.7 (-------) X-Spam-Report: SpamAssassin version 3.4.2 on bombadil.infradead.org summary: Content analysis details: (-7.7 points) pts rule name description ---- ---------------------- -------------------------------------------------- -0.0 RCVD_IN_DNSWL_NONE RBL: Sender listed at https://www.dnswl.org/, no trust [2607:f8b0:4864:20:0:0:0:b49 listed in] [list.dnswl.org] 0.0 SPF_HELO_NONE SPF: HELO does not publish an SPF Record -7.5 USER_IN_DEF_DKIM_WL From: address is in the default DKIM white-list -0.0 SPF_PASS SPF: sender matches SPF record -0.1 DKIM_VALID Message has at least one valid DKIM or DK signature 0.1 DKIM_SIGNED Message has a DKIM or DK signature, not necessarily valid -0.1 DKIM_VALID_EF Message has a valid DKIM or DK signature from envelope-from domain -0.1 DKIM_VALID_AU Message has a valid DKIM or DK signature from author's domain -0.0 DKIMWL_WL_MED DKIMwl.org - Medium sender 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: linux-arch@vger.kernel.org, Richard Earnshaw , Will Deacon , Szabolcs Nagy , Marc Zyngier , Kevin Brodsky , linux-mm@kvack.org, Andrey Konovalov , Vincenzo Frascino , Peter Collingbourne , Linux ARM Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org Signed-off-by: Peter Collingbourne --- On Thu, Dec 19, 2019 at 12:32 PM Peter Collingbourne wrote: > > On Wed, Dec 11, 2019 at 10:45 AM Catalin Marinas > wrote: > > +       if (current->thread.sctlr_tcf0 != next->thread.sctlr_tcf0) > > +               update_sctlr_el1_tcf0(next->thread.sctlr_tcf0); > > I don't entirely understand why yet, but I've found that this check is > insufficient for ensuring consistency between SCTLR_EL1.TCF0 and > sctlr_tcf0. In my Android test environment with some processes having > sctlr_tcf0=SCTLR_EL1_TCF0_SYNC and others having sctlr_tcf0=0, I am > seeing intermittent tag failures coming from the sctlr_tcf0=0 > processes. With this patch: > > diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c > index ef3bfa2bf2b1..4e5d02520a51 100644 > --- a/arch/arm64/mm/fault.c > +++ b/arch/arm64/mm/fault.c > @@ -663,6 +663,8 @@ static int do_sea(unsigned long addr, unsigned int > esr, struct pt_regs *regs) >  static int do_tag_check_fault(unsigned long addr, unsigned int esr, >                               struct pt_regs *regs) >  { > +       printk(KERN_ERR "do_tag_check_fault %lx %lx\n", > +              current->thread.sctlr_tcf0, read_sysreg(sctlr_el1)); >         do_bad_area(addr, esr, regs); >         return 0; >  } > > I see dmesg output like this: > > [   15.249216] do_tag_check_fault 0 c60fc64791d > > showing that SCTLR_EL1.TCF0 became inconsistent with sctlr_tcf0. This > patch fixes the problem for me: > > diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c > index fba89c9f070b..fb012f0baa12 100644 > --- a/arch/arm64/kernel/process.c > +++ b/arch/arm64/kernel/process.c > @@ -518,9 +518,7 @@ static void mte_thread_switch(struct task_struct *next) >         if (!system_supports_mte()) >                 return; > > -       /* avoid expensive SCTLR_EL1 accesses if no change */ > -       if (current->thread.sctlr_tcf0 != next->thread.sctlr_tcf0) > -               update_sctlr_el1_tcf0(next->thread.sctlr_tcf0); > +       update_sctlr_el1_tcf0(next->thread.sctlr_tcf0); >         update_gcr_el1_excl(next->thread.gcr_excl); >  } >  #else > @@ -643,15 +641,8 @@ static long set_mte_ctrl(unsigned long arg) >                 return -EINVAL; >         } > > -       /* > -        * mte_thread_switch() checks current->thread.sctlr_tcf0 as an > -        * optimisation. Disable preemption so that it does not see > -        * the variable update before the SCTLR_EL1.TCF0 one. > -        */ > -       preempt_disable(); >         current->thread.sctlr_tcf0 = tcf0; >         update_sctlr_el1_tcf0(tcf0); > -       preempt_enable(); > >         current->thread.gcr_excl = (arg & PR_MTE_EXCL_MASK) >> > PR_MTE_EXCL_SHIFT; >         update_gcr_el1_excl(current->thread.gcr_excl); > > Since sysreg_clear_set only sets the sysreg if it ended up changing, I > wouldn't expect this to cause a significant performance hit unless > just reading SCTLR_EL1 is expensive. That being said, if the > inconsistency is indicative of a deeper problem, we should probably > address that. I tracked it down to the flush_mte_state() function setting sctlr_tcf0 but failing to update SCTLR_EL1.TCF0. With this patch I am not seeing any more inconsistencies. Peter arch/arm64/kernel/process.c | 37 +++++++++++++++++++++---------------- 1 file changed, 21 insertions(+), 16 deletions(-) diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c index fba89c9f070b..07e8e7bd3bec 100644 --- a/arch/arm64/kernel/process.c +++ b/arch/arm64/kernel/process.c @@ -319,6 +319,25 @@ static void flush_tagged_addr_state(void) } #ifdef CONFIG_ARM64_MTE +static void update_sctlr_el1_tcf0(u64 tcf0) +{ + /* no need for ISB since this only affects EL0, implicit with ERET */ + sysreg_clear_set(sctlr_el1, SCTLR_EL1_TCF0_MASK, tcf0); +} + +static void set_sctlr_el1_tcf0(u64 tcf0) +{ + /* + * mte_thread_switch() checks current->thread.sctlr_tcf0 as an + * optimisation. Disable preemption so that it does not see + * the variable update before the SCTLR_EL1.TCF0 one. + */ + preempt_disable(); + current->thread.sctlr_tcf0 = tcf0; + update_sctlr_el1_tcf0(tcf0); + preempt_enable(); +} + static void flush_mte_state(void) { if (!system_supports_mte()) @@ -327,7 +346,7 @@ static void flush_mte_state(void) /* clear any pending asynchronous tag fault */ clear_thread_flag(TIF_MTE_ASYNC_FAULT); /* disable tag checking */ - current->thread.sctlr_tcf0 = 0; + set_sctlr_el1_tcf0(0); } #else static void flush_mte_state(void) @@ -497,12 +516,6 @@ static void ssbs_thread_switch(struct task_struct *next) } #ifdef CONFIG_ARM64_MTE -static void update_sctlr_el1_tcf0(u64 tcf0) -{ - /* no need for ISB since this only affects EL0, implicit with ERET */ - sysreg_clear_set(sctlr_el1, SCTLR_EL1_TCF0_MASK, tcf0); -} - static void update_gcr_el1_excl(u64 excl) { /* @@ -643,15 +656,7 @@ static long set_mte_ctrl(unsigned long arg) return -EINVAL; } - /* - * mte_thread_switch() checks current->thread.sctlr_tcf0 as an - * optimisation. Disable preemption so that it does not see - * the variable update before the SCTLR_EL1.TCF0 one. - */ - preempt_disable(); - current->thread.sctlr_tcf0 = tcf0; - update_sctlr_el1_tcf0(tcf0); - preempt_enable(); + set_sctlr_el1_tcf0(tcf0); current->thread.gcr_excl = (arg & PR_MTE_EXCL_MASK) >> PR_MTE_EXCL_SHIFT; update_gcr_el1_excl(current->thread.gcr_excl);