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=-15.3 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=ham 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 21655C433ED for ; Tue, 11 May 2021 12:54:02 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 7073D6191C for ; Tue, 11 May 2021 12:54:01 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 7073D6191C Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=arm.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id C84676B006E; Tue, 11 May 2021 08:54:00 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id C5A466B0071; Tue, 11 May 2021 08:54:00 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id B21ED6B0072; Tue, 11 May 2021 08:54:00 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0212.hostedemail.com [216.40.44.212]) by kanga.kvack.org (Postfix) with ESMTP id 9371A6B006E for ; Tue, 11 May 2021 08:54:00 -0400 (EDT) Received: from smtpin36.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay04.hostedemail.com (Postfix) with ESMTP id 42E7DA8D1 for ; Tue, 11 May 2021 12:54:00 +0000 (UTC) X-FDA: 78128942640.36.C95E2D7 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by imf01.hostedemail.com (Postfix) with ESMTP id 7EEF7500153B for ; Tue, 11 May 2021 12:53:56 +0000 (UTC) Received: by mail.kernel.org (Postfix) with ESMTPSA id 0072661921; Tue, 11 May 2021 12:53:56 +0000 (UTC) Date: Tue, 11 May 2021 13:53:54 +0100 From: Catalin Marinas To: Peter Collingbourne Cc: Andrey Konovalov , Alexander Potapenko , Vincenzo Frascino , Andrew Morton , Evgenii Stepanov , linux-mm@kvack.org, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH 2/3] arm64: mte: handle tags zeroing at page allocation time Message-ID: <20210511125354.GC9799@arm.com> References: <20210511073108.138837-1-pcc@google.com> <20210511073108.138837-2-pcc@google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210511073108.138837-2-pcc@google.com> User-Agent: Mutt/1.10.1 (2018-07-13) X-Rspamd-Queue-Id: 7EEF7500153B Authentication-Results: imf01.hostedemail.com; dkim=none; spf=pass (imf01.hostedemail.com: domain of cmarinas@kernel.org designates 198.145.29.99 as permitted sender) smtp.mailfrom=cmarinas@kernel.org; dmarc=fail reason="SPF not aligned (relaxed), No valid DKIM" header.from=arm.com (policy=none) X-Rspamd-Server: rspam03 X-Stat-Signature: kyzz1isedpbagxi457gy6hodyx8tz9wd Received-SPF: none (kernel.org>: No applicable sender policy available) receiver=imf01; identity=mailfrom; envelope-from=""; helo=mail.kernel.org; client-ip=198.145.29.99 X-HE-DKIM-Result: none/none X-HE-Tag: 1620737636-698113 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: Hi Peter, First of all, could you please add a cover letter to your series (in general) explaining the rationale for the patches, e.g. optimise tag initialisation for user pages? It makes it a lot easier to review if the overall picture is presented in the cover. On Tue, May 11, 2021 at 12:31:07AM -0700, Peter Collingbourne wrote: > Currently, on an anonymous page fault, the kernel allocates a zeroed > page and maps it in user space. If the mapping is tagged (PROT_MTE), > set_pte_at() additionally clears the tags. It is, however, more > efficient to clear the tags at the same time as zeroing the data on > allocation. To avoid clearing the tags on any page (which may not be > mapped as tagged), only do this if the vma flags contain VM_MTE. This > requires introducing a new GFP flag that is used to determine whether > to clear the tags. > > The DC GZVA instruction with a 0 top byte (and 0 tag) requires > top-byte-ignore. Set the TCR_EL1.{TBI1,TBID1} bits irrespective of > whether KASAN_HW is enabled. > > Signed-off-by: Peter Collingbourne > Co-developed-by: Catalin Marinas > Signed-off-by: Catalin Marinas > Link: https://linux-review.googlesource.com/id/Id46dc94e30fe11474f7e54f5d65e7658dbdddb26 This doesn't mention that the patch adds tag clearing on free as well. I'd actually leave this part out for a separate patch. It's not done for tags in current mainline when kasan is disabled, AFAICT. > diff --git a/arch/arm64/include/asm/page.h b/arch/arm64/include/asm/page.h > index 012cffc574e8..a0bcaa5f735e 100644 > --- a/arch/arm64/include/asm/page.h > +++ b/arch/arm64/include/asm/page.h > @@ -13,6 +13,7 @@ > #ifndef __ASSEMBLY__ > > #include /* for READ_IMPLIES_EXEC */ > +#include /* for gfp_t */ > #include > > struct page; > @@ -28,10 +29,16 @@ void copy_user_highpage(struct page *to, struct page *from, > void copy_highpage(struct page *to, struct page *from); > #define __HAVE_ARCH_COPY_HIGHPAGE > > -#define __alloc_zeroed_user_highpage(movableflags, vma, vaddr) \ > - alloc_page_vma(GFP_HIGHUSER | __GFP_ZERO | movableflags, vma, vaddr) > +struct page *__alloc_zeroed_user_highpage(gfp_t movableflags, > + struct vm_area_struct *vma, > + unsigned long vaddr); > #define __HAVE_ARCH_ALLOC_ZEROED_USER_HIGHPAGE > > +#define want_zero_tags_on_free() system_supports_mte() As I said above, unless essential to this patch, please move it to a separate one. Also, do we need this even when the kernel doesn't have kasan_hw? > diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c > index 871c82ab0a30..8127e0c0b8fb 100644 > --- a/arch/arm64/mm/fault.c > +++ b/arch/arm64/mm/fault.c > @@ -921,3 +921,28 @@ void do_debug_exception(unsigned long addr_if_watchpoint, unsigned int esr, > debug_exception_exit(regs); > } > NOKPROBE_SYMBOL(do_debug_exception); > + > +/* > + * Used during anonymous page fault handling. > + */ > +struct page *__alloc_zeroed_user_highpage(gfp_t flags, > + struct vm_area_struct *vma, > + unsigned long vaddr) > +{ > + /* > + * If the page is mapped with PROT_MTE, initialise the tags at the > + * point of allocation and page zeroing as this is usually faster than > + * separate DC ZVA and STGM. > + */ > + if (vma->vm_flags & VM_MTE) > + flags |= __GFP_ZEROTAGS; > + > + return alloc_page_vma(GFP_HIGHUSER | __GFP_ZERO | flags, vma, vaddr); > +} > + > +void tag_clear_highpage(struct page *page) > +{ > + mte_zero_clear_page_tags(page_address(page)); > + page_kasan_tag_reset(page); > + set_bit(PG_mte_tagged, &page->flags); > +} Do we need the page_kasan_tag_reset() here? Maybe we do. Is it because kasan_alloc_pages() is no longer calls kasan_unpoison_pages() below? > diff --git a/mm/kasan/hw_tags.c b/mm/kasan/hw_tags.c > index 45e552cb9172..34362c8d0955 100644 > --- a/mm/kasan/hw_tags.c > +++ b/mm/kasan/hw_tags.c > @@ -242,7 +242,14 @@ void kasan_alloc_pages(struct page *page, unsigned int order, gfp_t flags) > { > bool init = !want_init_on_free() && want_init_on_alloc(flags); > > - kasan_unpoison_pages(page, order, init); > + if (flags & __GFP_ZEROTAGS) { > + int i; > + > + for (i = 0; i != 1 << order; ++i) > + tag_clear_highpage(page + i); > + } else { > + kasan_unpoison_pages(page, order, init); > + } > } > > void kasan_free_pages(struct page *page, unsigned int order) > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > index 6e82a7f6fd6f..7ac0f0721d22 100644 > --- a/mm/page_alloc.c > +++ b/mm/page_alloc.c > @@ -1219,10 +1219,16 @@ static int free_tail_pages_check(struct page *head_page, struct page *page) > return ret; > } > > -static void kernel_init_free_pages(struct page *page, int numpages) > +static void kernel_init_free_pages(struct page *page, int numpages, bool zero_tags) > { > int i; > > + if (zero_tags) { > + for (i = 0; i < numpages; i++) > + tag_clear_highpage(page + i); > + return; > + } > + > /* s390's use of memset() could override KASAN redzones. */ > kasan_disable_current(); > for (i = 0; i < numpages; i++) { This function has another loop calling clear_highpage(). Do we end up zeroing the page twice? > @@ -1314,7 +1320,8 @@ static __always_inline bool free_pages_prepare(struct page *page, > bool init = want_init_on_free(); > > if (init) > - kernel_init_free_pages(page, 1 << order); > + kernel_init_free_pages(page, 1 << order, > + want_zero_tags_on_free()); > if (!skip_kasan_poison) > kasan_poison_pages(page, order, init); > } I think passing 'false' here to kernel_init_free_pages() matches the current mainline. You could make this dependent on kasan_hw being enabled rather than just system_supports_mte(). With kasan_hw disabled, the kernel accesses are not checked anyway, so it's pointless to erase the tags on free. -- Catalin