| Message ID | 20250127213310.2496133-1-wnliu@google.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | unwind, arm64: add sframe unwinder for kernel | expand |
On 1/27/25 1:33 PM, Weinan Liu wrote: > 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. I did file https://sourceware.org/bugzilla/show_bug.cgi?id=32589 earlier. It is misleading (and inconvenient) to see all 0s in the dump of non-relocated SFrame section. I get the sense that while issue 32589 may be causing confusion that the SFrame data is not correct, the problem is elsewhere ? I can share a fix for 32589 so atleast we can verify that the starting point is sane. > - 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/ > > Madhavan T. Venkataraman (1): > arm64: Define TIF_PATCH_PENDING for livepatch > > Weinan Liu (7): > unwind: build kernel with sframe info > arm64: entry: add unwind info for various kernel entries > unwind: add sframe v2 header > unwind: Implement generic sframe unwinder library > unwind: arm64: Add sframe unwinder on arm64 > unwind: arm64: add reliable stacktrace support for arm64 > arm64: Enable livepatch for ARM64 > > Makefile | 6 + > arch/Kconfig | 8 + > arch/arm64/Kconfig | 3 + > arch/arm64/Kconfig.debug | 10 + > arch/arm64/include/asm/stacktrace/common.h | 6 + > arch/arm64/include/asm/thread_info.h | 4 +- > arch/arm64/kernel/entry-common.c | 4 + > arch/arm64/kernel/entry.S | 10 + > arch/arm64/kernel/setup.c | 2 + > arch/arm64/kernel/stacktrace.c | 102 ++++++++++ > include/asm-generic/vmlinux.lds.h | 12 ++ > include/linux/sframe_lookup.h | 43 +++++ > kernel/Makefile | 1 + > kernel/sframe.h | 215 +++++++++++++++++++++ > kernel/sframe_lookup.c | 196 +++++++++++++++++++ > 15 files changed, 621 insertions(+), 1 deletion(-) > create mode 100644 include/linux/sframe_lookup.h > create mode 100644 kernel/sframe.h > create mode 100644 kernel/sframe_lookup.c >
Not only does objdump show that func_start_addr all 0s, but the func_start_addr in the kernel module's sframe table is also incorrect. I'll prepare a repro so we can investigate this. > > I can share a fix for 32589 so atleast we can verify that the starting > point is sane. > Great! Thanks. This will be helpful for comparing the values from the sframe table with the values we obtain at runtime
I missed this set before sending my RFC set. If this set works well, we won't need the other set. I will give this one a try. Thanks, Song On Mon, Jan 27, 2025 at 1:33 PM Weinan Liu <wnliu@google.com> wrote: > > 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/ > > Madhavan T. Venkataraman (1): > arm64: Define TIF_PATCH_PENDING for livepatch > > Weinan Liu (7): > unwind: build kernel with sframe info > arm64: entry: add unwind info for various kernel entries > unwind: add sframe v2 header > unwind: Implement generic sframe unwinder library > unwind: arm64: Add sframe unwinder on arm64 > unwind: arm64: add reliable stacktrace support for arm64 > arm64: Enable livepatch for ARM64 > > Makefile | 6 + > arch/Kconfig | 8 + > arch/arm64/Kconfig | 3 + > arch/arm64/Kconfig.debug | 10 + > arch/arm64/include/asm/stacktrace/common.h | 6 + > arch/arm64/include/asm/thread_info.h | 4 +- > arch/arm64/kernel/entry-common.c | 4 + > arch/arm64/kernel/entry.S | 10 + > arch/arm64/kernel/setup.c | 2 + > arch/arm64/kernel/stacktrace.c | 102 ++++++++++ > include/asm-generic/vmlinux.lds.h | 12 ++ > include/linux/sframe_lookup.h | 43 +++++ > kernel/Makefile | 1 + > kernel/sframe.h | 215 +++++++++++++++++++++ > kernel/sframe_lookup.c | 196 +++++++++++++++++++ > 15 files changed, 621 insertions(+), 1 deletion(-) > create mode 100644 include/linux/sframe_lookup.h > create mode 100644 kernel/sframe.h > create mode 100644 kernel/sframe_lookup.c > > -- > 2.48.1.262.g85cc9f2d1e-goog > >
On Thu, Jan 30, 2025 at 9:59 AM Song Liu <song@kernel.org> wrote: > > I missed this set before sending my RFC set. If this set works well, we > won't need the other set. I will give this one a try. I just realized that llvm doesn't support sframe yet. So we (Meta) still need some sframe-less approach before llvm supports sframe. IIRC, Google also uses llvm to compile the kernel. Weinan, would you mind share your thoughts on how we can adopt this before llvm supports sframe? (compile arm64 kernel with gcc?) Thanks, Song
On Thu, Jan 30, 2025 at 10:34:09AM -0800, Song Liu wrote: > On Thu, Jan 30, 2025 at 9:59 AM Song Liu <song@kernel.org> wrote: > > > > I missed this set before sending my RFC set. If this set works well, we > > won't need the other set. I will give this one a try. > > I just realized that llvm doesn't support sframe yet. So we (Meta) still > need some sframe-less approach before llvm supports sframe. > > IIRC, Google also uses llvm to compile the kernel. Weinan, would > you mind share your thoughts on how we can adopt this before > llvm supports sframe? (compile arm64 kernel with gcc?) Hi Song, the plan is to start the work on adding sframe support to llvm in parallel to landing these changes upstream. From the initial assessment it shouldn't be too hard. Thanks
Hi Roman, On Thu, Jan 30, 2025 at 11:01 AM Roman Gushchin <roman.gushchin@linux.dev> wrote: > > On Thu, Jan 30, 2025 at 10:34:09AM -0800, Song Liu wrote: > > On Thu, Jan 30, 2025 at 9:59 AM Song Liu <song@kernel.org> wrote: > > > > > > I missed this set before sending my RFC set. If this set works well, we > > > won't need the other set. I will give this one a try. > > > > I just realized that llvm doesn't support sframe yet. So we (Meta) still > > need some sframe-less approach before llvm supports sframe. > > > > IIRC, Google also uses llvm to compile the kernel. Weinan, would > > you mind share your thoughts on how we can adopt this before > > llvm supports sframe? (compile arm64 kernel with gcc?) > > Hi Song, > > the plan is to start the work on adding sframe support to llvm > in parallel to landing these changes upstream. From the initial > assessment it shouldn't be too hard. Thanks for the information! Song
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) Useful messages from kernel log [pr_debug enabled]: --------------------------------------------------- livepatch: enabling patch 'livepatch_sample' livepatch: 'livepatch_sample': initializing patching transition livepatch: 'livepatch_sample': starting patching transition livepatch: klp_try_switch_task: kdevtmpfs:340 has an unreliable stack livepatch: klp_try_switch_task: insmod:9226 has an unreliable stack livepatch: klp_try_switch_task: swapper/63:0 is running [......SNIP.......] livepatch: klp_try_switch_task: kdevtmpfs:340 has an unreliable stack [......SNIP.......] livepatch: signaling remaining tasks livepatch: klp_try_switch_task: kdevtmpfs:340 has an unreliable stack livepatch: 'livepatch_sample': reversing transition from patching to unpatching livepatch: 'livepatch_sample': starting unpatching transition livepatch: klp_try_switch_task: swapper/45:0 is running livepatch: 'livepatch_sample': completing unpatching transition livepatch: 'livepatch_sample': unpatching complete Please let me know if you are aware of this already or if this is expected behaviour with this version. I will try to debug this from my side as well. Also let me know if you need more details for debugging this. P.S. - I also saw multiple build warning like: ld: warning: orphan section `.eh_frame' from `arch/arm64/kernel/entry.o' being placed in section `.eh_frame' ld: warning: orphan section `.init.sframe' from `arch/arm64/kernel/pi/lib-fdt.pi.o' being placed in section `.init.sframe' Thanks, Puranjay
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
> 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 Thank you for reporting this issue. I just found out that Josh also intentionally uses '>' instead of '>=' for the same reason https://lore.kernel.org/lkml/20250122225257.h64ftfnorofe7cb4@jpoimboe/T/#m6d70a20ed9f5b3bbe5b24b24b8c5dcc603a79101 QQ, do we need to care the stacktrace after '__noreturn' function?
Weinan Liu <wnliu@google.com> writes: >> 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 > > Thank you for reporting this issue. > I just found out that Josh also intentionally uses '>' instead of '>=' for the same reason > https://lore.kernel.org/lkml/20250122225257.h64ftfnorofe7cb4@jpoimboe/T/#m6d70a20ed9f5b3bbe5b24b24b8c5dcc603a79101 > > QQ, do we need to care the stacktrace after '__noreturn' function? Yes, I think we should, but others people could add more to this. I have been testing this series with Kpatch and created a PR that works with this unwinder: https://github.com/dynup/kpatch/pull/1439 For the modules, I think we need per module sframe tables that are initialised when the module is loaded. And the unwinder should use the module specific table if the IP is in a module's code. Have you already started working on it? if not I would like to help and work on that. Thanks, Puranjay
On Fri, Feb 07, 2025 at 12:16:29PM +0000, Puranjay Mohan wrote: > Weinan Liu <wnliu@google.com> writes: > > Thank you for reporting this issue. > > I just found out that Josh also intentionally uses '>' instead of '>=' for the same reason > > https://lore.kernel.org/lkml/20250122225257.h64ftfnorofe7cb4@jpoimboe/T/#m6d70a20ed9f5b3bbe5b24b24b8c5dcc603a79101 > > > > QQ, do we need to care the stacktrace after '__noreturn' function? > > Yes, I think we should, but others people could add more to this. FYI, here's how ORC handles this: /* * Find the orc_entry associated with the text address. * * For a call frame (as opposed to a signal frame), state->ip points to * the instruction after the call. That instruction's stack layout * could be different from the call instruction's layout, for example * if the call was to a noreturn function. So get the ORC data for the * call instruction itself. */ orc = orc_find(state->signal ? state->ip : state->ip - 1); and state->signal is only set for exceptions/interrupts (including preemption and page faults) and newly forked tasks which haven't run yet.
On Fri, Feb 7, 2025 at 4:16 AM Puranjay Mohan <puranjay@kernel.org> wrote: > > Yes, I think we should, but others people could add more to this. > > I have been testing this series with Kpatch and created a PR that works > with this unwinder: https://github.com/dynup/kpatch/pull/1439 > > For the modules, I think we need per module sframe tables that are > initialised when the module is loaded. And the unwinder should use the > module specific table if the IP is in a module's code. > > Have you already started working on it? if not I would like to help and > work on that. > > Thanks, > Puranjay Thanks for updating the arm64 kpatch PR so quickly. So we can use kpatch to test this proposal. I already have a WIP patch to add sframe support to the kernel module. However, it is not yet working. I had trouble unwinding frames for the kernel module using the current algorithm. Indu has likely identified the issue and will be addressing it from the toolchain side. https://sourceware.org/bugzilla/show_bug.cgi?id=32666
I run some tests with this set and my RFC set [1]. Most of the test is done with kpatch-build. I tested both Puranjay's version [3] and my version [4]. For gcc 14.2.1, I have seen the following issue with this test [2]. This happens with both upstream and 6.13.2. The livepatch loaded fine, but the system spilled out the following warning quickly. On the other hand, the same test works with LLVM and my RFC set (LLVM doesn't support SFRAME, and thus doesn't work with this set yet). Thanks, Song [ 81.250437] ------------[ cut here ]------------ [ 81.250818] refcount_t: saturated; leaking memory. [ 81.251201] WARNING: CPU: 0 PID: 95 at lib/refcount.c:22 refcount_warn_saturate+0x6c/0x140 [ 81.251841] Modules linked in: livepatch_special_static(OEK) [ 81.252277] CPU: 0 UID: 0 PID: 95 Comm: bash Tainted: G OE K 6.13.2-00321-g52d2813b4b07 #49 [ 81.253003] Tainted: [O]=OOT_MODULE, [E]=UNSIGNED_MODULE, [K]=LIVEPATCH [ 81.253503] Hardware name: linux,dummy-virt (DT) [ 81.253856] pstate: 634000c5 (nZCv daIF +PAN -UAO +TCO +DIT -SSBS BTYPE=--) [ 81.254383] pc : refcount_warn_saturate+0x6c/0x140 [ 81.254748] lr : refcount_warn_saturate+0x6c/0x140 [ 81.255114] sp : ffff800085a6fc00 [ 81.255371] x29: ffff800085a6fc00 x28: 0000000001200000 x27: ffff0000c2966180 [ 81.255918] x26: 0000000000000000 x25: ffff8000829c0000 x24: ffff0000c2e9b608 [ 81.256462] x23: ffff800083351000 x22: ffff0000c2e9af80 x21: ffff0000c062e140 [ 81.257006] x20: ffff0000c1c10c00 x19: ffff800085a6fd80 x18: ffffffffffffffff [ 81.257544] x17: 0000000000000001 x16: ffffffffffffffff x15: 0000000000000006 [ 81.258083] x14: 0000000000000000 x13: 2e79726f6d656d20 x12: 676e696b61656c20 [ 81.258625] x11: ffff8000829f7d70 x10: 0000000000000147 x9 : ffff8000801546b4 [ 81.259165] x8 : 00000000fffeffff x7 : 00000000ffff0000 x6 : ffff800082f77d70 [ 81.259709] x5 : 80000000ffff0000 x4 : 0000000000000000 x3 : 0000000000000001 [ 81.260257] x2 : ffff8000829f7a88 x1 : ffff8000829f7a88 x0 : 0000000000000026 [ 81.260824] Call trace: [ 81.261015] refcount_warn_saturate+0x6c/0x140 (P) [ 81.261387] __refcount_add.constprop.0+0x60/0x70 [ 81.261748] copy_process+0xfdc/0xfd58 [livepatch_special_static] [ 81.262217] kernel_clone+0x80/0x3e0 [ 81.262499] __do_sys_clone+0x5c/0x88 [ 81.262786] __arm64_sys_clone+0x24/0x38 [ 81.263085] invoke_syscall+0x4c/0x108 [ 81.263378] el0_svc_common.constprop.0+0x44/0xe8 [ 81.263734] do_el0_svc+0x20/0x30 [ 81.263993] el0_svc+0x34/0xf8 [ 81.264231] el0t_64_sync_handler+0x104/0x130 [ 81.264561] el0t_64_sync+0x184/0x188 [ 81.264846] ---[ end trace 0000000000000000 ]--- [ 82.335559] ------------[ cut here ]------------ [ 82.335931] refcount_t: underflow; use-after-free. [ 82.336307] WARNING: CPU: 1 PID: 0 at lib/refcount.c:28 refcount_warn_saturate+0xec/0x140 [ 82.336949] Modules linked in: livepatch_special_static(OEK) [ 82.337389] CPU: 1 UID: 0 PID: 0 Comm: swapper/1 Tainted: G W OE K 6.13.2-00321-g52d2813b4b07 #49 [ 82.338148] Tainted: [W]=WARN, [O]=OOT_MODULE, [E]=UNSIGNED_MODULE, [K]=LIVEPATCH [ 82.338721] Hardware name: linux,dummy-virt (DT) [ 82.339083] pstate: 63400005 (nZCv daif +PAN -UAO +TCO +DIT -SSBS BTYPE=--) [ 82.339617] pc : refcount_warn_saturate+0xec/0x140 [ 82.340007] lr : refcount_warn_saturate+0xec/0x140 [ 82.340378] sp : ffff80008370fe40 [ 82.340637] x29: ffff80008370fe40 x28: 0000000000000000 x27: 0000000000000000 [ 82.341188] x26: 000000000000000a x25: ffff0000fdaf7ab8 x24: 0000000000000014 [ 82.341737] x23: ffff8000829c8d30 x22: ffff80008370ff28 x21: ffff0000fe020000 [ 82.342286] x20: ffff0000c062e140 x19: ffff0000c2e9af80 x18: ffffffffffffffff [ 82.342839] x17: ffff80007b7a0000 x16: ffff800083700000 x15: 0000000000000006 [ 82.343389] x14: 0000000000000000 x13: 2e656572662d7265 x12: 7466612d65737520 [ 82.343944] x11: ffff8000829f7d70 x10: 000000000000016a x9 : ffff8000801546b4 [ 82.344499] x8 : 00000000fffeffff x7 : 00000000ffff0000 x6 : ffff800082f77d70 [ 82.345051] x5 : 80000000ffff0000 x4 : 0000000000000000 x3 : 0000000000000001 [ 82.345604] x2 : ffff8000829f7a88 x1 : ffff8000829f7a88 x0 : 0000000000000026 [ 82.346163] Call trace: [ 82.346359] refcount_warn_saturate+0xec/0x140 (P) [ 82.346736] __put_task_struct+0x130/0x170 [ 82.347063] delayed_put_task_struct+0xbc/0xe8 [ 82.347411] rcu_core+0x20c/0x5f8 [ 82.347680] rcu_core_si+0x14/0x28 [ 82.347952] handle_softirqs+0x124/0x308 [ 82.348260] __do_softirq+0x18/0x20 [ 82.348536] ____do_softirq+0x14/0x28 [ 82.348828] call_on_irq_stack+0x24/0x30 [ 82.349137] do_softirq_own_stack+0x20/0x38 [ 82.349465] __irq_exit_rcu+0xcc/0x108 [ 82.349764] irq_exit_rcu+0x14/0x28 [ 82.350038] el1_interrupt+0x34/0x50 [ 82.350321] el1h_64_irq_handler+0x14/0x20 [ 82.350642] el1h_64_irq+0x6c/0x70 [ 82.350911] default_idle_call+0x30/0xd0 (P) [ 82.351248] do_idle+0x1d0/0x200 [ 82.351506] cpu_startup_entry+0x38/0x48 [ 82.351818] secondary_start_kernel+0x124/0x150 [ 82.352176] __secondary_switched+0xac/0xb0 [ 82.352505] ---[ end trace 0000000000000000 ]--- [1] SFRAME-less livepatch RFC https://lore.kernel.org/live-patching/20250129232936.1795412-1-song@kernel.org/ [2] special-static test from kpatch https://github.com/dynup/kpatch/blob/master/test/integration/linux-6.2.0/special-static.patch [3] Puranjay's kpatch with arm64 support https://github.com/puranjaymohan/kpatch/tree/arm64 [4] My version of kpatch with arm64 and LTO support https://github.com/liu-song-6/kpatch/tree/fb-6.13-v2 On Mon, Jan 27, 2025 at 1:33 PM Weinan Liu <wnliu@google.com> wrote: > > 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/ > > Madhavan T. Venkataraman (1): > arm64: Define TIF_PATCH_PENDING for livepatch > > Weinan Liu (7): > unwind: build kernel with sframe info > arm64: entry: add unwind info for various kernel entries > unwind: add sframe v2 header > unwind: Implement generic sframe unwinder library > unwind: arm64: Add sframe unwinder on arm64 > unwind: arm64: add reliable stacktrace support for arm64 > arm64: Enable livepatch for ARM64 > > Makefile | 6 + > arch/Kconfig | 8 + > arch/arm64/Kconfig | 3 + > arch/arm64/Kconfig.debug | 10 + > arch/arm64/include/asm/stacktrace/common.h | 6 + > arch/arm64/include/asm/thread_info.h | 4 +- > arch/arm64/kernel/entry-common.c | 4 + > arch/arm64/kernel/entry.S | 10 + > arch/arm64/kernel/setup.c | 2 + > arch/arm64/kernel/stacktrace.c | 102 ++++++++++ > include/asm-generic/vmlinux.lds.h | 12 ++ > include/linux/sframe_lookup.h | 43 +++++ > kernel/Makefile | 1 + > kernel/sframe.h | 215 +++++++++++++++++++++ > kernel/sframe_lookup.c | 196 +++++++++++++++++++ > 15 files changed, 621 insertions(+), 1 deletion(-) > create mode 100644 include/linux/sframe_lookup.h > create mode 100644 kernel/sframe.h > create mode 100644 kernel/sframe_lookup.c > > -- > 2.48.1.262.g85cc9f2d1e-goog > >
On Wed, Feb 12, 2025 at 03:32:40PM -0800, Song Liu wrote: > [ 81.250437] ------------[ cut here ]------------ > [ 81.250818] refcount_t: saturated; leaking memory. > [ 81.251201] WARNING: CPU: 0 PID: 95 at lib/refcount.c:22 > refcount_warn_saturate+0x6c/0x140 > [ 81.251841] Modules linked in: livepatch_special_static(OEK) > [ 81.252277] CPU: 0 UID: 0 PID: 95 Comm: bash Tainted: G > OE K 6.13.2-00321-g52d2813b4b07 #49 > [ 81.253003] Tainted: [O]=OOT_MODULE, [E]=UNSIGNED_MODULE, [K]=LIVEPATCH > [ 81.253503] Hardware name: linux,dummy-virt (DT) > [ 81.253856] pstate: 634000c5 (nZCv daIF +PAN -UAO +TCO +DIT -SSBS BTYPE=--) > [ 81.254383] pc : refcount_warn_saturate+0x6c/0x140 > [ 81.254748] lr : refcount_warn_saturate+0x6c/0x140 > [ 81.255114] sp : ffff800085a6fc00 > [ 81.255371] x29: ffff800085a6fc00 x28: 0000000001200000 x27: ffff0000c2966180 > [ 81.255918] x26: 0000000000000000 x25: ffff8000829c0000 x24: ffff0000c2e9b608 > [ 81.256462] x23: ffff800083351000 x22: ffff0000c2e9af80 x21: ffff0000c062e140 > [ 81.257006] x20: ffff0000c1c10c00 x19: ffff800085a6fd80 x18: ffffffffffffffff > [ 81.257544] x17: 0000000000000001 x16: ffffffffffffffff x15: 0000000000000006 > [ 81.258083] x14: 0000000000000000 x13: 2e79726f6d656d20 x12: 676e696b61656c20 > [ 81.258625] x11: ffff8000829f7d70 x10: 0000000000000147 x9 : ffff8000801546b4 > [ 81.259165] x8 : 00000000fffeffff x7 : 00000000ffff0000 x6 : ffff800082f77d70 > [ 81.259709] x5 : 80000000ffff0000 x4 : 0000000000000000 x3 : 0000000000000001 > [ 81.260257] x2 : ffff8000829f7a88 x1 : ffff8000829f7a88 x0 : 0000000000000026 > [ 81.260824] Call trace: > [ 81.261015] refcount_warn_saturate+0x6c/0x140 (P) > [ 81.261387] __refcount_add.constprop.0+0x60/0x70 > [ 81.261748] copy_process+0xfdc/0xfd58 [livepatch_special_static] Does that copy_process+0xfdc/0xfd58 resolve to this line in copy_process()? refcount_inc(¤t->signal->sigcnt); Maybe the klp rela reference to 'current' is bogus, or resolving to the wrong address somehow?
On 2/12/25 3:32 PM, Song Liu wrote: > I run some tests with this set and my RFC set [1]. Most of > the test is done with kpatch-build. I tested both Puranjay's > version [3] and my version [4]. > > For gcc 14.2.1, I have seen the following issue with this > test [2]. This happens with both upstream and 6.13.2. > The livepatch loaded fine, but the system spilled out the > following warning quickly. > In presence of the issue https://sourceware.org/bugzilla/show_bug.cgi?id=32666, I'd expect bad data in SFrame section. Which may be causing this symptom? To be clear, the issue affects loaded kernel modules. I cannot tell for certain - is there module loading involved in your test ? > On the other hand, the same test works with LLVM and > my RFC set (LLVM doesn't support SFRAME, and thus > doesn't work with this set yet). > > Thanks, > Song > > > [ 81.250437] ------------[ cut here ]------------ > [ 81.250818] refcount_t: saturated; leaking memory. > [ 81.251201] WARNING: CPU: 0 PID: 95 at lib/refcount.c:22 > refcount_warn_saturate+0x6c/0x140 > [ 81.251841] Modules linked in: livepatch_special_static(OEK) > [ 81.252277] CPU: 0 UID: 0 PID: 95 Comm: bash Tainted: G > OE K 6.13.2-00321-g52d2813b4b07 #49 > [ 81.253003] Tainted: [O]=OOT_MODULE, [E]=UNSIGNED_MODULE, [K]=LIVEPATCH > [ 81.253503] Hardware name: linux,dummy-virt (DT) > [ 81.253856] pstate: 634000c5 (nZCv daIF +PAN -UAO +TCO +DIT -SSBS BTYPE=--) > [ 81.254383] pc : refcount_warn_saturate+0x6c/0x140 > [ 81.254748] lr : refcount_warn_saturate+0x6c/0x140 > [ 81.255114] sp : ffff800085a6fc00 > [ 81.255371] x29: ffff800085a6fc00 x28: 0000000001200000 x27: ffff0000c2966180 > [ 81.255918] x26: 0000000000000000 x25: ffff8000829c0000 x24: ffff0000c2e9b608 > [ 81.256462] x23: ffff800083351000 x22: ffff0000c2e9af80 x21: ffff0000c062e140 > [ 81.257006] x20: ffff0000c1c10c00 x19: ffff800085a6fd80 x18: ffffffffffffffff > [ 81.257544] x17: 0000000000000001 x16: ffffffffffffffff x15: 0000000000000006 > [ 81.258083] x14: 0000000000000000 x13: 2e79726f6d656d20 x12: 676e696b61656c20 > [ 81.258625] x11: ffff8000829f7d70 x10: 0000000000000147 x9 : ffff8000801546b4 > [ 81.259165] x8 : 00000000fffeffff x7 : 00000000ffff0000 x6 : ffff800082f77d70 > [ 81.259709] x5 : 80000000ffff0000 x4 : 0000000000000000 x3 : 0000000000000001 > [ 81.260257] x2 : ffff8000829f7a88 x1 : ffff8000829f7a88 x0 : 0000000000000026 > [ 81.260824] Call trace: > [ 81.261015] refcount_warn_saturate+0x6c/0x140 (P) > [ 81.261387] __refcount_add.constprop.0+0x60/0x70 > [ 81.261748] copy_process+0xfdc/0xfd58 [livepatch_special_static] > [ 81.262217] kernel_clone+0x80/0x3e0 > [ 81.262499] __do_sys_clone+0x5c/0x88 > [ 81.262786] __arm64_sys_clone+0x24/0x38 > [ 81.263085] invoke_syscall+0x4c/0x108 > [ 81.263378] el0_svc_common.constprop.0+0x44/0xe8 > [ 81.263734] do_el0_svc+0x20/0x30 > [ 81.263993] el0_svc+0x34/0xf8 > [ 81.264231] el0t_64_sync_handler+0x104/0x130 > [ 81.264561] el0t_64_sync+0x184/0x188 > [ 81.264846] ---[ end trace 0000000000000000 ]--- > [ 82.335559] ------------[ cut here ]------------ > [ 82.335931] refcount_t: underflow; use-after-free. > [ 82.336307] WARNING: CPU: 1 PID: 0 at lib/refcount.c:28 > refcount_warn_saturate+0xec/0x140 > [ 82.336949] Modules linked in: livepatch_special_static(OEK) > [ 82.337389] CPU: 1 UID: 0 PID: 0 Comm: swapper/1 Tainted: G > W OE K 6.13.2-00321-g52d2813b4b07 #49 > [ 82.338148] Tainted: [W]=WARN, [O]=OOT_MODULE, [E]=UNSIGNED_MODULE, > [K]=LIVEPATCH > [ 82.338721] Hardware name: linux,dummy-virt (DT) > [ 82.339083] pstate: 63400005 (nZCv daif +PAN -UAO +TCO +DIT -SSBS BTYPE=--) > [ 82.339617] pc : refcount_warn_saturate+0xec/0x140 > [ 82.340007] lr : refcount_warn_saturate+0xec/0x140 > [ 82.340378] sp : ffff80008370fe40 > [ 82.340637] x29: ffff80008370fe40 x28: 0000000000000000 x27: 0000000000000000 > [ 82.341188] x26: 000000000000000a x25: ffff0000fdaf7ab8 x24: 0000000000000014 > [ 82.341737] x23: ffff8000829c8d30 x22: ffff80008370ff28 x21: ffff0000fe020000 > [ 82.342286] x20: ffff0000c062e140 x19: ffff0000c2e9af80 x18: ffffffffffffffff > [ 82.342839] x17: ffff80007b7a0000 x16: ffff800083700000 x15: 0000000000000006 > [ 82.343389] x14: 0000000000000000 x13: 2e656572662d7265 x12: 7466612d65737520 > [ 82.343944] x11: ffff8000829f7d70 x10: 000000000000016a x9 : ffff8000801546b4 > [ 82.344499] x8 : 00000000fffeffff x7 : 00000000ffff0000 x6 : ffff800082f77d70 > [ 82.345051] x5 : 80000000ffff0000 x4 : 0000000000000000 x3 : 0000000000000001 > [ 82.345604] x2 : ffff8000829f7a88 x1 : ffff8000829f7a88 x0 : 0000000000000026 > [ 82.346163] Call trace: > [ 82.346359] refcount_warn_saturate+0xec/0x140 (P) > [ 82.346736] __put_task_struct+0x130/0x170 > [ 82.347063] delayed_put_task_struct+0xbc/0xe8 > [ 82.347411] rcu_core+0x20c/0x5f8 > [ 82.347680] rcu_core_si+0x14/0x28 > [ 82.347952] handle_softirqs+0x124/0x308 > [ 82.348260] __do_softirq+0x18/0x20 > [ 82.348536] ____do_softirq+0x14/0x28 > [ 82.348828] call_on_irq_stack+0x24/0x30 > [ 82.349137] do_softirq_own_stack+0x20/0x38 > [ 82.349465] __irq_exit_rcu+0xcc/0x108 > [ 82.349764] irq_exit_rcu+0x14/0x28 > [ 82.350038] el1_interrupt+0x34/0x50 > [ 82.350321] el1h_64_irq_handler+0x14/0x20 > [ 82.350642] el1h_64_irq+0x6c/0x70 > [ 82.350911] default_idle_call+0x30/0xd0 (P) > [ 82.351248] do_idle+0x1d0/0x200 > [ 82.351506] cpu_startup_entry+0x38/0x48 > [ 82.351818] secondary_start_kernel+0x124/0x150 > [ 82.352176] __secondary_switched+0xac/0xb0 > [ 82.352505] ---[ end trace 0000000000000000 ]--- > > > > [1] SFRAME-less livepatch RFC > https://lore.kernel.org/live-patching/20250129232936.1795412-1-song@kernel.org/ > [2] special-static test from kpatch > https://github.com/dynup/kpatch/blob/master/test/integration/linux-6.2.0/special-static.patch > [3] Puranjay's kpatch with arm64 support > https://github.com/puranjaymohan/kpatch/tree/arm64 > [4] My version of kpatch with arm64 and LTO support > https://github.com/liu-song-6/kpatch/tree/fb-6.13-v2 > > On Mon, Jan 27, 2025 at 1:33 PM Weinan Liu <wnliu@google.com> wrote: >> >> 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/ >> >> Madhavan T. Venkataraman (1): >> arm64: Define TIF_PATCH_PENDING for livepatch >> >> Weinan Liu (7): >> unwind: build kernel with sframe info >> arm64: entry: add unwind info for various kernel entries >> unwind: add sframe v2 header >> unwind: Implement generic sframe unwinder library >> unwind: arm64: Add sframe unwinder on arm64 >> unwind: arm64: add reliable stacktrace support for arm64 >> arm64: Enable livepatch for ARM64 >> >> Makefile | 6 + >> arch/Kconfig | 8 + >> arch/arm64/Kconfig | 3 + >> arch/arm64/Kconfig.debug | 10 + >> arch/arm64/include/asm/stacktrace/common.h | 6 + >> arch/arm64/include/asm/thread_info.h | 4 +- >> arch/arm64/kernel/entry-common.c | 4 + >> arch/arm64/kernel/entry.S | 10 + >> arch/arm64/kernel/setup.c | 2 + >> arch/arm64/kernel/stacktrace.c | 102 ++++++++++ >> include/asm-generic/vmlinux.lds.h | 12 ++ >> include/linux/sframe_lookup.h | 43 +++++ >> kernel/Makefile | 1 + >> kernel/sframe.h | 215 +++++++++++++++++++++ >> kernel/sframe_lookup.c | 196 +++++++++++++++++++ >> 15 files changed, 621 insertions(+), 1 deletion(-) >> create mode 100644 include/linux/sframe_lookup.h >> create mode 100644 kernel/sframe.h >> create mode 100644 kernel/sframe_lookup.c >> >> -- >> 2.48.1.262.g85cc9f2d1e-goog >> >>
On Wed, Feb 12, 2025 at 3:49 PM Josh Poimboeuf <jpoimboe@kernel.org> wrote: > > On Wed, Feb 12, 2025 at 03:32:40PM -0800, Song Liu wrote: > > [ 81.250437] ------------[ cut here ]------------ > > [ 81.250818] refcount_t: saturated; leaking memory. > > [ 81.251201] WARNING: CPU: 0 PID: 95 at lib/refcount.c:22 > > refcount_warn_saturate+0x6c/0x140 > > [ 81.251841] Modules linked in: livepatch_special_static(OEK) > > [ 81.252277] CPU: 0 UID: 0 PID: 95 Comm: bash Tainted: G > > OE K 6.13.2-00321-g52d2813b4b07 #49 > > [ 81.253003] Tainted: [O]=OOT_MODULE, [E]=UNSIGNED_MODULE, [K]=LIVEPATCH > > [ 81.253503] Hardware name: linux,dummy-virt (DT) > > [ 81.253856] pstate: 634000c5 (nZCv daIF +PAN -UAO +TCO +DIT -SSBS BTYPE=--) > > [ 81.254383] pc : refcount_warn_saturate+0x6c/0x140 > > [ 81.254748] lr : refcount_warn_saturate+0x6c/0x140 > > [ 81.255114] sp : ffff800085a6fc00 > > [ 81.255371] x29: ffff800085a6fc00 x28: 0000000001200000 x27: ffff0000c2966180 > > [ 81.255918] x26: 0000000000000000 x25: ffff8000829c0000 x24: ffff0000c2e9b608 > > [ 81.256462] x23: ffff800083351000 x22: ffff0000c2e9af80 x21: ffff0000c062e140 > > [ 81.257006] x20: ffff0000c1c10c00 x19: ffff800085a6fd80 x18: ffffffffffffffff > > [ 81.257544] x17: 0000000000000001 x16: ffffffffffffffff x15: 0000000000000006 > > [ 81.258083] x14: 0000000000000000 x13: 2e79726f6d656d20 x12: 676e696b61656c20 > > [ 81.258625] x11: ffff8000829f7d70 x10: 0000000000000147 x9 : ffff8000801546b4 > > [ 81.259165] x8 : 00000000fffeffff x7 : 00000000ffff0000 x6 : ffff800082f77d70 > > [ 81.259709] x5 : 80000000ffff0000 x4 : 0000000000000000 x3 : 0000000000000001 > > [ 81.260257] x2 : ffff8000829f7a88 x1 : ffff8000829f7a88 x0 : 0000000000000026 > > [ 81.260824] Call trace: > > [ 81.261015] refcount_warn_saturate+0x6c/0x140 (P) > > [ 81.261387] __refcount_add.constprop.0+0x60/0x70 > > [ 81.261748] copy_process+0xfdc/0xfd58 [livepatch_special_static] > > Does that copy_process+0xfdc/0xfd58 resolve to this line in > copy_process()? > > refcount_inc(¤t->signal->sigcnt); > > Maybe the klp rela reference to 'current' is bogus, or resolving to the > wrong address somehow? It resolves the following line. p->signal->tty = tty_kref_get(current->signal->tty); I am not quite sure how 'current' should be resolved. The size of copy_process (0xfd58) is wrong. It is only about 5.5kB in size. Also, the copy_process function in the .ko file looks very broken. I will try a few more things. Thanks, Song
On Wed, Feb 12, 2025 at 4:10 PM Indu Bhagat <indu.bhagat@oracle.com> wrote: > > On 2/12/25 3:32 PM, Song Liu wrote: > > I run some tests with this set and my RFC set [1]. Most of > > the test is done with kpatch-build. I tested both Puranjay's > > version [3] and my version [4]. > > > > For gcc 14.2.1, I have seen the following issue with this > > test [2]. This happens with both upstream and 6.13.2. > > The livepatch loaded fine, but the system spilled out the > > following warning quickly. > > > > In presence of the issue > https://sourceware.org/bugzilla/show_bug.cgi?id=32666, I'd expect bad > data in SFrame section. Which may be causing this symptom? > > To be clear, the issue affects loaded kernel modules. I cannot tell for > certain - is there module loading involved in your test ? The KLP is a module, I guess that is also affected? During kpatch-build, we added some logic to drop the .sframe section. I guess this is wrong, as we need the .sframe section when we apply the next KLP. However, I don't think the issue is caused by missing .sframe section. Thanks, Song
On Wed, Feb 12, 2025 at 06:36:04PM -0800, Song Liu wrote: > > > [ 81.261748] copy_process+0xfdc/0xfd58 [livepatch_special_static] > > > > Does that copy_process+0xfdc/0xfd58 resolve to this line in > > copy_process()? > > > > refcount_inc(¤t->signal->sigcnt); > > > > Maybe the klp rela reference to 'current' is bogus, or resolving to the > > wrong address somehow? > > It resolves the following line. > > p->signal->tty = tty_kref_get(current->signal->tty); > > I am not quite sure how 'current' should be resolved. Hm, on arm64 it looks like the value of 'current' is stored in the SP_EL0 register. So I guess that shouldn't need any relocations. > The size of copy_process (0xfd58) is wrong. It is only about > 5.5kB in size. Also, the copy_process function in the .ko file > looks very broken. I will try a few more things. Ah ok, sounds like it's pretty borked.
On Wed, Feb 12, 2025 at 06:40:33PM -0800, Song Liu wrote: > On Wed, Feb 12, 2025 at 4:10 PM Indu Bhagat <indu.bhagat@oracle.com> wrote: > > > > On 2/12/25 3:32 PM, Song Liu wrote: > > > I run some tests with this set and my RFC set [1]. Most of > > > the test is done with kpatch-build. I tested both Puranjay's > > > version [3] and my version [4]. > > > > > > For gcc 14.2.1, I have seen the following issue with this > > > test [2]. This happens with both upstream and 6.13.2. > > > The livepatch loaded fine, but the system spilled out the > > > following warning quickly. > > > > > > > In presence of the issue > > https://sourceware.org/bugzilla/show_bug.cgi?id=32666, I'd expect bad > > data in SFrame section. Which may be causing this symptom? > > > > To be clear, the issue affects loaded kernel modules. I cannot tell for > > certain - is there module loading involved in your test ? > > The KLP is a module, I guess that is also affected? > > During kpatch-build, we added some logic to drop the .sframe section. > I guess this is wrong, as we need the .sframe section when we apply > the next KLP. However, I don't think the issue is caused by missing > .sframe section. Right, it looks like it unwound through the patch module just fine.
Song Liu <song@kernel.org> writes: > On Wed, Feb 12, 2025 at 4:10 PM Indu Bhagat <indu.bhagat@oracle.com> wrote: >> >> On 2/12/25 3:32 PM, Song Liu wrote: >> > I run some tests with this set and my RFC set [1]. Most of >> > the test is done with kpatch-build. I tested both Puranjay's >> > version [3] and my version [4]. >> > >> > For gcc 14.2.1, I have seen the following issue with this >> > test [2]. This happens with both upstream and 6.13.2. >> > The livepatch loaded fine, but the system spilled out the >> > following warning quickly. >> > >> >> In presence of the issue >> https://sourceware.org/bugzilla/show_bug.cgi?id=32666, I'd expect bad >> data in SFrame section. Which may be causing this symptom? >> >> To be clear, the issue affects loaded kernel modules. I cannot tell for >> certain - is there module loading involved in your test ? > > The KLP is a module, I guess that is also affected? > > During kpatch-build, we added some logic to drop the .sframe section. > I guess this is wrong, as we need the .sframe section when we apply > the next KLP. However, I don't think the issue is caused by missing > .sframe section. Hi, I did the same testing and did not get the Warning. I am testing on the 6.12.11 kernel with GCC 11.4.1. Just to verify, the patch we are testing is: --- >8 --- diff -Nupr src.orig/kernel/fork.c src/kernel/fork.c --- src.orig/kernel/fork.c 2023-01-12 11:20:05.408700033 -0500 +++ src/kernel/fork.c 2023-01-12 11:21:19.186137466 -0500 @@ -1700,10 +1700,18 @@ static void posix_cpu_timers_init_group( posix_cputimers_group_init(pct, cpu_limit); } +void kpatch_foo(void) +{ + if (!jiffies) + printk("kpatch copy signal\n"); +} + static int copy_signal(unsigned long clone_flags, struct task_struct *tsk) { struct signal_struct *sig; + kpatch_foo(); + if (clone_flags & CLONE_THREAD) return 0; --- 8< --- P.S. - I have a downstream patch for create-diff-object to generate .sframe sections for livepatch module, will add it to the PR after some cleanups. Thanks, Puranjay
On Wed, Feb 12, 2025 at 11:26 PM Puranjay Mohan <puranjay@kernel.org> wrote: > > Song Liu <song@kernel.org> writes: > > > On Wed, Feb 12, 2025 at 4:10 PM Indu Bhagat <indu.bhagat@oracle.com> wrote: > >> > >> On 2/12/25 3:32 PM, Song Liu wrote: > >> > I run some tests with this set and my RFC set [1]. Most of > >> > the test is done with kpatch-build. I tested both Puranjay's > >> > version [3] and my version [4]. > >> > > >> > For gcc 14.2.1, I have seen the following issue with this > >> > test [2]. This happens with both upstream and 6.13.2. > >> > The livepatch loaded fine, but the system spilled out the > >> > following warning quickly. > >> > > >> > >> In presence of the issue > >> https://sourceware.org/bugzilla/show_bug.cgi?id=32666, I'd expect bad > >> data in SFrame section. Which may be causing this symptom? > >> > >> To be clear, the issue affects loaded kernel modules. I cannot tell for > >> certain - is there module loading involved in your test ? > > > > The KLP is a module, I guess that is also affected? > > > > During kpatch-build, we added some logic to drop the .sframe section. > > I guess this is wrong, as we need the .sframe section when we apply > > the next KLP. However, I don't think the issue is caused by missing > > .sframe section. > > Hi, I did the same testing and did not get the Warning. > > I am testing on the 6.12.11 kernel with GCC 11.4.1. Could you please also try kernel 6.13.2? > Just to verify, the patch we are testing is: Yes, this is the test patch. > > --- >8 --- [...] > --- 8< --- > > P.S. - I have a downstream patch for create-diff-object to generate .sframe sections for > livepatch module, will add it to the PR after some cleanups. Yeah, I think the .sframe section is still needed. Thanks, Song
Song Liu <song@kernel.org> writes: > On Wed, Feb 12, 2025 at 6:45 PM Josh Poimboeuf <jpoimboe@kernel.org> wrote: >> >> On Wed, Feb 12, 2025 at 06:36:04PM -0800, Song Liu wrote: >> > > > [ 81.261748] copy_process+0xfdc/0xfd58 [livepatch_special_static] >> > > >> > > Does that copy_process+0xfdc/0xfd58 resolve to this line in >> > > copy_process()? >> > > >> > > refcount_inc(¤t->signal->sigcnt); >> > > >> > > Maybe the klp rela reference to 'current' is bogus, or resolving to the >> > > wrong address somehow? >> > >> > It resolves the following line. >> > >> > p->signal->tty = tty_kref_get(current->signal->tty); >> > >> > I am not quite sure how 'current' should be resolved. >> >> Hm, on arm64 it looks like the value of 'current' is stored in the >> SP_EL0 register. So I guess that shouldn't need any relocations. >> >> > The size of copy_process (0xfd58) is wrong. It is only about >> > 5.5kB in size. Also, the copy_process function in the .ko file >> > looks very broken. I will try a few more things. > > When I try each step of kpatch-build, the copy_process function > looks reasonable (according to gdb-disassemble) in fork.o and > output.o. However, copy_process looks weird in livepatch-special-static.o, > which is generated by ld: > > ld -EL -maarch64linux -z norelro -z noexecstack > --no-warn-rwx-segments -T ././kpatch.lds -r -o > livepatch-special-static.o ./patch-hook.o ./output.o > > I have attached these files to the email. I am not sure whether > the email server will let them through. I think, I am missing something here, I did : objdump -Dr livepatch-special-static.o | less and objdump -Dr output.o | less and the disassembly of copy_process() looks exactly same. > Indu, does this look like an issue with ld? > > Thanks, > Song
Song Liu <song@kernel.org> writes: > On Wed, Feb 12, 2025 at 11:26 PM Puranjay Mohan <puranjay@kernel.org> wrote: >> >> Song Liu <song@kernel.org> writes: >> >> > On Wed, Feb 12, 2025 at 4:10 PM Indu Bhagat <indu.bhagat@oracle.com> wrote: >> >> >> >> On 2/12/25 3:32 PM, Song Liu wrote: >> >> > I run some tests with this set and my RFC set [1]. Most of >> >> > the test is done with kpatch-build. I tested both Puranjay's >> >> > version [3] and my version [4]. >> >> > >> >> > For gcc 14.2.1, I have seen the following issue with this >> >> > test [2]. This happens with both upstream and 6.13.2. >> >> > The livepatch loaded fine, but the system spilled out the >> >> > following warning quickly. >> >> > >> >> >> >> In presence of the issue >> >> https://sourceware.org/bugzilla/show_bug.cgi?id=32666, I'd expect bad >> >> data in SFrame section. Which may be causing this symptom? >> >> >> >> To be clear, the issue affects loaded kernel modules. I cannot tell for >> >> certain - is there module loading involved in your test ? >> > >> > The KLP is a module, I guess that is also affected? >> > >> > During kpatch-build, we added some logic to drop the .sframe section. >> > I guess this is wrong, as we need the .sframe section when we apply >> > the next KLP. However, I don't think the issue is caused by missing >> > .sframe section. >> >> Hi, I did the same testing and did not get the Warning. >> >> I am testing on the 6.12.11 kernel with GCC 11.4.1. > > Could you please also try kernel 6.13.2? > >> Just to verify, the patch we are testing is: > > Yes, this is the test patch. >> >> --- >8 --- > [...] >> --- 8< --- >> >> P.S. - I have a downstream patch for create-diff-object to generate .sframe sections for >> livepatch module, will add it to the PR after some cleanups. > > Yeah, I think the .sframe section is still needed. > Hi Song, Can you try with this: https://github.com/puranjaymohan/kpatch/tree/arm64_wip This has the .sframe logic patch, but it looks as if I wrote that code in a 30 minute leetcode interview. I need to refactor it before I send it for review with the main PR. Can you test with this branch with your setup? Thanks, Puranjay
Song Liu <song@kernel.org> writes: > On Wed, Feb 12, 2025 at 11:26 PM Puranjay Mohan <puranjay@kernel.org> wrote: >> >> Song Liu <song@kernel.org> writes: >> >> > On Wed, Feb 12, 2025 at 4:10 PM Indu Bhagat <indu.bhagat@oracle.com> wrote: >> >> >> >> On 2/12/25 3:32 PM, Song Liu wrote: >> >> > I run some tests with this set and my RFC set [1]. Most of >> >> > the test is done with kpatch-build. I tested both Puranjay's >> >> > version [3] and my version [4]. >> >> > >> >> > For gcc 14.2.1, I have seen the following issue with this >> >> > test [2]. This happens with both upstream and 6.13.2. >> >> > The livepatch loaded fine, but the system spilled out the >> >> > following warning quickly. >> >> > >> >> >> >> In presence of the issue >> >> https://sourceware.org/bugzilla/show_bug.cgi?id=32666, I'd expect bad >> >> data in SFrame section. Which may be causing this symptom? >> >> >> >> To be clear, the issue affects loaded kernel modules. I cannot tell for >> >> certain - is there module loading involved in your test ? >> > >> > The KLP is a module, I guess that is also affected? >> > >> > During kpatch-build, we added some logic to drop the .sframe section. >> > I guess this is wrong, as we need the .sframe section when we apply >> > the next KLP. However, I don't think the issue is caused by missing >> > .sframe section. >> >> Hi, I did the same testing and did not get the Warning. >> >> I am testing on the 6.12.11 kernel with GCC 11.4.1. > > Could you please also try kernel 6.13.2? > I have tested with 6.13.2, here are the results: [ec2-user@ip-172-31-32-86 ~]$ uname -r 6.13.2+ My tree looks like: [ec2-user@ip-172-31-32-86 linux-upstream]$ git log --oneline bdf4086dd (HEAD) arm64: Enable livepatch for ARM64 3904c7058 arm64: Define TIF_PATCH_PENDING for livepatch b5de442a9 unwind: arm64: add reliable stacktrace support for arm64 fdc243fa8 unwind: arm64: Add sframe unwinder on arm64 87681b7c0 unwind: Implement generic sframe unwinder library c8833ba50 unwind: add sframe v2 header f3f3863cf arm64: entry: add unwind info for various kernel entries 57ab97f05 unwind: build kernel with sframe info 0da4b4b84 Linux 6.13.2 I have also fixed the 87681b7c0 ("unwind: Implement generic sframe unwinder library") with: --- >8 --- diff --git a/kernel/sframe_lookup.c b/kernel/sframe_lookup.c index 846f1da95d89..28bec5064dc7 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< --- GCC is gcc (GCC) 11.4.1 20230605 (Red Hat 11.4.1-2) kpatch is from https://github.com/puranjaymohan/kpatch/tree/arm64_wip I run the with following: ./kpatch/kpatch-build/kpatch-build --non-replace -d -v \ linux-upstream/vmlinux -s linux-upstream/ -c linux-upstream/.config \ kpatch/test/integration/linux-6.2.0/special-static.patch and my dmesg output is: [ec2-user@ip-172-31-32-86 ~]$ sudo dmesg [ 167.202596] livepatch_special_static: loading out-of-tree module taints kernel. [ 167.203217] livepatch_special_static: tainting kernel with TAINT_LIVEPATCH [ 167.203760] livepatch_special_static: module verification failed: signature and/or required key missing - tainting kernel [ 167.205659] livepatch: enabling patch 'livepatch_special_static' [ 167.207358] livepatch: 'livepatch_special_static': starting patching transition [ 168.264901] livepatch: 'livepatch_special_static': patching complete [ 410.641806] livepatch: 'livepatch_special_static': starting unpatching transition [ 412.389369] livepatch: 'livepatch_special_static': unpatching complete I even ran stress-ng with the livepatch loaded to see if something happens. P.S. - The livepatch doesn't have copy_process() but only copy_signal(), yours had copy_process() somehow. Here is the symbol table of the .ko Symbol table '.symtab' contains 169 entries: Num: Value Size Type Bind Vis Ndx Name 0: 0000000000000000 0 NOTYPE LOCAL DEFAULT UND 1: 0000000000000000 0 SECTION LOCAL DEFAULT 1 .note.gnu.build-id 2: 0000000000000000 0 SECTION LOCAL DEFAULT 2 .note.Linux 3: 0000000000000000 0 SECTION LOCAL DEFAULT 3 .text 4: 0000000000000000 0 SECTION LOCAL DEFAULT 5 .exit.text 5: 0000000000000000 0 SECTION LOCAL DEFAULT 7 .init.text 6: 0000000000000000 0 SECTION LOCAL DEFAULT 9 .text.copy_signal 7: 0000000000000000 0 SECTION LOCAL DEFAULT 11 .text.kpatch_foo 8: 0000000000000000 0 SECTION LOCAL DEFAULT 13 .altinstructions 9: 0000000000000000 0 SECTION LOCAL DEFAULT 15 __patchable_function_entries 10: 0000000000000000 0 SECTION LOCAL DEFAULT 17 .codetag.alloc_tags 11: 0000000000000000 0 SECTION LOCAL DEFAULT 18 .plt 12: 0000000000000000 0 SECTION LOCAL DEFAULT 19 .init.plt 13: 0000000000000000 0 SECTION LOCAL DEFAULT 20 .text.ftrace_trampoline 14: 0000000000000000 0 SECTION LOCAL DEFAULT 21 .rodata.str1.8 15: 0000000000000000 0 SECTION LOCAL DEFAULT 22 .modinfo 16: 0000000000000000 0 SECTION LOCAL DEFAULT 23 .sframe 17: 0000000000000000 0 SECTION LOCAL DEFAULT 25 .rodata.trace_raw_output_task_newtask.str1.8 18: 0000000000000000 0 SECTION LOCAL DEFAULT 26 .rodata.trace_raw_output_task_rename.str1.8 19: 0000000000000000 0 SECTION LOCAL DEFAULT 27 .rodata.sighand_ctor.str1.8 20: 0000000000000000 0 SECTION LOCAL DEFAULT 28 .rodata.str 21: 0000000000000000 0 SECTION LOCAL DEFAULT 29 .rodata.__mmdrop.str1.8 22: 0000000000000000 0 SECTION LOCAL DEFAULT 30 .rodata.copy_signal.str1.8 23: 0000000000000000 0 SECTION LOCAL DEFAULT 31 .rodata.mm_init.str1.8 24: 0000000000000000 0 SECTION LOCAL DEFAULT 32 .rodata.vm_area_alloc.str1.8 25: 0000000000000000 0 SECTION LOCAL DEFAULT 33 .rodata.dup_mmap.str1.8 26: 0000000000000000 0 SECTION LOCAL DEFAULT 34 .rodata.copy_process.str1.8 27: 0000000000000000 0 SECTION LOCAL DEFAULT 35 .rodata.kernel_clone.str1.8 28: 0000000000000000 0 SECTION LOCAL DEFAULT 36 .rodata.str__task__trace_system_name 29: 0000000000000000 0 SECTION LOCAL DEFAULT 37 .kpatch.strings 30: 0000000000000000 0 SECTION LOCAL DEFAULT 38 .kpatch.funcs 31: 0000000000000000 0 SECTION LOCAL DEFAULT 40 __versions 32: 0000000000000000 0 SECTION LOCAL DEFAULT 41 .bss 33: 0000000000000000 0 SECTION LOCAL DEFAULT 42 .bss.__key.4 34: 0000000000000000 0 SECTION LOCAL DEFAULT 43 .bss.__key.5 35: 0000000000000000 0 SECTION LOCAL DEFAULT 44 .bss.__key.6 36: 0000000000000000 0 SECTION LOCAL DEFAULT 45 .note.GNU-stack 37: 0000000000000000 0 SECTION LOCAL DEFAULT 46 .comment 38: 0000000000000000 0 SECTION LOCAL DEFAULT 47 .data 39: 0000000000000000 0 SECTION LOCAL DEFAULT 49 .exit.data 40: 0000000000000000 0 SECTION LOCAL DEFAULT 51 .init.data 41: 0000000000000000 0 SECTION LOCAL DEFAULT 53 .kpatch.callbacks.pre_patch 42: 0000000000000000 0 SECTION LOCAL DEFAULT 54 .kpatch.callbacks.post_patch 43: 0000000000000000 0 SECTION LOCAL DEFAULT 55 .kpatch.callbacks.pre_unpatch 44: 0000000000000000 0 SECTION LOCAL DEFAULT 56 .kpatch.callbacks.post_unpatch 45: 0000000000000000 0 SECTION LOCAL DEFAULT 57 .kpatch.force 46: 0000000000000000 0 SECTION LOCAL DEFAULT 58 .gnu.linkonce.this_module 47: 0000000000000000 0 SECTION LOCAL DEFAULT 60 .debug_info 48: 0000000000000000 0 SECTION LOCAL DEFAULT 62 .debug_abbrev 49: 0000000000000000 0 SECTION LOCAL DEFAULT 63 .debug_loc 50: 0000000000000000 0 SECTION LOCAL DEFAULT 65 .debug_aranges 51: 0000000000000000 0 SECTION LOCAL DEFAULT 67 .debug_ranges 52: 0000000000000000 0 SECTION LOCAL DEFAULT 69 .debug_line 53: 0000000000000000 0 SECTION LOCAL DEFAULT 71 .debug_str 54: 0000000000000000 0 SECTION LOCAL DEFAULT 72 .debug_frame 55: 0000000000000000 0 FILE LOCAL DEFAULT ABS module-common.c 56: 0000000000000062 52 OBJECT LOCAL DEFAULT 22 __UNIQUE_ID_vermagic547 57: 0000000000000000 0 NOTYPE LOCAL DEFAULT 2 $d 58: 0000000000000000 24 OBJECT LOCAL DEFAULT 2 _note_19 59: 0000000000000018 52 OBJECT LOCAL DEFAULT 2 _note_18 60: 0000000000000000 0 FILE LOCAL DEFAULT ABS patch-hook.c 61: 0000000000000000 0 NOTYPE LOCAL DEFAULT 3 $x 62: 0000000000000000 0 NOTYPE LOCAL DEFAULT 15 $d 63: 0000000000000008 104 FUNC LOCAL DEFAULT 3 patch_free_livepatch 64: 0000000000000000 0 NOTYPE LOCAL DEFAULT 5 $x 65: 0000000000000000 36 FUNC LOCAL DEFAULT 5 patch_exit 66: 0000000000000078 260 FUNC LOCAL DEFAULT 3 patch_free_scaffold 67: 0000000000000000 0 NOTYPE LOCAL DEFAULT 21 $d 68: 0000000000000188 280 FUNC LOCAL DEFAULT 3 patch_find_object_by_name 69: 00000000000002a8 432 FUNC LOCAL DEFAULT 3 add_callbacks_to_patch_objects 70: 0000000000000000 0 NOTYPE LOCAL DEFAULT 7 $x 71: 0000000000000008 584 FUNC LOCAL DEFAULT 7 patch_init 72: 0000000000000000 0 NOTYPE LOCAL DEFAULT 47 $d 73: 0000000000000000 16 OBJECT LOCAL DEFAULT 47 patch_objects 74: 0000000000000000 0 NOTYPE LOCAL DEFAULT 41 $d 75: 0000000000000000 8 OBJECT LOCAL DEFAULT 41 lpatch 76: 0000000000000008 4 OBJECT LOCAL DEFAULT 41 patch_objects_nr 77: 0000000000000000 0 NOTYPE LOCAL DEFAULT 49 $d 78: 0000000000000000 8 OBJECT LOCAL DEFAULT 49 __UNIQUE_ID___addressable_cleanup_module737 79: 0000000000000000 0 NOTYPE LOCAL DEFAULT 51 $d 80: 0000000000000000 8 OBJECT LOCAL DEFAULT 51 __UNIQUE_ID___addressable_init_module736 81: 0000000000000000 12 OBJECT LOCAL DEFAULT 22 __UNIQUE_ID_livepatch739 82: 000000000000000c 12 OBJECT LOCAL DEFAULT 22 __UNIQUE_ID_license738 83: 0000000000000000 0 FILE LOCAL DEFAULT ABS fork.c 84: 0000000000000008 496 FUNC LOCAL DEFAULT 9 copy_signal 85: 0000000000000000 0 NOTYPE LOCAL DEFAULT 25 $d 86: 0000000000000000 0 NOTYPE LOCAL DEFAULT 26 $d 87: 0000000000000000 0 NOTYPE LOCAL DEFAULT 27 $d 88: 0000000000000000 0 NOTYPE LOCAL DEFAULT 29 $d 89: 0000000000000000 0 NOTYPE LOCAL DEFAULT 30 $d 90: 0000000000000000 0 NOTYPE LOCAL DEFAULT 31 $d 91: 0000000000000000 0 NOTYPE LOCAL DEFAULT 32 $d 92: 0000000000000000 0 NOTYPE LOCAL DEFAULT 33 $d 93: 0000000000000128 0 NOTYPE LOCAL DEFAULT 21 $d 94: 0000000000000000 0 NOTYPE LOCAL DEFAULT 34 $d 95: 0000000000000000 0 NOTYPE LOCAL DEFAULT 35 $d 96: 0000000000000000 0 OBJECT LOCAL DEFAULT 42 __key.4 97: 0000000000000000 0 OBJECT LOCAL DEFAULT 43 __key.5 98: 0000000000000000 0 OBJECT LOCAL DEFAULT 44 __key.6 99: 0000000000000000 5 OBJECT LOCAL DEFAULT 36 str__task__trace_system_name 100: 0000000000000000 0 NOTYPE LOCAL DEFAULT 36 $d 101: 0000000000000000 0 NOTYPE LOCAL DEFAULT 28 .L14472^B1 102: 000000000000000e 0 NOTYPE LOCAL DEFAULT 28 .L14472^B2 103: 000000000000001c 0 NOTYPE LOCAL DEFAULT 28 .L14472^B3 104: 000000000000002a 0 NOTYPE LOCAL DEFAULT 28 .L14472^B4 105: 0000000000000038 0 NOTYPE LOCAL DEFAULT 28 .L14472^B5 106: 0000000000000046 0 NOTYPE LOCAL DEFAULT 28 .L14472^B6 107: 0000000000000054 0 NOTYPE LOCAL DEFAULT 28 .L14472^B7 108: 0000000000000062 0 NOTYPE LOCAL DEFAULT 28 .L14472^B8 109: 0000000000000070 0 NOTYPE LOCAL DEFAULT 28 .L14472^B9 110: 000000000000007e 0 NOTYPE LOCAL DEFAULT 28 .L14472^B10 111: 000000000000008c 0 NOTYPE LOCAL DEFAULT 28 .L14472^B11 112: 00000000000000a9 0 NOTYPE LOCAL DEFAULT 28 .L14472^B12 113: 00000000000000c1 0 NOTYPE LOCAL DEFAULT 28 .L14472^B13 114: 00000000000000d9 0 NOTYPE LOCAL DEFAULT 28 .L14472^B14 115: 00000000000000f1 0 NOTYPE LOCAL DEFAULT 28 .L14472^B15 116: 0000000000000109 0 NOTYPE LOCAL DEFAULT 28 .L14472^B16 117: 0000000000000124 0 NOTYPE LOCAL DEFAULT 28 .L14472^B17 118: 0000000000000132 0 NOTYPE LOCAL DEFAULT 28 .L14472^B18 119: 000000000000014d 0 NOTYPE LOCAL DEFAULT 28 .L14472^B19 120: 0000000000000168 0 NOTYPE LOCAL DEFAULT 28 .L14472^B20 121: 0000000000000000 0 FILE LOCAL DEFAULT ABS livepatch-special-static.mod.c 122: 0000000000000000 0 NOTYPE LOCAL DEFAULT 58 $d 123: 0000000000000018 35 OBJECT LOCAL DEFAULT 22 __UNIQUE_ID_srcversion549 124: 000000000000003b 9 OBJECT LOCAL DEFAULT 22 __UNIQUE_ID_depends548 125: 0000000000000044 30 OBJECT LOCAL DEFAULT 22 __UNIQUE_ID_name547 126: 0000000000000000 0 NOTYPE LOCAL DEFAULT 40 $d 127: 0000000000000000 0 OBJECT LOCAL DEFAULT OS [0xff20] .klp.sym.vmlinux.signal_cachep,1 128: 0000000000000000 1216 OBJECT LOCAL DEFAULT 40 ____versions 129: 0000000000000000 0 NOTYPE GLOBAL DEFAULT 55 __kpatch_callbacks_pre_unpatch_end 130: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UND __init_rwsem 131: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UND __list_add_valid_or_report 132: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UND __kmalloc_noprof 133: 0000000000000000 1344 OBJECT GLOBAL DEFAULT 58 __this_module 134: 0000000000000000 0 NOTYPE GLOBAL DEFAULT 54 __kpatch_callbacks_post_patch 135: 0000000000000000 0 NOTYPE GLOBAL DEFAULT 57 __kpatch_force_funcs 136: 0000000000000000 0 NOTYPE GLOBAL DEFAULT 17 __stop_alloc_tags 137: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UND hrtimer_init 138: 0000000000000000 36 FUNC GLOBAL DEFAULT 5 cleanup_module 139: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UND memcpy 140: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UND klp_enable_patch 141: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UND kfree 142: 0000000000000000 0 NOTYPE GLOBAL DEFAULT 53 __kpatch_callbacks_pre_patch 143: 0000000000000008 584 FUNC GLOBAL DEFAULT 7 init_module 144: 0000000000000000 0 NOTYPE GLOBAL DEFAULT 55 __kpatch_callbacks_pre_unpatch 145: 0000000000000000 0 NOTYPE GLOBAL DEFAULT 17 __start_alloc_tags 146: 0000000000000000 0 NOTYPE GLOBAL DEFAULT 38 __kpatch_funcs 147: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UND _printk 148: 0000000000000000 0 NOTYPE GLOBAL DEFAULT 57 __kpatch_force_funcs_end 149: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UND queued_spin_lock_slowpath 150: 0000000000000038 0 NOTYPE GLOBAL DEFAULT 38 __kpatch_funcs_end 151: 0000000000000000 0 NOTYPE GLOBAL DEFAULT 54 __kpatch_callbacks_post_patch_end 152: 0000000000000000 0 NOTYPE GLOBAL DEFAULT 53 __kpatch_callbacks_pre_patch_end 153: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UND __list_del_entry_valid_or_report 154: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UND __mutex_init 155: 0000000000000008 60 FUNC GLOBAL DEFAULT 11 kpatch_foo 156: 0000000000000000 0 NOTYPE GLOBAL DEFAULT 56 __kpatch_callbacks_post_unpatch_end 157: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UND kmem_cache_alloc_noprof 158: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UND __init_waitqueue_head 159: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UND strcmp 160: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UND jiffies 161: 0000000000000000 0 NOTYPE GLOBAL DEFAULT 56 __kpatch_callbacks_post_unpatch 162: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UND __kmalloc_cache_noprof 163: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UND alt_cb_patch_nops 164: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UND kmalloc_caches 165: 0000000000000000 0 NOTYPE GLOBAL DEFAULT OS [0xff20] .klp.sym.vmlinux.it_real_fn,0 166: 0000000000000000 0 NOTYPE GLOBAL DEFAULT OS [0xff20] .klp.sym.vmlinux.posix_cputimers_group_init,0 167: 0000000000000000 0 NOTYPE GLOBAL DEFAULT OS [0xff20] .klp.sym.vmlinux.tty_audit_fork,0 168: 0000000000000000 0 NOTYPE GLOBAL DEFAULT OS [0xff20] .klp.sym.vmlinux.sched_autogroup_fork,0 and Sections: [ec2-user@ip-172-31-32-86 ~]$ readelf -WS livepatch-special-static.ko There are 78 section headers, starting at offset 0x1727d8: Section Headers: [Nr] Name Type Address Off Size ES Flg Lk Inf Al [ 0] NULL 0000000000000000 000000 000000 00 0 0 0 [ 1] .note.gnu.build-id NOTE 0000000000000000 000040 000024 00 A 0 0 4 [ 2] .note.Linux NOTE 0000000000000000 000064 00004c 00 A 0 0 4 [ 3] .text PROGBITS 0000000000000000 0000c0 000458 00 AX 0 0 32 [ 4] .rela.text RELA 0000000000000000 000518 000570 18 I 74 3 8 [ 5] .exit.text PROGBITS 0000000000000000 000a88 000024 00 AX 0 0 8 [ 6] .rela.exit.text RELA 0000000000000000 000ab0 000048 18 I 74 5 8 [ 7] .init.text PROGBITS 0000000000000000 000b00 000250 00 AX 0 0 32 [ 8] .rela.init.text RELA 0000000000000000 000d50 000348 18 I 74 7 8 [ 9] .text.copy_signal PROGBITS 0000000000000000 001098 0001f8 00 AX 0 0 8 [10] .rela.text.copy_signal RELA 0000000000000000 001290 000240 18 I 74 9 8 [11] .text.kpatch_foo PROGBITS 0000000000000000 0014d0 000044 00 AX 0 0 8 [12] .rela.text.kpatch_foo RELA 0000000000000000 001518 000078 18 I 74 11 8 [13] .altinstructions PROGBITS 0000000000000000 001590 00000c 00 A 0 0 8 [14] .rela.altinstructions RELA 0000000000000000 0015a0 000030 18 I 74 13 8 [15] __patchable_function_entries PROGBITS 0000000000000010 0015d0 000038 00 WAL 3 0 8 [16] .rela__patchable_function_entries RELA 0000000000000000 001608 0000a8 18 I 74 15 8 [17] .codetag.alloc_tags PROGBITS 0000000000000540 0016b0 000000 00 W 0 0 1 [18] .plt PROGBITS 0000000000000000 0016b0 000001 00 AX 0 0 1 [19] .init.plt PROGBITS 0000000000000000 0016b1 000001 00 A 0 0 1 [20] .text.ftrace_trampoline PROGBITS 0000000000000000 0016b2 000001 00 AX 0 0 1 [21] .rodata.str1.8 PROGBITS 0000000000000000 0016b8 000455 01 AMS 0 0 8 [22] .modinfo PROGBITS 0000000000000000 001b0d 000096 00 A 0 0 1 [23] .sframe PROGBITS 0000000000000000 001ba8 0001ad 00 A 0 0 8 [24] .rela.sframe RELA 0000000000000000 001d58 0000c0 18 I 74 23 8 [25] .rodata.trace_raw_output_task_newtask.str1.8 PROGBITS 0000000000000000 001e18 000032 01 AMS 0 0 8 [26] .rodata.trace_raw_output_task_rename.str1.8 PROGBITS 0000000000000000 001e50 000030 01 AMS 0 0 8 [27] .rodata.sighand_ctor.str1.8 PROGBITS 0000000000000000 001e80 000017 01 AMS 0 0 8 [28] .rodata.str PROGBITS 0000000000000000 001e97 000180 01 AMS 0 0 1 [29] .rodata.__mmdrop.str1.8 PROGBITS 0000000000000000 002018 00006b 01 AMS 0 0 8 [30] .rodata.copy_signal.str1.8 PROGBITS 0000000000000000 002088 00005f 01 AMS 0 0 8 [31] .rodata.mm_init.str1.8 PROGBITS 0000000000000000 0020e8 00000f 01 AMS 0 0 8 [32] .rodata.vm_area_alloc.str1.8 PROGBITS 0000000000000000 0020f8 000014 01 AMS 0 0 8 [33] .rodata.dup_mmap.str1.8 PROGBITS 0000000000000000 002110 000054 01 AMS 0 0 8 [34] .rodata.copy_process.str1.8 PROGBITS 0000000000000000 002168 000017 01 AMS 0 0 8 [35] .rodata.kernel_clone.str1.8 PROGBITS 0000000000000000 002180 000009 01 AMS 0 0 8 [36] .rodata.str__task__trace_system_name PROGBITS 0000000000000000 002190 000005 00 A 0 0 8 [37] .kpatch.strings PROGBITS 0000000000000000 002195 00006c 00 A 0 0 1 [38] .kpatch.funcs PROGBITS 0000000000000000 002208 000038 00 A 0 0 8 [39] .rela.kpatch.funcs RELA 0000000000000000 002240 000048 18 I 74 38 8 [40] __versions PROGBITS 0000000000000000 002288 0004c0 00 A 0 0 8 [41] .bss NOBITS 0000000000000000 002748 00000c 00 WA 0 0 8 [42] .bss.__key.4 NOBITS 0000000000000000 002748 000000 00 WA 0 0 8 [43] .bss.__key.5 NOBITS 0000000000000000 002748 000000 00 WA 0 0 8 [44] .bss.__key.6 NOBITS 0000000000000000 002748 000000 00 WA 0 0 8 [45] .note.GNU-stack PROGBITS 0000000000000000 002748 000000 00 0 0 1 [46] .comment PROGBITS 0000000000000000 002748 00008d 01 MS 0 0 1 [47] .data PROGBITS 0000000000000048 0027d8 000010 00 WA 0 0 8 [48] .rela.data RELA 0000000000000000 0027e8 000030 18 I 74 47 8 [49] .exit.data PROGBITS 0000000000000000 002818 000008 00 WA 0 0 8 [50] .rela.exit.data RELA 0000000000000000 002820 000018 18 I 74 49 8 [51] .init.data PROGBITS 0000000000000000 002838 000008 00 WA 0 0 8 [52] .rela.init.data RELA 0000000000000000 002840 000018 18 I 74 51 8 [53] .kpatch.callbacks.pre_patch PROGBITS 0000000000000000 002858 000008 00 WA 0 0 1 [54] .kpatch.callbacks.post_patch PROGBITS 0000000000000000 002860 000008 00 WA 0 0 1 [55] .kpatch.callbacks.pre_unpatch PROGBITS 0000000000000000 002868 000008 00 WA 0 0 1 [56] .kpatch.callbacks.post_unpatch PROGBITS 0000000000000000 002870 000008 00 WA 0 0 1 [57] .kpatch.force PROGBITS 0000000000000000 002878 000008 00 WA 0 0 1 [58] .gnu.linkonce.this_module PROGBITS 0000000000000000 002880 000540 00 WA 0 0 64 [59] .rela.gnu.linkonce.this_module RELA 0000000000000000 002dc0 000030 18 I 74 58 8 [60] .debug_info PROGBITS 0000000000000000 002df0 06041d 00 0 0 1 [61] .rela.debug_info RELA 0000000000000000 063210 08a390 18 I 74 60 8 [62] .debug_abbrev PROGBITS 0000000000000000 0ed5a0 002392 00 0 0 1 [63] .debug_loc PROGBITS 0000000000000000 0ef932 031dd6 00 0 0 1 [64] .rela.debug_loc RELA 0000000000000000 121708 004e30 18 I 74 63 8 [65] .debug_aranges PROGBITS 0000000000000000 126538 000740 00 0 0 1 [66] .rela.debug_aranges RELA 0000000000000000 126c78 0000d8 18 I 74 65 8 [67] .debug_ranges PROGBITS 0000000000000000 126d50 00a870 00 0 0 1 [68] .rela.debug_ranges RELA 0000000000000000 1315c0 001320 18 I 74 67 8 [69] .debug_line PROGBITS 0000000000000000 1328e0 010623 00 0 0 1 [70] .rela.debug_line RELA 0000000000000000 142f08 000078 18 I 74 69 8 [71] .debug_str PROGBITS 0000000000000000 142f80 02b764 01 MS 0 0 1 [72] .debug_frame PROGBITS 0000000000000000 16e6e8 001900 00 0 0 8 [73] .rela.debug_frame RELA 0000000000000000 16ffe8 000b28 18 I 74 72 8 [74] .symtab SYMTAB 0000000000000000 170b10 000fd8 18 75 129 8 [75] .strtab STRTAB 0000000000000000 171ae8 0006cc 00 0 0 1 [76] .shstrtab STRTAB 0000000000000000 1721b4 000576 00 0 0 1 [77] .klp.rela.vmlinux..text.copy_signal RELA 0000000000000000 172730 0000a8 18 AIo 74 9 8 Thanks, Puranjay
On Wed, Feb 12, 2025 at 11:46 PM Puranjay Mohan <puranjay@kernel.org> wrote: > > Song Liu <song@kernel.org> writes: > > > On Wed, Feb 12, 2025 at 6:45 PM Josh Poimboeuf <jpoimboe@kernel.org> wrote: > >> > >> On Wed, Feb 12, 2025 at 06:36:04PM -0800, Song Liu wrote: > >> > > > [ 81.261748] copy_process+0xfdc/0xfd58 [livepatch_special_static] > >> > > > >> > > Does that copy_process+0xfdc/0xfd58 resolve to this line in > >> > > copy_process()? > >> > > > >> > > refcount_inc(¤t->signal->sigcnt); > >> > > > >> > > Maybe the klp rela reference to 'current' is bogus, or resolving to the > >> > > wrong address somehow? > >> > > >> > It resolves the following line. > >> > > >> > p->signal->tty = tty_kref_get(current->signal->tty); > >> > > >> > I am not quite sure how 'current' should be resolved. > >> > >> Hm, on arm64 it looks like the value of 'current' is stored in the > >> SP_EL0 register. So I guess that shouldn't need any relocations. > >> > >> > The size of copy_process (0xfd58) is wrong. It is only about > >> > 5.5kB in size. Also, the copy_process function in the .ko file > >> > looks very broken. I will try a few more things. > > > > When I try each step of kpatch-build, the copy_process function > > looks reasonable (according to gdb-disassemble) in fork.o and > > output.o. However, copy_process looks weird in livepatch-special-static.o, > > which is generated by ld: > > > > ld -EL -maarch64linux -z norelro -z noexecstack > > --no-warn-rwx-segments -T ././kpatch.lds -r -o > > livepatch-special-static.o ./patch-hook.o ./output.o > > > > I have attached these files to the email. I am not sure whether > > the email server will let them through. > > I think, I am missing something here, > > I did : > > objdump -Dr livepatch-special-static.o | less > > and > > objdump -Dr output.o | less > > and the disassembly of copy_process() looks exactly same. Yeah, objdump does show the same disassembly. However, if I open the file with gdb, and do "disassemble copy_process", the one in livepatch-special-static.o looks very weird. Thanks, Song
On Wed, Feb 12, 2025 at 11:53 PM Puranjay Mohan <puranjay@kernel.org> wrote: > > Song Liu <song@kernel.org> writes: > > > On Wed, Feb 12, 2025 at 11:26 PM Puranjay Mohan <puranjay@kernel.org> wrote: > >> > >> Song Liu <song@kernel.org> writes: > >> > >> > On Wed, Feb 12, 2025 at 4:10 PM Indu Bhagat <indu.bhagat@oracle.com> wrote: > >> >> > >> >> On 2/12/25 3:32 PM, Song Liu wrote: > >> >> > I run some tests with this set and my RFC set [1]. Most of > >> >> > the test is done with kpatch-build. I tested both Puranjay's > >> >> > version [3] and my version [4]. > >> >> > > >> >> > For gcc 14.2.1, I have seen the following issue with this > >> >> > test [2]. This happens with both upstream and 6.13.2. > >> >> > The livepatch loaded fine, but the system spilled out the > >> >> > following warning quickly. > >> >> > > >> >> > >> >> In presence of the issue > >> >> https://sourceware.org/bugzilla/show_bug.cgi?id=32666, I'd expect bad > >> >> data in SFrame section. Which may be causing this symptom? > >> >> > >> >> To be clear, the issue affects loaded kernel modules. I cannot tell for > >> >> certain - is there module loading involved in your test ? > >> > > >> > The KLP is a module, I guess that is also affected? > >> > > >> > During kpatch-build, we added some logic to drop the .sframe section. > >> > I guess this is wrong, as we need the .sframe section when we apply > >> > the next KLP. However, I don't think the issue is caused by missing > >> > .sframe section. > >> > >> Hi, I did the same testing and did not get the Warning. > >> > >> I am testing on the 6.12.11 kernel with GCC 11.4.1. > > > > Could you please also try kernel 6.13.2? > > > >> Just to verify, the patch we are testing is: > > > > Yes, this is the test patch. > >> > >> --- >8 --- > > [...] > >> --- 8< --- > >> > >> P.S. - I have a downstream patch for create-diff-object to generate .sframe sections for > >> livepatch module, will add it to the PR after some cleanups. > > > > Yeah, I think the .sframe section is still needed. > > > > Hi Song, > > Can you try with this: > https://github.com/puranjaymohan/kpatch/tree/arm64_wip > > This has the .sframe logic patch, but it looks as if I wrote that code > in a 30 minute leetcode interview. I need to refactor it before I send > it for review with the main PR. > > Can you test with this branch with your setup? This branch has the same issue as the other branch. Song
On Thu, Feb 13, 2025 at 12:38 AM Puranjay Mohan <puranjay@kernel.org> wrote: [...] > > P.S. - The livepatch doesn't have copy_process() but only copy_signal(), > yours had copy_process() somehow. In my build, copy_signal is inlined to copy_process, unless I add noinline. If I do add noinline, the issue will not reproduce. I tried more combinations. The issue doesn't reproduce if I either 1) add noinline to copy_signal, so we are not patching the whole copy_process function; or 2) Switch compiler from gcc 14.2.1 to gcc 11.5.0. So it appears something in gcc 14.2.1 is causing live patch to fail for copy_process(). Thanks, Song [...]
Song Liu <song@kernel.org> writes: > On Thu, Feb 13, 2025 at 12:38 AM Puranjay Mohan <puranjay@kernel.org> wrote: > [...] >> >> P.S. - The livepatch doesn't have copy_process() but only copy_signal(), >> yours had copy_process() somehow. > > In my build, copy_signal is inlined to copy_process, unless I add noinline. > If I do add noinline, the issue will not reproduce. > > I tried more combinations. The issue doesn't reproduce if I either > 1) add noinline to copy_signal, so we are not patching the whole > copy_process function; > or > 2) Switch compiler from gcc 14.2.1 to gcc 11.5.0. > > So it appears something in gcc 14.2.1 is causing live patch to fail > for copy_process(). So, can you test your RFC set (without SFRAME) with gcc 14.2.1, so we can be sure that it is not a sframe problem? And about having the .sframe section in the livepatch module, I realised that this set doesn't include support for reading/using sframe data from any module(livepatches included), so the patch I added for generating .sframe in kpatch is irrelevant because it is a no-op with the current setup. Thanks, Puranjay
On 2/12/25 11:25 PM, Song Liu wrote: > On Wed, Feb 12, 2025 at 6:45 PM Josh Poimboeuf <jpoimboe@kernel.org> wrote: >> >> On Wed, Feb 12, 2025 at 06:36:04PM -0800, Song Liu wrote: >>>>> [ 81.261748] copy_process+0xfdc/0xfd58 [livepatch_special_static] >>>> >>>> Does that copy_process+0xfdc/0xfd58 resolve to this line in >>>> copy_process()? >>>> >>>> refcount_inc(¤t->signal->sigcnt); >>>> >>>> Maybe the klp rela reference to 'current' is bogus, or resolving to the >>>> wrong address somehow? >>> >>> It resolves the following line. >>> >>> p->signal->tty = tty_kref_get(current->signal->tty); >>> >>> I am not quite sure how 'current' should be resolved. >> >> Hm, on arm64 it looks like the value of 'current' is stored in the >> SP_EL0 register. So I guess that shouldn't need any relocations. >> >>> The size of copy_process (0xfd58) is wrong. It is only about >>> 5.5kB in size. Also, the copy_process function in the .ko file >>> looks very broken. I will try a few more things. > > When I try each step of kpatch-build, the copy_process function > looks reasonable (according to gdb-disassemble) in fork.o and > output.o. However, copy_process looks weird in livepatch-special-static.o, > which is generated by ld: > > ld -EL -maarch64linux -z norelro -z noexecstack > --no-warn-rwx-segments -T ././kpatch.lds -r -o > livepatch-special-static.o ./patch-hook.o ./output.o > > I have attached these files to the email. I am not sure whether > the email server will let them through. > > Indu, does this look like an issue with ld? > Sorry for the delay. Looks like there has been progress since and issue may be elsewhere, but: FWIW, I looked at the .sframe and .rela.sframe sections here, the data does look OK. I noted that there is no .sframe for copy_process () in output.o... I will take a look into it.
On Thu, Feb 13, 2025 at 2:22 PM Puranjay Mohan <puranjay@kernel.org> wrote: > > Song Liu <song@kernel.org> writes: > > > On Thu, Feb 13, 2025 at 12:38 AM Puranjay Mohan <puranjay@kernel.org> wrote: > > [...] > >> > >> P.S. - The livepatch doesn't have copy_process() but only copy_signal(), > >> yours had copy_process() somehow. > > > > In my build, copy_signal is inlined to copy_process, unless I add noinline. > > If I do add noinline, the issue will not reproduce. > > > > I tried more combinations. The issue doesn't reproduce if I either > > 1) add noinline to copy_signal, so we are not patching the whole > > copy_process function; > > or > > 2) Switch compiler from gcc 14.2.1 to gcc 11.5.0. > > > > So it appears something in gcc 14.2.1 is causing live patch to fail > > for copy_process(). > > So, can you test your RFC set (without SFRAME) with gcc 14.2.1, so we > can be sure that it is not a sframe problem? My RFC set is the same. No issue with gcc 11.5.0; but hits the same WARNING with gcc 14.2.1. My previous tests are all with clang. I didn't see a similar issue there. > And about having the .sframe section in the livepatch module, I realised > that this set doesn't include support for reading/using sframe data from > any module(livepatches included), so the patch I added for generating > .sframe in kpatch is irrelevant because it is a no-op with the current setup. Agreed, this issue should be irrelevant to the .sframe section in the .ko file. Thanks, Song
On Thu, Feb 13, 2025 at 3:23 PM Indu Bhagat <indu.bhagat@oracle.com> wrote: > > On 2/12/25 11:25 PM, Song Liu wrote: > > On Wed, Feb 12, 2025 at 6:45 PM Josh Poimboeuf <jpoimboe@kernel.org> wrote: > >> > >> On Wed, Feb 12, 2025 at 06:36:04PM -0800, Song Liu wrote: > >>>>> [ 81.261748] copy_process+0xfdc/0xfd58 [livepatch_special_static] > >>>> > >>>> Does that copy_process+0xfdc/0xfd58 resolve to this line in > >>>> copy_process()? > >>>> > >>>> refcount_inc(¤t->signal->sigcnt); > >>>> > >>>> Maybe the klp rela reference to 'current' is bogus, or resolving to the > >>>> wrong address somehow? > >>> > >>> It resolves the following line. > >>> > >>> p->signal->tty = tty_kref_get(current->signal->tty); > >>> > >>> I am not quite sure how 'current' should be resolved. > >> > >> Hm, on arm64 it looks like the value of 'current' is stored in the > >> SP_EL0 register. So I guess that shouldn't need any relocations. > >> > >>> The size of copy_process (0xfd58) is wrong. It is only about > >>> 5.5kB in size. Also, the copy_process function in the .ko file > >>> looks very broken. I will try a few more things. > > > > When I try each step of kpatch-build, the copy_process function > > looks reasonable (according to gdb-disassemble) in fork.o and > > output.o. However, copy_process looks weird in livepatch-special-static.o, > > which is generated by ld: > > > > ld -EL -maarch64linux -z norelro -z noexecstack > > --no-warn-rwx-segments -T ././kpatch.lds -r -o > > livepatch-special-static.o ./patch-hook.o ./output.o > > > > I have attached these files to the email. I am not sure whether > > the email server will let them through. > > > > Indu, does this look like an issue with ld? > > > > Sorry for the delay. > > Looks like there has been progress since and issue may be elsewhere, but: > > FWIW, I looked at the .sframe and .rela.sframe sections here, the data > does look OK. I noted that there is no .sframe for copy_process () in > output.o... I will take a look into it. That output.o was generated with a version of kpatch-build that blindly removes all .sframe sections. Thanks, Song
On Thu, Feb 13, 2025 at 2:22 PM Puranjay Mohan <puranjay@kernel.org> wrote: > > Song Liu <song@kernel.org> writes: > > > On Thu, Feb 13, 2025 at 12:38 AM Puranjay Mohan <puranjay@kernel.org> wrote: > > [...] > >> > >> P.S. - The livepatch doesn't have copy_process() but only copy_signal(), > >> yours had copy_process() somehow. > > > > In my build, copy_signal is inlined to copy_process, unless I add noinline. > > If I do add noinline, the issue will not reproduce. > > > > I tried more combinations. The issue doesn't reproduce if I either > > 1) add noinline to copy_signal, so we are not patching the whole > > copy_process function; > > or > > 2) Switch compiler from gcc 14.2.1 to gcc 11.5.0. > > > > So it appears something in gcc 14.2.1 is causing live patch to fail > > for copy_process(). > > So, can you test your RFC set (without SFRAME) with gcc 14.2.1, so we > can be sure that it is not a sframe problem? > > And about having the .sframe section in the livepatch module, I realised > that this set doesn't include support for reading/using sframe data from > any module(livepatches included), so the patch I added for generating > .sframe in kpatch is irrelevant because it is a no-op with the current setup. Puranjay, Could you please try the following? 1. Use gcc 11.4.1; 2. Add __always_inline to copy_signal(); 3. Build kernel, and livepatch with the same test (we need to add __always_inline to the .patch file). 4. Run gdb livepatch-xxx.ko 5. In gdb do disassemble copy_process. In my tests, both gcc-14.2.1 and gcc-11.5.0 generated a .ko file that looks weird in gdb-disassemble. Specifically, readels shows copy_process is about 5.5kB, but gdb-disassemble only shows 140 bytes or so for copy_process. clang doesn't seem to have this problem. I am really curious whether you have the same problem in your setup. Thanks, Song
Indu Bhagat <indu.bhagat@oracle.com> writes: > On 2/12/25 11:25 PM, Song Liu wrote: >> On Wed, Feb 12, 2025 at 6:45 PM Josh Poimboeuf <jpoimboe@kernel.org> wrote: >>> >>> On Wed, Feb 12, 2025 at 06:36:04PM -0800, Song Liu wrote: >>>>>> [ 81.261748] copy_process+0xfdc/0xfd58 [livepatch_special_static] >>>>> >>>>> Does that copy_process+0xfdc/0xfd58 resolve to this line in >>>>> copy_process()? >>>>> >>>>> refcount_inc(¤t->signal->sigcnt); >>>>> >>>>> Maybe the klp rela reference to 'current' is bogus, or resolving to the >>>>> wrong address somehow? >>>> >>>> It resolves the following line. >>>> >>>> p->signal->tty = tty_kref_get(current->signal->tty); >>>> >>>> I am not quite sure how 'current' should be resolved. >>> >>> Hm, on arm64 it looks like the value of 'current' is stored in the >>> SP_EL0 register. So I guess that shouldn't need any relocations. >>> >>>> The size of copy_process (0xfd58) is wrong. It is only about >>>> 5.5kB in size. Also, the copy_process function in the .ko file >>>> looks very broken. I will try a few more things. >> >> When I try each step of kpatch-build, the copy_process function >> looks reasonable (according to gdb-disassemble) in fork.o and >> output.o. However, copy_process looks weird in livepatch-special-static.o, >> which is generated by ld: >> >> ld -EL -maarch64linux -z norelro -z noexecstack >> --no-warn-rwx-segments -T ././kpatch.lds -r -o >> livepatch-special-static.o ./patch-hook.o ./output.o >> >> I have attached these files to the email. I am not sure whether >> the email server will let them through. >> >> Indu, does this look like an issue with ld? >> > > Sorry for the delay. > > Looks like there has been progress since and issue may be elsewhere, but: > > FWIW, I looked at the .sframe and .rela.sframe sections here, the data > does look OK. I noted that there is no .sframe for copy_process () in > output.o... I will take a look into it. Hi Indu, I saw another issue in my kernel build with sframes enabled (-Wa,--gsframe): ld: warning: orphan section `.init.sframe' from `arch/arm64/kernel/pi/lib-fdt.pi.o' being placed in section `.init.sframe' [... Many more similar warnings (.init.sframe) ...] So, this orphan sections is generated in the build process. I am using GNU ld version 2.41-50 and gcc (GCC) 11.4.1 Is this section needed for sframes to work? or can we discard .init.sframe section with a patch like following to the linker script: -- 8< -- diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h index 6a437bd08..8e704c0a6 100644 --- a/include/asm-generic/vmlinux.lds.h +++ b/include/asm-generic/vmlinux.lds.h @@ -1044,9 +1044,16 @@ defined(CONFIG_AUTOFDO_CLANG) || defined(CONFIG_PROPELLER_CLANG) # define SANITIZER_DISCARDS #endif +#if defined(CONFIG_SFRAME_UNWIND_TABLE) +#define DISCARD_INIT_SFRAME *(.init.sframe) +#else +#define DISCARD_INIT_SFRAME +#endif + #define COMMON_DISCARDS \ SANITIZER_DISCARDS \ PATCHABLE_DISCARDS \ + DISCARD_INIT_SFRAME \ *(.discard) \ *(.discard.*) \ *(.export_symbol) \ -- >8 -- Thanks, Puranjay
On Thu, Feb 13, 2025 at 11:40:43AM -0800, Song Liu wrote: > Yeah, objdump does show the same disassembly. However, if > I open the file with gdb, and do "disassemble copy_process", > the one in livepatch-special-static.o looks very weird. The symbol table looks ok. I'm not sure why gdb is getting confused, but that could possibly be a red herring. Maybe it doesn't like the -ffunction-sections for some reason. It's really weird the function length reported by kallsyms is so wrong. Can you share the .ko? The refcount warning might indicate it's passing some bogus memory to tty_kref_get(). Any chance you have struct randomization enabled? Are you sure there's no code or .config mismatch between the built kernel and the running kernel? Ignorant arm64 question: is the module's text further away from slab memory than vmlinux text, thus requiring a different instruction (or GOT/TOC) to access memory further away in the address space?
Song Liu <song@kernel.org> writes: > On Thu, Feb 13, 2025 at 2:22 PM Puranjay Mohan <puranjay@kernel.org> wrote: >> >> Song Liu <song@kernel.org> writes: >> >> > On Thu, Feb 13, 2025 at 12:38 AM Puranjay Mohan <puranjay@kernel.org> wrote: >> > [...] >> >> >> >> P.S. - The livepatch doesn't have copy_process() but only copy_signal(), >> >> yours had copy_process() somehow. >> > >> > In my build, copy_signal is inlined to copy_process, unless I add noinline. >> > If I do add noinline, the issue will not reproduce. >> > >> > I tried more combinations. The issue doesn't reproduce if I either >> > 1) add noinline to copy_signal, so we are not patching the whole >> > copy_process function; >> > or >> > 2) Switch compiler from gcc 14.2.1 to gcc 11.5.0. >> > >> > So it appears something in gcc 14.2.1 is causing live patch to fail >> > for copy_process(). >> >> So, can you test your RFC set (without SFRAME) with gcc 14.2.1, so we >> can be sure that it is not a sframe problem? >> >> And about having the .sframe section in the livepatch module, I realised >> that this set doesn't include support for reading/using sframe data from >> any module(livepatches included), so the patch I added for generating >> .sframe in kpatch is irrelevant because it is a no-op with the current setup. > > Puranjay, > > Could you please try the following? > > 1. Use gcc 11.4.1; > 2. Add __always_inline to copy_signal(); > 3. Build kernel, and livepatch with the same test (we need to > add __always_inline to the .patch file). > 4. Run gdb livepatch-xxx.ko > 5. In gdb do disassemble copy_process. > > In my tests, both gcc-14.2.1 and gcc-11.5.0 generated a .ko file > that looks weird in gdb-disassemble. Specifically, readels shows > copy_process is about 5.5kB, but gdb-disassemble only shows > 140 bytes or so for copy_process. clang doesn't seem to have > this problem. > > I am really curious whether you have the same problem in your > setup. Hi Song, I did this test and found the same issue as you (gdb assembly broken), but I can see this issue even without the inlining. I think GDB tried to load the debuginfo and that is somehow broken therefore it fails to disassemblt properly. But even with inlining, I couldn't see the warning about the refcount with my setup. Thanks, Puranjay
On 2/13/25 11:57 PM, Puranjay Mohan wrote: > Indu Bhagat <indu.bhagat@oracle.com> writes: > >> On 2/12/25 11:25 PM, Song Liu wrote: >>> On Wed, Feb 12, 2025 at 6:45 PM Josh Poimboeuf <jpoimboe@kernel.org> wrote: >>>> >>>> On Wed, Feb 12, 2025 at 06:36:04PM -0800, Song Liu wrote: >>>>>>> [ 81.261748] copy_process+0xfdc/0xfd58 [livepatch_special_static] >>>>>> >>>>>> Does that copy_process+0xfdc/0xfd58 resolve to this line in >>>>>> copy_process()? >>>>>> >>>>>> refcount_inc(¤t->signal->sigcnt); >>>>>> >>>>>> Maybe the klp rela reference to 'current' is bogus, or resolving to the >>>>>> wrong address somehow? >>>>> >>>>> It resolves the following line. >>>>> >>>>> p->signal->tty = tty_kref_get(current->signal->tty); >>>>> >>>>> I am not quite sure how 'current' should be resolved. >>>> >>>> Hm, on arm64 it looks like the value of 'current' is stored in the >>>> SP_EL0 register. So I guess that shouldn't need any relocations. >>>> >>>>> The size of copy_process (0xfd58) is wrong. It is only about >>>>> 5.5kB in size. Also, the copy_process function in the .ko file >>>>> looks very broken. I will try a few more things. >>> >>> When I try each step of kpatch-build, the copy_process function >>> looks reasonable (according to gdb-disassemble) in fork.o and >>> output.o. However, copy_process looks weird in livepatch-special-static.o, >>> which is generated by ld: >>> >>> ld -EL -maarch64linux -z norelro -z noexecstack >>> --no-warn-rwx-segments -T ././kpatch.lds -r -o >>> livepatch-special-static.o ./patch-hook.o ./output.o >>> >>> I have attached these files to the email. I am not sure whether >>> the email server will let them through. >>> >>> Indu, does this look like an issue with ld? >>> >> >> Sorry for the delay. >> >> Looks like there has been progress since and issue may be elsewhere, but: >> >> FWIW, I looked at the .sframe and .rela.sframe sections here, the data >> does look OK. I noted that there is no .sframe for copy_process () in >> output.o... I will take a look into it. > > Hi Indu, > > I saw another issue in my kernel build with sframes enabled (-Wa,--gsframe): > > ld: warning: orphan section `.init.sframe' from `arch/arm64/kernel/pi/lib-fdt.pi.o' being placed in section `.init.sframe' > [... Many more similar warnings (.init.sframe) ...] > > So, this orphan sections is generated in the build process. > > I am using GNU ld version 2.41-50 and gcc (GCC) 11.4.1 > > Is this section needed for sframes to work? or can we discard No this should not be discarded. This looks like a wrongly-named but valid SFrame section. Once correctly named as .sframe, the linker should do the right thing. Let me check whats going on.. > .init.sframe section with a patch like following to the linker script: > > -- 8< -- > > diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h > index 6a437bd08..8e704c0a6 100644 > --- a/include/asm-generic/vmlinux.lds.h > +++ b/include/asm-generic/vmlinux.lds.h > @@ -1044,9 +1044,16 @@ defined(CONFIG_AUTOFDO_CLANG) || defined(CONFIG_PROPELLER_CLANG) > # define SANITIZER_DISCARDS > #endif > > +#if defined(CONFIG_SFRAME_UNWIND_TABLE) > +#define DISCARD_INIT_SFRAME *(.init.sframe) > +#else > +#define DISCARD_INIT_SFRAME > +#endif > + > #define COMMON_DISCARDS \ > SANITIZER_DISCARDS \ > PATCHABLE_DISCARDS \ > + DISCARD_INIT_SFRAME \ > *(.discard) \ > *(.discard.*) \ > *(.export_symbol) \ > > -- >8 -- > > Thanks, > Puranjay
Hi Puranjay, Thanks for running the tests. On Fri, Feb 14, 2025 at 12:56 AM Puranjay Mohan <puranjay@kernel.org> wrote: [...] > > > > I am really curious whether you have the same problem in your > > setup. > > Hi Song, > > I did this test and found the same issue as you (gdb assembly broken), > but I can see this issue even without the inlining. I think GDB tried to > load the debuginfo and that is somehow broken therefore it fails to > disassemblt properly. Yes, this matches my observations: gcc-11 generates the .ko that confuses gdb. I tested with two versions of gdb (10.2 and 14.2), both have the problem. OTOH, lldb is able to disassemble copy_process from a gcc-compiled .ko file properly. > But even with inlining, I couldn't see the warning about the refcount > with my setup. This also matches my observations. gcc-11 compiled livepatch works fine. Thanks, Song
On Fri, Feb 14, 2025 at 08:56:45AM +0000, Puranjay Mohan wrote: > I did this test and found the same issue as you (gdb assembly broken), > but I can see this issue even without the inlining. I think GDB tried to > load the debuginfo and that is somehow broken therefore it fails to > disassemblt properly. I had the same theory about the debuginfo, but I stripped debug sections from that file and gdb still got confused. Still, the symbol table looked normal, so the gdb issue might be completely separate from the kernel runtime issues.
On 2/14/25 9:39 AM, Indu Bhagat wrote: > On 2/13/25 11:57 PM, Puranjay Mohan wrote: >> Indu Bhagat <indu.bhagat@oracle.com> writes: >> >>> On 2/12/25 11:25 PM, Song Liu wrote: >>>> On Wed, Feb 12, 2025 at 6:45 PM Josh Poimboeuf <jpoimboe@kernel.org> >>>> wrote: >>>>> >>>>> On Wed, Feb 12, 2025 at 06:36:04PM -0800, Song Liu wrote: >>>>>>>> [ 81.261748] copy_process+0xfdc/0xfd58 >>>>>>>> [livepatch_special_static] >>>>>>> >>>>>>> Does that copy_process+0xfdc/0xfd58 resolve to this line in >>>>>>> copy_process()? >>>>>>> >>>>>>> refcount_inc(¤t->signal->sigcnt); >>>>>>> >>>>>>> Maybe the klp rela reference to 'current' is bogus, or resolving >>>>>>> to the >>>>>>> wrong address somehow? >>>>>> >>>>>> It resolves the following line. >>>>>> >>>>>> p->signal->tty = tty_kref_get(current->signal->tty); >>>>>> >>>>>> I am not quite sure how 'current' should be resolved. >>>>> >>>>> Hm, on arm64 it looks like the value of 'current' is stored in the >>>>> SP_EL0 register. So I guess that shouldn't need any relocations. >>>>> >>>>>> The size of copy_process (0xfd58) is wrong. It is only about >>>>>> 5.5kB in size. Also, the copy_process function in the .ko file >>>>>> looks very broken. I will try a few more things. >>>> >>>> When I try each step of kpatch-build, the copy_process function >>>> looks reasonable (according to gdb-disassemble) in fork.o and >>>> output.o. However, copy_process looks weird in livepatch-special- >>>> static.o, >>>> which is generated by ld: >>>> >>>> ld -EL -maarch64linux -z norelro -z noexecstack >>>> --no-warn-rwx-segments -T ././kpatch.lds -r -o >>>> livepatch-special-static.o ./patch-hook.o ./output.o >>>> >>>> I have attached these files to the email. I am not sure whether >>>> the email server will let them through. >>>> >>>> Indu, does this look like an issue with ld? >>>> >>> >>> Sorry for the delay. >>> >>> Looks like there has been progress since and issue may be elsewhere, >>> but: >>> >>> FWIW, I looked at the .sframe and .rela.sframe sections here, the data >>> does look OK. I noted that there is no .sframe for copy_process () in >>> output.o... I will take a look into it. >> >> Hi Indu, >> >> I saw another issue in my kernel build with sframes enabled (-Wa,-- >> gsframe): >> >> ld: warning: orphan section `.init.sframe' from `arch/arm64/kernel/pi/ >> lib-fdt.pi.o' being placed in section `.init.sframe' >> [... Many more similar warnings (.init.sframe) ...] >> >> So, this orphan sections is generated in the build process. >> >> I am using GNU ld version 2.41-50 and gcc (GCC) 11.4.1 >> >> Is this section needed for sframes to work? or can we discard > > No this should not be discarded. This looks like a wrongly-named but > valid SFrame section. > Not wrongly named. --prefix-alloc-sections=.init is expected to do that as .sframe is an allocated section. > Once correctly named as .sframe, the linker should do the right thing. > Let me check whats going on.. > >> .init.sframe section with a patch like following to the linker script: >> >> -- 8< -- >> >> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/ >> vmlinux.lds.h >> index 6a437bd08..8e704c0a6 100644 >> --- a/include/asm-generic/vmlinux.lds.h >> +++ b/include/asm-generic/vmlinux.lds.h >> @@ -1044,9 +1044,16 @@ defined(CONFIG_AUTOFDO_CLANG) || >> defined(CONFIG_PROPELLER_CLANG) >> # define SANITIZER_DISCARDS >> #endif >> >> +#if defined(CONFIG_SFRAME_UNWIND_TABLE) >> +#define DISCARD_INIT_SFRAME *(.init.sframe) >> +#else >> +#define DISCARD_INIT_SFRAME >> +#endif >> + >> #define >> COMMON_DISCARDS \ >> >> SANITIZER_DISCARDS \ >> >> PATCHABLE_DISCARDS \ >> + DISCARD_INIT_SFRAME \ >> >> *(.discard) \ >> >> *(.discard.*) \ >> >> *(.export_symbol) \ >> >> -- >8 -- >> >> Thanks, >> Puranjay >
Indu Bhagat <indu.bhagat@oracle.com> writes: > On 2/14/25 9:39 AM, Indu Bhagat wrote: >> On 2/13/25 11:57 PM, Puranjay Mohan wrote: >>> Indu Bhagat <indu.bhagat@oracle.com> writes: >>> >>>> On 2/12/25 11:25 PM, Song Liu wrote: >>>>> On Wed, Feb 12, 2025 at 6:45 PM Josh Poimboeuf <jpoimboe@kernel.org> >>>>> wrote: >>>>>> >>>>>> On Wed, Feb 12, 2025 at 06:36:04PM -0800, Song Liu wrote: >>>>>>>>> [ 81.261748] copy_process+0xfdc/0xfd58 >>>>>>>>> [livepatch_special_static] >>>>>>>> >>>>>>>> Does that copy_process+0xfdc/0xfd58 resolve to this line in >>>>>>>> copy_process()? >>>>>>>> >>>>>>>> refcount_inc(¤t->signal->sigcnt); >>>>>>>> >>>>>>>> Maybe the klp rela reference to 'current' is bogus, or resolving >>>>>>>> to the >>>>>>>> wrong address somehow? >>>>>>> >>>>>>> It resolves the following line. >>>>>>> >>>>>>> p->signal->tty = tty_kref_get(current->signal->tty); >>>>>>> >>>>>>> I am not quite sure how 'current' should be resolved. >>>>>> >>>>>> Hm, on arm64 it looks like the value of 'current' is stored in the >>>>>> SP_EL0 register. So I guess that shouldn't need any relocations. >>>>>> >>>>>>> The size of copy_process (0xfd58) is wrong. It is only about >>>>>>> 5.5kB in size. Also, the copy_process function in the .ko file >>>>>>> looks very broken. I will try a few more things. >>>>> >>>>> When I try each step of kpatch-build, the copy_process function >>>>> looks reasonable (according to gdb-disassemble) in fork.o and >>>>> output.o. However, copy_process looks weird in livepatch-special- >>>>> static.o, >>>>> which is generated by ld: >>>>> >>>>> ld -EL -maarch64linux -z norelro -z noexecstack >>>>> --no-warn-rwx-segments -T ././kpatch.lds -r -o >>>>> livepatch-special-static.o ./patch-hook.o ./output.o >>>>> >>>>> I have attached these files to the email. I am not sure whether >>>>> the email server will let them through. >>>>> >>>>> Indu, does this look like an issue with ld? >>>>> >>>> >>>> Sorry for the delay. >>>> >>>> Looks like there has been progress since and issue may be elsewhere, >>>> but: >>>> >>>> FWIW, I looked at the .sframe and .rela.sframe sections here, the data >>>> does look OK. I noted that there is no .sframe for copy_process () in >>>> output.o... I will take a look into it. >>> >>> Hi Indu, >>> >>> I saw another issue in my kernel build with sframes enabled (-Wa,-- >>> gsframe): >>> >>> ld: warning: orphan section `.init.sframe' from `arch/arm64/kernel/pi/ >>> lib-fdt.pi.o' being placed in section `.init.sframe' >>> [... Many more similar warnings (.init.sframe) ...] >>> >>> So, this orphan sections is generated in the build process. >>> >>> I am using GNU ld version 2.41-50 and gcc (GCC) 11.4.1 >>> >>> Is this section needed for sframes to work? or can we discard >> >> No this should not be discarded. This looks like a wrongly-named but >> valid SFrame section. >> > > Not wrongly named. --prefix-alloc-sections=.init is expected to do that > as .sframe is an allocated section. > So, these .init.sframe sections are then moved into .sframe by the linker? (see linker script line below) Here are some outputs from the build, do they look correct? One of the objects that were emitting the warning [ec2-user@ip-172-31-32-86 linux-upstream]$ readelf -SW arch/arm64/kernel/pi/lib-fdt.pi.o | grep sframe [47] .init.sframe PROGBITS 0000000000000000 003c90 000226 00 A 0 0 8 [48] .rela.init.sframe RELA 0000000000000000 008f08 000180 18 I 51 47 8 Final vmlinux ELF [ec2-user@ip-172-31-32-86 linux-upstream]$ readelf -SW vmlinux | grep sframe [ 5] .init.sframe PROGBITS ffff80008118c298 119c298 002a88 00 A 0 0 8 [ 6] .sframe PROGBITS ffff80008118ed20 119ed20 247c45 00 A 0 0 8 [ec2-user@ip-172-31-32-86 linux-upstream]$ readelf --sframe=.sframe vmlinux | head Contents of the SFrame section .sframe: Header : Version: SFRAME_VERSION_2 Flags: SFRAME_F_FDE_SORTED Num FDEs: 51842 Num FREs: 321245 Function Index : Does this also look correct? [ec2-user@ip-172-31-32-86 linux-upstream]$ readelf --sframe=.init.sframe vmlinux | head Contents of the SFrame section .init.sframe: Header : Version: SFRAME_VERSION_2 Flags: NONE Num FDEs: 16 Num FREs: 50 Function Index : and the linker script has this line: .sframe : AT(ADDR(.sframe) - 0) { __start_sframe_header = .; KEEP(*(.sframe)) __stop_sframe_header = .; } So, do can you suggest the best way to fix these warnings? Thanks, Puranjay
On Fri, Feb 14, 2025 at 09:51:41AM -0800, Song Liu wrote: > > Ignorant arm64 question: is the module's text further away from slab > > memory than vmlinux text, thus requiring a different instruction (or > > GOT/TOC) to access memory further away in the address space? > > It appears to me the module text is very close to vmlinux text: > > vmlinux: ffff8000800b4b68 T copy_process > module: ffff80007b0f06d0 t copy_process [livepatch_always_inline_special_static] Hm... the only other thing I can think of is that the klp relas might be wrong somewhere. If you share patched.o and .ko files from the same build I could take a look. BTW, I realized the wrong function size shown in the WARNING stack trace is probably just due to a kallsyms quirk. It calculates a symbol's size by subtracting its start address from the next symbol's start address. It doesn't actually use the ELF symbol size. So the next symbol after copy_process() in the loaded module's address space might just be far away. That kallsyms issue has caused other headaches. It really needs to be fixed to use the actual ELF symbol size.
On Fri, Feb 14, 2025 at 06:58:01PM +0000, Puranjay Mohan wrote: > and the linker script has this line: > > .sframe : AT(ADDR(.sframe) - 0) { __start_sframe_header = .; KEEP(*(.sframe)) __stop_sframe_header = .; } > > So, do can you suggest the best way to fix these warnings? Just add *(.init.sframe) like so: .sframe : AT(ADDR(.sframe) - 0) { __start_sframe_header = .; KEEP(*(.sframe) *(.init.sframe)) __stop_sframe_header = .; }
On Fri, Feb 14, 2025 at 11:38:22AM -0800, Josh Poimboeuf wrote: > On Fri, Feb 14, 2025 at 06:58:01PM +0000, Puranjay Mohan wrote: > > and the linker script has this line: > > > > .sframe : AT(ADDR(.sframe) - 0) { __start_sframe_header = .; KEEP(*(.sframe)) __stop_sframe_header = .; } > > > > So, do can you suggest the best way to fix these warnings? > > Just add *(.init.sframe) like so: > > .sframe : AT(ADDR(.sframe) - 0) { __start_sframe_header = .; KEEP(*(.sframe) *(.init.sframe)) __stop_sframe_header = .; } Actually each probably needs its own KEEP: ... KEEP(*(.sframe)) KEEP(*(.init.sframe)) ... or so.
Hi Josh, On Fri, Feb 14, 2025 at 11:34 AM Josh Poimboeuf <jpoimboe@kernel.org> wrote: > > On Fri, Feb 14, 2025 at 09:51:41AM -0800, Song Liu wrote: > > > Ignorant arm64 question: is the module's text further away from slab > > > memory than vmlinux text, thus requiring a different instruction (or > > > GOT/TOC) to access memory further away in the address space? > > > > It appears to me the module text is very close to vmlinux text: > > > > vmlinux: ffff8000800b4b68 T copy_process > > module: ffff80007b0f06d0 t copy_process [livepatch_always_inline_special_static] > > Hm... the only other thing I can think of is that the klp relas might be > wrong somewhere. If you share patched.o and .ko files from the same > build I could take a look. A tarball with these files is available here: https://drive.google.com/file/d/1ONB1tC9oK-Z5ShmSXneqWLTjJgC5Xq-C/view?usp=drive_link > BTW, I realized the wrong function size shown in the WARNING stack trace > is probably just due to a kallsyms quirk. It calculates a symbol's size > by subtracting its start address from the next symbol's start address. > It doesn't actually use the ELF symbol size. So the next symbol after > copy_process() in the loaded module's address space might just be far > away. Yeah, I also think kallsyms logic was the issue here. So it is not the same as the gdb-disassembly issue. > That kallsyms issue has caused other headaches. It really needs to be > fixed to use the actual ELF symbol size. Maybe we should have a "module_text_end" symbol? Thanks, Song
On Fri, Feb 14, 2025 at 02:04:17PM -0800, Song Liu wrote: > Hi Josh, > > On Fri, Feb 14, 2025 at 11:34 AM Josh Poimboeuf <jpoimboe@kernel.org> wrote: > > > > On Fri, Feb 14, 2025 at 09:51:41AM -0800, Song Liu wrote: > > > > Ignorant arm64 question: is the module's text further away from slab > > > > memory than vmlinux text, thus requiring a different instruction (or > > > > GOT/TOC) to access memory further away in the address space? > > > > > > It appears to me the module text is very close to vmlinux text: > > > > > > vmlinux: ffff8000800b4b68 T copy_process > > > module: ffff80007b0f06d0 t copy_process [livepatch_always_inline_special_static] > > > > Hm... the only other thing I can think of is that the klp relas might be > > wrong somewhere. If you share patched.o and .ko files from the same > > build I could take a look. > > A tarball with these files is available here: > > https://drive.google.com/file/d/1ONB1tC9oK-Z5ShmSXneqWLTjJgC5Xq-C/view?usp=drive_link Thanks, I'll take a look. > > That kallsyms issue has caused other headaches. It really needs to be > > fixed to use the actual ELF symbol size. > > Maybe we should have a "module_text_end" symbol? Maybe, though it would be a lot cleaner for kallsyms to just use the actual ELF sizes. And actually I'm thinking that would be a pretty trivial change.
On Fri, Feb 14, 2025 at 02:04:17PM -0800, Song Liu wrote: > Hi Josh, > > On Fri, Feb 14, 2025 at 11:34 AM Josh Poimboeuf <jpoimboe@kernel.org> wrote: > > > > On Fri, Feb 14, 2025 at 09:51:41AM -0800, Song Liu wrote: > > > > Ignorant arm64 question: is the module's text further away from slab > > > > memory than vmlinux text, thus requiring a different instruction (or > > > > GOT/TOC) to access memory further away in the address space? > > > > > > It appears to me the module text is very close to vmlinux text: > > > > > > vmlinux: ffff8000800b4b68 T copy_process > > > module: ffff80007b0f06d0 t copy_process [livepatch_always_inline_special_static] > > > > Hm... the only other thing I can think of is that the klp relas might be > > wrong somewhere. If you share patched.o and .ko files from the same > > build I could take a look. > > A tarball with these files is available here: > > https://drive.google.com/file/d/1ONB1tC9oK-Z5ShmSXneqWLTjJgC5Xq-C/view?usp=drive_link Poking around the arm64 module code, arch/arm64/kernel/module-plts.c is looking at all the relocations in order to set up the PLT. That also needs to be done for klp relas, or are your patches already doing that?
On Fri, Feb 14, 2025 at 3:23 PM Josh Poimboeuf <jpoimboe@kernel.org> wrote: > > On Fri, Feb 14, 2025 at 02:04:17PM -0800, Song Liu wrote: > > Hi Josh, > > > > On Fri, Feb 14, 2025 at 11:34 AM Josh Poimboeuf <jpoimboe@kernel.org> wrote: > > > > > > On Fri, Feb 14, 2025 at 09:51:41AM -0800, Song Liu wrote: > > > > > Ignorant arm64 question: is the module's text further away from slab > > > > > memory than vmlinux text, thus requiring a different instruction (or > > > > > GOT/TOC) to access memory further away in the address space? > > > > > > > > It appears to me the module text is very close to vmlinux text: > > > > > > > > vmlinux: ffff8000800b4b68 T copy_process > > > > module: ffff80007b0f06d0 t copy_process [livepatch_always_inline_special_static] > > > > > > Hm... the only other thing I can think of is that the klp relas might be > > > wrong somewhere. If you share patched.o and .ko files from the same > > > build I could take a look. > > > > A tarball with these files is available here: > > > > https://drive.google.com/file/d/1ONB1tC9oK-Z5ShmSXneqWLTjJgC5Xq-C/view?usp=drive_link > > Poking around the arm64 module code, arch/arm64/kernel/module-plts.c > is looking at all the relocations in order to set up the PLT. That also > needs to be done for klp relas, or are your patches already doing that? I don't think either version (this set and my RFC) added logic for PLT. There is some rela logic on the kpatch-build side. But I am not sure whether it is sufficient. Thanks, Song
On Mon, Feb 17, 2025 at 08:38:22PM -0800, Song Liu wrote: > On Fri, Feb 14, 2025 at 3:23 PM Josh Poimboeuf <jpoimboe@kernel.org> wrote: > > Poking around the arm64 module code, arch/arm64/kernel/module-plts.c > > is looking at all the relocations in order to set up the PLT. That also > > needs to be done for klp relas, or are your patches already doing that? > > I don't think either version (this set and my RFC) added logic for PLT. > There is some rela logic on the kpatch-build side. But I am not sure > whether it is sufficient. The klp relas looked ok. I didn't see any signs of kpatch-build doing anything wrong. AFAICT the problem is that module-plts.c creates PLT entries for regular relas but not klp relas.
Hi Josh, On Mon, Feb 17, 2025 at 10:37 PM Josh Poimboeuf <jpoimboe@kernel.org> wrote: > > On Mon, Feb 17, 2025 at 08:38:22PM -0800, Song Liu wrote: > > On Fri, Feb 14, 2025 at 3:23 PM Josh Poimboeuf <jpoimboe@kernel.org> wrote: > > > Poking around the arm64 module code, arch/arm64/kernel/module-plts.c > > > is looking at all the relocations in order to set up the PLT. That also > > > needs to be done for klp relas, or are your patches already doing that? > > > > I don't think either version (this set and my RFC) added logic for PLT. > > There is some rela logic on the kpatch-build side. But I am not sure > > whether it is sufficient. > > The klp relas looked ok. I didn't see any signs of kpatch-build doing > anything wrong. AFAICT the problem is that module-plts.c creates PLT > entries for regular relas but not klp relas. In my tests (with printk) module-plts.c processes the . klp.rela.vmlinux..text.copy_process section just like any other .rela.* sections. Do we need special handling of the klp.rela.* sections? Thanks, Song
On Tue, Feb 18, 2025 at 10:20:10AM -0800, Song Liu wrote: > Hi Josh, > > On Mon, Feb 17, 2025 at 10:37 PM Josh Poimboeuf <jpoimboe@kernel.org> wrote: > > > > On Mon, Feb 17, 2025 at 08:38:22PM -0800, Song Liu wrote: > > > On Fri, Feb 14, 2025 at 3:23 PM Josh Poimboeuf <jpoimboe@kernel.org> wrote: > > > > Poking around the arm64 module code, arch/arm64/kernel/module-plts.c > > > > is looking at all the relocations in order to set up the PLT. That also > > > > needs to be done for klp relas, or are your patches already doing that? > > > > > > I don't think either version (this set and my RFC) added logic for PLT. > > > There is some rela logic on the kpatch-build side. But I am not sure > > > whether it is sufficient. > > > > The klp relas looked ok. I didn't see any signs of kpatch-build doing > > anything wrong. AFAICT the problem is that module-plts.c creates PLT > > entries for regular relas but not klp relas. > > In my tests (with printk) module-plts.c processes the . > klp.rela.vmlinux..text.copy_process section just like any other .rela.* > sections. Do we need special handling of the klp.rela.* sections? Ok, I see how it works now: klp_write_section_relocs() apply_relocate_add() module_emit_plt_entry() If that code is working correctly then I'm fresh out of ideas...
On Tue, Feb 18, 2025 at 10:41 AM Josh Poimboeuf <jpoimboe@kernel.org> wrote: > > On Tue, Feb 18, 2025 at 10:20:10AM -0800, Song Liu wrote: > > Hi Josh, > > > > On Mon, Feb 17, 2025 at 10:37 PM Josh Poimboeuf <jpoimboe@kernel.org> wrote: > > > > > > On Mon, Feb 17, 2025 at 08:38:22PM -0800, Song Liu wrote: > > > > On Fri, Feb 14, 2025 at 3:23 PM Josh Poimboeuf <jpoimboe@kernel.org> wrote: > > > > > Poking around the arm64 module code, arch/arm64/kernel/module-plts.c > > > > > is looking at all the relocations in order to set up the PLT. That also > > > > > needs to be done for klp relas, or are your patches already doing that? > > > > > > > > I don't think either version (this set and my RFC) added logic for PLT. > > > > There is some rela logic on the kpatch-build side. But I am not sure > > > > whether it is sufficient. > > > > > > The klp relas looked ok. I didn't see any signs of kpatch-build doing > > > anything wrong. AFAICT the problem is that module-plts.c creates PLT > > > entries for regular relas but not klp relas. > > > > In my tests (with printk) module-plts.c processes the . > > klp.rela.vmlinux..text.copy_process section just like any other .rela.* > > sections. Do we need special handling of the klp.rela.* sections? > > Ok, I see how it works now: > > klp_write_section_relocs() > apply_relocate_add() > module_emit_plt_entry() > > If that code is working correctly then I'm fresh out of ideas... I tried to dump assembly of copy_process, but couldn't find any clue. I am wondering whether this is an issue with gcc-14.2.1. Puranjay, could you please try with a different gcc, like some version of gcc-14? Thanks, Song
On Wed, Feb 19, 2025 at 08:50:09PM -0800, Song Liu wrote: > Indu, is this behavior (symbols with same name are not in > sorted order from readelf -s) expected? Or is this a bug? > I am using this gcc: > > $ gcc --version > gcc (GCC) 14.2.1 20240801 (Red Hat 14.2.1-1) > Copyright (C) 2024 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. Are you using different binutils versions as well? It sounds like a linker "issue" to me. I'm not sure if it qualifies as a bug, the linker might be free to layout symbols how it wishes.
On Thu, Feb 20, 2025 at 10:33 AM Song Liu <song@kernel.org> wrote: > > > On Thu, Feb 20, 2025 at 10:22 AM Josh Poimboeuf <jpoimboe@kernel.org> wrote: >> >> On Wed, Feb 19, 2025 at 08:50:09PM -0800, Song Liu wrote: >> > Indu, is this behavior (symbols with same name are not in >> > sorted order from readelf -s) expected? Or is this a bug? >> > I am using this gcc: >> > >> > $ gcc --version >> > gcc (GCC) 14.2.1 20240801 (Red Hat 14.2.1-1) >> > Copyright (C) 2024 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. >> >> Are you using different binutils versions as well? > > > I am using binutil 2.41, for both gcc -11 and gcc-14. > >> >> It sounds like a linker "issue" to me. I'm not sure if it qualifies as >> a bug, the linker might be free to layout symbols how it wishes. > > > We can probably handle that in kpatch-build. OK, something like the following gets the proper sympos for gcc-14 built kernel. Thanks, Song diff --git i/kpatch-build/lookup.c w/kpatch-build/lookup.c index bd2b732de910..87ac127184c5 100644 --- i/kpatch-build/lookup.c +++ w/kpatch-build/lookup.c @@ -479,13 +479,10 @@ static bool lookup_local_symbol(struct lookup_table *table, struct object_symbol *sym; unsigned long sympos = 0; int i, in_file = 0; + bool found = false; memset(result, 0, sizeof(*result)); for_each_obj_symbol(i, sym, table) { - if (sym->bind == STB_LOCAL && !strcmp(sym->name, - lookup_sym->name)) - sympos++; - if (lookup_sym->lookup_table_file_sym == sym) { in_file = 1; continue; @@ -499,20 +496,29 @@ static bool lookup_local_symbol(struct lookup_table *table, if (sym->bind == STB_LOCAL && !strcmp(sym->name, lookup_sym->name)) { - if (result->objname) + if (found) ERROR("duplicate local symbol found for %s", lookup_sym->name); result->objname = table->objname; result->addr = sym->addr; result->size = sym->size; - result->sympos = sympos; result->global = false; result->exported = false; + found = true; } } + if (!found) + return false; - return !!result->objname; + for_each_obj_symbol(i, sym, table) { + if (sym->bind == STB_LOCAL && + !strcmp(sym->name, lookup_sym->name) && + sym->addr < result->addr) + sympos++; + } + result->sympos = sympos; + return true; } static bool lookup_exported_symbol(struct lookup_table *table, char *name,
On Mon, Feb 10, 2025 at 12:30 AM Weinan Liu <wnliu@google.com> wrote: > I already have a WIP patch to add sframe support to the kernel module. > However, it is not yet working. I had trouble unwinding frames for the > kernel module using the current algorithm. > > Indu has likely identified the issue and will be addressing it from the > toolchain side. > > https://sourceware.org/bugzilla/show_bug.cgi?id=32666 I have a working in progress patch that adds sframe support for kernel module. https://github.com/heuza/linux/tree/sframe_unwinder.rfc According to the sframe table values I got during runtime testing, looks like the offsets are not correct . When unwind symbols init_module(0xffff80007b155048) from the kernel module(livepatch-sample.ko), the start_address of the FDE entries in the sframe table of the kernel modules appear incorrect. For instance, the first FDE's start_addr is reported as -20564. Adding this offset to the module's sframe section address (0xffff80007b15a040) yields 0xffff80007b154fec, which is not within the livepatch-sample.ko memory region(It should be larger than 0xffff80007b155000). Here are the sframe table values of the livepatch-samples.ko that I print by qemu + gdb. ``` $ /usr/bin/aarch64-linux-gnu-objdump -L --sframe=.sframe ./samples/livepatch/livepatch-sample.ko ./samples/livepatch/livepatch-sample.ko: file format elf64-littleaarch64 Contents of the SFrame section .sframe: Header : Version: SFRAME_VERSION_2 Flags: SFRAME_F_FDE_SORTED Num FDEs: 3 Num FREs: 11 Function Index : func idx [0]: pc = 0x0, size = 12 bytes STARTPC CFA FP RA 0000000000000000 sp+0 u u func idx [1]: pc = 0x0, size = 44 bytes STARTPC CFA FP RA 0000000000000000 sp+0 u u 000000000000000c sp+0 u u[s] 0000000000000010 sp+16 c-16 c-8[s] 0000000000000024 sp+0 u u[s] 0000000000000028 sp+0 u u func idx [2]: pc = 0x0, size = 56 bytes STARTPC CFA FP RA 0000000000000000 sp+0 u u 000000000000000c sp+0 u u[s] 0000000000000010 sp+16 c-16 c-8[s] 0000000000000030 sp+0 u u[s] 0000000000000034 sp+0 u u (gdb) bt #0 find_fde (tbl=0xffff80007b157708, pc=18446603338286190664) at kernel/sframe_lookup.c:75 #1 0xffff80008031e260 in sframe_find_pc (pc=18446603338286190664, entry=0xffff800086f83800) at kernel/sframe_lookup.c:175 #2 0xffff800080035a48 in unwind_next_frame_sframe (state=0xffff800086f83828) at arch/arm64/kernel/stacktrace.c:270 #3 kunwind_next (state=0xffff800086f83828) at arch/arm64/kernel/stacktrace.c:332 ... (gdb) lx-symbols loading vmlinux scanning for modules in /home/wnliu/kernel loading @0xffff80007b155000: /home/wnliu/kernel/samples/livepatch/livepatch-sample.ko loading @0xffff80007b14d000: /home/wnliu/kernel/fs/fat/vfat.ko loading @0xffff80007b130000: /home/wnliu/kernel/fs/fat/fat.ko (gdb) p/x *tbl->sfhdr_p $5 = {preamble = {magic = 0xdee2, version = 0x2, flags = 0x1}, abi_arch = 0x2, cfa_fixed_fp_offset = 0x0, cfa_fixed_ra_offset = 0x0, auxhdr_len = 0x0, num_fdes = 0x3, num_fres = 0xb, fre_len = 0x25, fdes_off = 0x0, fres_off = 0x3c} (gdb) p/x tbl->sfhdr_p $6 = 0xffff80007b15a040 (gdb) p *tbl->fde_p $7 = {start_addr = -20564, size = 12, fres_off = 0, fres_num = 1, info = 0 '\000', rep_size = 0 '\000', padding = 0} (gdb) p *(tbl->fde_p + 1) $11 = {start_addr = -20552, size = 44, fres_off = 3, fres_num = 5, info = 0 '\000', rep_size = 0 '\000', padding = 0} (gdb) p *(tbl->fde_p + 2) $12 = {start_addr = -20508, size = 56, fres_off = 20, fres_num = 5, info = 0 '\000', rep_size = 0 '\000', padding = 0} /* -20564 + 0xffff80007b15a040 = 0xffff80007b154fec */ (gdb) info symbol 0xffff80007b154fec No symbol matches 0xffff80007b154fec ```
On Tue, Feb 25, 2025 at 01:02:24AM +0000, Weinan Liu wrote: > On Mon, Feb 10, 2025 at 12:30 AM Weinan Liu <wnliu@google.com> wrote: > > I already have a WIP patch to add sframe support to the kernel module. > > However, it is not yet working. I had trouble unwinding frames for the > > kernel module using the current algorithm. > > > > Indu has likely identified the issue and will be addressing it from the > > toolchain side. > > > > https://sourceware.org/bugzilla/show_bug.cgi?id=32666 > > > I have a working in progress patch that adds sframe support for kernel > module. > https://github.com/heuza/linux/tree/sframe_unwinder.rfc > > According to the sframe table values I got during runtime testing, looks > like the offsets are not correct . > > When unwind symbols init_module(0xffff80007b155048) from the kernel > module(livepatch-sample.ko), the start_address of the FDE entries in the > sframe table of the kernel modules appear incorrect. > For instance, the first FDE's start_addr is reported as -20564. Adding > this offset to the module's sframe section address (0xffff80007b15a040) > yields 0xffff80007b154fec, which is not within the livepatch-sample.ko > memory region(It should be larger than 0xffff80007b155000). I assume kpatch create-diff-object needs to copy over a subset of the .sframe section. Similar to what kpatch_regenerate_orc_sections() does.
On Mon, Feb 10, 2025 at 12:30 AM Weinan Liu <wnliu@google.com> wrote: >> I already have a WIP patch to add sframe support to the kernel module. >> However, it is not yet working. I had trouble unwinding frames for the >> kernel module using the current algorithm. >> >> Indu has likely identified the issue and will be addressing it from the >> toolchain side. >> >> https://sourceware.org/bugzilla/show_bug.cgi?id=32666 > > I have a working in progress patch that adds sframe support for kernel > module. > https://github.com/heuza/linux/tree/sframe_unwinder.rfc > > According to the sframe table values I got during runtime testing, looks > like the offsets are not correct . > I hope to sanitize the fix for 32666 and post upstream soon (I had to address other related issues). Unless fixed, relocating .sframe sections using the .rela.sframe is expected to generate incorrect output. > When unwind symbols init_module(0xffff80007b155048) from the kernel > module(livepatch-sample.ko), the start_address of the FDE entries in the > sframe table of the kernel modules appear incorrect. init_module will apply the relocations on the .sframe section, isnt it ? > For instance, the first FDE's start_addr is reported as -20564. Adding > this offset to the module's sframe section address (0xffff80007b15a040) > yields 0xffff80007b154fec, which is not within the livepatch-sample.ko > memory region(It should be larger than 0xffff80007b155000). > Hmm..something seems off here. Having tested a potential fix for 32666 locally, I do not expect the first FDE to show this symptom. > Here are the sframe table values of the livepatch-samples.ko that I print > by qemu + gdb. > > ``` > $ /usr/bin/aarch64-linux-gnu-objdump -L --sframe=.sframe ./samples/livepatch/livepatch-sample.ko > ./samples/livepatch/livepatch-sample.ko: file format elf64-littleaarch64 > > Contents of the SFrame section .sframe: > Header : > > Version: SFRAME_VERSION_2 > Flags: SFRAME_F_FDE_SORTED > Num FDEs: 3 > Num FREs: 11 > > Function Index : > > func idx [0]: pc = 0x0, size = 12 bytes > STARTPC CFA FP RA > 0000000000000000 sp+0 u u > > func idx [1]: pc = 0x0, size = 44 bytes > STARTPC CFA FP RA > 0000000000000000 sp+0 u u > 000000000000000c sp+0 u u[s] > 0000000000000010 sp+16 c-16 c-8[s] > 0000000000000024 sp+0 u u[s] > 0000000000000028 sp+0 u u > > func idx [2]: pc = 0x0, size = 56 bytes > STARTPC CFA FP RA > 0000000000000000 sp+0 u u > 000000000000000c sp+0 u u[s] > 0000000000000010 sp+16 c-16 c-8[s] > 0000000000000030 sp+0 u u[s] > 0000000000000034 sp+0 u u > > > > (gdb) bt > #0 find_fde (tbl=0xffff80007b157708, pc=18446603338286190664) at kernel/sframe_lookup.c:75 > #1 0xffff80008031e260 in sframe_find_pc (pc=18446603338286190664, entry=0xffff800086f83800) at kernel/sframe_lookup.c:175 > #2 0xffff800080035a48 in unwind_next_frame_sframe (state=0xffff800086f83828) at arch/arm64/kernel/stacktrace.c:270 > #3 kunwind_next (state=0xffff800086f83828) at arch/arm64/kernel/stacktrace.c:332 > ... > > (gdb) lx-symbols > loading vmlinux > scanning for modules in /home/wnliu/kernel > loading @0xffff80007b155000: /home/wnliu/kernel/samples/livepatch/livepatch-sample.ko > loading @0xffff80007b14d000: /home/wnliu/kernel/fs/fat/vfat.ko > loading @0xffff80007b130000: /home/wnliu/kernel/fs/fat/fat.ko > > (gdb) p/x *tbl->sfhdr_p > $5 = {preamble = {magic = 0xdee2, version = 0x2, flags = 0x1}, abi_arch = 0x2, cfa_fixed_fp_offset = 0x0, cfa_fixed_ra_offset = 0x0, auxhdr_len = 0x0, num_fdes = 0x3, num_fres = 0xb, fre_len = 0x25, fdes_off = 0x0, fres_off = 0x3c} > > (gdb) p/x tbl->sfhdr_p > $6 = 0xffff80007b15a040 > > (gdb) p *tbl->fde_p > $7 = {start_addr = -20564, size = 12, fres_off = 0, fres_num = 1, info = 0 '\000', rep_size = 0 '\000', padding = 0} > > (gdb) p *(tbl->fde_p + 1) > $11 = {start_addr = -20552, size = 44, fres_off = 3, fres_num = 5, info = 0 '\000', rep_size = 0 '\000', padding = 0} > > (gdb) p *(tbl->fde_p + 2) > $12 = {start_addr = -20508, size = 56, fres_off = 20, fres_num = 5, info = 0 '\000', rep_size = 0 '\000', padding = 0} > > /* -20564 + 0xffff80007b15a040 = 0xffff80007b154fec */ > (gdb) info symbol 0xffff80007b154fec > No symbol matches 0xffff80007b154fec > ```
On Tue, Feb 25, 2025 at 10:13 AM Josh Poimboeuf <jpoimboe@kernel.org> wrote: > > On Tue, Feb 25, 2025 at 01:02:24AM +0000, Weinan Liu wrote: > > On Mon, Feb 10, 2025 at 12:30 AM Weinan Liu <wnliu@google.com> wrote: > > > I already have a WIP patch to add sframe support to the kernel module. > > > However, it is not yet working. I had trouble unwinding frames for the > > > kernel module using the current algorithm. > > > > > > Indu has likely identified the issue and will be addressing it from the > > > toolchain side. > > > > > > https://sourceware.org/bugzilla/show_bug.cgi?id=32666 > > > > > > I have a working in progress patch that adds sframe support for kernel > > module. > > https://github.com/heuza/linux/tree/sframe_unwinder.rfc > > > > According to the sframe table values I got during runtime testing, looks > > like the offsets are not correct . > > > > When unwind symbols init_module(0xffff80007b155048) from the kernel > > module(livepatch-sample.ko), the start_address of the FDE entries in the > > sframe table of the kernel modules appear incorrect. > > For instance, the first FDE's start_addr is reported as -20564. Adding > > this offset to the module's sframe section address (0xffff80007b15a040) > > yields 0xffff80007b154fec, which is not within the livepatch-sample.ko > > memory region(It should be larger than 0xffff80007b155000). > > I assume kpatch create-diff-object needs to copy over a subset of the > .sframe section. Similar to what kpatch_regenerate_orc_sections() does. You're right that we need to process the sframe section like what kpatch_regenerate_orc_sections() does when building livepatch by kpatch. However, livepatch-sample.ko is not generated by kpatch. It is built directly from samples/livepatch/livepatch-sample.c by gcc during the kernel build
On Tue, Feb 25, 2025 at 11:38 AM Indu Bhagat <indu.bhagat@oracle.com> wrote: > > On Mon, Feb 10, 2025 at 12:30 AM Weinan Liu <wnliu@google.com> wrote: > >> I already have a WIP patch to add sframe support to the kernel module. > >> However, it is not yet working. I had trouble unwinding frames for the > >> kernel module using the current algorithm. > >> > >> Indu has likely identified the issue and will be addressing it from the > >> toolchain side. > >> > >> https://sourceware.org/bugzilla/show_bug.cgi?id=32666 > > > > I have a working in progress patch that adds sframe support for kernel > > module. > > https://github.com/heuza/linux/tree/sframe_unwinder.rfc > > > > According to the sframe table values I got during runtime testing, looks > > like the offsets are not correct . > > > > I hope to sanitize the fix for 32666 and post upstream soon (I had to > address other related issues). Unless fixed, relocating .sframe > sections using the .rela.sframe is expected to generate incorrect output. > > > When unwind symbols init_module(0xffff80007b155048) from the kernel > > module(livepatch-sample.ko), the start_address of the FDE entries in the > > sframe table of the kernel modules appear incorrect. > > init_module will apply the relocations on the .sframe section, isnt it ? > > > For instance, the first FDE's start_addr is reported as -20564. Adding > > this offset to the module's sframe section address (0xffff80007b15a040) > > yields 0xffff80007b154fec, which is not within the livepatch-sample.ko > > memory region(It should be larger than 0xffff80007b155000). > > > > Hmm..something seems off here. Having tested a potential fix for 32666 > locally, I do not expect the first FDE to show this symptom. > Yes, I think init_module will apply the relocation as well. To further investigate, here's the relevant relocation and symbol table information for the kernel module: Relocation section '.rela.sframe' at offset 0x28350 contains 3 entries: Offset Info Type Sym. Value Sym. Name + Addend 00000000001c 000100000105 R_AARCH64_PREL32 0000000000000000 .text + 8 000000000030 000100000105 R_AARCH64_PREL32 0000000000000000 .text + 28 000000000044 000100000105 R_AARCH64_PREL32 0000000000000000 .text + 68 Symbol table '.symtab' contains 68 entries: Num: Value Size Type Bind Vis Ndx Name 0: 0000000000000000 0 NOTYPE LOCAL DEFAULT UND 1: 0000000000000000 0 SECTION LOCAL DEFAULT 1 .text ... 32: 0000000000000008 12 FUNC LOCAL DEFAULT 1 livepatch_exit 33: 0000000000000008 0 NOTYPE LOCAL DEFAULT 3 $d 34: 0000000000000028 44 FUNC LOCAL DEFAULT 1 livepatch_init 35: 0000000000000000 0 NOTYPE LOCAL DEFAULT 9 $d 36: 0000000000000010 0 NOTYPE LOCAL DEFAULT 3 $d 37: 0000000000000068 56 FUNC LOCAL DEFAULT 1 livepatch_cmdlin[...] ... 63: 0000000000000008 12 FUNC GLOBAL DEFAULT 1 cleanup_module 64: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UND klp_enable_patch 65: 0000000000000028 44 FUNC GLOBAL DEFAULT 1 init_module
On 2/25/25 3:54 PM, Weinan Liu wrote: > On Tue, Feb 25, 2025 at 11:38 AM Indu Bhagat <indu.bhagat@oracle.com> wrote: >> >> On Mon, Feb 10, 2025 at 12:30 AM Weinan Liu <wnliu@google.com> wrote: >>>> I already have a WIP patch to add sframe support to the kernel module. >>>> However, it is not yet working. I had trouble unwinding frames for the >>>> kernel module using the current algorithm. >>>> >>>> Indu has likely identified the issue and will be addressing it from the >>>> toolchain side. >>>> >>>> https://sourceware.org/bugzilla/show_bug.cgi?id=32666 >>> >>> I have a working in progress patch that adds sframe support for kernel >>> module. >>> https://github.com/heuza/linux/tree/sframe_unwinder.rfc >>> >>> According to the sframe table values I got during runtime testing, looks >>> like the offsets are not correct . >>> >> >> I hope to sanitize the fix for 32666 and post upstream soon (I had to >> address other related issues). Unless fixed, relocating .sframe >> sections using the .rela.sframe is expected to generate incorrect output. >> >>> When unwind symbols init_module(0xffff80007b155048) from the kernel >>> module(livepatch-sample.ko), the start_address of the FDE entries in the >>> sframe table of the kernel modules appear incorrect. >> >> init_module will apply the relocations on the .sframe section, isnt it ? >> >>> For instance, the first FDE's start_addr is reported as -20564. Adding >>> this offset to the module's sframe section address (0xffff80007b15a040) >>> yields 0xffff80007b154fec, which is not within the livepatch-sample.ko >>> memory region(It should be larger than 0xffff80007b155000). >>> >> >> Hmm..something seems off here. Having tested a potential fix for 32666 >> locally, I do not expect the first FDE to show this symptom. >> > > Yes, I think init_module will apply the relocation as well. > To further investigate, here's the relevant relocation and symbol table > information for the kernel module: > > Relocation section '.rela.sframe' at offset 0x28350 contains 3 entries: > Offset Info Type Sym. Value Sym. Name + Addend > 00000000001c 000100000105 R_AARCH64_PREL32 0000000000000000 .text + 8 > 000000000030 000100000105 R_AARCH64_PREL32 0000000000000000 .text + 28 > 000000000044 000100000105 R_AARCH64_PREL32 0000000000000000 .text + 68 > The offsets look OK.. > Symbol table '.symtab' contains 68 entries: > Num: Value Size Type Bind Vis Ndx Name > 0: 0000000000000000 0 NOTYPE LOCAL DEFAULT UND > 1: 0000000000000000 0 SECTION LOCAL DEFAULT 1 .text > ... > 32: 0000000000000008 12 FUNC LOCAL DEFAULT 1 livepatch_exit > 33: 0000000000000008 0 NOTYPE LOCAL DEFAULT 3 $d > 34: 0000000000000028 44 FUNC LOCAL DEFAULT 1 livepatch_init > 35: 0000000000000000 0 NOTYPE LOCAL DEFAULT 9 $d > 36: 0000000000000010 0 NOTYPE LOCAL DEFAULT 3 $d > 37: 0000000000000068 56 FUNC LOCAL DEFAULT 1 livepatch_cmdlin[...] > ... > 63: 0000000000000008 12 FUNC GLOBAL DEFAULT 1 cleanup_module > 64: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UND klp_enable_patch > 65: 0000000000000028 44 FUNC GLOBAL DEFAULT 1 init_module
Indu Bhagat <indu.bhagat@oracle.com> writes: > On 2/25/25 3:54 PM, Weinan Liu wrote: >> On Tue, Feb 25, 2025 at 11:38 AM Indu Bhagat <indu.bhagat@oracle.com> wrote: >>> >>> On Mon, Feb 10, 2025 at 12:30 AM Weinan Liu <wnliu@google.com> wrote: >>>>> I already have a WIP patch to add sframe support to the kernel module. >>>>> However, it is not yet working. I had trouble unwinding frames for the >>>>> kernel module using the current algorithm. >>>>> >>>>> Indu has likely identified the issue and will be addressing it from the >>>>> toolchain side. >>>>> >>>>> https://sourceware.org/bugzilla/show_bug.cgi?id=32666 >>>> >>>> I have a working in progress patch that adds sframe support for kernel >>>> module. >>>> https://github.com/heuza/linux/tree/sframe_unwinder.rfc >>>> >>>> According to the sframe table values I got during runtime testing, looks >>>> like the offsets are not correct . >>>> >>> >>> I hope to sanitize the fix for 32666 and post upstream soon (I had to >>> address other related issues). Unless fixed, relocating .sframe >>> sections using the .rela.sframe is expected to generate incorrect output. >>> >>>> When unwind symbols init_module(0xffff80007b155048) from the kernel >>>> module(livepatch-sample.ko), the start_address of the FDE entries in the >>>> sframe table of the kernel modules appear incorrect. >>> >>> init_module will apply the relocations on the .sframe section, isnt it ? >>> >>>> For instance, the first FDE's start_addr is reported as -20564. Adding >>>> this offset to the module's sframe section address (0xffff80007b15a040) >>>> yields 0xffff80007b154fec, which is not within the livepatch-sample.ko >>>> memory region(It should be larger than 0xffff80007b155000). >>>> >>> >>> Hmm..something seems off here. Having tested a potential fix for 32666 >>> locally, I do not expect the first FDE to show this symptom. >>> >> Hi, Sorry for not responding in the past few days. I was on PTO and was trying to improve my snowboarding technique, I am back now!! I think what we are seeing is expected behaviour: | For instance, the first FDE's start_addr is reported as -20564. Adding | this offset to the module's sframe section address (0xffff80007b15a040) | yields 0xffff80007b154fec, which is not within the livepatch-sample.ko | memory region(It should be larger than 0xffff80007b155000). Let me explain using a __dummy__ example. Assume Memory layout before relocation: | Address | Element | Relocation | .... | .... | | 60 | init_module (start address) | | 72 | init_module (end address) | | .... | ..... | | 100 | Sframe section header start address | | 128 | First FDE's start address | RELOC_OP_PREL -> Put init_module address (60) - current address (128) So, after relocation First FDE's start address has value 60 - 128 = -68 Now, while doing unwinding we Try to add this value to the sframe section header's start address which is in this example 100, so 100 + (-68) = 32 So, 32 is not within [60, 72], i.e. within init_module. You can see that it is possible for this value to be less than the start address of the module's memory region when this function's address is very close to the start of the memory region. The crux is that the offset in the FDE's start address is calculated based on the address of the FDE's start_address and not based on the address of the sframe section. Thanks, Puranjay
On 2/26/25 2:23 AM, Puranjay Mohan wrote: > Indu Bhagat <indu.bhagat@oracle.com> writes: > >> On 2/25/25 3:54 PM, Weinan Liu wrote: >>> On Tue, Feb 25, 2025 at 11:38 AM Indu Bhagat <indu.bhagat@oracle.com> wrote: >>>> >>>> On Mon, Feb 10, 2025 at 12:30 AM Weinan Liu <wnliu@google.com> wrote: >>>>>> I already have a WIP patch to add sframe support to the kernel module. >>>>>> However, it is not yet working. I had trouble unwinding frames for the >>>>>> kernel module using the current algorithm. >>>>>> >>>>>> Indu has likely identified the issue and will be addressing it from the >>>>>> toolchain side. >>>>>> >>>>>> https://sourceware.org/bugzilla/show_bug.cgi?id=32666 >>>>> >>>>> I have a working in progress patch that adds sframe support for kernel >>>>> module. >>>>> https://github.com/heuza/linux/tree/sframe_unwinder.rfc >>>>> >>>>> According to the sframe table values I got during runtime testing, looks >>>>> like the offsets are not correct . >>>>> >>>> >>>> I hope to sanitize the fix for 32666 and post upstream soon (I had to >>>> address other related issues). Unless fixed, relocating .sframe >>>> sections using the .rela.sframe is expected to generate incorrect output. >>>> >>>>> When unwind symbols init_module(0xffff80007b155048) from the kernel >>>>> module(livepatch-sample.ko), the start_address of the FDE entries in the >>>>> sframe table of the kernel modules appear incorrect. >>>> >>>> init_module will apply the relocations on the .sframe section, isnt it ? >>>> >>>>> For instance, the first FDE's start_addr is reported as -20564. Adding >>>>> this offset to the module's sframe section address (0xffff80007b15a040) >>>>> yields 0xffff80007b154fec, which is not within the livepatch-sample.ko >>>>> memory region(It should be larger than 0xffff80007b155000). >>>>> >>>> >>>> Hmm..something seems off here. Having tested a potential fix for 32666 >>>> locally, I do not expect the first FDE to show this symptom. >>>> >>> > > Hi, > > Sorry for not responding in the past few days. I was on PTO and was > trying to improve my snowboarding technique, I am back now!! > > I think what we are seeing is expected behaviour: > > | For instance, the first FDE's start_addr is reported as -20564. Adding > | this offset to the module's sframe section address (0xffff80007b15a040) > | yields 0xffff80007b154fec, which is not within the livepatch-sample.ko > | memory region(It should be larger than 0xffff80007b155000). > > > Let me explain using a __dummy__ example. > > Assume Memory layout before relocation: > > | Address | Element | Relocation > | .... | .... | > | 60 | init_module (start address) | > | 72 | init_module (end address) | > | .... | ..... | > | 100 | Sframe section header start address | > | 128 | First FDE's start address | RELOC_OP_PREL -> Put init_module address (60) - current address (128) > > So, after relocation First FDE's start address has value 60 - 128 = -68 > For SFrame FDE function start address is : "Signed 32-bit integral field denoting the virtual memory address of the described function, for which the SFrame FDE applies. The value encoded in the ‘sfde_func_start_address’ field is the offset in bytes of the function’s start address, from the SFrame section." So, in your case, after applying the relocations, you will get: S + A - P = 60 - 128 = -68 This is the distance of the function start address (60) from the current location in SFrame section (128) But what we intend to store is the distance of the function start address from the start of the SFrame section. So we need to do an additional step for SFrame FDE: Value += r_offset -68 + 28 = -40 Where 28 is the r_offset in the RELA. So we really expect a -40 in the relocated SFrame section instead of -68 above. IOW, the RELAs of SFrame section will need an additional step after relocation. > Now, while doing unwinding we Try to add this value to the sframe > section header's start address which is in this example 100, > > so 100 + (-68) = 32 > > So, 32 is not within [60, 72], i.e. within init_module. > > You can see that it is possible for this value to be less than the start > address of the module's memory region when this function's address is > very close to the start of the memory region. > > The crux is that the offset in the FDE's start address is calculated > based on the address of the FDE's start_address and not based on the > address of the sframe section. > > > Thanks, > Puranjay
Indu Bhagat <indu.bhagat@oracle.com> writes: > On 2/26/25 2:23 AM, Puranjay Mohan wrote: >> Indu Bhagat <indu.bhagat@oracle.com> writes: >> >>> On 2/25/25 3:54 PM, Weinan Liu wrote: >>>> On Tue, Feb 25, 2025 at 11:38 AM Indu Bhagat <indu.bhagat@oracle.com> wrote: >>>>> >>>>> On Mon, Feb 10, 2025 at 12:30 AM Weinan Liu <wnliu@google.com> wrote: >>>>>>> I already have a WIP patch to add sframe support to the kernel module. >>>>>>> However, it is not yet working. I had trouble unwinding frames for the >>>>>>> kernel module using the current algorithm. >>>>>>> >>>>>>> Indu has likely identified the issue and will be addressing it from the >>>>>>> toolchain side. >>>>>>> >>>>>>> https://sourceware.org/bugzilla/show_bug.cgi?id=32666 >>>>>> >>>>>> I have a working in progress patch that adds sframe support for kernel >>>>>> module. >>>>>> https://github.com/heuza/linux/tree/sframe_unwinder.rfc >>>>>> >>>>>> According to the sframe table values I got during runtime testing, looks >>>>>> like the offsets are not correct . >>>>>> >>>>> >>>>> I hope to sanitize the fix for 32666 and post upstream soon (I had to >>>>> address other related issues). Unless fixed, relocating .sframe >>>>> sections using the .rela.sframe is expected to generate incorrect output. >>>>> >>>>>> When unwind symbols init_module(0xffff80007b155048) from the kernel >>>>>> module(livepatch-sample.ko), the start_address of the FDE entries in the >>>>>> sframe table of the kernel modules appear incorrect. >>>>> >>>>> init_module will apply the relocations on the .sframe section, isnt it ? >>>>> >>>>>> For instance, the first FDE's start_addr is reported as -20564. Adding >>>>>> this offset to the module's sframe section address (0xffff80007b15a040) >>>>>> yields 0xffff80007b154fec, which is not within the livepatch-sample.ko >>>>>> memory region(It should be larger than 0xffff80007b155000). >>>>>> >>>>> >>>>> Hmm..something seems off here. Having tested a potential fix for 32666 >>>>> locally, I do not expect the first FDE to show this symptom. >>>>> >>>> >> >> Hi, >> >> Sorry for not responding in the past few days. I was on PTO and was >> trying to improve my snowboarding technique, I am back now!! >> >> I think what we are seeing is expected behaviour: >> >> | For instance, the first FDE's start_addr is reported as -20564. Adding >> | this offset to the module's sframe section address (0xffff80007b15a040) >> | yields 0xffff80007b154fec, which is not within the livepatch-sample.ko >> | memory region(It should be larger than 0xffff80007b155000). >> >> >> Let me explain using a __dummy__ example. >> >> Assume Memory layout before relocation: >> >> | Address | Element | Relocation >> | .... | .... | >> | 60 | init_module (start address) | >> | 72 | init_module (end address) | >> | .... | ..... | >> | 100 | Sframe section header start address | >> | 128 | First FDE's start address | RELOC_OP_PREL -> Put init_module address (60) - current address (128) >> >> So, after relocation First FDE's start address has value 60 - 128 = -68 >> > > For SFrame FDE function start address is : > > "Signed 32-bit integral field denoting the virtual memory address of the > described function, for which the SFrame FDE applies. The value encoded > in the ‘sfde_func_start_address’ field is the offset in bytes of the > function’s start address, from the SFrame section." > > So, in your case, after applying the relocations, you will get: > S + A - P = 60 - 128 = -68 > > This is the distance of the function start address (60) from the current > location in SFrame section (128) > > But what we intend to store is the distance of the function start > address from the start of the SFrame section. So we need to do an > additional step for SFrame FDE: Value += r_offset Thanks for the explaination, now it makes sense. But I couldn't find a relocation type in AARCH64 that does this extra += r_offset along with PREL32. The kernel's module loader is only doing the R_AARCH64_PREL32 which is why we see this issue. How is this working even for the kernel itself? or for that matter, any other binary compiled with sframe? From my limited undestanding, the way to fix this would be to hack the relocator to do this additional step while relocating .sframe sections. Or the 'addend' values in .rela.sframe should already have the +r_offset added to it, then no change to the relocator would be needed. > -68 + 28 = -40 > Where 28 is the r_offset in the RELA. > > So we really expect a -40 in the relocated SFrame section instead of -68 > above. IOW, the RELAs of SFrame section will need an additional step > after relocation. > Thanks, Puranjay
On 2/27/25 1:38 AM, Puranjay Mohan wrote: > Indu Bhagat <indu.bhagat@oracle.com> writes: > >> On 2/26/25 2:23 AM, Puranjay Mohan wrote: >>> Indu Bhagat <indu.bhagat@oracle.com> writes: >>> >>>> On 2/25/25 3:54 PM, Weinan Liu wrote: >>>>> On Tue, Feb 25, 2025 at 11:38 AM Indu Bhagat <indu.bhagat@oracle.com> wrote: >>>>>> >>>>>> On Mon, Feb 10, 2025 at 12:30 AM Weinan Liu <wnliu@google.com> wrote: >>>>>>>> I already have a WIP patch to add sframe support to the kernel module. >>>>>>>> However, it is not yet working. I had trouble unwinding frames for the >>>>>>>> kernel module using the current algorithm. >>>>>>>> >>>>>>>> Indu has likely identified the issue and will be addressing it from the >>>>>>>> toolchain side. >>>>>>>> >>>>>>>> https://sourceware.org/bugzilla/show_bug.cgi?id=32666 >>>>>>> >>>>>>> I have a working in progress patch that adds sframe support for kernel >>>>>>> module. >>>>>>> https://github.com/heuza/linux/tree/sframe_unwinder.rfc >>>>>>> >>>>>>> According to the sframe table values I got during runtime testing, looks >>>>>>> like the offsets are not correct . >>>>>>> >>>>>> >>>>>> I hope to sanitize the fix for 32666 and post upstream soon (I had to >>>>>> address other related issues). Unless fixed, relocating .sframe >>>>>> sections using the .rela.sframe is expected to generate incorrect output. >>>>>> >>>>>>> When unwind symbols init_module(0xffff80007b155048) from the kernel >>>>>>> module(livepatch-sample.ko), the start_address of the FDE entries in the >>>>>>> sframe table of the kernel modules appear incorrect. >>>>>> >>>>>> init_module will apply the relocations on the .sframe section, isnt it ? >>>>>> >>>>>>> For instance, the first FDE's start_addr is reported as -20564. Adding >>>>>>> this offset to the module's sframe section address (0xffff80007b15a040) >>>>>>> yields 0xffff80007b154fec, which is not within the livepatch-sample.ko >>>>>>> memory region(It should be larger than 0xffff80007b155000). >>>>>>> >>>>>> >>>>>> Hmm..something seems off here. Having tested a potential fix for 32666 >>>>>> locally, I do not expect the first FDE to show this symptom. >>>>>> >>>>> >>> >>> Hi, >>> >>> Sorry for not responding in the past few days. I was on PTO and was >>> trying to improve my snowboarding technique, I am back now!! >>> >>> I think what we are seeing is expected behaviour: >>> >>> | For instance, the first FDE's start_addr is reported as -20564. Adding >>> | this offset to the module's sframe section address (0xffff80007b15a040) >>> | yields 0xffff80007b154fec, which is not within the livepatch-sample.ko >>> | memory region(It should be larger than 0xffff80007b155000). >>> >>> >>> Let me explain using a __dummy__ example. >>> >>> Assume Memory layout before relocation: >>> >>> | Address | Element | Relocation >>> | .... | .... | >>> | 60 | init_module (start address) | >>> | 72 | init_module (end address) | >>> | .... | ..... | >>> | 100 | Sframe section header start address | >>> | 128 | First FDE's start address | RELOC_OP_PREL -> Put init_module address (60) - current address (128) >>> >>> So, after relocation First FDE's start address has value 60 - 128 = -68 >>> >> >> For SFrame FDE function start address is : >> >> "Signed 32-bit integral field denoting the virtual memory address of the >> described function, for which the SFrame FDE applies. The value encoded >> in the ‘sfde_func_start_address’ field is the offset in bytes of the >> function’s start address, from the SFrame section." >> >> So, in your case, after applying the relocations, you will get: >> S + A - P = 60 - 128 = -68 >> >> This is the distance of the function start address (60) from the current >> location in SFrame section (128) >> >> But what we intend to store is the distance of the function start >> address from the start of the SFrame section. So we need to do an >> additional step for SFrame FDE: Value += r_offset > > Thanks for the explaination, now it makes sense. > > But I couldn't find a relocation type in AARCH64 that does this extra += > r_offset along with PREL32. > > The kernel's module loader is only doing the R_AARCH64_PREL32 which is > why we see this issue. > > How is this working even for the kernel itself? or for that matter, any > other binary compiled with sframe? > For the usual executables or shared objects, the calculations are applied by ld.bfd at this time. Hence, the issue manifests in relocatable files. > From my limited undestanding, the way to fix this would be to hack the > relocator to do this additional step while relocating .sframe sections. > Or the 'addend' values in .rela.sframe should already have the +r_offset > added to it, then no change to the relocator would be needed. > Of the two, adjusting the addend values in .rela.sframe may be a reasonable way to go about it. Let me try it out in GAS and ld.bfd. >> -68 + 28 = -40 >> Where 28 is the r_offset in the RELA. >> >> So we really expect a -40 in the relocated SFrame section instead of -68 >> above. IOW, the RELAs of SFrame section will need an additional step >> after relocation. >> > > Thanks, > Puranjay
On 2/27/25 10:47 PM, Indu Bhagat wrote: > On 2/27/25 1:38 AM, Puranjay Mohan wrote: >> Indu Bhagat <indu.bhagat@oracle.com> writes: >> >>> On 2/26/25 2:23 AM, Puranjay Mohan wrote: >>>> Indu Bhagat <indu.bhagat@oracle.com> writes: >>>> >>>>> On 2/25/25 3:54 PM, Weinan Liu wrote: >>>>>> On Tue, Feb 25, 2025 at 11:38 AM Indu Bhagat >>>>>> <indu.bhagat@oracle.com> wrote: >>>>>>> >>>>>>> On Mon, Feb 10, 2025 at 12:30 AM Weinan Liu <wnliu@google.com> >>>>>>> wrote: >>>>>>>>> I already have a WIP patch to add sframe support to the kernel >>>>>>>>> module. >>>>>>>>> However, it is not yet working. I had trouble unwinding frames >>>>>>>>> for the >>>>>>>>> kernel module using the current algorithm. >>>>>>>>> >>>>>>>>> Indu has likely identified the issue and will be addressing it >>>>>>>>> from the >>>>>>>>> toolchain side. >>>>>>>>> >>>>>>>>> https://sourceware.org/bugzilla/show_bug.cgi?id=32666 >>>>>>>> >>>>>>>> I have a working in progress patch that adds sframe support for >>>>>>>> kernel >>>>>>>> module. >>>>>>>> https://github.com/heuza/linux/tree/sframe_unwinder.rfc >>>>>>>> >>>>>>>> According to the sframe table values I got during runtime >>>>>>>> testing, looks >>>>>>>> like the offsets are not correct . >>>>>>>> >>>>>>> >>>>>>> I hope to sanitize the fix for 32666 and post upstream soon (I >>>>>>> had to >>>>>>> address other related issues). Unless fixed, relocating .sframe >>>>>>> sections using the .rela.sframe is expected to generate incorrect >>>>>>> output. >>>>>>> >>>>>>>> When unwind symbols init_module(0xffff80007b155048) from the kernel >>>>>>>> module(livepatch-sample.ko), the start_address of the FDE >>>>>>>> entries in the >>>>>>>> sframe table of the kernel modules appear incorrect. >>>>>>> >>>>>>> init_module will apply the relocations on the .sframe section, >>>>>>> isnt it ? >>>>>>> >>>>>>>> For instance, the first FDE's start_addr is reported as -20564. >>>>>>>> Adding >>>>>>>> this offset to the module's sframe section address >>>>>>>> (0xffff80007b15a040) >>>>>>>> yields 0xffff80007b154fec, which is not within the livepatch- >>>>>>>> sample.ko >>>>>>>> memory region(It should be larger than 0xffff80007b155000). >>>>>>>> >>>>>>> >>>>>>> Hmm..something seems off here. Having tested a potential fix for >>>>>>> 32666 >>>>>>> locally, I do not expect the first FDE to show this symptom. >>>>>>> >>>>>> >>>> >>>> Hi, >>>> >>>> Sorry for not responding in the past few days. I was on PTO and was >>>> trying to improve my snowboarding technique, I am back now!! >>>> >>>> I think what we are seeing is expected behaviour: >>>> >>>> | For instance, the first FDE's start_addr is reported as -20564. >>>> Adding >>>> | this offset to the module's sframe section address >>>> (0xffff80007b15a040) >>>> | yields 0xffff80007b154fec, which is not within the livepatch- >>>> sample.ko >>>> | memory region(It should be larger than 0xffff80007b155000). >>>> >>>> >>>> Let me explain using a __dummy__ example. >>>> >>>> Assume Memory layout before relocation: >>>> >>>> | Address | Element | Relocation >>>> | .... | .... | >>>> | 60 | init_module (start address) | >>>> | 72 | init_module (end address) | >>>> | .... | ..... | >>>> | 100 | Sframe section header start address | >>>> | 128 | First FDE's start address | >>>> RELOC_OP_PREL -> Put init_module address (60) - current address (128) >>>> >>>> So, after relocation First FDE's start address has value 60 - 128 = -68 >>>> >>> >>> For SFrame FDE function start address is : >>> >>> "Signed 32-bit integral field denoting the virtual memory address of the >>> described function, for which the SFrame FDE applies. The value encoded >>> in the ‘sfde_func_start_address’ field is the offset in bytes of the >>> function’s start address, from the SFrame section." >>> >>> So, in your case, after applying the relocations, you will get: >>> S + A - P = 60 - 128 = -68 >>> >>> This is the distance of the function start address (60) from the current >>> location in SFrame section (128) >>> >>> But what we intend to store is the distance of the function start >>> address from the start of the SFrame section. So we need to do an >>> additional step for SFrame FDE: Value += r_offset >> >> Thanks for the explaination, now it makes sense. >> >> But I couldn't find a relocation type in AARCH64 that does this extra += >> r_offset along with PREL32. >> >> The kernel's module loader is only doing the R_AARCH64_PREL32 which is >> why we see this issue. >> >> How is this working even for the kernel itself? or for that matter, any >> other binary compiled with sframe? >> > > For the usual executables or shared objects, the calculations are > applied by ld.bfd at this time. Hence, the issue manifests in > relocatable files. > >> From my limited undestanding, the way to fix this would be to hack the >> relocator to do this additional step while relocating .sframe sections. >> Or the 'addend' values in .rela.sframe should already have the +r_offset >> added to it, then no change to the relocator would be needed. >> > > Of the two, adjusting the addend values in .rela.sframe may be a > reasonable way to go about it. Let me try it out in GAS and ld.bfd. > A fix for this is in the works (being discussed on the binutils@sourceware list). I will keep you posted. Thanks Indu
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/ Madhavan T. Venkataraman (1): arm64: Define TIF_PATCH_PENDING for livepatch Weinan Liu (7): unwind: build kernel with sframe info arm64: entry: add unwind info for various kernel entries unwind: add sframe v2 header unwind: Implement generic sframe unwinder library unwind: arm64: Add sframe unwinder on arm64 unwind: arm64: add reliable stacktrace support for arm64 arm64: Enable livepatch for ARM64 Makefile | 6 + arch/Kconfig | 8 + arch/arm64/Kconfig | 3 + arch/arm64/Kconfig.debug | 10 + arch/arm64/include/asm/stacktrace/common.h | 6 + arch/arm64/include/asm/thread_info.h | 4 +- arch/arm64/kernel/entry-common.c | 4 + arch/arm64/kernel/entry.S | 10 + arch/arm64/kernel/setup.c | 2 + arch/arm64/kernel/stacktrace.c | 102 ++++++++++ include/asm-generic/vmlinux.lds.h | 12 ++ include/linux/sframe_lookup.h | 43 +++++ kernel/Makefile | 1 + kernel/sframe.h | 215 +++++++++++++++++++++ kernel/sframe_lookup.c | 196 +++++++++++++++++++ 15 files changed, 621 insertions(+), 1 deletion(-) create mode 100644 include/linux/sframe_lookup.h create mode 100644 kernel/sframe.h create mode 100644 kernel/sframe_lookup.c