* [PATCH] Fix unwind_frame for clang-built kernels @ 2020-06-16 22:36 Nathan Huckleberry 2020-06-17 4:23 ` Sedat Dilek 2020-06-18 20:22 ` Nick Desaulniers 0 siblings, 2 replies; 8+ messages in thread From: Nathan Huckleberry @ 2020-06-16 22:36 UTC (permalink / raw) To: linux, vincent.whitchurch Cc: clang-built-linux, linux-kernel, linux-arm-kernel, 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 <nhuck@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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] Fix unwind_frame for clang-built kernels 2020-06-16 22:36 [PATCH] Fix unwind_frame for clang-built kernels Nathan Huckleberry @ 2020-06-17 4:23 ` Sedat Dilek 2020-06-18 22:03 ` Russell King - ARM Linux admin 2020-06-18 20:22 ` Nick Desaulniers 1 sibling, 1 reply; 8+ messages in thread From: Sedat Dilek @ 2020-06-17 4:23 UTC (permalink / raw) To: Nathan Huckleberry Cc: Clang-Built-Linux ML, vincent.whitchurch, linux, linux-arm-kernel, linux-kernel On Wed, Jun 17, 2020 at 12:36 AM 'Nathan Huckleberry' via Clang Built Linux <clang-built-linux@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 <nhuck@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 - > --- > 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 > > -- > 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-linux+unsubscribe@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. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Fix unwind_frame for clang-built kernels 2020-06-17 4:23 ` Sedat Dilek @ 2020-06-18 22:03 ` Russell King - ARM Linux admin 0 siblings, 0 replies; 8+ messages in thread From: Russell King - ARM Linux admin @ 2020-06-18 22:03 UTC (permalink / raw) To: Sedat Dilek Cc: Clang-Built-Linux ML, vincent.whitchurch, linux-kernel, linux-arm-kernel, Nathan Huckleberry 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-built-linux@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 <nhuck@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! _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Fix unwind_frame for clang-built kernels 2020-06-16 22:36 [PATCH] Fix unwind_frame for clang-built kernels Nathan Huckleberry 2020-06-17 4:23 ` Sedat Dilek @ 2020-06-18 20:22 ` Nick Desaulniers 2020-06-18 20:29 ` Nick Desaulniers 2020-06-19 1:55 ` Miles Chen 1 sibling, 2 replies; 8+ messages in thread From: Nick Desaulniers @ 2020-06-18 20:22 UTC (permalink / raw) To: Nathan Huckleberry, Miles Chen (陳民樺) Cc: Catalin Marinas, Vincent Whitchurch, Russell King, LKML, clang-built-linux, Sedat Dilek, Kristof Beyls, Linux ARM On Tue, Jun 16, 2020 at 3:36 PM 'Nathan Huckleberry' via Clang Built Linux <clang-built-linux@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 <nhuck@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.Chen@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: 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 > --- > 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. > + 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Fix unwind_frame for clang-built kernels 2020-06-18 20:22 ` Nick Desaulniers @ 2020-06-18 20:29 ` Nick Desaulniers 2020-06-19 1:55 ` Miles Chen 1 sibling, 0 replies; 8+ messages in thread From: Nick Desaulniers @ 2020-06-18 20:29 UTC (permalink / raw) To: Nathan Huckleberry, Miles Chen (陳民樺) Cc: Catalin Marinas, Vincent Whitchurch, Russell King, LKML, clang-built-linux, Sedat Dilek, Kristof Beyls, Linux ARM On Thu, Jun 18, 2020 at 1:22 PM Nick Desaulniers <ndesaulniers@google.com> wrote: > > On Tue, Jun 16, 2020 at 3:36 PM 'Nathan Huckleberry' via Clang Built > Linux <clang-built-linux@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 <nhuck@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.Chen@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: 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 > > > --- > > 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. > > > + 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Fix unwind_frame for clang-built kernels 2020-06-18 20:22 ` Nick Desaulniers 2020-06-18 20:29 ` Nick Desaulniers @ 2020-06-19 1:55 ` Miles Chen 2020-07-06 18:27 ` [PATCH v2] ARM: stacktrace: " Nathan Huckleberry 1 sibling, 1 reply; 8+ messages in thread From: Miles Chen @ 2020-06-19 1:55 UTC (permalink / raw) To: Nick Desaulniers Cc: Catalin Marinas, Vincent Whitchurch, LKML, Russell King, clang-built-linux, Sedat Dilek, Kristof Beyls, Nathan Huckleberry, Linux ARM 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 <clang-built-linux@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 <nhuck@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.Chen@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? Thanks for the patch. Tested-by: Miles Chen <miles.chen@mediatek.com> > > 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 > > > --- > > 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. > > > + 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2] ARM: stacktrace: Fix unwind_frame for clang-built kernels 2020-06-19 1:55 ` Miles Chen @ 2020-07-06 18:27 ` Nathan Huckleberry 2020-07-10 16:45 ` Nick Desaulniers 0 siblings, 1 reply; 8+ messages in thread From: Nathan Huckleberry @ 2020-07-06 18:27 UTC (permalink / raw) To: linux, vincent.whitchurch Cc: Nick Desaulniers, linux-kernel, stable, Nathan Huckleberry, clang-built-linux, Miles Chen, 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 <miles.chen@mediatek.com> Tested-by: Miles Chen <miles.chen@mediatek.com> Cc: stable@vger.kernel.org Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> Signed-off-by: Nathan Huckleberry <nhuck@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.212.ge8ba1cc988-goog _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2] ARM: stacktrace: Fix unwind_frame for clang-built kernels 2020-07-06 18:27 ` [PATCH v2] ARM: stacktrace: " Nathan Huckleberry @ 2020-07-10 16:45 ` Nick Desaulniers 0 siblings, 0 replies; 8+ messages in thread From: Nick Desaulniers @ 2020-07-10 16:45 UTC (permalink / raw) To: Nathan Huckleberry Cc: Vincent Whitchurch, Russell King, # 3.4.x, LKML, clang-built-linux, Miles Chen, Linux ARM 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 <nhuck@google.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 > Reported-by: Miles Chen <miles.chen@mediatek.com> > Tested-by: Miles Chen <miles.chen@mediatek.com> > Cc: stable@vger.kernel.org > Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> > Signed-off-by: Nathan Huckleberry <nhuck@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.212.ge8ba1cc988-goog > -- Thanks, ~Nick Desaulniers _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-07-10 16:47 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-06-16 22:36 [PATCH] Fix unwind_frame for clang-built kernels Nathan Huckleberry 2020-06-17 4:23 ` Sedat Dilek 2020-06-18 22:03 ` Russell King - ARM Linux admin 2020-06-18 20:22 ` Nick Desaulniers 2020-06-18 20:29 ` Nick Desaulniers 2020-06-19 1:55 ` Miles Chen 2020-07-06 18:27 ` [PATCH v2] ARM: stacktrace: " Nathan Huckleberry 2020-07-10 16:45 ` Nick Desaulniers
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).