[PATCH 1/2] powerpc/irq: don't use current_stack_pointer() in check_stack_overflow()

[PATCH 1/2] powerpc/irq: don't use current_stack_pointer() in check_stack_overflow()

[Christophe Leroy]

current_stack_pointer() doesn't return the stack pointer, but the
caller's stack frame. See commit bfe9a2cfe91a ("powerpc: Reimplement
__get_SP() as a function not a define") and commit acf620ecf56c
("powerpc: Rename __get_SP() to current_stack_pointer()") for details.

The purpose of check_stack_overflow() is to verify that the stack has
not overflowed.

To really know whether the stack pointer is still within boundaries,
the check must be done directly on the value of r1.

Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
 arch/powerpc/kernel/irq.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c
index bb34005ff9d2..4d468d835558 100644
--- a/arch/powerpc/kernel/irq.c
+++ b/arch/powerpc/kernel/irq.c
@@ -599,9 +599,8 @@ u64 arch_irq_stat_cpu(unsigned int cpu)
 static inline void check_stack_overflow(void)
 {
 #ifdef CONFIG_DEBUG_STACKOVERFLOW
-	long sp;
-
-	sp = current_stack_pointer() & (THREAD_SIZE-1);
+	register unsigned long r1 asm("r1");
+	long sp = r1 & (THREAD_SIZE - 1);
 
 	/* check for stack overflow: is there less than 2KB free? */
 	if (unlikely(sp < 2048)) {
-- 
2.13.3

[PATCH 2/2] powerpc/irq: use IS_ENABLED() in check_stack_overflow()

[Christophe Leroy]

Instead of #ifdef, use IS_ENABLED(CONFIG_DEBUG_STACKOVERFLOW).
This enable GCC to check for code validity even when the option
is not selected.

Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
 arch/powerpc/kernel/irq.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c
index 4d468d835558..0aebd7843c73 100644
--- a/arch/powerpc/kernel/irq.c
+++ b/arch/powerpc/kernel/irq.c
@@ -598,16 +598,17 @@ u64 arch_irq_stat_cpu(unsigned int cpu)
 
 static inline void check_stack_overflow(void)
 {
-#ifdef CONFIG_DEBUG_STACKOVERFLOW
 	register unsigned long r1 asm("r1");
 	long sp = r1 & (THREAD_SIZE - 1);
 
+	if (!IS_ENABLED(CONFIG_DEBUG_STACKOVERFLOW))
+		return;
+
 	/* check for stack overflow: is there less than 2KB free? */
 	if (unlikely(sp < 2048)) {
 		pr_err("do_IRQ: stack overflow: %ld\n", sp);
 		dump_stack();
 	}
-#endif
 }
 
 #ifdef CONFIG_PPC32
-- 
2.13.3

Re: [PATCH 1/2] powerpc/irq: don't use current_stack_pointer() in check_stack_overflow()

[Michael Ellerman]

Christophe Leroy <christophe.leroy@c-s.fr> writes:

> current_stack_pointer() doesn't return the stack pointer, but the
> caller's stack frame. See commit bfe9a2cfe91a ("powerpc: Reimplement
> __get_SP() as a function not a define") and commit acf620ecf56c
> ("powerpc: Rename __get_SP() to current_stack_pointer()") for details.
>
> The purpose of check_stack_overflow() is to verify that the stack has
> not overflowed.
>
> To really know whether the stack pointer is still within boundaries,
> the check must be done directly on the value of r1.
>
> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
> ---
>  arch/powerpc/kernel/irq.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c
> index bb34005ff9d2..4d468d835558 100644
> --- a/arch/powerpc/kernel/irq.c
> +++ b/arch/powerpc/kernel/irq.c
> @@ -599,9 +599,8 @@ u64 arch_irq_stat_cpu(unsigned int cpu)
>  static inline void check_stack_overflow(void)
>  {
>  #ifdef CONFIG_DEBUG_STACKOVERFLOW
> -	long sp;
> -
> -	sp = current_stack_pointer() & (THREAD_SIZE-1);
> +	register unsigned long r1 asm("r1");
> +	long sp = r1 & (THREAD_SIZE - 1);

This appears to work but seems to be "unsupported" by GCC, and clang
actually complains about it:

  /linux/arch/powerpc/kernel/irq.c:603:12: error: variable 'r1' is uninitialized when used here [-Werror,-Wuninitialized]
          long sp = r1 & (THREAD_SIZE - 1);
                    ^~

The GCC docs say:

  The only supported use for this feature is to specify registers for
  input and output operands when calling Extended asm (see Extended
  Asm).

https://gcc.gnu.org/onlinedocs/gcc-9.1.0/gcc/Local-Register-Variables.html#Local-Register-Variables


If I do this it seems to work, but feels a little dicey:

	asm ("" : "=r" (r1));
	sp = r1 & (THREAD_SIZE - 1);


Generated code looks OK ish:

clang:

        sp = r1 & (THREAD_SIZE - 1);
    22e0:       a0 04 24 78     clrldi  r4,r1,50
        if (unlikely(sp < 2048)) {
    22e4:       ff 07 24 28     cmpldi  r4,2047
    22e8:       58 00 81 40     ble     2340 <do_IRQ+0xe0>


gcc:
	if (unlikely(sp < 2048)) {
    2eb4:	00 38 28 70 	andi.   r8,r1,14336
...
    2ecc:	24 00 82 40 	bne     c000000000002ef0 <do_IRQ+0xa0>


cheers

Re: [PATCH 1/2] powerpc/irq: don't use current_stack_pointer() in check_stack_overflow()

[Christophe Leroy]

Le 24/01/2020 à 06:46, Michael Ellerman a écrit :
> Christophe Leroy <christophe.leroy@c-s.fr> writes:
> 
>> current_stack_pointer() doesn't return the stack pointer, but the
>> caller's stack frame. See commit bfe9a2cfe91a ("powerpc: Reimplement
>> __get_SP() as a function not a define") and commit acf620ecf56c
>> ("powerpc: Rename __get_SP() to current_stack_pointer()") for details.
>>
>> The purpose of check_stack_overflow() is to verify that the stack has
>> not overflowed.
>>
>> To really know whether the stack pointer is still within boundaries,
>> the check must be done directly on the value of r1.
>>
>> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
>> ---
>>   arch/powerpc/kernel/irq.c | 5 ++---
>>   1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c
>> index bb34005ff9d2..4d468d835558 100644
>> --- a/arch/powerpc/kernel/irq.c
>> +++ b/arch/powerpc/kernel/irq.c
>> @@ -599,9 +599,8 @@ u64 arch_irq_stat_cpu(unsigned int cpu)
>>   static inline void check_stack_overflow(void)
>>   {
>>   #ifdef CONFIG_DEBUG_STACKOVERFLOW
>> -	long sp;
>> -
>> -	sp = current_stack_pointer() & (THREAD_SIZE-1);
>> +	register unsigned long r1 asm("r1");
>> +	long sp = r1 & (THREAD_SIZE - 1);
> 
> This appears to work but seems to be "unsupported" by GCC, and clang
> actually complains about it:
> 
>    /linux/arch/powerpc/kernel/irq.c:603:12: error: variable 'r1' is uninitialized when used here [-Werror,-Wuninitialized]
>            long sp = r1 & (THREAD_SIZE - 1);
>                      ^~
> 
> The GCC docs say:
> 
>    The only supported use for this feature is to specify registers for
>    input and output operands when calling Extended asm (see Extended
>    Asm).
> 
> https://gcc.gnu.org/onlinedocs/gcc-9.1.0/gcc/Local-Register-Variables.html#Local-Register-Variables
> 
> 
> If I do this it seems to work, but feels a little dicey:
> 
> 	asm ("" : "=r" (r1));
> 	sp = r1 & (THREAD_SIZE - 1);


Or we could do add in asm/reg.h what we have in boot/reg.h:

register void *__stack_pointer asm("r1");
#define get_sp()	(__stack_pointer)

And use get_sp()

I'll try it.

Christophe

Re: [PATCH 1/2] powerpc/irq: don't use current_stack_pointer() in check_stack_overflow()

[Christophe Leroy]

On 01/24/2020 06:19 AM, Christophe Leroy wrote:
> 
> 
> Le 24/01/2020 à 06:46, Michael Ellerman a écrit :
>>
>> If I do this it seems to work, but feels a little dicey:
>>
>>     asm ("" : "=r" (r1));
>>     sp = r1 & (THREAD_SIZE - 1);
> 
> 
> Or we could do add in asm/reg.h what we have in boot/reg.h:
> 
> register void *__stack_pointer asm("r1");
> #define get_sp()    (__stack_pointer)
> 
> And use get_sp()
> 

It works, and I guess doing it this way is acceptable as it's exactly 
what's done for current in asm/current.h with register r2.

Now I (still) get:

	sp = get_sp() & (THREAD_SIZE - 1);
  b9c:	54 24 04 fe 	clrlwi  r4,r1,19
	if (unlikely(sp < 2048)) {
  ba4:	2f 84 07 ff 	cmpwi   cr7,r4,2047





Allthough GCC 8.1 what doing exactly the same with the form CLANG don't 
like:

	register unsigned long r1 asm("r1");
	long sp = r1 & (THREAD_SIZE - 1);
  b84:	54 24 04 fe 	clrlwi  r4,r1,19
	if (unlikely(sp < 2048)) {
  b8c:	2f 84 07 ff 	cmpwi   cr7,r4,2047


Christophe

Re: [PATCH 1/2] powerpc/irq: don't use current_stack_pointer() in check_stack_overflow()

[Segher Boessenkool]

On Fri, Jan 24, 2020 at 07:03:36AM +0000, Christophe Leroy wrote:
> >Le 24/01/2020 à 06:46, Michael Ellerman a écrit :
> >>
> >>If I do this it seems to work, but feels a little dicey:
> >>
> >>    asm ("" : "=r" (r1));
> >>    sp = r1 & (THREAD_SIZE - 1);
> >
> >
> >Or we could do add in asm/reg.h what we have in boot/reg.h:
> >
> >register void *__stack_pointer asm("r1");
> >#define get_sp()    (__stack_pointer)
> >
> >And use get_sp()
> >
> 
> It works, and I guess doing it this way is acceptable as it's exactly 
> what's done for current in asm/current.h with register r2.

That is a *global* register variable.  That works.  We still need to
document a bit better what it does exactly, but this is the expected
use case, so that will work.

> Now I (still) get:
> 
> 	sp = get_sp() & (THREAD_SIZE - 1);
>  b9c:	54 24 04 fe 	clrlwi  r4,r1,19
> 	if (unlikely(sp < 2048)) {
>  ba4:	2f 84 07 ff 	cmpwi   cr7,r4,2047
> 
> Allthough GCC 8.1 what doing exactly the same with the form CLANG don't 
> like:
> 
> 	register unsigned long r1 asm("r1");
> 	long sp = r1 & (THREAD_SIZE - 1);
>  b84:	54 24 04 fe 	clrlwi  r4,r1,19
> 	if (unlikely(sp < 2048)) {
>  b8c:	2f 84 07 ff 	cmpwi   cr7,r4,2047

Sure, if it did what you expected, things will usually work out fine ;-)

(Pity that the compiler didn't come up with
    rlwinm. r4,r1,0,19,20
    bne bad
Or are the low bits of r4 used later again?)


Segher

Re: [PATCH 1/2] powerpc/irq: don't use current_stack_pointer() in check_stack_overflow()

[Segher Boessenkool]

Hi!

On Fri, Jan 24, 2020 at 04:46:24PM +1100, Michael Ellerman wrote:
> Christophe Leroy <christophe.leroy@c-s.fr> writes:
> >  static inline void check_stack_overflow(void)
> >  {
> >  #ifdef CONFIG_DEBUG_STACKOVERFLOW
> > -	long sp;
> > -
> > -	sp = current_stack_pointer() & (THREAD_SIZE-1);
> > +	register unsigned long r1 asm("r1");
> > +	long sp = r1 & (THREAD_SIZE - 1);
> 
> This appears to work but seems to be "unsupported" by GCC, and clang
> actually complains about it:
> 
>   /linux/arch/powerpc/kernel/irq.c:603:12: error: variable 'r1' is uninitialized when used here [-Werror,-Wuninitialized]
>           long sp = r1 & (THREAD_SIZE - 1);
>                     ^~
> 
> The GCC docs say:
> 
>   The only supported use for this feature is to specify registers for
>   input and output operands when calling Extended asm (see Extended
>   Asm).
> 
> https://gcc.gnu.org/onlinedocs/gcc-9.1.0/gcc/Local-Register-Variables.html#Local-Register-Variables

Yes.  Don't use local register variables any other way.  It *will* break.

> If I do this it seems to work, but feels a little dicey:
> 
> 	asm ("" : "=r" (r1));
> 	sp = r1 & (THREAD_SIZE - 1);

The only thing dicey about that is that you are writing to r1.  Heh.
Well that certainly is bad enough, the compiler does not know how to
handle that at all...  Of course you aren't *actually* changing
anything, so it might just work.


Segher