Subject: Re: [PATCH v2 2/2] arm64: vdso: Fix CFI directives in sigreturn trampoline

From: Dave Martin <[email-masked]>
Date: Wed, 20 May 2020 10:42:13 +0100

On Tue, May 19, 2020 at 05:28:21PM +0100, Will Deacon wrote:
> Daniel reports that the .cfi_startproc is misplaced for the sigreturn
> trampoline, which causes LLVM's unwinder to misbehave:
> 
>   | I run into this with LLVM’s unwinder.
>   | This combination was always broken.
> 
> This prompted Dave to question our use of CFI directives more generally,
> and I ended up going down a rabbit hole trying to figure out how this
> very poorly documented stuff gets used.
> 
> Move the CFI directives so that the "mysterious NOP" is included in
> the .cfi_{start,end}proc block and add a bunch of comments so that I
> can save myself another headache in future.
> 
> Cc: Tamas Zsoldos <[email-masked]>
> Reported-by: Dave Martin <[email-masked]>
> Reported-by: Daniel Kiss <[email-masked]>
> Signed-off-by: Will Deacon <[email-masked]>
> ---
>  arch/arm64/kernel/vdso/sigreturn.S | 40 ++++++++++++++++++++++++------
>  1 file changed, 33 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/arm64/kernel/vdso/sigreturn.S b/arch/arm64/kernel/vdso/sigreturn.S
> index 0c921130002a..cb47dfb3bd5a 100644
> --- a/arch/arm64/kernel/vdso/sigreturn.S
> +++ b/arch/arm64/kernel/vdso/sigreturn.S
> @@ -1,7 +1,11 @@
>  /* SPDX-License-Identifier: GPL-2.0-only */
>  /*
>   * Sigreturn trampoline for returning from a signal when the SA_RESTORER
> - * flag is not set.
> + * flag is not set. It serves primarily as a hall of shame for crappy
> + * unwinders and features an exciting but mysterious NOP instruction.
> + *
> + * It's also fragile as hell, so please think twice before changing anything
> + * in here.
>   *
>   * Copyright (C) 2012 ARM Limited
>   *
> @@ -14,7 +18,34 @@
>  
>  	.text
>  
> -	nop
> +/* Ensure that the mysterious NOP can be associated with a function. */
> +	.cfi_startproc
> +
> +/*
> + * .cfi_signal_frame causes the corresponding Frame Description Entry in the
> + * .eh_frame section to be annotated as a signal frame. This allows DWARF
> + * unwinders (e.g. libstdc++) to implement _Unwind_GetIPInfo(), which permits
> + * unwinding out of the signal trampoline without the need for the mysterious
> + * NOP.
> + */
> +	.cfi_signal_frame
> +
> +/*
> + * Tell the unwinder where to locate the frame record linking back to the
> + * interrupted context.
> + */
> +	.cfi_def_cfa    x29, 0
> +	.cfi_offset     x29, 0 * 8
> +	.cfi_offset     x29, 1 * 8

We should also give rationale for why we don't describe how to recover
other regs here.  At a signal, every reg is potentially live with data
essential to the backtrace, so custom unwind entries further up the
stack may unwind badly after trying to unwind out of the signal handler.

Otherwise, looks reasonable -- it should be easier now to understand
next time!

[...]

Cheers
---Dave

_______________________________________________
linux-arm-kernel mailing list
[email-masked]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel