[PATCH] Fix unwind_frame for clang-built kernels

Since clang does not push pc and sp in function prologues, the current
implementation of unwind_frame does not work. By using the previous
frame's lr/fp instead of saved pc/sp we get valid unwinds on clang-built
kernels.

The bounds check on next frame pointer must be changed as well since
there are 8 less bytes between frames.

This fixes /proc/<pid>/stack.

Link: https://github.com/ClangBuiltLinux/linux/issues/912
Signed-off-by: Nathan Huckleberry <redacted>
---
 arch/arm/kernel/stacktrace.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/arch/arm/kernel/stacktrace.c b/arch/arm/kernel/stacktrace.c
index cc726afea023..76ea4178a55c 100644
--- a/arch/arm/kernel/stacktrace.c
+++ b/arch/arm/kernel/stacktrace.c

@@ -22,6 +22,19 @@
  * A simple function epilogue looks like this:
  *	ldm	sp, {fp, sp, pc}
  *
+ * When compiled with clang, pc and sp are not pushed. A simple function
+ * prologue looks like this when built with clang:
+ *
+ *	stmdb	{..., fp, lr}
+ *	add	fp, sp, #x
+ *	sub	sp, sp, #y
+ *
+ * A simple function epilogue looks like this when built with clang:
+ *
+ *	sub	sp, fp, #x
+ *	ldm	{..., fp, pc}
+ *
+ *
  * Note that with framepointer enabled, even the leaf functions have the same
  * prologue and epilogue, therefore we can ignore the LR value in this case.
  */

@@ -34,6 +47,16 @@ int notrace unwind_frame(struct stackframe *frame)
 	low = frame->sp;
 	high = ALIGN(low, THREAD_SIZE);
 
+#ifdef CONFIG_CC_IS_CLANG
+	/* check current frame pointer is within bounds */
+	if (fp < low + 4 || fp > high - 4)
+		return -EINVAL;
+
+	frame->sp = frame->fp;
+	frame->fp = *(unsigned long *)(fp);
+	frame->pc = frame->lr;
+	frame->lr = *(unsigned long *)(fp + 4);
+#else
 	/* check current frame pointer is within bounds */
 	if (fp < low + 12 || fp > high - 4)
 		return -EINVAL;

@@ -42,6 +65,7 @@ int notrace unwind_frame(struct stackframe *frame)
 	frame->fp = *(unsigned long *)(fp - 12);
 	frame->sp = *(unsigned long *)(fp - 8);
 	frame->pc = *(unsigned long *)(fp - 4);
+#endif
 
 	return 0;
 }

-- 
2.27.0.290.gba653c62da-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Wed, Jun 17, 2020 at 12:36 AM 'Nathan Huckleberry' via Clang Built
Linux [off-list ref] wrote:

Since clang does not push pc and sp in function prologues, the current
implementation of unwind_frame does not work. By using the previous
frame's lr/fp instead of saved pc/sp we get valid unwinds on clang-built
kernels.

The bounds check on next frame pointer must be changed as well since
there are 8 less bytes between frames.

This fixes /proc/<pid>/stack.

Link: https://github.com/ClangBuiltLinux/linux/issues/912
Signed-off-by: Nathan Huckleberry <redacted>

Just a small nit.

Please label your patch with: "arm/stacktrace: ..." or "arm: stacktrace: ..."

git log --oneline arch/arm/kernel/stacktrace.c
...is your friend :-).

- Sedat -

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Tue, Jun 16, 2020 at 3:36 PM 'Nathan Huckleberry' via Clang Built
Linux [off-list ref] wrote:

Since clang does not push pc and sp in function prologues, the current
implementation of unwind_frame does not work. By using the previous
frame's lr/fp instead of saved pc/sp we get valid unwinds on clang-built
kernels.

The bounds check on next frame pointer must be changed as well since
there are 8 less bytes between frames.

This fixes /proc/<pid>/stack.

