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

From: Dave Martin <Dave.Martin@arm•com>
To: Will Deacon <will@kernel•org>
Cc: Tamas Zsoldos <tamas.zsoldos@arm•com>,
	Mark Brown <broonie@kernel•org>,
	kernel-team@android•com, linux-arm-kernel@lists•infradead.org,
	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 12:06:53 +0100	[thread overview]
Message-ID: <20200520110652.GO5031@arm.com> (raw)
In-Reply-To: <20200520104803.GA25694@willie-the-truck>

On Wed, May 20, 2020 at 11:48:03AM +0100, Will Deacon wrote:
> 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.

Dang, didn't spot that.  Yes.

Must have been momory corruption in your editor...

Cheers
---Dave

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