Re: [PATCH 0/3] kasan: Fix ordering between MTE tag colouring and page->flags

From: Andrey Konovalov <andreyknvl@gmail.com>
To: Catalin Marinas <catalin.marinas@arm.com>
Cc: Andrey Ryabinin <ryabinin.a.a@gmail.com>,
	Will Deacon <will@kernel.org>,
	 Vincenzo Frascino <vincenzo.frascino@arm.com>,
	Peter Collingbourne <pcc@google.com>,
	 kasan-dev <kasan-dev@googlegroups.com>,
	 Linux Memory Management List <linux-mm@kvack.org>,
	Linux ARM <linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH 0/3] kasan: Fix ordering between MTE tag colouring and page->flags
Date: Tue, 31 May 2022 19:16:03 +0200	[thread overview]
Message-ID: <CA+fCnZfZv3Q-2Xj1X6wEN13R6kJQbE_3EgzYMyZ8ZmWogf28Ww@mail.gmail.com> (raw)
In-Reply-To: <Yo9xbkyfj0zkc1qa@arm.com>

On Thu, May 26, 2022 at 2:24 PM Catalin Marinas <catalin.marinas@arm.com> wrote:
>
> On Wed, May 25, 2022 at 07:41:08PM +0200, Andrey Konovalov wrote:
> > On Wed, May 25, 2022 at 5:45 PM Catalin Marinas <catalin.marinas@arm.com> wrote:
> > > > Adding __GFP_SKIP_KASAN_UNPOISON makes sense, but we still need to
> > > > reset the tag in page->flags.
> > >
> > > My thought was to reset the tag in page->flags based on 'unpoison'
> > > alone without any extra flags. We use this flag for vmalloc() pages but
> > > it seems we don't reset the page tags (as we do via
> > > kasan_poison_slab()).
> >
> > I just realized that we already have __GFP_ZEROTAGS that initializes
> > both in-memory and page->flags tags.
>
> IIUC it only zeroes the tags and skips the unpoisoning but
> page_kasan_tag() remains unchanged.

No, it does page_kasan_tag_reset() via tag_clear_highpage(). At least,
currently.

> > Currently only used for user
> > pages allocated via alloc_zeroed_user_highpage_movable(). Perhaps we
> > can add this flag to GFP_HIGHUSER_MOVABLE?
>
> I wouldn't add __GFP_ZEROTAGS to GFP_HIGHUSER_MOVABLE as we only need it
> if the page is mapped with PROT_MTE. Clearing a page without tags may be
> marginally faster.

Ah, right. We need a dedicated flag for PROT_MTE allocations.

> > We'll also need to change the behavior of __GFP_ZEROTAGS to work even
> > when GFP_ZERO is not set, but this doesn't seem to be a problem.
>
> Why? We'd get unnecessary tag zeroing. We have these cases for
> anonymous, private pages:
>
> 1. Zeroed page allocation without PROT_MTE: we need GFP_ZERO and
>    page_kasan_tag_reset() in case of later mprotect(PROT_MTE).
>
> 2. Zeroed page allocation with PROT_MTE: we need GFP_ZERO,
>    __GFP_ZEROTAGS and page_kasan_tag_reset().
>
> 3. CoW page allocation without PROT_MTE: copy data and we only need
>    page_kasan_tag_reset() in case of later mprotect(PROT_MTE).
>
> 4. CoW page allocation with PROT_MTE: copy data and tags together with
>    page_kasan_tag_reset().
>
> So basically we always need page_kasan_tag_reset() for pages mapped in
> user space even if they are not PROT_MTE, in case of a later
> mprotect(PROT_MTE). For (1), (3) and (4) we don't need to zero the tags.
> For (1) maybe we could do it as part of data zeroing (subject to some
> benchmarks) but for (3) and (4) they'd be overridden by the copy anyway.

Ack.

> > And, at this point, we can probably combine __GFP_ZEROTAGS with
> > __GFP_SKIP_KASAN_POISON, as they both would target user pages.
>
> For user pages, I think we should skip unpoisoning as well. We can keep
> unpoisoning around but if we end up calling page_kasan_tag_reset(),
> there's not much value, at least in page_address() accesses since the
> pointer would match all tags. That's unless you want to detect other
> stray pointers to such pages but we already skip the poisoning on free,
> so it doesn't seem to be a use-case.

Skipping unpoisoning makes sense.

> If we skip unpoisoning (not just poisoning as we already do) for user
> pages, we should reset the tags in page->flags. Whether __GFP_ZEROTAGS
> is passed is complementary, depending on the reason for allocation.

[...]

> Currently if __GFP_ZEROTAGS is passed, the unpoisoning is skipped but I
> think we should have just added __GFP_SKIP_KASAN_UNPOISON instead and
> not add a new argument to should_skip_kasan_unpoison(). If we decide to
> always skip unpoisoning, something like below on top of the vanilla
> kernel:

[...]

> With the above, we can wire up page_kasan_tag_reset() to the
> __GFP_SKIP_KASAN_UNPOISON check without any additional flags.

This would make __GFP_SKIP_KASAN_UNPOISON do two logically unrelated
things: skip setting memory tags and reset page tags. This seems
weird.

I think it makes more sense to split __GFP_ZEROTAGS into
__GFP_ZERO_MEMORY_TAGS and __GFP_ZERO_PAGE_TAGS: the first one does
tag_clear_highpage() without page_kasan_tag_reset() and the second one
does page_kasan_tag_reset() in post_alloc_hook(). Then, add
__GFP_ZERO_PAGE_TAGS to GFP_HIGHUSER_MOVABLE along with
__GFP_SKIP_KASAN_UNPOISON and __GFP_SKIP_KASAN_POISON. And replace
__GFP_ZEROTAGS with __GFP_ZERO_MEMORY_TAGS in
alloc_zeroed_user_highpage_movable().

An a alternative approach that would reduce the number of GFP flags,
we could extend your suggestion and pre-combining all standalone
MTE-related GFP flags based on their use cases:

__GFP_KASAN_VMALLOC == essentially __GFP_SKIP_KASAN_UNPOISON | __GFP_SKIP_ZERO
__GFP_MTE_USER == essentially __GFP_ZERO_PAGE_TAGS |
__GFP_SKIP_KASAN_UNPOISON | __GFP_SKIP_KASAN_POISON
__GFP_MTE_USER_ZERO == essentially __GFP_ZERO_MEMORY_TAGS

Then we would only need 3 flags instead of 5.

However, this seems to be unaligned with the idea that __GFP flags
should enable/disable a single piece of functionality. So I like the
first approach better.

What do you think?

Thanks!