Link: https://github.com/ClangBuiltLinux/linux/issues/912
Signed-off-by: Nathan Huckleberry <redacted>

Thanks for the patch, Nathan!  When I looked into this, I found the
latest ARM AAPCS [0] specifically says (with `it` referring to `a
platform`: "It may permit the frame pointer register to be used as a
general-purpose callee-saved register, but provide a platform-specific
mechanism for external agents to reliably locate the chain of frame
records."   While it's good that's acknowledged in the documentation,
the current wording is relaxed in order to not force current
implementations to converge.  This has the unfortunate side effect of
making finding the frame pointer toolchain dependendent, hence this
patch and your previous commit 6dc5fd93b2f1 ("ARM: 8900/1:
UNWINDER_FRAME_POINTER implementation for Clang").  Being more
specific in the documentation would force at least one implementation
to change, and I think that would also imply an ABI break.  So I can
see it both ways, though I still would prefer that the language pin
this down, even if we had to change LLVM.  Just providing additional
context for folks on the thread.

This should also have a reported by tag from Miles, in v2.

Reported-by: Miles Chen <redacted>

Miles mentioned to me that he tested it, but maybe Miles can confirm
that publicly on-list via an explicit Tested-by: tag?

This would be useful for us to have in stable; otherwise we'll have to
carry out of tree in Android and CrOS, which I'd rather not do.  Via
Documentation/process/stable-kernel-rules.rst, if you add this tag to
V2, that will greatly simplify submitting this to stable:
Cc: stable@vger.kernel.org

Miles also showed me the behavior of this patch for different kernel
versions, which varies anywhere from empty or single entry traces to
panics, so this is pretty important that this works for Clang builds.

[0] https://static.docs.arm.com/ihi0042/i/aapcs32.pdf

The patch LGTM; maybe Russell or Catalin could triple check this
bounds check?  Assuming there's no issue, you can include on a v2 my
reviewed by:

Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

I'd probably wait the remainder of a week before sending a v2 to
collect additional feedback.  Thank you again.

-- 
Thanks,
~Nick Desaulniers

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Thu, Jun 18, 2020 at 1:22 PM Nick Desaulniers
[off-list ref] wrote:

On Tue, Jun 16, 2020 at 3:36 PM 'Nathan Huckleberry' via Clang Built
Linux [off-list ref] wrote:

quoted

Since clang does not push pc and sp in function prologues, the current
implementation of unwind_frame does not work. By using the previous
frame's lr/fp instead of saved pc/sp we get valid unwinds on clang-built
kernels.

The bounds check on next frame pointer must be changed as well since
there are 8 less bytes between frames.

This fixes /proc/<pid>/stack.

Link: https://github.com/ClangBuiltLinux/linux/issues/912
Signed-off-by: Nathan Huckleberry <redacted>

Thanks for the patch, Nathan!  When I looked into this, I found the
latest ARM AAPCS [0] specifically says (with `it` referring to `a
platform`: "It may permit the frame pointer register to be used as a
general-purpose callee-saved register, but provide a platform-specific
mechanism for external agents to reliably locate the chain of frame
records."   While it's good that's acknowledged in the documentation,
the current wording is relaxed in order to not force current
implementations to converge.  This has the unfortunate side effect of
making finding the frame pointer toolchain dependendent, hence this
patch and your previous commit 6dc5fd93b2f1 ("ARM: 8900/1:
UNWINDER_FRAME_POINTER implementation for Clang").  Being more
specific in the documentation would force at least one implementation
to change, and I think that would also imply an ABI break.  So I can
see it both ways, though I still would prefer that the language pin
this down, even if we had to change LLVM.  Just providing additional
context for folks on the thread.

This should also have a reported by tag from Miles, in v2.

Reported-by: Miles Chen <redacted>

Miles mentioned to me that he tested it, but maybe Miles can confirm
that publicly on-list via an explicit Tested-by: tag?

This would be useful for us to have in stable; otherwise we'll have to
carry out of tree in Android and CrOS, which I'd rather not do.  Via
Documentation/process/stable-kernel-rules.rst, if you add this tag to
V2, that will greatly simplify submitting this to stable:
Cc: stable@vger.kernel.org

Miles also showed me the behavior of this patch for different kernel

s/this patch/this function before this patch is applied/

versions, which varies anywhere from empty or single entry traces to
panics, so this is pretty important that this works for Clang builds.

[0] https://static.docs.arm.com/ihi0042/i/aapcs32.pdf

quoted

---
 arch/arm/kernel/stacktrace.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/arch/arm/kernel/stacktrace.c b/arch/arm/kernel/stacktrace.c
index cc726afea023..76ea4178a55c 100644
--- a/arch/arm/kernel/stacktrace.c
+++ b/arch/arm/kernel/stacktrace.c

@@ -22,6 +22,19 @@
  * A simple function epilogue looks like this:
  *     ldm     sp, {fp, sp, pc}
  *
+ * When compiled with clang, pc and sp are not pushed. A simple function
+ * prologue looks like this when built with clang:
+ *
+ *     stmdb   {..., fp, lr}
+ *     add     fp, sp, #x
+ *     sub     sp, sp, #y
+ *
+ * A simple function epilogue looks like this when built with clang:
+ *
+ *     sub     sp, fp, #x
+ *     ldm     {..., fp, pc}
+ *
+ *
  * Note that with framepointer enabled, even the leaf functions have the same
  * prologue and epilogue, therefore we can ignore the LR value in this case.
  */

@@ -34,6 +47,16 @@ int notrace unwind_frame(struct stackframe *frame)
        low = frame->sp;
        high = ALIGN(low, THREAD_SIZE);

+#ifdef CONFIG_CC_IS_CLANG
+       /* check current frame pointer is within bounds */
+       if (fp < low + 4 || fp > high - 4)

The patch LGTM; maybe Russell or Catalin could triple check this
bounds check?  Assuming there's no issue, you can include on a v2 my
reviewed by:

Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

I'd probably wait the remainder of a week before sending a v2 to
collect additional feedback.  Thank you again.

quoted

+               return -EINVAL;
+
+       frame->sp = frame->fp;
+       frame->fp = *(unsigned long *)(fp);
+       frame->pc = frame->lr;
+       frame->lr = *(unsigned long *)(fp + 4);
+#else
        /* check current frame pointer is within bounds */
        if (fp < low + 12 || fp > high - 4)
                return -EINVAL;

@@ -42,6 +65,7 @@ int notrace unwind_frame(struct stackframe *frame)
        frame->fp = *(unsigned long *)(fp - 12);
        frame->sp = *(unsigned long *)(fp - 8);
        frame->pc = *(unsigned long *)(fp - 4);
+#endif

        return 0;
 }
--

2.27.0.290.gba653c62da-goog

--

--
Thanks,
~Nick Desaulniers

-- 
Thanks,
~Nick Desaulniers

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Wed, Jun 17, 2020 at 06:23:48AM +0200, Sedat Dilek wrote:

On Wed, Jun 17, 2020 at 12:36 AM 'Nathan Huckleberry' via Clang Built
Linux [off-list ref] wrote:

quoted

Since clang does not push pc and sp in function prologues, the current
implementation of unwind_frame does not work. By using the previous
frame's lr/fp instead of saved pc/sp we get valid unwinds on clang-built
kernels.

The bounds check on next frame pointer must be changed as well since
there are 8 less bytes between frames.

This fixes /proc/<pid>/stack.

Link: https://github.com/ClangBuiltLinux/linux/issues/912
Signed-off-by: Nathan Huckleberry <redacted>

Just a small nit.

Please label your patch with: "arm/stacktrace: ..." or "arm: stacktrace: ..."

Convention is to use "ARM: ..." for arch/arm.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Thu, 2020-06-18 at 13:22 -0700, Nick Desaulniers wrote:

On Tue, Jun 16, 2020 at 3:36 PM 'Nathan Huckleberry' via Clang Built
Linux [off-list ref] wrote:

quoted

Since clang does not push pc and sp in function prologues, the current
implementation of unwind_frame does not work. By using the previous
frame's lr/fp instead of saved pc/sp we get valid unwinds on clang-built
kernels.

The bounds check on next frame pointer must be changed as well since
there are 8 less bytes between frames.

This fixes /proc/<pid>/stack.

Link: https://github.com/ClangBuiltLinux/linux/issues/912
Signed-off-by: Nathan Huckleberry <redacted>

Thanks for the patch, Nathan!  When I looked into this, I found the
latest ARM AAPCS [0] specifically says (with `it` referring to `a
platform`: "It may permit the frame pointer register to be used as a
general-purpose callee-saved register, but provide a platform-specific
mechanism for external agents to reliably locate the chain of frame
records."   While it's good that's acknowledged in the documentation,
the current wording is relaxed in order to not force current
implementations to converge.  This has the unfortunate side effect of
making finding the frame pointer toolchain dependendent, hence this
patch and your previous commit 6dc5fd93b2f1 ("ARM: 8900/1:
UNWINDER_FRAME_POINTER implementation for Clang").  Being more
specific in the documentation would force at least one implementation
to change, and I think that would also imply an ABI break.  So I can
see it both ways, though I still would prefer that the language pin
this down, even if we had to change LLVM.  Just providing additional
context for folks on the thread.

This should also have a reported by tag from Miles, in v2.

Reported-by: Miles Chen <redacted>

Miles mentioned to me that he tested it, but maybe Miles can confirm
that publicly on-list via an explicit Tested-by: tag?

Thanks for the patch.

Tested-by: Miles Chen <redacted>

This would be useful for us to have in stable; otherwise we'll have to
carry out of tree in Android and CrOS, which I'd rather not do.  Via
Documentation/process/stable-kernel-rules.rst, if you add this tag to
V2, that will greatly simplify submitting this to stable:
Cc: stable@vger.kernel.org

Miles also showed me the behavior of this patch for different kernel
versions, which varies anywhere from empty or single entry traces to
panics, so this is pretty important that this works for Clang builds.

[0] https://static.docs.arm.com/ihi0042/i/aapcs32.pdf

quoted

---
 arch/arm/kernel/stacktrace.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/arch/arm/kernel/stacktrace.c b/arch/arm/kernel/stacktrace.c
index cc726afea023..76ea4178a55c 100644
--- a/arch/arm/kernel/stacktrace.c
+++ b/arch/arm/kernel/stacktrace.c

@@ -22,6 +22,19 @@
  * A simple function epilogue looks like this:
  *     ldm     sp, {fp, sp, pc}
  *
+ * When compiled with clang, pc and sp are not pushed. A simple function
+ * prologue looks like this when built with clang:
+ *
+ *     stmdb   {..., fp, lr}
+ *     add     fp, sp, #x
+ *     sub     sp, sp, #y
+ *
+ * A simple function epilogue looks like this when built with clang:
+ *
+ *     sub     sp, fp, #x
+ *     ldm     {..., fp, pc}
+ *
+ *
  * Note that with framepointer enabled, even the leaf functions have the same
  * prologue and epilogue, therefore we can ignore the LR value in this case.
  */

@@ -34,6 +47,16 @@ int notrace unwind_frame(struct stackframe *frame)
        low = frame->sp;
        high = ALIGN(low, THREAD_SIZE);

+#ifdef CONFIG_CC_IS_CLANG
+       /* check current frame pointer is within bounds */
+       if (fp < low + 4 || fp > high - 4)

The patch LGTM; maybe Russell or Catalin could triple check this
bounds check?  Assuming there's no issue, you can include on a v2 my
reviewed by:

Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

I'd probably wait the remainder of a week before sending a v2 to
collect additional feedback.  Thank you again.

quoted

+               return -EINVAL;
+
+       frame->sp = frame->fp;
+       frame->fp = *(unsigned long *)(fp);
+       frame->pc = frame->lr;
+       frame->lr = *(unsigned long *)(fp + 4);
+#else
        /* check current frame pointer is within bounds */
        if (fp < low + 12 || fp > high - 4)
                return -EINVAL;

@@ -42,6 +65,7 @@ int notrace unwind_frame(struct stackframe *frame)
        frame->fp = *(unsigned long *)(fp - 12);
        frame->sp = *(unsigned long *)(fp - 8);
        frame->pc = *(unsigned long *)(fp - 4);
+#endif

        return 0;
 }
--

2.27.0.290.gba653c62da-goog

--

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Since clang does not push pc and sp in function prologues, the current
implementation of unwind_frame does not work. By using the previous
frame's lr/fp instead of saved pc/sp we get valid unwinds on clang-built
kernels.

The bounds check on next frame pointer must be changed as well since
there are 8 less bytes between frames.

This fixes /proc/<pid>/stack.

Link: https://github.com/ClangBuiltLinux/linux/issues/912
Reported-by: Miles Chen <redacted>
Tested-by: Miles Chen <redacted>
Cc: stable@vger.kernel.org
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
Signed-off-by: Nathan Huckleberry <redacted>
---
 arch/arm/kernel/stacktrace.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/arch/arm/kernel/stacktrace.c b/arch/arm/kernel/stacktrace.c
index cc726afea023..76ea4178a55c 100644
--- a/arch/arm/kernel/stacktrace.c
+++ b/arch/arm/kernel/stacktrace.c

@@ -22,6 +22,19 @@
  * A simple function epilogue looks like this:
  *	ldm	sp, {fp, sp, pc}
  *
+ * When compiled with clang, pc and sp are not pushed. A simple function
+ * prologue looks like this when built with clang:
+ *
+ *	stmdb	{..., fp, lr}
+ *	add	fp, sp, #x
+ *	sub	sp, sp, #y
+ *
+ * A simple function epilogue looks like this when built with clang:
+ *
+ *	sub	sp, fp, #x
+ *	ldm	{..., fp, pc}
+ *
+ *
  * Note that with framepointer enabled, even the leaf functions have the same
  * prologue and epilogue, therefore we can ignore the LR value in this case.
  */

@@ -34,6 +47,16 @@ int notrace unwind_frame(struct stackframe *frame)
 	low = frame->sp;
 	high = ALIGN(low, THREAD_SIZE);
 
+#ifdef CONFIG_CC_IS_CLANG
+	/* check current frame pointer is within bounds */
+	if (fp < low + 4 || fp > high - 4)
+		return -EINVAL;
+
+	frame->sp = frame->fp;
+	frame->fp = *(unsigned long *)(fp);
+	frame->pc = frame->lr;
+	frame->lr = *(unsigned long *)(fp + 4);
+#else
 	/* check current frame pointer is within bounds */
 	if (fp < low + 12 || fp > high - 4)
 		return -EINVAL;

@@ -42,6 +65,7 @@ int notrace unwind_frame(struct stackframe *frame)
 	frame->fp = *(unsigned long *)(fp - 12);
 	frame->sp = *(unsigned long *)(fp - 8);
 	frame->pc = *(unsigned long *)(fp - 4);
+#endif
 
 	return 0;
 }

-- 
2.27.0.212.ge8ba1cc988-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Hi Nathan,
Would you please submit this to Russell's patch tracking system:
https://www.arm.linux.org.uk/developer/patches/
login -> add new patch.

On Mon, Jul 6, 2020 at 11:27 AM Nathan Huckleberry [off-list ref] wrote:

-- 
Thanks,
~Nick Desaulniers

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel