Skip to contentSkip to conversation

Fix end_of_stack() fn and location of stack canary for archs using STACK_GROWSUP

PATCH Superseded Chuck Ebbert 3 participants 3 messages Sep 16, 2014 – Sep 19, 2014 Scheduler

v1 of 3 Superseded Latest is v3 →

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 Fix end_of_stack() fn and location of stack canary for archs using STACK_GROWSUP

Aaron Tomlin recently posted patches [1] to enable checking the stack canary on every task switch. Looking at the canary code, I realized that every arch (except ia64, which adds some space for register spill above the stack) shares a definition of end_of_stack() that makes it the first long after the threadinfo.

For stacks that grow down, this low address is correct because the stack starts at the end of the thread area and grows toward lower addresses. However, for stacks that grow up, toward higher addresses, this is wrong. (The stack actually grows away from the canary.) On these archs end_of_stack() should return the address of the last long, at the highest possible address for the stack.

[1] http://lkml.org/lkml/2014/9/12/293

---

Compile tested only, with Aaron's patches applied and the new option CONFIG_SCHED_STACK_END_CHECK they add enabled. I have no way to test this any further.

include/linux/sched.h +4 -0
diff a/include/linux/sched.h b/include/linux/sched.h
@@ -2610,7 +2610,11 @@ static inline void setup_thread_stack(struct task_struct *p, struct task_struct
 
 static inline unsigned long *end_of_stack(struct task_struct *p)
 {
+#ifdef CONFIG_STACK_GROWSUP
+	return (unsigned long *)((unsigned long)task_thread_info(p) + THREAD_SIZE) - 1;
+#else
 	return (unsigned long *)(task_thread_info(p) + 1);
+#endif
 }
 
 #endif
Tested-by Acked-by on v1 1/1
#1 lore raw

Hi Chuck,

include/linux/sched.h static inline void setup_thread_stack(struct task_struct *p, struct task_struct · line 2614

Quoted 4 lines · Chuck Ebbert

static inline unsigned long *end_of_stack(struct task_struct *p) {

include/linux/sched.h +2 -0
+#ifdef CONFIG_STACK_GROWSUP
+	return (unsigned long *)((unsigned long)task_thread_info(p) + THREAD_SIZE) - 1;

Nit: this line should probably be wrapped to 80 columns.

Other than that, I've tested this on metag and can confirm that it fixes the following BUG which you would otherwise get during boot with Aaron's patches:

BUG: failure at kernel/sched/core.c:2664/schedule_debug()!
Kernel panic - not syncing: BUG!

Aaron: please can you try to get this patch applied before your patch series.

Thanks
James

Acked-by
#2 lore raw
Quoted 14 lines · James Hogan

Hi Chuck,

include/linux/sched.h static inline void setup_thread_stack(struct task_struct *p, struct task_struct · line 2614

Quoted 4 lines · Chuck Ebbert

static inline unsigned long *end_of_stack(struct task_struct *p) {

include/linux/sched.h +2 -0
+#ifdef CONFIG_STACK_GROWSUP
+	return (unsigned long *)((unsigned long)task_thread_info(p) + THREAD_SIZE) - 1;

Nit: this line should probably be wrapped to 80 columns.

Other than that, I've tested this on metag and can confirm that it fixes the following BUG which you would otherwise get during boot with Aaron's patches:

Quoted 5 lines
BUG: failure at kernel/sched/core.c:2664/schedule_debug()!
Kernel panic - not syncing: BUG!

OK.

Quoted 2 lines

Aaron: please can you try to get this patch applied before your patch series.

Ingo,

I hope it's not too late to get this patch in (once the nit has been addressed) for CONFIG_STACK_GROWSUP?

Regards,

Signature · 1 line
Aaron Tomlin