diff mbox series

[mm,v4,28/39] kasan, page_alloc: allow skipping unpoisoning for HW_TAGS

Message ID 73a0b47ec72a9c29e0efc18a9941237b3b3ad736.1640036051.git.andreyknvl@google.com (mailing list archive)
State New, archived
Headers show
Series kasan, vmalloc, arm64: add vmalloc tagging support for SW/HW_TAGS | expand

Commit Message

andrey.konovalov@linux.dev Dec. 20, 2021, 10:02 p.m. UTC 
From: Andrey Konovalov <andreyknvl@google.com>

Add a new GFP flag __GFP_SKIP_KASAN_UNPOISON that allows skipping KASAN
poisoning for page_alloc allocations. The flag is only effective with
HW_TAGS KASAN.

This flag will be used by vmalloc code for page_alloc allocations
backing vmalloc() mappings in a following patch. The reason to skip
KASAN poisoning for these pages in page_alloc is because vmalloc code
will be poisoning them instead.

Also reword the comment for __GFP_SKIP_KASAN_POISON.

Signed-off-by: Andrey Konovalov <andreyknvl@google.com>

---

Changes v3->v4:
- Only define __GFP_SKIP_KASAN_POISON when CONFIG_KASAN_HW_TAGS is enabled.

Changes v2->v3:
- Update patch description.

Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
---
 include/linux/gfp.h            | 18 ++++++++++++------
 include/trace/events/mmflags.h |  4 +++-
 mm/page_alloc.c                | 31 ++++++++++++++++++++++---------
 3 files changed, 37 insertions(+), 16 deletions(-)

Comments

Marco Elver Dec. 21, 2021, 12:04 p.m. UTC | #1 
On Mon, Dec 20, 2021 at 11:02PM +0100, andrey.konovalov@linux.dev wrote:
[...]
>  #ifdef CONFIG_KASAN_HW_TAGS
>  #define __def_gfpflag_names_kasan					      \
> -	, {(unsigned long)__GFP_SKIP_KASAN_POISON, "__GFP_SKIP_KASAN_POISON"}
> +	, {(unsigned long)__GFP_SKIP_KASAN_POISON, "__GFP_SKIP_KASAN_POISON"} \
> +	, {(unsigned long)__GFP_SKIP_KASAN_UNPOISON,			      \
> +						"__GFP_SKIP_KASAN_UNPOISON"}
>  #else
>  #define __def_gfpflag_names_kasan
>  #endif

Adhering to 80 cols here makes the above less readable. If you do a v5,
my suggestion is:

diff --git a/include/trace/events/mmflags.h b/include/trace/events/mmflags.h
index f18eeb5fdde2..f9f0ae3a4b6b 100644
--- a/include/trace/events/mmflags.h
+++ b/include/trace/events/mmflags.h
@@ -51,11 +51,10 @@
 	{(unsigned long)__GFP_ZEROTAGS,		"__GFP_ZEROTAGS"}	\
 
 #ifdef CONFIG_KASAN_HW_TAGS
-#define __def_gfpflag_names_kasan					      \
-	, {(unsigned long)__GFP_SKIP_ZERO, "__GFP_SKIP_ZERO"}		      \
-	, {(unsigned long)__GFP_SKIP_KASAN_POISON, "__GFP_SKIP_KASAN_POISON"} \
-	, {(unsigned long)__GFP_SKIP_KASAN_UNPOISON,			      \
-						"__GFP_SKIP_KASAN_UNPOISON"}
+#define __def_gfpflag_names_kasan ,							\
+	{(unsigned long)__GFP_SKIP_ZERO,		"__GFP_SKIP_ZERO"},		\
+	{(unsigned long)__GFP_SKIP_KASAN_POISON,	"__GFP_SKIP_KASAN_POISON"},	\
+	{(unsigned long)__GFP_SKIP_KASAN_UNPOISON,	"__GFP_SKIP_KASAN_UNPOISON"}
 #else
 #define __def_gfpflag_names_kasan
 #endif
Marco Elver Dec. 21, 2021, 12:14 p.m. UTC | #2 
On Mon, Dec 20, 2021 at 11:02PM +0100, andrey.konovalov@linux.dev wrote:
[...]
> +static inline bool should_skip_kasan_unpoison(gfp_t flags, bool init_tags)
> +{
> +	/* Don't skip if a software KASAN mode is enabled. */
> +	if (IS_ENABLED(CONFIG_KASAN_GENERIC) ||
> +	    IS_ENABLED(CONFIG_KASAN_SW_TAGS))
> +		return false;
> +
> +	/* Skip, if hardware tag-based KASAN is not enabled. */
> +	if (!kasan_hw_tags_enabled())
> +		return true;

Same question here: why is IS_ENABLED(CONFIG_KASAN_{GENERIC,SW_TAGS})
check required if kasan_hw_tags_enabled() is always false if one of
those is configured?
Marco Elver Dec. 21, 2021, 12:19 p.m. UTC | #3 
On Tue, 21 Dec 2021 at 13:14, Marco Elver <elver@google.com> wrote:
>
> On Mon, Dec 20, 2021 at 11:02PM +0100, andrey.konovalov@linux.dev wrote:
> [...]
> > +static inline bool should_skip_kasan_unpoison(gfp_t flags, bool init_tags)
> > +{
> > +     /* Don't skip if a software KASAN mode is enabled. */
> > +     if (IS_ENABLED(CONFIG_KASAN_GENERIC) ||
> > +         IS_ENABLED(CONFIG_KASAN_SW_TAGS))
> > +             return false;
> > +
> > +     /* Skip, if hardware tag-based KASAN is not enabled. */
> > +     if (!kasan_hw_tags_enabled())
> > +             return true;
>
> Same question here: why is IS_ENABLED(CONFIG_KASAN_{GENERIC,SW_TAGS})
> check required if kasan_hw_tags_enabled() is always false if one of
> those is configured?

Hmm, I pattern-matched too quickly. In this case there's probably no
way around it because the return value is different, so not exactly
like the should_skip_init().
Andrey Konovalov Dec. 30, 2021, 7:11 p.m. UTC | #4 
On Tue, Dec 21, 2021 at 1:05 PM Marco Elver <elver@google.com> wrote:
>
> On Mon, Dec 20, 2021 at 11:02PM +0100, andrey.konovalov@linux.dev wrote:
> [...]
> >  #ifdef CONFIG_KASAN_HW_TAGS
> >  #define __def_gfpflag_names_kasan                                          \
> > -     , {(unsigned long)__GFP_SKIP_KASAN_POISON, "__GFP_SKIP_KASAN_POISON"}
> > +     , {(unsigned long)__GFP_SKIP_KASAN_POISON, "__GFP_SKIP_KASAN_POISON"} \
> > +     , {(unsigned long)__GFP_SKIP_KASAN_UNPOISON,                          \
> > +                                             "__GFP_SKIP_KASAN_UNPOISON"}
> >  #else
> >  #define __def_gfpflag_names_kasan
> >  #endif
>
> Adhering to 80 cols here makes the above less readable. If you do a v5,
> my suggestion is:
>
> diff --git a/include/trace/events/mmflags.h b/include/trace/events/mmflags.h
> index f18eeb5fdde2..f9f0ae3a4b6b 100644
> --- a/include/trace/events/mmflags.h
> +++ b/include/trace/events/mmflags.h
> @@ -51,11 +51,10 @@
>         {(unsigned long)__GFP_ZEROTAGS,         "__GFP_ZEROTAGS"}       \
>
>  #ifdef CONFIG_KASAN_HW_TAGS
> -#define __def_gfpflag_names_kasan                                            \
> -       , {(unsigned long)__GFP_SKIP_ZERO, "__GFP_SKIP_ZERO"}                 \
> -       , {(unsigned long)__GFP_SKIP_KASAN_POISON, "__GFP_SKIP_KASAN_POISON"} \
> -       , {(unsigned long)__GFP_SKIP_KASAN_UNPOISON,                          \
> -                                               "__GFP_SKIP_KASAN_UNPOISON"}
> +#define __def_gfpflag_names_kasan ,                                                    \
> +       {(unsigned long)__GFP_SKIP_ZERO,                "__GFP_SKIP_ZERO"},             \
> +       {(unsigned long)__GFP_SKIP_KASAN_POISON,        "__GFP_SKIP_KASAN_POISON"},     \
> +       {(unsigned long)__GFP_SKIP_KASAN_UNPOISON,      "__GFP_SKIP_KASAN_UNPOISON"}
>  #else
>  #define __def_gfpflag_names_kasan
>  #endif

Will do in v5, thanks!
diff mbox series

Patch

diff --git a/include/linux/gfp.h b/include/linux/gfp.h
index 22709fcc4d3a..600f0749c3f2 100644
--- a/include/linux/gfp.h
+++ b/include/linux/gfp.h
@@ -55,12 +55,14 @@  struct vm_area_struct;
 #define ___GFP_ACCOUNT		0x400000u
 #define ___GFP_ZEROTAGS		0x800000u
 #ifdef CONFIG_KASAN_HW_TAGS
-#define ___GFP_SKIP_KASAN_POISON	0x1000000u
+#define ___GFP_SKIP_KASAN_UNPOISON	0x1000000u
+#define ___GFP_SKIP_KASAN_POISON	0x2000000u
 #else
+#define ___GFP_SKIP_KASAN_UNPOISON	0
 #define ___GFP_SKIP_KASAN_POISON	0
 #endif
 #ifdef CONFIG_LOCKDEP
-#define ___GFP_NOLOCKDEP	0x2000000u
+#define ___GFP_NOLOCKDEP	0x4000000u
 #else
 #define ___GFP_NOLOCKDEP	0
 #endif
@@ -235,21 +237,25 @@  struct vm_area_struct;
  * %__GFP_ZEROTAGS zeroes memory tags at allocation time if the memory itself
  * is being zeroed (either via __GFP_ZERO or via init_on_alloc).
  *
- * %__GFP_SKIP_KASAN_POISON returns a page which does not need to be poisoned
- * on deallocation. Typically used for userspace pages. Currently only has an
- * effect in HW tags mode.
+ * %__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.
  */
 #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_KASAN_POISON	((__force gfp_t)___GFP_SKIP_KASAN_POISON)
+#define __GFP_SKIP_KASAN_UNPOISON ((__force gfp_t)___GFP_SKIP_KASAN_UNPOISON)
+#define __GFP_SKIP_KASAN_POISON   ((__force gfp_t)___GFP_SKIP_KASAN_POISON)
 
 /* Disable lockdep for GFP context tracking */
 #define __GFP_NOLOCKDEP ((__force gfp_t)___GFP_NOLOCKDEP)
 
 /* Room for N __GFP_FOO bits */
 #define __GFP_BITS_SHIFT (24 +					\
+			  IS_ENABLED(CONFIG_KASAN_HW_TAGS) +	\
 			  IS_ENABLED(CONFIG_KASAN_HW_TAGS) +	\
 			  IS_ENABLED(CONFIG_LOCKDEP))
 #define __GFP_BITS_MASK ((__force gfp_t)((1 << __GFP_BITS_SHIFT) - 1))
diff --git a/include/trace/events/mmflags.h b/include/trace/events/mmflags.h
index 414bf4367283..1329d9c4df56 100644
--- a/include/trace/events/mmflags.h
+++ b/include/trace/events/mmflags.h
@@ -52,7 +52,9 @@ 
 
 #ifdef CONFIG_KASAN_HW_TAGS
 #define __def_gfpflag_names_kasan					      \
-	, {(unsigned long)__GFP_SKIP_KASAN_POISON, "__GFP_SKIP_KASAN_POISON"}
+	, {(unsigned long)__GFP_SKIP_KASAN_POISON, "__GFP_SKIP_KASAN_POISON"} \
+	, {(unsigned long)__GFP_SKIP_KASAN_UNPOISON,			      \
+						"__GFP_SKIP_KASAN_UNPOISON"}
 #else
 #define __def_gfpflag_names_kasan
 #endif
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 2ef0f531e881..2076b5cc7e2c 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -2394,6 +2394,26 @@  static bool check_new_pages(struct page *page, unsigned int order)
 	return false;
 }
 
