[PATCH] mm/slub, kunit: Make slub_kunit pass even when SLAB_RED_ZONE flag is set

[PATCH] mm/slub, kunit: Make slub_kunit pass even when SLAB_RED_ZONE flag is set

[Hyeonggon Yoo]

Testcase test_next_pointer in slub_kunit fails when SLAB_RED_ZONE flag
is globally set. This is because on_freelist() cuts corrupted freelist
chain and does not update cut objects' redzone to SLUB_RED_ACTIVE.

When the test validates a slab that whose freelist is cut, it expects
redzone of objects unreachable by freelist is set to SLUB_RED_ACTIVE.
And it reports "Left Redzone overritten" error because the expectation
failed.

This patch makes slub_kunit expect two more errors for reporting and
fixing red overwritten error when SLAB_RED_ZONE flag is set.

The test passes on slub_debug and slub_debug=Z after this patch.

Signed-off-by: Hyeonggon Yoo <42.hyeyoo@gmail.com>
---
 lib/slub_kunit.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/lib/slub_kunit.c b/lib/slub_kunit.c
index 8662dc6cb509..7cf1fb5a7fde 100644
--- a/lib/slub_kunit.c
+++ b/lib/slub_kunit.c
@@ -45,21 +45,36 @@ static void test_next_pointer(struct kunit *test)
 	 * Expecting three errors.
 	 * One for the corrupted freechain and the other one for the wrong
 	 * count of objects in use. The third error is fixing broken cache.
+	 *
+	 * When flag SLUB_RED_ZONE is set, we expect two more errors for reporting
+	 * and fixing overwritten redzone error. This two errors are detected
+	 * because SLUB cuts corrupted freelist in on_freelist(), but does not
+	 * update its redzone to SLUB_RED_ACTIVE.
 	 */
 	validate_slab_cache(s);
-	KUNIT_EXPECT_EQ(test, 3, slab_errors);
+
+	if (s->flags & SLAB_RED_ZONE)
+		KUNIT_EXPECT_EQ(test, 5, slab_errors);
+	else
+		KUNIT_EXPECT_EQ(test, 3, slab_errors);
 
 	/*
 	 * Try to repair corrupted freepointer.
 	 * Still expecting two errors. The first for the wrong count
 	 * of objects in use.
 	 * The second error is for fixing broken cache.
+	 *
+	 * When SLUB_RED_ZONE flag is set, we expect two more errors
+	 * for same reason as above.
 	 */
 	*ptr_addr = tmp;
 	slab_errors = 0;
 
 	validate_slab_cache(s);
