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 <[email protected]>
Signed-off-by: Shunyong Yang <[email protected]>
---
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
# 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);
}
#elif !defined(__HAVE_THREAD_FUNCTIONS)
--
2.19.1
On Thu, Nov 22, 2018 at 11:54 PM, Wang Dongsheng
<[email protected]> 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 <[email protected]>
> Signed-off-by: Shunyong Yang <[email protected]>
> ---
> 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.
>
> # 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.
-Kees
>
> #elif !defined(__HAVE_THREAD_FUNCTIONS)
> --
> 2.19.1
>
--
Kees Cook
Hello Kees,
On 2018/11/28 6:38, Kees Cook wrote:
> On Thu, Nov 22, 2018 at 11:54 PM, Wang Dongsheng
> <[email protected]> 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 <[email protected]>
>> Signed-off-by: Shunyong Yang <[email protected]>
>> ---
>> 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.
>> # 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).
Cheers,
Dongsheng
On Tue, Nov 27, 2018 at 8:38 PM Wang, Dongsheng
<[email protected]> wrote:
>
> Hello Kees,
>
> On 2018/11/28 6:38, Kees Cook wrote:
> > On Thu, Nov 22, 2018 at 11:54 PM, Wang Dongsheng
> > <[email protected]> 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 <[email protected]>
> >> Signed-off-by: Shunyong Yang <[email protected]>
> >> ---
> >> 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)
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?
--
Kees Cook
On 2018/11/30 5:22, Kees Cook wrote:
> On Tue, Nov 27, 2018 at 8:38 PM Wang, Dongsheng
> <[email protected]> wrote:
>> Hello Kees,
>>
>> On 2018/11/28 6:38, Kees Cook wrote:
>>> On Thu, Nov 22, 2018 at 11:54 PM, Wang Dongsheng
>>> <[email protected]> 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 <[email protected]>
>>>> Signed-off-by: Shunyong Yang <[email protected]>
>>>> ---
>>>> 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)
> 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
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
>> <[email protected]> wrote:
>>> Hello Kees,
>>>
>>> On 2018/11/28 6:38, Kees Cook wrote:
>>>> On Thu, Nov 22, 2018 at 11:54 PM, Wang Dongsheng
>>>> <[email protected]> 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 <[email protected]>
>>>>> Signed-off-by: Shunyong Yang <[email protected]>
>>>>> ---
>>>>> 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
>
>
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
>>> <[email protected]> wrote:
>>>> Hello Kees,
>>>>
>>>> On 2018/11/28 6:38, Kees Cook wrote:
>>>>> On Thu, Nov 22, 2018 at 11:54 PM, Wang Dongsheng
>>>>> <[email protected]> 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 <[email protected]>
>>>>>> Signed-off-by: Shunyong Yang <[email protected]>
>>>>>> ---
>>>>>> 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
>>
>>
>