* Re: [PATCH 0/6] Fix unwinding through sigreturn trampolines [not found] ` <07001CCE-6AF8-4491-9B3A-4FC9C1E27FDB@arm.com> @ 2020-06-23 15:34 ` Daniel Kiss 2020-06-23 15:43 ` Ard Biesheuvel 0 siblings, 1 reply; 6+ messages in thread From: Daniel Kiss @ 2020-06-23 15:34 UTC (permalink / raw) To: Will Deacon Cc: Szabolcs Nagy, Catalin Marinas, Nick Desaulniers, Ard Biesheuvel, Tamas Zsoldos, Vincenzo Frascino, kernel-team@android.com, Dave P Martin, Linux ARM Hi Will, The CFI is correct for PowerPC[1] and X86[2] at least; I did not check others. so LLVM’s unwinder can unwind/backtrace sigreturn without any specific hack on those architectures. The a4eb355a3fda change implements the same CFI as PowerPC and X86 have so the generic unwind logic is able to process the sigreturn. IMHO the kernel change was fine. I’m not comfortable to say the instruction sequence is the ABI that describe the unwinding information. I guess the pattern matching was implemented due to the CFI was wrong. The .cfi_signal_frame could be useful once the entry is found, but needs the right .CFI annotations at the right place. This change definitely a not good for LLVM’s unwinder. Cheers, Daniel [1] PowerPC: The label (.Lsigrt_start) that is used for the CFI info include the nop: https://elixir.bootlin.com/linux/latest/source/arch/powerpc/kernel/vdso64/sigtramp.S#L25 https://elixir.bootlin.com/linux/latest/source/arch/powerpc/kernel/vdso64/sigtramp.S#L291 [2] X86: https://elixir.bootlin.com/linux/latest/source/arch/x86/entry/vdso/vdso32/sigreturn.S#L13 https://elixir.bootlin.com/linux/latest/source/arch/x86/entry/vdso/vdso32/sigreturn.S#L59 > On 23 Jun 2020, at 13:08, Ard Biesheuvel <ardb@kernel.org> wrote: > > On Tue, 23 Jun 2020 at 12:58, Will Deacon <will@kernel.org> wrote: >> >> On Tue, Jun 23, 2020 at 12:11:24PM +0200, Ard Biesheuvel wrote: >>> On Tue, 23 Jun 2020 at 11:54, Dave Martin <Dave.Martin@arm.com> wrote: >>>> In the longer term, we should perhaps keep .cfi_signal_frame, and ditch >>>> all the other annotations. If the unwinder knows for sure it's the >>>> signal frame (and understands this annotation), it can use its own >>>> knowledge to do the right thing. Otherwise, it can fall back on its >>>> other heuristics. >>> >>> What we found out is that libunwind unconditionally uses its own >>> unwind strategy when it spots the mov/svc instruction pair at the >>> return address, and so what we put here does not matter at all. >>> The libgcc case is different, as it only applies the heuristics if it >>> doesn't find any unwind info covering the return address. So this is >>> what bit us here: by fixing the way the CFI directives are emitted, >>> libgcc no longer uses its own baked in unwind strategy for the signal >>> frame, but interprets the one we provide, which results in a bogus >>> address for the /next/ frame. Since that is not covered by unwind info >>> either, libgcc attempts to apply the heuristics at this level, and >>> explodes because the bogus address cannot be dereferenced to get at >>> the opcodes. >>> >>> So libgcc doesn't even check whether it looks like a signal frame >>> unless the unwind info is missing. >>> >>>> This is probably better than having rich annotations that are >>>> pedantically correct, but only half-supported by unwinders out in the >>>> wild. >>>> >>> >>> Agreed. But without providing any annotations, other unwinders will >>> fail to unwind through the sigreturn trampoline, which is why the >>> change was made in the first place. >>> >>> So the safest way to fix this (imho) is to add CFI directives that >>> mirror what libunwind does unconditionally, and libgcc when it spots >>> the mov/svc pair on unannotated addresses, which is to restore all the >>> registers from the sigframe, and to use regs->pc as the return address >>> (which is where the unwind should proceed, rather than at whatever >>> value happened to reside in x30 at the point the signal was taken) >>> >>> The only hairy bit is the FP/SIMD registers, which are ignored by >>> libunwind, and only half-restored by libgcc (which appears to assume >>> that only d8..d15 need to be restored). This means throwing a C++ >>> exception in a signal handler and catching it outside of it is going >>> to be difficult to get 100% correct, but this is something that >>> appears to be broken already everywhere. (Unless there are some >>> AAPCS/ABI rules that specify that FP/SIMD registers have to be assumed >>> clobbered when catching an exception) >> >> I think Dave makes an interesting point about .cfi_signal_frame, though. >> With any released arm64 kernel, unwinding here requires heuristics to >> identify the signal frame. The usual approach seems to pattern match the >> instruction stream (and I'm not convinced libunwind works on big-endian >> here), but you could also have looked up pc+4 to see if the function was >> marked with .cfi_signal_frame. You can't do that anymore after this patch. > > Good point. > >> We have no evidence anybody bothered with that (it seems like more trouble >> than it's worth), but it's an interesting observation. Furthermore, maybe >> the right answer *is* to punt this all to the unwinder given that we're >> already forced to keep the instruction sequence exactly as it is. I suspect >> the LLVM unwinder has to use heuristics for most other architectures >> anyway, so it should already have whatever hooks it needs. >> > > The unwinder is definitely better equipped to deal with this, given > how difficult it is to come up with CFI directives at compile time > that apply universally to any kind of sigframe we may instantiate at > runtime. > >> It would be a different story if most unwinders were expecting to use >> CFI and it generally worked well on other architectures, but that really >> doesn't appear to be the case. >> > > If fixing LLVM unwinding is no longer a requirement, reverting to what > we had is probably the best, although it is rather unfortunate that we > will be encouraging the LLVM developers to put another bodge in user > space. Could we at least encourage them to take the libgcc approach, > where the builtin strategy is only used as a fallback if the return > address is not covered by unwind info? Otherwise, we are painting > ourselves into a corner even more than we have already with respect to > our ability to ever start doing this properly in the future. _______________________________________________ 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] 6+ messages in thread
* Re: [PATCH 0/6] Fix unwinding through sigreturn trampolines 2020-06-23 15:34 ` [PATCH 0/6] Fix unwinding through sigreturn trampolines Daniel Kiss @ 2020-06-23 15:43 ` Ard Biesheuvel [not found] ` <51A5A3CB-B8D1-4BAD-BC7B-0337754AEB9D@arm.com> 0 siblings, 1 reply; 6+ messages in thread From: Ard Biesheuvel @ 2020-06-23 15:43 UTC (permalink / raw) To: Daniel Kiss Cc: Szabolcs Nagy, Will Deacon, Nick Desaulniers, Tamas Zsoldos, Catalin Marinas, Vincenzo Frascino, kernel-team@android.com, Dave P Martin, Linux ARM On Tue, 23 Jun 2020 at 17:34, Daniel Kiss <Daniel.Kiss@arm.com> wrote: > > Hi Will, > > The CFI is correct for PowerPC[1] and X86[2] at least; I did not check others. > so LLVM’s unwinder can unwind/backtrace sigreturn without any specific hack on those architectures. > The a4eb355a3fda change implements the same CFI as PowerPC and X86 have so the generic unwind logic is able to process the sigreturn. It most certainly did not implement the same CFI. The x86 and PowerPC examples you quote have elaborate asm foo to emit the eh_frame by hand. > IMHO the kernel change was fine. > It creates easily reproducible segfaults in the libgcc unwinder. > I’m not comfortable to say the instruction sequence is the ABI that describe the unwinding information. > I guess the pattern matching was implemented due to the CFI was wrong. > > The .cfi_signal_frame could be useful once the entry is found, but needs the right .CFI annotations at the right place. > > This change definitely a not good for LLVM’s unwinder. > I agree that it would be better for the CFI to be correct, but that takes a bit of work at the very least, and even then, the variable nature of our sigframe may make it impossible to restore the FP/SIMD register state reliably. I did some tests with the below CFI directives, where ARM64_SIGFRAME_REGS_OFFSET is emitted by asm-offsets as the offset from the top of the sigframe to the regs[] array. This fixes the segfaults, and seems to do the right thing if i single step through the unwinder as it unwinds the stack in response to a call to pthread_cancel(). But we need a lot of testing to ensure that this is correct. .cfi_def_cfa_offset ARM64_SIGFRAME_REGS_OFFSET .irp r, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, \ 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, \ 24, 25, 26, 27, 28, 29, 30, 31 .cfi_offset \r, \r * 8 .endr .cfi_offset 96, 32 * 8 // regs->pc .cfi_return_column 96 _______________________________________________ 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] 6+ messages in thread
[parent not found: <51A5A3CB-B8D1-4BAD-BC7B-0337754AEB9D@arm.com>]
* Re: [PATCH 0/6] Fix unwinding through sigreturn trampolines [not found] ` <51A5A3CB-B8D1-4BAD-BC7B-0337754AEB9D@arm.com> @ 2020-07-06 9:29 ` Daniel Kiss 2020-07-06 9:41 ` Ard Biesheuvel 2020-07-08 16:57 ` Dave Martin 0 siblings, 2 replies; 6+ messages in thread From: Daniel Kiss @ 2020-07-06 9:29 UTC (permalink / raw) To: Ard Biesheuvel Cc: Szabolcs Nagy, Will Deacon, Nick Desaulniers, Tamas Zsoldos, Catalin Marinas, Vincenzo Frascino, kernel-team@android.com, Dave P Martin, Linux ARM Hi Ard, I like your suggestions and tuned a bit and now it works with the LLVM’s unwinders. Register 96 is out of the DWARF spec[1] and will collide with SVE registers[2] so 32 is better which is the reserved register for PC. my version: #define ARM64_SIGFRAME_REGS_OFFSET 312 /* offsetof (struct rt_sigframe, uc.uc_mcontext.regs) */ .text .cfi_startproc .cfi_signal_frame .cfi_def_cfa sp, ARM64_SIGFRAME_REGS_OFFSET .irp x, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, \ 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, \ 24, 25, 26, 27, 28, 29, 30, 31 .cfi_offset \x, \x * 8 .endr .cfi_offset 32, 32 * 8 // regs->pc .cfi_return_column 32 nop ENTRY(__kernel_rt_sigreturn) mov x8, #__NR_rt_sigreturn svc #0 .cfi_endproc ENDPROC(__kernel_rt_sigreturn) [1] https://developer.arm.com/documentation/ihi0057/e [2] https://developer.arm.com/documentation/100985/0000/ > > >> On 23 Jun 2020, at 17:43, Ard Biesheuvel <ardb@kernel.org> wrote: >> >> On Tue, 23 Jun 2020 at 17:34, Daniel Kiss <Daniel.Kiss@arm.com> wrote: >>> >>> Hi Will, >>> >>> The CFI is correct for PowerPC[1] and X86[2] at least; I did not check others. >>> so LLVM’s unwinder can unwind/backtrace sigreturn without any specific hack on those architectures. >>> The a4eb355a3fda change implements the same CFI as PowerPC and X86 have so the generic unwind logic is able to process the sigreturn. >> >> It most certainly did not implement the same CFI. The x86 and PowerPC >> examples you quote have elaborate asm foo to emit the eh_frame by >> hand. >> >>> IMHO the kernel change was fine. >>> >> >> It creates easily reproducible segfaults in the libgcc unwinder. >> >>> I’m not comfortable to say the instruction sequence is the ABI that describe the unwinding information. >>> I guess the pattern matching was implemented due to the CFI was wrong. >>> >>> The .cfi_signal_frame could be useful once the entry is found, but needs the right .CFI annotations at the right place. >>> >>> This change definitely a not good for LLVM’s unwinder. >>> >> >> I agree that it would be better for the CFI to be correct, but that >> takes a bit of work at the very least, and even then, the variable >> nature of our sigframe may make it impossible to restore the FP/SIMD >> register state reliably. >> >> I did some tests with the below CFI directives, where >> ARM64_SIGFRAME_REGS_OFFSET is emitted by asm-offsets as the offset >> from the top of the sigframe to the regs[] array. This fixes the >> segfaults, and seems to do the right thing if i single step through >> the unwinder as it unwinds the stack in response to a call to >> pthread_cancel(). But we need a lot of testing to ensure that this is >> correct. >> >> >> .cfi_def_cfa_offset ARM64_SIGFRAME_REGS_OFFSET >> .irp r, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, \ >> 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, \ >> 24, 25, 26, 27, 28, 29, 30, 31 >> .cfi_offset \r, \r * 8 >> .endr >> >> .cfi_offset 96, 32 * 8 // regs->pc >> .cfi_return_column 96 > _______________________________________________ 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] 6+ messages in thread
* Re: [PATCH 0/6] Fix unwinding through sigreturn trampolines 2020-07-06 9:29 ` Daniel Kiss @ 2020-07-06 9:41 ` Ard Biesheuvel 2020-07-08 16:57 ` Dave Martin 1 sibling, 0 replies; 6+ messages in thread From: Ard Biesheuvel @ 2020-07-06 9:41 UTC (permalink / raw) To: Daniel Kiss Cc: Szabolcs Nagy, Will Deacon, Nick Desaulniers, Tamas Zsoldos, Catalin Marinas, Vincenzo Frascino, kernel-team@android.com, Dave P Martin, Linux ARM On Mon, 6 Jul 2020 at 12:29, Daniel Kiss <Daniel.Kiss@arm.com> wrote: > > Hi Ard, > > I like your suggestions and tuned a bit and now it works with the LLVM’s unwinders. > > Register 96 is out of the DWARF spec[1] and will collide with SVE registers[2] so 32 is better which is the reserved register for PC. > I just copied that from libgcc's implementation, so using the existing reserved register for that indeed seems like a much better approach. The only remaining question is how to deal with FP/SIMD and SVE registers, but given Szabolcs's assertion that exceptions thrown in signal handlers cannot be caught outside of them anyway, this does not seem like a huge deal, with the exception of the nested function case for pthread cleanup handlers but perhaps the solution for that is 'don't do that'. > my version: > > #define ARM64_SIGFRAME_REGS_OFFSET 312 /* offsetof (struct rt_sigframe, uc.uc_mcontext.regs) */ > > .text > .cfi_startproc > .cfi_signal_frame > > .cfi_def_cfa sp, ARM64_SIGFRAME_REGS_OFFSET > .irp x, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, \ > 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, \ > 24, 25, 26, 27, 28, 29, 30, 31 > .cfi_offset \x, \x * 8 > .endr > > .cfi_offset 32, 32 * 8 // regs->pc > .cfi_return_column 32 > > nop > ENTRY(__kernel_rt_sigreturn) > mov x8, #__NR_rt_sigreturn > svc #0 > .cfi_endproc > ENDPROC(__kernel_rt_sigreturn) > > [1] https://developer.arm.com/documentation/ihi0057/e > [2] https://developer.arm.com/documentation/100985/0000/ > > > > > > >> On 23 Jun 2020, at 17:43, Ard Biesheuvel <ardb@kernel.org> wrote: > >> > >> On Tue, 23 Jun 2020 at 17:34, Daniel Kiss <Daniel.Kiss@arm.com> wrote: > >>> > >>> Hi Will, > >>> > >>> The CFI is correct for PowerPC[1] and X86[2] at least; I did not check others. > >>> so LLVM’s unwinder can unwind/backtrace sigreturn without any specific hack on those architectures. > >>> The a4eb355a3fda change implements the same CFI as PowerPC and X86 have so the generic unwind logic is able to process the sigreturn. > >> > >> It most certainly did not implement the same CFI. The x86 and PowerPC > >> examples you quote have elaborate asm foo to emit the eh_frame by > >> hand. > >> > >>> IMHO the kernel change was fine. > >>> > >> > >> It creates easily reproducible segfaults in the libgcc unwinder. > >> > >>> I’m not comfortable to say the instruction sequence is the ABI that describe the unwinding information. > >>> I guess the pattern matching was implemented due to the CFI was wrong. > >>> > >>> The .cfi_signal_frame could be useful once the entry is found, but needs the right .CFI annotations at the right place. > >>> > >>> This change definitely a not good for LLVM’s unwinder. > >>> > >> > >> I agree that it would be better for the CFI to be correct, but that > >> takes a bit of work at the very least, and even then, the variable > >> nature of our sigframe may make it impossible to restore the FP/SIMD > >> register state reliably. > >> > >> I did some tests with the below CFI directives, where > >> ARM64_SIGFRAME_REGS_OFFSET is emitted by asm-offsets as the offset > >> from the top of the sigframe to the regs[] array. This fixes the > >> segfaults, and seems to do the right thing if i single step through > >> the unwinder as it unwinds the stack in response to a call to > >> pthread_cancel(). But we need a lot of testing to ensure that this is > >> correct. > >> > >> > >> .cfi_def_cfa_offset ARM64_SIGFRAME_REGS_OFFSET > >> .irp r, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, \ > >> 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, \ > >> 24, 25, 26, 27, 28, 29, 30, 31 > >> .cfi_offset \r, \r * 8 > >> .endr > >> > >> .cfi_offset 96, 32 * 8 // regs->pc > >> .cfi_return_column 96 > > > _______________________________________________ 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] 6+ messages in thread
* Re: [PATCH 0/6] Fix unwinding through sigreturn trampolines 2020-07-06 9:29 ` Daniel Kiss 2020-07-06 9:41 ` Ard Biesheuvel @ 2020-07-08 16:57 ` Dave Martin 2020-07-10 17:08 ` Daniel Kiss 1 sibling, 1 reply; 6+ messages in thread From: Dave Martin @ 2020-07-08 16:57 UTC (permalink / raw) To: Daniel Kiss Cc: kernel-team@android.com, Szabolcs Nagy, Catalin Marinas, Nick Desaulniers, Tamas Zsoldos, Vincenzo Frascino, Will Deacon, Ard Biesheuvel, Linux ARM On Mon, Jul 06, 2020 at 09:29:24AM +0000, Daniel Kiss wrote: > Hi Ard, > > I like your suggestions and tuned a bit and now it works with the LLVM’s unwinders. > > Register 96 is out of the DWARF spec[1] and will collide with SVE registers[2] so 32 is better which is the reserved register for PC. > > my version: > > #define ARM64_SIGFRAME_REGS_OFFSET 312 /* offsetof (struct rt_sigframe, uc.uc_mcontext.regs) */ > > .text > .cfi_startproc > .cfi_signal_frame > > .cfi_def_cfa sp, ARM64_SIGFRAME_REGS_OFFSET > .irp x, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, \ > 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, \ > 24, 25, 26, 27, 28, 29, 30, 31 > .cfi_offset \x, \x * 8 > .endr > > .cfi_offset 32, 32 * 8 // regs->pc > .cfi_return_column 32 Have you verified that this works with the GNU unwinders? I seem to remember experimenting with .cfi_return_column in the past and hitting problems when trying to use a fake register. At the time I just assumed I was doing something wrong and didn't go digging. Maybe I _was_ doing something wrong... 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] 6+ messages in thread
* Re: [PATCH 0/6] Fix unwinding through sigreturn trampolines 2020-07-08 16:57 ` Dave Martin @ 2020-07-10 17:08 ` Daniel Kiss 0 siblings, 0 replies; 6+ messages in thread From: Daniel Kiss @ 2020-07-10 17:08 UTC (permalink / raw) To: Dave P Martin Cc: kernel-team@android.com, Szabolcs Nagy, Catalin Marinas, Nick Desaulniers, Tamas Zsoldos, Vincenzo Frascino, Will Deacon, Ard Biesheuvel, Linux ARM > Have you verified that this works with the GNU unwinders? I seem to > remember experimenting with .cfi_return_column in the past and hitting > problems when trying to use a fake register. > > At the time I just assumed I was doing something wrong and didn't go > digging. Maybe I _was_ doing something wrong… Not yet, I hope there is a CI that would do it for me :) Cheers, Daniel _______________________________________________ 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] 6+ messages in thread
end of thread, other threads:[~2020-07-10 17:09 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20200623085436.3696-1-will@kernel.org>
[not found] ` <20200623095417.GV25945@arm.com>
[not found] ` <CAMj1kXFhCC4coSQ+Q-d3rkXFtAes6mcqMrEZr-rbO1uj+OmLCA@mail.gmail.com>
[not found] ` <20200623105827.GA3954@willie-the-truck>
[not found] ` <CAMj1kXE9_HYpD=kzpPLOir0rGGf7jWTs+S3es_0Jo3ufPuA1eQ@mail.gmail.com>
[not found] ` <07001CCE-6AF8-4491-9B3A-4FC9C1E27FDB@arm.com>
2020-06-23 15:34 ` [PATCH 0/6] Fix unwinding through sigreturn trampolines Daniel Kiss
2020-06-23 15:43 ` Ard Biesheuvel
[not found] ` <51A5A3CB-B8D1-4BAD-BC7B-0337754AEB9D@arm.com>
2020-07-06 9:29 ` Daniel Kiss
2020-07-06 9:41 ` Ard Biesheuvel
2020-07-08 16:57 ` Dave Martin
2020-07-10 17:08 ` Daniel Kiss
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).