[PATCH v2 0/3] slab: fix and cleanup of slub_debug

[PATCH v2 0/3] slab: fix and cleanup of slub_debug

[Chengming Zhou]

Changes in v2:
- Change check_object() to do all the checks without skipping, report
  their specific error findings in check_bytes_and_report() but not
  print_trailer(). Once all checks were done, if any found an error,
  print the trailer once from check_object(), suggested by Vlastimil.
- Consolidate the two cases with flags & SLAB_RED_ZONE and make the
  complex conditional expressions a little prettier and add comments
  about extending right redzone, per Vlastimil.
- Add Reviewed-by from Feng Tang.
- Link to v1: https://lore.kernel.org/r/20240528-b4-slab-debug-v1-0-8694ef4802df@linux.dev

Hello,

This series includes minor fix and cleanup of slub_debug, please see
the commits for details.

Signed-off-by: Chengming Zhou <chengming.zhou@linux.dev>
---
Chengming Zhou (3):
      slab: make check_object() more consistent
      slab: don't put freepointer outside of object if only orig_size
      slab: delete useless RED_INACTIVE and RED_ACTIVE

 include/linux/poison.h       |  7 ++----
 mm/slub.c                    | 60 +++++++++++++++++++++++---------------------
 tools/include/linux/poison.h |  7 ++----
 3 files changed, 36 insertions(+), 38 deletions(-)
---
base-commit: 1613e604df0cd359cf2a7fbd9be7a0bcfacfabd0
change-id: 20240528-b4-slab-debug-1d8179fc996a

Best regards,
-- 
Chengming Zhou <chengming.zhou@linux.dev>

[PATCH v2 1/3] slab: make check_object() more consistent

[Chengming Zhou]

Now check_object() calls check_bytes_and_report() multiple times to
check every section of the object it cares about, like left and right
redzones, object poison, paddings poison and freepointer. It will
abort the checking process and return 0 once it finds an error.

There are two inconsistencies in check_object(), which are alignment
padding checking and object padding checking. We only print the error
messages but don't return 0 to tell callers that something is wrong
and needs to be handled. Please see alloc_debug_processing() and
free_debug_processing() for details.

If the above inconsistencies are not intentional, we should fix it.
And we want to do all checks without skipping, so use a local variable
"ret" to save each check result and change check_bytes_and_report() to
only report specific error findings. Then at end of check_object(),
print the trailer once if any found an error.

Suggested-by: Vlastimil Babka <vbabka@suse.cz>
Signed-off-by: Chengming Zhou <chengming.zhou@linux.dev>
---
 mm/slub.c | 45 ++++++++++++++++++++++++---------------------
 1 file changed, 24 insertions(+), 21 deletions(-)

