* [PATCH] arm64: vdso: Fix CFI info in sigreturn. @ 2020-05-15 16:20 dankis01 2020-05-18 15:59 ` Dave Martin 0 siblings, 1 reply; 7+ messages in thread From: dankis01 @ 2020-05-15 16:20 UTC (permalink / raw) To: linux-arm-kernel, Vincenzo.Frascino, mark.rutland Cc: Tamas Zsoldos, Daniel Kiss 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] arm64: vdso: Fix CFI info in sigreturn. 2020-05-15 16:20 [PATCH] arm64: vdso: Fix CFI info in sigreturn dankis01 @ 2020-05-18 15:59 ` Dave Martin 2020-05-18 17:00 ` Daniel Kiss 0 siblings, 1 reply; 7+ messages in thread From: Dave Martin @ 2020-05-18 15:59 UTC (permalink / raw) To: dankis01 Cc: mark.rutland, Tamas Zsoldos, Vincenzo.Frascino, linux-arm-kernel, Daniel Kiss 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] arm64: vdso: Fix CFI info in sigreturn. 2020-05-18 15:59 ` Dave Martin @ 2020-05-18 17:00 ` Daniel Kiss 2020-05-19 9:29 ` Dave Martin 0 siblings, 1 reply; 7+ messages in thread From: Daniel Kiss @ 2020-05-18 17:00 UTC (permalink / raw) To: Dave P Martin Cc: Mark Rutland, Tamas Zsoldos, Vincenzo Frascino, linux-arm-kernel@lists.infradead.org > 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] arm64: vdso: Fix CFI info in sigreturn. 2020-05-18 17:00 ` Daniel Kiss @ 2020-05-19 9:29 ` Dave Martin 2020-05-19 11:34 ` Will Deacon 0 siblings, 1 reply; 7+ messages in thread From: Dave Martin @ 2020-05-19 9:29 UTC (permalink / raw) To: Daniel Kiss Cc: Mark Rutland, Tamas Zsoldos, Vincenzo Frascino, linux-arm-kernel@lists.infradead.org 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] arm64: vdso: Fix CFI info in sigreturn. 2020-05-19 9:29 ` Dave Martin @ 2020-05-19 11:34 ` Will Deacon 0 siblings, 0 replies; 7+ messages in thread From: Will Deacon @ 2020-05-19 11:34 UTC (permalink / raw) To: Dave Martin Cc: Mark Rutland, Tamas Zsoldos, Vincenzo Frascino, linux-arm-kernel@lists.infradead.org, Daniel Kiss 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
[parent not found: <30E488CA-46FF-4927-A07F-8CE11263B92E@arm.com>]
[parent not found: <CF896434-E995-438C-88F8-86CCFE24C5A2@arm.com>]
* [PATCH] arm64: vdso: Fix CFI info in sigreturn. [not found] ` <CF896434-E995-438C-88F8-86CCFE24C5A2@arm.com> @ 2020-05-08 9:52 ` Daniel Kiss 2020-05-15 15:23 ` Mark Rutland 0 siblings, 1 reply; 7+ messages in thread From: Daniel Kiss @ 2020-05-08 9:52 UTC (permalink / raw) To: linux-arm-kernel@lists.infradead.org; +Cc: Tamas Zsoldos, Vincenzo Frascino [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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] arm64: vdso: Fix CFI info in sigreturn. 2020-05-08 9:52 ` Daniel Kiss @ 2020-05-15 15:23 ` Mark Rutland 0 siblings, 0 replies; 7+ messages in thread From: Mark Rutland @ 2020-05-15 15:23 UTC (permalink / raw) To: Daniel Kiss Cc: Tamas Zsoldos, Vincenzo Frascino, linux-arm-kernel@lists.infradead.org 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-05-19 11:34 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-05-15 16:20 [PATCH] arm64: vdso: Fix CFI info in sigreturn dankis01
2020-05-18 15:59 ` Dave Martin
2020-05-18 17:00 ` Daniel Kiss
2020-05-19 9:29 ` Dave Martin
2020-05-19 11:34 ` Will Deacon
[not found] <30E488CA-46FF-4927-A07F-8CE11263B92E@arm.com>
[not found] ` <CF896434-E995-438C-88F8-86CCFE24C5A2@arm.com>
2020-05-08 9:52 ` Daniel Kiss
2020-05-15 15:23 ` Mark Rutland
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).