-	KUNIT_EXPECT_EQ(test, 2, slab_errors);
+	if (s->flags & SLAB_RED_ZONE)
+		KUNIT_EXPECT_EQ(test, 4, slab_errors);
+	else
+		KUNIT_EXPECT_EQ(test, 2, slab_errors);
 
 	/*
 	 * Previous validation repaired the count of objects in use.
-- 
2.33.1

Re: [PATCH] mm/slub, kunit: Make slub_kunit pass even when SLAB_RED_ZONE flag is set

[Vlastimil Babka]

On 3/16/22 15:38, Hyeonggon Yoo wrote:
> Testcase test_next_pointer in slub_kunit fails when SLAB_RED_ZONE flag
> is globally set. This is because on_freelist() cuts corrupted freelist
> chain and does not update cut objects' redzone to SLUB_RED_ACTIVE.
> 
> When the test validates a slab that whose freelist is cut, it expects
> redzone of objects unreachable by freelist is set to SLUB_RED_ACTIVE.
> And it reports "Left Redzone overritten" error because the expectation
> failed.
> 
> This patch makes slub_kunit expect two more errors for reporting and
> fixing red overwritten error when SLAB_RED_ZONE flag is set.
> 
> The test passes on slub_debug and slub_debug=Z after this patch.

Hmm I think it's not optimal strategy for unit tests to adapt like this to
external influence. It seems rather fragile. The test cases should be
designed to test a specific condition and that's it. So maybe we could e.g.
introduce a new SLAB_ flag passed to kmem_cache_create that tells it to
ignore any globally specified slub debug flags?

> Signed-off-by: Hyeonggon Yoo <42.hyeyoo@gmail.com>
> ---
>  lib/slub_kunit.c | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/slub_kunit.c b/lib/slub_kunit.c
> index 8662dc6cb509..7cf1fb5a7fde 100644
> --- a/lib/slub_kunit.c
> +++ b/lib/slub_kunit.c
> @@ -45,21 +45,36 @@ static void test_next_pointer(struct kunit *test)
>  	 * Expecting three errors.
>  	 * One for the corrupted freechain and the other one for the wrong
>  	 * count of objects in use. The third error is fixing broken cache.
> +	 *
> +	 * When flag SLUB_RED_ZONE is set, we expect two more errors for reporting
> +	 * and fixing overwritten redzone error. This two errors are detected
> +	 * because SLUB cuts corrupted freelist in on_freelist(), but does not
> +	 * update its redzone to SLUB_RED_ACTIVE.
>  	 */
>  	validate_slab_cache(s);
> -	KUNIT_EXPECT_EQ(test, 3, slab_errors);
> +
> +	if (s->flags & SLAB_RED_ZONE)
> +		KUNIT_EXPECT_EQ(test, 5, slab_errors);
> +	else
> +		KUNIT_EXPECT_EQ(test, 3, slab_errors);
>  
>  	/*
>  	 * Try to repair corrupted freepointer.
>  	 * Still expecting two errors. The first for the wrong count
>  	 * of objects in use.
>  	 * The second error is for fixing broken cache.
> +	 *
> +	 * When SLUB_RED_ZONE flag is set, we expect two more errors
> +	 * for same reason as above.
>  	 */
>  	*ptr_addr = tmp;
>  	slab_errors = 0;
>  
>  	validate_slab_cache(s);
> -	KUNIT_EXPECT_EQ(test, 2, slab_errors);
> +	if (s->flags & SLAB_RED_ZONE)
> +		KUNIT_EXPECT_EQ(test, 4, slab_errors);
> +	else
> +		KUNIT_EXPECT_EQ(test, 2, slab_errors);
>  
>  	/*
>  	 * Previous validation repaired the count of objects in use.

Re: [PATCH] mm/slub, kunit: Make slub_kunit pass even when SLAB_RED_ZONE flag is set

[Hyeonggon Yoo]

On Wed, Mar 16, 2022 at 11:11:08PM +0100, Vlastimil Babka wrote:
> On 3/16/22 15:38, Hyeonggon Yoo wrote:
> > Testcase test_next_pointer in slub_kunit fails when SLAB_RED_ZONE flag
> > is globally set. This is because on_freelist() cuts corrupted freelist
> > chain and does not update cut objects' redzone to SLUB_RED_ACTIVE.
> > 
> > When the test validates a slab that whose freelist is cut, it expects
> > redzone of objects unreachable by freelist is set to SLUB_RED_ACTIVE.
> > And it reports "Left Redzone overritten" error because the expectation
> > failed.
> > 
> > This patch makes slub_kunit expect two more errors for reporting and
> > fixing red overwritten error when SLAB_RED_ZONE flag is set.
> > 
> > The test passes on slub_debug and slub_debug=Z after this patch.
> 
> Hmm I think it's not optimal strategy for unit tests to adapt like this to
> external influence. It seems rather fragile. The test cases should be
> designed to test a specific condition and that's it. So maybe we could e.g.
> introduce a new SLAB_ flag passed to kmem_cache_create that tells it to
> ignore any globally specified slub debug flags?

Agree. It's so easy to be broken. I think your suggestion is
good for unit tests. I'll send a patch.

BTW, The situation is that SLUB makes (by cutting freelist chain)
objects invalid (invalid redzone) and reporting what caused by SLUB
itself is quite ugly.

No simple solution comes into mind but yeah... it's not a big problem
as we rarely call validate_slab_cache().

> > Signed-off-by: Hyeonggon Yoo <42.hyeyoo@gmail.com>
> > ---
> >  lib/slub_kunit.c | 19 +++++++++++++++++--
> >  1 file changed, 17 insertions(+), 2 deletions(-)
> > 
> > diff --git a/lib/slub_kunit.c b/lib/slub_kunit.c
> > index 8662dc6cb509..7cf1fb5a7fde 100644
> > --- a/lib/slub_kunit.c
> > +++ b/lib/slub_kunit.c
> > @@ -45,21 +45,36 @@ static void test_next_pointer(struct kunit *test)
> >  	 * Expecting three errors.
> >  	 * One for the corrupted freechain and the other one for the wrong
> >  	 * count of objects in use. The third error is fixing broken cache.
> > +	 *
> > +	 * When flag SLUB_RED_ZONE is set, we expect two more errors for reporting
> > +	 * and fixing overwritten redzone error. This two errors are detected
> > +	 * because SLUB cuts corrupted freelist in on_freelist(), but does not
> > +	 * update its redzone to SLUB_RED_ACTIVE.
> >  	 */
> >  	validate_slab_cache(s);
> > -	KUNIT_EXPECT_EQ(test, 3, slab_errors);
> > +
> > +	if (s->flags & SLAB_RED_ZONE)
> > +		KUNIT_EXPECT_EQ(test, 5, slab_errors);
> > +	else
> > +		KUNIT_EXPECT_EQ(test, 3, slab_errors);
> >  
> >  	/*
> >  	 * Try to repair corrupted freepointer.
> >  	 * Still expecting two errors. The first for the wrong count
> >  	 * of objects in use.
> >  	 * The second error is for fixing broken cache.
> > +	 *
> > +	 * When SLUB_RED_ZONE flag is set, we expect two more errors
> > +	 * for same reason as above.
> >  	 */
> >  	*ptr_addr = tmp;
> >  	slab_errors = 0;
> >  
> >  	validate_slab_cache(s);
> > -	KUNIT_EXPECT_EQ(test, 2, slab_errors);
> > +	if (s->flags & SLAB_RED_ZONE)
> > +		KUNIT_EXPECT_EQ(test, 4, slab_errors);
> > +	else
> > +		KUNIT_EXPECT_EQ(test, 2, slab_errors);
> >  
> >  	/*
> >  	 * Previous validation repaired the count of objects in use.
> 

-- 
Thank you, You are awesome!
Hyeonggon :-)

[PATCH] mm/slub, kunit: Make slub_kunit unaffected by global slub debugging flags

[Hyeonggon Yoo]

slub_kunit does not expect other debugging flags to be set when running
tests. When SLAB_RED_ZONE flag is set globally, test fails because the
flag affects number of errors reported.

To make slub_kunit unaffected by global slub debugging flags, introduce
SLAB_NO_GLOBAL_FLAGS to ignore them. It's still allowed to specify
debugging flags by specifying cache name(s) in slub_debug parameter.

Suggested-by: Vlastimil Babka <vbabka@suse.cz>
Signed-off-by: Hyeonggon Yoo <42.hyeyoo@gmail.com>
---
 include/linux/slab.h |  3 +++
 lib/slub_kunit.c     | 10 +++++-----
 mm/slab.h            |  5 +++--
 mm/slub.c            |  3 +++
 4 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/include/linux/slab.h b/include/linux/slab.h
index 0381868e5118..11fe2c28422d 100644
--- a/include/linux/slab.h
+++ b/include/linux/slab.h
@@ -112,6 +112,9 @@
 #define SLAB_KASAN		0
 #endif
 
+/* Ignore globally specified debugging flags */
+#define SLAB_NO_GLOBAL_FLAGS	((slab_flags_t __force)0x10000000U)
+
 /* The following flags affect the page allocator grouping pages by mobility */
 /* Objects are reclaimable */
 #define SLAB_RECLAIM_ACCOUNT	((slab_flags_t __force)0x00020000U)
diff --git a/lib/slub_kunit.c b/lib/slub_kunit.c
index 8662dc6cb509..acf061dc558d 100644
--- a/lib/slub_kunit.c
+++ b/lib/slub_kunit.c
@@ -12,7 +12,7 @@ static int slab_errors;
 static void test_clobber_zone(struct kunit *test)
 {
 	struct kmem_cache *s = kmem_cache_create("TestSlub_RZ_alloc", 64, 0,
-				SLAB_RED_ZONE, NULL);
+				SLAB_RED_ZONE|SLAB_NO_GLOBAL_FLAGS, NULL);
 	u8 *p = kmem_cache_alloc(s, GFP_KERNEL);
 
 	kasan_disable_current();
@@ -30,7 +30,7 @@ static void test_clobber_zone(struct kunit *test)
 static void test_next_pointer(struct kunit *test)
 {
 	struct kmem_cache *s = kmem_cache_create("TestSlub_next_ptr_free", 64, 0,
-				SLAB_POISON, NULL);
+				SLAB_POISON|SLAB_NO_GLOBAL_FLAGS, NULL);
 	u8 *p = kmem_cache_alloc(s, GFP_KERNEL);
 	unsigned long tmp;
 	unsigned long *ptr_addr;
@@ -75,7 +75,7 @@ static void test_next_pointer(struct kunit *test)
 static void test_first_word(struct kunit *test)
 {
 	struct kmem_cache *s = kmem_cache_create("TestSlub_1th_word_free", 64, 0,
-				SLAB_POISON, NULL);
+				SLAB_POISON|SLAB_NO_GLOBAL_FLAGS, NULL);
 	u8 *p = kmem_cache_alloc(s, GFP_KERNEL);
 
 	kmem_cache_free(s, p);
@@ -90,7 +90,7 @@ static void test_first_word(struct kunit *test)
 static void test_clobber_50th_byte(struct kunit *test)
 {
 	struct kmem_cache *s = kmem_cache_create("TestSlub_50th_word_free", 64, 0,
-				SLAB_POISON, NULL);
+				SLAB_POISON|SLAB_NO_GLOBAL_FLAGS, NULL);
 	u8 *p = kmem_cache_alloc(s, GFP_KERNEL);
 
 	kmem_cache_free(s, p);
@@ -106,7 +106,7 @@ static void test_clobber_50th_byte(struct kunit *test)
 static void test_clobber_redzone_free(struct kunit *test)
 {
 	struct kmem_cache *s = kmem_cache_create("TestSlub_RZ_free", 64, 0,
-				SLAB_RED_ZONE, NULL);
+				SLAB_RED_ZONE|SLAB_NO_GLOBAL_FLAGS, NULL);
 	u8 *p = kmem_cache_alloc(s, GFP_KERNEL);
 
 	kasan_disable_current();
diff --git a/mm/slab.h b/mm/slab.h
index c7f2abc2b154..69946131208a 100644
--- a/mm/slab.h
+++ b/mm/slab.h
@@ -330,7 +330,7 @@ static inline slab_flags_t kmem_cache_flags(unsigned int object_size,
 			  SLAB_ACCOUNT)
 #elif defined(CONFIG_SLUB)
 #define SLAB_CACHE_FLAGS (SLAB_NOLEAKTRACE | SLAB_RECLAIM_ACCOUNT | \
-			  SLAB_TEMPORARY | SLAB_ACCOUNT)
+			  SLAB_TEMPORARY | SLAB_ACCOUNT | SLAB_NO_GLOBAL_FLAGS)
 #else
 #define SLAB_CACHE_FLAGS (SLAB_NOLEAKTRACE)
 #endif
@@ -349,7 +349,8 @@ static inline slab_flags_t kmem_cache_flags(unsigned int object_size,
 			      SLAB_NOLEAKTRACE | \
 			      SLAB_RECLAIM_ACCOUNT | \
 			      SLAB_TEMPORARY | \
-			      SLAB_ACCOUNT)
+			      SLAB_ACCOUNT | \
+			      SLAB_NO_GLOBAL_FLAGS)
 
 bool __kmem_cache_empty(struct kmem_cache *);
 int __kmem_cache_shutdown(struct kmem_cache *);
