Re: [PATCH] arm64: vdso: Fix CFI info in sigreturn.

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

On Mon, May 18, 2020 at 05:00:32PM +0000, Daniel Kiss wrote:
> 
> 
> > On 18 May 2020, at 17:59, Dave Martin <Dave.Martin@arm.com> wrote:
> > 
> > On Fri, May 15, 2020 at 06:20:20PM +0200, dankis01 wrote:
> >> When the signal handler is called the registers set up as the return address
> >> points to the __kernel_rt_sigreturn. The NOP here is the placeholder of the

(Just to be clear about why I originally picked up on this, your
statement about the purpose of the NOP here seems to be an assumption.
Can you say how you reached this conclusion?)

> >> branch and link instruction that "calls" the signal handler. In case of a
> >> return address the unwinder identifies the location of the caller because
> >> in some cases the return address might not exist. Since the .cfi_startproc
> >> is after the NOP, it won't be associated with any location and the
> >> unwinder will stop walking.
> >> 
> >> This change corrects the generated EHFrames only.
> > 
> > This is a can of worms.
> > 
> > Which unwinder are you look at, and what do other unwinders do?  Are you
> > sure the unwinder is doing something valid?  Is this a newly observed
> > problem, or has it happened forever?
> I run into this with LLVM’s unwinder.
> This combination was always broken.

OK, so we've narrowed the breakage down to one of two things ;)

I still don't see why there must be a valid instruction (or even mapped
memory) before lr.  Examples include backtracing noreturn functions, and
backtracing the SIGSEGV when execution falls through into a non-executable
page.

So, sigreturn is just one example if this issue.

This is why I'm not sure the problem is well-understood.  Adding a nop
into the __kernel_sigreturn unwind block may paper over this particular
instance, but what about the other similar scenarios?

> 
> > Why should there be any instruction that "calls" the signal handler?
> It is just from the unwinder/user space point of view.  Normally that instruction would set the return address,
> and some cases in the userspace no instruction is generated for the return address when the compiler knows 
> it is unreachable.
> 
> > In the case is a SIGSEGV the affected instruction is after the pc and
> > not before it; for an asynchronous signal and notion of a "calling"
> > instruction is nonsense.
> > 
> > 
> > Certainly I've seen correct unwinding through signal handlers with glibc
> > and gdb, but I hadn't tried everything…
> GDB recognise __kernel_rt_sigreturn to unwind it correctly, as I see it:
> https://github.com/bminor/binutils-gdb/blob/3580810c51bc17c947d0dd6a7f4eb399d7ca4619/gdb/i386-linux-tdep.c#L265

i386?

> > 
> >> 
> >> Signed-off-by: Daniel Kiss <daniel.kiss@arm.com>
> >> Signed-off-by: Tamas Zsoldos <tamas.zsoldos@arm.com>
> >> ---
> >> arch/arm64/kernel/vdso/sigreturn.S | 4 ++--
> >> 1 file changed, 2 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/arch/arm64/kernel/vdso/sigreturn.S b/arch/arm64/kernel/vdso/sigreturn.S
> >> index 12324863d5c2..5d50ee92faa4 100644
> >> --- a/arch/arm64/kernel/vdso/sigreturn.S
> >> +++ b/arch/arm64/kernel/vdso/sigreturn.S
> >> @@ -13,13 +13,13 @@
> >> 
> >> 	.text
> >> 
> >> -	nop
> >> -SYM_FUNC_START(__kernel_rt_sigreturn)
> >> 	.cfi_startproc
> >> 	.cfi_signal_frame
> >> 	.cfi_def_cfa	x29, 0
> >> 	.cfi_offset	x29, 0 * 8
> >> 	.cfi_offset	x30, 1 * 8
> > 
> > Hmm, recovering x29,x30 like this will be wrong if the signal handler
> > munges sigcontext in the meantime (say, doing some kind of userspace
> > context switch).
> > 
> > They should be pulled out of sigcontext instead really.  AFAIK, that's
> > what ".cfi_signal_frame" is supposed to tell the unwinder.  I'm not sure
> > why we have these additional, conflicting annotations here.
> The unwinder won’t find the “cfi_signal_frame” until it figures out the unwind entry.
> 
> > Any ideas, Will?
> > 
> > This probably isn't related to the bug here, but it would be good to
> > understand.
> > 
> >> +	nop  /* placeholder for bl signalhandler */
> > 
> > Will can correct me on this, but I seem to remember something about nop
> > being there for padding, so that there is a guaranteed gap between
> > unwind entries.
> > 
> > I can't remember the precise reasoning, but there are some nasty edge
> > cases connected with the fact that the linker can place another random
> > unwind block from another .o immediately before this one.

[...]

Cheers
---Dave

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