[PATCH] Fix unwind_frame for clang-built kernels

[Nathan Huckleberry]

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 <nh...@google.com>
---
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

[Sedat Dilek]

On Wed, Jun 17, 2020 at 12:36 AM 'Nathan Huckleberry' via Clang Built
Linux <clang-bu...@googlegroups.com> 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 <nh...@google.com>

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 -

> --
> You received this message because you are subscribed to the Google Groups "Clang Built Linux" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to clang-built-li...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/clang-built-linux/20200616223633.73971-1-nhuck%40google.com.

[Nick Desaulniers]

On Tue, Jun 16, 2020 at 3:36 PM 'Nathan Huckleberry' via Clang Built
Linux <clang-bu...@googlegroups.com> 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 <nh...@google.com>

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 <Miles...@mediatek.com>

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: sta...@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 <ndesau...@google.com>

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

> + 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

[Nick Desaulniers]

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

--
Thanks,
~Nick Desaulniers

[Russell King - ARM Linux admin]

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 <clang-bu...@googlegroups.com> 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 <nh...@google.com>
>
> 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!

[Miles Chen]

Thanks for the patch.

Tested-by: Miles Chen <miles...@mediatek.com>

[Nathan Huckleberry]

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 <miles...@mediatek.com>
Tested-by: Miles Chen <miles...@mediatek.com>
Cc: sta...@vger.kernel.org
Reviewed-by: Nick Desaulniers <ndesau...@google.com>
Signed-off-by: Nathan Huckleberry <nh...@google.com>

+ 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

[Nick Desaulniers]

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.

--
Thanks,
~Nick Desaulniers