Re: [PATCH 0/8] unwind, arm64: add sframe unwinder for kernel

[Puranjay Mohan <puranjay@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 8151 bytes --]

Puranjay Mohan <puranjay@kernel.org> writes:

> Weinan Liu <wnliu@google.com> writes:
>
>> This patchset implements a generic kernel sframe-based [1] unwinder.
>> The main goal is to support reliable stacktraces on arm64.
>>
>> On x86 orc unwinder provides reliable stacktraces. But arm64 misses the
>> required support from objtool: it cannot generate orc unwind tables for
>> arm64.
>>
>> Currently, there's already a sframe unwinder proposed for userspace: [2].
>> Since the sframe unwind table algorithm is similar, these two proposal
>> could integrate common functionality in the future.
>>
>> There are some incomplete features or challenges:
>>   - The unwinder doesn't yet work with kernel modules. The `start_addr` of
>>     FRE from kernel modules doesn't appear correct, preventing us from
>>     unwinding functions from kernel modules.
>>   - Currently, only GCC supports sframe.
>>
>> Ref:
>> [1]: https://sourceware.org/binutils/docs/sframe-spec.html
>> [2]: https://lore.kernel.org/lkml/cover.1730150953.git.jpoimboe@kernel.org/
>>
>
> Hi Weinan,
> Thanks for working on this.
>
> I tested this set on my setup and faced some issues, here are the
> details:
>
> Here is my setup [on AWS c6gd.16xlarge instance]:
> -------------------------------------------------
>
> [root@ip-172-31-32-86 linux-upstream]# uname -a
> Linux ip-172-31-32-86.ec2.internal 6.14.0-rc1+ #1 SMP Tue Feb  4 14:15:55 UTC 2025 aarch64 aarch64 aarch64 GNU/Linux
>
> [root@ip-172-31-32-86 linux-upstream]# git log --oneline
> e9a702365 (HEAD -> master) arm64: Enable livepatch for ARM64
> 5dedc956e arm64: Define TIF_PATCH_PENDING for livepatch
> ba563b31a unwind: arm64: add reliable stacktrace support for arm64
> d807d392d unwind: arm64: Add sframe unwinder on arm64
> 7872f050b unwind: Implement generic sframe unwinder library
> 03d2ad003 unwind: add sframe v2 header
> 5e95cc051 arm64: entry: add unwind info for various kernel entries
> faff6cbc3 unwind: build kernel with sframe info
> 0de63bb7d (origin/master, origin/HEAD) Merge tag 'pull-fix' of git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs
> 902e09c8a fix braino in "9p: fix ->rename_sem exclusion"
> f286757b6 Merge tag 'timers-urgent-2025-02-03' of git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip
> a360f3ffd (grafted) Merge tag 'irq-urgent-2025-02-03' of git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip
> bb2784d9a (grafted) jiffies: Cast to unsigned long in secs_to_jiffies() conversion
> 30d61efe1 (grafted) 9p: fix ->rename_sem exclusion
>
> [root@ip-172-31-32-86 linux-upstream]# grep SFRAME .config
> CONFIG_AS_HAS_SFRAME_SUPPORT=y
> CONFIG_SFRAME_UNWIND_TABLE=y
> CONFIG_SFRAME_UNWINDER=y
> [root@ip-172-31-32-86 linux-upstream]# grep LIVEPATCH .config
> CONFIG_HAVE_LIVEPATCH=y
> CONFIG_LIVEPATCH=y
> CONFIG_SAMPLE_LIVEPATCH=m
>
> [root@ip-172-31-32-86 linux-upstream]# as --version
> GNU assembler version 2.41-50.al2023.0.2
> Copyright (C) 2023 Free Software Foundation, Inc.
> This program is free software; you may redistribute it under the terms of
> the GNU General Public License version 3 or later.
> This program has absolutely no warranty.
> This assembler was configured for a target of `aarch64-amazon-linux'.
>
> [root@ip-172-31-32-86 linux-upstream]# gcc --version
> gcc (GCC) 11.4.1 20230605 (Red Hat 11.4.1-2)
> Copyright (C) 2021 Free Software Foundation, Inc.
> This is free software; see the source for copying conditions.  There is NO
> warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
>
> Loading the livepatch-sameple module:
> -------------------------------------
>
> [root@ip-172-31-32-86 linux-upstream]# kpatch load /lib/modules/6.14.0-rc1+/kernel/samples/livepatch/livepatch-sample.ko
> loading patch module: /lib/modules/6.14.0-rc1+/kernel/samples/livepatch/livepatch-sample.ko
> waiting (up to 15 seconds) for patch transition to complete...
> patch transition has stalled!
> <4>kpatch: Livepatch process signaling is performed automatically on your system.
> <4>kpatch: Skipping manual process signaling.
> waiting (up to 60 seconds) for patch transition to complete...
>
> Stalled processes:
> 340 kdevtmpfs
> stack:
> [<0>] devtmpfs_work_loop+0x2cc/0x2d8
> [<0>] devtmpfsd+0x4c/0x58
> [<0>] kthread+0xf0/0x100
> [<0>] ret_from_fork+0x10/0x20
> module livepatch_sample did not complete its transition, unloading...
> disabling patch module: livepatch_sample
> waiting (up to 15 seconds) for patch transition to complete...
> transition complete (3 seconds)
> unloading patch module: livepatch_sample
> <4>kpatch: error: failed to load module livepatch_sample (transition stalled)

After some debugging this is what I found:

devtmpfsd() calls devtmpfs_work_loop() which is marked '__noreturn' and has an
infinite loop. The compiler puts the `bl` to devtmpfs_work_loop() as the the
last instruction in devtmpfsd() and therefore on entry to devtmpfs_work_loop(),
LR points to an instruction beyond devtmpfsd() and this consfuses the unwinder.

ffff800080d9a070 <devtmpfsd>:
ffff800080d9a070:       d503201f        nop
ffff800080d9a074:       d503201f        nop
ffff800080d9a078:       d503233f        paciasp
ffff800080d9a07c:       a9be7bfd        stp     x29, x30, [sp, #-32]!
ffff800080d9a080:       910003fd        mov     x29, sp
ffff800080d9a084:       f9000bf3        str     x19, [sp, #16]
ffff800080d9a088:       943378e8        bl      ffff800081a78428 <devtmpfs_setup>
ffff800080d9a08c:       90006ca1        adrp    x1, ffff800081b2e000 <unique_processor_ids+0x3758>
ffff800080d9a090:       2a0003f3        mov     w19, w0
ffff800080d9a094:       912de021        add     x1, x1, #0xb78
ffff800080d9a098:       91002020        add     x0, x1, #0x8
ffff800080d9a09c:       97cd2a43        bl      ffff8000800e49a8 <complete>
ffff800080d9a0a0:       340000d3        cbz     w19, ffff800080d9a0b8 <devtmpfsd+0x48>
ffff800080d9a0a4:       2a1303e0        mov     w0, w19
ffff800080d9a0a8:       f9400bf3        ldr     x19, [sp, #16]
ffff800080d9a0ac:       a8c27bfd        ldp     x29, x30, [sp], #32
ffff800080d9a0b0:       d50323bf        autiasp
ffff800080d9a0b4:       d65f03c0        ret
ffff800080d9a0b8:       97f06526        bl      ffff8000809b3550 <devtmpfs_work_loop>
ffff800080d9a0bc:       00000000        udf     #0
ffff800080d9a0c0:       d503201f        nop
ffff800080d9a0c4:       d503201f        nop

find_fde() got pc=0xffff800080d9a0bc which is not in [sfde_func_start_address, sfde_func_size)

output for readelf --sframe for devtmpfsd()

func idx [51825]: pc = 0xffff800080d9a070, size = 76 bytes
    STARTPC           CFA       FP        RA
    ffff800080d9a070  sp+0      u         u
    ffff800080d9a07c  sp+0      u         u[s]
    ffff800080d9a080  sp+32     c-32      c-24[s]
    ffff800080d9a0b0  sp+0      u         u[s]
    ffff800080d9a0b4  sp+0      u         u
    ffff800080d9a0b8  sp+32     c-32      c-24[s]

The unwinder and all the related infra is assuming that the return address
will be part of a valid function which is not the case here.

I am not sure which component needs to be fixed here, but the following
patch(which is a hack) fixes the issue by considering the return address as
part of the function descriptor entry.

-- 8< --

diff --git a/kernel/sframe_lookup.c b/kernel/sframe_lookup.c
index 846f1da95..28bec5064 100644
--- a/kernel/sframe_lookup.c
+++ b/kernel/sframe_lookup.c
@@ -82,7 +82,7 @@ static struct sframe_fde *find_fde(const struct sframe_table *tbl, unsigned long
        if (f >= tbl->sfhdr_p->num_fdes || f < 0)
                return NULL;
        fdep = tbl->fde_p + f;
-       if (ip < fdep->start_addr || ip >= fdep->start_addr + fdep->size)
+       if (ip < fdep->start_addr || ip > fdep->start_addr + fdep->size)
                return NULL;

        return fdep;
@@ -106,7 +106,7 @@ static int find_fre(const struct sframe_table *tbl, unsigned long pc,
        else
                ip_off = (int32_t)(pc - (unsigned long)tbl->sfhdr_p) - fdep->start_addr;

-       if (ip_off < 0 || ip_off >= fdep->size)
+       if (ip_off < 0 || ip_off > fdep->size)
                return -EINVAL;

        /*

-- >8 --

Thanks,
Puranjay

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 255 bytes --]