Hi,
Compared to previous version, add alignment checking to make sure
memory space storing redzone2 and last user tags is 8 byte alignment.
>From 949e8c29e8681a2359e23a8fbd8b9d4833f42344 Mon Sep 17 00:00:00 2001
From: Shiyong Li <[email protected]>
Date: Mon, 12 Apr 2010 13:48:21 +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 unless
the required alignment breaks redzone's.
Signed-off-by: Shiyong Li <[email protected]>
---
mm/slab.c | 8 ++++----
1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/mm/slab.c b/mm/slab.c
index a8a38ca..b97c57e 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -2267,8 +2267,8 @@ 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))
+ /* disable debug if not aligning with REDZONE_ALIGN */
+ if (ralign & (__alignof__(unsigned long long) - 1))
flags &= ~(SLAB_RED_ZONE | SLAB_STORE_USER);
/*
* 4) Store it.
@@ -2289,8 +2289,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
Actually we greped "kmem_cache_create" in whole kernel souce tree
(2.6.29 and 2.6.32).
Either "align" equal to "0" or flag SLAB_HWCACHE_ALIGN is used when
calling kmem_cache_create().
Seems all of arch's cache-line-size is multiple of 64-bit/8-byte
(sizeof(long long)) except arch-microblaze (4-byte).
The smallest (except arch-microblaze) cache-line-size is 2^4= 16-byte
as I can see.
So even considering possible sizeof(long long) == 128-bit/16-byte, it
is still safe to apply Shiyong's original version.
Anyway, Shiyong's new patch check the weired situation that "align >
sizeof(long long) && align is NOT multiple of sizeof (long long)"
Let us know whether the new version address your concerns.
--
Best Regards
Hu Tao
On Mon, Apr 12, 2010 at 1:50 PM, ShiYong LI <[email protected]> wrote:
> Hi,
>
> Compared to previous version, add alignment checking to make sure
> memory space storing redzone2 and last user tags is 8 byte alignment.
>
> From 949e8c29e8681a2359e23a8fbd8b9d4833f42344 Mon Sep 17 00:00:00 2001
> From: Shiyong Li <[email protected]>
> Date: Mon, 12 Apr 2010 13:48:21 +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 unless
> the required alignment breaks redzone's.
>
> Signed-off-by: Shiyong Li <[email protected]>
> ---
> ?mm/slab.c | ? ?8 ++++----
> ?1 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/mm/slab.c b/mm/slab.c
> index a8a38ca..b97c57e 100644
> --- a/mm/slab.c
> +++ b/mm/slab.c
> @@ -2267,8 +2267,8 @@ 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))
> + ? ? ? /* disable debug if not aligning with REDZONE_ALIGN */
> + ? ? ? if (ralign & (__alignof__(unsigned long long) - 1))
> ? ? ? ? ? ? ? ?flags &= ~(SLAB_RED_ZONE | SLAB_STORE_USER);
> ? ? ? ?/*
> ? ? ? ? * 4) Store it.
> @@ -2289,8 +2289,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
>
TAO HU kirjoitti:
> Actually we greped "kmem_cache_create" in whole kernel souce tree
> (2.6.29 and 2.6.32).
>
> Either "align" equal to "0" or flag SLAB_HWCACHE_ALIGN is used when
> calling kmem_cache_create().
I don't think that's correct. The "task_xstate" has alignof(struct
task_xstate) and there seems to be so GCC attributes that force
non-default alignment on the struct.
> Seems all of arch's cache-line-size is multiple of 64-bit/8-byte
> (sizeof(long long)) except arch-microblaze (4-byte).
> The smallest (except arch-microblaze) cache-line-size is 2^4= 16-byte
> as I can see.
> So even considering possible sizeof(long long) == 128-bit/16-byte, it
> is still safe to apply Shiyong's original version.
>
> Anyway, Shiyong's new patch check the weired situation that "align >
> sizeof(long long) && align is NOT multiple of sizeof (long long)"
> Let us know whether the new version address your concerns.
Yeah, sorry for dragging this issue on. I've been looking at the patch
but haven't been able to convince myself that it's correct. Nick, David,
Christoph, Matt, could you also please take a look at this?
Pekka
ShiYong LI wrote:
> Hi,
>
> Compared to previous version, add alignment checking to make sure
> memory space storing redzone2 and last user tags is 8 byte alignment.
>
> From 949e8c29e8681a2359e23a8fbd8b9d4833f42344 Mon Sep 17 00:00:00 2001
> From: Shiyong Li <[email protected]>
> Date: Mon, 12 Apr 2010 13:48:21 +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 unless
> the required alignment breaks redzone's.
>
> Signed-off-by: Shiyong Li <[email protected]>
OK, I added this to linux-next for testing. Thanks!
> ---
> mm/slab.c | 8 ++++----
> 1 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/mm/slab.c b/mm/slab.c
> index a8a38ca..b97c57e 100644
> --- a/mm/slab.c
> +++ b/mm/slab.c
> @@ -2267,8 +2267,8 @@ 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))
> + /* disable debug if not aligning with REDZONE_ALIGN */
> + if (ralign & (__alignof__(unsigned long long) - 1))
> flags &= ~(SLAB_RED_ZONE | SLAB_STORE_USER);
> /*
> * 4) Store it.
> @@ -2289,8 +2289,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
Hi, Pekka Enberg
Thanks!
If we hadn't seen the last user info, we would not have fixed a
difficult bug in our system.
So very glad to know.
--
Best Regards
Hu Tao
On Thu, Apr 15, 2010 at 1:56 AM, Pekka Enberg <[email protected]> wrote:
> ShiYong LI wrote:
>>
>> Hi,
>>
>> Compared to previous version, add alignment checking to make sure
>> memory space storing redzone2 and last user tags is 8 byte alignment.
>>
>> From 949e8c29e8681a2359e23a8fbd8b9d4833f42344 Mon Sep 17 00:00:00 2001
>> From: Shiyong Li <[email protected]>
>> Date: Mon, 12 Apr 2010 13:48:21 +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 unless
>> the required alignment breaks redzone's.
>>
>> Signed-off-by: Shiyong Li <[email protected]>
>
> OK, I added this to linux-next for testing. Thanks!
>
>> ---
>> ?mm/slab.c | ? ?8 ++++----
>> ?1 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/mm/slab.c b/mm/slab.c
>> index a8a38ca..b97c57e 100644
>> --- a/mm/slab.c
>> +++ b/mm/slab.c
>> @@ -2267,8 +2267,8 @@ 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))
>> + ? ? ? /* disable debug if not aligning with REDZONE_ALIGN */
>> + ? ? ? if (ralign & (__alignof__(unsigned long long) - 1))
>> ? ? ? ? ? ? ? ?flags &= ~(SLAB_RED_ZONE | SLAB_STORE_USER);
>> ? ? ? ?/*
>> ? ? ? ? * 4) Store it.
>> @@ -2289,8 +2289,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
>
> --
> 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>
>