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

From: Dave Martin <Dave.Martin@arm.com>
To: dankis01 <dankis01@arm.com>
Cc: mark.rutland@arm.com, Tamas Zsoldos <tamas.zsoldos@arm.com>,
	Vincenzo.Frascino@arm.com, linux-arm-kernel@lists.infradead.org,
	Daniel Kiss <daniel.kiss@arm.com>
Subject: Re: [PATCH] arm64: vdso: Fix CFI info in sigreturn.
Date: Mon, 18 May 2020 16:59:27 +0100	[thread overview]
Message-ID: <20200518155926.GA21779@arm.com> (raw)
In-Reply-To: <20200515162020.78169-1-daniel.kiss@arm.com>

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
> 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?

Why should there be any instruction that "calls" the signal handler?
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...

> 
> 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.

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

> +SYM_FUNC_START(__kernel_rt_sigreturn)
>  	mov	x8, #__NR_rt_sigreturn
>  	svc	#0
>  	.cfi_endproc
> -- 
> 2.17.1

[...]

Cheers
---Dave

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