Re: [PATCH] Fix unwind_frame for clang-built kernels

From: Miles Chen
Date: Thu Jun 18 2020 - 21:55:13 EST


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@xxxxxxxxxxxxxxxx> 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@xxxxxxxxxx>
>
> 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@xxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxx>

>
> 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@xxxxxxxxxxxxxxx
>
> 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@xxxxxxxxxx>
>
> 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
> >
> > --
>