[PATCH] arm64: stacktrace: Relax frame record alignment requirement to 8 bytes

[Peter Collingbourne]

On Mon, Oct 26, 2020 at 5:42 AM Mark Rutland <mark.rutland at arm.com> wrote:
>
> Hi,
>
> On Fri, Oct 23, 2020 at 05:58:35PM -0700, Peter Collingbourne wrote:
> > The AAPCS places no requirements on the alignment of the frame
> > record. In theory it could be placed anywhere, although it seems
> > sensible to require it to be aligned to 8 bytes.
>
> To save people finding the spec, the latest AAPCS doc (2020Q3, as of
> 1st October 2020), says:
>
> | Conforming code shall construct a linked list of stack-frames. Each
> | frame shall link to the frame of its caller by means of a frame record
> | of two 64-bit values on the stack (independent of the data model).
>
> ... and I can confirm there's no mention of additional alignment
> restrictions. The "Universal stack constraints" only mandatre alignment
> for the SP, and not the FP. I would expect per the usual data model
> conventions each 64-bit value should be at least naturally aligned, and
> if people want to argue that we should tighten the spec to more clearly
> mandate at least that.
>
> > With an upcoming enhancement to tag-based KASAN Clang will begin
> > creating frame records located at an address that is only aligned to 8
> > bytes. Accommodate such frame records in the stack unwinding code.
>
> This is going to break user unwinding, too. I Can see our
> perf_callchain_user() expects 16-byte alignment, and that will need a
> similar fixup.

I see. Note that we have already had these frame records enabled for
at least a year in Android's userspace implementation of tag-based
ASAN. Since such a frame record would only appear after tag-based ASAN
has detected an error, and at that point we are about to crash anyway
(since in userspace we don't currently support resumption from error
detection) it would be unlikely that we would see this in practice. I
guess it could only happen if we are taking a perf recording and
happen to sample just before we crash. It seems worth fixing though
and I've done that in v2.

> I also fear this has the potential to break things like GDB, glibc, or
> libunwind. Are we certain nothing else assumes 16-byte alignment?

For the userspace implementation of tag-based ASAN we use the system
unwinder (libgcc at this point) to unwind past these stack frames to
obtain a stack trace and haven't had a problem with it.

I'm not sure about the other tools. I don't recall seeing a problem
with this on gdb, and we don't use glibc on Android but I think it
uses the libgcc unwinder as well. If there are any problems with them
that can be dealt with separately of course.

> I'm a bit surprised this is of any benefit to Clang for codegen. Is the
> SP always 16-byte aligned, or is that similary relaxed within functions?
> Disregarding entry asm, Linux requires the SP to always be 16-byte
> aligned at all times for correct operation, so if that is relaxed we
> have bigger problems.

No, we aren't misaligning SP. The affected functions store the frame
record at sp+232 [1] and then call a helper runtime function that
points fp to the frame record [2] (that's the userspace implementation
of the runtime function, the kernel implementation would do the same).

The stack frames created by this function are intended to be used to
save the contents of every GPR for an error report. For the
convenience of the consumer the contents of register xN are stored at
offset sp+8*N and you can see that this naturally results in the frame
record being aligned to 8 and not 16.

[1] https://clang.llvm.org/docs/HardwareAssistedAddressSanitizerDesign.html#memory-accesses
[2] http://llvm-cs.pcc.me.uk/projects/compiler-rt/lib/hwasan/hwasan_tag_mismatch_aarch64.S#116

> > Signed-off-by: Peter Collingbourne <pcc at google.com>
> > Link: https://linux-review.googlesource.com/id/Ia22c375230e67ca055e9e4bb639383567f7ad268
> > ---
> >  arch/arm64/kernel/stacktrace.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
> > index fa56af1a59c3..5dfc05068328 100644
> > --- a/arch/arm64/kernel/stacktrace.c
> > +++ b/arch/arm64/kernel/stacktrace.c
> > @@ -44,7 +44,7 @@ int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame)
> >       unsigned long fp = frame->fp;
> >       struct stack_info info;
> >
> > -     if (fp & 0xf)
> > +     if (fp & 0x7)
> >               return -EINVAL;
>
> The unwind code implictly relies on this alignment for its accessibility
> checks -- knowing that this is aligned means we know that if the start
> address of a frame record is on one stack the entire record is on that
> stack.
>
> To be able to safely relax this, we'll need to change
> on_accessible_stack() to take the size of the object being checked so
> that we can ensure that doesn't straddle a stack boundary.

Okay, I've done that in v2 as a dependent patch.

Peter