diff --git a/mm/slub.c b/mm/slub.c
index 0809760cf789..7fbd5ce4320a 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -1192,8 +1192,6 @@ static int check_bytes_and_report(struct kmem_cache *s, struct slab *slab,
 	pr_err("0x%p-0x%p @offset=%tu. First byte 0x%x instead of 0x%x\n",
 					fault, end - 1, fault - addr,
 					fault[0], value);
-	print_trailer(s, slab, object);
-	add_taint(TAINT_BAD_PAGE, LOCKDEP_NOW_UNRELIABLE);
 
 skip_bug_print:
 	restore_bytes(s, what, value, fault, end);
@@ -1302,15 +1300,16 @@ static int check_object(struct kmem_cache *s, struct slab *slab,
 	u8 *p = object;
 	u8 *endobject = object + s->object_size;
 	unsigned int orig_size, kasan_meta_size;
+	int ret = 1;
 
 	if (s->flags & SLAB_RED_ZONE) {
 		if (!check_bytes_and_report(s, slab, object, "Left Redzone",
 			object - s->red_left_pad, val, s->red_left_pad))
-			return 0;
+			ret = 0;
 
 		if (!check_bytes_and_report(s, slab, object, "Right Redzone",
 			endobject, val, s->inuse - s->object_size))
-			return 0;
+			ret = 0;
 
 		if (slub_debug_orig_size(s) && val == SLUB_RED_ACTIVE) {
 			orig_size = get_orig_size(s, object);
@@ -1319,14 +1318,15 @@ static int check_object(struct kmem_cache *s, struct slab *slab,
 				!check_bytes_and_report(s, slab, object,
 					"kmalloc Redzone", p + orig_size,
 					val, s->object_size - orig_size)) {
-				return 0;
+				ret = 0;
 			}
 		}
 	} else {
 		if ((s->flags & SLAB_POISON) && s->object_size < s->inuse) {
-			check_bytes_and_report(s, slab, p, "Alignment padding",
+			if (!check_bytes_and_report(s, slab, p, "Alignment padding",
 				endobject, POISON_INUSE,
-				s->inuse - s->object_size);
+				s->inuse - s->object_size))
+				ret = 0;
 		}
 	}
 
@@ -1342,27 +1342,25 @@ static int check_object(struct kmem_cache *s, struct slab *slab,
 			    !check_bytes_and_report(s, slab, p, "Poison",
 					p + kasan_meta_size, POISON_FREE,
 					s->object_size - kasan_meta_size - 1))
-				return 0;
+				ret = 0;
 			if (kasan_meta_size < s->object_size &&
 			    !check_bytes_and_report(s, slab, p, "End Poison",
 					p + s->object_size - 1, POISON_END, 1))
-				return 0;
+				ret = 0;
 		}
 		/*
 		 * check_pad_bytes cleans up on its own.
 		 */
-		check_pad_bytes(s, slab, p);
+		if (!check_pad_bytes(s, slab, p))
+			ret = 0;
 	}
 
-	if (!freeptr_outside_object(s) && val == SLUB_RED_ACTIVE)
-		/*
-		 * Object and freepointer overlap. Cannot check
-		 * freepointer while object is allocated.
-		 */
-		return 1;
-
-	/* Check free pointer validity */
-	if (!check_valid_pointer(s, slab, get_freepointer(s, p))) {
+	/*
+	 * Cannot check freepointer while object is allocated if
+	 * object and freepointer overlap.
+	 */
+	if (!freeptr_outside_object(s) && val == SLUB_RED_ACTIVE &&
+	    !check_valid_pointer(s, slab, get_freepointer(s, p))) {
 		object_err(s, slab, p, "Freepointer corrupt");
 		/*
 		 * No choice but to zap it and thus lose the remainder
@@ -1370,9 +1368,14 @@ static int check_object(struct kmem_cache *s, struct slab *slab,
 		 * another error because the object count is now wrong.
 		 */
 		set_freepointer(s, p, NULL);
-		return 0;
 	}
-	return 1;
+
+	if (!ret && !slab_add_kunit_errors()) {
+		print_trailer(s, slab, object);
+		add_taint(TAINT_BAD_PAGE, LOCKDEP_NOW_UNRELIABLE);
+	}
+
+	return ret;
 }
 
 static int check_slab(struct kmem_cache *s, struct slab *slab)

-- 
2.45.1

[PATCH v2 2/3] slab: don't put freepointer outside of object if only orig_size

[Chengming Zhou]

The commit 946fa0dbf2d8 ("mm/slub: extend redzone check to extra
allocated kmalloc space than requested") will extend right redzone
when allocating for orig_size < object_size. So we can't overlay the
freepointer in the object space in this case.

But the code looks like it forgot to check SLAB_RED_ZONE, since there
won't be extended right redzone if only orig_size enabled.

As we are here, make this complex conditional expressions a little
prettier and add some comments about extending right redzone when
slub_debug_orig_size() enabled.

Reviewed-by: Feng Tang <feng.tang@intel.com>
Signed-off-by: Chengming Zhou <chengming.zhou@linux.dev>
---
 mm/slub.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/mm/slub.c b/mm/slub.c
index 7fbd5ce4320a..704c662227e6 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -5152,10 +5152,9 @@ static int calculate_sizes(struct kmem_cache *s)
 	 */
 	s->inuse = size;
 
-	if (slub_debug_orig_size(s) ||
-	    (flags & (SLAB_TYPESAFE_BY_RCU | SLAB_POISON)) ||
-	    ((flags & SLAB_RED_ZONE) && s->object_size < sizeof(void *)) ||
-	    s->ctor) {
+	if ((flags & (SLAB_TYPESAFE_BY_RCU | SLAB_POISON)) || s->ctor ||
+	    ((flags & SLAB_RED_ZONE) &&
+	     (s->object_size < sizeof(void *) || slub_debug_orig_size(s)))) {
 		/*
 		 * Relocate free pointer after the object if it is not
 		 * permitted to overwrite the first word of the object on
@@ -5163,7 +5162,9 @@ static int calculate_sizes(struct kmem_cache *s)
 		 *
 		 * This is the case if we do RCU, have a constructor or
 		 * destructor, are poisoning the objects, or are
-		 * redzoning an object smaller than sizeof(void *).
+		 * redzoning an object smaller than sizeof(void *) or are
+		 * redzoning an object with slub_debug_orig_size() enabled,
+		 * in which case the right redzone may be extended.
 		 *
 		 * The assumption that s->offset >= s->inuse means free
 		 * pointer is outside of the object is used in the

-- 
2.45.1

[PATCH v2 3/3] slab: delete useless RED_INACTIVE and RED_ACTIVE

[Chengming Zhou]

These seem useless since we use the SLUB_RED_INACTIVE and SLUB_RED_ACTIVE,
so just delete them, no functional change.

Signed-off-by: Chengming Zhou <chengming.zhou@linux.dev>
---
 include/linux/poison.h       | 7 ++-----
 mm/slub.c                    | 4 ++--
 tools/include/linux/poison.h | 7 ++-----
 3 files changed, 6 insertions(+), 12 deletions(-)

diff --git a/include/linux/poison.h b/include/linux/poison.h
index 1f0ee2459f2a..9c1a035af97c 100644
--- a/include/linux/poison.h
+++ b/include/linux/poison.h
@@ -38,11 +38,8 @@
  * Magic nums for obj red zoning.
  * Placed in the first word before and the first word after an obj.
  */
-#define	RED_INACTIVE	0x09F911029D74E35BULL	/* when obj is inactive */
-#define	RED_ACTIVE	0xD84156C5635688C0ULL	/* when obj is active */
-
-#define SLUB_RED_INACTIVE	0xbb
-#define SLUB_RED_ACTIVE		0xcc
+#define SLUB_RED_INACTIVE	0xbb	/* when obj is inactive */
+#define SLUB_RED_ACTIVE		0xcc	/* when obj is active */
 
 /* ...and for poisoning */
 #define	POISON_INUSE	0x5a	/* for use-uninitialised poisoning */
diff --git a/mm/slub.c b/mm/slub.c
index 704c662227e6..0bab0f041ab2 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -1214,8 +1214,8 @@ static int check_bytes_and_report(struct kmem_cache *s, struct slab *slab,
  * 	Padding is extended by another word if Redzoning is enabled and
  * 	object_size == inuse.
  *
- * 	We fill with 0xbb (RED_INACTIVE) for inactive objects and with
- * 	0xcc (RED_ACTIVE) for objects in use.
+ * 	We fill with 0xbb (SLUB_RED_INACTIVE) for inactive objects and with
+ * 	0xcc (SLUB_RED_ACTIVE) for objects in use.
  *
  * object + s->inuse
  * 	Meta data starts here.
diff --git a/tools/include/linux/poison.h b/tools/include/linux/poison.h
index 2e6338ac5eed..e530e54046c9 100644
--- a/tools/include/linux/poison.h
+++ b/tools/include/linux/poison.h
@@ -47,11 +47,8 @@
  * Magic nums for obj red zoning.
  * Placed in the first word before and the first word after an obj.
  */
-#define	RED_INACTIVE	0x09F911029D74E35BULL	/* when obj is inactive */
-#define	RED_ACTIVE	0xD84156C5635688C0ULL	/* when obj is active */
-
-#define SLUB_RED_INACTIVE	0xbb
-#define SLUB_RED_ACTIVE		0xcc
+#define SLUB_RED_INACTIVE	0xbb	/* when obj is inactive */
+#define SLUB_RED_ACTIVE		0xcc	/* when obj is active */
 
 /* ...and for poisoning */
 #define	POISON_INUSE	0x5a	/* for use-uninitialised poisoning */

-- 
2.45.1

Re: [PATCH v2 1/3] slab: make check_object() more consistent

[Vlastimil Babka]

On 6/5/24 9:13 AM, Chengming Zhou wrote:
> Now check_object() calls check_bytes_and_report() multiple times to
> check every section of the object it cares about, like left and right
> redzones, object poison, paddings poison and freepointer. It will
> abort the checking process and return 0 once it finds an error.
> 
> There are two inconsistencies in check_object(), which are alignment
> padding checking and object padding checking. We only print the error
> messages but don't return 0 to tell callers that something is wrong
> and needs to be handled. Please see alloc_debug_processing() and
> free_debug_processing() for details.
> 
> If the above inconsistencies are not intentional, we should fix it.

It doesn't seem intentional, I don't see why padding specifically would be
different from the other tests here.

<snip>

> -	if (!freeptr_outside_object(s) && val == SLUB_RED_ACTIVE)
> -		/*
> -		 * Object and freepointer overlap. Cannot check
> -		 * freepointer while object is allocated.
> -		 */
> -		return 1;
> -
> -	/* Check free pointer validity */
> -	if (!check_valid_pointer(s, slab, get_freepointer(s, p))) {
> +	/*
> +	 * Cannot check freepointer while object is allocated if
> +	 * object and freepointer overlap.
> +	 */
> +	if (!freeptr_outside_object(s) && val == SLUB_RED_ACTIVE &&

Seems this condition should have been logically flipped?

> +	    !check_valid_pointer(s, slab, get_freepointer(s, p))) {
>  		object_err(s, slab, p, "Freepointer corrupt");
>  		/*
>  		 * No choice but to zap it and thus lose the remainder
> @@ -1370,9 +1368,14 @@ static int check_object(struct kmem_cache *s, struct slab *slab,
>  		 * another error because the object count is now wrong.
>  		 */
>  		set_freepointer(s, p, NULL);
> -		return 0;

Should set ret = 0 here?

>  	}
> -	return 1;
> +
> +	if (!ret && !slab_add_kunit_errors()) {

Also 5/6 of slub_kunit tests now fail as we increased the number of recorded
errors vs expected. Either the slab_add_kunit_errors() test above should
have a variant (parameter?) so it will only detect we are in slab-kunit test
(to suppress the printing and taint) but doesn't increase slab_errors (we
increased them for the individual issues already), or simply raise the
expectations of the tests so it matches the new implementation.

Thanks,
Vlastimil

> +		print_trailer(s, slab, object);
> +		add_taint(TAINT_BAD_PAGE, LOCKDEP_NOW_UNRELIABLE);
> +	}
> +
> +	return ret;
>  }
>  
>  static int check_slab(struct kmem_cache *s, struct slab *slab)
> 

Re: [PATCH v2 2/3] slab: don't put freepointer outside of object if only orig_size

[Vlastimil Babka]

On 6/5/24 9:13 AM, Chengming Zhou wrote:
> The commit 946fa0dbf2d8 ("mm/slub: extend redzone check to extra
> allocated kmalloc space than requested") will extend right redzone
> when allocating for orig_size < object_size. So we can't overlay the
> freepointer in the object space in this case.
> 
> But the code looks like it forgot to check SLAB_RED_ZONE, since there
> won't be extended right redzone if only orig_size enabled.
> 
> As we are here, make this complex conditional expressions a little
> prettier and add some comments about extending right redzone when
> slub_debug_orig_size() enabled.
> 
> Reviewed-by: Feng Tang <feng.tang@intel.com>
> Signed-off-by: Chengming Zhou <chengming.zhou@linux.dev>

Reviewed-by: Vlastimil Babka <vbabka@suse.cz>

> ---
>  mm/slub.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/mm/slub.c b/mm/slub.c
> index 7fbd5ce4320a..704c662227e6 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -5152,10 +5152,9 @@ static int calculate_sizes(struct kmem_cache *s)
>  	 */
>  	s->inuse = size;
>  
> -	if (slub_debug_orig_size(s) ||
> -	    (flags & (SLAB_TYPESAFE_BY_RCU | SLAB_POISON)) ||
> -	    ((flags & SLAB_RED_ZONE) && s->object_size < sizeof(void *)) ||
> -	    s->ctor) {
> +	if ((flags & (SLAB_TYPESAFE_BY_RCU | SLAB_POISON)) || s->ctor ||
> +	    ((flags & SLAB_RED_ZONE) &&
> +	     (s->object_size < sizeof(void *) || slub_debug_orig_size(s)))) {
>  		/*
>  		 * Relocate free pointer after the object if it is not
>  		 * permitted to overwrite the first word of the object on
> @@ -5163,7 +5162,9 @@ static int calculate_sizes(struct kmem_cache *s)
>  		 *
>  		 * This is the case if we do RCU, have a constructor or
>  		 * destructor, are poisoning the objects, or are
> -		 * redzoning an object smaller than sizeof(void *).
> +		 * redzoning an object smaller than sizeof(void *) or are
> +		 * redzoning an object with slub_debug_orig_size() enabled,
> +		 * in which case the right redzone may be extended.
>  		 *
>  		 * The assumption that s->offset >= s->inuse means free
>  		 * pointer is outside of the object is used in the
> 

Re: [PATCH v2 3/3] slab: delete useless RED_INACTIVE and RED_ACTIVE

[Vlastimil Babka]

On 6/5/24 9:13 AM, Chengming Zhou wrote:
> These seem useless since we use the SLUB_RED_INACTIVE and SLUB_RED_ACTIVE,
> so just delete them, no functional change.
> 
> Signed-off-by: Chengming Zhou <chengming.zhou@linux.dev>

Reviewed-by: Vlastimil Babka <vbabka@suse.cz>

> ---
>  include/linux/poison.h       | 7 ++-----
>  mm/slub.c                    | 4 ++--
>  tools/include/linux/poison.h | 7 ++-----
>  3 files changed, 6 insertions(+), 12 deletions(-)
> 
> diff --git a/include/linux/poison.h b/include/linux/poison.h
> index 1f0ee2459f2a..9c1a035af97c 100644
> --- a/include/linux/poison.h
> +++ b/include/linux/poison.h
> @@ -38,11 +38,8 @@
>   * Magic nums for obj red zoning.
>   * Placed in the first word before and the first word after an obj.
>   */
> -#define	RED_INACTIVE	0x09F911029D74E35BULL	/* when obj is inactive */
> -#define	RED_ACTIVE	0xD84156C5635688C0ULL	/* when obj is active */
> -
> -#define SLUB_RED_INACTIVE	0xbb
> -#define SLUB_RED_ACTIVE		0xcc
> +#define SLUB_RED_INACTIVE	0xbb	/* when obj is inactive */
> +#define SLUB_RED_ACTIVE		0xcc	/* when obj is active */
>  
>  /* ...and for poisoning */
>  #define	POISON_INUSE	0x5a	/* for use-uninitialised poisoning */
> diff --git a/mm/slub.c b/mm/slub.c
> index 704c662227e6..0bab0f041ab2 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -1214,8 +1214,8 @@ static int check_bytes_and_report(struct kmem_cache *s, struct slab *slab,
>   * 	Padding is extended by another word if Redzoning is enabled and
>   * 	object_size == inuse.
>   *
> - * 	We fill with 0xbb (RED_INACTIVE) for inactive objects and with
> - * 	0xcc (RED_ACTIVE) for objects in use.
> + * 	We fill with 0xbb (SLUB_RED_INACTIVE) for inactive objects and with
> + * 	0xcc (SLUB_RED_ACTIVE) for objects in use.
>   *
>   * object + s->inuse
>   * 	Meta data starts here.
> diff --git a/tools/include/linux/poison.h b/tools/include/linux/poison.h
> index 2e6338ac5eed..e530e54046c9 100644
> --- a/tools/include/linux/poison.h
> +++ b/tools/include/linux/poison.h
> @@ -47,11 +47,8 @@
>   * Magic nums for obj red zoning.
>   * Placed in the first word before and the first word after an obj.
>   */
> -#define	RED_INACTIVE	0x09F911029D74E35BULL	/* when obj is inactive */
> -#define	RED_ACTIVE	0xD84156C5635688C0ULL	/* when obj is active */
> -
> -#define SLUB_RED_INACTIVE	0xbb
> -#define SLUB_RED_ACTIVE		0xcc
> +#define SLUB_RED_INACTIVE	0xbb	/* when obj is inactive */
> +#define SLUB_RED_ACTIVE		0xcc	/* when obj is active */
>  
>  /* ...and for poisoning */
>  #define	POISON_INUSE	0x5a	/* for use-uninitialised poisoning */
> 

Re: [PATCH v2 1/3] slab: make check_object() more consistent

[Chengming Zhou]

On 2024/6/6 16:28, Vlastimil Babka wrote:
> On 6/5/24 9:13 AM, Chengming Zhou wrote:
>> Now check_object() calls check_bytes_and_report() multiple times to
>> check every section of the object it cares about, like left and right
>> redzones, object poison, paddings poison and freepointer. It will
>> abort the checking process and return 0 once it finds an error.
>>
[...]
>> -	/* Check free pointer validity */
>> -	if (!check_valid_pointer(s, slab, get_freepointer(s, p))) {
>> +	/*
>> +	 * Cannot check freepointer while object is allocated if
>> +	 * object and freepointer overlap.
>> +	 */
>> +	if (!freeptr_outside_object(s) && val == SLUB_RED_ACTIVE &&
> 
> Seems this condition should have been logically flipped?

Ah, right, will fix.

> 
>> +	    !check_valid_pointer(s, slab, get_freepointer(s, p))) {
>>  		object_err(s, slab, p, "Freepointer corrupt");
>>  		/*
>>  		 * No choice but to zap it and thus lose the remainder
>> @@ -1370,9 +1368,14 @@ static int check_object(struct kmem_cache *s, struct slab *slab,
>>  		 * another error because the object count is now wrong.
>>  		 */
>>  		set_freepointer(s, p, NULL);
>> -		return 0;
> 
> Should set ret = 0 here?

Yes.

> 
>>  	}
>> -	return 1;
>> +
>> +	if (!ret && !slab_add_kunit_errors()) {
> 
> Also 5/6 of slub_kunit tests now fail as we increased the number of recorded

My bad, I didn't test with slub_kunit, will test later.

> errors vs expected. Either the slab_add_kunit_errors() test above should
> have a variant (parameter?) so it will only detect we are in slab-kunit test
> (to suppress the printing and taint) but doesn't increase slab_errors (we

I think this way is simpler for me, only suppress the printing but doesn't
increase slab_errors, will take this way and test again.

Thanks!

> increased them for the individual issues already), or simply raise the
> expectations of the tests so it matches the new implementation.
>