Hello all,
Any comments about this patch?
Cheers, Dongsheng
On 2018/11/30 10:19, Wang, Dongsheng wrote:
On 2018/11/30 10:04, Wang, Dongsheng wrote:
On 2018/11/30 5:22, Kees Cook wrote:
On Tue, Nov 27, 2018 at 8:38 PM Wang, Dongsheng dongsheng.wang@hxt-semitech.com wrote:
Hello Kees,
On 2018/11/28 6:38, Kees Cook wrote:
On Thu, Nov 22, 2018 at 11:54 PM, Wang Dongsheng dongsheng.wang@hxt-semitech.com wrote:
When select ARCH_TASK_STRUCT_ON_STACK the first of thread_info variable is overwritten by STACK_END_MAGIC. In fact, the ARCH_TASK_STRUCT_ON_STACK is not a real task on stack, it's only init_task on init_stack.
Commit 0500871f21b2 ("Construct init thread stack in the linker script rather than by union") added this macro and put task_strcut into thread_union. This brings us the following possibilities: TASK_ON_STACK THREAD_INFO_IN_TASK STACK ----- <-- thread_info & stack N N | | --- <-- task | | | | ----- ---
----- <-- stack N Y | | --- <-- task(Including thread_info) | | | | ----- --- ----- <-- stack & task & thread_info Y N | | | | ----- ----- <-- stack & task(Including thread_info) Y Y | | | | -----The kernel has handled the first two cases correctly.
For the third case: TASK_ON_STACK: Y. THREAD_INFO_IN_TASK: N. this case should never happen, because the task and thread_info will overlap. So when TASK_ON_STACK is selected, THREAD_INFO_IN_TASK must be selected too.
For the fourth case: When task on stack, the end of stack should add a sizeof(task_struct) offset.
This patch handled with the third and fourth case.
Fixes: 0500871f21b2 ("Construct init thread stack in the linker ...")
Signed-off-by: Wang Dongsheng dongsheng.wang@hxt-semitech.com Signed-off-by: Shunyong Yang shunyong.yang@hxt-semitech.com
arch/Kconfig | 1 + include/linux/sched/task_stack.h | 5 ++++- 2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/arch/Kconfig b/arch/Kconfig index e1e540ffa979..0a2c73e73195 100644 --- a/arch/Kconfig +++ b/arch/Kconfig @@ -251,6 +251,7 @@ config ARCH_HAS_SET_MEMORY # Select if arch init_task must go in the __init_task_data section config ARCH_TASK_STRUCT_ON_STACK bool
depends on THREAD_INFO_IN_TASK || IA64The "IA64" part shouldn't be needed since IA64 already selects it.
Since it's selected, it also can't have a depends, IIUC.
Since the IA64 thread_info including task_struct, it doesn't need to select THREAD_INFO_IN_TASK. So we need to allow IA64 select ARCH_TASK_STRUCT_ON_STACK without THREAD_INFO.
Okay.
# Select if arch has its private alloc_task_struct() function config ARCH_TASK_STRUCT_ALLOCATOR diff --git a/include/linux/sched/task_stack.h b/include/linux/sched/task_stack.h index 6a841929073f..624c48defb9e 100644 --- a/include/linux/sched/task_stack.h +++ b/include/linux/sched/task_stack.h @@ -7,6 +7,7 @@ */
#include <linux/sched.h> +#include <linux/sched/task.h> #include <linux/magic.h>
#ifdef CONFIG_THREAD_INFO_IN_TASK @@ -25,7 +26,9 @@ static inline void *task_stack_page(const struct task_struct *task)
static inline unsigned long *end_of_stack(const struct task_struct *task) {
return task->stack;
if (!IS_ENABLED(CONFIG_ARCH_TASK_STRUCT_ON_STACK) || task != &init_task)return task->stack;return (unsigned long *)(task + 1);}
This seems like a strange place for the change. It feels more like init_task has been defined incorrectly.
The init_task will put into init_stack when ARCH_TASK_STRUCT_ON_STACK is selected. include/asm-generic/vmlinux.lds.h: #define INIT_TASK_DATA(align) \ . = ALIGN(align); \ __start_init_task = .; \ init_thread_union = .; \ init_stack = .; \ KEEP(*(.data..init_task)) \ KEEP(*(.data..init_thread_info)) \ . = __start_init_task + THREAD_SIZE; \ __end_init_task = .;
So we need end_of_stack to offset sizeof(task_struct).
Well, I guess I mean I'd rather the end_of_stack() code not be special-cased in the if. The default should be how it was. Perhaps:
if (IS_ENABLED(CONFIG_ARCH_TASK_STRUCT_ON_STACK) && task == &init_task)
About this special case: As I mentioned in the description of patch, The ARCH_TASK_STRUCT_ON_STACK is not a real task on stack, it's only init_task on init_stack. The alloc task is not in alloc stack, So if condition including "task == &init_task".
Cheers, Dongsheng
return (unsigned long *)(task + 1);return task->stack;
But it still feels strange: why can't task->stack point to the correct place in this special case?
Normally, task->stack is the bottom of the stack, the end_of_stack just need to return task->stack. The task->stack always represents the bottom of the stack, and it cannot be changed, so what happens here is we have some data(task or thread info)that we want to put at the bottom of the stack. The end_of_stack just refers to the size of the stack available to us. In ARCH_TASK_STRUCT_ON_STACK case, the init_task has been placed in a fixed location, the task is placed at the bottom of the stack. Current end_of_stack doesn't handle this situation, so we need add a if condition to handle it. And this is just like !THREAD_INFO_IN_TASK(the thead_info on stack), the thread_info is placed at the bottom of the stack, the end_of_stack = the bottom of stack + sizeof(*thread_info).
Cheers, Dongsheng