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=-14.4 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_ADSP_CUSTOM_MED,DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_CR_TRAILER,INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED autolearn=unavailable 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 C8FF1C433B4 for ; Tue, 11 May 2021 20:35:25 +0000 (UTC) Received: from desiato.infradead.org (desiato.infradead.org [90.155.92.199]) (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 3565961626 for ; Tue, 11 May 2021 20:35:25 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 3565961626 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+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=desiato.20200630; h=Sender:Content-Transfer-Encoding :Content-Type:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:Cc:To:Subject:Message-ID:Date:From:In-Reply-To: References:MIME-Version:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=gkx6bKC+wDOvJGmv1KMV3EpNmAKE+C6sp5hprZ8VPVA=; b=VTLVGqGhpiuFFZEs5It33fVQF TNrhquDtxRom+Tr2W8EIKd/kLhKmkY7m9k0t3SEvVgQiLpH0RtO340jMZwvhhyVXpQ9zcuIvXj6YT z1FReAByPSKMAYhoD1y9qz8k/rOYxnGYwO7H4HjoMRknLzbFy4fHtXcE4zCO+5XM7fqqc0cY5aENH RKj624R1sMKWlSBZ4qlA3QHo1+XqdRPGhWEgCaAo12LnYNM2zr06olsERsDpLem96Gci+i9ndQNva lJfZIn/XuMi7Nrj0pdofgybljmaQEkf7JVBHwB193AB8m3UK4j+G3clMF9woDmRDIt/HE+VcQmD+m 4dML1st6A==; Received: from localhost ([::1] helo=desiato.infradead.org) by desiato.infradead.org with esmtp (Exim 4.94 #2 (Red Hat Linux)) id 1lgZ4H-001IKG-Rk; Tue, 11 May 2021 20:33:26 +0000 Received: from bombadil.infradead.org ([2607:7c80:54:e::133]) by desiato.infradead.org with esmtps (Exim 4.94 #2 (Red Hat Linux)) id 1lgZ4D-001IJg-7T for linux-arm-kernel@desiato.infradead.org; Tue, 11 May 2021 20:33:21 +0000 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=bombadil.20210309; h=Content-Type:Cc:To:Subject:Message-ID :Date:From:In-Reply-To:References:MIME-Version:Sender:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description; bh=h40frLs8n9UJa9r1NYPALLnPMlmcL80u1XMkItLS6OQ=; b=Yt2qC+t3uV+MyAHe3B2mh9khND dc0WLO1yYfMthrGIQ63VlC/i1mN5v1yAJk5kwGxbHgtTm4YJtBH3ywLaAD4CwpBNa6tpE1pECsPfB C99v379kSlS4rci8rLkFIn1tJCo4ROX+YmNGqQQ1NrJlCu2l49tUsQ0oJ9hd5XIMwWmFr0qNbp53S SoEfU46P38adud8L4S5BZPRtkj9aRI7m/ssEiUO0p14sZtsuo1x0Uc765D+/dgU9VSmqTdp6vKHQD qqSVBKTCyxcQNcG0A3a72wGOUmdPuKs1BS8zw1FB3zxLZo3vjWf50FleFnz13QlpdIDwB1Pm8gSnk 8xAKko7A==; Received: from mail-io1-xd2c.google.com ([2607:f8b0:4864:20::d2c]) by bombadil.infradead.org with esmtps (Exim 4.94 #2 (Red Hat Linux)) id 1lgZ49-009u7w-Qx for linux-arm-kernel@lists.infradead.org; Tue, 11 May 2021 20:33:19 +0000 Received: by mail-io1-xd2c.google.com with SMTP id o21so19066252iow.13 for ; Tue, 11 May 2021 13:33:16 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=h40frLs8n9UJa9r1NYPALLnPMlmcL80u1XMkItLS6OQ=; b=qjamw3isER9qbDI7iVpsOzjg6wFX/SGNHPUXYGx0z8OFU9HhjJNFWrNdt8n9M524SU +V92+yl3L/wvYiCpjvPYIRdP3TfXmW4usDHzvoEWtW450JKmcg8JUnLZCMarwaeMvwlu P9uIy4N2j2XWecIibQ7tMi0I5/06APf5+NDwNNdb2xyzakrYOJID7fm6jSfDaT+XCn6l vnN6nW/dYKvJ46xgjcC7p+6zjSD7XfgE+XFBM5O5QSsmlrSi//QOOQ85uCv+ZM06N54s aIEQx83nz1StYV3U63qdJM3ncx/QA8/s71MVwX6eGALTv5AWQnPwTAF36pFPd3aBLpRp QZOA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=h40frLs8n9UJa9r1NYPALLnPMlmcL80u1XMkItLS6OQ=; b=Sc5ATpAEQQFUbXePLF3YQ1qaor/gdGEdxAYbReRTDKkabv67R1V5TkbWaFudACMAuV BieT7Ro3wAWBvSjGUAWEnWpBrVX5XxxQ7IsnqRJCa6MxCFP9fxKHe5mdjTSORQqXculK 6bWX4RPSdjWwMW2HO2x5aSNG7OwWVDP41kRgS2xdYhad4Q5IPEAkVftMCApxGJv/KLs0 SNZ2VjvesMNciUtWvnQL6EQxg9lkRO7DKMNh03FGCEQnvHnIRg0BF/qBIFBPmfsRLgMk hflbmJFAkKKipf0Au1kxC0IHXZeLvfW8CnpzP6trxkkaDwIE54zTjnkGx2zcI6LOGWSV oWEQ== X-Gm-Message-State: AOAM530O1fhgL1Ula45XI5u9f1EHZfwD3BVkWO2+b8LTG9keEbIc4Z/0 bJ56xFiG92w9nzTYCcusJ7bVvxFpeQjtMFjADAU2ZQ== X-Google-Smtp-Source: ABdhPJzk/U0AW0IEWMkbxMz3eLQF0I9sHCZakUzcUpqySnW3BkYOyB465xG3xbgv/KmEw9istTJgXLChSRGJuh90JKI= X-Received: by 2002:a02:91c1:: with SMTP id s1mr28287329jag.61.1620765195243; Tue, 11 May 2021 13:33:15 -0700 (PDT) MIME-Version: 1.0 References: <20210511073108.138837-1-pcc@google.com> <20210511073108.138837-2-pcc@google.com> <20210511125354.GC9799@arm.com> In-Reply-To: <20210511125354.GC9799@arm.com> From: Peter Collingbourne Date: Tue, 11 May 2021 13:33:03 -0700 Message-ID: Subject: Re: [PATCH 2/3] arm64: mte: handle tags zeroing at page allocation time To: Catalin Marinas Cc: Andrey Konovalov , Alexander Potapenko , Vincenzo Frascino , Andrew Morton , Evgenii Stepanov , Linux Memory Management List , Linux ARM X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210511_133317_919030_87679FD8 X-CRM114-Status: GOOD ( 48.42 ) 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 Tue, May 11, 2021 at 5:54 AM Catalin Marinas wrote: > > 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. Sure. It seems appropriate in cases where the series is doing a number of different things like this series, but maybe not in simpler cases (e.g. one cleanup patch followed by a main patch with a self-explanatory commit message). > 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. The tag clearing on free was thought to be necessary (because clear on free implies no clear on alloc) but, upon further reflection, it isn't. This is because we clear the struct page flags, including PG_mte_tagged, on free, which means that we will set the tags if we later end up needing to reuse the page as a tagged page. This means that clear on free is quite inefficient, but no less efficient than before. As you mentioned, we can leave any improvements there to a separate patch. > > 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. Will do. > Also, do we need this even when the kernel doesn't have kasan_hw? Yes, if we preserved PG_mte_tagged across page free/alloc then this would be needed even without KASAN. > > 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? Yes, otherwise the page tag will be left at an arbitrary (most likely poison) value, which would mean that any kernel-side accesses to these pages would fail (unless userspace happened to tag its memory using the page tag). page_kasan_tag_reset() sets the page tag to the TCMA tag which allows those accesses to succeed. We need to call page_kasan_tag_reset() in mte_sync_page_tags() for similar reasons. It's unrelated to no longer calling kasan_unpoison_pages() because even if we were still calling it the end result would still be that userspace's tags won't necessarily match the kernel's page tag. > > 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? No because we return after the loop that calls tag_clear_highpage(). > > @@ -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. I'll just pass false here as it's unneeded for KASAN. Peter _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel