[PATCH] powerpc/64: Avoid link stack corruption in kexec_wait()

Subsystems: linux for powerpc (32-bit and 64-bit), the rest

STALE1809d

4 messages, 2 authors, 2021-08-31 · open the first message on its own page

[PATCH] powerpc/64: Avoid link stack corruption in kexec_wait()

From: Christophe Leroy <hidden>
Date: 2021-08-23 07:53:12

Use bcl 20,31,+4 instead of bl in order to preserve link stack.

See commit c974809a26a1 ("powerpc/vdso: Avoid link stack corruption
in __get_datapage()") for details.

Signed-off-by: Christophe Leroy <redacted>
---
 arch/powerpc/kernel/misc_64.S | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/kernel/misc_64.S b/arch/powerpc/kernel/misc_64.S
index 4b761a18a74d..613509907166 100644
--- a/arch/powerpc/kernel/misc_64.S
+++ b/arch/powerpc/kernel/misc_64.S
@@ -255,7 +255,7 @@ _GLOBAL(scom970_write)
  * Physical (hardware) cpu id should be in r3.
  */
 _GLOBAL(kexec_wait)
-	bl	1f
+	bcl	20,31,1f
 1:	mflr	r5
 	addi	r5,r5,kexec_flag-1b
 
-- 
2.25.0

Re: [PATCH] powerpc/64: Avoid link stack corruption in kexec_wait()

From: Daniel Axtens <hidden>
Date: 2021-08-31 06:17:57

Hi Christophe,
Use bcl 20,31,+4 instead of bl in order to preserve link stack.

See commit c974809a26a1 ("powerpc/vdso: Avoid link stack corruption
in __get_datapage()") for details.
From my understanding of that commit message, the change helps to keep
the link stack correctly balanced which is helpful for performance,
rather than for correctness. If I understand correctly, kexec_wait is
not in a hot path - rather it is where CPUs spin while waiting for
kexec. Is there any benefit in using the more complicated opcode in this
situation?
quoted hunk
Signed-off-by: Christophe Leroy <redacted>
---
 arch/powerpc/kernel/misc_64.S | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/kernel/misc_64.S b/arch/powerpc/kernel/misc_64.S
index 4b761a18a74d..613509907166 100644
--- a/arch/powerpc/kernel/misc_64.S
+++ b/arch/powerpc/kernel/misc_64.S
@@ -255,7 +255,7 @@ _GLOBAL(scom970_write)
  * Physical (hardware) cpu id should be in r3.
  */
 _GLOBAL(kexec_wait)
-	bl	1f
+	bcl	20,31,1f
 1:	mflr	r5
Would it be better to create a macro of some sort to wrap this unusual
special form so that the meaning is more clear?

Kind regards,
Daniel
 	addi	r5,r5,kexec_flag-1b
 
-- 
2.25.0

Re: [PATCH] powerpc/64: Avoid link stack corruption in kexec_wait()

From: Christophe Leroy <hidden>
Date: 2021-08-31 08:54:50


Le 31/08/2021 à 08:17, Daniel Axtens a écrit :
Hi Christophe,
quoted
Use bcl 20,31,+4 instead of bl in order to preserve link stack.

See commit c974809a26a1 ("powerpc/vdso: Avoid link stack corruption
in __get_datapage()") for details.
 From my understanding of that commit message, the change helps to keep
the link stack correctly balanced which is helpful for performance,
rather than for correctness. If I understand correctly, kexec_wait is
not in a hot path - rather it is where CPUs spin while waiting for
kexec. Is there any benefit in using the more complicated opcode in this
situation?
AFAICS the main benefit is to keep things consistent over the kernel and not have to wonder "is it a 
hot path or not ? If it is I use bcl 20,31, if it is not I use bl". The best way to keep things in 
order is to always use the right instruction.
quoted
Signed-off-by: Christophe Leroy <redacted>
---
  arch/powerpc/kernel/misc_64.S | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/kernel/misc_64.S b/arch/powerpc/kernel/misc_64.S
index 4b761a18a74d..613509907166 100644
--- a/arch/powerpc/kernel/misc_64.S
+++ b/arch/powerpc/kernel/misc_64.S
@@ -255,7 +255,7 @@ _GLOBAL(scom970_write)
   * Physical (hardware) cpu id should be in r3.
   */
  _GLOBAL(kexec_wait)
-	bl	1f
+	bcl	20,31,1f
  1:	mflr	r5
Would it be better to create a macro of some sort to wrap this unusual
special form so that the meaning is more clear?
Not sure, I think people working with assembly will easily recognise that form whereas an obscure 
macro is always puzzling.

I like macros when they allow you to not repeat again and again the same sequence of several 
instructions, but here it is a single quite simple instruction which is not worth a macro in my mind.

Christophe

Re: [PATCH] powerpc/64: Avoid link stack corruption in kexec_wait()

From: Daniel Axtens <hidden>
Date: 2021-08-31 12:43:12

Christophe Leroy [off-list ref] writes:
Le 31/08/2021 à 08:17, Daniel Axtens a écrit :
quoted
Hi Christophe,
quoted
Use bcl 20,31,+4 instead of bl in order to preserve link stack.

See commit c974809a26a1 ("powerpc/vdso: Avoid link stack corruption
in __get_datapage()") for details.
 From my understanding of that commit message, the change helps to keep
the link stack correctly balanced which is helpful for performance,
rather than for correctness. If I understand correctly, kexec_wait is
not in a hot path - rather it is where CPUs spin while waiting for
kexec. Is there any benefit in using the more complicated opcode in this
situation?
AFAICS the main benefit is to keep things consistent over the kernel and not have to wonder "is it a 
hot path or not ? If it is I use bcl 20,31, if it is not I use bl". The best way to keep things in 
order is to always use the right instruction.
Yeah, Nick Piggin convinced me of this offline as well.
quoted
quoted
Signed-off-by: Christophe Leroy <redacted>
---
  arch/powerpc/kernel/misc_64.S | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/kernel/misc_64.S b/arch/powerpc/kernel/misc_64.S
index 4b761a18a74d..613509907166 100644
--- a/arch/powerpc/kernel/misc_64.S
+++ b/arch/powerpc/kernel/misc_64.S
@@ -255,7 +255,7 @@ _GLOBAL(scom970_write)
   * Physical (hardware) cpu id should be in r3.
   */
  _GLOBAL(kexec_wait)
-	bl	1f
+	bcl	20,31,1f
  1:	mflr	r5
Would it be better to create a macro of some sort to wrap this unusual
special form so that the meaning is more clear?
Not sure, I think people working with assembly will easily recognise that form whereas an obscure 
macro is always puzzling.

I like macros when they allow you to not repeat again and again the same sequence of several 
instructions, but here it is a single quite simple instruction which is not worth a macro in my mind.

Sure - I was mostly thinking specifically of the bcl; mflr situation but
I agree that for the single instruction it's not needed.

In short, I am convinced, and so:
Reviewed-by: Daniel Axtens <redacted>

Kind regards,
Daniel
Christophe
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help