Re: [PATCH 1/1] sched/headers: fix thread_info.<first> is overwritten by STACK_END_MAGIC
From: Wang, Dongsheng
Date: Thu Nov 29 2018 - 21:20:39 EST
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@xxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxx>
>>>>> Signed-off-by: Shunyong Yang <shunyong.yang@xxxxxxxxxxxxxxxx>
>>>>> ---
>>>>> 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 || IA64
>>>> The "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
>
>