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: 11304643 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 C137F109A for ; Fri, 20 Dec 2019 01:49:22 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 714062465E for ; Fri, 20 Dec 2019 01:49:22 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b="QnD2X8ug" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 714062465E Authentication-Results: mail.kernel.org; dmarc=fail (p=reject dis=none) header.from=google.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id A5BA58E0187; Thu, 19 Dec 2019 20:49:21 -0500 (EST) Delivered-To: linux-mm-outgoing@kvack.org Received: by kanga.kvack.org (Postfix, from userid 40) id A0C028E0184; Thu, 19 Dec 2019 20:49:21 -0500 (EST) X-Original-To: int-list-linux-mm@kvack.org X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 9215C8E0187; Thu, 19 Dec 2019 20:49:21 -0500 (EST) X-Original-To: linux-mm@kvack.org X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0176.hostedemail.com [216.40.44.176]) by kanga.kvack.org (Postfix) with ESMTP id 7D1848E0184 for ; Thu, 19 Dec 2019 20:49:21 -0500 (EST) Received: from smtpin04.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay05.hostedemail.com (Postfix) with SMTP id 27BAD181AC9B6 for ; Fri, 20 Dec 2019 01:49:21 +0000 (UTC) X-FDA: 76283837322.04.fight94_5199c45a27a40 X-Spam-Summary: 2,0,0,f125323af3e1b7e6,d41d8cd98f00b204,3ocj8xqmkcnci559hh9e7.5hfebgnq-ffdo35d.hk9@flex--pcc.bounces.google.com,:catalin.marinas@arm.com:eugenis@google.com:kcc@google.com:pcc@google.com:linux-arm-kernel@lists.infradead.org:linux-arch@vger.kernel.org:richard.earnshaw@arm.com:szabolcs.nagy@arm.com:maz@kernel.org:kevin.brodsky@arm.com::andreyknvl@google.com:vincenzo.frascino@arm.com:will@kernel.org,RULES_HIT:2:41:69:152:355:379:541:800:960:973:988:989:1260:1277:1311:1313:1314:1345:1359:1437:1513:1515:1516:1518:1521:1535:1593:1594:1605:1606:1730:1747:1777:1792:2393:2553:2559:2562:2689:2692:2895:3138:3139:3140:3141:3142:3152:3865:3866:3867:3868:3870:3871:3872:3874:4120:4250:4321:5007:6119:6261:6653:6742:7903:9036:9592:9969:10004:11026:11232:11473:11658:11783:11914:12043:12291:12296:12297:12438:12555:12663:12683:12895:13868:14096:14097:14394:14659:21080:21444:21525:21627:21990:30003:30054:30070:30090:30091,0,RBL:209.85.161.74:@flex--pcc.bounces.google.com:.lbl8.mail shell.ne X-HE-Tag: fight94_5199c45a27a40 X-Filterd-Recvd-Size: 9531 Received: from mail-yw1-f74.google.com (mail-yw1-f74.google.com [209.85.161.74]) by imf04.hostedemail.com (Postfix) with ESMTP for ; Fri, 20 Dec 2019 01:49:20 +0000 (UTC) Received: by mail-yw1-f74.google.com with SMTP id j137so5545850ywa.8 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=olTrCWNrJMcFRs2iu2/MkwBblePbvO1CqZF7tKhN/0H0rt5jrtPztip8xylU9qp8D5 nJD7bWCRTrca15zDNe/iXybEQR0wY53Lf9D5QNLO6XS4sBEhZrHnCLJv2z0/0Se5f40t HIaSEszmd4hsD4Do/6IQnPRZANe0qsq0T3I0N/2TizpFBLkF3FLeNCA9wAABEHul2ekm N+yOHjXIUgGR5kvH9cYtP3++FnG6e7dkSHHheaTKHRO6kwmmxk8kLe4+XlyOimiJFykI IeThnlL7+OcLAvEd8IANpQV2FLy6kwqrfyHQVCSZECYa4JXUHtBH575ZRCo8KW2/O9aN NnUg== X-Gm-Message-State: APjAAAVRbm4kTtGZk/MXVc9+u7ZN8Ev9LflqDcZwlW82z/OBEemkalVc KAgGzzxo7yXsckfmo1QhHT2+X3c= 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 Cc: Peter Collingbourne , Linux ARM , linux-arch@vger.kernel.org, Richard Earnshaw , Szabolcs Nagy , Marc Zyngier , Kevin Brodsky , linux-mm@kvack.org, Andrey Konovalov , Vincenzo Frascino , Will Deacon X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: 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);