Hello,
This series includes minor fix and cleanup of slub_debug, please see
the commits for details.
Signed-off-by: Chengming Zhou <[email protected]>
---
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 <[email protected]>
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
>
> Signed-off-by: Chengming Zhou <[email protected]>
> ---
> 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
>
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.
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.
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.
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.
>
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 <[email protected]>
Hm right, only SLAB was using those. Thanks.
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 <[email protected]>
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) {
>
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 <[email protected]>
>
> 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) {
>>
>