diff --git a/mm/slub.c b/mm/slub.c
index 71e8663f6037..2a3cffd7b27f 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -1592,6 +1592,9 @@ slab_flags_t kmem_cache_flags(unsigned int object_size,
 	if (flags & SLAB_NOLEAKTRACE)
 		slub_debug_local &= ~SLAB_STORE_USER;
 
+	if (flags & SLAB_NO_GLOBAL_FLAGS)
+		slub_debug_local = 0;
+
 	len = strlen(name);
 	next_block = slub_debug_string;
 	/* Go through all blocks of debug options, see if any matches our slab's name */
-- 
2.33.1

Re: [PATCH] mm/slub, kunit: Make slub_kunit unaffected by global slub debugging flags

[Vlastimil Babka]

On 3/17/22 09:10, Hyeonggon Yoo wrote:
> slub_kunit does not expect other debugging flags to be set when running
> tests. When SLAB_RED_ZONE flag is set globally, test fails because the
> flag affects number of errors reported.
> 
> To make slub_kunit unaffected by global slub debugging flags, introduce
> SLAB_NO_GLOBAL_FLAGS to ignore them. It's still allowed to specify
> debugging flags by specifying cache name(s) in slub_debug parameter.

Given how we support globbing, I think it would be safest to just ignore
everything that comes from slub_debug parameter when the
SLAB_NO_GLOBAL_FLAGS flag is specified, even if it involves a (partial)
cache name match.
Maybe name it SLAB_NO_USER_FLAGS then?

> Suggested-by: Vlastimil Babka <vbabka@suse.cz>
> Signed-off-by: Hyeonggon Yoo <42.hyeyoo@gmail.com>
> ---
>  include/linux/slab.h |  3 +++
>  lib/slub_kunit.c     | 10 +++++-----
>  mm/slab.h            |  5 +++--
>  mm/slub.c            |  3 +++
>  4 files changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/include/linux/slab.h b/include/linux/slab.h
> index 0381868e5118..11fe2c28422d 100644
> --- a/include/linux/slab.h
> +++ b/include/linux/slab.h
> @@ -112,6 +112,9 @@
>  #define SLAB_KASAN		0
>  #endif
>  
> +/* Ignore globally specified debugging flags */

I'd add that this is intended for caches created for self-tests so they
always have flags as specified in the code.

> +#define SLAB_NO_GLOBAL_FLAGS	((slab_flags_t __force)0x10000000U)
> +
>  /* The following flags affect the page allocator grouping pages by mobility */
>  /* Objects are reclaimable */
>  #define SLAB_RECLAIM_ACCOUNT	((slab_flags_t __force)0x00020000U)
> diff --git a/lib/slub_kunit.c b/lib/slub_kunit.c
> index 8662dc6cb509..acf061dc558d 100644
> --- a/lib/slub_kunit.c
> +++ b/lib/slub_kunit.c
> @@ -12,7 +12,7 @@ static int slab_errors;
>  static void test_clobber_zone(struct kunit *test)
>  {
>  	struct kmem_cache *s = kmem_cache_create("TestSlub_RZ_alloc", 64, 0,
> -				SLAB_RED_ZONE, NULL);
> +				SLAB_RED_ZONE|SLAB_NO_GLOBAL_FLAGS, NULL);
>  	u8 *p = kmem_cache_alloc(s, GFP_KERNEL);
>  
>  	kasan_disable_current();
> @@ -30,7 +30,7 @@ static void test_clobber_zone(struct kunit *test)
>  static void test_next_pointer(struct kunit *test)
>  {
>  	struct kmem_cache *s = kmem_cache_create("TestSlub_next_ptr_free", 64, 0,
> -				SLAB_POISON, NULL);
> +				SLAB_POISON|SLAB_NO_GLOBAL_FLAGS, NULL);
>  	u8 *p = kmem_cache_alloc(s, GFP_KERNEL);
>  	unsigned long tmp;
>  	unsigned long *ptr_addr;
> @@ -75,7 +75,7 @@ static void test_next_pointer(struct kunit *test)
>  static void test_first_word(struct kunit *test)
>  {
>  	struct kmem_cache *s = kmem_cache_create("TestSlub_1th_word_free", 64, 0,
> -				SLAB_POISON, NULL);
> +				SLAB_POISON|SLAB_NO_GLOBAL_FLAGS, NULL);
>  	u8 *p = kmem_cache_alloc(s, GFP_KERNEL);
>  
>  	kmem_cache_free(s, p);
> @@ -90,7 +90,7 @@ static void test_first_word(struct kunit *test)
>  static void test_clobber_50th_byte(struct kunit *test)
>  {
>  	struct kmem_cache *s = kmem_cache_create("TestSlub_50th_word_free", 64, 0,
> -				SLAB_POISON, NULL);
> +				SLAB_POISON|SLAB_NO_GLOBAL_FLAGS, NULL);
>  	u8 *p = kmem_cache_alloc(s, GFP_KERNEL);
>  
>  	kmem_cache_free(s, p);
> @@ -106,7 +106,7 @@ static void test_clobber_50th_byte(struct kunit *test)
>  static void test_clobber_redzone_free(struct kunit *test)
>  {
>  	struct kmem_cache *s = kmem_cache_create("TestSlub_RZ_free", 64, 0,
> -				SLAB_RED_ZONE, NULL);
> +				SLAB_RED_ZONE|SLAB_NO_GLOBAL_FLAGS, NULL);
>  	u8 *p = kmem_cache_alloc(s, GFP_KERNEL);
>  
>  	kasan_disable_current();
> diff --git a/mm/slab.h b/mm/slab.h
> index c7f2abc2b154..69946131208a 100644
> --- a/mm/slab.h
> +++ b/mm/slab.h
> @@ -330,7 +330,7 @@ static inline slab_flags_t kmem_cache_flags(unsigned int object_size,
>  			  SLAB_ACCOUNT)
>  #elif defined(CONFIG_SLUB)
>  #define SLAB_CACHE_FLAGS (SLAB_NOLEAKTRACE | SLAB_RECLAIM_ACCOUNT | \
> -			  SLAB_TEMPORARY | SLAB_ACCOUNT)
> +			  SLAB_TEMPORARY | SLAB_ACCOUNT | SLAB_NO_GLOBAL_FLAGS)
>  #else
>  #define SLAB_CACHE_FLAGS (SLAB_NOLEAKTRACE)
>  #endif
> @@ -349,7 +349,8 @@ static inline slab_flags_t kmem_cache_flags(unsigned int object_size,
>  			      SLAB_NOLEAKTRACE | \
>  			      SLAB_RECLAIM_ACCOUNT | \
>  			      SLAB_TEMPORARY | \
> -			      SLAB_ACCOUNT)
> +			      SLAB_ACCOUNT | \
> +			      SLAB_NO_GLOBAL_FLAGS)
>  
>  bool __kmem_cache_empty(struct kmem_cache *);
>  int __kmem_cache_shutdown(struct kmem_cache *);
> diff --git a/mm/slub.c b/mm/slub.c
> index 71e8663f6037..2a3cffd7b27f 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -1592,6 +1592,9 @@ slab_flags_t kmem_cache_flags(unsigned int object_size,
>  	if (flags & SLAB_NOLEAKTRACE)
>  		slub_debug_local &= ~SLAB_STORE_USER;
>  
> +	if (flags & SLAB_NO_GLOBAL_FLAGS)
> +		slub_debug_local = 0;
> +
>  	len = strlen(name);
>  	next_block = slub_debug_string;
>  	/* Go through all blocks of debug options, see if any matches our slab's name */

[PATCH v2] mm/slub, kunit: Make slub_kunit unaffected by user specified flags

[Hyeonggon Yoo]

slub_kunit does not expect other debugging flags to be set when running
tests. When SLAB_RED_ZONE flag is set globally, test fails because the
flag affects number of errors reported.

To make slub_kunit unaffected by user specified debugging flags,
introduce SLAB_NO_USER_FLAGS to ignore them. With this flag, only flags
specified in the code are used and others are ignored.

Suggested-by: Vlastimil Babka <vbabka@suse.cz>
Signed-off-by: Hyeonggon Yoo <42.hyeyoo@gmail.com>
---
 include/linux/slab.h |  7 +++++++
 lib/slub_kunit.c     | 10 +++++-----
 mm/slab.h            |  5 +++--
 mm/slub.c            |  3 +++
 4 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/include/linux/slab.h b/include/linux/slab.h
index 373b3ef99f4e..11ceddcae9f4 100644
--- a/include/linux/slab.h
+++ b/include/linux/slab.h
@@ -112,6 +112,13 @@
 #define SLAB_KASAN		0
 #endif
 
+/*
+ * Ignore user specified debugging flags.
+ * Intended for caches created for self-tests so they have only flags
+ * specified in the code and other flags are ignored.
+ */
+#define SLAB_NO_USER_FLAGS	((slab_flags_t __force)0x10000000U)
+
 /* The following flags affect the page allocator grouping pages by mobility */
 /* Objects are reclaimable */
 #define SLAB_RECLAIM_ACCOUNT	((slab_flags_t __force)0x00020000U)
diff --git a/lib/slub_kunit.c b/lib/slub_kunit.c
index 8662dc6cb509..7a0564d7cb7a 100644
--- a/lib/slub_kunit.c
+++ b/lib/slub_kunit.c
@@ -12,7 +12,7 @@ static int slab_errors;
 static void test_clobber_zone(struct kunit *test)
 {
 	struct kmem_cache *s = kmem_cache_create("TestSlub_RZ_alloc", 64, 0,
-				SLAB_RED_ZONE, NULL);
+				SLAB_RED_ZONE|SLAB_NO_USER_FLAGS, NULL);
 	u8 *p = kmem_cache_alloc(s, GFP_KERNEL);
 
 	kasan_disable_current();
@@ -30,7 +30,7 @@ static void test_clobber_zone(struct kunit *test)
 static void test_next_pointer(struct kunit *test)
 {
 	struct kmem_cache *s = kmem_cache_create("TestSlub_next_ptr_free", 64, 0,
-				SLAB_POISON, NULL);
+				SLAB_POISON|SLAB_NO_USER_FLAGS, NULL);
 	u8 *p = kmem_cache_alloc(s, GFP_KERNEL);
 	unsigned long tmp;
 	unsigned long *ptr_addr;
@@ -75,7 +75,7 @@ static void test_next_pointer(struct kunit *test)
 static void test_first_word(struct kunit *test)
 {
 	struct kmem_cache *s = kmem_cache_create("TestSlub_1th_word_free", 64, 0,
-				SLAB_POISON, NULL);
+				SLAB_POISON|SLAB_NO_USER_FLAGS, NULL);
 	u8 *p = kmem_cache_alloc(s, GFP_KERNEL);
 
 	kmem_cache_free(s, p);
@@ -90,7 +90,7 @@ static void test_first_word(struct kunit *test)
 static void test_clobber_50th_byte(struct kunit *test)
 {
 	struct kmem_cache *s = kmem_cache_create("TestSlub_50th_word_free", 64, 0,
-				SLAB_POISON, NULL);
+				SLAB_POISON|SLAB_NO_USER_FLAGS, NULL);
 	u8 *p = kmem_cache_alloc(s, GFP_KERNEL);
 
 	kmem_cache_free(s, p);
@@ -106,7 +106,7 @@ static void test_clobber_50th_byte(struct kunit *test)
 static void test_clobber_redzone_free(struct kunit *test)
 {
 	struct kmem_cache *s = kmem_cache_create("TestSlub_RZ_free", 64, 0,
-				SLAB_RED_ZONE, NULL);
+				SLAB_RED_ZONE|SLAB_NO_USER_FLAGS, NULL);
 	u8 *p = kmem_cache_alloc(s, GFP_KERNEL);
 
 	kasan_disable_current();
diff --git a/mm/slab.h b/mm/slab.h
index fd7ae2024897..f7d018100994 100644
--- a/mm/slab.h
+++ b/mm/slab.h
@@ -331,7 +331,7 @@ static inline slab_flags_t kmem_cache_flags(unsigned int object_size,
 			  SLAB_ACCOUNT)
 #elif defined(CONFIG_SLUB)
 #define SLAB_CACHE_FLAGS (SLAB_NOLEAKTRACE | SLAB_RECLAIM_ACCOUNT | \
-			  SLAB_TEMPORARY | SLAB_ACCOUNT)
+			  SLAB_TEMPORARY | SLAB_ACCOUNT | SLAB_NO_USER_FLAGS)
 #else
 #define SLAB_CACHE_FLAGS (SLAB_NOLEAKTRACE)
 #endif
