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 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id F1350C433F5 for ; Mon, 8 Nov 2021 11:03:01 +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 BEF3D61242 for ; Mon, 8 Nov 2021 11:03:01 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org BEF3D61242 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=arm.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc: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=qFBpHWYn0dWlBje9pOf8Z3fvQlv4knPDLyX2HOljLc8=; b=hA9K0wsCYgxzxS FK76IyR4QotQjLm5I4yvzAZpXyvfHtbGYHHFSrbgVL1IMSykpuG/sscgC7NRxG9KuSr2emilsbwcV 2jCJ3LA8/NkviEPlGEKy0ZF2W+xEf1IgQIIKucoa4Faw5DG4hvSlPboZm1v1CW/fCycr17assBe1S cfHqWSXbUYOCvktSHcjDThWQXKj5jNaoHv2R8MW63Hmu4sqmievNvLNwAZVtSXTIYrc/2THTXsTz+ FEspsbyisnr3xeC1qOxV4RiLVsbcvLM1ppAFF4la6EFOTKL058E63Vi8s2D2RwKmPSbo4prfvWYwW ly2NDvGPCBOw/ClI3WgA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1mk2PO-00GDGF-OW; Mon, 08 Nov 2021 11:01:51 +0000 Received: from foss.arm.com ([217.140.110.172]) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1mk2PK-00GDFB-5M for linux-arm-kernel@lists.infradead.org; Mon, 08 Nov 2021 11:01:48 +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 C5E352B; Mon, 8 Nov 2021 03:01:44 -0800 (PST) Received: from FVFF77S0Q05N (unknown [10.57.58.140]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 84A383F800; Mon, 8 Nov 2021 03:01:43 -0800 (PST) Date: Mon, 8 Nov 2021 11:01:39 +0000 From: Mark Rutland To: Peter Collingbourne Cc: Catalin Marinas , Vincenzo Frascino , Will Deacon , Andrey Konovalov , Evgenii Stepanov , Linux ARM Subject: Re: [PATCH] arm64: mte: avoid clearing PSTATE.TCO on entry unless necessary Message-ID: References: <20210929194525.3252555-1-pcc@google.com> <20211007102029.GB45209@C02TD0UTHF1T.local> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20211108_030146_334678_3814A213 X-CRM114-Status: GOOD ( 56.03 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , 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 Fri, Nov 05, 2021 at 01:18:38PM -0700, Peter Collingbourne wrote: > On Thu, Oct 7, 2021 at 3:20 AM Mark Rutland wrote: > > > > On Tue, Oct 05, 2021 at 12:08:58PM -0700, Peter Collingbourne wrote: > > > On Tue, Oct 5, 2021 at 9:46 AM Catalin Marinas wrote: > > > > On Wed, Sep 29, 2021 at 12:45:24PM -0700, Peter Collingbourne wrote: > > > > > diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S > > > > > index 2f69ae43941d..85ead6bbb38e 100644 > > > > > --- a/arch/arm64/kernel/entry.S > > > > > +++ b/arch/arm64/kernel/entry.S > > > > > @@ -269,7 +269,28 @@ alternative_else_nop_endif > > > > > .else > > > > > add x21, sp, #PT_REGS_SIZE > > > > > get_current_task tsk > > > > > + ldr x0, [tsk, THREAD_SCTLR_USER] > > > > > .endif /* \el == 0 */ > > > > > + > > > > > + /* > > > > > + * Re-enable tag checking (TCO set on exception entry). This is only > > > > > + * necessary if MTE is enabled in either the kernel or the userspace > > > > > + * task in synchronous mode. With MTE disabled in the kernel and > > > > > + * disabled or asynchronous in userspace, tag check faults (including in > > > > > + * uaccesses) are not reported, therefore there is no need to re-enable > > > > > + * checking. This is beneficial on microarchitectures where re-enabling > > > > > + * TCO is expensive. > > > > > + */ > > > > > +#ifdef CONFIG_ARM64_MTE > > > > > +alternative_cb kasan_hw_tags_enable > > > > > + tbz x0, #SCTLR_EL1_TCF0_SHIFT, 1f > > > > > +alternative_cb_end > > > > > +alternative_if ARM64_MTE > > > > > + SET_PSTATE_TCO(0) > > > > > +alternative_else_nop_endif > > > > > +1: > > > > > +#endif > > > > > > > > I think we can get here from an interrupt as well. Can we guarantee that > > > > the sctlr_user is valid? We are not always in a user process context. > > > > > > Looking through the code in entry.S carefully it doesn't appear that > > > the tsk pointer is ever used when taking an exception from EL1. The > > > last user appears to have been removed in commit 3d2403fd10a1 ("arm64: > > > uaccess: remove set_fs()"). I just did a quick boot test on a couple > > > of platforms and things seem to work without the "get_current_task > > > tsk" line. > > > > > > So I can't be confident that tsk points to a valid task, but at least > > > it looks like it was the case prior to that commit. > > The EL1 entries unconditionally call enter_from_kernel_mode() which > (at least in some configurations) unconditionally accesses current, so > I'm reasonably confident that we have a valid current pointer here. That's the value in SP_EL0, which is valid so long as we never take an exception from the entry assembly itself. We restore that from __entry_task in the EL0 entry paths, and context-switch it when switching threads. I would like to get rid of `tsk` in the entry assembly, but that requires moving more of that assembly to C. > > > > Maybe only do the above checks if \el == 0, otherwise just bracket it > > > > with kasan_hw_tags_enable. > > > > > > Is it possible for us to do a uaccess inside the EL1 -> EL1 exception > > > handler? That was the one thing I was unsure about and it's why I > > > opted to do the same check regardless of EL. > > > > Today we can do so if we take a PMU IRQ from EL1 and try to do an EL0 > > stack unwind or stack dump, and similar holds for some eBPF stuff. > > > > We also need to ensure that PSTATE.TCO is configured consistently so > > that context-switch works, otherwise where a CPU switches between tasks > > A and B, where A is preempted by an EL1 IRQ, and B is explicitly > > switching via a direct call to schedule(), the state of TCO will not be > > as expected (unless we track this per thread, and handle it in the > > context switch). > > Isn't this already context switched via storing the per-task SPSR? > Otherwise e.g. the TCO controls in __uaccess_disable_tco() and > __uaccess_enable_tco() would have the same problem. That's the case for pre-emptive scheduling off the back of an interrupt, but tasks can call into schedule without an intervening exception, e.g. blocking on something like a mutex. Those blocking primitives aren't permitted to be used within a __uaccess_{disable,enable)_tco() critical section. > > I'd strongly prefer that the state of PSTATE.TCO upon taking an > > exception to EL1 is consistent (by the end of the early entry code), > > regardless of where that was taken from. > > > > Is it easier to manage this from within entry-common.c? > > It doesn't look like we have any common location to add code that runs > on every kernel entry in entry-common.c. I was thinking we could rework the existing mte_check_tfsr_*() callsites into mte_enter_from_{user,kernel}() hooks, which'd allow us to avoid redundant checks for MTE support. > I tried adding one (patch > below), but this ended up leading to a performance regression (versus > baseline) on some of the cores on the hardware that I have access to. How big a regression? On a real workload or a microbenchmark? Is that true for a simple move of the current logic to C, i.e. for an unconditional SET_PSTATE_TCO(0) gated by cpus_have_final_cap(ARM64_MTE) ? Thanks, Mark. > > Peter > > diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c > index 64cfe4a3798f..ad066b09a6b7 100644 > --- a/arch/arm64/kernel/entry-common.c > +++ b/arch/arm64/kernel/entry-common.c > @@ -17,6 +17,23 @@ > #include > #include > > +static void mte_disable_tco_entry(void) > +{ > + /* > + * Re-enable tag checking (TCO set on exception entry). This is only > + * necessary if MTE is enabled in either the kernel or the userspace > + * task in synchronous mode. With MTE disabled in the kernel and > + * disabled or asynchronous in userspace, tag check faults (including in > + * uaccesses) are not reported, therefore there is no need to re-enable > + * checking. This is beneficial on microarchitectures where re-enabling > + * TCO is expensive. > + */ > + if (IS_ENABLED(CONFIG_ARM64_MTE) && > + (kasan_hw_tags_enabled() || > + (current->thread.sctlr_user & (1UL << SCTLR_EL1_TCF0_SHIFT)))) > + asm volatile(SET_PSTATE_TCO(0)); > +} > + > /* > * This is intended to match the logic in irqentry_enter(), handling the kernel > * mode transitions only. > @@ -39,6 +56,7 @@ static void noinstr enter_from_kernel_mode(struct > pt_regs *regs) > trace_hardirqs_off_finish(); > > mte_check_tfsr_entry(); > + mte_disable_tco_entry(); > } > > /* > @@ -235,6 +253,7 @@ asmlinkage void noinstr enter_from_user_mode(void) > CT_WARN_ON(ct_state() != CONTEXT_USER); > user_exit_irqoff(); > trace_hardirqs_off_finish(); > + mte_disable_tco_entry(); > } > > asmlinkage void noinstr exit_to_user_mode(void) > diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S > index 42c404d508ca..8c63279ffea7 100644 > --- a/arch/arm64/kernel/entry.S > +++ b/arch/arm64/kernel/entry.S > @@ -339,13 +339,6 @@ alternative_if ARM64_HAS_IRQ_PRIO_MASKING > msr_s SYS_ICC_PMR_EL1, x20 > alternative_else_nop_endif > > - /* Re-enable tag checking (TCO set on exception entry) */ > -#ifdef CONFIG_ARM64_MTE > -alternative_if ARM64_MTE > - SET_PSTATE_TCO(0) > -alternative_else_nop_endif > -#endif > - > /* > * Registers that may be useful after this macro is invoked: > * _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel