[PATCH] debug: Do not permit CONFIG_DEBUG_STACK_USAGE=y on IA64 or PARISC

[PATCH] debug: Do not permit CONFIG_DEBUG_STACK_USAGE=y on IA64 or PARISC

[Tony Luck]

The stack_not_used() function in <linux/sched.h> assumes that stacks
grow downwards. This is not true on IA64 or PARISC, so this function
would walk off in the wrong direction and into the weeds.

Found on IA64 because of a compilation failure with recursive dependencies
on IA64_TASKSIZE and IA64_THREAD_INFO_SIZE.

Fixing the code is possible, but should be combined with other
infrastructure additions to set up the "canary" at the end of the stack.

Reported-by: Fengguang Wu <fengguang.wu@intel.com> (failed allmodconfig build)
Signed-off-by: Tony Luck <tony.luck@intel.com>
---
 lib/Kconfig.debug | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index ff5bdee..4a18650 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -714,7 +714,7 @@ config STACKTRACE
 
 config DEBUG_STACK_USAGE
 	bool "Stack utilization instrumentation"
-	depends on DEBUG_KERNEL
+	depends on DEBUG_KERNEL && !IA64 && !PARISC
 	help
 	  Enables the display of the minimum amount of free stack which each
 	  task has ever had available in the sysrq-T and sysrq-P debug output.
-- 
1.7.10.2.552.gaa3bb87

Re: [PATCH] debug: Do not permit CONFIG_DEBUG_STACK_USAGE=y on IA64 or PARISC

[Ingo Molnar]

* Tony Luck <tony.luck@intel.com> wrote:

> The stack_not_used() function in <linux/sched.h> assumes that stacks
> grow downwards. This is not true on IA64 or PARISC, so this function
> would walk off in the wrong direction and into the weeds.
> 
> Found on IA64 because of a compilation failure with recursive dependencies
> on IA64_TASKSIZE and IA64_THREAD_INFO_SIZE.
> 
> Fixing the code is possible, but should be combined with other
> infrastructure additions to set up the "canary" at the end of the stack.
> 
> Reported-by: Fengguang Wu <fengguang.wu@intel.com> (failed allmodconfig build)
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> ---
>  lib/Kconfig.debug | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index ff5bdee..4a18650 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -714,7 +714,7 @@ config STACKTRACE
>  
>  config DEBUG_STACK_USAGE
>  	bool "Stack utilization instrumentation"
> -	depends on DEBUG_KERNEL
> +	depends on DEBUG_KERNEL && !IA64 && !PARISC

The modern way of doing this is by adding an ARCH_SUPPORTS_ 
flag.

Thanks,

	Ingo

Re: [PATCH] debug: Do not permit CONFIG_DEBUG_STACK_USAGE=y on IA64 or PARISC

[James Bottomley]

On Wed, 2012-07-25 at 09:45 +0200, Ingo Molnar wrote:
> * Tony Luck <tony.luck@intel.com> wrote:
> 
> > The stack_not_used() function in <linux/sched.h> assumes that stacks
> > grow downwards. This is not true on IA64 or PARISC, so this function
> > would walk off in the wrong direction and into the weeds.
> > 
> > Found on IA64 because of a compilation failure with recursive dependencies
> > on IA64_TASKSIZE and IA64_THREAD_INFO_SIZE.
> > 
> > Fixing the code is possible, but should be combined with other
> > infrastructure additions to set up the "canary" at the end of the stack.
> > 
> > Reported-by: Fengguang Wu <fengguang.wu@intel.com> (failed allmodconfig build)
> > Signed-off-by: Tony Luck <tony.luck@intel.com>
> > ---
> >  lib/Kconfig.debug | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> > index ff5bdee..4a18650 100644
> > --- a/lib/Kconfig.debug
> > +++ b/lib/Kconfig.debug
> > @@ -714,7 +714,7 @@ config STACKTRACE
> >  
> >  config DEBUG_STACK_USAGE
> >  	bool "Stack utilization instrumentation"
> > -	depends on DEBUG_KERNEL
> > +	depends on DEBUG_KERNEL && !IA64 && !PARISC
> 
> The modern way of doing this is by adding an ARCH_SUPPORTS_ 
> flag.

That's a bit daft, isn't it?  We'd have to add ARCH_SUPPORTS_ flags to
about 25 separate architectures just to get it not supported on these
two.

Since the problem is an invalid assumption about how the stack grows,
why not just condition it on that.  We actually have a config option for
this: CONFIG_STACK_GROWSUP.  But for some reason ia64 doesn't define
this, why not, Tony?  It looks deliberate because you have replaced a
lot of

#ifdef CONFIG_STACK_GROWSUP

with

#if defined(CONFIG_STACK_GROWSUP) || defined(CONFIG_IA64)

but not all of them.

James

RE: [PATCH] debug: Do not permit CONFIG_DEBUG_STACK_USAGE=y on IA64 or PARISC

[Luck, Tony]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1054 bytes --]

> Since the problem is an invalid assumption about how the stack grows,
> why not just condition it on that.  We actually have a config option for
> this: CONFIG_STACK_GROWSUP.  But for some reason ia64 doesn't define
> this, why not, Tony?  It looks deliberate because you have replaced a
> lot of
>
> #ifdef CONFIG_STACK_GROWSUP
>
> with
>
> #if defined(CONFIG_STACK_GROWSUP) || defined(CONFIG_IA64)
>
> but not all of them.

ia64 is special - we have stacks that grow both upwards and downwards!

The typical "C" stack for local function variables that need to be allocated in
memory (arrays, structures, things we take the address of, things that just
don't fit because we run out of registers) grows downwards. But local
variables assigned to registers get quietly saved away to an upwards growing
stack by the register stack engine (working somewhat asynchronously from
the cpu).l

-Tony
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH] debug: Do not permit CONFIG_DEBUG_STACK_USAGE=y on IA64 or PARISC

[Ingo Molnar]

* James Bottomley <James.Bottomley@HansenPartnership.com> wrote:

> On Wed, 2012-07-25 at 09:45 +0200, Ingo Molnar wrote:
> > * Tony Luck <tony.luck@intel.com> wrote:
> > 
> > > The stack_not_used() function in <linux/sched.h> assumes that stacks
> > > grow downwards. This is not true on IA64 or PARISC, so this function
> > > would walk off in the wrong direction and into the weeds.
> > > 
> > > Found on IA64 because of a compilation failure with recursive dependencies
> > > on IA64_TASKSIZE and IA64_THREAD_INFO_SIZE.
> > > 
> > > Fixing the code is possible, but should be combined with other
> > > infrastructure additions to set up the "canary" at the end of the stack.
> > > 
> > > Reported-by: Fengguang Wu <fengguang.wu@intel.com> (failed allmodconfig build)
> > > Signed-off-by: Tony Luck <tony.luck@intel.com>
> > > ---
> > >  lib/Kconfig.debug | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> > > index ff5bdee..4a18650 100644
> > > --- a/lib/Kconfig.debug
> > > +++ b/lib/Kconfig.debug
> > > @@ -714,7 +714,7 @@ config STACKTRACE
> > >  
> > >  config DEBUG_STACK_USAGE
> > >  	bool "Stack utilization instrumentation"
> > > -	depends on DEBUG_KERNEL
> > > +	depends on DEBUG_KERNEL && !IA64 && !PARISC
> > 
> > The modern way of doing this is by adding an ARCH_SUPPORTS_ 
> > flag.
> 
> That's a bit daft, isn't it? [...]

It's generally more maintainable than a random list of 
architecture exclusions because every (old or new) architecture 
can just grep for ARCH_SUPPORTS_ pattern and see whether they 
support everything that others support.

The above exclusion list of architectures is much harder to find 
in a structured way.

> [...]  We'd have to add ARCH_SUPPORTS_ flags to about 25 
> separate architectures just to get it not supported on these 
> two.

That is one off overhead and it makes things easier to maintain 
going forward.

Anyway, that's the current upstream technique and it's been in 
place for years.

> Since the problem is an invalid assumption about how the stack 
> grows, why not just condition it on that.  We actually have a 
> config option for this: CONFIG_STACK_GROWSUP.  But for some 
> reason ia64 doesn't define this, why not, Tony?  It looks 
> deliberate because you have replaced a lot of
> 
> #ifdef CONFIG_STACK_GROWSUP
> 
> with
> 
> #if defined(CONFIG_STACK_GROWSUP) || defined(CONFIG_IA64)
> 
> but not all of them.

Yes, that's another possible solution, assuming that it's really 
only about the up/down difference.

Thanks,

	Ingo

Re: [PATCH] debug: Do not permit CONFIG_DEBUG_STACK_USAGE=y on IA64 or PARISC

[Peter Chubb]

>>>>> "Ingo" == Ingo Molnar <mingo@kernel.org> writes:

Ingo> * James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
>> Since the problem is an invalid assumption about how the stack
>> grows, why not just condition it on that.  We actually have a
>> config option for this: CONFIG_STACK_GROWSUP.  But for some reason
>> ia64 doesn't define this, why not, Tony?  It looks deliberate
>> because you have replaced a lot of
>> 
>> #ifdef CONFIG_STACK_GROWSUP
>> 
>> with
>> 
>> #if defined(CONFIG_STACK_GROWSUP) || defined(CONFIG_IA64)
>> 
>> but not all of them.

Ingo> Yes, that's another possible solution, assuming that it's really
Ingo> only about the up/down difference.

Ingo> Thanks,

IA64 has two stacks -- the standard one, that grows down, and the
register stack engine backing store, that grows up.  The usual
mechanisms for stack growth are used, so only some of the bits
predicated on `STACK_GROWSUP' are useful.

Peter C
--
Dr Peter Chubb				        peter.chubb AT nicta.com.au
http://www.ssrg.nicta.com.au          Software Systems Research Group/NICTA

Re: [PATCH] debug: Do not permit CONFIG_DEBUG_STACK_USAGE=y on IA64 or PARISC

[James Bottomley]

On Wed, 2012-07-18 at 10:35 -0700, Tony Luck wrote:
> The stack_not_used() function in <linux/sched.h> assumes that stacks
> grow downwards. This is not true on IA64 or PARISC, so this function
> would walk off in the wrong direction and into the weeds.

OK, so looking at all of this, that statement's not quite true ... at
least for parisc, we begin the stack where end_of_stack() says the end
should be and so the walker will likely find the next word after the
canary skip occupied and terminate there, so we think the stack is
larger than it really is.  It gets the wrong value, but it will never
even walk out of the stack area.

> Found on IA64 because of a compilation failure with recursive dependencies
> on IA64_TASKSIZE and IA64_THREAD_INFO_SIZE.
> 
> Fixing the code is possible, but should be combined with other
> infrastructure additions to set up the "canary" at the end of the stack.

I agree with this.  Most of it looks easily fixable, but how would I
enable the fix for ia64?  For PA it's simple: I'll just use
CONFIG_STACK_GROWSUP, but that won't work for you.

James

RE: [PATCH] debug: Do not permit CONFIG_DEBUG_STACK_USAGE=y on IA64 or PARISC

[Luck, Tony]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 846 bytes --]

> I agree with this.  Most of it looks easily fixable, but how would I
> enable the fix for ia64?  For PA it's simple: I'll just use
> CONFIG_STACK_GROWSUP, but that won't work for you.

ia64 has an ugly chicken vs. egg build dependency. When trying to build our asm-offsets.h
file (to get #define constants for various structure sizes and offsets in a format that is
usable in assembly code) we get:

include/linux/sched.h:2539: error: 'IA64_TASK_SIZE' undeclared (first use in this function)

Which is sad because IA64_TASK_SIZE is one of the #defines that asm-offsets.h is trying
to produce.

Which is why I just threw up my hands in despair and said "!IA64" for this option.

-Tony
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