+static inline bool should_skip_kasan_unpoison(gfp_t flags, bool init_tags)
+{
+	/* Don't skip if a software KASAN mode is enabled. */
+	if (IS_ENABLED(CONFIG_KASAN_GENERIC) ||
+	    IS_ENABLED(CONFIG_KASAN_SW_TAGS))
+		return false;
+
+	/* Skip, if hardware tag-based KASAN is not enabled. */
+	if (!kasan_hw_tags_enabled())
+		return true;
+
+	/*
+	 * With hardware tag-based KASAN enabled, skip if either:
+	 *
+	 * 1. Memory tags have already been cleared via tag_clear_highpage().
+	 * 2. Skipping has been requested via __GFP_SKIP_KASAN_UNPOISON.
+	 */
+	return init_tags || (flags & __GFP_SKIP_KASAN_UNPOISON);
+}
+
 inline void post_alloc_hook(struct page *page, unsigned int order,
 				gfp_t gfp_flags)
 {
@@ -2433,15 +2453,8 @@  inline void post_alloc_hook(struct page *page, unsigned int order,
 		/* Note that memory is already initialized by the loop above. */
 		init = false;
 	}
-	/*
-	 * If either a software KASAN mode is enabled, or,
-	 * in the case of hardware tag-based KASAN,
-	 * if memory tags have not been cleared via tag_clear_highpage().
-	 */
-	if (IS_ENABLED(CONFIG_KASAN_GENERIC) ||
-	    IS_ENABLED(CONFIG_KASAN_SW_TAGS) ||
-	    kasan_hw_tags_enabled() && !init_tags) {
-		/* Mark shadow memory or set memory tags. */
+	if (!should_skip_kasan_unpoison(gfp_flags, init_tags)) {
+		/* Unpoison shadow memory or set memory tags. */
 		kasan_unpoison_pages(page, order, init);
 
 		/* Note that memory is already initialized by KASAN. */