kasan: remove PG_skip_kasan_poison flag
Subsystems (4)
PATCH 1/1 kasan: remove PG_skip_kasan_poison flag
Code 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.
---
I sent this independently of
https://lore.kernel.org/all/[email protected]/
because I initially thought that the patches were independent. But moments after sending it, I realized that this patch depends on that one, because without that patch, this patch will end up disabling page poisoning altogether! But it's too late to turn them into a series now; I'll do that for v2.
include/linux/page-flags.h | 9 --------- include/trace/events/mmflags.h | 9 +-------- mm/page_alloc.c | 28 ++++++++-------------------- 3 files changed, 9 insertions(+), 37 deletions(-)
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 +1 -8
@@ -96,12 +96,6 @@#define IF_HAVE_PG_ARCH_X (flag,string)#endif -#ifdef CONFIG_KASAN_HW_TAGS -#define IF_HAVE_PG_SKIP_KASAN_POISON (flag,string) ,{1UL << flag, string} -#else -#define IF_HAVE_PG_SKIP_KASAN_POISON (flag,string) -#endif -#define __def_pageflag_names \ {1UL << PG_locked, "locked" }, \ {1UL << PG_waiters, "waiters" }, \
@@ -130,8 +124,7 @@ IF_HAVE_PG_HWPOISON(PG_hwpoison, "hwpoison" ) \ IF_HAVE_PG_IDLE(PG_young, "young" ) \ IF_HAVE_PG_IDLE(PG_idle, "idle" ) \ IF_HAVE_PG_ARCH_X(PG_arch_2, "arch_2" ) \ -IF_HAVE_PG_ARCH_X(PG_arch_3, "arch_3" ) \ -IF_HAVE_PG_SKIP_KASAN_POISON(PG_skip_kasan_poison, "skip_kasan_poison") +IF_HAVE_PG_ARCH_X(PG_arch_3, "arch_3" )#define show_page_flags (flags) \ (flags) ? __print_flags(flags, "|", \
mm/page_alloc.c +8 -20
@@ -1380,7 +1380,7 @@ static inline bool should_skip_kasan_poison(struct page *page, fpi_t fpi_flags)return deferred_pages_enabled () || (!IS_ENABLED (CONFIG_KASAN_GENERIC ) && (fpi_flags &FPI_SKIP_KASAN_POISON )) || -PageSkipKASanPoison (page); +page_kasan_tag (page) ==0xff ; }static void kernel_init_pages (struct page *page,int numpages)
@@ -2511,22 +2511,13 @@ inline void post_alloc_hook(struct page *page, unsigned int order,/* Take note that memory was initialized by the loop above. */ init = false; } -if (!should_skip_kasan_unpoison (gfp_flags)) { -/* Try unpoisoning (or setting tags) and initializing memory. */ -if (kasan_unpoison_pages (page, order, init)) { -/* Take note that memory was initialized by KASAN. */ -if (kasan_has_integrated_init ()) - init = false; -/* Take note that memory tags were set by KASAN. */ - reset_tags = false; - }else { -/* -* KASAN decided to exclude this allocation from being -* (un)poisoned due to sampling. Make KASAN skip -* poisoning when the allocation is freed. -*/ -SetPageSkipKASanPoison (page); - } +if (!should_skip_kasan_unpoison (gfp_flags) && +kasan_unpoison_pages (page, order, init)) { +/* Take note that memory was initialized by KASAN. */ +if (kasan_has_integrated_init ()) + init = false; +/* Take note that memory tags were set by KASAN. */ + reset_tags = false; }/* * If memory tags have not been set by KASAN, reset the page tags to
@@ -2539,9 +2530,6 @@ inline void post_alloc_hook(struct page *page, unsigned int order,/* If memory is still not initialized, initialize it now. */ if (init)kernel_init_pages (page,1 << order); -/* Propagate __GFP_SKIP_KASAN_POISON to page flags. */ -if (kasan_hw_tags_enabled () && (gfp_flags & __GFP_SKIP_KASAN_POISON)) -SetPageSkipKASanPoison (page);set_page_owner (page, order, gfp_flags);page_table_check_alloc (page, order);
Signature · 1 line
2.39.2.637.g21b0678d19-goog
Quoted 9 lines · Peter Collingbourne
Code inspection reveals that
PG_skip_kasan_poisonis 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 inshould_skip_kasan_poison()to check kasantag instead, and removePG_skip_kasan_poison.
This seems reasonable.
Quoted 75 lines
Signed-off-by Peter Collingbourne <[email protected]>---
I sent this independently of
https://lore.kernel.org/all/[email protected]/
because I initially thought that the patches were independent. But moments after sending it, I realized that this patch depends on that one, because without that patch, this patch will end up disabling page poisoning altogether! But it's too late to turn them into a series now; I'll do that for v2.include/linux/page-flags.h | 9 --------- include/trace/events/mmflags.h | 9 +-------- mm/page_alloc.c | 28 ++++++++-------------------- 3 files changed, 9 insertions(+), 37 deletions(-)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 +1
-8@@ -96,12 +96,6 @@#define IF_HAVE_PG_ARCH_X (flag,string)#endif -#ifdef CONFIG_KASAN_HW_TAGS -#define IF_HAVE_PG_SKIP_KASAN_POISON (flag,string) ,{1UL << flag, string} -#else -#define IF_HAVE_PG_SKIP_KASAN_POISON (flag,string) -#endif -#define __def_pageflag_names \ {1UL << PG_locked, "locked" }, \ {1UL << PG_waiters, "waiters" }, \@@ -130,8 +124,7 @@ IF_HAVE_PG_HWPOISON(PG_hwpoison, "hwpoison" ) \ IF_HAVE_PG_IDLE(PG_young, "young" ) \ IF_HAVE_PG_IDLE(PG_idle, "idle" ) \ IF_HAVE_PG_ARCH_X(PG_arch_2, "arch_2" ) \ -IF_HAVE_PG_ARCH_X(PG_arch_3, "arch_3" ) \ -IF_HAVE_PG_SKIP_KASAN_POISON(PG_skip_kasan_poison, "skip_kasan_poison") +IF_HAVE_PG_ARCH_X(PG_arch_3, "arch_3" )#define show_page_flags (flags) \ (flags) ? __print_flags(flags, "|", \mm/page_alloc.c +0
-0
mm/page_alloc.c static inline bool should_skip_kasan_poison(struct page *page, fpi_t fpi_flags) · line 1383
Quoted 5 lines
mm/page_alloc.c +1
-1@@ -1380,7 +1380,7 @@ static inline bool should_skip_kasan_poison(struct page *page, fpi_t fpi_flags)return deferred_pages_enabled () || (!IS_ENABLED (CONFIG_KASAN_GENERIC ) && (fpi_flags &FPI_SKIP_KASAN_POISON )) || -PageSkipKASanPoison (page); +page_kasan_tag (page) ==0xff ;
Please also update the comment above should_skip_kasan_poison.
I think we can drop #3 and #4 from that comment and instead add a more generic #3: "Page tags have not been assigned, as unpoisoning has been skipped".
Quoted 34 lines
mm/page_alloc.c +7
-16}static void kernel_init_pages (struct page *page,int numpages)@@ -2511,22 +2511,13 @@ inline void post_alloc_hook(struct page *page, unsigned int order,/* Take note that memory was initialized by the loop above. */ init = false; } -if (!should_skip_kasan_unpoison (gfp_flags)) { -/* Try unpoisoning (or setting tags) and initializing memory. */ -if (kasan_unpoison_pages (page, order, init)) { -/* Take note that memory was initialized by KASAN. */ -if (kasan_has_integrated_init ()) - init = false; -/* Take note that memory tags were set by KASAN. */ - reset_tags = false; - }else { -/* -* KASAN decided to exclude this allocation from being -* (un)poisoned due to sampling. Make KASAN skip -* poisoning when the allocation is freed. -*/ -SetPageSkipKASanPoison (page); - } +if (!should_skip_kasan_unpoison (gfp_flags) && +kasan_unpoison_pages (page, order, init)) { +/* Take note that memory was initialized by KASAN. */ +if (kasan_has_integrated_init ()) + init = false; +/* Take note that memory tags were set by KASAN. */ + reset_tags = false; }/* * If memory tags have not been set by KASAN, reset the page tags to
mm/page_alloc.c inline void post_alloc_hook(struct page *page, unsigned int order, · line 2544
Quoted 6 lines
mm/page_alloc.c +0
-3@@ -2539,9 +2530,6 @@ inline void post_alloc_hook(struct page *page, unsigned int order,/* If memory is still not initialized, initialize it now. */ if (init)kernel_init_pages (page,1 << order); -/* Propagate __GFP_SKIP_KASAN_POISON to page flags. */ -if (kasan_hw_tags_enabled () && (gfp_flags & __GFP_SKIP_KASAN_POISON)) -SetPageSkipKASanPoison (page);
With this removed, __GFP_SKIP_KASAN_POISON is no longer used and can be removed too.
Quoted 6 lines
mm/page_alloc.c +0
-1set_page_owner (page, order, gfp_flags);page_table_check_alloc (page, order); --2.39.2.637.g21b0678d19-goog
Quoted 82 lines · Andrey Konovalov
Quoted 9 lines · Peter Collingbourne
Code inspection reveals that
PG_skip_kasan_poisonis 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 inshould_skip_kasan_poison()to check kasantag instead, and removePG_skip_kasan_poison.This seems reasonable.
Quoted 68 lines
Signed-off-by Peter Collingbourne <[email protected]>---
I sent this independently of
https://lore.kernel.org/all/[email protected]/
because I initially thought that the patches were independent. But moments after sending it, I realized that this patch depends on that one, because without that patch, this patch will end up disabling page poisoning altogether! But it's too late to turn them into a series now; I'll do that for v2.include/linux/page-flags.h | 9 --------- include/trace/events/mmflags.h | 9 +-------- mm/page_alloc.c | 28 ++++++++-------------------- 3 files changed, 9 insertions(+), 37 deletions(-)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 +1
-8@@ -96,12 +96,6 @@#define IF_HAVE_PG_ARCH_X (flag,string)#endif -#ifdef CONFIG_KASAN_HW_TAGS -#define IF_HAVE_PG_SKIP_KASAN_POISON (flag,string) ,{1UL << flag, string} -#else -#define IF_HAVE_PG_SKIP_KASAN_POISON (flag,string) -#endif -#define __def_pageflag_names \ {1UL << PG_locked, "locked" }, \ {1UL << PG_waiters, "waiters" }, \@@ -130,8 +124,7 @@ IF_HAVE_PG_HWPOISON(PG_hwpoison, "hwpoison" ) \ IF_HAVE_PG_IDLE(PG_young, "young" ) \ IF_HAVE_PG_IDLE(PG_idle, "idle" ) \ IF_HAVE_PG_ARCH_X(PG_arch_2, "arch_2" ) \ -IF_HAVE_PG_ARCH_X(PG_arch_3, "arch_3" ) \ -IF_HAVE_PG_SKIP_KASAN_POISON (PG_skip_kasan_poison,"skip_kasan_poison" ) +IF_HAVE_PG_ARCH_X (PG_arch_3,"arch_3" )
Quoted 14 lines
mm/page_alloc.c static inline bool should_skip_kasan_poison(struct page *page, fpi_t fpi_flags) · line 1383
Quoted 12 lines
include/trace/events/mmflags.h +0
-0#define show_page_flags (flags) \ (flags) ? __print_flags(flags, "|", \mm/page_alloc.c +1
-1@@ -1380,7 +1380,7 @@ static inline bool should_skip_kasan_poison(struct page *page, fpi_t fpi_flags)return deferred_pages_enabled () || (!IS_ENABLED (CONFIG_KASAN_GENERIC ) && (fpi_flags &FPI_SKIP_KASAN_POISON )) || -PageSkipKASanPoison (page); +page_kasan_tag (page) ==0xff ;Please also update the comment above
should_skip_kasan_poison.
Done in v2.
Quoted 3 lines
I think we can drop #3 and #4 from that comment and instead add a more generic #3: "Page tags have not been assigned, as unpoisoning has been skipped".
I realized that the page tag will also be unassigned when the page is first being initialized, so I decided to be more explicit in the comment here about the circumstances where this will happen. I also took the opportunity to remove the FPI_SKIP_KASAN_POISON flag, since I realized that it is now also redundant with the page tag.
Quoted 35 lines
Quoted 35 lines
mm/page_alloc.c +7
-16}static void kernel_init_pages (struct page *page,int numpages)@@ -2511,22 +2511,13 @@ inline void post_alloc_hook(struct page *page, unsigned int order,/* Take note that memory was initialized by the loop above. */ init = false; } -if (!should_skip_kasan_unpoison (gfp_flags)) { -/* Try unpoisoning (or setting tags) and initializing memory. */ -if (kasan_unpoison_pages (page, order, init)) { -/* Take note that memory was initialized by KASAN. */ -if (kasan_has_integrated_init ()) - init = false; -/* Take note that memory tags were set by KASAN. */ - reset_tags = false; - }else { -/* -* KASAN decided to exclude this allocation from being -* (un)poisoned due to sampling. Make KASAN skip -* poisoning when the allocation is freed. -*/ -SetPageSkipKASanPoison (page); - } +if (!should_skip_kasan_unpoison (gfp_flags) && +kasan_unpoison_pages (page, order, init)) { +/* Take note that memory was initialized by KASAN. */ +if (kasan_has_integrated_init ()) + init = false; +/* Take note that memory tags were set by KASAN. */ + reset_tags = false; }/* * If memory tags have not been set by KASAN, reset the page tags to @@ -2539,9 +2530,6 @@ inline void post_alloc_hook(struct page *page, unsigned int order,/* If memory is still not initialized, initialize it now. */
Quoted 8 lines
mm/page_alloc.c inline void post_alloc_hook(struct page *page, unsigned int order, · line 2544
Quoted 5 lines
mm/page_alloc.c +0
-3if (init)kernel_init_pages (page,1 << order); -/* Propagate __GFP_SKIP_KASAN_POISON to page flags. */ -if (kasan_hw_tags_enabled () && (gfp_flags & __GFP_SKIP_KASAN_POISON)) -SetPageSkipKASanPoison (page);With this removed,
__GFP_SKIP_KASAN_POISONis no longer used and can be removed too.
Done in v2. Since the remaining flag will skip both poisoning and unpoisoning, I decided to rename it to GFP_SKIP_KASAN.
Peter