Hi,
Even with SLAB_RED_ZONE and SLAB_STORE_USER enabled, kernel would NOT
store redzone and last user data around allocated memory space if arch
cache line > sizeof(unsigned long long). As a result, last user information
is unexpectedly MISSED while dumping slab corruption log.
This patch makes sure that redzone and last user tags get stored whatever
arch cache line.
Compared to original codes, the change surely affects head redzone (redzone1).
Actually, with SLAB_RED_ZONE and SLAB_STORE_USER enabled,
allocated memory layout is as below:
[ redzone1 ] <--------- Affected area.
[ real object space ]
[ redzone2 ]
[ last user ]
[ ... ]
Let's do some analysis: (whatever SLAB_STORE_USER is).
1) With SLAB_RED_ZONE on, "align" >= sizeof(unsigned long long) according to
the following codes:
/* 2) arch mandated alignment */
if (ralign < ARCH_SLAB_MINALIGN) {
ralign = ARCH_SLAB_MINALIGN;
}
/* 3) caller mandated alignment */
if (ralign < align) {
ralign = align;
}
...
/*
* 4) Store it.
*/
align = ralign;
That's to say, could guarantee that redzone1 does NOT get broken
at all. Meanwhile,
Real object space could meet the need of cache line size by using
"align" argument.
2) With SLAB_RED_ZONE off, the change has no impact.
>From 03b28964311090533643acd267abe0cbc3c9b0a5 Mon Sep 17 00:00:00 2001
From: Shiyong Li <[email protected]>
Date: Fri, 2 Apr 2010 14:50:30 +0800
Subject: [PATCH] Fix missing of last user info while getting
DEBUG_SLAB config enabled.
Even with SLAB_RED_ZONE and SLAB_STORE_USER enabled, kernel would NOT
store redzone and last user data around allocated memory space if arch
cache line > sizeof(unsigned long long). As a result, last user information
is unexpectedly MISSED while dumping slab corruption log.
This fix makes sure that redzone and last user tags get stored whatever
cache line.
Signed-off-by: Shiyong Li <[email protected]>
---
mm/slab.c | 7 ++-----
1 files changed, 2 insertions(+), 5 deletions(-)
diff --git a/mm/slab.c b/mm/slab.c
index a8a38ca..84af997 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -2267,9 +2267,6 @@ kmem_cache_create (const char *name, size_t
size, size_t align,
if (ralign < align) {
ralign = align;
}
- /* disable debug if necessary */
- if (ralign > __alignof__(unsigned long long))
- flags &= ~(SLAB_RED_ZONE | SLAB_STORE_USER);
/*
* 4) Store it.
*/
@@ -2289,8 +2286,8 @@ kmem_cache_create (const char *name, size_t
size, size_t align,
*/
if (flags & SLAB_RED_ZONE) {
/* add space for red zone words */
- cachep->obj_offset += sizeof(unsigned long long);
- size += 2 * sizeof(unsigned long long);
+ cachep->obj_offset += align;
+ size += align + sizeof(unsigned long long);
}
if (flags & SLAB_STORE_USER) {
/* user store requires one word storage behind the end of
--
1.6.0.4
Thanks & Best Regards
Shiyong
Hi, Pekka Enberg
This is the updated version with format fix and comments refinement.
Let us know whether you still have comments.
--
Best Regards
Hu Tao
On Fri, Apr 2, 2010 at 3:21 PM, ShiYong LI <[email protected]> wrote:
> Hi,
>
> Even with SLAB_RED_ZONE and SLAB_STORE_USER enabled, kernel would NOT
> store redzone and last user data around allocated memory space if arch
> cache line > sizeof(unsigned long long). As a result, last user information
> is unexpectedly MISSED while dumping slab corruption log.
>
> This patch makes sure that redzone and last user tags get stored whatever
> arch cache line.
>
> Compared to original codes, the change surely affects head redzone (redzone1).
> Actually, with SLAB_RED_ZONE and SLAB_STORE_USER enabled,
> allocated memory layout is as below:
>
> [ redzone1 ] ? <--------- Affected area.
> [ real object space ]
> [ redzone2 ]
> [ last user ]
> [ ... ]
>
> Let's do some analysis: (whatever SLAB_STORE_USER is).
>
> 1) With SLAB_RED_ZONE on, "align" >= sizeof(unsigned long long) according to
> ? ?the following codes:
> ? ? ? ?/* 2) arch mandated alignment */
> ? ? ? ?if (ralign < ARCH_SLAB_MINALIGN) {
> ? ? ? ? ? ? ? ?ralign = ARCH_SLAB_MINALIGN;
> ? ? ? ?}
> ? ? ? ?/* 3) caller mandated alignment */
> ? ? ? ?if (ralign < align) {
> ? ? ? ? ? ? ? ?ralign = align;
> ? ? ? ?}
> ? ? ? ?...
> ? ? ? ?/*
> ? ? ? ? * 4) Store it.
> ? ? ? ? */
> ? ? ? ?align = ralign;
>
> ? ?That's to say, could guarantee that redzone1 does NOT get broken
> at all. Meanwhile,
> ? ?Real object space could meet the need of cache line size by using
> "align" ?argument.
>
> 2) With SLAB_RED_ZONE off, the change has no impact.
>
>
> From 03b28964311090533643acd267abe0cbc3c9b0a5 Mon Sep 17 00:00:00 2001
> From: Shiyong Li <[email protected]>
> Date: Fri, 2 Apr 2010 14:50:30 +0800
> Subject: [PATCH] Fix missing of last user info while getting
> DEBUG_SLAB config enabled.
>
> Even with SLAB_RED_ZONE and SLAB_STORE_USER enabled, kernel would NOT
> store redzone and last user data around allocated memory space if arch
> cache line > sizeof(unsigned long long). As a result, last user information
> is unexpectedly MISSED while dumping slab corruption log.
>
> This fix makes sure that redzone and last user tags get stored whatever
> cache line.
>
> Signed-off-by: Shiyong Li <[email protected]>
> ---
> ?mm/slab.c | ? ?7 ++-----
> ?1 files changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/mm/slab.c b/mm/slab.c
> index a8a38ca..84af997 100644
> --- a/mm/slab.c
> +++ b/mm/slab.c
> @@ -2267,9 +2267,6 @@ kmem_cache_create (const char *name, size_t
> size, size_t align,
> ? ? ? ?if (ralign < align) {
> ? ? ? ? ? ? ? ?ralign = align;
> ? ? ? ?}
> - ? ? ? /* disable debug if necessary */
> - ? ? ? if (ralign > __alignof__(unsigned long long))
> - ? ? ? ? ? ? ? flags &= ~(SLAB_RED_ZONE | SLAB_STORE_USER);
> ? ? ? ?/*
> ? ? ? ? * 4) Store it.
> ? ? ? ? */
> @@ -2289,8 +2286,8 @@ kmem_cache_create (const char *name, size_t
> size, size_t align,
> ? ? ? ? */
> ? ? ? ?if (flags & SLAB_RED_ZONE) {
> ? ? ? ? ? ? ? ?/* add space for red zone words */
> - ? ? ? ? ? ? ? cachep->obj_offset += sizeof(unsigned long long);
> - ? ? ? ? ? ? ? size += 2 * sizeof(unsigned long long);
> + ? ? ? ? ? ? ? cachep->obj_offset += align;
> + ? ? ? ? ? ? ? size += align + sizeof(unsigned long long);
> ? ? ? ?}
> ? ? ? ?if (flags & SLAB_STORE_USER) {
> ? ? ? ? ? ? ? ?/* user store requires one word storage behind the end of
> --
> 1.6.0.4
>
>
> Thanks & Best Regards
> Shiyong
>
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to [email protected]. ?For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
>
Hi, Pekka Enberg
Can we assume you have no objection for this patch? :)
--
Best Regards
Hu Tao
On Tue, Apr 6, 2010 at 9:24 AM, TAO HU <[email protected]> wrote:
> Hi, Pekka Enberg
>
> This is the updated version with format fix and comments refinement.
> Let us know whether you still have comments.
>
> --
> Best Regards
> Hu Tao
>
> On Fri, Apr 2, 2010 at 3:21 PM, ShiYong LI <[email protected]> wrote:
>> Hi,
>>
>> Even with SLAB_RED_ZONE and SLAB_STORE_USER enabled, kernel would NOT
>> store redzone and last user data around allocated memory space if arch
>> cache line > sizeof(unsigned long long). As a result, last user information
>> is unexpectedly MISSED while dumping slab corruption log.
>>
>> This patch makes sure that redzone and last user tags get stored whatever
>> arch cache line.
>>
>> Compared to original codes, the change surely affects head redzone (redzone1).
>> Actually, with SLAB_RED_ZONE and SLAB_STORE_USER enabled,
>> allocated memory layout is as below:
>>
>> [ redzone1 ] ? <--------- Affected area.
>> [ real object space ]
>> [ redzone2 ]
>> [ last user ]
>> [ ... ]
>>
>> Let's do some analysis: (whatever SLAB_STORE_USER is).
>>
>> 1) With SLAB_RED_ZONE on, "align" >= sizeof(unsigned long long) according to
>> ? ?the following codes:
>> ? ? ? ?/* 2) arch mandated alignment */
>> ? ? ? ?if (ralign < ARCH_SLAB_MINALIGN) {
>> ? ? ? ? ? ? ? ?ralign = ARCH_SLAB_MINALIGN;
>> ? ? ? ?}
>> ? ? ? ?/* 3) caller mandated alignment */
>> ? ? ? ?if (ralign < align) {
>> ? ? ? ? ? ? ? ?ralign = align;
>> ? ? ? ?}
>> ? ? ? ?...
>> ? ? ? ?/*
>> ? ? ? ? * 4) Store it.
>> ? ? ? ? */
>> ? ? ? ?align = ralign;
>>
>> ? ?That's to say, could guarantee that redzone1 does NOT get broken
>> at all. Meanwhile,
>> ? ?Real object space could meet the need of cache line size by using
>> "align" ?argument.
>>
>> 2) With SLAB_RED_ZONE off, the change has no impact.
>>
>>
>> From 03b28964311090533643acd267abe0cbc3c9b0a5 Mon Sep 17 00:00:00 2001
>> From: Shiyong Li <[email protected]>
>> Date: Fri, 2 Apr 2010 14:50:30 +0800
>> Subject: [PATCH] Fix missing of last user info while getting
>> DEBUG_SLAB config enabled.
>>
>> Even with SLAB_RED_ZONE and SLAB_STORE_USER enabled, kernel would NOT
>> store redzone and last user data around allocated memory space if arch
>> cache line > sizeof(unsigned long long). As a result, last user information
>> is unexpectedly MISSED while dumping slab corruption log.
>>
>> This fix makes sure that redzone and last user tags get stored whatever
>> cache line.
>>
>> Signed-off-by: Shiyong Li <[email protected]>
>> ---
>> ?mm/slab.c | ? ?7 ++-----
>> ?1 files changed, 2 insertions(+), 5 deletions(-)
>>
>> diff --git a/mm/slab.c b/mm/slab.c
>> index a8a38ca..84af997 100644
>> --- a/mm/slab.c
>> +++ b/mm/slab.c
>> @@ -2267,9 +2267,6 @@ kmem_cache_create (const char *name, size_t
>> size, size_t align,
>> ? ? ? ?if (ralign < align) {
>> ? ? ? ? ? ? ? ?ralign = align;
>> ? ? ? ?}
>> - ? ? ? /* disable debug if necessary */
>> - ? ? ? if (ralign > __alignof__(unsigned long long))
>> - ? ? ? ? ? ? ? flags &= ~(SLAB_RED_ZONE | SLAB_STORE_USER);
>> ? ? ? ?/*
>> ? ? ? ? * 4) Store it.
>> ? ? ? ? */
>> @@ -2289,8 +2286,8 @@ kmem_cache_create (const char *name, size_t
>> size, size_t align,
>> ? ? ? ? */
>> ? ? ? ?if (flags & SLAB_RED_ZONE) {
>> ? ? ? ? ? ? ? ?/* add space for red zone words */
>> - ? ? ? ? ? ? ? cachep->obj_offset += sizeof(unsigned long long);
>> - ? ? ? ? ? ? ? size += 2 * sizeof(unsigned long long);
>> + ? ? ? ? ? ? ? cachep->obj_offset += align;
>> + ? ? ? ? ? ? ? size += align + sizeof(unsigned long long);
>> ? ? ? ?}
>> ? ? ? ?if (flags & SLAB_STORE_USER) {
>> ? ? ? ? ? ? ? ?/* user store requires one word storage behind the end of
>> --
>> 1.6.0.4
>>
>>
>> Thanks & Best Regards
>> Shiyong
>>
>> --
>> To unsubscribe, send a message with 'unsubscribe linux-mm' in
>> the body to [email protected]. ?For more info on Linux MM,
>> see: http://www.linux-mm.org/ .
>> Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
>>
>
Hi,
(I'm cc'ing David who did some fixes in this area previously.)
On Fri, Apr 2, 2010 at 10:21 AM, ShiYong LI <[email protected]> wrote:
> Even with SLAB_RED_ZONE and SLAB_STORE_USER enabled, kernel would NOT
> store redzone and last user data around allocated memory space if arch
> cache line > sizeof(unsigned long long). As a result, last user information
> is unexpectedly MISSED while dumping slab corruption log.
>
> This patch makes sure that redzone and last user tags get stored whatever
> arch cache line.
>
> Compared to original codes, the change surely affects head redzone (redzone1).
> Actually, with SLAB_RED_ZONE and SLAB_STORE_USER enabled,
> allocated memory layout is as below:
>
> [ redzone1 ] ? <--------- Affected area.
> [ real object space ]
> [ redzone2 ]
> [ last user ]
> [ ... ]
>
> Let's do some analysis: (whatever SLAB_STORE_USER is).
>
> 1) With SLAB_RED_ZONE on, "align" >= sizeof(unsigned long long) according to
> ? ?the following codes:
> ? ? ? ?/* 2) arch mandated alignment */
> ? ? ? ?if (ralign < ARCH_SLAB_MINALIGN) {
> ? ? ? ? ? ? ? ?ralign = ARCH_SLAB_MINALIGN;
> ? ? ? ?}
> ? ? ? ?/* 3) caller mandated alignment */
> ? ? ? ?if (ralign < align) {
> ? ? ? ? ? ? ? ?ralign = align;
> ? ? ? ?}
> ? ? ? ?...
> ? ? ? ?/*
> ? ? ? ? * 4) Store it.
> ? ? ? ? */
> ? ? ? ?align = ralign;
>
> ? ?That's to say, could guarantee that redzone1 does NOT get broken
> at all. Meanwhile,
> ? ?Real object space could meet the need of cache line size by using
> "align" ?argument.
>
> 2) With SLAB_RED_ZONE off, the change has no impact.
>
>
> From 03b28964311090533643acd267abe0cbc3c9b0a5 Mon Sep 17 00:00:00 2001
> From: Shiyong Li <[email protected]>
> Date: Fri, 2 Apr 2010 14:50:30 +0800
> Subject: [PATCH] Fix missing of last user info while getting
> DEBUG_SLAB config enabled.
>
> Even with SLAB_RED_ZONE and SLAB_STORE_USER enabled, kernel would NOT
> store redzone and last user data around allocated memory space if arch
> cache line > sizeof(unsigned long long). As a result, last user information
> is unexpectedly MISSED while dumping slab corruption log.
>
> This fix makes sure that redzone and last user tags get stored whatever
> cache line.
>
> Signed-off-by: Shiyong Li <[email protected]>
> ---
> ?mm/slab.c | ? ?7 ++-----
> ?1 files changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/mm/slab.c b/mm/slab.c
> index a8a38ca..84af997 100644
> --- a/mm/slab.c
> +++ b/mm/slab.c
> @@ -2267,9 +2267,6 @@ kmem_cache_create (const char *name, size_t
> size, size_t align,
> ? ? ? ?if (ralign < align) {
> ? ? ? ? ? ? ? ?ralign = align;
> ? ? ? ?}
> - ? ? ? /* disable debug if necessary */
> - ? ? ? if (ralign > __alignof__(unsigned long long))
> - ? ? ? ? ? ? ? flags &= ~(SLAB_RED_ZONE | SLAB_STORE_USER);
> ? ? ? ?/*
> ? ? ? ? * 4) Store it.
> ? ? ? ? */
> @@ -2289,8 +2286,8 @@ kmem_cache_create (const char *name, size_t
> size, size_t align,
> ? ? ? ? */
> ? ? ? ?if (flags & SLAB_RED_ZONE) {
> ? ? ? ? ? ? ? ?/* add space for red zone words */
> - ? ? ? ? ? ? ? cachep->obj_offset += sizeof(unsigned long long);
> - ? ? ? ? ? ? ? size += 2 * sizeof(unsigned long long);
> + ? ? ? ? ? ? ? cachep->obj_offset += align;
> + ? ? ? ? ? ? ? size += align + sizeof(unsigned long long);
> ? ? ? ?}
> ? ? ? ?if (flags & SLAB_STORE_USER) {
> ? ? ? ? ? ? ? ?/* user store requires one word storage behind the end of
The problem I have with this patch is that I am unable to convince
myself that it's correct.
The code in question is pretty hard to follow and it can break on
architectures with strict alignment requirements. I'm not sure I fully
understand why we've been disabling debugging in the first place.
On which architectures have you verified this fix?
Pekka
Hi, Pekka Enberg
We verified on OMAP3 (ARM V7).
1 thing we're sure
a) cachep->obj_offset will be aligned with "align" passed to
kmem_cache_create ()
Are you concerned about alignment for red-zone obj?
We'll double check.
--
Best Regards
Hu Tao
On Wed, Apr 7, 2010 at 11:25 PM, Pekka Enberg <[email protected]> wrote:
> Hi,
>
> (I'm cc'ing David who did some fixes in this area previously.)
>
> On Fri, Apr 2, 2010 at 10:21 AM, ShiYong LI <[email protected]> wrote:
>> Even with SLAB_RED_ZONE and SLAB_STORE_USER enabled, kernel would NOT
>> store redzone and last user data around allocated memory space if arch
>> cache line > sizeof(unsigned long long). As a result, last user information
>> is unexpectedly MISSED while dumping slab corruption log.
>>
>> This patch makes sure that redzone and last user tags get stored whatever
>> arch cache line.
>>
>> Compared to original codes, the change surely affects head redzone (redzone1).
>> Actually, with SLAB_RED_ZONE and SLAB_STORE_USER enabled,
>> allocated memory layout is as below:
>>
>> [ redzone1 ] ? <--------- Affected area.
>> [ real object space ]
>> [ redzone2 ]
>> [ last user ]
>> [ ... ]
>>
>> Let's do some analysis: (whatever SLAB_STORE_USER is).
>>
>> 1) With SLAB_RED_ZONE on, "align" >= sizeof(unsigned long long) according to
>> ? ?the following codes:
>> ? ? ? ?/* 2) arch mandated alignment */
>> ? ? ? ?if (ralign < ARCH_SLAB_MINALIGN) {
>> ? ? ? ? ? ? ? ?ralign = ARCH_SLAB_MINALIGN;
>> ? ? ? ?}
>> ? ? ? ?/* 3) caller mandated alignment */
>> ? ? ? ?if (ralign < align) {
>> ? ? ? ? ? ? ? ?ralign = align;
>> ? ? ? ?}
>> ? ? ? ?...
>> ? ? ? ?/*
>> ? ? ? ? * 4) Store it.
>> ? ? ? ? */
>> ? ? ? ?align = ralign;
>>
>> ? ?That's to say, could guarantee that redzone1 does NOT get broken
>> at all. Meanwhile,
>> ? ?Real object space could meet the need of cache line size by using
>> "align" ?argument.
>>
>> 2) With SLAB_RED_ZONE off, the change has no impact.
>>
>>
>> From 03b28964311090533643acd267abe0cbc3c9b0a5 Mon Sep 17 00:00:00 2001
>> From: Shiyong Li <[email protected]>
>> Date: Fri, 2 Apr 2010 14:50:30 +0800
>> Subject: [PATCH] Fix missing of last user info while getting
>> DEBUG_SLAB config enabled.
>>
>> Even with SLAB_RED_ZONE and SLAB_STORE_USER enabled, kernel would NOT
>> store redzone and last user data around allocated memory space if arch
>> cache line > sizeof(unsigned long long). As a result, last user information
>> is unexpectedly MISSED while dumping slab corruption log.
>>
>> This fix makes sure that redzone and last user tags get stored whatever
>> cache line.
>>
>> Signed-off-by: Shiyong Li <[email protected]>
>> ---
>> ?mm/slab.c | ? ?7 ++-----
>> ?1 files changed, 2 insertions(+), 5 deletions(-)
>>
>> diff --git a/mm/slab.c b/mm/slab.c
>> index a8a38ca..84af997 100644
>> --- a/mm/slab.c
>> +++ b/mm/slab.c
>> @@ -2267,9 +2267,6 @@ kmem_cache_create (const char *name, size_t
>> size, size_t align,
>> ? ? ? ?if (ralign < align) {
>> ? ? ? ? ? ? ? ?ralign = align;
>> ? ? ? ?}
>> - ? ? ? /* disable debug if necessary */
>> - ? ? ? if (ralign > __alignof__(unsigned long long))
>> - ? ? ? ? ? ? ? flags &= ~(SLAB_RED_ZONE | SLAB_STORE_USER);
>> ? ? ? ?/*
>> ? ? ? ? * 4) Store it.
>> ? ? ? ? */
>> @@ -2289,8 +2286,8 @@ kmem_cache_create (const char *name, size_t
>> size, size_t align,
>> ? ? ? ? */
>> ? ? ? ?if (flags & SLAB_RED_ZONE) {
>> ? ? ? ? ? ? ? ?/* add space for red zone words */
>> - ? ? ? ? ? ? ? cachep->obj_offset += sizeof(unsigned long long);
>> - ? ? ? ? ? ? ? size += 2 * sizeof(unsigned long long);
>> + ? ? ? ? ? ? ? cachep->obj_offset += align;
>> + ? ? ? ? ? ? ? size += align + sizeof(unsigned long long);
>> ? ? ? ?}
>> ? ? ? ?if (flags & SLAB_STORE_USER) {
>> ? ? ? ? ? ? ? ?/* user store requires one word storage behind the end of
>
> The problem I have with this patch is that I am unable to convince
> myself that it's correct.
>
> The code in question is pretty hard to follow and it can break on
> architectures with strict alignment requirements. I'm not sure I fully
> understand why we've been disabling debugging in the first place.
>
> On which architectures have you verified this fix?
>
> ? ? ? ? ? ? ? ? ? ? ? ?Pekka
>