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

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

[Daniel Kiss]

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

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.
Signed-off-by: Daniel Kiss <daniel.kiss@arm.com(opens in new tab)>
Signed-off-by: Tamas Zsoldos <tamas.zsoldos@arm.com(opens in new tab)>
---
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
+ nop /* placeholder for bl signalhandler */
+SYM_FUNC_START(__kernel_rt_sigreturn)
mov x8, #__NR_rt_sigreturn
svc #0
.cfi_endproc
-- 
2.17.1



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

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

[Mark Rutland]

Hi Daniel,

On Fri, May 08, 2020 at 09:52:14AM +0000, Daniel Kiss wrote:
> [PATCH] arm64: vdso: Fix CFI info in sigreturn.
> 
> 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.
> Signed-off-by: Daniel Kiss <daniel.kiss@arm.com(opens in new tab)>
> Signed-off-by: Tamas Zsoldos <tamas.zsoldos@arm.com(opens in new tab)>

Something appears to have gone wrong here; was this copy-pasted from
somewhere?

> ---
> 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
> + nop /* placeholder for bl signalhandler */
> +SYM_FUNC_START(__kernel_rt_sigreturn)
> mov x8, #__NR_rt_sigreturn
> svc #0
> .cfi_endproc

This appears to have been whitespace damaged (leading tabs have gone).

Can you please resend this via git-send-email?

Thanks,
Mark.

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

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

[Will Deacon]

On Tue, May 19, 2020 at 10:29:37AM +0100, Dave Martin wrote:
> On Mon, May 18, 2020 at 05:00:32PM +0000, Daniel Kiss wrote:
> > >> 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.

I think we should just remove the .cfi_def_cfa and .cfi_offset directives
from this sequence. Daniel, Tamas, is that ok with you?

> > 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 think it is to do with unwinding, where something was subtracting 1
from the return address to identify the caller. There's an old relic here
that sheds a bit of light on it all (we inherited this from ppc, it seems):

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=26208#c15

However, looking at gdb just now, it looks like it tries to match the
instructions in the trampoline in order to identify a signal frame. We just
changed that with the BTI patches, so I think it's now broken. Given that we
don't need the BTI C in there (we only call the thing via RET), I'll send a
patch to remove it.

Will

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

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

[Dave Martin]

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

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

[Daniel Kiss]

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

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

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

[Dave Martin]

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

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

[dankis01]

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.

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
+	nop  /* placeholder for bl signalhandler */
+SYM_FUNC_START(__kernel_rt_sigreturn)
 	mov	x8, #__NR_rt_sigreturn
 	svc	#0
 	.cfi_endproc
-- 
2.17.1


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