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

[Mark Rutland]

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

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.

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

Thanks,
Mark.