kasan: bugfix and cleanup
Subsystems (7)
v4 cover kasan: bugfix and cleanup
Hi,
This patch series reverts a cleanup patch that turned out to introduce a bug, and does some cleanup of its own by removing some flags that I realized were redundant while investigating the bug.
Rebased onto linux-next at akpm's request.
Peter
Peter Collingbourne (2):
Revert "kasan: drop skip_kasan_poison variable in free_pages_prepare"
kasan: remove PG_skip_kasan_poison flag
include/linux/gfp_types.h | 30 ++++++------ include/linux/page-flags.h | 9 ---- include/trace/events/mmflags.h | 13 +----- mm/kasan/hw_tags.c | 2 +- mm/page_alloc.c | 84 +++++++++++++--------------------- mm/vmalloc.c | 2 +- 6 files changed, 49 insertions(+), 91 deletions(-)
Signature · 1 line
2.40.0.rc1.284.g88254d51c5-goog
PATCH v4 1/2 Revert "kasan: drop skip_kasan_poison variable in free_pages_prepare"
This reverts commit 487a32ec24be819e747af8c2ab0d5c515508086a.
The should_skip_kasan_poison() function reads the PG_skip_kasan_poison flag from page->flags. However, this line of code in free_pages_prepare():
page->flags &= ~PAGE_FLAGS_CHECK_AT_PREP;
clears most of page->flags, including PG_skip_kasan_poison, before calling should_skip_kasan_poison(), which meant that it would never return true as a result of the page flag being set. Therefore, fix the code to call should_skip_kasan_poison() before clearing the flags, as we were doing before the reverted patch.
This fixes a measurable performance regression introduced in the reverted commit, where munmap() takes longer than intended if HW tags KASAN is supported and enabled at runtime. Without this patch, we see a single-digit percentage performance regression in a particular mmap()-heavy benchmark when enabling HW tags KASAN, and with the patch, there is no statistically significant performance impact when enabling HW tags KASAN.
--- mm/page_alloc.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
mm/page_alloc.c +2 -1
@@ -1413,6 +1413,7 @@ static __always_inline bool free_pages_prepare(struct page *page, unsigned int order, fpi_t fpi_flags) { int bad = 0; +bool skip_kasan_poison =should_skip_kasan_poison (page, fpi_flags);bool init =want_init_on_free ();VM_BUG_ON_PAGE (PageTail (page), page);
@@ -1489,7 +1490,7 @@ static __always_inline bool free_pages_prepare(struct page *page,* With hardware tag-based KASAN, memory tags must be set before the * page becomes unavailable via debug_pagealloc or arch_free_page. */ -if (!should_skip_kasan_poison (page, fpi_flags)) { +if (!skip_kasan_poison) {kasan_poison_pages (page, order, init);/* Memory is already initialized if KASAN did it internally. */
Signature · 1 line
2.40.0.rc1.284.g88254d51c5-goog
PATCH v4 2/2 kasan: remove PG_skip_kasan_poison flag
In reply to Peter CollingbourneCode inspection reveals that PG_skip_kasan_poison is redundant with kasantag, because the former is intended to be set iff the latter is the match-all tag. It can also be observed that it's basically pointless to poison pages which have kasantag=0, because any pages with this tag would have been pointed to by pointers with match-all tags, so poisoning the pages would have little to no effect in terms of bug detection. Therefore, change the condition in should_skip_kasan_poison() to check kasantag instead, and remove PG_skip_kasan_poison and associated flags.
---
v4:
- rebased to linux-next
v3:
- update comments
v2:
- also remove GFP_SKIP_KASAN_POISON and FPI_SKIP_KASAN_POISON
- rename GFP_SKIP_KASAN_UNPOISON to GFP_SKIP_KASAN
- update comments
- simplify control flow by removing reset_tags
include/linux/gfp_types.h | 30 ++++++------- include/linux/page-flags.h | 9 ---- include/trace/events/mmflags.h | 13 +----- mm/kasan/hw_tags.c | 2 +- mm/page_alloc.c | 81 +++++++++++++--------------------- mm/vmalloc.c | 2 +- 6 files changed, 47 insertions(+), 90 deletions(-)
include/linux/gfp_types.h +13 -17
@@ -47,16 +47,14 @@ typedef unsigned int __bitwise gfp_t;#define ___GFP_ACCOUNT 0x400000u#define ___GFP_ZEROTAGS 0x800000u#ifdef CONFIG_KASAN_HW_TAGS -#define ___GFP_SKIP_ZERO 0x1000000u -#define ___GFP_SKIP_KASAN_UNPOISON 0x2000000u -#define ___GFP_SKIP_KASAN_POISON 0x4000000u +#define ___GFP_SKIP_ZERO 0x1000000u +#define ___GFP_SKIP_KASAN 0x2000000u#else -#define ___GFP_SKIP_ZERO 0 -#define ___GFP_SKIP_KASAN_UNPOISON 0 -#define ___GFP_SKIP_KASAN_POISON 0 +#define ___GFP_SKIP_ZERO 0 +#define ___GFP_SKIP_KASAN 0#endif #ifdef CONFIG_LOCKDEP -#define ___GFP_NOLOCKDEP 0x8000000u +#define ___GFP_NOLOCKDEP 0x4000000u#else #define ___GFP_NOLOCKDEP 0#endif
@@ -234,25 +232,24 @@ typedef unsigned int __bitwise gfp_t;* memory tags at the same time as zeroing memory has minimal additional * performace impact. * -* %__GFP_SKIP_KASAN_UNPOISON makes KASAN skip unpoisoning on page allocation. -* Only effective in HW_TAGS mode. -* -* %__GFP_SKIP_KASAN_POISON makes KASAN skip poisoning on page deallocation. -* Typically, used for userspace pages. Only effective in HW_TAGS mode. +* %__GFP_SKIP_KASAN makes KASAN skip unpoisoning on page allocation. +* Used for userspace and vmalloc pages; the latter are unpoisoned by +* kasan_unpoison_vmalloc instead. For userspace pages, results in +* poisoning being skipped as well, see should_skip_kasan_poison for +* details. Only effective in HW_TAGS mode. */ #define __GFP_NOWARN ((__force gfp_t)___GFP_NOWARN)#define __GFP_COMP ((__force gfp_t)___GFP_COMP)#define __GFP_ZERO ((__force gfp_t)___GFP_ZERO)#define __GFP_ZEROTAGS ((__force gfp_t)___GFP_ZEROTAGS)#define __GFP_SKIP_ZERO ((__force gfp_t)___GFP_SKIP_ZERO) -#define __GFP_SKIP_KASAN_UNPOISON ((__force gfp_t)___GFP_SKIP_KASAN_UNPOISON) -#define __GFP_SKIP_KASAN_POISON ((__force gfp_t)___GFP_SKIP_KASAN_POISON) +#define __GFP_SKIP_KASAN ((__force gfp_t)___GFP_SKIP_KASAN)/* Disable lockdep for GFP context tracking */ #define __GFP_NOLOCKDEP ((__force gfp_t)___GFP_NOLOCKDEP)/* Room for N __GFP_FOO bits */ -#define __GFP_BITS_SHIFT (27 + IS_ENABLED(CONFIG_LOCKDEP)) +#define __GFP_BITS_SHIFT (26 + IS_ENABLED(CONFIG_LOCKDEP))#define __GFP_BITS_MASK ((__force gfp_t)((1 << __GFP_BITS_SHIFT) - 1))/**
@@ -335,8 +332,7 @@ typedef unsigned int __bitwise gfp_t;#define GFP_DMA __GFP_DMA#define GFP_DMA32 __GFP_DMA32#define GFP_HIGHUSER (GFP_USER | __GFP_HIGHMEM) -#define GFP_HIGHUSER_MOVABLE (GFP_HIGHUSER | __GFP_MOVABLE | \ - __GFP_SKIP_KASAN_POISON | __GFP_SKIP_KASAN_UNPOISON) +#define GFP_HIGHUSER_MOVABLE (GFP_HIGHUSER | __GFP_MOVABLE | __GFP_SKIP_KASAN)#define GFP_TRANSHUGE_LIGHT ((GFP_HIGHUSER_MOVABLE | __GFP_COMP | \ __GFP_NOMEMALLOC | __GFP_NOWARN) & ~__GFP_RECLAIM)#define GFP_TRANSHUGE (GFP_TRANSHUGE_LIGHT | __GFP_DIRECT_RECLAIM)
include/linux/page-flags.h +0 -9
@@ -135,9 +135,6 @@ enum pageflags {#ifdef CONFIG_ARCH_USES_PG_ARCH_X PG_arch_2, PG_arch_3, -#endif -#ifdef CONFIG_KASAN_HW_TAGS - PG_skip_kasan_poison, #endif __NR_PAGEFLAGS,
@@ -594,12 +591,6 @@ TESTCLEARFLAG(Young, young, PF_ANY) PAGEFLAG(Idle, idle, PF_ANY)#endif -#ifdef CONFIG_KASAN_HW_TAGS -PAGEFLAG(SkipKASanPoison, skip_kasan_poison, PF_HEAD) -#else -PAGEFLAG_FALSE(SkipKASanPoison, skip_kasan_poison) -#endif -/* * PageReported() is used to track reported free pages within the Buddy * allocator. We can use the non-atomic version of the test and set
include/trace/events/mmflags.h +2 -11
@@ -55,8 +55,7 @@#ifdef CONFIG_KASAN_HW_TAGS #define __def_gfpflag_names_kasan , \ gfpflag_string(__GFP_SKIP_ZERO), \ - gfpflag_string(__GFP_SKIP_KASAN_POISON), \ - gfpflag_string(__GFP_SKIP_KASAN_UNPOISON) + gfpflag_string(__GFP_SKIP_KASAN)#else #define __def_gfpflag_names_kasan#endif
@@ -96,13 +95,6 @@#define IF_HAVE_PG_ARCH_X (_name)#endif -#ifdef CONFIG_KASAN_HW_TAGS -#define IF_HAVE_PG_SKIP_KASAN_POISON (_name) \ - ,{1UL << PG_##_name, __stringify(_name)} -#else -#define IF_HAVE_PG_SKIP_KASAN_POISON (_name) -#endif -#define DEF_PAGEFLAG_NAME (_name) { 1UL << PG_##_name, __stringify(_name) }#define __def_pageflag_names \
@@ -133,8 +125,7 @@ IF_HAVE_PG_HWPOISON(hwpoison) \ IF_HAVE_PG_IDLE(idle) \ IF_HAVE_PG_IDLE(young) \ IF_HAVE_PG_ARCH_X(arch_2) \ -IF_HAVE_PG_ARCH_X(arch_3) \ -IF_HAVE_PG_SKIP_KASAN_POISON(skip_kasan_poison) +IF_HAVE_PG_ARCH_X(arch_3) #define show_page_flags(flags) \ (flags) ? __print_flags(flags, "|", \
mm/kasan/hw_tags.c +1 -1
@@ -318,7 +318,7 @@ void *__kasan_unpoison_vmalloc(const void *start, unsigned long size,* Thus, for VM_ALLOC mappings, hardware tag-based KASAN only tags * the first virtual mapping, which is created by vmalloc(). * Tagging the page_alloc memory backing that vmalloc() allocation is -* skipped, see ___GFP_SKIP_KASAN_UNPOISON. +* skipped, see ___GFP_SKIP_KASAN. * * For non-VM_ALLOC allocations, page_alloc memory is tagged as usual. */
mm/page_alloc.c +30 -51
130 lines not on this page · raw message
mm/vmalloc.c +1 -1
@@ -3188,7 +3188,7 @@ void *__vmalloc_node_range(unsigned long size, unsigned long align,* pages backing VM_ALLOC mapping. Memory is instead * poisoned and zeroed by kasan_unpoison_vmalloc(). */ - gfp_mask |= __GFP_SKIP_KASAN_UNPOISON | __GFP_SKIP_ZERO; + gfp_mask |= __GFP_SKIP_KASAN | __GFP_SKIP_ZERO; }/* Take note that the mapping is PAGE_KERNEL. */
Signature · 1 line
2.40.0.rc1.284.g88254d51c5-goog