@@ -350,7 +350,8 @@ static inline slab_flags_t kmem_cache_flags(unsigned int object_size,
 			      SLAB_NOLEAKTRACE | \
 			      SLAB_RECLAIM_ACCOUNT | \
 			      SLAB_TEMPORARY | \
-			      SLAB_ACCOUNT)
+			      SLAB_ACCOUNT | \
+			      SLAB_NO_USER_FLAGS)
 
 bool __kmem_cache_empty(struct kmem_cache *);
 int __kmem_cache_shutdown(struct kmem_cache *);
diff --git a/mm/slub.c b/mm/slub.c
index 74d92aa4a3a2..4c78f5919356 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -1584,6 +1584,9 @@ slab_flags_t kmem_cache_flags(unsigned int object_size,
 	slab_flags_t block_flags;
 	slab_flags_t slub_debug_local = slub_debug;
 
+	if (flags & SLAB_NO_USER_FLAGS)
+		return flags;
+
 	/*
 	 * If the slab cache is for debugging (e.g. kmemleak) then
 	 * don't store user (stack trace) information by default,
-- 
2.32.0

Re: [PATCH] mm/slub, kunit: Make slub_kunit unaffected by global slub debugging flags

[Hyeonggon Yoo]

On Tue, Apr 05, 2022 at 12:58:53PM +0200, Vlastimil Babka wrote:
> On 3/17/22 09:10, Hyeonggon Yoo wrote:
> > slub_kunit does not expect other debugging flags to be set when running
> > tests. When SLAB_RED_ZONE flag is set globally, test fails because the
> > flag affects number of errors reported.
> > 
> > To make slub_kunit unaffected by global slub debugging flags, introduce
> > SLAB_NO_GLOBAL_FLAGS to ignore them. It's still allowed to specify
> > debugging flags by specifying cache name(s) in slub_debug parameter.
> 
> Given how we support globbing, I think it would be safest to just ignore
> everything that comes from slub_debug parameter when the
> SLAB_NO_GLOBAL_FLAGS flag is specified, even if it involves a (partial)
> cache name match.
> Maybe name it SLAB_NO_USER_FLAGS then?
> 

Seems reasonable. letting your specify debugging flags for self-test
cache is not useful for any case.

Let's make self-test caches completely isolated from user-specified flags.

I sent v2 with your comments in mind.

> > Suggested-by: Vlastimil Babka <vbabka@suse.cz>
> > Signed-off-by: Hyeonggon Yoo <42.hyeyoo@gmail.com>
> > ---
> >  include/linux/slab.h |  3 +++
> >  lib/slub_kunit.c     | 10 +++++-----
> >  mm/slab.h            |  5 +++--
> >  mm/slub.c            |  3 +++
> >  4 files changed, 14 insertions(+), 7 deletions(-)
> > 
> > diff --git a/include/linux/slab.h b/include/linux/slab.h
> > index 0381868e5118..11fe2c28422d 100644
> > --- a/include/linux/slab.h
> > +++ b/include/linux/slab.h
> > @@ -112,6 +112,9 @@
> >  #define SLAB_KASAN		0
> >  #endif
> >  
> > +/* Ignore globally specified debugging flags */
> 
> I'd add that this is intended for caches created for self-tests so they
> always have flags as specified in the code.
>

Did it in v2.

Thanks!
Hyeonggon

> > +#define SLAB_NO_GLOBAL_FLAGS	((slab_flags_t __force)0x10000000U)
> > +
> >  /* The following flags affect the page allocator grouping pages by mobility */
> >  /* Objects are reclaimable */
> >  #define SLAB_RECLAIM_ACCOUNT	((slab_flags_t __force)0x00020000U)
> > diff --git a/lib/slub_kunit.c b/lib/slub_kunit.c
> > index 8662dc6cb509..acf061dc558d 100644
> > --- a/lib/slub_kunit.c
> > +++ b/lib/slub_kunit.c
> > @@ -12,7 +12,7 @@ static int slab_errors;
> >  static void test_clobber_zone(struct kunit *test)
> >  {
> >  	struct kmem_cache *s = kmem_cache_create("TestSlub_RZ_alloc", 64, 0,
> > -				SLAB_RED_ZONE, NULL);
> > +				SLAB_RED_ZONE|SLAB_NO_GLOBAL_FLAGS, NULL);
> >  	u8 *p = kmem_cache_alloc(s, GFP_KERNEL);
> >  
> >  	kasan_disable_current();
> > @@ -30,7 +30,7 @@ static void test_clobber_zone(struct kunit *test)
> >  static void test_next_pointer(struct kunit *test)
> >  {
> >  	struct kmem_cache *s = kmem_cache_create("TestSlub_next_ptr_free", 64, 0,
> > -				SLAB_POISON, NULL);
> > +				SLAB_POISON|SLAB_NO_GLOBAL_FLAGS, NULL);
> >  	u8 *p = kmem_cache_alloc(s, GFP_KERNEL);
> >  	unsigned long tmp;
> >  	unsigned long *ptr_addr;
> > @@ -75,7 +75,7 @@ static void test_next_pointer(struct kunit *test)
> >  static void test_first_word(struct kunit *test)
> >  {
> >  	struct kmem_cache *s = kmem_cache_create("TestSlub_1th_word_free", 64, 0,
> > -				SLAB_POISON, NULL);
> > +				SLAB_POISON|SLAB_NO_GLOBAL_FLAGS, NULL);
> >  	u8 *p = kmem_cache_alloc(s, GFP_KERNEL);
> >  
> >  	kmem_cache_free(s, p);
> > @@ -90,7 +90,7 @@ static void test_first_word(struct kunit *test)
> >  static void test_clobber_50th_byte(struct kunit *test)
> >  {
> >  	struct kmem_cache *s = kmem_cache_create("TestSlub_50th_word_free", 64, 0,
> > -				SLAB_POISON, NULL);
> > +				SLAB_POISON|SLAB_NO_GLOBAL_FLAGS, NULL);
> >  	u8 *p = kmem_cache_alloc(s, GFP_KERNEL);
> >  
> >  	kmem_cache_free(s, p);
> > @@ -106,7 +106,7 @@ static void test_clobber_50th_byte(struct kunit *test)
> >  static void test_clobber_redzone_free(struct kunit *test)
> >  {
> >  	struct kmem_cache *s = kmem_cache_create("TestSlub_RZ_free", 64, 0,
> > -				SLAB_RED_ZONE, NULL);
> > +				SLAB_RED_ZONE|SLAB_NO_GLOBAL_FLAGS, NULL);
> >  	u8 *p = kmem_cache_alloc(s, GFP_KERNEL);
> >  
> >  	kasan_disable_current();
> > diff --git a/mm/slab.h b/mm/slab.h
> > index c7f2abc2b154..69946131208a 100644
> > --- a/mm/slab.h
> > +++ b/mm/slab.h
> > @@ -330,7 +330,7 @@ static inline slab_flags_t kmem_cache_flags(unsigned int object_size,
> >  			  SLAB_ACCOUNT)
> >  #elif defined(CONFIG_SLUB)
> >  #define SLAB_CACHE_FLAGS (SLAB_NOLEAKTRACE | SLAB_RECLAIM_ACCOUNT | \
> > -			  SLAB_TEMPORARY | SLAB_ACCOUNT)
> > +			  SLAB_TEMPORARY | SLAB_ACCOUNT | SLAB_NO_GLOBAL_FLAGS)
> >  #else
> >  #define SLAB_CACHE_FLAGS (SLAB_NOLEAKTRACE)
> >  #endif
> > @@ -349,7 +349,8 @@ static inline slab_flags_t kmem_cache_flags(unsigned int object_size,
> >  			      SLAB_NOLEAKTRACE | \
> >  			      SLAB_RECLAIM_ACCOUNT | \
> >  			      SLAB_TEMPORARY | \
> > -			      SLAB_ACCOUNT)
> > +			      SLAB_ACCOUNT | \
> > +			      SLAB_NO_GLOBAL_FLAGS)
> >  
> >  bool __kmem_cache_empty(struct kmem_cache *);
> >  int __kmem_cache_shutdown(struct kmem_cache *);
> > diff --git a/mm/slub.c b/mm/slub.c
> > index 71e8663f6037..2a3cffd7b27f 100644
> > --- a/mm/slub.c
> > +++ b/mm/slub.c
> > @@ -1592,6 +1592,9 @@ slab_flags_t kmem_cache_flags(unsigned int object_size,
> >  	if (flags & SLAB_NOLEAKTRACE)
> >  		slub_debug_local &= ~SLAB_STORE_USER;
> >  
> > +	if (flags & SLAB_NO_GLOBAL_FLAGS)
> > +		slub_debug_local = 0;
> > +
> >  	len = strlen(name);
> >  	next_block = slub_debug_string;
> >  	/* Go through all blocks of debug options, see if any matches our slab's name */
> 

Re: [PATCH v2] mm/slub, kunit: Make slub_kunit unaffected by user specified flags

[Vlastimil Babka]

On 4/6/22 08:00, Hyeonggon Yoo wrote:
> slub_kunit does not expect other debugging flags to be set when running
> tests. When SLAB_RED_ZONE flag is set globally, test fails because the
> flag affects number of errors reported.
> 
> To make slub_kunit unaffected by user specified debugging flags,
> introduce SLAB_NO_USER_FLAGS to ignore them. With this flag, only flags
> specified in the code are used and others are ignored.
> 
> Suggested-by: Vlastimil Babka <vbabka@suse.cz>
> Signed-off-by: Hyeonggon Yoo <42.hyeyoo@gmail.com>

Thanks, applied.

> ---
>  include/linux/slab.h |  7 +++++++
>  lib/slub_kunit.c     | 10 +++++-----
>  mm/slab.h            |  5 +++--
>  mm/slub.c            |  3 +++
>  4 files changed, 18 insertions(+), 7 deletions(-)
> 
> diff --git a/include/linux/slab.h b/include/linux/slab.h
> index 373b3ef99f4e..11ceddcae9f4 100644
> --- a/include/linux/slab.h
> +++ b/include/linux/slab.h
> @@ -112,6 +112,13 @@
>  #define SLAB_KASAN		0
>  #endif
>  
> +/*
> + * Ignore user specified debugging flags.
> + * Intended for caches created for self-tests so they have only flags
> + * specified in the code and other flags are ignored.
> + */
> +#define SLAB_NO_USER_FLAGS	((slab_flags_t __force)0x10000000U)
> +
>  /* The following flags affect the page allocator grouping pages by mobility */
>  /* Objects are reclaimable */
>  #define SLAB_RECLAIM_ACCOUNT	((slab_flags_t __force)0x00020000U)
> diff --git a/lib/slub_kunit.c b/lib/slub_kunit.c
> index 8662dc6cb509..7a0564d7cb7a 100644
> --- a/lib/slub_kunit.c
> +++ b/lib/slub_kunit.c
> @@ -12,7 +12,7 @@ static int slab_errors;
>  static void test_clobber_zone(struct kunit *test)
>  {
>  	struct kmem_cache *s = kmem_cache_create("TestSlub_RZ_alloc", 64, 0,
> -				SLAB_RED_ZONE, NULL);
> +				SLAB_RED_ZONE|SLAB_NO_USER_FLAGS, NULL);
>  	u8 *p = kmem_cache_alloc(s, GFP_KERNEL);
>  
>  	kasan_disable_current();
> @@ -30,7 +30,7 @@ static void test_clobber_zone(struct kunit *test)
>  static void test_next_pointer(struct kunit *test)
>  {
>  	struct kmem_cache *s = kmem_cache_create("TestSlub_next_ptr_free", 64, 0,
> -				SLAB_POISON, NULL);
> +				SLAB_POISON|SLAB_NO_USER_FLAGS, NULL);
>  	u8 *p = kmem_cache_alloc(s, GFP_KERNEL);
>  	unsigned long tmp;
>  	unsigned long *ptr_addr;
> @@ -75,7 +75,7 @@ static void test_next_pointer(struct kunit *test)
>  static void test_first_word(struct kunit *test)
>  {
>  	struct kmem_cache *s = kmem_cache_create("TestSlub_1th_word_free", 64, 0,
> -				SLAB_POISON, NULL);
> +				SLAB_POISON|SLAB_NO_USER_FLAGS, NULL);
>  	u8 *p = kmem_cache_alloc(s, GFP_KERNEL);
>  
>  	kmem_cache_free(s, p);
> @@ -90,7 +90,7 @@ static void test_first_word(struct kunit *test)
>  static void test_clobber_50th_byte(struct kunit *test)
>  {
>  	struct kmem_cache *s = kmem_cache_create("TestSlub_50th_word_free", 64, 0,
> -				SLAB_POISON, NULL);
> +				SLAB_POISON|SLAB_NO_USER_FLAGS, NULL);
>  	u8 *p = kmem_cache_alloc(s, GFP_KERNEL);
>  
>  	kmem_cache_free(s, p);
> @@ -106,7 +106,7 @@ static void test_clobber_50th_byte(struct kunit *test)
>  static void test_clobber_redzone_free(struct kunit *test)
>  {
>  	struct kmem_cache *s = kmem_cache_create("TestSlub_RZ_free", 64, 0,
> -				SLAB_RED_ZONE, NULL);
> +				SLAB_RED_ZONE|SLAB_NO_USER_FLAGS, NULL);
>  	u8 *p = kmem_cache_alloc(s, GFP_KERNEL);
>  
>  	kasan_disable_current();
> diff --git a/mm/slab.h b/mm/slab.h
> index fd7ae2024897..f7d018100994 100644
> --- a/mm/slab.h
> +++ b/mm/slab.h
> @@ -331,7 +331,7 @@ static inline slab_flags_t kmem_cache_flags(unsigned int object_size,
>  			  SLAB_ACCOUNT)
>  #elif defined(CONFIG_SLUB)
>  #define SLAB_CACHE_FLAGS (SLAB_NOLEAKTRACE | SLAB_RECLAIM_ACCOUNT | \
> -			  SLAB_TEMPORARY | SLAB_ACCOUNT)
> +			  SLAB_TEMPORARY | SLAB_ACCOUNT | SLAB_NO_USER_FLAGS)
>  #else
>  #define SLAB_CACHE_FLAGS (SLAB_NOLEAKTRACE)
>  #endif
> @@ -350,7 +350,8 @@ static inline slab_flags_t kmem_cache_flags(unsigned int object_size,
>  			      SLAB_NOLEAKTRACE | \
>  			      SLAB_RECLAIM_ACCOUNT | \
>  			      SLAB_TEMPORARY | \
> -			      SLAB_ACCOUNT)
> +			      SLAB_ACCOUNT | \
> +			      SLAB_NO_USER_FLAGS)
>  
>  bool __kmem_cache_empty(struct kmem_cache *);
>  int __kmem_cache_shutdown(struct kmem_cache *);
> diff --git a/mm/slub.c b/mm/slub.c
> index 74d92aa4a3a2..4c78f5919356 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -1584,6 +1584,9 @@ slab_flags_t kmem_cache_flags(unsigned int object_size,
>  	slab_flags_t block_flags;
>  	slab_flags_t slub_debug_local = slub_debug;
>  
> +	if (flags & SLAB_NO_USER_FLAGS)
> +		return flags;
> +
>  	/*
>  	 * If the slab cache is for debugging (e.g. kmemleak) then
>  	 * don't store user (stack trace) information by default,