* Re: [PATCH v5 8/8] unwind: arm64: Use sframe to unwind interrupt frames
From: Mark Rutland @ 2026-05-05 10:29 UTC (permalink / raw)
To: Jens Remus
Cc: Dylan Hatch, Roman Gushchin, Weinan Liu, Will Deacon,
Josh Poimboeuf, Indu Bhagat, Peter Zijlstra, Steven Rostedt,
Catalin Marinas, Jiri Kosina, Prasanna Kumar T S M,
Puranjay Mohan, Song Liu, joe.lawrence, linux-toolchains,
linux-kernel, live-patching, linux-arm-kernel, Randy Dunlap,
Heiko Carstens
In-Reply-To: <bc3fb59b-9d80-4957-af51-20db38e3487e@linux.ibm.com>
On Mon, May 04, 2026 at 10:47:26AM +0200, Jens Remus wrote:
> Hello Mark,
>
> I mostly have comments regarding your the SFrame related remarks.
Thanks for this; I have a few more questions and comments below.
> On 5/1/2026 6:46 PM, Mark Rutland wrote:
> > Thanks for putting this together. I think this is looking pretty good.
> > However, there are some things that aren't quite right and need some
> > work, which I've commented on below.
>
> > (2) To make unwinding generally possible, we'll need to annotate some
> > assembly functions as unwindable. We'll need to do that for string
> > routines under lib/, and probably some crypto code, but we don't
> > need to do that for most code in head.S, entry.S, etc.
> >
> > The vast majority of relevant assembly functions are leaf functions
> > (where the return address is never moved out of the LR), so we can
> > probably get away with a simple annotation for those that avoids the
> > need for open-coded CFI directives everywhere.
>
> Wrapping them in .cfi_startproc ... .cfi_endproc should do. For instance
> by extending SYM_FUNC_START() and SYM_FUNC_END() or introducing flavors
> that do. Or where you thinking of something else?
I was expecting we'd do something like that, either with distinct
versions, or some entirely separate annotation.
We can't override SYM_FUNC_START() or SYM_FUNC_END() since those are
also used for non-leaf functions. The bulk of the work is going to be
making sure we only annotate leaf functions specifically, which will
require some human analysis.
> > On Tue, Apr 28, 2026 at 06:36:43PM +0000, Dylan Hatch wrote:
> >> diff --git a/arch/arm64/include/asm/stacktrace/common.h b/arch/arm64/include/asm/stacktrace/common.h
>
> >> @@ -21,6 +21,8 @@ struct stack_info {
> >> *
> >> * @fp: The fp value in the frame record (or the real fp)
> >> * @pc: The lr value in the frame record (or the real lr)
> >> + * @sp: The sp value at the call site of the current function.
> >> + * @unreliable: Stacktrace is unreliable.
> >> *
> >> * @stack: The stack currently being unwound.
> >> * @stacks: An array of stacks which can be unwound.
> >> @@ -29,7 +31,11 @@ struct stack_info {
> >> struct unwind_state {
> >> unsigned long fp;
> >> unsigned long pc;
> >> +#ifdef CONFIG_HAVE_UNWIND_KERNEL_SFRAME
> >> + unsigned long sp;
> >> +#endif
> >
> > As this is only used by the kernel unwinder (and not the hyp unwinder),
> > this should live in struct kunwind_state in stacktrace.c.
> >
> > That said, for unwinding across exception boundaries we should not need
> > this, as the SP value will be in the pt_regs. If we only use SFrame for
> > the exception boundary case, we can remove this entirely. I would
> > strongly prefer that we do that.
> >> + /* Get the Canonical Frame Address (CFA) */
> >> + switch (frame.cfa.rule) {
> >> + case UNWIND_CFA_RULE_SP_OFFSET:
> >> + cfa = state->common.sp;
>
> IIUC you suggest this to be changed as follows?
>
> cfa = regs->regs[31];
I was suggesting:
cfa = regs->sp;
Note: arm64's struct pt_regs has:
union {
struct user_pt_regs user_regs;
struct {
u64 regs[31];
u64 sp;
u64 pc;
u64 pstate;
};
};
... so regs->regs[31] would be an out-of-bounds array access.
[...]
> >> + case UNWIND_CFA_RULE_REG_OFFSET:
> >> + case UNWIND_CFA_RULE_REG_OFFSET_DEREF:
> >> + /* regs only available in topmost/interrupt frame */
> >> + if (!regs || frame.cfa.regnum > 30)
> >> + return -EINVAL;
> >> + cfa = regs->regs[frame.cfa.regnum];
> >> + break;
> >
> > Do we ever expect to see UNWIND_CFA_RULE_REG_OFFSET or
> > UNWIND_CFA_RULE_REG_OFFSET_DEREF in practice for kernel code?
>
> No. Those can only occur with SFrame V3 flexible FDE, which are
> currently not generated by GNU assembler for arm64/aarch64, and thus
> could be omitted in the arm64-specific kernel sframe unwinder:
>
> https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=gas/config/tc-aarch64.h;hb=binutils-2_46#l342
Ok.
Do we know whether there are currently cases on aarch64 that cannot be
encoded in SFrame (without flexible FDE), or whether SFrame without
flexible FDE is sufficient for arm64 as-is? ... or do we have
counter-examples today?
Looking at:
https://sourceware.org/binutils/docs/sframe-spec.html#Flexible-FDE-Type-Interpretation-1
For arm64 I'm not sure whether we'd encounter the DRAP or stack
realignment cases within the kernel (perhaps with SVE?), nor whether the
Register-based RA/FP Locations cases would apply if we assume that we
continue to use frame records.
[... ]
> I must admit that while reviewing I thought it would be future-proof to
> have support for rules that can only be represented with SFrame V3
> flexible FDE, even if they are currently not used on arm64. Ideally
> kunwind_next_sframe() could be made common, if another architecture
> would implement kernel unwinding using sframe.
While I understand that principle, I think that for now it would be
better to keep this arch-specific and minimal:
* We have arch-specific concerns (e.g. the FRAME_META_TYPE_FINAL
frames), and factoring that into generic code is going to be painful
to adapt (which we're likely to need to do in the near future), and to
maintain going forwards. Keeping that arch-specific for now will make
it easier/quicker to get to a stable state.
* Code which isn't used is liable to be wrong or made wrong by accident.
For example, with all the SP cases I mentioned in my initial reply.
We can certainly look at making that more generic in future, but for now
I'd prefer to omit the code that cannot be used (and have some sort of
build or boot/module-load time check that SFrame only has elements that
we expect), and make sure that we thoroughly test the cases that do
exist in practice.
Do we expect SFrame V3 flexible FDE to be generated by toolchains in the
near future?
[...]
> >> + /* CFA alignment 8 bytes */
> >> + if (cfa & 0x7)
> >> + return -EINVAL;
> >
> > If the CFA is the SP upon entry to the function, then per AAPCS64 rules
> > it should be aligned to 16 bytes. Otherwise, where has this 8 byte
> > alignment requirement come from? Does SFrame mandate that?
>
> That originates from the common unwind user logic (see
> kernel/unwind/user.c, unwind_user_next_common()), which currently
> assumes 8-byte/4-byte SP alignment for all 64-bit/32-bit architectures.
>
> So checking for 16-byte alignment here would make sense.
Just to confirm, am I correct to understand that the SFrame definition
of CFA is intended to be the same as the DWARF definition of CFA, and so
for arm64 the CFA is the SP when the function is called?
That's the case for DWARF on arm64:
https://github.com/ARM-software/abi-aa/releases/download/2025Q4/aadwarf64.pdf
https://github.com/ARM-software/abi-aa/blob/daa7a94ca55973736c0e434a67a6e4bbcd35d7fa/aadwarf64/aadwarf64.rst
| The CFA is the value of the stack pointer (sp) at the call site in the
| previous frame.
I couldn't find an explciit statement to that effect in:
https://sourceware.org/binutils/docs/sframe-spec.html
... but I guess that is implied, given the other bits inherited from
DWARF.
I see that the documented behaviour for CFA on AMD64 and s390x are
consistent with their DWARF behaviour.
> >> +
> >> + /* Get the Return Address (RA) */
> >> + switch (frame.ra.rule) {
> >> + case UNWIND_RULE_RETAIN:
> >> + /* regs only available in topmost/interrupt frame */
> >> + if (!regs)
> >> + return -EINVAL;
> >> + ra = regs->regs[30];
> >> + source = KUNWIND_SOURCE_REGS_LR;
> >> + break;
> >> + /* UNWIND_USER_RULE_CFA_OFFSET not implemented on purpose */
>
> Nit: s/UNWIND_USER_RULE_CFA_OFFSET/UNWIND_RULE_CFA_OFFSET/
>
> >
> > It would be better for the comment to say *why* that's not implemented.
> >
> > I assume that's because UNWIND_USER_RULE_CFA_OFFSET would mean that the return
> > address is a stack address, and that's obviously not legitimate.
>
> That and SFrame V3 currently cannot represent FP/RA as CFA + offset
> (i.e. UNWIND_RULE_CFA_OFFSET; .cfi_val_offset FP/RA).
>
> The comment originates from the common unwind user logic (see
> kernel/unwind/user.c). I am open to improve that. What about:
>
> /*
> * UNWIND_RULE_CFA_OFFSET not implemented on purpose, as a stack
> * address cannot be a legitimate return address value. It is
> * also not used (e.g. not represented in sframe).
> */
I'd go with something simpler, e.g.
/*
* UNWIND_RULE_CFA_OFFSET doesn't make sense for RA.
* The return address cannot legitimately be a stack addres.
*/
[...]
> >
> > I don't think we expect UNWIND_RULE_REG_OFFSET unless that's sometimes used
> > instead of UNWIND_RULE_RETAIN to express that the return address is in x30
> > (with zero offset).
>
> No. Unless there would be nonsense .cfi_register 30, 30, which would
> require SFrame V3 flexible FDE to be represented.
Ok.
> @Indu: We may consider to treat .cfi_register <reg>, <reg> (for FP/RA)
> like .cfi_restore <reg> in the GNU assembler?
>
> > Similarly, if the address is on the stack it should be in a frame
> > record. Would we ever expect UNWIND_RULE_REG_OFFSET_DEREF rather than
> > UNWIND_RULE_CFA_OFFSET_DEREF?
>
> No. See above (SFrame V3 flexible FDE).
Ok.
> >> + default:
> >> + WARN_ON_ONCE(1);
> >> + return -EINVAL;
> >> + }
> >> +
> >> + /* Get the Frame Pointer (FP) */
> >> + switch (frame.fp.rule) {
> >> + case UNWIND_RULE_RETAIN:
> >> + fp = state->common.fp;
> >> + break;
> >> + /* UNWIND_USER_RULE_CFA_OFFSET not implemented on purpose */
> >
> > As for RA, the comment should explain why that's not implemented.
>
> I am open to improve the comment in the the common unwind user logic.
> What about:
>
> /*
> * UNWIND_RULE_CFA_OFFSET not implemented on purpose, as it is
> * not used (e.g. not represented in sframe).
> */
For me, this wording raises more questions, e.g.
* Does 'not used' mean that toolchains don't use that, or that the spec
doesn't permit that?
* Does 'not represented' mean that this is not represntable, or that
toolchains currently don't generate SFrame with the appropriate
elements.
IIUC you're saying that this *is* representable with flexible FDE, but
current toolchains don't generate that.
Mark.
^ permalink raw reply
* Re: [PATCH v2 24/53] klp-build: Fix checksum comparison for changed offsets
From: Miroslav Benes @ 2026-05-05 10:05 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: x86, linux-kernel, live-patching, Peter Zijlstra, Joe Lawrence,
Song Liu, Miroslav Benes, Petr Mladek
In-Reply-To: <6392d4f0c8837ccc0498a1c79a2d9534dacfce82.1777575752.git.jpoimboe@kernel.org>
On Thu, 30 Apr 2026 21:08:12 -0700, Josh Poimboeuf <jpoimboe@kernel.org> wrote:
> The klp-build -f/--show-first-changed feature uses diff to compare
> checksum log lines between original and patched objects. However, diff
> compares entire lines, including the offset field. When a function is
> at a different section offset, the offset field differs even though the
> instruction checksum is identical, causing the wrong instruction to be
> printed.
>
> [...]
Reviewed-by: Miroslav Benes <mbenes@suse.cz>
--
Miroslav
^ permalink raw reply
* Re: [PATCH v2 23/53] klp-build: Fix hang on out-of-date .config
From: Miroslav Benes @ 2026-05-05 9:44 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: x86, linux-kernel, live-patching, Peter Zijlstra, Joe Lawrence,
Song Liu, Miroslav Benes, Petr Mladek
In-Reply-To: <5e2a75b8ce5120bbbec6c8e992f1d3c772b8e5d5.1777575752.git.jpoimboe@kernel.org>
On Thu, 30 Apr 2026 21:08:11 -0700, Josh Poimboeuf <jpoimboe@kernel.org> wrote:
> If .config is out of date with the kernel source, 'make syncconfig'
> hangs while waiting for user input on new config options. Detect the
> mismatch and return an error.
Reviewed-by: Miroslav Benes <mbenes@suse.cz>
--
Miroslav
^ permalink raw reply
* Re: [PATCH v2 21/53] objtool/klp: Fix reloc corruption in convert_reloc_sym_to_secsym()
From: Miroslav Benes @ 2026-05-05 9:44 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: x86, linux-kernel, live-patching, Peter Zijlstra, Joe Lawrence,
Song Liu, Miroslav Benes, Petr Mladek
In-Reply-To: <9b419d82a20dbc54be4a59cfec04ab13987a2e6c.1777575752.git.jpoimboe@kernel.org>
On Thu, 30 Apr 2026 21:08:09 -0700, Josh Poimboeuf <jpoimboe@kernel.org> wrote:
> Use the section symbol's index instead of the old symbol's index when
> updating the ELF relocation entry in convert_reloc_sym_to_secsym().
>
> Found by Sashiko review.
Reviewed-by: Miroslav Benes <mbenes@suse.cz>
--
Miroslav
^ permalink raw reply
* Re: [PATCH v2 20/53] objtool/klp: Don't correlate .rodata.cst* constant pool objects
From: Miroslav Benes @ 2026-05-05 9:44 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: x86, linux-kernel, live-patching, Peter Zijlstra, Joe Lawrence,
Song Liu, Miroslav Benes, Petr Mladek
In-Reply-To: <80d6f8df4db610a6c9f68031dc0153f04814f2fa.1777575752.git.jpoimboe@kernel.org>
On Thu, 30 Apr 2026 21:08:08 -0700, Josh Poimboeuf <jpoimboe@kernel.org> wrote:
> Clang aggregates UBSAN type descriptors into shared anonymous
> .data..L__unnamed_* sections. This data is used by UBSAN trap handlers.
>
> When a changed function has an UBSAN bounds check, klp-diff clones the
> entire UBSAN data section associated with the TU. Relocations within
> the cloned section that reference named rodata objects in .rodata.cst*
> (like 'exponent', 'pirq_ali_set.irqmap') become KLP relocations because
> those objects now get correlated.
>
> [...]
Reviewed-by: Miroslav Benes <mbenes@suse.cz>
--
Miroslav
^ permalink raw reply
* Re: [PATCH v2 19/53] objtool/klp: Fix pointer comparisons for rodata objects
From: Miroslav Benes @ 2026-05-05 9:44 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: x86, linux-kernel, live-patching, Peter Zijlstra, Joe Lawrence,
Song Liu, Miroslav Benes, Petr Mladek
In-Reply-To: <07de8098fd8981321baab0ff552f65aa2cfc31ec.1777575752.git.jpoimboe@kernel.org>
On Thu, 30 Apr 2026 21:08:07 -0700, Josh Poimboeuf <jpoimboe@kernel.org> wrote:
> klp-diff treats all rodata as uncorrelated, so any reference to it uses
> a duplicated copy rather than using a KLP reloc.
>
> For the contents of the data itself, a duplicated copy is fine.
> However, pointer comparisons (e.g., f->f_op == &foo_ops) are broken.
>
> Fix it by correlating non-anonymous rodata objects.
>
> [...]
Reviewed-by: Miroslav Benes <mbenes@suse.cz>
--
Miroslav
^ permalink raw reply
* Re: [PATCH v5 0/6] kselftests: livepatch: Adapt tests to be executed on 4.12 kernels
From: Joe Lawrence @ 2026-05-04 20:43 UTC (permalink / raw)
To: Marcos Paulo de Souza, Josh Poimboeuf, Jiri Kosina,
Miroslav Benes, Petr Mladek, Shuah Khan
Cc: live-patching, linux-kselftest, linux-kernel, marcos
In-Reply-To: <20260504-lp-tests-old-fixes-v5-0-0be26d94ab9a@suse.com>
On 5/4/26 2:34 PM, Marcos Paulo de Souza wrote:
> This is the fifth version of the patchset, which fixes the last Sashiko
> comment, about overwriting MOD_LIVEPATCH global variable and using a
> local variable to avoid module load clashing when an older kernel
> don't support some sysfs attributes.
>
> Original cover-letter:
> These patches don't really change how the patches are run, just skip
> some tests on kernels that don't support a feature (like kprobe and
> livepatched living together) or when a livepatch sysfs attribute is
> missing.
>
> These patches are based on printk/for-next branch.
>
> Please review! Thanks!
>
> Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
> ---
> Changes in v5:
> - Edit the last three patches to avoid overwriting MOD_LIVEPATCH
> variable, using a local variable. This fixed the last Sashiko report.
> - Link to v4: https://patch.msgid.link/20260429-lp-tests-old-fixes-v4-0-59b9741989d0@suse.com
>
> Changes in v4:
> - Patch 5 was changed in order to address a comment made by Sashiko, where
> subsequent tests rewrite the variables that contain the modules being loaded.
> - Link to v3: https://patch.msgid.link/20260427-lp-tests-old-fixes-v3-0-ccf3c90f744c@suse.com
>
> Changes in v3:
> - Patch 1 was changed to reorganize the ifdeffery to handle multiple archs syscall wrapper (Miroslav)
> - Patch 3 was changed to rework the commit message and to address function naming (Joe)
> - Patches 4, 5 and 6 where had the commit messages to include the kernel version where
> the given sysfs attributes were included (Petr Mladek)
> - Link to v2: https://patch.msgid.link/20260413-lp-tests-old-fixes-v2-0-367c7cb5006f@suse.com
>
> Changes in v2:
> - Patch descriptions were changed to remove "test-X", since it was polluting the commit subjects (Miroslav Benes)
> - Patch 8 was dropped since it was checking for a message from an out-of-tree patch. (Petr Mladek)
> - Patch 3 was dropped as should be treated as expected failure for older kernels. (Petr Mladek)
> - Patch 2 was changed to use y/n instead of 1/0, since it's more natural to use it.
> - Patch 1 was changed to handle ppc and loongson, and error out if dealing with a different architecture that sets
> CONFIG_ARCH_HAS_SYSCALL_WRAPPER and haven't changed the test to include the proper wrapper prefix.
> - Patch 4 was changed to invert the return of the bash function to return 1 in failure, like
> a normal bash function (Joe Lawrence)
> - Patches 5, 6 an 7 were changed to not split the tests, but to only run the tests
> when the attribute were present (Miroslav Benes)
> - Link to v1: https://patch.msgid.link/20260313-lp-tests-old-fixes-v1-0-71ac6dfb3253@suse.com
>
> ---
> Marcos Paulo de Souza (6):
> selftests: livepatch: Check for ARCH_HAS_SYSCALL_WRAPPER config
> selftests: livepatch: Replace true/false module parameter by y/n
> selftests: livepatch: Introduce does_sysfs_exist function
> selftests: livepatch: Check if patched sysfs attribute exists
> selftests: livepatch: Check if replace sysfs attribute exists
> selftests: livepatch: Check if stack_order sysfs attribute exists
>
> tools/testing/selftests/livepatch/functions.sh | 10 +
> tools/testing/selftests/livepatch/test-kprobe.sh | 8 +-
> tools/testing/selftests/livepatch/test-sysfs.sh | 219 +++++++++++----------
> .../livepatch/test_modules/test_klp_syscall.c | 27 ++-
> 4 files changed, 153 insertions(+), 111 deletions(-)
> ---
> base-commit: 712c0756828becbfc629ff8d8b82deff5d1115e4
> change-id: 20260309-lp-tests-old-fixes-f955abc8ec27
>
Acked-by: Joe Lawrence <joe.lawrence@redhat.com>
FWIW, I tried this out on 4.18.0-372.137.1.el8_6.x86_64, the oldest
kernel we're supporting for livepatching at the momement, and the modern
selftests + Marcos's patchset happily ran without any problems.
--
Joe
^ permalink raw reply
* [PATCH v5 6/6] selftests: livepatch: Check if stack_order sysfs attribute exists
From: Marcos Paulo de Souza @ 2026-05-04 18:34 UTC (permalink / raw)
To: Josh Poimboeuf, Jiri Kosina, Miroslav Benes, Petr Mladek,
Joe Lawrence, Shuah Khan
Cc: live-patching, linux-kselftest, linux-kernel,
Marcos Paulo de Souza, marcos
In-Reply-To: <20260504-lp-tests-old-fixes-v5-0-0be26d94ab9a@suse.com>
The commit 3dae09de4061 ("livepatch: Add stack_order sysfs attribute"),
merged in v6.14, introduced a new sysfs attribute.
In order to run the selftests on older kernels, check if given kernel
has support for the attribute. If the attribute is not supported, skip
the checks.
Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
---
tools/testing/selftests/livepatch/test-sysfs.sh | 43 ++++++++++++++-----------
1 file changed, 25 insertions(+), 18 deletions(-)
diff --git a/tools/testing/selftests/livepatch/test-sysfs.sh b/tools/testing/selftests/livepatch/test-sysfs.sh
index fed656021271..3b16285c6e67 100755
--- a/tools/testing/selftests/livepatch/test-sysfs.sh
+++ b/tools/testing/selftests/livepatch/test-sysfs.sh
@@ -10,6 +10,7 @@ MOD_LIVEPATCH3=test_klp_syscall
HAS_PATCH_ATTR=0
HAS_REPLACE_ATTR=0
+HAS_STACK_ORDER_ATTR=0
setup_config
@@ -23,8 +24,6 @@ check_sysfs_rights "$MOD_LIVEPATCH" "" "drwxr-xr-x"
check_sysfs_rights "$MOD_LIVEPATCH" "enabled" "-rw-r--r--"
check_sysfs_value "$MOD_LIVEPATCH" "enabled" "1"
check_sysfs_rights "$MOD_LIVEPATCH" "force" "--w-------"
-check_sysfs_rights "$MOD_LIVEPATCH" "stack_order" "-r--r--r--"
-check_sysfs_value "$MOD_LIVEPATCH" "stack_order" "1"
check_sysfs_rights "$MOD_LIVEPATCH" "transition" "-r--r--r--"
check_sysfs_value "$MOD_LIVEPATCH" "transition" "0"
@@ -39,6 +38,12 @@ if does_sysfs_exist "$MOD_LIVEPATCH" "replace"; then
HAS_REPLACE_ATTR=1
fi
+if does_sysfs_exist "$MOD_LIVEPATCH" "stack_order"; then
+ check_sysfs_rights "$MOD_LIVEPATCH" "stack_order" "-r--r--r--"
+ check_sysfs_value "$MOD_LIVEPATCH" "stack_order" "1"
+ HAS_STACK_ORDER_ATTR=1
+fi
+
disable_lp $MOD_LIVEPATCH
unload_lp $MOD_LIVEPATCH
@@ -149,33 +154,34 @@ livepatch: '$MOD_ATOMIC_REPLACE': unpatching complete
% rmmod $MOD_ATOMIC_REPLACE"
fi
-start_test "sysfs test stack_order value"
+if [[ "$HAS_STACK_ORDER_ATTR" == "1" ]]; then
+ start_test "sysfs test stack_order value"
-load_lp $MOD_LIVEPATCH
+ load_lp $MOD_LIVEPATCH
-check_sysfs_value "$MOD_LIVEPATCH" "stack_order" "1"
+ check_sysfs_value "$MOD_LIVEPATCH" "stack_order" "1"
-load_lp $MOD_LIVEPATCH2
+ load_lp $MOD_LIVEPATCH2
-check_sysfs_value "$MOD_LIVEPATCH2" "stack_order" "2"
+ check_sysfs_value "$MOD_LIVEPATCH2" "stack_order" "2"
-load_lp $MOD_LIVEPATCH3
+ load_lp $MOD_LIVEPATCH3
-check_sysfs_value "$MOD_LIVEPATCH3" "stack_order" "3"
+ check_sysfs_value "$MOD_LIVEPATCH3" "stack_order" "3"
-disable_lp $MOD_LIVEPATCH2
-unload_lp $MOD_LIVEPATCH2
+ disable_lp $MOD_LIVEPATCH2
+ unload_lp $MOD_LIVEPATCH2
-check_sysfs_value "$MOD_LIVEPATCH" "stack_order" "1"
-check_sysfs_value "$MOD_LIVEPATCH3" "stack_order" "2"
+ check_sysfs_value "$MOD_LIVEPATCH" "stack_order" "1"
+ check_sysfs_value "$MOD_LIVEPATCH3" "stack_order" "2"
-disable_lp $MOD_LIVEPATCH3
-unload_lp $MOD_LIVEPATCH3
+ disable_lp $MOD_LIVEPATCH3
+ unload_lp $MOD_LIVEPATCH3
-disable_lp $MOD_LIVEPATCH
-unload_lp $MOD_LIVEPATCH
+ disable_lp $MOD_LIVEPATCH
+ unload_lp $MOD_LIVEPATCH
-check_result "% insmod test_modules/$MOD_LIVEPATCH.ko
+ check_result "% insmod test_modules/$MOD_LIVEPATCH.ko
livepatch: enabling patch '$MOD_LIVEPATCH'
livepatch: '$MOD_LIVEPATCH': initializing patching transition
livepatch: '$MOD_LIVEPATCH': starting patching transition
@@ -215,5 +221,6 @@ livepatch: '$MOD_LIVEPATCH': starting unpatching transition
livepatch: '$MOD_LIVEPATCH': completing unpatching transition
livepatch: '$MOD_LIVEPATCH': unpatching complete
% rmmod $MOD_LIVEPATCH"
+fi
exit 0
--
2.54.0
^ permalink raw reply related
* [PATCH v5 5/6] selftests: livepatch: Check if replace sysfs attribute exists
From: Marcos Paulo de Souza @ 2026-05-04 18:34 UTC (permalink / raw)
To: Josh Poimboeuf, Jiri Kosina, Miroslav Benes, Petr Mladek,
Joe Lawrence, Shuah Khan
Cc: live-patching, linux-kselftest, linux-kernel,
Marcos Paulo de Souza, marcos
In-Reply-To: <20260504-lp-tests-old-fixes-v5-0-0be26d94ab9a@suse.com>
The commit adb68ed26a3e ("livepatch: Add "replace" sysfs attribute"),
merged in v6.11, introduced a new sysfs attribute.
In order to run the selftests on older kernels, check if given kernel
has support for the attribute. If the attribute is not supported, skip
the checks.
While at it, create a local variable to hold the module name to be
tested, instead of overwriting MOD_LIVEPATCH.
Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
---
tools/testing/selftests/livepatch/test-sysfs.sh | 101 +++++++++++++-----------
1 file changed, 54 insertions(+), 47 deletions(-)
diff --git a/tools/testing/selftests/livepatch/test-sysfs.sh b/tools/testing/selftests/livepatch/test-sysfs.sh
index 7fa2223f9533..fed656021271 100755
--- a/tools/testing/selftests/livepatch/test-sysfs.sh
+++ b/tools/testing/selftests/livepatch/test-sysfs.sh
@@ -9,6 +9,7 @@ MOD_LIVEPATCH2=test_klp_callbacks_demo
MOD_LIVEPATCH3=test_klp_syscall
HAS_PATCH_ATTR=0
+HAS_REPLACE_ATTR=0
setup_config
@@ -22,7 +23,6 @@ check_sysfs_rights "$MOD_LIVEPATCH" "" "drwxr-xr-x"
check_sysfs_rights "$MOD_LIVEPATCH" "enabled" "-rw-r--r--"
check_sysfs_value "$MOD_LIVEPATCH" "enabled" "1"
check_sysfs_rights "$MOD_LIVEPATCH" "force" "--w-------"
-check_sysfs_rights "$MOD_LIVEPATCH" "replace" "-r--r--r--"
check_sysfs_rights "$MOD_LIVEPATCH" "stack_order" "-r--r--r--"
check_sysfs_value "$MOD_LIVEPATCH" "stack_order" "1"
check_sysfs_rights "$MOD_LIVEPATCH" "transition" "-r--r--r--"
@@ -34,6 +34,11 @@ if does_sysfs_exist "$MOD_LIVEPATCH/vmlinux" "patched"; then
HAS_PATCH_ATTR=1
fi
+if does_sysfs_exist "$MOD_LIVEPATCH" "replace"; then
+ check_sysfs_rights "$MOD_LIVEPATCH" "replace" "-r--r--r--"
+ HAS_REPLACE_ATTR=1
+fi
+
disable_lp $MOD_LIVEPATCH
unload_lp $MOD_LIVEPATCH
@@ -95,52 +100,54 @@ livepatch: '$MOD_LIVEPATCH2': unpatching complete
% rmmod $MOD_LIVEPATCH2"
fi
-start_test "sysfs test replace enabled"
-
-MOD_LIVEPATCH=test_klp_atomic_replace
-load_lp $MOD_LIVEPATCH replace=1
-
-check_sysfs_rights "$MOD_LIVEPATCH" "replace" "-r--r--r--"
-check_sysfs_value "$MOD_LIVEPATCH" "replace" "1"
-
-disable_lp $MOD_LIVEPATCH
-unload_lp $MOD_LIVEPATCH
-
-check_result "% insmod test_modules/$MOD_LIVEPATCH.ko replace=1
-livepatch: enabling patch '$MOD_LIVEPATCH'
-livepatch: '$MOD_LIVEPATCH': initializing patching transition
-livepatch: '$MOD_LIVEPATCH': starting patching transition
-livepatch: '$MOD_LIVEPATCH': completing patching transition
-livepatch: '$MOD_LIVEPATCH': patching complete
-% echo 0 > $SYSFS_KLP_DIR/$MOD_LIVEPATCH/enabled
-livepatch: '$MOD_LIVEPATCH': initializing unpatching transition
-livepatch: '$MOD_LIVEPATCH': starting unpatching transition
-livepatch: '$MOD_LIVEPATCH': completing unpatching transition
-livepatch: '$MOD_LIVEPATCH': unpatching complete
-% rmmod $MOD_LIVEPATCH"
-
-start_test "sysfs test replace disabled"
-
-load_lp $MOD_LIVEPATCH replace=0
-
-check_sysfs_rights "$MOD_LIVEPATCH" "replace" "-r--r--r--"
-check_sysfs_value "$MOD_LIVEPATCH" "replace" "0"
-
-disable_lp $MOD_LIVEPATCH
-unload_lp $MOD_LIVEPATCH
-
-check_result "% insmod test_modules/$MOD_LIVEPATCH.ko replace=0
-livepatch: enabling patch '$MOD_LIVEPATCH'
-livepatch: '$MOD_LIVEPATCH': initializing patching transition
-livepatch: '$MOD_LIVEPATCH': starting patching transition
-livepatch: '$MOD_LIVEPATCH': completing patching transition
-livepatch: '$MOD_LIVEPATCH': patching complete
-% echo 0 > $SYSFS_KLP_DIR/$MOD_LIVEPATCH/enabled
-livepatch: '$MOD_LIVEPATCH': initializing unpatching transition
-livepatch: '$MOD_LIVEPATCH': starting unpatching transition
-livepatch: '$MOD_LIVEPATCH': completing unpatching transition
-livepatch: '$MOD_LIVEPATCH': unpatching complete
-% rmmod $MOD_LIVEPATCH"
+if [[ "$HAS_REPLACE_ATTR" == "1" ]]; then
+ start_test "sysfs test replace enabled"
+
+ MOD_ATOMIC_REPLACE=test_klp_atomic_replace
+ load_lp $MOD_ATOMIC_REPLACE replace=1
+
+ check_sysfs_rights "$MOD_ATOMIC_REPLACE" "replace" "-r--r--r--"
+ check_sysfs_value "$MOD_ATOMIC_REPLACE" "replace" "1"
+
+ disable_lp $MOD_ATOMIC_REPLACE
+ unload_lp $MOD_ATOMIC_REPLACE
+
+ check_result "% insmod test_modules/$MOD_ATOMIC_REPLACE.ko replace=1
+livepatch: enabling patch '$MOD_ATOMIC_REPLACE'
+livepatch: '$MOD_ATOMIC_REPLACE': initializing patching transition
+livepatch: '$MOD_ATOMIC_REPLACE': starting patching transition
+livepatch: '$MOD_ATOMIC_REPLACE': completing patching transition
+livepatch: '$MOD_ATOMIC_REPLACE': patching complete
+% echo 0 > $SYSFS_KLP_DIR/$MOD_ATOMIC_REPLACE/enabled
+livepatch: '$MOD_ATOMIC_REPLACE': initializing unpatching transition
+livepatch: '$MOD_ATOMIC_REPLACE': starting unpatching transition
+livepatch: '$MOD_ATOMIC_REPLACE': completing unpatching transition
+livepatch: '$MOD_ATOMIC_REPLACE': unpatching complete
+% rmmod $MOD_ATOMIC_REPLACE"
+
+ start_test "sysfs test replace disabled"
+
+ load_lp $MOD_ATOMIC_REPLACE replace=0
+
+ check_sysfs_rights "$MOD_ATOMIC_REPLACE" "replace" "-r--r--r--"
+ check_sysfs_value "$MOD_ATOMIC_REPLACE" "replace" "0"
+
+ disable_lp $MOD_ATOMIC_REPLACE
+ unload_lp $MOD_ATOMIC_REPLACE
+
+ check_result "% insmod test_modules/$MOD_ATOMIC_REPLACE.ko replace=0
+livepatch: enabling patch '$MOD_ATOMIC_REPLACE'
+livepatch: '$MOD_ATOMIC_REPLACE': initializing patching transition
+livepatch: '$MOD_ATOMIC_REPLACE': starting patching transition
+livepatch: '$MOD_ATOMIC_REPLACE': completing patching transition
+livepatch: '$MOD_ATOMIC_REPLACE': patching complete
+% echo 0 > $SYSFS_KLP_DIR/$MOD_ATOMIC_REPLACE/enabled
+livepatch: '$MOD_ATOMIC_REPLACE': initializing unpatching transition
+livepatch: '$MOD_ATOMIC_REPLACE': starting unpatching transition
+livepatch: '$MOD_ATOMIC_REPLACE': completing unpatching transition
+livepatch: '$MOD_ATOMIC_REPLACE': unpatching complete
+% rmmod $MOD_ATOMIC_REPLACE"
+fi
start_test "sysfs test stack_order value"
--
2.54.0
^ permalink raw reply related
* [PATCH v5 4/6] selftests: livepatch: Check if patched sysfs attribute exists
From: Marcos Paulo de Souza @ 2026-05-04 18:34 UTC (permalink / raw)
To: Josh Poimboeuf, Jiri Kosina, Miroslav Benes, Petr Mladek,
Joe Lawrence, Shuah Khan
Cc: live-patching, linux-kselftest, linux-kernel,
Marcos Paulo de Souza, marcos
In-Reply-To: <20260504-lp-tests-old-fixes-v5-0-0be26d94ab9a@suse.com>
The commit bb26cfd9e77e
("livepatch: add sysfs entry "patched" for each klp_object") was merged
in v6.1, introducing a new sysfs attribute.
In order to run the selftests on older kernels, check if given kernel
has support for the attribute. If the attribute is not supported, skip
the checks.
Along with this change, use MOD_LIVEPATCH2 variable instead of
reassigning a new value to MOD_LIVEPATCH, and also use the variable
names in the check_result, to avoid using the module names.
Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
---
tools/testing/selftests/livepatch/test-sysfs.sh | 87 +++++++++++++------------
1 file changed, 47 insertions(+), 40 deletions(-)
diff --git a/tools/testing/selftests/livepatch/test-sysfs.sh b/tools/testing/selftests/livepatch/test-sysfs.sh
index 58fe1d96997c..7fa2223f9533 100755
--- a/tools/testing/selftests/livepatch/test-sysfs.sh
+++ b/tools/testing/selftests/livepatch/test-sysfs.sh
@@ -8,6 +8,8 @@ MOD_LIVEPATCH=test_klp_livepatch
MOD_LIVEPATCH2=test_klp_callbacks_demo
MOD_LIVEPATCH3=test_klp_syscall
+HAS_PATCH_ATTR=0
+
setup_config
# - load a livepatch and verifies the sysfs entries work as expected
@@ -25,8 +27,12 @@ check_sysfs_rights "$MOD_LIVEPATCH" "stack_order" "-r--r--r--"
check_sysfs_value "$MOD_LIVEPATCH" "stack_order" "1"
check_sysfs_rights "$MOD_LIVEPATCH" "transition" "-r--r--r--"
check_sysfs_value "$MOD_LIVEPATCH" "transition" "0"
-check_sysfs_rights "$MOD_LIVEPATCH" "vmlinux/patched" "-r--r--r--"
-check_sysfs_value "$MOD_LIVEPATCH" "vmlinux/patched" "1"
+
+if does_sysfs_exist "$MOD_LIVEPATCH/vmlinux" "patched"; then
+ check_sysfs_rights "$MOD_LIVEPATCH" "vmlinux/patched" "-r--r--r--"
+ check_sysfs_value "$MOD_LIVEPATCH" "vmlinux/patched" "1"
+ HAS_PATCH_ATTR=1
+fi
disable_lp $MOD_LIVEPATCH
@@ -45,48 +51,49 @@ livepatch: '$MOD_LIVEPATCH': completing unpatching transition
livepatch: '$MOD_LIVEPATCH': unpatching complete
% rmmod $MOD_LIVEPATCH"
-start_test "sysfs test object/patched"
+if [[ "$HAS_PATCH_ATTR" == "1" ]]; then
+ start_test "sysfs test object/patched"
-MOD_LIVEPATCH=test_klp_callbacks_demo
-MOD_TARGET=test_klp_callbacks_mod
-load_lp $MOD_LIVEPATCH
+ MOD_TARGET=test_klp_callbacks_mod
+ load_lp $MOD_LIVEPATCH2
-# check the "patch" file changes as target module loads/unloads
-check_sysfs_value "$MOD_LIVEPATCH" "$MOD_TARGET/patched" "0"
-load_mod $MOD_TARGET
-check_sysfs_value "$MOD_LIVEPATCH" "$MOD_TARGET/patched" "1"
-unload_mod $MOD_TARGET
-check_sysfs_value "$MOD_LIVEPATCH" "$MOD_TARGET/patched" "0"
+ # check the "patch" file changes as target module loads/unloads
+ check_sysfs_value "$MOD_LIVEPATCH2" "$MOD_TARGET/patched" "0"
+ load_mod $MOD_TARGET
+ check_sysfs_value "$MOD_LIVEPATCH2" "$MOD_TARGET/patched" "1"
+ unload_mod $MOD_TARGET
+ check_sysfs_value "$MOD_LIVEPATCH2" "$MOD_TARGET/patched" "0"
-disable_lp $MOD_LIVEPATCH
-unload_lp $MOD_LIVEPATCH
+ disable_lp $MOD_LIVEPATCH2
+ unload_lp $MOD_LIVEPATCH2
-check_result "% insmod test_modules/test_klp_callbacks_demo.ko
-livepatch: enabling patch 'test_klp_callbacks_demo'
-livepatch: 'test_klp_callbacks_demo': initializing patching transition
-test_klp_callbacks_demo: pre_patch_callback: vmlinux
-livepatch: 'test_klp_callbacks_demo': starting patching transition
-livepatch: 'test_klp_callbacks_demo': completing patching transition
-test_klp_callbacks_demo: post_patch_callback: vmlinux
-livepatch: 'test_klp_callbacks_demo': patching complete
-% insmod test_modules/test_klp_callbacks_mod.ko
-livepatch: applying patch 'test_klp_callbacks_demo' to loading module 'test_klp_callbacks_mod'
-test_klp_callbacks_demo: pre_patch_callback: test_klp_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
-test_klp_callbacks_demo: post_patch_callback: test_klp_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
-test_klp_callbacks_mod: test_klp_callbacks_mod_init
-% rmmod test_klp_callbacks_mod
-test_klp_callbacks_mod: test_klp_callbacks_mod_exit
-test_klp_callbacks_demo: pre_unpatch_callback: test_klp_callbacks_mod -> [MODULE_STATE_GOING] Going away
-livepatch: reverting patch 'test_klp_callbacks_demo' on unloading module 'test_klp_callbacks_mod'
-test_klp_callbacks_demo: post_unpatch_callback: test_klp_callbacks_mod -> [MODULE_STATE_GOING] Going away
-% echo 0 > $SYSFS_KLP_DIR/test_klp_callbacks_demo/enabled
-livepatch: 'test_klp_callbacks_demo': initializing unpatching transition
-test_klp_callbacks_demo: pre_unpatch_callback: vmlinux
-livepatch: 'test_klp_callbacks_demo': starting unpatching transition
-livepatch: 'test_klp_callbacks_demo': completing unpatching transition
-test_klp_callbacks_demo: post_unpatch_callback: vmlinux
-livepatch: 'test_klp_callbacks_demo': unpatching complete
-% rmmod test_klp_callbacks_demo"
+ check_result "% insmod test_modules/$MOD_LIVEPATCH2.ko
+livepatch: enabling patch '$MOD_LIVEPATCH2'
+livepatch: '$MOD_LIVEPATCH2': initializing patching transition
+$MOD_LIVEPATCH2: pre_patch_callback: vmlinux
+livepatch: '$MOD_LIVEPATCH2': starting patching transition
+livepatch: '$MOD_LIVEPATCH2': completing patching transition
+$MOD_LIVEPATCH2: post_patch_callback: vmlinux
+livepatch: '$MOD_LIVEPATCH2': patching complete
+% insmod test_modules/$MOD_TARGET.ko
+livepatch: applying patch '$MOD_LIVEPATCH2' to loading module '$MOD_TARGET'
+$MOD_LIVEPATCH2: pre_patch_callback: $MOD_TARGET -> [MODULE_STATE_COMING] Full formed, running module_init
+$MOD_LIVEPATCH2: post_patch_callback: $MOD_TARGET -> [MODULE_STATE_COMING] Full formed, running module_init
+$MOD_TARGET: test_klp_callbacks_mod_init
+% rmmod $MOD_TARGET
+$MOD_TARGET: test_klp_callbacks_mod_exit
+$MOD_LIVEPATCH2: pre_unpatch_callback: $MOD_TARGET -> [MODULE_STATE_GOING] Going away
+livepatch: reverting patch '$MOD_LIVEPATCH2' on unloading module '$MOD_TARGET'
+$MOD_LIVEPATCH2: post_unpatch_callback: $MOD_TARGET -> [MODULE_STATE_GOING] Going away
+% echo 0 > $SYSFS_KLP_DIR/$MOD_LIVEPATCH2/enabled
+livepatch: '$MOD_LIVEPATCH2': initializing unpatching transition
+$MOD_LIVEPATCH2: pre_unpatch_callback: vmlinux
+livepatch: '$MOD_LIVEPATCH2': starting unpatching transition
+livepatch: '$MOD_LIVEPATCH2': completing unpatching transition
+$MOD_LIVEPATCH2: post_unpatch_callback: vmlinux
+livepatch: '$MOD_LIVEPATCH2': unpatching complete
+% rmmod $MOD_LIVEPATCH2"
+fi
start_test "sysfs test replace enabled"
--
2.54.0
^ permalink raw reply related
* [PATCH v5 3/6] selftests: livepatch: Introduce does_sysfs_exist function
From: Marcos Paulo de Souza @ 2026-05-04 18:34 UTC (permalink / raw)
To: Josh Poimboeuf, Jiri Kosina, Miroslav Benes, Petr Mladek,
Joe Lawrence, Shuah Khan
Cc: live-patching, linux-kselftest, linux-kernel,
Marcos Paulo de Souza, marcos
In-Reply-To: <20260504-lp-tests-old-fixes-v5-0-0be26d94ab9a@suse.com>
Returns true if the livepatch sysfs attribute exists, and false otherwise.
This new function will be used in the next patches.
Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
---
tools/testing/selftests/livepatch/functions.sh | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/tools/testing/selftests/livepatch/functions.sh b/tools/testing/selftests/livepatch/functions.sh
index 8ec0cb64ad94..2bc50271729c 100644
--- a/tools/testing/selftests/livepatch/functions.sh
+++ b/tools/testing/selftests/livepatch/functions.sh
@@ -339,6 +339,16 @@ function check_result {
fi
}
+# does_sysfs_exist(modname, attr) - check sysfs attribute existence
+# modname - livepatch module creating the sysfs interface
+# attr - attribute name to be checked
+function does_sysfs_exist() {
+ local mod="$1"; shift
+ local attr="$1"; shift
+
+ [[ -f "$SYSFS_KLP_DIR/$mod/$attr" ]]
+}
+
# check_sysfs_rights(modname, rel_path, expected_rights) - check sysfs
# path permissions
# modname - livepatch module creating the sysfs interface
--
2.54.0
^ permalink raw reply related
* [PATCH v5 2/6] selftests: livepatch: Replace true/false module parameter by y/n
From: Marcos Paulo de Souza @ 2026-05-04 18:34 UTC (permalink / raw)
To: Josh Poimboeuf, Jiri Kosina, Miroslav Benes, Petr Mladek,
Joe Lawrence, Shuah Khan
Cc: live-patching, linux-kselftest, linux-kernel,
Marcos Paulo de Souza, marcos
In-Reply-To: <20260504-lp-tests-old-fixes-v5-0-0be26d94ab9a@suse.com>
Older kernels don't support true/false for boolean module parameters
because they lack commit 0d6ea3ac94ca
("lib/kstrtox.c: add "false"/"true" support to kstrtobool()"). Replace
true/false by y/n so the test module can be loaded on older kernels.
No functional changes.
Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
---
tools/testing/selftests/livepatch/test-kprobe.sh | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/tools/testing/selftests/livepatch/test-kprobe.sh b/tools/testing/selftests/livepatch/test-kprobe.sh
index b67dfad03d97..7ced4082cff3 100755
--- a/tools/testing/selftests/livepatch/test-kprobe.sh
+++ b/tools/testing/selftests/livepatch/test-kprobe.sh
@@ -20,11 +20,11 @@ start_test "livepatch interaction with kprobed function with post_handler"
echo 1 > "$SYSFS_KPROBES_DIR/enabled"
-load_mod $MOD_KPROBE has_post_handler=true
+load_mod $MOD_KPROBE has_post_handler=y
load_failing_mod $MOD_LIVEPATCH
unload_mod $MOD_KPROBE
-check_result "% insmod test_modules/test_klp_kprobe.ko has_post_handler=true
+check_result "% insmod test_modules/test_klp_kprobe.ko has_post_handler=y
% insmod test_modules/$MOD_LIVEPATCH.ko
livepatch: enabling patch '$MOD_LIVEPATCH'
livepatch: '$MOD_LIVEPATCH': initializing patching transition
@@ -39,14 +39,14 @@ insmod: ERROR: could not insert module test_modules/$MOD_LIVEPATCH.ko: Device or
start_test "livepatch interaction with kprobed function without post_handler"
-load_mod $MOD_KPROBE has_post_handler=false
+load_mod $MOD_KPROBE has_post_handler=n
load_lp $MOD_LIVEPATCH
unload_mod $MOD_KPROBE
disable_lp $MOD_LIVEPATCH
unload_lp $MOD_LIVEPATCH
-check_result "% insmod test_modules/test_klp_kprobe.ko has_post_handler=false
+check_result "% insmod test_modules/test_klp_kprobe.ko has_post_handler=n
% insmod test_modules/$MOD_LIVEPATCH.ko
livepatch: enabling patch '$MOD_LIVEPATCH'
livepatch: '$MOD_LIVEPATCH': initializing patching transition
--
2.54.0
^ permalink raw reply related
* [PATCH v5 1/6] selftests: livepatch: Check for ARCH_HAS_SYSCALL_WRAPPER config
From: Marcos Paulo de Souza @ 2026-05-04 18:34 UTC (permalink / raw)
To: Josh Poimboeuf, Jiri Kosina, Miroslav Benes, Petr Mladek,
Joe Lawrence, Shuah Khan
Cc: live-patching, linux-kselftest, linux-kernel,
Marcos Paulo de Souza, marcos
In-Reply-To: <20260504-lp-tests-old-fixes-v5-0-0be26d94ab9a@suse.com>
Older kernels that lack CONFIG_ARCH_HAS_SYSCALL_WRAPPER config don't
have any prefixes for their syscalls. The same applies to current
powerpc and loongarch, covering all currently supported architectures
that support livepatch.
The other supported architectures have specific prefixes, so error out
when a new architecture adds livepatch support with wrappers but didn't
update the test to include it.
Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
---
.../livepatch/test_modules/test_klp_syscall.c | 27 +++++++++++++++-------
1 file changed, 19 insertions(+), 8 deletions(-)
diff --git a/tools/testing/selftests/livepatch/test_modules/test_klp_syscall.c b/tools/testing/selftests/livepatch/test_modules/test_klp_syscall.c
index dd802783ea84..0630ffd9d9a1 100644
--- a/tools/testing/selftests/livepatch/test_modules/test_klp_syscall.c
+++ b/tools/testing/selftests/livepatch/test_modules/test_klp_syscall.c
@@ -12,15 +12,26 @@
#include <linux/slab.h>
#include <linux/livepatch.h>
-#if defined(__x86_64__)
-#define FN_PREFIX __x64_
-#elif defined(__s390x__)
-#define FN_PREFIX __s390x_
-#elif defined(__aarch64__)
-#define FN_PREFIX __arm64_
+/*
+ * Before CONFIG_ARCH_HAS_SYSCALL_WRAPPER was introduced there were no
+ * prefixes for system calls.
+ * powerpc set this config based on configs, so it can be enabled or not.
+ */
+#if defined(CONFIG_ARCH_HAS_SYSCALL_WRAPPER)
+ #if defined(__x86_64__)
+ #define FN_PREFIX __x64_
+ #elif defined(__s390x__)
+ #define FN_PREFIX __s390x_
+ #elif defined(__aarch64__)
+ #define FN_PREFIX __arm64_
+ #elif defined(__powerpc__)
+ #define FN_PREFIX
+ #else
+ #error "Missing syscall wrapper for the given architecture."
+ #endif
#else
-/* powerpc does not select ARCH_HAS_SYSCALL_WRAPPER */
-#define FN_PREFIX
+ /* Do not set a prefix for architectures that do not enable wrappers. */
+ #define FN_PREFIX
#endif
/* Protects klp_pids */
--
2.54.0
^ permalink raw reply related
* [PATCH v5 0/6] kselftests: livepatch: Adapt tests to be executed on 4.12 kernels
From: Marcos Paulo de Souza @ 2026-05-04 18:34 UTC (permalink / raw)
To: Josh Poimboeuf, Jiri Kosina, Miroslav Benes, Petr Mladek,
Joe Lawrence, Shuah Khan
Cc: live-patching, linux-kselftest, linux-kernel,
Marcos Paulo de Souza, marcos
This is the fifth version of the patchset, which fixes the last Sashiko
comment, about overwriting MOD_LIVEPATCH global variable and using a
local variable to avoid module load clashing when an older kernel
don't support some sysfs attributes.
Original cover-letter:
These patches don't really change how the patches are run, just skip
some tests on kernels that don't support a feature (like kprobe and
livepatched living together) or when a livepatch sysfs attribute is
missing.
These patches are based on printk/for-next branch.
Please review! Thanks!
Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
---
Changes in v5:
- Edit the last three patches to avoid overwriting MOD_LIVEPATCH
variable, using a local variable. This fixed the last Sashiko report.
- Link to v4: https://patch.msgid.link/20260429-lp-tests-old-fixes-v4-0-59b9741989d0@suse.com
Changes in v4:
- Patch 5 was changed in order to address a comment made by Sashiko, where
subsequent tests rewrite the variables that contain the modules being loaded.
- Link to v3: https://patch.msgid.link/20260427-lp-tests-old-fixes-v3-0-ccf3c90f744c@suse.com
Changes in v3:
- Patch 1 was changed to reorganize the ifdeffery to handle multiple archs syscall wrapper (Miroslav)
- Patch 3 was changed to rework the commit message and to address function naming (Joe)
- Patches 4, 5 and 6 where had the commit messages to include the kernel version where
the given sysfs attributes were included (Petr Mladek)
- Link to v2: https://patch.msgid.link/20260413-lp-tests-old-fixes-v2-0-367c7cb5006f@suse.com
Changes in v2:
- Patch descriptions were changed to remove "test-X", since it was polluting the commit subjects (Miroslav Benes)
- Patch 8 was dropped since it was checking for a message from an out-of-tree patch. (Petr Mladek)
- Patch 3 was dropped as should be treated as expected failure for older kernels. (Petr Mladek)
- Patch 2 was changed to use y/n instead of 1/0, since it's more natural to use it.
- Patch 1 was changed to handle ppc and loongson, and error out if dealing with a different architecture that sets
CONFIG_ARCH_HAS_SYSCALL_WRAPPER and haven't changed the test to include the proper wrapper prefix.
- Patch 4 was changed to invert the return of the bash function to return 1 in failure, like
a normal bash function (Joe Lawrence)
- Patches 5, 6 an 7 were changed to not split the tests, but to only run the tests
when the attribute were present (Miroslav Benes)
- Link to v1: https://patch.msgid.link/20260313-lp-tests-old-fixes-v1-0-71ac6dfb3253@suse.com
---
Marcos Paulo de Souza (6):
selftests: livepatch: Check for ARCH_HAS_SYSCALL_WRAPPER config
selftests: livepatch: Replace true/false module parameter by y/n
selftests: livepatch: Introduce does_sysfs_exist function
selftests: livepatch: Check if patched sysfs attribute exists
selftests: livepatch: Check if replace sysfs attribute exists
selftests: livepatch: Check if stack_order sysfs attribute exists
tools/testing/selftests/livepatch/functions.sh | 10 +
tools/testing/selftests/livepatch/test-kprobe.sh | 8 +-
tools/testing/selftests/livepatch/test-sysfs.sh | 219 +++++++++++----------
.../livepatch/test_modules/test_klp_syscall.c | 27 ++-
4 files changed, 153 insertions(+), 111 deletions(-)
---
base-commit: 712c0756828becbfc629ff8d8b82deff5d1115e4
change-id: 20260309-lp-tests-old-fixes-f955abc8ec27
Best regards,
--
Marcos Paulo de Souza <mpdesouza@suse.com>
^ permalink raw reply
* Re: [PATCH v2 18/53] objtool/klp: Simplify reloc symbol conversion
From: Miroslav Benes @ 2026-05-04 14:04 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: x86, linux-kernel, live-patching, Peter Zijlstra, Joe Lawrence,
Song Liu, Miroslav Benes, Petr Mladek
In-Reply-To: <9572b2e15500e5ed8dcbaac78c966557d3000d85.1777575752.git.jpoimboe@kernel.org>
On Thu, 30 Apr 2026 21:08:06 -0700, Josh Poimboeuf <jpoimboe@kernel.org> wrote:
> Inline section_reference_needed() and is_reloc_allowed() into
> convert_reloc_sym() and remove the redundant is_reloc_allowed() check in
> clone_reloc().
>
> Move the is_sec_sym() checks into the convert callees so they become
> no-ops when the reloc is already in the right format. This allows
> convert_reloc_sym() to unconditionally dispatch to the right converter
> based on section type.
>
> [...]
Reviewed-by: Miroslav Benes <mbenes@suse.cz>
--
Miroslav
^ permalink raw reply
* Re: [PATCH v2 10/53] objtool/klp: Fix --debug-checksum for duplicate symbol names
From: Miroslav Benes @ 2026-05-04 14:04 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: x86, linux-kernel, live-patching, Peter Zijlstra, Joe Lawrence,
Song Liu, Miroslav Benes, Petr Mladek
In-Reply-To: <83781c62e85a35d641b7d793b133195e3a4abba7.1777575752.git.jpoimboe@kernel.org>
On Thu, 30 Apr 2026 21:07:58 -0700, Josh Poimboeuf <jpoimboe@kernel.org> wrote:
> find_symbol_by_name() only returns the first match, so
> --debug-checksum=<func> silently ignores any subsequent duplicately
> named functions after the first.
>
> Fix that, along with a new for_each_sym_by_name() helper.
>
>
> [...]
Reviewed-by: Miroslav Benes <mbenes@suse.cz>
--
Miroslav
^ permalink raw reply
* Re: [PATCH v2 09/53] objtool: Replace iterator callback with for_each_sym_by_mangled_name()
From: Miroslav Benes @ 2026-05-04 13:59 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: x86, linux-kernel, live-patching, Peter Zijlstra, Joe Lawrence,
Song Liu, Miroslav Benes, Petr Mladek
In-Reply-To: <cb95eae9cc63ca04f881c69c93eed6bac0c751fe.1777575752.git.jpoimboe@kernel.org>
On Thu, 30 Apr 2026 21:07:57 -0700, Josh Poimboeuf <jpoimboe@kernel.org> wrote:
> Convert the callback-based iterate_sym_by_demangled_name() with a new
> for_each_sym_by_demangled_name() macro. This eliminates the callback
> struct/function and makes the code more compact and readable.
Reviewed-by: Miroslav Benes <mbenes@suse.cz>
--
Miroslav
^ permalink raw reply
* Re: [PATCH v5 8/8] unwind: arm64: Use sframe to unwind interrupt frames
From: Jens Remus @ 2026-05-04 8:47 UTC (permalink / raw)
To: Mark Rutland, Dylan Hatch
Cc: Roman Gushchin, Weinan Liu, Will Deacon, Josh Poimboeuf,
Indu Bhagat, Peter Zijlstra, Steven Rostedt, Catalin Marinas,
Jiri Kosina, Prasanna Kumar T S M, Puranjay Mohan, Song Liu,
joe.lawrence, linux-toolchains, linux-kernel, live-patching,
linux-arm-kernel, Randy Dunlap, Heiko Carstens
In-Reply-To: <afTYzAF_x41pyilu@J2N7QTR9R3>
Hello Mark,
I mostly have comments regarding your the SFrame related remarks.
On 5/1/2026 6:46 PM, Mark Rutland wrote:
> Thanks for putting this together. I think this is looking pretty good.
> However, there are some things that aren't quite right and need some
> work, which I've commented on below.
> (2) To make unwinding generally possible, we'll need to annotate some
> assembly functions as unwindable. We'll need to do that for string
> routines under lib/, and probably some crypto code, but we don't
> need to do that for most code in head.S, entry.S, etc.
>
> The vast majority of relevant assembly functions are leaf functions
> (where the return address is never moved out of the LR), so we can
> probably get away with a simple annotation for those that avoids the
> need for open-coded CFI directives everywhere.
Wrapping them in .cfi_startproc ... .cfi_endproc should do. For instance
by extending SYM_FUNC_START() and SYM_FUNC_END() or introducing flavors
that do. Or where you thinking of something else?
> On Tue, Apr 28, 2026 at 06:36:43PM +0000, Dylan Hatch wrote:
>> diff --git a/arch/arm64/include/asm/stacktrace/common.h b/arch/arm64/include/asm/stacktrace/common.h
>> @@ -21,6 +21,8 @@ struct stack_info {
>> *
>> * @fp: The fp value in the frame record (or the real fp)
>> * @pc: The lr value in the frame record (or the real lr)
>> + * @sp: The sp value at the call site of the current function.
>> + * @unreliable: Stacktrace is unreliable.
>> *
>> * @stack: The stack currently being unwound.
>> * @stacks: An array of stacks which can be unwound.
>> @@ -29,7 +31,11 @@ struct stack_info {
>> struct unwind_state {
>> unsigned long fp;
>> unsigned long pc;
>> +#ifdef CONFIG_HAVE_UNWIND_KERNEL_SFRAME
>> + unsigned long sp;
>> +#endif
>
> As this is only used by the kernel unwinder (and not the hyp unwinder),
> this should live in struct kunwind_state in stacktrace.c.
>
> That said, for unwinding across exception boundaries we should not need
> this, as the SP value will be in the pt_regs. If we only use SFrame for
> the exception boundary case, we can remove this entirely. I would
> strongly prefer that we do that.
>> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
>> +/*
>> + * Unwind to the next frame according to sframe.
>> + */
>> +static __always_inline int
>> +unwind_next_frame_sframe(struct kunwind_state *state)
>> +{
>> + struct unwind_frame frame;
>> + unsigned long cfa, fp, ra;
>> + enum kunwind_source source = KUNWIND_SOURCE_FRAME;
>> + struct pt_regs *regs = state->regs;
>> +
>> + int err;
>
> As above, we should only use this for unwinding from the regs, after a
> KUNWIND_SOURCE_REGS_PC step.
>
> With that in mind, it would be good to:
>
> (1) Rename this to something like kunwind_next_regs_sframe(). Note
> 'kunwind' rather than 'unwind' for consistency with the rest of this
> file.
>
> (2) Add the following sanity checks:
>
> if (WARN_ON_ONCE(state->source != KUNWIND_SOURCE_REGS_PC))
> return -EINVAL;
> if (WARN_ON_ONCE(!state->regs))
> return -EINVAL;
>
> ... which will also allow the code below to be simplified.
>
>> +
>> + /* FP/SP alignment 8 bytes */
>> + if (state->common.fp & 0x7 || state->common.sp & 0x7)
>> + return -EINVAL;
>> +
>> + /*
>> + * Most/all outermost functions are not visible to sframe. So, check for
>> + * a meta frame record if the sframe lookup fails.
>> + */
>> + err = sframe_find_kernel(state->common.pc, &frame);
>> + if (err)
>> + return kunwind_next_frame_record_meta(state);
>> +
>> + if (frame.outermost)
>> + return -ENOENT;
>
> I don't think we ever expect an outermost frame within the kernel. We
> haven't added any annotations for that, and we expect to unwind all the
> way to a FRAME_META_TYPE_FINAL frame.
>
> We cannot fall back to kunwind_next_frame_record_meta() here. We don't
> know that the next frame is a meta frame (and this results in a warning
> noted above), and we don't know the result is going to be reliable if we
> don't have SFrame data, so the right thing to do is return an error.
>
> I think this should be:
>
> /*
> * A kernel unwind should always end at a FRAME_META_TYPE_FINAL
> * frame. There should be no outermost frames within the kernel.
> */
> if (frame.outermost)
> return -EINVAL;
Makes sense.
>
> err = sframe_find_kernel(state->common.pc, &frame);
> if (err)
> return -EINVAL;
>
>> + /* Get the Canonical Frame Address (CFA) */
>> + switch (frame.cfa.rule) {
>> + case UNWIND_CFA_RULE_SP_OFFSET:
>> + cfa = state->common.sp;
IIUC you suggest this to be changed as follows?
cfa = regs->regs[31];
>> + break;
>> + case UNWIND_CFA_RULE_FP_OFFSET:
>> + if (state->common.fp < state->common.sp)
>> + return -EINVAL;
>> + cfa = state->common.fp;
>> + break;
>> + case UNWIND_CFA_RULE_REG_OFFSET:
>> + case UNWIND_CFA_RULE_REG_OFFSET_DEREF:
>> + /* regs only available in topmost/interrupt frame */
>> + if (!regs || frame.cfa.regnum > 30)
>> + return -EINVAL;
>> + cfa = regs->regs[frame.cfa.regnum];
>> + break;
>
> Do we ever expect to see UNWIND_CFA_RULE_REG_OFFSET or
> UNWIND_CFA_RULE_REG_OFFSET_DEREF in practice for kernel code?
No. Those can only occur with SFrame V3 flexible FDE, which are
currently not generated by GNU assembler for arm64/aarch64, and thus
could be omitted in the arm64-specific kernel sframe unwinder:
https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=gas/config/tc-aarch64.h;hb=binutils-2_46#l342
I must admit that while reviewing I thought it would be future-proof to
have support for rules that can only be represented with SFrame V3
flexible FDE, even if they are currently not used on arm64. Ideally
kunwind_next_sframe() could be made common, if another architecture
would implement kernel unwinding using sframe.
>
>> + default:
>> + WARN_ON_ONCE(1);
>> + return -EINVAL;
>> + }
>> + cfa += frame.cfa.offset;
>> +
>> + /*
>> + * CFA typically points to a higher address than RA or FP, so don't
>> + * consume from the stack when we read it.
>> + */
>> + if (frame.cfa.rule & UNWIND_RULE_DEREF &&
>> + !get_word(&state->common, &cfa))
>> + return -EINVAL;
>
> Per the switch above, this could only be
> UNWIND_CFA_RULE_REG_OFFSET_DEREF. As above, do we ever expect to
> encounter that in practice for kernel code?
No. See above.
>
>> +
>> + /* CFA alignment 8 bytes */
>> + if (cfa & 0x7)
>> + return -EINVAL;
>
> If the CFA is the SP upon entry to the function, then per AAPCS64 rules
> it should be aligned to 16 bytes. Otherwise, where has this 8 byte
> alignment requirement come from? Does SFrame mandate that?
That originates from the common unwind user logic (see
kernel/unwind/user.c, unwind_user_next_common()), which currently
assumes 8-byte/4-byte SP alignment for all 64-bit/32-bit architectures.
So checking for 16-byte alignment here would make sense.
>
>> +
>> + /* Get the Return Address (RA) */
>> + switch (frame.ra.rule) {
>> + case UNWIND_RULE_RETAIN:
>> + /* regs only available in topmost/interrupt frame */
>> + if (!regs)
>> + return -EINVAL;
>> + ra = regs->regs[30];
>> + source = KUNWIND_SOURCE_REGS_LR;
>> + break;
>> + /* UNWIND_USER_RULE_CFA_OFFSET not implemented on purpose */
Nit: s/UNWIND_USER_RULE_CFA_OFFSET/UNWIND_RULE_CFA_OFFSET/
>
> It would be better for the comment to say *why* that's not implemented.
>
> I assume that's because UNWIND_USER_RULE_CFA_OFFSET would mean that the return
> address is a stack address, and that's obviously not legitimate.
That and SFrame V3 currently cannot represent FP/RA as CFA + offset
(i.e. UNWIND_RULE_CFA_OFFSET; .cfi_val_offset FP/RA).
The comment originates from the common unwind user logic (see
kernel/unwind/user.c). I am open to improve that. What about:
/*
* UNWIND_RULE_CFA_OFFSET not implemented on purpose, as a stack
* address cannot be a legitimate return address value. It is
* also not used (e.g. not represented in sframe).
*/
>
>> + case UNWIND_RULE_CFA_OFFSET_DEREF:
>> + ra = cfa + frame.ra.offset;
>> + break;
>> + case UNWIND_RULE_REG_OFFSET:
>> + case UNWIND_RULE_REG_OFFSET_DEREF:
>> + /* regs only available in topmost/interrupt frame */
>> + if (!regs)
>> + return -EINVAL;
>> + ra = regs->regs[frame.cfa.regnum];
>> + ra += frame.ra.offset;
>> + break;
>
> Do we ever expect UNWIND_RULE_REG_OFFSET or UNWIND_RULE_REG_OFFSET_DEREF
> in practice for kernel code?
No. See above (SFrame V3 flexible FDE).
>
> I don't think we expect UNWIND_RULE_REG_OFFSET unless that's sometimes used
> instead of UNWIND_RULE_RETAIN to express that the return address is in x30
> (with zero offset).
No. Unless there would be nonsense .cfi_register 30, 30, which would
require SFrame V3 flexible FDE to be represented.
@Indu: We may consider to treat .cfi_register <reg>, <reg> (for FP/RA)
like .cfi_restore <reg> in the GNU assembler?
>
> Similarly, if the address is on the stack it should be in a frame
> record. Would we ever expect UNWIND_RULE_REG_OFFSET_DEREF rather than
> UNWIND_RULE_CFA_OFFSET_DEREF?
No. See above (SFrame V3 flexible FDE).
>
>> + default:
>> + WARN_ON_ONCE(1);
>> + return -EINVAL;
>> + }
>> +
>> + /* Get the Frame Pointer (FP) */
>> + switch (frame.fp.rule) {
>> + case UNWIND_RULE_RETAIN:
>> + fp = state->common.fp;
>> + break;
>> + /* UNWIND_USER_RULE_CFA_OFFSET not implemented on purpose */
>
> As for RA, the comment should explain why that's not implemented.
I am open to improve the comment in the the common unwind user logic.
What about:
/*
* UNWIND_RULE_CFA_OFFSET not implemented on purpose, as it is
* not used (e.g. not represented in sframe).
*/
>
>> + case UNWIND_RULE_CFA_OFFSET_DEREF:
>> + fp = cfa + frame.fp.offset;
>> + break;
>> + case UNWIND_RULE_REG_OFFSET:
>> + case UNWIND_RULE_REG_OFFSET_DEREF:
>> + /* regs only available in topmost/interrupt frame */
>> + if (!regs)
>> + return -EINVAL;
>> + fp = regs->regs[frame.fp.regnum];
>> + fp += frame.fp.offset;
>> + break;
Likewise neither UNWIND_RULE_REG_OFFSET nor UNWIND_RULE_REG_OFFSET_DEREF
can currently occur on arm64. See above (SFrame V3 flexible FDE).
>> + default:
>> + WARN_ON_ONCE(1);
>> + return -EINVAL;
>> + }
>> +
>> + /*
>> + * Consume RA and FP from the stack. The frame record puts FP at a lower
>> + * address than RA, so we always read FP first.
>> + */
>> + if (frame.fp.rule & UNWIND_RULE_DEREF &&
>> + !get_word(&state->common, &fp))
>> + return -EINVAL;
>
> Why is this get_word() rather than get_consume_word()?
>
>> +
>> + if (frame.ra.rule & UNWIND_RULE_DEREF &&
>> + get_consume_word(&state->common, &ra))
>> + return -EINVAL;
>> +
>> + state->common.pc = ra;
>> + state->common.sp = cfa;
>
> As above, the SP can be removed.
>
>> + state->common.fp = fp;
>> +
>> + state->source = source;
>> +
>> + return 0;
>> +}
Regards,
Jens
--
Jens Remus
Linux on Z Development (D3303)
jremus@de.ibm.com / jremus@linux.ibm.com
IBM Deutschland Research & Development GmbH; Vorsitzender des Aufsichtsrats: Wolfgang Wendt; Geschäftsführung: David Faller; Sitz der Gesellschaft: Ehningen; Registergericht: Amtsgericht Stuttgart, HRB 243294
IBM Data Privacy Statement: https://www.ibm.com/privacy/
^ permalink raw reply
* Re: [PATCH v2 00/53] objtool/klp: Some klp-build fixes and improvements
From: Josh Poimboeuf @ 2026-05-01 18:47 UTC (permalink / raw)
To: x86
Cc: linux-kernel, live-patching, Peter Zijlstra, Joe Lawrence,
Song Liu, Miroslav Benes, Petr Mladek
In-Reply-To: <cover.1777575752.git.jpoimboe@kernel.org>
On Thu, Apr 30, 2026 at 09:07:48PM -0700, Josh Poimboeuf wrote:
> Changes since v1 (https://lore.kernel.org/cover.1776916871.git.jpoimboe@kernel.org):
>
> - Add comment for find_reloc_by_dest_range() first-match behavior
> [Peter]
> - Simplify is_cold_func() [Peter]
> - Grow __cfi_ symbols [Peter]
> - Rename "Ignore __UNIQUE_ID_*() PCI stub functions" to more general
> "Don't report uncorrelated functions as new" [Song]
> - Move rodata non-correlation into pointer-comparison fix [Miroslav]
> - Add comments for convert_reloc_sym() return values [Song]
> - Remove redundant SRC/OBJ variables [Song]
> - Use "if (mismatch) {} else" in for_each_sym_by_*() [Song]
> - Flatten nested if-else chain in short-circuit validation [Song]
> - Add comments with examples to symbol correlation algorithm [Song]
> - Move callback refactor to earlier in the patch set [Miroslav]
> - Fix reloc corruption in convert_reloc_sym_to_secsym() [Sashiko]
> - Include offset in object checksum hashing [Sashiko]
> - Fix klp-build checksum comparison output for added/removed
> instructions [Sashiko]
> - Fix kCFI prefix finding/cloning
> - Add reloc symbol conversion simplification cleanup
> - Improve local label check for uncorrelated symbols
> - Drop "Make function prefix handling more generic" for now (refactored
> version will come with arm64 patches)
> - Refactor inline alternative cloning into separate
> clone_inline_alternatives()
> - Add Acked-by/Reviewed-by tags
I'm squashing a few Sashiko nits (see below) into their relevant
patches, along with a minor bisectability issue.
latest is at:
git://git.kernel.org/pub/scm/linux/kernel/git/jpoimboe/linux.git klp-build-arm64
diff --git a/scripts/livepatch/klp-build b/scripts/livepatch/klp-build
index 31604d48ff49..10145b1dd089 100755
--- a/scripts/livepatch/klp-build
+++ b/scripts/livepatch/klp-build
@@ -275,7 +275,7 @@ validate_config() {
[[ "$CONFIG_AS_VERSION" -lt 200000 ]] && \
die "Clang assembler version < 20 not supported"
- "$OBJTOOL" klp 2>&1 | command grep -q "not implemented" && \
+ [[ -x "$OBJTOOL" ]] && "$OBJTOOL" klp 2>&1 | command grep -q "not implemented" && \
die "objtool not built with KLP support; install xxhash-devel/libxxhash-dev (version >= 0.8) and recompile"
return 0
@@ -741,7 +741,7 @@ diff_objects() {
(
cd "$ORIG_CSUM_DIR"
- [[ -v VERBOSE ]] && echo "${cmd[@]}"
+ [[ -v VERBOSE ]] && echo "cd $ORIG_CSUM_DIR && ${cmd[*]}"
"${cmd[@]}" \
1> >(tee -a "$log") \
2> >(tee -a "$log" | "${filter[@]}" >&2) || \
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index ca360239ea2b..2b03a2d6fc95 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -2494,7 +2494,7 @@ static int __annotate_late(struct objtool_file *file, int type, struct instructi
ERROR_INSN(insn, "dodgy NOCFI annotation");
return -1;
}
- insn_sym(insn)->nocfi = 1;
+ sym->nocfi = 1;
break;
default:
^ permalink raw reply related
* Re: [PATCH v2 20/53] objtool/klp: Don't correlate .rodata.cst* constant pool objects
From: Song Liu @ 2026-05-01 17:31 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: x86, linux-kernel, live-patching, Peter Zijlstra, Joe Lawrence,
Miroslav Benes, Petr Mladek
In-Reply-To: <yd3becobjg77p7yylqqgmrdznkejvwbdzojzxk5lqlsihp4377@d6yfaqstriec>
On Fri, May 1, 2026 at 6:04 PM Josh Poimboeuf <jpoimboe@kernel.org> wrote:
>
> On Fri, May 01, 2026 at 11:37:49AM +0100, Song Liu wrote:
> > > +/*
> > > + * Some .rodata is anonymous and can't be correlated due to there being no
> > > + * symbol names.
> > > + *
> > > + * The .rodata.cst* sections aren't technically anonymous, they're SHF_MERGE
> > > + * constant pool sections containing small fixed-size data (lookup tables,
> > > + * bitmasks) which are only read by value, so pointer equivalence isn't needed.
> > > + * They are typically referenced by UBSAN data sections.
> > > + */
> > > +static bool is_anonymous_rodata(struct symbol *sym)
> > > +{
> > > + return is_rodata_sec(sym->sec) &&
> > > + (!is_object_sym(sym) || strstarts(sym->sec->name, ".rodata.cst"));
> > > +}
> > > +
> > > /*
> > > * These symbols should never be correlated, so their local patched versions
> > > * are used instead of linking to the originals.
> > > @@ -386,7 +401,7 @@ static bool dont_correlate(struct symbol *sym)
> > > is_uncorrelated_static_local(sym) ||
> > > is_local_label(sym) ||
> > > is_string_sec(sym->sec) ||
> > > - (is_rodata_sec(sym->sec) && !is_object_sym(sym)) ||
> > ^^^^
> > This line was added in 19/53. Maybe we can merge 19 and 20?
>
> I think I'd prefer to keep them separate as they are two distinct issues
> related to rodata: pointer equivalence (patch 19) and UBSAN mergeable
> constants (patch 20).
Fair enough. I think we can keep these two patches as-is.
Thanks,
Song
Acked-by: Song Liu <song@kernel.org>
^ permalink raw reply
* Re: [PATCH v2 20/53] objtool/klp: Don't correlate .rodata.cst* constant pool objects
From: Josh Poimboeuf @ 2026-05-01 17:04 UTC (permalink / raw)
To: Song Liu
Cc: x86, linux-kernel, live-patching, Peter Zijlstra, Joe Lawrence,
Miroslav Benes, Petr Mladek
In-Reply-To: <CAPhsuW7wAG1MEb2xVxFLN2V_BG4KddhKRXaL9LPxWEP2DUFAJQ@mail.gmail.com>
On Fri, May 01, 2026 at 11:37:49AM +0100, Song Liu wrote:
> > +/*
> > + * Some .rodata is anonymous and can't be correlated due to there being no
> > + * symbol names.
> > + *
> > + * The .rodata.cst* sections aren't technically anonymous, they're SHF_MERGE
> > + * constant pool sections containing small fixed-size data (lookup tables,
> > + * bitmasks) which are only read by value, so pointer equivalence isn't needed.
> > + * They are typically referenced by UBSAN data sections.
> > + */
> > +static bool is_anonymous_rodata(struct symbol *sym)
> > +{
> > + return is_rodata_sec(sym->sec) &&
> > + (!is_object_sym(sym) || strstarts(sym->sec->name, ".rodata.cst"));
> > +}
> > +
> > /*
> > * These symbols should never be correlated, so their local patched versions
> > * are used instead of linking to the originals.
> > @@ -386,7 +401,7 @@ static bool dont_correlate(struct symbol *sym)
> > is_uncorrelated_static_local(sym) ||
> > is_local_label(sym) ||
> > is_string_sec(sym->sec) ||
> > - (is_rodata_sec(sym->sec) && !is_object_sym(sym)) ||
> ^^^^
> This line was added in 19/53. Maybe we can merge 19 and 20?
I think I'd prefer to keep them separate as they are two distinct issues
related to rodata: pointer equivalence (patch 19) and UBSAN mergeable
constants (patch 20).
--
Josh
^ permalink raw reply
* Re: [PATCH v5 8/8] unwind: arm64: Use sframe to unwind interrupt frames
From: Mark Rutland @ 2026-05-01 16:46 UTC (permalink / raw)
To: Dylan Hatch
Cc: Roman Gushchin, Weinan Liu, Will Deacon, Josh Poimboeuf,
Indu Bhagat, Peter Zijlstra, Steven Rostedt, Catalin Marinas,
Jiri Kosina, Jens Remus, Prasanna Kumar T S M, Puranjay Mohan,
Song Liu, joe.lawrence, linux-toolchains, linux-kernel,
live-patching, linux-arm-kernel, Randy Dunlap
In-Reply-To: <20260428183643.3796063-9-dylanbhatch@google.com>
Hi Dylan,
Thanks for putting this together. I think this is looking pretty good.
However, there are some things that aren't quite right and need some
work, which I've commented on below.
More generally, there are a few things that aren't addressed by this
series that we will also need to address. Importantly:
(1) For correctness, we'll need to address a latent issue with unwinding
across an fgraph return trampoline, where the return address is
transiently unrecoverable.
Before this series, that doesn't matter for livepatching because the
livepatching code isn't called synchronously within the fgraph
handler, and unwinds which cross an exception boundary are marked as
unreliable.
After this series, that does matter as we can unwind across an
exception boundary, and might happen to interrupt that transient
window.
I think we can solve that with some restructuring of that code,
restoring the original address *before* removing that from the
fgraph return stack, and ensuring that the unwinder can find it.
I'm not immediately sure whether kretprobes has a similar issue.
(2) To make unwinding generally possible, we'll need to annotate some
assembly functions as unwindable. We'll need to do that for string
routines under lib/, and probably some crypto code, but we don't
need to do that for most code in head.S, entry.S, etc.
The vast majority of relevant assembly functions are leaf functions
(where the return address is never moved out of the LR), so we can
probably get away with a simple annotation for those that avoids the
need for open-coded CFI directives everywhere.
I've pushed some reliable stacktrace tests to:
git://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git stacktrace/tests
That finds the fgraph issue (regardless of this series). When merged
with this series triggers a warning in kunwind_next_frame_record_meta(),
where unwind_next_frame_sframe() calls that erroneously as a fallback.
As noted below, I think that fallback path should be removed, and
unwind_next_frame_sframe() should return an error in that case.
On Tue, Apr 28, 2026 at 06:36:43PM +0000, Dylan Hatch wrote:
> Add unwind_next_frame_sframe() function to unwind by sframe info if
> present. Use this method at exception boundaries, falling back to
> frame-pointer unwind only on failure. In such failure cases, the
> stacktrace is considered unreliable.
>
> During normal unwind, prefer frame pointer unwind (for better
> performance) with sframe as a backup.
We should certainly use SFrame at an exception boundary. However, when
frame point unwind fails I do not think we should use it as a backup.
That only fails when something is already wrong, and an SFrame unwind
isn't necessarily going to be better. I think we should immediately fail
in those cases.
> This change restores the LR behavior originally introduced in commit
> c2c6b27b5aa14fa2 ("arm64: stacktrace: unwind exception boundaries"),
> But later removed in commit 32ed1205682e ("arm64: stacktrace: Skip
> reporting LR at exception boundaries")
>
> This can be done because the sframe data can be used to determine
> whether the LR is current for the PC value recovered from pt_regs at the
> exception boundary.
>
> Signed-off-by: Weinan Liu <wnliu@google.com>
> Reviewed-by: Prasanna Kumar T S M <ptsm@linux.microsoft.com>
> Reviewed-by: Jens Remus <jremus@linux.ibm.com>
> Signed-off-by: Dylan Hatch <dylanbhatch@google.com>
> ---
> arch/arm64/include/asm/stacktrace/common.h | 6 +
> arch/arm64/kernel/stacktrace.c | 246 +++++++++++++++++++--
> 2 files changed, 232 insertions(+), 20 deletions(-)
>
> diff --git a/arch/arm64/include/asm/stacktrace/common.h b/arch/arm64/include/asm/stacktrace/common.h
> index 821a8fdd31af..4df68181e1b5 100644
> --- a/arch/arm64/include/asm/stacktrace/common.h
> +++ b/arch/arm64/include/asm/stacktrace/common.h
> @@ -21,6 +21,8 @@ struct stack_info {
> *
> * @fp: The fp value in the frame record (or the real fp)
> * @pc: The lr value in the frame record (or the real lr)
> + * @sp: The sp value at the call site of the current function.
> + * @unreliable: Stacktrace is unreliable.
> *
> * @stack: The stack currently being unwound.
> * @stacks: An array of stacks which can be unwound.
> @@ -29,7 +31,11 @@ struct stack_info {
> struct unwind_state {
> unsigned long fp;
> unsigned long pc;
> +#ifdef CONFIG_HAVE_UNWIND_KERNEL_SFRAME
> + unsigned long sp;
> +#endif
As this is only used by the kernel unwinder (and not the hyp unwinder),
this should live in struct kunwind_state in stacktrace.c.
That said, for unwinding across exception boundaries we should not need
this, as the SP value will be in the pt_regs. If we only use SFrame for
the exception boundary case, we can remove this entirely. I would
strongly prefer that we do that.
> + bool unreliable;
Likewise, this should live in struct kunwind_state.
> struct stack_info stack;
> struct stack_info *stacks;
> int nr_stacks;
> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
> index 3ebcf8c53fb0..c935323f393b 100644
> --- a/arch/arm64/kernel/stacktrace.c
> +++ b/arch/arm64/kernel/stacktrace.c
> @@ -14,6 +14,7 @@
> #include <linux/sched/debug.h>
> #include <linux/sched/task_stack.h>
> #include <linux/stacktrace.h>
> +#include <linux/sframe.h>
Nit: these are supposed to be ordered alphabetically, so the include of
<linux/sframe.h> should be just before <linux/stacktrace.h>.
>
> #include <asm/efi.h>
> #include <asm/irq.h>
> @@ -26,6 +27,7 @@ enum kunwind_source {
> KUNWIND_SOURCE_CALLER,
> KUNWIND_SOURCE_TASK,
> KUNWIND_SOURCE_REGS_PC,
> + KUNWIND_SOURCE_REGS_LR,
> };
>
> union unwind_flags {
> @@ -85,6 +87,9 @@ kunwind_init_from_regs(struct kunwind_state *state,
> state->regs = regs;
> state->common.fp = regs->regs[29];
> state->common.pc = regs->pc;
> +#ifdef CONFIG_HAVE_UNWIND_KERNEL_SFRAME
> + state->common.sp = regs->sp;
> +#endif
As above, I don't think we need to stash the SP, as it only matters when
performing the next unwind from the KUNWIND_SOURCE_REGS_PC state, and in
that state we have the regs.
> state->source = KUNWIND_SOURCE_REGS_PC;
> }
>
> @@ -103,6 +108,9 @@ kunwind_init_from_caller(struct kunwind_state *state)
>
> state->common.fp = (unsigned long)__builtin_frame_address(1);
> state->common.pc = (unsigned long)__builtin_return_address(0);
> +#ifdef CONFIG_HAVE_UNWIND_KERNEL_SFRAME
> + state->common.sp = (unsigned long)__builtin_frame_address(0);
> +#endif
> state->source = KUNWIND_SOURCE_CALLER;
> }
This is not correct. On arm64, __builtin_frame_address(0) returns the
address of the current function's frame record. That's not the same as
the SP of the caller (which would necessarily differ by at least the
size of that frame record).
For example, the following:
void *return_own_frame(void)
{
return __builtin_frame_address(0);
}
... is compiled by GCC 15.2.0 as:
0000000000000000 <return_own_frame>:
0: d503233f paciasp
4: a9bf7bfd stp x29, x30, [sp, #-16]!
8: 910003fd mov x29, sp
c: aa1d03e0 mov x0, x29
10: a8c17bfd ldp x29, x30, [sp], #16
14: d50323bf autiasp
18: d65f03c0 ret
1c: d503201f nop
As above, I think we can remove unwind_state:sp entirely, and omit this.
> @@ -124,6 +132,9 @@ kunwind_init_from_task(struct kunwind_state *state,
>
> state->common.fp = thread_saved_fp(task);
> state->common.pc = thread_saved_pc(task);
> +#ifdef CONFIG_HAVE_UNWIND_KERNEL_SFRAME
> + state->common.sp = thread_saved_sp(task);
> +#endif
> state->source = KUNWIND_SOURCE_TASK;
> }
As above, I think we can remove unwind_state:sp entirely, and omit this.
In contrast to kunwind_init_from_caller() above, given the way the
cpu_switch_to() assembly function saves the FP/PC/SP, those should all
be consistent with the values in the caller immediately after the caller
is returned to.
> @@ -181,7 +192,6 @@ int kunwind_next_regs_pc(struct kunwind_state *state)
> state->regs = regs;
> state->common.pc = regs->pc;
> state->common.fp = regs->regs[29];
> - state->regs = NULL;
> state->source = KUNWIND_SOURCE_REGS_PC;
> return 0;
> }
> @@ -237,6 +247,9 @@ kunwind_next_frame_record(struct kunwind_state *state)
>
> unwind_consume_stack(&state->common, info, fp, sizeof(*record));
>
> +#ifdef CONFIG_HAVE_UNWIND_KERNEL_SFRAME
> + state->common.sp = state->common.fp;
> +#endif
This is not correct. The caller's frame record can be anywhere in the
caller's stack frame, and we definitely have cases today where FP != SP
at a function call boundary.
For example, the add_random_kstack_offset() logic in invoke_syscall()
typically results in the SP being decremented after the frame record has
been placed on the stack.
That looks roughly like the following:
void callee(void *ptr);
void caller(size_t size)
{
void *ptr = __builtin_alloca(size);
callee(ptr);
}
... which GCC 15.2.0 can compile as:
0000000000000000 <caller>:
0: d503233f paciasp
4: 91003c00 add x0, x0, #0xf
8: a9bf7bfd stp x29, x30, [sp, #-16]!
c: 927cec00 and x0, x0, #0xfffffffffffffff0
10: 910003fd mov x29, sp
14: cb2063ff sub sp, sp, x0
18: 910003e0 mov x0, sp
1c: 94000000 bl 0 <callee>
20: 910003bf mov sp, x29
24: a8c17bfd ldp x29, x30, [sp], #16
28: d50323bf autiasp
2c: d65f03c0 ret
As above, I think we can remove unwind_state:sp entirely, and omit this.
> state->common.fp = new_fp;
> state->common.pc = new_pc;
> state->source = KUNWIND_SOURCE_FRAME;
> @@ -244,6 +257,176 @@ kunwind_next_frame_record(struct kunwind_state *state)
> return 0;
> }
>
> +#ifdef CONFIG_HAVE_UNWIND_KERNEL_SFRAME
> +
> +static __always_inline struct stack_info *
> +get_word(struct unwind_state *state, unsigned long *word)
> +{
> + unsigned long addr = *word;
> + struct stack_info *info;
> +
> + info = unwind_find_stack(state, addr, sizeof(addr));
> + if (!info)
> + return info;
> +
> + *word = READ_ONCE(*(unsigned long *)addr);
> +
> + return info;
> +}
> +
> +static __always_inline int
> +get_consume_word(struct unwind_state *state, unsigned long *word)
> +{
> + struct stack_info *info;
> + unsigned long addr = *word;
> +
> + info = get_word(state, word);
> + if (!info)
> + return -EINVAL;
> +
> + unwind_consume_stack(state, info, addr, sizeof(addr));
> + return 0;
> +}
I was hoping that we wouldn't need these if we only used SFrame to
determine whether to use the LR or frame record, but I see that there
could be cases where the frame record might be partially constructed.
> +
> +/*
> + * Unwind to the next frame according to sframe.
> + */
> +static __always_inline int
> +unwind_next_frame_sframe(struct kunwind_state *state)
> +{
> + struct unwind_frame frame;
> + unsigned long cfa, fp, ra;
> + enum kunwind_source source = KUNWIND_SOURCE_FRAME;
> + struct pt_regs *regs = state->regs;
> +
> + int err;
As above, we should only use this for unwinding from the regs, after a
KUNWIND_SOURCE_REGS_PC step.
With that in mind, it would be good to:
(1) Rename this to something like kunwind_next_regs_sframe(). Note
'kunwind' rather than 'unwind' for consistency with the rest of this
file.
(2) Add the following sanity checks:
if (WARN_ON_ONCE(state->source != KUNWIND_SOURCE_REGS_PC))
return -EINVAL;
if (WARN_ON_ONCE(!state->regs))
return -EINVAL;
... which will also allow the code below to be simplified.
> +
> + /* FP/SP alignment 8 bytes */
> + if (state->common.fp & 0x7 || state->common.sp & 0x7)
> + return -EINVAL;
> +
> + /*
> + * Most/all outermost functions are not visible to sframe. So, check for
> + * a meta frame record if the sframe lookup fails.
> + */
> + err = sframe_find_kernel(state->common.pc, &frame);
> + if (err)
> + return kunwind_next_frame_record_meta(state);
> +
> + if (frame.outermost)
> + return -ENOENT;
I don't think we ever expect an outermost frame within the kernel. We
haven't added any annotations for that, and we expect to unwind all the
way to a FRAME_META_TYPE_FINAL frame.
We cannot fall back to kunwind_next_frame_record_meta() here. We don't
know that the next frame is a meta frame (and this results in a warning
noted above), and we don't know the result is going to be reliable if we
don't have SFrame data, so the right thing to do is return an error.
I think this should be:
/*
* A kernel unwind should always end at a FRAME_META_TYPE_FINAL
* frame. There should be no outermost frames within the kernel.
*/
if (frame.outermost)
return -EINVAL;
err = sframe_find_kernel(state->common.pc, &frame);
if (err)
return -EINVAL;
> + /* Get the Canonical Frame Address (CFA) */
> + switch (frame.cfa.rule) {
> + case UNWIND_CFA_RULE_SP_OFFSET:
> + cfa = state->common.sp;
> + break;
> + case UNWIND_CFA_RULE_FP_OFFSET:
> + if (state->common.fp < state->common.sp)
> + return -EINVAL;
> + cfa = state->common.fp;
> + break;
> + case UNWIND_CFA_RULE_REG_OFFSET:
> + case UNWIND_CFA_RULE_REG_OFFSET_DEREF:
> + /* regs only available in topmost/interrupt frame */
> + if (!regs || frame.cfa.regnum > 30)
> + return -EINVAL;
> + cfa = regs->regs[frame.cfa.regnum];
> + break;
Do we ever expect to see UNWIND_CFA_RULE_REG_OFFSET or
UNWIND_CFA_RULE_REG_OFFSET_DEREF in practice for kernel code?
> + default:
> + WARN_ON_ONCE(1);
> + return -EINVAL;
> + }
> + cfa += frame.cfa.offset;
> +
> + /*
> + * CFA typically points to a higher address than RA or FP, so don't
> + * consume from the stack when we read it.
> + */
> + if (frame.cfa.rule & UNWIND_RULE_DEREF &&
> + !get_word(&state->common, &cfa))
> + return -EINVAL;
Per the switch above, this could only be
UNWIND_CFA_RULE_REG_OFFSET_DEREF. As above, do we ever expect to
encounter that in practice for kernel code?
> +
> + /* CFA alignment 8 bytes */
> + if (cfa & 0x7)
> + return -EINVAL;
If the CFA is the SP upon entry to the function, then per AAPCS64 rules
it should be aligned to 16 bytes. Otherwise, where has this 8 byte
alignment requirement come from? Does SFrame mandate that?
> +
> + /* Get the Return Address (RA) */
> + switch (frame.ra.rule) {
> + case UNWIND_RULE_RETAIN:
> + /* regs only available in topmost/interrupt frame */
> + if (!regs)
> + return -EINVAL;
> + ra = regs->regs[30];
> + source = KUNWIND_SOURCE_REGS_LR;
> + break;
> + /* UNWIND_USER_RULE_CFA_OFFSET not implemented on purpose */
It would be better for the comment to say *why* that's not implemented.
I assume that's because UNWIND_USER_RULE_CFA_OFFSET would mean that the return
address is a stack address, and that's obviously not legitimate.
> + case UNWIND_RULE_CFA_OFFSET_DEREF:
> + ra = cfa + frame.ra.offset;
> + break;
> + case UNWIND_RULE_REG_OFFSET:
> + case UNWIND_RULE_REG_OFFSET_DEREF:
> + /* regs only available in topmost/interrupt frame */
> + if (!regs)
> + return -EINVAL;
> + ra = regs->regs[frame.cfa.regnum];
> + ra += frame.ra.offset;
> + break;
Do we ever expect UNWIND_RULE_REG_OFFSET or UNWIND_RULE_REG_OFFSET_DEREF
in practice for kernel code?
I don't think we expect UNWIND_RULE_REG_OFFSET unless that's sometimes used
instead of UNWIND_RULE_RETAIN to express that the return address is in x30
(with zero offset).
Similarly, if the address is on the stack it should be in a frame
record. Would we ever expect UNWIND_RULE_REG_OFFSET_DEREF rather than
UNWIND_RULE_CFA_OFFSET_DEREF?
> + default:
> + WARN_ON_ONCE(1);
> + return -EINVAL;
> + }
> +
> + /* Get the Frame Pointer (FP) */
> + switch (frame.fp.rule) {
> + case UNWIND_RULE_RETAIN:
> + fp = state->common.fp;
> + break;
> + /* UNWIND_USER_RULE_CFA_OFFSET not implemented on purpose */
As for RA, the comment should explain why that's not implemented.
> + case UNWIND_RULE_CFA_OFFSET_DEREF:
> + fp = cfa + frame.fp.offset;
> + break;
> + case UNWIND_RULE_REG_OFFSET:
> + case UNWIND_RULE_REG_OFFSET_DEREF:
> + /* regs only available in topmost/interrupt frame */
> + if (!regs)
> + return -EINVAL;
> + fp = regs->regs[frame.fp.regnum];
> + fp += frame.fp.offset;
> + break;
> + default:
> + WARN_ON_ONCE(1);
> + return -EINVAL;
> + }
> +
> + /*
> + * Consume RA and FP from the stack. The frame record puts FP at a lower
> + * address than RA, so we always read FP first.
> + */
> + if (frame.fp.rule & UNWIND_RULE_DEREF &&
> + !get_word(&state->common, &fp))
> + return -EINVAL;
Why is this get_word() rather than get_consume_word()?
> +
> + if (frame.ra.rule & UNWIND_RULE_DEREF &&
> + get_consume_word(&state->common, &ra))
> + return -EINVAL;
> +
> + state->common.pc = ra;
> + state->common.sp = cfa;
As above, the SP can be removed.
> + state->common.fp = fp;
> +
> + state->source = source;
> +
> + return 0;
> +}
> +
> +#else /* !CONFIG_HAVE_UNWIND_KERNEL_SFRAME */
> +
> +static __always_inline int
> +unwind_next_frame_sframe(struct kunwind_state *state) { return -EINVAL; }
> +
> +#endif /* !CONFIG_HAVE_UNWIND_KERNEL_SFRAME*/
> +
> /*
> * Unwind from one frame record (A) to the next frame record (B).
> *
> @@ -259,12 +442,25 @@ kunwind_next(struct kunwind_state *state)
> state->flags.all = 0;
>
> switch (state->source) {
> + case KUNWIND_SOURCE_REGS_PC:
> + err = unwind_next_frame_sframe(state);
> +
> + if (err && err != -ENOENT) {
> + /* Fallback to FP based unwinder */
> + err = kunwind_next_frame_record(state);
> + state->common.unreliable = true;
> + }
> + state->regs = NULL;
> + break;
This makes sense to me.
> case KUNWIND_SOURCE_FRAME:
> case KUNWIND_SOURCE_CALLER:
> case KUNWIND_SOURCE_TASK:
> - case KUNWIND_SOURCE_REGS_PC:
> + case KUNWIND_SOURCE_REGS_LR:
> err = kunwind_next_frame_record(state);
> + if (err && err != -ENOENT)
> + err = unwind_next_frame_sframe(state);
This isn't sound as we cannot track the SP reliably across all
transitions.
If a regular frame pointer unwind has failed, something is already
wrong, and we should give up immediately.
Please remove the fallback to sframe here.
> break;
> +
> default:
No need for this whitespace change.
> err = -EINVAL;
> }
> @@ -350,6 +546,9 @@ kunwind_stack_walk(kunwind_consume_fn consume_state,
> .common = {
> .stacks = stacks,
> .nr_stacks = ARRAY_SIZE(stacks),
> +#ifdef CONFIG_HAVE_UNWIND_KERNEL_SFRAME
> + .sp = 0,
> +#endif
As above, this can go.
Thanks,
Mark.
> },
> };
>
> @@ -390,34 +589,40 @@ noinline noinstr void arch_stack_walk(stack_trace_consume_fn consume_entry,
> kunwind_stack_walk(arch_kunwind_consume_entry, &data, task, regs);
> }
>
> +struct kunwind_reliable_consume_entry_data {
> + stack_trace_consume_fn consume_entry;
> + void *cookie;
> + bool unreliable;
> +};
> +
> static __always_inline bool
> -arch_reliable_kunwind_consume_entry(const struct kunwind_state *state, void *cookie)
> +arch_kunwind_reliable_consume_entry(const struct kunwind_state *state, void *cookie)
> {
> - /*
> - * At an exception boundary we can reliably consume the saved PC. We do
> - * not know whether the LR was live when the exception was taken, and
> - * so we cannot perform the next unwind step reliably.
> - *
> - * All that matters is whether the *entire* unwind is reliable, so give
> - * up as soon as we hit an exception boundary.
> - */
> - if (state->source == KUNWIND_SOURCE_REGS_PC)
> - return false;
> + struct kunwind_reliable_consume_entry_data *data = cookie;
>
> - return arch_kunwind_consume_entry(state, cookie);
> + if (state->common.unreliable) {
> + data->unreliable = true;
> + return false;
> + }
> + return data->consume_entry(data->cookie, state->common.pc);
> }
>
> -noinline noinstr int arch_stack_walk_reliable(stack_trace_consume_fn consume_entry,
> - void *cookie,
> - struct task_struct *task)
> +noinline notrace int arch_stack_walk_reliable(
> + stack_trace_consume_fn consume_entry,
> + void *cookie, struct task_struct *task)
> {
> - struct kunwind_consume_entry_data data = {
> + struct kunwind_reliable_consume_entry_data data = {
> .consume_entry = consume_entry,
> .cookie = cookie,
> + .unreliable = false,
> };
>
> - return kunwind_stack_walk(arch_reliable_kunwind_consume_entry, &data,
> - task, NULL);
> + kunwind_stack_walk(arch_kunwind_reliable_consume_entry, &data, task, NULL);
> +
> + if (data.unreliable)
> + return -EINVAL;
> +
> + return 0;
> }
>
> struct bpf_unwind_consume_entry_data {
> @@ -452,6 +657,7 @@ static const char *state_source_string(const struct kunwind_state *state)
> case KUNWIND_SOURCE_CALLER: return "C";
> case KUNWIND_SOURCE_TASK: return "T";
> case KUNWIND_SOURCE_REGS_PC: return "P";
> + case KUNWIND_SOURCE_REGS_LR: return "L";
> default: return "U";
> }
> }
> --
> 2.54.0.545.g6539524ca2-goog
>
^ permalink raw reply
* Re: [PATCH v2 51/53] objtool/klp: Fix kCFI prefix finding/cloning
From: Song Liu @ 2026-05-01 12:17 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: x86, linux-kernel, live-patching, Peter Zijlstra, Joe Lawrence,
Miroslav Benes, Petr Mladek
In-Reply-To: <3a6674fa08e32ea6b02adf3e2de185b8ad99b01f.1777575753.git.jpoimboe@kernel.org>
On Fri, May 1, 2026 at 5:09 AM Josh Poimboeuf <jpoimboe@kernel.org> wrote:
>
> With CFI+CALL_PADDING, Clang places .Ltmp labels at the start of the NOP
> padding (offset 5) between the __cfi_ prefix and the function entry
> point. get_func_prefix() only checks the immediately previous symbol,
> so the intervening .Ltmp label causes it to miss the __cfi_ prefix
> symbol.
>
> This results in klp-diff not cloning the kCFI type hash into the
> livepatch module, causing a CFI failure at module load when calling
> callback functions through indirect calls:
>
> CFI failure at __klp_enable_patch+0xab/0x140
> (target: pre_patch_callback+0x0/0x80 [livepatch_combined];
> expected type: 0xde073954)
>
> Instead of walking backward through the section's symbol list, just use
> find_func_containing() for the byte before the function. This works now
> that __cfi_ symbols are being grown by objtool to fill the padding.
>
> Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org>
Acked-by: Song Liu <song@kernel.org>
^ permalink raw reply
* Re: [PATCH v2 49/53] objtool/klp: Fix position-dependent checksums for non-relocated jumps/calls
From: Song Liu @ 2026-05-01 12:16 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: x86, linux-kernel, live-patching, Peter Zijlstra, Joe Lawrence,
Miroslav Benes, Petr Mladek
In-Reply-To: <b9fe30f891bddaff919e48bc2f620f7f66fa98ca.1777575752.git.jpoimboe@kernel.org>
On Fri, May 1, 2026 at 5:09 AM Josh Poimboeuf <jpoimboe@kernel.org> wrote:
>
> When computing klp checksums, instructions with non-relocated jump/call
> destination offsets are problematic because the offset values can change
> when surrounding code has moved, causing the function to be incorrectly
> marked as changed.
>
> Specifically, that includes jumps from alternatives to the end of the
> alternative, which from objtool's perspective are jumps to the end of
> the alternative instruction block in the original function.
>
> Note that 'jump_dest' jumps don't include sibling calls (those use
> call_dest), nor do they include jumps to/from .cold sub functions (those
> are cross-section and need a reloc).
>
> Fix it by hashing the opcode bytes (excluding the immediate operand)
> along with a position-independent representation of the destination.
> For calls, use the function name, and for jumps, use the destination's
> offset within its function.
>
> [Note the "9 bit hole" comment was wrong: it has been 8 bits since
> commit 70589843b36f ("objtool: Add option to trace function validation")
> added the 'trace' field. Adding the 4-bit 'immediate_len' field now
> leaves a 4-bit hole.]
>
> Fixes: 0d83da43b1e1 ("objtool/klp: Add --checksum option to generate per-function checksums")
> Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org>
Acked-by: Song Liu <song@kernel.org>
^ permalink raw reply
* Re: [PATCH v2 48/53] objtool: Add insn_sym() helper
From: Song Liu @ 2026-05-01 12:11 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: x86, linux-kernel, live-patching, Peter Zijlstra, Joe Lawrence,
Miroslav Benes, Petr Mladek
In-Reply-To: <52633c62366f87d9b78ebd77873a08b9ac6d31c8.1777575752.git.jpoimboe@kernel.org>
On Fri, May 1, 2026 at 5:09 AM Josh Poimboeuf <jpoimboe@kernel.org> wrote:
>
> Alternative replacement instructions awkwardly have insn->sym set to the
> function they get patched to rather than the symbol (or rather lack
> thereof) they belong to in the file.
>
> This makes it difficult to know where a given instruction actually
> lives.
>
> Add a new insn_sym() helper which preserves the existing semantic of
> insn->sym. Rename insn->sym to insn->_sym, which contains the actual
> ELF binary symbol (or NULL, for alternative replacements) an instruction
> lives in.
>
> The private insn->_sym value will be needed for a subsequent patch.
>
> Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org>
Acked-by: Song Liu <song@kernel.org>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox