lib/55719: Unwind tables for signal trampoline on amd64 are incorrect

To: lib-bug-people%netbsd.org@localhost,gnats-admin%netbsd.org@localhost,netbsd-bugs%netbsd.org@localhost
Subject: lib/55719: Unwind tables for signal trampoline on amd64 are incorrect
From: nikhil.benesch%gmail.com@localhost
Date: Sun, 11 Oct 2020 23:30:00 +0000 (UTC)
>Number:         55719
>Category:       lib
>Synopsis:       Unwind tables for signal trampoline on amd64 are incorrect
>Confidential:   no
>Severity:       non-critical
>Priority:       medium
>Responsible:    lib-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Sun Oct 11 23:30:00 +0000 2020
>Originator:     Nikhil Benesch
>Release:        current
>Organization:
>Environment:
NetBSD  9.99.73 NetBSD 9.99.73 (GENERIC) #0: Sat Oct 10 07:28:04 UTC 2020  mkrepro%mkrepro.NetBSD.org@localhost:/usr/src/sys/arch/amd64/compile/GENERIC amd64
>Description:
The current unwind tables for the amd64 version of __sigtramp_siginfo_2 look incorrect to me.

Per my reading of the DWARF specification and the GCC source code, the unwind tables for a signal trampoline need to specify the CFA as the value of RSP *in the saved context*, rather than the standard location of RSP + 8. They also need to specify where to find the values of each register in the saved context.

The CFI directives added to the amd64 in http://cvsweb.netbsd.org/bsdweb.cgi/src/lib/libc/arch/x86_64/sys/__sigtramp2.S?rev=1.7&content-type=text/x-cvsweb-markup&only_with_tag=MAIN instead seem to specify a standard function frame, rather than accounting for any of the special behavior of the signal trampoline. I believe the reason that GDB is able to backtrace through signal handlers on NetBSD is due to custom code in GDB (see "nbsd_pc_in_sigtramp" here: https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=gdb/nbsd-tdep.c;h=2db994af21811f82183cb9d2f342a8f3806deb51;hb=HEAD), not due to the unwind tables as presently generated.

For context, I am in the process of porting gccgo to NetBSD. The Go language has the unusual property that it promises that segfaults are recoverable panics (which in gccgo work using the same infrastructure as C++ exceptions). This requires that the unwinder be able to unwind through signal handlers. The WIP port can be seen here: https://go-review.googlesource.com/c/gofrontend/+/261137 . The resulting compiler generally works, but fails several test cases in the test suite that test whether segfaults are properly caught.

The patch I've included below fixes these tests in the test suite. It should also make unwinding "just work" when using a DWARF-based unwinder that are not specially-aware of NetBSD's signal trampolines.

In theory, an alternative would be to hardcode a NetBSD sigtramp sniffer in libgcc, like exists for FreeBSD (https://gcc.gnu.org/git/?p=gcc.git;a=blob;f=libgcc/config/i386/freebsd-unwind.h;h=0ea4e522712a0fb9ad0eb4681bc1cff9697c6505;hb=HEAD), but this approach seems a bit like madness to me. (Generating unwind tables by hand is also a bit crazy, but at least it only needs to be done *once*, rather than for every unwinder.)
>How-To-Repeat:
Attempt to unwind through a signal handler on NetBSD using an unwinder that is not NetBSD-aware (like the unwinder that ships in libgcc).
>Fix:
The following patch to libc fixes the issue on amd64 for me:

RCS file: /cvsroot/src/lib/libc/arch/x86_64/sys/__sigtramp2.S,v
retrieving revision 1.7
diff -u -r1.7 __sigtramp2.S
--- arch/x86_64/sys/__sigtramp2.S       2 Dec 2019 01:38:54 -0000       1.7
+++ arch/x86_64/sys/__sigtramp2.S       11 Oct 2020 23:10:32 -0000
@@ -36,19 +36,103 @@
  */
 
 #include "SYS.h"
+#include "assym.h"
 
 /*
  * The x86-64 signal trampoline is invoked only to return from
  * the signal; the kernel calls the signal handler directly.
  */
+
+/*
+ * Unwinders find the FDE for the caller by looking up (return PC - 1),
+ * assuming that the return PC points to the instruction after a call
+ * instruction and so (return PC - 1) points into the middle of the call
+ * instruction. Since the trampoline doesn't actually call the signal
+ * handler, we need to manually pad its FDE with a nop to account for
+ * the subtraction.
+ */
+.Ltramp_start:
+       nop
 NENTRY(__sigtramp_siginfo_2)
-       .cfi_startproc
-       .cfi_def_cfa rsp, 8
        movq    %r15,%rdi
        movq    $SYS_setcontext, %rax
        syscall
        movq    $-1,%rdi /* if we return here, something is wrong */
        movq    $SYS_exit, %rax
        syscall
-       .cfi_endproc
+.Ltramp_end:
 END(__sigtramp_siginfo_2)
+
+/*
+ * Manually build an unwind table for the trampoline.
+ * The unwind table consists of a CIE and an FDE within that CIE.
+ * The .cfi_* directives don't work here because they don't support the
+ * complex expressions required to restore registers relative to the
+ * address in R15 rather than relative to the CFA.
+ */
+
+/* CIE */
+       .section .eh_frame, "a", @progbits
+.Lcie:
+       .int .Lcie_end - .Lcie_start /* CIE length */
+.Lcie_start:
+       .int      0x00  /* CIE ID */
+       .byte     0x01  /* CIE version */
+       .string   "zRS" /* Augmentation string */
+       .uleb128  0x01  /* Code alignment factor */
+       .sleb128  0x01  /* Data alignment factor */
+       .byte     0x10  /* Return address column */
+       .uleb128  0x01  /* Augmentation length */
+       .byte     0x1b  /* Augmentation data: FDE encoding (DW_EH_PE_pcrel | DW_EH_PE_sdata4) */
+       .align    0x08
+.Lcie_end:
+
+/* FDE */
+       .int .Lfde_end - .Lfde_start            /* FDE length */
+.Lfde_start:
+       .int .Lfde_start - .Lcie                /* Relative CIE offset */
+       .int .Ltramp_start - .                  /* PC-relative start address */
+       .int .Ltramp_end - .Ltramp_start        /* PC-relative end address */
+       .uleb128  0                             /* Augmentation length */
+
+       /* Frame table instructions. */
+
+       /* Set the CFA to the value of RSP in the saved context. */
+       .byte     0x0f                          /* DW_CFA_def_cfa_expression */
+       .uleb128  1f - 0f                       /* Length of expression */
+0:     .byte     0x7f                          /* DW_OP_breg15 */
+       .sleb128  _OFFSETOF_UC_GREGS_RSP        /* Offset of RSP */
+       .byte     0x06                          /* DW_OP_deref */
+1:
+
+       /* Restore each register from its value in the saved context. */
+
+#define RESTORE_REG(regno, reg) \
+       .byte     0x10;                         /* DW_CFA_expression */         \
+       .uleb128  regno;                        /* Register number */           \
+       .uleb128  1f - 0f;                      /* Length of expression */      \
+0:     .byte     0x7f;                         /* DW_OP_breg15 */              \
+       .sleb128  _OFFSETOF_UC_GREGS_##reg;     /* Offset of <reg> */           \
+1:
+
+       RESTORE_REG(0, RAX)
+       RESTORE_REG(1, RDX)
+       RESTORE_REG(2, RCX)
+       RESTORE_REG(3, RBX)
+       RESTORE_REG(4, RSI)
+       RESTORE_REG(5, RDI)
+       RESTORE_REG(6, RBP)
+       /* The unwinder will use the CFA to restore RSP. */
+       RESTORE_REG(8, R8)
+       RESTORE_REG(9, R9)
+       RESTORE_REG(10, R10)
+       RESTORE_REG(11, R11)
+       RESTORE_REG(12, R12)
+       RESTORE_REG(13, R13)
+       RESTORE_REG(14, R14)
+       RESTORE_REG(15, R15)
+       RESTORE_REG(16, RIP) 
+
+       .align    0x08
+.Lfde_end:
+
Index: arch/x86_64/genassym.cf
===================================================================
RCS file: arch/x86_64/genassym.cf
diff -N arch/x86_64/genassym.cf
--- /dev/null   1 Jan 1970 00:00:00 -0000
+++ arch/x86_64/genassym.cf     11 Oct 2020 23:10:32 -0000
@@ -0,0 +1,19 @@
+include <ucontext.h>
+
+define _OFFSETOF_UC_GREGS_RAX offsetof(ucontext_t, uc_mcontext.__gregs[_REG_RAX])
+define _OFFSETOF_UC_GREGS_RDX offsetof(ucontext_t, uc_mcontext.__gregs[_REG_RDX])
+define _OFFSETOF_UC_GREGS_RCX offsetof(ucontext_t, uc_mcontext.__gregs[_REG_RCX])
+define _OFFSETOF_UC_GREGS_RBX offsetof(ucontext_t, uc_mcontext.__gregs[_REG_RBX])
+define _OFFSETOF_UC_GREGS_RSI offsetof(ucontext_t, uc_mcontext.__gregs[_REG_RSI])
+define _OFFSETOF_UC_GREGS_RDI offsetof(ucontext_t, uc_mcontext.__gregs[_REG_RDI])
+define _OFFSETOF_UC_GREGS_RBP offsetof(ucontext_t, uc_mcontext.__gregs[_REG_RBP])
+define _OFFSETOF_UC_GREGS_RSP offsetof(ucontext_t, uc_mcontext.__gregs[_REG_RSP])
+define _OFFSETOF_UC_GREGS_R8  offsetof(ucontext_t, uc_mcontext.__gregs[_REG_R8])
+define _OFFSETOF_UC_GREGS_R9  offsetof(ucontext_t, uc_mcontext.__gregs[_REG_R9])
+define _OFFSETOF_UC_GREGS_R10 offsetof(ucontext_t, uc_mcontext.__gregs[_REG_R10])
+define _OFFSETOF_UC_GREGS_R11 offsetof(ucontext_t, uc_mcontext.__gregs[_REG_R11])
+define _OFFSETOF_UC_GREGS_R12 offsetof(ucontext_t, uc_mcontext.__gregs[_REG_R12])
+define _OFFSETOF_UC_GREGS_R13 offsetof(ucontext_t, uc_mcontext.__gregs[_REG_R13])
+define _OFFSETOF_UC_GREGS_R14 offsetof(ucontext_t, uc_mcontext.__gregs[_REG_R14])
+define _OFFSETOF_UC_GREGS_R15 offsetof(ucontext_t, uc_mcontext.__gregs[_REG_R15])
+define _OFFSETOF_UC_GREGS_RIP offsetof(ucontext_t, uc_mcontext.__gregs[_REG_RIP])
Prev by Date: Re: kern/55718: error reading fsbn when trying to mount file system from SD card
Next by Date: Re: lib/55719
Previous by Thread: Re: lib/55719 (Unwind tables for signal trampoline on amd64 are incorrect)
Next by Thread: Re: lib/55719
Indexes:
Home | Main Index | Thread Index | Old Index