Re: [PATCH 2/3] arm64: mte: handle tags zeroing at page allocation time

From: Catalin Marinas <catalin.marinas@arm.com>
To: Peter Collingbourne <pcc@google.com>
Cc: Andrey Konovalov <andreyknvl@gmail.com>,
	Alexander Potapenko <glider@google.com>,
	Vincenzo Frascino <vincenzo.frascino@arm.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	Evgenii Stepanov <eugenis@google.com>,
	linux-mm@kvack.org, linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH 2/3] arm64: mte: handle tags zeroing at page allocation time
Date: Tue, 11 May 2021 13:53:54 +0100	[thread overview]
Message-ID: <20210511125354.GC9799@arm.com> (raw)
In-Reply-To: <20210511073108.138837-2-pcc@google.com>

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 <pcc@google.com>
> Co-developed-by: Catalin Marinas <catalin.marinas@arm.com>
> Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
> 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 <linux/personality.h> /* for READ_IMPLIES_EXEC */
> +#include <linux/types.h> /* for gfp_t */
>  #include <asm/pgtable-types.h>
>  
>  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