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@xxxxxxxxxx>
> Co-developed-by: Catalin Marinas <catalin.marinas@xxxxxxx>
> Signed-off-by: Catalin Marinas <catalin.marinas@xxxxxxx>
> 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
