[PATCH] sched: unlikely corrupted stack end

[PATCH] sched: unlikely corrupted stack end

[WANG Chao]

unlikely() was dropped in commit ce03e41 ("sched/core: Drop unlikely
behind BUG_ON()"), but commit 29d6455 ("sched: panic on corrupted stack
end") dropped BUG_ON() and called panic directly.

Now we should bring unlikely() back for branch prediction.

Signed-off-by: WANG Chao <wcwxyz@gmail.com>
---
 kernel/sched/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 017d539..7db442c 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -3170,7 +3170,7 @@ static noinline void __schedule_bug(struct task_struct *prev)
 static inline void schedule_debug(struct task_struct *prev)
 {
 #ifdef CONFIG_SCHED_STACK_END_CHECK
-	if (task_stack_end_corrupted(prev))
+	if (unlikely(task_stack_end_corrupted(prev)))
 		panic("corrupted stack end detected inside scheduler\n");
 #endif
 
-- 
2.8.4

Re: [PATCH] sched: unlikely corrupted stack end

[Ingo Molnar]

* WANG Chao <wcwxyz@gmail.com> wrote:

> unlikely() was dropped in commit ce03e41 ("sched/core: Drop unlikely
> behind BUG_ON()"), but commit 29d6455 ("sched: panic on corrupted stack
> end") dropped BUG_ON() and called panic directly.
> 
> Now we should bring unlikely() back for branch prediction.
> 
> Signed-off-by: WANG Chao <wcwxyz@gmail.com>
> ---
>  kernel/sched/core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 017d539..7db442c 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -3170,7 +3170,7 @@ static noinline void __schedule_bug(struct task_struct *prev)
>  static inline void schedule_debug(struct task_struct *prev)
>  {
>  #ifdef CONFIG_SCHED_STACK_END_CHECK
> -	if (task_stack_end_corrupted(prev))
> +	if (unlikely(task_stack_end_corrupted(prev)))
>  		panic("corrupted stack end detected inside scheduler\n");
>  #endif

It would be better and cleaner to push that into the task_stack_end_corrupted() 
definition. (and to turn it into an inline function while we are touching it.)

Thanks,

	Ingo

Re: [PATCH] sched: unlikely corrupted stack end

[Peter Zijlstra]

On Tue, Jun 14, 2016 at 02:43:06PM +0800, WANG Chao wrote:
> unlikely() was dropped in commit ce03e41 ("sched/core: Drop unlikely
> behind BUG_ON()"), but commit 29d6455 ("sched: panic on corrupted stack
> end") dropped BUG_ON() and called panic directly.

Please use git config core.abbrev=12 and try again.

[PATCH v2] sched: unlikely corrupted stack end

[WANG Chao]

unlikely() was dropped in commit ce03e4137bb2 ("sched/core: Drop
unlikely behind BUG_ON()"), but commit 29d6455178a0 ("sched: panic on
corrupted stack end") dropped BUG_ON() and called panic directly.

Now we should bring unlikely() back for branch prediction. While we're
at it, it's better and cleaner to turn task_stack_end_corrupted() into
inline function.

Signed-off-by: WANG Chao <wcwxyz@gmail.com>
---
 include/linux/sched.h | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index 6e42ada26345..797ca1975431 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -2997,8 +2997,11 @@ static inline unsigned long *end_of_stack(struct task_struct *p)
 }
 
 #endif
-#define task_stack_end_corrupted(task) \
-		(*(end_of_stack(task)) != STACK_END_MAGIC)
+
+static inline int task_stack_end_corrupted(struct task_struct *p)
+{
+	return unlikely(*(end_of_stack(p)) != STACK_END_MAGIC);
+}
 
 static inline int object_is_on_stack(void *obj)
 {
-- 
2.8.4

Re: [PATCH v2] sched: unlikely corrupted stack end

[Ingo Molnar]

* WANG Chao <wcwxyz@gmail.com> wrote:

> unlikely() was dropped in commit ce03e4137bb2 ("sched/core: Drop
> unlikely behind BUG_ON()"), but commit 29d6455178a0 ("sched: panic on
> corrupted stack end") dropped BUG_ON() and called panic directly.
> 
> Now we should bring unlikely() back for branch prediction. While we're
> at it, it's better and cleaner to turn task_stack_end_corrupted() into
> inline function.
> 
> Signed-off-by: WANG Chao <wcwxyz@gmail.com>
> ---
>  include/linux/sched.h | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 6e42ada26345..797ca1975431 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -2997,8 +2997,11 @@ static inline unsigned long *end_of_stack(struct task_struct *p)
>  }
>  
>  #endif
> -#define task_stack_end_corrupted(task) \
> -		(*(end_of_stack(task)) != STACK_END_MAGIC)
> +
> +static inline int task_stack_end_corrupted(struct task_struct *p)
> +{
> +	return unlikely(*(end_of_stack(p)) != STACK_END_MAGIC);
> +}

The passed in pointer should be const, and the extra parentheses around the 
end_of_stack() call are not needed anymore (since it's now proper C code now).

Thanks,

	Ingo

Re: [PATCH v2] sched: unlikely corrupted stack end

[WANG Chao]

> 在 2016年6月14日，下午4:56，Ingo Molnar <mingo@kernel.org> 写道：
> 
> 
> * WANG Chao <wcwxyz@gmail.com> wrote:
> 
>> unlikely() was dropped in commit ce03e4137bb2 ("sched/core: Drop
>> unlikely behind BUG_ON()"), but commit 29d6455178a0 ("sched: panic on
>> corrupted stack end") dropped BUG_ON() and called panic directly.
>> 
>> Now we should bring unlikely() back for branch prediction. While we're
>> at it, it's better and cleaner to turn task_stack_end_corrupted() into
>> inline function.
>> 
>> Signed-off-by: WANG Chao <wcwxyz@gmail.com>
>> ---
>> include/linux/sched.h | 7 +++++--
>> 1 file changed, 5 insertions(+), 2 deletions(-)
>> 
>> diff --git a/include/linux/sched.h b/include/linux/sched.h
>> index 6e42ada26345..797ca1975431 100644
>> --- a/include/linux/sched.h
>> +++ b/include/linux/sched.h
>> @@ -2997,8 +2997,11 @@ static inline unsigned long *end_of_stack(struct task_struct *p)
>> }
>> 
>> #endif
>> -#define task_stack_end_corrupted(task) \
>> -		(*(end_of_stack(task)) != STACK_END_MAGIC)
>> +
>> +static inline int task_stack_end_corrupted(struct task_struct *p)
>> +{
>> +	return unlikely(*(end_of_stack(p)) != STACK_END_MAGIC);
>> +}
> 
> The passed in pointer should be const, and the extra parentheses around the 
> end_of_stack() call are not needed anymore (since it's now proper C code now).

end_of_stack() will discard const and cause an compiler warning.
Should I add const to end_of_stack()?

Re: [PATCH v2] sched: unlikely corrupted stack end

[Ingo Molnar]

* WANG Chao <wcwxyz@gmail.com> wrote:

> 
> > 在 2016年6月14日，下午4:56，Ingo Molnar <mingo@kernel.org> 写道：
> > 
> > 
> > * WANG Chao <wcwxyz@gmail.com> wrote:
> > 
> >> unlikely() was dropped in commit ce03e4137bb2 ("sched/core: Drop
> >> unlikely behind BUG_ON()"), but commit 29d6455178a0 ("sched: panic on
> >> corrupted stack end") dropped BUG_ON() and called panic directly.
> >> 
> >> Now we should bring unlikely() back for branch prediction. While we're
> >> at it, it's better and cleaner to turn task_stack_end_corrupted() into
> >> inline function.
> >> 
> >> Signed-off-by: WANG Chao <wcwxyz@gmail.com>
> >> ---
> >> include/linux/sched.h | 7 +++++--
> >> 1 file changed, 5 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/include/linux/sched.h b/include/linux/sched.h
> >> index 6e42ada26345..797ca1975431 100644
> >> --- a/include/linux/sched.h
> >> +++ b/include/linux/sched.h
> >> @@ -2997,8 +2997,11 @@ static inline unsigned long *end_of_stack(struct task_struct *p)
> >> }
> >> 
> >> #endif
> >> -#define task_stack_end_corrupted(task) \
> >> -		(*(end_of_stack(task)) != STACK_END_MAGIC)
> >> +
> >> +static inline int task_stack_end_corrupted(struct task_struct *p)
> >> +{
> >> +	return unlikely(*(end_of_stack(p)) != STACK_END_MAGIC);
> >> +}
> > 
> > The passed in pointer should be const, and the extra parentheses around the 
> > end_of_stack() call are not needed anymore (since it's now proper C code now).
> 
> end_of_stack() will discard const and cause an compiler warning.
> Should I add const to end_of_stack()?

Yes. Also make sure ia64 still builds and such.

Thanks,

	Ingo

Re: [PATCH v2] sched: unlikely corrupted stack end

[WANG Chao]

> 在 2016年6月14日，下午6:26，Ingo Molnar <mingo@kernel.org> 写道：
> 
> 
> * WANG Chao <wcwxyz@gmail.com> wrote:
> 
>> 
>>> 在 2016年6月14日，下午4:56，Ingo Molnar <mingo@kernel.org> 写道：
>>> 
>>> 
>>> * WANG Chao <wcwxyz@gmail.com> wrote:
>>> 
>>>> unlikely() was dropped in commit ce03e4137bb2 ("sched/core: Drop
>>>> unlikely behind BUG_ON()"), but commit 29d6455178a0 ("sched: panic on
>>>> corrupted stack end") dropped BUG_ON() and called panic directly.
>>>> 
>>>> Now we should bring unlikely() back for branch prediction. While we're
>>>> at it, it's better and cleaner to turn task_stack_end_corrupted() into
>>>> inline function.
>>>> 
>>>> Signed-off-by: WANG Chao <wcwxyz@gmail.com>
>>>> ---
>>>> include/linux/sched.h | 7 +++++--
>>>> 1 file changed, 5 insertions(+), 2 deletions(-)
>>>> 
>>>> diff --git a/include/linux/sched.h b/include/linux/sched.h
>>>> index 6e42ada26345..797ca1975431 100644
>>>> --- a/include/linux/sched.h
>>>> +++ b/include/linux/sched.h
>>>> @@ -2997,8 +2997,11 @@ static inline unsigned long *end_of_stack(struct task_struct *p)
>>>> }
>>>> 
>>>> #endif
>>>> -#define task_stack_end_corrupted(task) \
>>>> -		(*(end_of_stack(task)) != STACK_END_MAGIC)
>>>> +
>>>> +static inline int task_stack_end_corrupted(struct task_struct *p)
>>>> +{
>>>> +	return unlikely(*(end_of_stack(p)) != STACK_END_MAGIC);
>>>> +}
>>> 
>>> The passed in pointer should be const, and the extra parentheses around the 
>>> end_of_stack() call are not needed anymore (since it's now proper C code now).
>> 
>> end_of_stack() will discard const and cause an compiler warning.
>> Should I add const to end_of_stack()?
> 
> Yes. Also make sure ia64 still builds and such.

It seems convert task_stack_end_corrupted() into inline isn’t trivial.

In ia64, end_of_stack() is expanded to:

(unsigned long *)((void *)(p) + ((IA64_TASK_SIZE + IA64_THREAD_INFO_SIZE + 31) & ~31))

IA64_TASK_SIZE and IA64_THREAD_INFO_SIZE is defined in arch/ia64/kernel/asm-offsets.c,
which needs to include linux/sched.h.

So the problem is task_stack_end_corrupted() doesn’t compile before asm-offsets.c is compiled.
asm-offsets.c also needs to include linux/sched.h to compile. I think maybe that’s why
task_stack_end_corrupted() is introduced as marco, not inline.

Any idea?

Thanks
WANG Chao

Re: [PATCH v2] sched: unlikely corrupted stack end

[Ingo Molnar]

* WANG Chao <wcwxyz@gmail.com> wrote:

> 
> > 在 2016年6月14日，下午6:26，Ingo Molnar <mingo@kernel.org> 写道：
> > 
> > 
> > * WANG Chao <wcwxyz@gmail.com> wrote:
> > 
> >> 
> >>> 在 2016年6月14日，下午4:56，Ingo Molnar <mingo@kernel.org> 写道：
> >>> 
> >>> 
> >>> * WANG Chao <wcwxyz@gmail.com> wrote:
> >>> 
> >>>> unlikely() was dropped in commit ce03e4137bb2 ("sched/core: Drop
> >>>> unlikely behind BUG_ON()"), but commit 29d6455178a0 ("sched: panic on
> >>>> corrupted stack end") dropped BUG_ON() and called panic directly.
> >>>> 
> >>>> Now we should bring unlikely() back for branch prediction. While we're
> >>>> at it, it's better and cleaner to turn task_stack_end_corrupted() into
> >>>> inline function.
> >>>> 
> >>>> Signed-off-by: WANG Chao <wcwxyz@gmail.com>
> >>>> ---
> >>>> include/linux/sched.h | 7 +++++--
> >>>> 1 file changed, 5 insertions(+), 2 deletions(-)
> >>>> 
> >>>> diff --git a/include/linux/sched.h b/include/linux/sched.h
> >>>> index 6e42ada26345..797ca1975431 100644
> >>>> --- a/include/linux/sched.h
> >>>> +++ b/include/linux/sched.h
> >>>> @@ -2997,8 +2997,11 @@ static inline unsigned long *end_of_stack(struct task_struct *p)
> >>>> }
> >>>> 
> >>>> #endif
> >>>> -#define task_stack_end_corrupted(task) \
> >>>> -		(*(end_of_stack(task)) != STACK_END_MAGIC)
> >>>> +
> >>>> +static inline int task_stack_end_corrupted(struct task_struct *p)
> >>>> +{
> >>>> +	return unlikely(*(end_of_stack(p)) != STACK_END_MAGIC);
> >>>> +}
> >>> 
> >>> The passed in pointer should be const, and the extra parentheses around the 
> >>> end_of_stack() call are not needed anymore (since it's now proper C code now).
> >> 
> >> end_of_stack() will discard const and cause an compiler warning.
> >> Should I add const to end_of_stack()?
> > 
> > Yes. Also make sure ia64 still builds and such.
> 
> It seems convert task_stack_end_corrupted() into inline isn’t trivial.
> 
> In ia64, end_of_stack() is expanded to:
> 
> (unsigned long *)((void *)(p) + ((IA64_TASK_SIZE + IA64_THREAD_INFO_SIZE + 31) & ~31))
> 
> IA64_TASK_SIZE and IA64_THREAD_INFO_SIZE is defined in arch/ia64/kernel/asm-offsets.c,
> which needs to include linux/sched.h.
> 
> So the problem is task_stack_end_corrupted() doesn’t compile before asm-offsets.c is compiled.
> asm-offsets.c also needs to include linux/sched.h to compile. I think maybe that’s why
> task_stack_end_corrupted() is introduced as marco, not inline.
> 
> Any idea?

Oh well ...

I guess we'll have to add the unlikely() to the macro itself.

Thanks,

	Ingo

[PATCH v3] sched: unlikely corrupted stack end

[WANG Chao]

unlikely() was dropped in commit ce03e4137bb2 ("sched/core: Drop
unlikely behind BUG_ON()"), but commit 29d6455178a0 ("sched: panic on
corrupted stack end") dropped BUG_ON() and called panic directly.

Now we should bring unlikely() back for branch prediction. While we're
at it, it's better and cleaner to add unlikely() to
task_stack_end_corrupted() macro.

Signed-off-by: WANG Chao <wcwxyz@gmail.com>
---
 include/linux/sched.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index 6e42ada26345..74a02bf30827 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -2998,7 +2998,7 @@ static inline unsigned long *end_of_stack(struct task_struct *p)
 
 #endif
 #define task_stack_end_corrupted(task) \
-		(*(end_of_stack(task)) != STACK_END_MAGIC)
+		(unlikely(*(end_of_stack(task)) != STACK_END_MAGIC))
 
 static inline int object_is_on_stack(void *obj)
 {
-- 
2.9.0

Re: [PATCH v2] sched: unlikely corrupted stack end

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 5499 bytes --]

Hi,

[auto build test ERROR on tip/sched/core]
[also build test ERROR on v4.7-rc3 next-20160614]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/WANG-Chao/sched-unlikely-corrupted-stack-end/20160614-162711
config: ia64-allmodconfig (attached as .config)
compiler: ia64-linux-gcc (GCC) 4.9.0
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=ia64 

All error/warnings (new ones prefixed by >>):

   warning: (FAULT_INJECTION_STACKTRACE_FILTER && LATENCYTOP && KMEMCHECK && LOCKDEP) selects FRAME_POINTER which has unmet direct dependencies (DEBUG_KERNEL && (CRIS || M68K || FRV || UML || AVR32 || SUPERH || BLACKFIN || MN10300 || METAG) || ARCH_WANT_FRAME_POINTERS)
   warning: (FAULT_INJECTION_STACKTRACE_FILTER && LATENCYTOP && KMEMCHECK && LOCKDEP) selects FRAME_POINTER which has unmet direct dependencies (DEBUG_KERNEL && (CRIS || M68K || FRV || UML || AVR32 || SUPERH || BLACKFIN || MN10300 || METAG) || ARCH_WANT_FRAME_POINTERS)
   In file included from include/uapi/linux/stddef.h:1:0,
                    from include/linux/stddef.h:4,
                    from include/uapi/linux/posix_types.h:4,
                    from include/uapi/linux/types.h:13,
                    from include/linux/types.h:5,
                    from include/uapi/linux/capability.h:16,
                    from include/linux/capability.h:15,
                    from include/linux/sched.h:15,
                    from arch/ia64/kernel/asm-offsets.c:9:
   include/linux/sched.h: In function 'task_stack_end_corrupted':
>> arch/ia64/include/asm/ptrace.h:37:29: error: 'IA64_TASK_SIZE' undeclared (first use in this function)
    #define IA64_RBS_OFFSET   ((IA64_TASK_SIZE + IA64_THREAD_INFO_SIZE + 31) & ~31)
                                ^
   include/linux/compiler.h:170:42: note: in definition of macro 'unlikely'
    # define unlikely(x) __builtin_expect(!!(x), 0)
                                             ^
>> arch/ia64/include/asm/thread_info.h:74:57: note: in expansion of macro 'IA64_RBS_OFFSET'
    #define end_of_stack(p) (unsigned long *)((void *)(p) + IA64_RBS_OFFSET)
                                                            ^
>> include/linux/sched.h:3006:20: note: in expansion of macro 'end_of_stack'
     return unlikely(*(end_of_stack(p)) != STACK_END_MAGIC);
                       ^
   arch/ia64/include/asm/ptrace.h:37:29: note: each undeclared identifier is reported only once for each function it appears in
    #define IA64_RBS_OFFSET   ((IA64_TASK_SIZE + IA64_THREAD_INFO_SIZE + 31) & ~31)
                                ^
   include/linux/compiler.h:170:42: note: in definition of macro 'unlikely'
    # define unlikely(x) __builtin_expect(!!(x), 0)
                                             ^
>> arch/ia64/include/asm/thread_info.h:74:57: note: in expansion of macro 'IA64_RBS_OFFSET'
    #define end_of_stack(p) (unsigned long *)((void *)(p) + IA64_RBS_OFFSET)
                                                            ^
>> include/linux/sched.h:3006:20: note: in expansion of macro 'end_of_stack'
     return unlikely(*(end_of_stack(p)) != STACK_END_MAGIC);
                       ^
>> arch/ia64/include/asm/ptrace.h:37:46: error: 'IA64_THREAD_INFO_SIZE' undeclared (first use in this function)
    #define IA64_RBS_OFFSET   ((IA64_TASK_SIZE + IA64_THREAD_INFO_SIZE + 31) & ~31)
                                                 ^
   include/linux/compiler.h:170:42: note: in definition of macro 'unlikely'
    # define unlikely(x) __builtin_expect(!!(x), 0)
                                             ^
>> arch/ia64/include/asm/thread_info.h:74:57: note: in expansion of macro 'IA64_RBS_OFFSET'
    #define end_of_stack(p) (unsigned long *)((void *)(p) + IA64_RBS_OFFSET)
                                                            ^
>> include/linux/sched.h:3006:20: note: in expansion of macro 'end_of_stack'
     return unlikely(*(end_of_stack(p)) != STACK_END_MAGIC);
                       ^
   make[2]: *** [arch/ia64/kernel/asm-offsets.s] Error 1
   make[2]: Target '__build' not remade because of errors.
   make[1]: *** [prepare0] Error 2
   make[1]: Target 'prepare' not remade because of errors.
   make: *** [sub-make] Error 2

vim +/end_of_stack +3006 include/linux/sched.h

  2990	 * When the stack grows up, this is the highest address.
  2991	 * Beyond that position, we corrupt data on the next page.
  2992	 */
  2993	static inline unsigned long *end_of_stack(struct task_struct *p)
  2994	{
  2995	#ifdef CONFIG_STACK_GROWSUP
  2996		return (unsigned long *)((unsigned long)task_thread_info(p) + THREAD_SIZE) - 1;
  2997	#else
  2998		return (unsigned long *)(task_thread_info(p) + 1);
  2999	#endif
  3000	}
  3001	
  3002	#endif
  3003	
  3004	static inline int task_stack_end_corrupted(struct task_struct *p)
  3005	{
> 3006		return unlikely(*(end_of_stack(p)) != STACK_END_MAGIC);
  3007	}
  3008	
  3009	static inline int object_is_on_stack(void *obj)
  3010	{
  3011		void *stack = task_stack_page(current);
  3012	
  3013		return (obj >= stack) && (obj < (stack + THREAD_SIZE));
  3014	}

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 43700 bytes --]