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

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

[Chengming Zhou]

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: check the return value of check_bytes_and_report()
      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                    | 11 ++++++-----
 tools/include/linux/poison.h |  7 ++-----
 3 files changed, 10 insertions(+), 15 deletions(-)
---
base-commit: 1613e604df0cd359cf2a7fbd9be7a0bcfacfabd0
change-id: 20240528-b4-slab-debug-1d8179fc996a

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

[PATCH 1/3] slab: check the return value of check_bytes_and_report()

[Chengming Zhou]

All check_bytes_and_report() callers have checked its return value except
for this one. We should also check its return value here obviously, so
fix it.

Signed-off-by: Chengming Zhou <chengming.zhou@linux.dev>
---
 mm/slub.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/mm/slub.c b/mm/slub.c
index 0809760cf789..de57512734ac 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -1324,9 +1324,10 @@ static int check_object(struct kmem_cache *s, struct slab *slab,
 		}
 	} 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))
+				return 0;
 		}
 	}
 

-- 
2.45.1

Re: [PATCH 1/3] slab: check the return value of check_bytes_and_report()

[Christoph Lameter (Ampere)]

On Tue, 28 May 2024, Chengming Zhou wrote:

> diff --git a/mm/slub.c b/mm/slub.c
> index 0809760cf789..de57512734ac 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -1324,9 +1324,10 @@ static int check_object(struct kmem_cache *s, struct slab *slab,
> 		}
> 	} 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))
> +				return 0;
> 		}
> 	}

This change means we will then skip the rest of the checks in 
check_object() such as the poison check.

Re: [PATCH 1/3] slab: check the return value of check_bytes_and_report()

[Chengming Zhou]

On 2024/5/30 23:20, Christoph Lameter (Ampere) wrote:
> On Tue, 28 May 2024, Chengming Zhou wrote:
> 
>> diff --git a/mm/slub.c b/mm/slub.c
>> index 0809760cf789..de57512734ac 100644
>> --- a/mm/slub.c
>> +++ b/mm/slub.c
>> @@ -1324,9 +1324,10 @@ static int check_object(struct kmem_cache *s, struct slab *slab,
>>         }
>>     } 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))
>> +                return 0;
>>         }
>>     }
> 
> This change means we will then skip the rest of the checks in check_object() such as the poison check.

Yeah, only when this padding checking failed.

Now, we always abort checking and return 0 when the first checking error happens,
such as redzones checking above.

Thanks.

Re: [PATCH 1/3] slab: check the return value of check_bytes_and_report()

[Vlastimil Babka]

On 5/31/24 10:31 AM, Chengming Zhou wrote:
> On 2024/5/30 23:20, Christoph Lameter (Ampere) wrote:
>> On Tue, 28 May 2024, Chengming Zhou wrote:
>> 
>>> diff --git a/mm/slub.c b/mm/slub.c
>>> index 0809760cf789..de57512734ac 100644
>>> --- a/mm/slub.c
>>> +++ b/mm/slub.c
>>> @@ -1324,9 +1324,10 @@ static int check_object(struct kmem_cache *s, struct slab *slab,
>>>         }
>>>     } 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))
>>> +                return 0;
>>>         }
>>>     }
>> 
>> This change means we will then skip the rest of the checks in check_object() such as the poison check.
> 
> Yeah, only when this padding checking failed.
> 
> Now, we always abort checking and return 0 when the first checking error happens,
> such as redzones checking above.

Yes your patch will make it consistent. But IMHO it would be better 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(). Thoughts?

> Thanks.

Re: [PATCH 1/3] slab: check the return value of check_bytes_and_report()

[Chengming Zhou]

On 2024/6/3 15:46, Vlastimil Babka wrote:
> On 5/31/24 10:31 AM, Chengming Zhou wrote:
>> On 2024/5/30 23:20, Christoph Lameter (Ampere) wrote:
>>> On Tue, 28 May 2024, Chengming Zhou wrote:
>>>
>>>> diff --git a/mm/slub.c b/mm/slub.c
>>>> index 0809760cf789..de57512734ac 100644
>>>> --- a/mm/slub.c
>>>> +++ b/mm/slub.c
>>>> @@ -1324,9 +1324,10 @@ static int check_object(struct kmem_cache *s, struct slab *slab,
>>>>         }
>>>>     } 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))
>>>> +                return 0;
>>>>         }
>>>>     }
>>>
>>> This change means we will then skip the rest of the checks in check_object() such as the poison check.
>>
>> Yeah, only when this padding checking failed.
>>
>> Now, we always abort checking and return 0 when the first checking error happens,
>> such as redzones checking above.
> 
> Yes your patch will make it consistent. But IMHO it would be better 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(). Thoughts?

