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

From: Will Deacon <will@kernel.org>
To: Dave P Martin <Dave.Martin@arm.com>
Cc: Tamas Zsoldos <Tamas.Zsoldos@arm.com>,
	Mark Brown <broonie@kernel.org>,
	"kernel-team@android.com" <kernel-team@android.com>,
	"linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>,
	Daniel Kiss <Daniel.Kiss@arm.com>
Subject: Re: [PATCH 3/3] arm64: vdso: Fix CFI directives in sigreturn trampoline
Date: Tue, 19 May 2020 14:39:41 +0100	[thread overview]
Message-ID: <20200519133941.GB14570@willie-the-truck> (raw)
In-Reply-To: <20200519130930.GO10636@e103592.cambridge.arm.com>

On Tue, May 19, 2020 at 02:09:31PM +0100, Dave P Martin wrote:
> On Tue, May 19, 2020 at 01:18:18PM +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 realise that our CFI directives are contradictory,
> > as we specify both .cfi_signal_frame *and* .cfi_def_cfa, with the latter
> > unconditionally identifying the interrupted context as opposed to the
> > values in the sigcontext.
> > 
> > Rework the CFI directives so that we only use .cfi_signal_frame, and
> > include the "mysterious NOP" as part of the .cfi_{start,end}proc block.
> > 
> > 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 | 8 +++-----
> >  1 file changed, 3 insertions(+), 5 deletions(-)
> > 
> > diff --git a/arch/arm64/kernel/vdso/sigreturn.S b/arch/arm64/kernel/vdso/sigreturn.S
> > index 7853fa9692f6..28b33f7d0604 100644
> > --- a/arch/arm64/kernel/vdso/sigreturn.S
> > +++ b/arch/arm64/kernel/vdso/sigreturn.S
> > @@ -14,6 +14,9 @@
> >  
> >  	.text
> >  
> > +/* Ensure that the mysterious NOP can be associated with a function. */
> > +	.cfi_startproc
> > +	.cfi_signal_frame
> >  /*
> >   * This mysterious NOP is required for some unwinders that subtract one from
> >   * the return address in order to identify the calling function.
> > @@ -28,11 +31,6 @@
> >   * is perfectly fine.
> >   */
> >  SYM_START(__kernel_rt_sigreturn, SYM_L_GLOBAL, SYM_A_ALIGN)
> > -	.cfi_startproc
> > -	.cfi_signal_frame
> > -	.cfi_def_cfa	x29, 0
> > -	.cfi_offset	x29, 0 * 8
> > -	.cfi_offset	x30, 1 * 8
> 
> Having thought about this again, I think it might be better to stick to
> the original version.
> 
> If the signal handler is halfway through mungeing the sigcontext then
> backtracing using sigcontext won't be reliable.

I suppose, but then what does .cfi_signal_frame do? I'll see if I can
find something that uses it. The frame record is still sitting on the
stack, so it does feel redundant to say both '.cfi_signal_frame' and
'.cfi_def_cfa' (and other architectures, e.g. riscv don't do this).

But I'm also happy to play it safe if I can stick a comment in here
saying what it does.

> Plus, in the absence of any spec that says exactly what
> .cfi_signal_frame means*, we probably don't want to rock the boat.

The gas docs say:

	"Mark current function as signal trampoline."

which is really informative.

Will

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