[Crash-utility] Re: [PATCH] arm64: add pac mask to better support gdb stack unwind

[lijiang <lijiang@xxxxxxxxxx>]

Thank you for the patch, Guanyou.

On Tue, Dec 31, 2024 at 7:22 PM <devel-request@xxxxxxxxxxxxxxxxxxxxxxxxxxx> wrote:

Date: Thu, 26 Dec 2024 00:08:50 +0800
From: Guanyou Chen <chenguanyou9338@xxxxxxxxx>
Subject: [PATCH] arm64: add pac mask to better support
        gdb stack unwind
To: Lianbo <lijiang@xxxxxxxxxx>, Tao Liu <ltao@xxxxxxxxxx>,
        devel@xxxxxxxxxxxxxxxxxxxxxxxxxxx
Message-ID:
        <CAHS3RMXJG6OxB_zmgnr60KriPOxo9tnb_63K+rjJHfk1t7aT0A@xxxxxxxxxxxxxx>
Content-Type: multipart/mixed; boundary="0000000000009f35d6062a1a727c"

--0000000000009f35d6062a1a727c
Content-Type: multipart/alternative; boundary="0000000000009f35d5062a1a727a"

--0000000000009f35d5062a1a727a
Content-Type: text/plain; charset="UTF-8"

Hi  Lianbo & Tao

Currently, gdb passthroughs of 'bt', 'frame', 'up', 'down',
'info, locals' don't work on arm64 machine enabled pauth.
This is due to gdb not knowing the lr register real values
to unwind the stack frames.

                                      ----------------------------
           gdb passthrough (eg. "bt") |                          |
   crash   -------------------------> |                          |
                                      |      gdb_interface       |
                                      |                          |
                                      |                          |
                                      |  ----------------------  |
               get_kernel_pac_mask    |  |                    |  |
crash_target<-------------------------+--|        gdb         |  |
            --------------------------+->|                    |  |
     arm64: CONFIG_ARM64_KERNELPACMASK|  |                    |  |
     other: ~0UL                      |  |                    |  |
                                      |  ----------------------  |
                                      ----------------------------

With the patch:
    crash> gdb bt
    #0  __switch_to (prev=prev@entry=0xffffff8001af92c0,
next=next@entry=0xffffff889da7a580)
at /proc/self/cwd/common/arch/arm64/kernel/process.c:569
    #1  0xffffffd3602132c0 in context_switch (rq=0xffffff8a7295a080,
prev=0xffffff8001af92c0, next=0xffffff889da7a580, rf=<optimized out>) at
/proc/self/cwd/common/kernel/sched/core.c:5515
    #2  __schedule (sched_mode=<optimized out>, sched_mode@entry=2147859424)
at /proc/self/cwd/common/kernel/sched/core.c:6843
    #3  0xffffffd3602136d8 in schedule () at
/proc/self/cwd/common/kernel/sched/core.c:6917
    ...

Without the patch:
    crash> gdb bt
    #0  __switch_to (prev=0xffffff8001af92c0, next=0xffffff889da7a580) at
/proc/self/cwd/common/arch/arm64/kernel/process.c:569
    #1  0x9fc5c5d3602132c0 in ?? ()
    Backtrace stopped: previous frame identical to this frame (corrupt
stack?)

Signed-off-by: Guanyou.Chen <chenguanyou@xxxxxxxxxx>
---
 gdb-10.2.patch  | 24 ++++++++++++++++++++++++
 gdb_interface.c | 11 +++++++++++
 2 files changed, 35 insertions(+)

diff --git a/gdb-10.2.patch b/gdb-10.2.patch
index c867660..4c13a6b 100644
--- a/gdb-10.2.patch
+++ b/gdb-10.2.patch
@@ -16216,3 +16216,27 @@ exit 0
        printf_filtered (_("Backtrace stopped: %s\n"),
                 frame_stop_reason_string (trailing));
    }
+--- gdb-10.2/gdb/frame.c.orig
++++ gdb-10.2/gdb/frame.c
+@@ -944,6 +944,10 @@ frame_find_by_id (struct frame_id id)
+   return NULL;
+ }
+
++#ifdef CRASH_MERGE
++extern "C" unsigned long crash_get_kernel_pac_mask(void);
++#endif
++
+ static CORE_ADDR
+ frame_unwind_pc (frame_info_ptr this_frame)
+ {
+@@ -974,6 +978,10 @@ frame_unwind_pc (struct frame_info_ptr *this_frame)
+       try
+   {
+     pc = gdbarch_unwind_pc (prev_gdbarch, this_frame);
++#ifdef CRASH_MERGE
++    CORE_ADDR mask = crash_get_kernel_pac_mask();
++    pc |= mask;

 

Can you help double check if a sanity check needs to be added? I saw the value 'pc|mask' is always checked in crash code as below: 

...

                LR = regs->regs[30];
                if (is_kernel_text (LR | ms->CONFIG_ARM64_KERNELPACMASK))
                        LR |= ms->CONFIG_ARM64_KERNELPACMASK;

...

if yes, the value pc can be passed as an argument in crash_get_kernel_pac_mask(), and then deal with this one.

 

++#endif
+     pc_p = true;
+   }
+       catch (const gdb_exception_error &ex)
diff --git a/gdb_interface.c b/gdb_interface.c
index 315711e..765dafe 100644
--- a/gdb_interface.c
+++ b/gdb_interface.c
@@ -1083,3 +1083,14 @@ int crash_get_current_task_reg (int regno, const
char *regname,
    return machdep->get_current_task_reg(regno, regname, regsize, value);
 }

+/* arm64 kernel lr pac mask */
+unsigned long crash_get_kernel_pac_mask(void);
+unsigned long crash_get_kernel_pac_mask(void)
+{
+#ifdef ARM64
+   struct machine_specific *ms = machdep->machspec;
+   return ms->CONFIG_ARM64_KERNELPACMASK;
+#else
+   return ~0UL;

 

The "~0UL" is 0xFFFFFFFFFFFFFFFF, is this expected? Or do you want it to overflow?

++    pc |= mask;

It probably has the same result, but the "return 0UL" should be more readable. Please see kernel code:

        vmcoreinfo_append_str("NUMBER(KERNELPACMASK)=0x%llx\n",
                                                system_supports_address_auth() ?
                                                ptrauth_kernel_pac_mask() : 0);

BTW: I cannot apply your patch with git command, and ran into some issues, probably it is a coding issue.

Thanks

Lianbo

 

+#endif /* !ARM64 */
+}
--
2.34.1

Thanks,
Guanyou

--
Crash-utility mailing list -- devel@xxxxxxxxxxxxxxxxxxxxxxxxxxx
To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxxxxxxxxxxxx
https://${domain_name}/admin/lists/devel.lists.crash-utility.osci.io/
Contribution Guidelines: https://github.com/crash-utility/crash/wiki