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 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 smtp.lore.kernel.org (Postfix) with ESMTPS id 5B006ECAAD5 for ; Fri, 2 Sep 2022 14:48:48 +0000 (UTC) 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:From:References:Cc:To: Subject:MIME-Version:Date:Message-ID:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=rwptXn8gojX66ZRRWW2eQlRh/rDTieeDOAsxF/3zpi0=; b=Tqk4CTAX1jVI/m Sm8P36Rn+URKh5aLQXFr5ln90JDeqXfyXkkOjSuysaAqjOx5zKcIJHtKWYbtfi3bMA0d5hX+R+cZF RRy/LXFnF1CH4TCjcDIQljb9prHrd1eCeWFyvdoOzZcRxmJq8vqrBjIMQro6yOElehjldOpUyzNgB LsFil6zdnwvuSgJn32xrGCOOx0mDftH/K+Iiw0Y8Si7QQqli5gKAHmKrLIyDyWQvdkj2KZb4DFcUe mfNLrMuQn2/uCGLG5Ki7LkQdJxJkuufYrSj1Ja+cg7B1uPSqqUXMvVNWtGz/RtETVO418GM6Pglgw 3kG6rbdhnFGm56//3h+w==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1oU7xP-005k5t-Uh; Fri, 02 Sep 2022 14:47:44 +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 1oU7xJ-005jyW-6o for linux-arm-kernel@lists.infradead.org; Fri, 02 Sep 2022 14:47:39 +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 3BFBFD6E; Fri, 2 Sep 2022 07:47:38 -0700 (PDT) Received: from [10.57.45.3] (unknown [10.57.45.3]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 046503F766; Fri, 2 Sep 2022 07:47:29 -0700 (PDT) Message-ID: Date: Fri, 2 Sep 2022 15:47:27 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.11.0 Subject: Re: [PATCH v3 1/7] arm64: mte: Fix/clarify the PG_mte_tagged semantics Content-Language: en-GB To: Peter Collingbourne , linux-arm-kernel@lists.infradead.org, kvmarm@lists.cs.columbia.edu Cc: Catalin Marinas , Cornelia Huck , Will Deacon , Marc Zyngier , Evgenii Stepanov , kvm@vger.kernel.org, Vincenzo Frascino References: <20220810193033.1090251-1-pcc@google.com> <20220810193033.1090251-2-pcc@google.com> From: Steven Price In-Reply-To: <20220810193033.1090251-2-pcc@google.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220902_074737_426645_C24380C0 X-CRM114-Status: GOOD ( 40.32 ) 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 10/08/2022 20:30, Peter Collingbourne wrote: > From: Catalin Marinas > > Currently the PG_mte_tagged page flag mostly means the page contains > valid tags and it should be set after the tags have been cleared or > restored. However, in mte_sync_tags() it is set before setting the tags > to avoid, in theory, a race with concurrent mprotect(PROT_MTE) for > shared pages. However, a concurrent mprotect(PROT_MTE) with a copy on > write in another thread can cause the new page to have stale tags. > Similarly, tag reading via ptrace() can read stale tags of the > PG_mte_tagged flag is set before actually clearing/restoring the tags. > > Fix the PG_mte_tagged semantics so that it is only set after the tags > have been cleared or restored. This is safe for swap restoring into a > MAP_SHARED or CoW page since the core code takes the page lock. Add two > functions to test and set the PG_mte_tagged flag with acquire and > release semantics. The downside is that concurrent mprotect(PROT_MTE) on > a MAP_SHARED page may cause tag loss. This is already the case for KVM > guests if a VMM changes the page protection while the guest triggers a > user_mem_abort(). > > Signed-off-by: Catalin Marinas > Cc: Will Deacon > Cc: Marc Zyngier > Cc: Steven Price > Cc: Peter Collingbourne Reviewed-by: Steven Price > --- > v3: > - fix build with CONFIG_ARM64_MTE disabled > > arch/arm64/include/asm/mte.h | 30 ++++++++++++++++++++++++++++++ > arch/arm64/include/asm/pgtable.h | 2 +- > arch/arm64/kernel/cpufeature.c | 4 +++- > arch/arm64/kernel/elfcore.c | 2 +- > arch/arm64/kernel/hibernate.c | 2 +- > arch/arm64/kernel/mte.c | 12 +++++++----- > arch/arm64/kvm/guest.c | 4 ++-- > arch/arm64/kvm/mmu.c | 4 ++-- > arch/arm64/mm/copypage.c | 4 ++-- > arch/arm64/mm/fault.c | 2 +- > arch/arm64/mm/mteswap.c | 2 +- > 11 files changed, 51 insertions(+), 17 deletions(-) > > diff --git a/arch/arm64/include/asm/mte.h b/arch/arm64/include/asm/mte.h > index aa523591a44e..46618c575eac 100644 > --- a/arch/arm64/include/asm/mte.h > +++ b/arch/arm64/include/asm/mte.h > @@ -37,6 +37,29 @@ void mte_free_tag_storage(char *storage); > /* track which pages have valid allocation tags */ > #define PG_mte_tagged PG_arch_2 > > +static inline void set_page_mte_tagged(struct page *page) > +{ > + /* > + * Ensure that the tags written prior to this function are visible > + * before the page flags update. > + */ > + smp_wmb(); > + set_bit(PG_mte_tagged, &page->flags); > +} > + > +static inline bool page_mte_tagged(struct page *page) > +{ > + bool ret = test_bit(PG_mte_tagged, &page->flags); > + > + /* > + * If the page is tagged, ensure ordering with a likely subsequent > + * read of the tags. > + */ > + if (ret) > + smp_rmb(); > + return ret; > +} > + > void mte_zero_clear_page_tags(void *addr); > void mte_sync_tags(pte_t old_pte, pte_t pte); > void mte_copy_page_tags(void *kto, const void *kfrom); > @@ -54,6 +77,13 @@ size_t mte_probe_user_range(const char __user *uaddr, size_t size); > /* unused if !CONFIG_ARM64_MTE, silence the compiler */ > #define PG_mte_tagged 0 > > +static inline void set_page_mte_tagged(struct page *page) > +{ > +} > +static inline bool page_mte_tagged(struct page *page) > +{ > + return false; > +} > static inline void mte_zero_clear_page_tags(void *addr) > { > } > diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h > index b5df82aa99e6..82719fa42c0e 100644 > --- a/arch/arm64/include/asm/pgtable.h > +++ b/arch/arm64/include/asm/pgtable.h > @@ -1050,7 +1050,7 @@ static inline void arch_swap_invalidate_area(int type) > static inline void arch_swap_restore(swp_entry_t entry, struct folio *folio) > { > if (system_supports_mte() && mte_restore_tags(entry, &folio->page)) > - set_bit(PG_mte_tagged, &folio->flags); > + set_page_mte_tagged(&folio->page); > } > > #endif /* CONFIG_ARM64_MTE */ > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c > index 907401e4fffb..562c301bbf15 100644 > --- a/arch/arm64/kernel/cpufeature.c > +++ b/arch/arm64/kernel/cpufeature.c > @@ -2037,8 +2037,10 @@ static void cpu_enable_mte(struct arm64_cpu_capabilities const *cap) > * Clear the tags in the zero page. This needs to be done via the > * linear map which has the Tagged attribute. > */ > - if (!test_and_set_bit(PG_mte_tagged, &ZERO_PAGE(0)->flags)) > + if (!page_mte_tagged(ZERO_PAGE(0))) { > mte_clear_page_tags(lm_alias(empty_zero_page)); > + set_page_mte_tagged(ZERO_PAGE(0)); > + } > > kasan_init_hw_tags_cpu(); > } > diff --git a/arch/arm64/kernel/elfcore.c b/arch/arm64/kernel/elfcore.c > index 98d67444a5b6..f91bb1572d22 100644 > --- a/arch/arm64/kernel/elfcore.c > +++ b/arch/arm64/kernel/elfcore.c > @@ -47,7 +47,7 @@ static int mte_dump_tag_range(struct coredump_params *cprm, > * Pages mapped in user space as !pte_access_permitted() (e.g. > * PROT_EXEC only) may not have the PG_mte_tagged flag set. > */ > - if (!test_bit(PG_mte_tagged, &page->flags)) { > + if (!page_mte_tagged(page)) { > put_page(page); > dump_skip(cprm, MTE_PAGE_TAG_STORAGE); > continue; > diff --git a/arch/arm64/kernel/hibernate.c b/arch/arm64/kernel/hibernate.c > index af5df48ba915..788597a6b6a2 100644 > --- a/arch/arm64/kernel/hibernate.c > +++ b/arch/arm64/kernel/hibernate.c > @@ -271,7 +271,7 @@ static int swsusp_mte_save_tags(void) > if (!page) > continue; > > - if (!test_bit(PG_mte_tagged, &page->flags)) > + if (!page_mte_tagged(page)) > continue; > > ret = save_tags(page, pfn); > diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c > index b2b730233274..2287316639f3 100644 > --- a/arch/arm64/kernel/mte.c > +++ b/arch/arm64/kernel/mte.c > @@ -41,14 +41,17 @@ static void mte_sync_page_tags(struct page *page, pte_t old_pte, > if (check_swap && is_swap_pte(old_pte)) { > swp_entry_t entry = pte_to_swp_entry(old_pte); > > - if (!non_swap_entry(entry) && mte_restore_tags(entry, page)) > + if (!non_swap_entry(entry) && mte_restore_tags(entry, page)) { > + set_page_mte_tagged(page); > return; > + } > } > > if (!pte_is_tagged) > return; > > mte_clear_page_tags(page_address(page)); > + set_page_mte_tagged(page); > } > > void mte_sync_tags(pte_t old_pte, pte_t pte) > @@ -64,7 +67,7 @@ void mte_sync_tags(pte_t old_pte, pte_t pte) > > /* if PG_mte_tagged is set, tags have already been initialised */ > for (i = 0; i < nr_pages; i++, page++) { > - if (!test_and_set_bit(PG_mte_tagged, &page->flags)) > + if (!page_mte_tagged(page)) > mte_sync_page_tags(page, old_pte, check_swap, > pte_is_tagged); > } > @@ -91,8 +94,7 @@ int memcmp_pages(struct page *page1, struct page *page2) > * pages is tagged, set_pte_at() may zero or change the tags of the > * other page via mte_sync_tags(). > */ > - if (test_bit(PG_mte_tagged, &page1->flags) || > - test_bit(PG_mte_tagged, &page2->flags)) > + if (page_mte_tagged(page1) || page_mte_tagged(page2)) > return addr1 != addr2; > > return ret; > @@ -398,7 +400,7 @@ static int __access_remote_tags(struct mm_struct *mm, unsigned long addr, > put_page(page); > break; > } > - WARN_ON_ONCE(!test_bit(PG_mte_tagged, &page->flags)); > + WARN_ON_ONCE(!page_mte_tagged(page)); > > /* limit access to the end of the page */ > offset = offset_in_page(addr); > diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c > index 8c607199cad1..3b04e69006b4 100644 > --- a/arch/arm64/kvm/guest.c > +++ b/arch/arm64/kvm/guest.c > @@ -1058,7 +1058,7 @@ long kvm_vm_ioctl_mte_copy_tags(struct kvm *kvm, > maddr = page_address(page); > > if (!write) { > - if (test_bit(PG_mte_tagged, &page->flags)) > + if (page_mte_tagged(page)) > num_tags = mte_copy_tags_to_user(tags, maddr, > MTE_GRANULES_PER_PAGE); > else > @@ -1075,7 +1075,7 @@ long kvm_vm_ioctl_mte_copy_tags(struct kvm *kvm, > * completed fully > */ > if (num_tags == MTE_GRANULES_PER_PAGE) > - set_bit(PG_mte_tagged, &page->flags); > + set_page_mte_tagged(page); > > kvm_release_pfn_dirty(pfn); > } > diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c > index 87f1cd0df36e..c9012707f69c 100644 > --- a/arch/arm64/kvm/mmu.c > +++ b/arch/arm64/kvm/mmu.c > @@ -1075,9 +1075,9 @@ static int sanitise_mte_tags(struct kvm *kvm, kvm_pfn_t pfn, > return -EFAULT; > > for (i = 0; i < nr_pages; i++, page++) { > - if (!test_bit(PG_mte_tagged, &page->flags)) { > + if (!page_mte_tagged(page)) { > mte_clear_page_tags(page_address(page)); > - set_bit(PG_mte_tagged, &page->flags); > + set_page_mte_tagged(page); > } > } > > diff --git a/arch/arm64/mm/copypage.c b/arch/arm64/mm/copypage.c > index 24913271e898..4223389b6180 100644 > --- a/arch/arm64/mm/copypage.c > +++ b/arch/arm64/mm/copypage.c > @@ -21,9 +21,9 @@ void copy_highpage(struct page *to, struct page *from) > > copy_page(kto, kfrom); > > - if (system_supports_mte() && test_bit(PG_mte_tagged, &from->flags)) { > - set_bit(PG_mte_tagged, &to->flags); > + if (system_supports_mte() && page_mte_tagged(from)) { > mte_copy_page_tags(kto, kfrom); > + set_page_mte_tagged(to); > } > } > EXPORT_SYMBOL(copy_highpage); > diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c > index c33f1fad2745..d095bfa16771 100644 > --- a/arch/arm64/mm/fault.c > +++ b/arch/arm64/mm/fault.c > @@ -931,5 +931,5 @@ struct page *alloc_zeroed_user_highpage_movable(struct vm_area_struct *vma, > void tag_clear_highpage(struct page *page) > { > mte_zero_clear_page_tags(page_address(page)); > - set_bit(PG_mte_tagged, &page->flags); > + set_page_mte_tagged(page); > } > diff --git a/arch/arm64/mm/mteswap.c b/arch/arm64/mm/mteswap.c > index 4334dec93bd4..a78c1db23c68 100644 > --- a/arch/arm64/mm/mteswap.c > +++ b/arch/arm64/mm/mteswap.c > @@ -24,7 +24,7 @@ int mte_save_tags(struct page *page) > { > void *tag_storage, *ret; > > - if (!test_bit(PG_mte_tagged, &page->flags)) > + if (!page_mte_tagged(page)) > return 0; > > tag_storage = mte_allocate_tag_storage(); _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel