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

From: Will Deacon <will@kernel.org>
To: linux-arm-kernel@lists.infradead.org
Cc: Tamas Zsoldos <tamas.zsoldos@arm.com>,
	Mark Brown <broonie@kernel.org>,
	kernel-team@android.com, Dave Martin <dave.martin@arm.com>,
	Daniel Kiss <daniel.kiss@arm.com>
Subject: Re: [PATCH v2 2/2] arm64: vdso: Fix CFI directives in sigreturn trampoline
Date: Wed, 20 May 2020 11:48:03 +0100	[thread overview]
Message-ID: <20200520104803.GA25694@willie-the-truck> (raw)
In-Reply-To: <20200519162821.16857-3-will@kernel.org>

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 <tamas.zsoldos@arm.com>
> Reported-by: Dave Martin <dave.martin@arm.com>
> Reported-by: Daniel Kiss <daniel.kiss@arm.com>
> Signed-off-by: Will Deacon <will@kernel.org>
> ---
>  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

Oops, just spotted this bug: second entry should be x30.

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel