Skip to contentSkip to conversation

Fix kernel stack allocation alignment

PATCH In review Benjamin Herrenschmidt 2 participants 3 messages Apr 18, 2008

Subsystems (2)
PowerPC Generic include/asm header files
lore
Share & tools
lore

Benjamin Herrenschmidt

author
#0 lore raw

PATCH 2/3 Fix kernel stack allocation alignment

The powerpc kernel stacks need to be naturally aligned, as they contain the thread info at the bottom, which is obtained by clearing the low bits of the stack pointer.

However, when using 64K pages (the stack is smaller than a page), we use kmalloc to allocate it, which doesn't provide that guarantee.

It appeared to work so far... until one enables SLUB debugging which then returns unaligned pointers. Ooops...

This patch fixes it by using a slab cache with enforced alignment for those. It replies on my previous patch that adds a thread_info_cache_init() callback.

Signed-off-by Benjamin Herrenschmidt <[email protected]>

---
Note that m32r seems to also have this problem.

 arch/powerpc/kernel/process.c     |   31 +++++++++++++++++++++++++++++++
 include/asm-powerpc/thread_info.h |    8 ++------
 2 files changed, 33 insertions(+), 6 deletions(-)
arch/powerpc/kernel/process.c +31 -0
@@ -1038,3 +1038,34 @@ void ppc64_runlatch_off(void)
 	}
 }
 #endif
+
+#if THREAD_SHIFT < PAGE_SHIFT
+
+static struct kmem_cache *thread_info_cache;
+
+struct thread_info *alloc_thread_info(struct task_struct *tsk)
+{
+	struct thread_info *ti;
+
+	ti = kmem_cache_alloc(thread_info_cache, GFP_KERNEL);
+	if (unlikely(ti == NULL))
+		return NULL;
+#ifdef CONFIG_DEBUG_STACK_USAGE
+	memset(ti, 0, THREAD_SIZE);
+#endif
+	return ti;
+}
+
+void free_thread_info(struct thread_info *ti)
+{
+	kmem_cache_free(thread_info_cache, ti);
+}
+
+void thread_info_cache_init(void)
+{
+	thread_info_cache = kmem_cache_create("thread_info", THREAD_SIZE,
+					      THREAD_SIZE, 0, NULL);
+	BUG_ON(thread_info_cache == NULL);
+}
+
+#endif /* THREAD_SHIFT < PAGE_SHIFT */
include/asm-powerpc/thread_info.h +2 -6
Index: linux-work/include/asm-powerpc/thread_info.h
===================================================================
@@ -80,12 +80,8 @@ struct thread_info {
 
 #else /* THREAD_SHIFT < PAGE_SHIFT */
 
-#ifdef CONFIG_DEBUG_STACK_USAGE
-#define alloc_thread_info(tsk)	kzalloc(THREAD_SIZE, GFP_KERNEL)
-#else
-#define alloc_thread_info(tsk)	kmalloc(THREAD_SIZE, GFP_KERNEL)
-#endif
-#define free_thread_info(ti)	kfree(ti)
+extern struct thread_info *alloc_thread_info(struct task_struct *tsk);
+extern void free_thread_info(struct thread_info *ti);
 
 #endif /* THREAD_SHIFT < PAGE_SHIFT */
#1 lore raw
Quoted 16 lines · On Fri, 18 Apr 2008 16:56:17 +1000 Benjamin Herrenschmidt…

The powerpc kernel stacks need to be naturally aligned, as they contain the thread info at the bottom, which is obtained by clearing the low bits of the stack pointer.

However, when using 64K pages (the stack is smaller than a page), we use kmalloc to allocate it, which doesn't provide that guarantee.

It appeared to work so far... until one enables SLUB debugging which then returns unaligned pointers. Ooops...

This patch fixes it by using a slab cache with enforced alignment for those. It replies on my previous patch that adds a thread_info_cache_init() callback.

...

arch/powerpc/kernel/process.c void ppc64_runlatch_off(void) · line 1069

Quoted 6 lines
arch/powerpc/kernel/process.c +6 -0
+void thread_info_cache_init(void)
+{
+	thread_info_cache = kmem_cache_create("thread_info", THREAD_SIZE,
+					      THREAD_SIZE, 0, NULL);
+	BUG_ON(thread_info_cache == NULL);
+}

so... the "0" defeats all of SLAB_DEBUG_FREE, SLAB_RED_ZONE, SLAB_POISON and SLAB_STORE_USER, if the comment in slab.h is to be believed.

Was that overkill?

author
#2 lore raw
Quoted 5 lines

so... the "0" defeats all of SLAB_DEBUG_FREE, SLAB_RED_ZONE, SLAB_POISON and SLAB_STORE_USER, if the comment in slab.h is to be believed.

Was that overkill?

Well, I copied over from something else, looks like there's a -lot- of 0 users here...

Shouldn't there be some kind of SLAB_DEFAULT that contain those things ?

Ben.