Re: [PATCH 3/3] 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" <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:55:37 +0100	[thread overview]
Message-ID: <20200519135537.GG5031@arm.com> (raw)
In-Reply-To: <20200519133941.GB14570@willie-the-truck>

On Tue, May 19, 2020 at 02:39:41PM +0100, Will Deacon wrote:
> 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.

Well, we've demonstrated that identifying the signal frame is a gross
bodge.  The cfi annotation should provide a reliable way to identify the
signal frame, but I guess it was too poorly specified or came too late
to prevent the bodges from spreading.

Since this seems to be a nonstandard invention, I wouldn't hold out
much hope of finding a usable spec.

Of course, something might be using it now, so I guess we have to leave
it.

---Dave

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