Hi lianbo

attached pacth v3.

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

/* arm64 kernel lr maybe has patuh */
void crash_decode_ptrauth_pc(ulong *pc);
void crash_decode_ptrauth_pc(ulong *pc)
{
#ifdef ARM64
   struct machine_specific *ms = machdep->machspec;
   if (is_kernel_text(*pc | ms->CONFIG_ARM64_KERNELPACMASK))
       *pc |= ms->CONFIG_ARM64_KERNELPACMASK;
#endif /* !ARM64 */
}

Guanyou Chen <chenguanyou9...@gmail.com> 于2025年1月15日周三 17:55写道:

> Hi  lianbo
>
> attach patch v2.
>
> lijiang <liji...@redhat.com> 于2025年1月15日周三 16:42写道:
>
>> Thank you for the patch, Guanyou.
>>
>> On Tue, Dec 31, 2024 at 7:22 PM <
>> devel-requ...@lists.crash-utility.osci.io> wrote:
>>
>>> Date: Thu, 26 Dec 2024 00:08:50 +0800
>>> From: Guanyou Chen <chenguanyou9...@gmail.com>
>>> Subject: [Crash-utility] [PATCH] arm64: add pac mask to better support
>>>         gdb stack unwind
>>> To: Lianbo <liji...@redhat.com>, Tao Liu <l...@redhat.com>,
>>>         devel@lists.crash-utility.osci.io
>>> Message-ID:
>>>         <
>>> cahs3rmxjg6oxb_zmgnr60kripoxo9tnb_63k+rjjhfk1t7a...@mail.gmail.com>
>>> 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 <chenguan...@xiaomi.com>
>>> ---
>>>  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
>>>
>>

Attachment: 0001-arm64-add-pac-mask-to-better-support-gdb-stack-unwin.patch.v3
Description: Binary data

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

Reply via email to