On Thu, May 26, 2022 at 2:24 PM Catalin Marinas <catalin.marinas@xxxxxxx> 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@xxxxxxx> 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!
