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

[Dave Martin <Dave.Martin@arm.com>]

On Wed, May 20, 2020 at 11:36:40AM +0100, Will Deacon wrote:
> On Wed, May 20, 2020 at 11:27:47AM +0100, Dave Martin wrote:
> > On Wed, May 20, 2020 at 10:50:28AM +0100, Will Deacon wrote:
> > > On Wed, May 20, 2020 at 10:42:13AM +0100, Dave Martin wrote:
> > > > On Tue, May 19, 2020 at 05:28:21PM +0100, Will Deacon wrote:
> > > > > @@ -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.
> > > 
> > > Hmm, I'm not sure I get what you're asking for. We can't recover the other
> > > registers even if we tried, can we? I think the only way to get a reliable
> > > backtrace here is not to clobber the framepointer.
> > 
> > A caller somewhere up the stack could have stashed stuff in nonstandard
> > places, with a custom unwind entry that doesn't use x29 in the usual way.
> > 
> > If x29 and x30 were stashed in x8 and x9, say, then the unwinder needs
> > to restore x8 and x9 correctly before that frame is reached.  Dwarf
> > unwind tables are expressive enough to describe how to unwind such a
> > frames: the directives work on all the registers, not just x29, lr.
> 
> Understood, I just can't figure out how we could support that even if we
> wanted to. The only evidence we have of those registers is in the
> sigcontext, but that may have been modified by the time we end up in the
> return trampoline. Would we need to push the registers twice (i.e. expand
> the frame record to include the GPRs)? Not saying we should do this, just
> wondering what it would take.

No, it's inevitably best effort.

If the signal handler doesn't intend to return, than the backtrace may
be nonsense anyway.  The signal might result from the regs being garbage
anyway, or the signal might be deliberate suicide by the "caller".

If the signal handler does intend to return normally, then it is
responsible for manipulating the sigcontext in a way that doesn't break
the interrupted code -- which implies that the backtrace will be valid,
and also means that invasive non-atomic changes to the sigcontext are
unlikely.  Because we can't know the intent of the handler, no amount
of pushing duplicates etc. can work 100% of the time.

The overwhelmingly common case of is that the signal handler doesn't
mess with sigcontext at all, though.  So we could probably restore the
integer regs correctly for the common case.


Nonetheless, there are limitations.  Dwarf unwind can't describe how to
unwind the FPSIMD/SVE regs etc.  We're really into debugger territory
if we start to care about that stuff.


> > For this kind of unwinding scenario to wokr, the userspace environment
> > would need to provide correct unwind info for _everything_ rather than
> > relying on the frame chain on the stack alone, so this scenario isn't
> > applicable to C.
> > 
> > I'm not saying we should try to support this, but a comment to indicate
> > what we are and are not trying to do might be a good idea.
> > 
> > How about something along these lines:
> > 
> > /*
> >  * Don't try to provide unwind into for the other regs of the
> >  * interrupted context here.  C/C++ based runtimes don't rely on
> >  * this for unwinding in practice.  Debuggers need more, but they
> >  * already have baked-in knowledge about how to unwind out of
> >  * signals.
> >  */
> 
> I'll fold that in, thanks.

Thanks.  This just avoids having to ask the question again or go back
over all the messy rationale above.

If someone _needs_ this to be extended in future, we can revisit it.
But I hope not!

Cheers
---Dave

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