Ok, it's feasible, will change to this.

> 
>> Thanks.
> 

[PATCH 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.

Signed-off-by: Chengming Zhou <chengming.zhou@linux.dev>
---
 mm/slub.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/slub.c b/mm/slub.c
index de57512734ac..b92d9a557852 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -5150,7 +5150,7 @@ static int calculate_sizes(struct kmem_cache *s)
 	 */
 	s->inuse = size;
 
-	if (slub_debug_orig_size(s) ||
+	if (((flags & SLAB_RED_ZONE) && slub_debug_orig_size(s)) ||
 	    (flags & (SLAB_TYPESAFE_BY_RCU | SLAB_POISON)) ||
 	    ((flags & SLAB_RED_ZONE) && s->object_size < sizeof(void *)) ||
 	    s->ctor) {

-- 
2.45.1

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

[Feng Tang]

On Tue, May 28, 2024 at 03:16:47PM +0800, 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.

Look good to me. Thanks!

Reviewed-by: Feng Tang <feng.tang@intel.com>

> 
> Signed-off-by: Chengming Zhou <chengming.zhou@linux.dev>
> ---
>  mm/slub.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/slub.c b/mm/slub.c
> index de57512734ac..b92d9a557852 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -5150,7 +5150,7 @@ static int calculate_sizes(struct kmem_cache *s)
>  	 */
>  	s->inuse = size;
>  
> -	if (slub_debug_orig_size(s) ||
> +	if (((flags & SLAB_RED_ZONE) && slub_debug_orig_size(s)) ||
>  	    (flags & (SLAB_TYPESAFE_BY_RCU | SLAB_POISON)) ||
>  	    ((flags & SLAB_RED_ZONE) && s->object_size < sizeof(void *)) ||
>  	    s->ctor) {
> 
> -- 
> 2.45.1
> 

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

[Vlastimil Babka]

On 5/28/24 9:16 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.
> 
> Signed-off-by: Chengming Zhou <chengming.zhou@linux.dev>

Seems OK.

> ---
>  mm/slub.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/slub.c b/mm/slub.c
> index de57512734ac..b92d9a557852 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -5150,7 +5150,7 @@ static int calculate_sizes(struct kmem_cache *s)
>  	 */
>  	s->inuse = size;
>  
> -	if (slub_debug_orig_size(s) ||
> +	if (((flags & SLAB_RED_ZONE) && slub_debug_orig_size(s)) ||
>  	    (flags & (SLAB_TYPESAFE_BY_RCU | SLAB_POISON)) ||
>  	    ((flags & SLAB_RED_ZONE) && s->object_size < sizeof(void *)) ||

Should we consolidate the two cases with flags & SLAB_RED_ZONE?

Also below this is a comment that could also mention the slub_debug_orig_size().

>  	    s->ctor) {
> 

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

[Chengming Zhou]

On 2024/6/3 17:25, Vlastimil Babka wrote:
> On 5/28/24 9:16 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.
>>
>> Signed-off-by: Chengming Zhou <chengming.zhou@linux.dev>
> 
> Seems OK.
> 
>> ---
>>  mm/slub.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/mm/slub.c b/mm/slub.c
>> index de57512734ac..b92d9a557852 100644
>> --- a/mm/slub.c
>> +++ b/mm/slub.c
>> @@ -5150,7 +5150,7 @@ static int calculate_sizes(struct kmem_cache *s)
>>  	 */
>>  	s->inuse = size;
>>  
>> -	if (slub_debug_orig_size(s) ||
>> +	if (((flags & SLAB_RED_ZONE) && slub_debug_orig_size(s)) ||
>>  	    (flags & (SLAB_TYPESAFE_BY_RCU | SLAB_POISON)) ||
>>  	    ((flags & SLAB_RED_ZONE) && s->object_size < sizeof(void *)) ||
> 
> Should we consolidate the two cases with flags & SLAB_RED_ZONE?

Yes, we can.

> 
> Also below this is a comment that could also mention the slub_debug_orig_size().

Ok, will add.

Thanks.

> 
>>  	    s->ctor) {
>>
> 

[PATCH 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 b92d9a557852..9af868fa68a4 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -1216,8 +1216,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 3/3] slab: delete useless RED_INACTIVE and RED_ACTIVE

[Vlastimil Babka]

On 5/28/24 9:16 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>

Hm right, only SLAB was using those. Thanks.