* Re: [PATCH v3 21/21] klp-build: Add arm64 syscall patching macro
From: sashiko-bot @ 2026-05-15 2:44 UTC (permalink / raw)
To: x86
Cc: Miroslav Benes, live-patching, Peter Zijlstra, Will Deacon,
Joe Lawrence, Catalin Marinas, linux-arm-kernel, Mark Rutland,
Song Liu, linux-kernel, Petr Mladek
In-Reply-To: <3bf3cee43039fc4da84e83d0891a92e25f40f00f.1778642121.git.jpoimboe@kernel.org>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The arm64 __KLP_SYSCALL_DEFINEx macro omits the ALLOW_ERROR_INJECTION declaration, silently breaking BPF error injection for livepatched syscalls.
--
commit b4d1a9ea352f196a7fd3e7198fdade51440d08e5
Author: Josh Poimboeuf <jpoimboe@kernel.org>
klp-build: Add arm64 syscall patching macro
This commit adds arm64 support for KLP_SYSCALL_DEFINEx(). The commit
message states it is mirroring the arm64 __SYSCALL_DEFINEx() pattern from
arch/arm64/include/asm/syscall_wrapper.h.
> diff --git a/include/linux/livepatch_helpers.h b/include/linux/livepatch_helpers.h
> index 99d68d0773fa8..4b647b83865f9 100644
> --- a/include/linux/livepatch_helpers.h
> +++ b/include/linux/livepatch_helpers.h
> @@ -72,6 +72,25 @@
> } \
> static inline long __klp_do_sys##name(__MAP(x,__SC_DECL,__VA_ARGS__))
>
> +#elif defined(CONFIG_ARM64)
> +
> +#define __KLP_SYSCALL_DEFINEx(x, name, ...) \
> + static long __se_sys##name(__MAP(x,__SC_LONG,__VA_ARGS__)); \
> + static inline long __klp_do_sys##name(__MAP(x,__SC_DECL,__VA_ARGS__));\
> + asmlinkage long __arm64_sys##name(const struct pt_regs *regs); \
> + asmlinkage long __arm64_sys##name(const struct pt_regs *regs) \
> + { \
> + return __se_sys##name(SC_ARM64_REGS_TO_ARGS(x,__VA_ARGS__));\
> + } \
Does this macro definition omit the ALLOW_ERROR_INJECTION declaration?
The standard arm64 __SYSCALL_DEFINEx() macro explicitly registers the top-level
syscall handler with ALLOW_ERROR_INJECTION() to enable BPF error injection
workflows like bpf_override_return(). The x86 implementation correctly
preserves this functionality.
Because this macro is missing that declaration, it looks like it might
introduce a regression where BPF cannot inject errors into the replacement
syscalls in a livepatch module on arm64.
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1778642120.git.jpoimboe@kernel.org?part=21
^ permalink raw reply
* Re: [PATCH v3 19/21] objtool/klp: Introduce objtool for arm64
From: sashiko-bot @ 2026-05-15 2:08 UTC (permalink / raw)
To: x86
Cc: live-patching, linux-kernel, Peter Zijlstra, Song Liu,
linux-arm-kernel, Petr Mladek, Joe Lawrence, Mark Rutland,
Will Deacon, Miroslav Benes, Catalin Marinas
In-Reply-To: <5b66146373b1ff1aba1318fd51867dea2eb882f6.1778642121.git.jpoimboe@kernel.org>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The arm64 instruction decoder misclassifies trap instructions (e.g., `brk #0x800` used for `BUG()`) as `INSN_OTHER` instead of `INSN_BUG`.
--
commit dd63f0c03bab6a47f34cfcb310454460653638ed
Author: Josh Poimboeuf <jpoimboe@kernel.org>
objtool/klp: Introduce objtool for arm64
This commit adds basic support for arm64 in objtool, initially enabling
only the "objtool klp" subcommands.
> diff --git a/tools/objtool/arch/arm64/decode.c b/tools/objtool/arch/arm64/decode.c
> new file mode 100644
> index 0000000000000..47658c76e1af0
> --- /dev/null
> +++ b/tools/objtool/arch/arm64/decode.c
[ ... ]
> +int arch_decode_instruction(struct objtool_file *file, const struct section *sec,
> + unsigned long offset, unsigned int maxlen,
> + struct instruction *insn)
> +{
[ ... ]
> + if (ins == 0xd503201f) {
> + /* NOP: static branch */
> + insn->type = INSN_NOP;
> + } else if ((ins & 0xfc000000) == 0x14000000) {
[ ... ]
> + } else if ((ins & 0x7e000000) == 0x36000000) {
> + /* TBZ/TBNZ: intra-TU sibling call */
> + insn->type = INSN_JUMP_CONDITIONAL;
> + insn->immediate = sign_extend64((ins >> 5) & 0x3fff, 13);
> + } else {
> + insn->type = INSN_OTHER;
> + }
Does this code misclassify trap instructions?
On arm64, BUG() is implemented using the brk instruction (such as brk #0x800).
Since the decoder doesn't appear to check for brk, it will fall through to the
else block and classify it as INSN_OTHER.
If INSN_BUG is misclassified as a normal instruction, won't this lose dead-end
propagation and corrupt objtool's control-flow analysis?
> +
> + insn->len = 4;
> + return 0;
> +}
--
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1778642120.git.jpoimboe@kernel.org?part=19
^ permalink raw reply
* Re: [PATCH v3 17/21] objtool/klp: Don't correlate arm64 mapping symbols
From: sashiko-bot @ 2026-05-15 1:19 UTC (permalink / raw)
To: x86
Cc: linux-kernel, Petr Mladek, Song Liu, live-patching, Mark Rutland,
Peter Zijlstra, linux-arm-kernel, Will Deacon, Miroslav Benes,
Catalin Marinas, Joe Lawrence
In-Reply-To: <48efc64058f667159d3dedf367c1d4cdedf84f1c.1778642120.git.jpoimboe@kernel.org>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The `is_mapping_sym()` function relies on an overly broad substring match (`strchr(sym->name, '$')`), which risks falsely classifying any unannotated (`STT_NOTYPE`) symbol containing a `$` as an ARM64 mapping symbol.
--
commit fb925cb10f7a9c6ab8ecefca87d3cd3b4e4da879
Author: Josh Poimboeuf <jpoimboe@kernel.org>
objtool/klp: Don't correlate arm64 mapping symbols
This commit updates the livepatch diff tool to skip the correlation of arm64
mapping symbols. These symbols mark transitions between code and data but are
not standard symbols that require correlation.
> diff --git a/tools/objtool/klp-diff.c b/tools/objtool/klp-diff.c
> index eb21f3bf3120b..e1d4d94c9d77c 100644
> --- a/tools/objtool/klp-diff.c
> +++ b/tools/objtool/klp-diff.c
> @@ -501,6 +501,7 @@ static bool dont_correlate(struct symbol *sym)
> is_prefix_func(sym) ||
> is_uncorrelated_static_local(sym) ||
> is_local_label(sym) ||
> + is_mapping_sym(sym) ||
Does this code risk falsely classifying user-defined or macro-generated
assembly labels as mapping symbols?
Looking at the definition:
tools/objtool/include/objtool/elf.h:is_mapping_sym() {
return is_notype_sym(sym) && strchr(sym->name, '$');
}
Because it relies on a broad substring match, could this exclude valid
unannotated STT_NOTYPE symbols containing a $ from correlation across all
architectures?
Triggering a livepatching failure would require an unusual edge case, but
would it be safer to use a stricter match for standard arm64 mapping symbol
formats like $x or $d?
> is_string_sec(sym->sec) ||
> is_anonymous_rodata(sym) ||
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1778642120.git.jpoimboe@kernel.org?part=17
^ permalink raw reply
* Re: [PATCH v3 14/21] objtool: Prevent kCFI hashes from being decoded as instructions
From: sashiko-bot @ 2026-05-15 0:16 UTC (permalink / raw)
To: x86
Cc: Will Deacon, Mark Rutland, Catalin Marinas, linux-arm-kernel,
live-patching, Petr Mladek, linux-kernel, Peter Zijlstra,
Miroslav Benes, Song Liu, Joe Lawrence
In-Reply-To: <b1d50c9fc9e6b9bca43833cc4ccbd88a31fed84b.1778642120.git.jpoimboe@kernel.org>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Build failure due to undeclared mapping symbol functions.
- [Low] Unused header inclusion.
--
commit 4ad78a24a64cd6a2d10b9a2f9c0d306fba3e6d19
Author: Josh Poimboeuf <jpoimboe@kernel.org>
objtool: Prevent kCFI hashes from being decoded as instructions
On arm64 with CONFIG_CFI=y, Clang places a 4-byte kCFI type hash immediately
before each address-taken function entry. This commit uses mapping symbols
to mark such data as NOP in text sections to prevent objtool from decoding
them.
> diff --git a/tools/objtool/check.c b/tools/objtool/check.c
> index e05dc7a93dc1e..2b03a2d6fc952 100644
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -25,6 +25,7 @@
> #include <linux/kernel.h>
> #include <linux/static_call_types.h>
> #include <linux/string.h>
> +#include <linux/kconfig.h>
This isn't a bug, but is this include needed? It doesn't appear that any
Kconfig configuration macros are being used in the modified code.
[ ... ]
> @@ -478,6 +483,16 @@ static int decode_instructions(struct objtool_file *file)
>
> prev_len = insn->len;
>
> + /* Use mapping symbols to skip data in text sections */
> + sec_for_each_sym_from(sec, map_sym) {
> + if (map_sym->offset > offset)
> + break;
> + if (is_mapping_sym(map_sym))
> + is_data = is_data_mapping_sym(map_sym);
Could this cause a build failure across architectures here? It appears
is_mapping_sym() and is_data_mapping_sym() are neither declared nor defined
anywhere in the codebase at this point in the patch series.
If these functions are only introduced in a subsequent commit, applying
this patch independently might result in an implicit declaration of function
error, breaking git bisectability.
> + }
> + if (is_data)
> + insn->type = INSN_NOP;
> +
> /*
--
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1778642120.git.jpoimboe@kernel.org?part=14
^ permalink raw reply
* Re: [PATCH v3 12/21] objtool: Refactor elf_add_data() to use a growable data buffer
From: sashiko-bot @ 2026-05-14 23:13 UTC (permalink / raw)
To: x86
Cc: linux-arm-kernel, live-patching, Will Deacon, Peter Zijlstra,
linux-kernel, Catalin Marinas, Joe Lawrence, Song Liu,
Mark Rutland, Petr Mladek, Miroslav Benes
In-Reply-To: <a0fe2363d017a2833e98ae50de797fe55c2796a4.1778642120.git.jpoimboe@kernel.org>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] An integer overflow in `elf_add_data()` due to an uncontrolled `sh_addralign` value allows an out-of-bounds heap write.
- [Low] In `elf_add_data`, a failed `calloc` for the new data buffer overwrites `sec->data->d_buf` with NULL and leaks the original buffer.
--
commit 02f18d399bba6e3d751363b4e85f507e3f288e34
Author: Josh Poimboeuf <jpoimboe@kernel.org>
objtool: Refactor elf_add_data() to use a growable data buffer
Instead of calling elf_newdata() for each new piece of data with its own
separate buffer, keep it all in the same growable buffer so the
section's entire data can be accessed if needed.
> diff --git a/tools/objtool/elf.c b/tools/objtool/elf.c
> --- a/tools/objtool/elf.c
> +++ b/tools/objtool/elf.c
[ ... ]
> @@ -1409,30 +1406,55 @@ void *elf_add_data(struct elf *elf, struct section *sec, const void *data, size_
> return NULL;
> }
>
> - sec->data = elf_newdata(s);
> if (!sec->data) {
[ ... ]
> + sec->data->d_align = sec->sh.sh_addralign;
> }
>
> - sec->data->d_buf = calloc(1, size);
> - if (!sec->data->d_buf) {
> - ERROR_GLIBC("calloc");
> - return NULL;
> + size_old = sec->data->d_size;
> + offset = ALIGN(size_old, sec->sh.sh_addralign);
> + size_new = offset + size;
Could this ALIGN calculation overflow if a malformed ELF file provides an
extremely large sh_addralign value?
If offset wraps around, it seems size_new might also wrap, potentially causing
roundup_pow_of_two to return a small allocation size. Could this lead to an
out-of-bounds write during the subsequent memcpy further down in this function?
> - if (data)
> - memcpy(sec->data->d_buf, data, size);
> + if (!sec->data_overallocated)
> + alloc_size_old = size_old;
> + else
> + alloc_size_old = max(64UL, roundup_pow_of_two(size_old ? : 1));
>
> - sec->data->d_size = size;
> - sec->data->d_align = sec->sh.sh_addralign;
> + alloc_size_new = max(64UL, roundup_pow_of_two(size_new ? : 1));
>
> - offset = ALIGN(sec_size(sec), sec->sh.sh_addralign);
> - sec->sh.sh_size = offset + size;
> + if (alloc_size_new > alloc_size_old) {
> + void *orig_buf = sec->data->d_buf;
> +
> + sec->data->d_buf = calloc(1, alloc_size_new);
> + if (!sec->data->d_buf) {
> + ERROR_GLIBC("calloc");
> + return NULL;
> + }
If calloc fails here, does assigning its result directly to sec->data->d_buf
clobber the valid pointer to orig_buf?
It appears orig_buf is never freed on this error path, which might leak the
original buffer if it was previously owned by objtool. Would it be safer to
keep the original buffer pointer intact on allocation failure?
--
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1778642120.git.jpoimboe@kernel.org?part=12
^ permalink raw reply
* Re: [RFC PATCH 6/6] livepatch: Support replace_set in shadow variable API
From: sashiko-bot @ 2026-05-14 23:01 UTC (permalink / raw)
To: jpoimboe, joe.lawrence, song, mbenes, jikos, pmladek
Cc: Yafang Shao, live-patching
In-Reply-To: <20260513143321.26185-7-laoar.shao@gmail.com>
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Newly allocated shadow variables fail to store the `replace_set` identifier, breaking all subsequent lookups and leading to unbounded memory leaks.
- [High] The `klp_shadow_combined_id()` function invokes undefined behavior and loses data on 32-bit architectures by shifting a 32-bit integer by 32 bits.
- [High] Changing the shadow API `id` parameter to `unsigned int` silently truncates 64-bit `klp_state` identifiers, leading to memory leaks.
- [Low] Modifying the `klp_shadow_*` function signatures breaks the build for in-tree callers.
--
commit 91116e31e667d0beea437b5048ab5a62099a5eed
Author: Yafang Shao <laoar.shao@gmail.com>
livepatch: Support replace_set in shadow variable API
To support more complex livepatching scenarios where multiple
replacement sets might coexist, extend the klp_shadow API to
include a replace_set identifier.
> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> index c9f2036dfefcf..cee35f5cae9ad 100644
> --- a/include/linux/livepatch.h
> +++ b/include/linux/livepatch.h
> @@ -192,15 +192,17 @@ static inline bool klp_have_reliable_stack(void)
> IS_ENABLED(CONFIG_HAVE_RELIABLE_STACKTRACE);
> }
>
> -void *klp_shadow_get(void *obj, unsigned long id);
> -void *klp_shadow_alloc(void *obj, unsigned long id,
> +void *klp_shadow_get(void *obj, unsigned int replace_set, unsigned int id);
> +void *klp_shadow_alloc(void *obj, unsigned int replace_set, unsigned int id,
> size_t size, gfp_t gfp_flags,
> klp_shadow_ctor_t ctor, void *ctor_data);
Does this signature change break the build for existing in-tree callers?
It looks like samples/livepatch/livepatch-shadow-fix1.c and
tools/testing/selftests/livepatch/test_modules/test_klp_shadow_vars.c
might not have been updated to pass the new replace_set argument.
[ ... ]
> diff --git a/kernel/livepatch/shadow.c b/kernel/livepatch/shadow.c
> index c2e724d97ddf2..35e507fae4458 100644
> --- a/kernel/livepatch/shadow.c
> +++ b/kernel/livepatch/shadow.c
> @@ -59,6 +60,11 @@ struct klp_shadow {
> char data[];
> };
>
> +static unsigned long klp_shadow_combined_id(unsigned int set, unsigned int id)
> +{
> + return ((unsigned long)set << 32) | id;
> +}
On 32-bit architectures that support livepatching (such as 32-bit PowerPC),
unsigned long is 32 bits wide. Does shifting a 32-bit integer by 32 bits
invoke undefined behavior here?
Furthermore, since struct klp_shadow->id remains defined as an unsigned long,
it cannot physically store a 64-bit combined ID on these systems. The
replace_set value might be entirely truncated, resulting in hash collisions
and subsequent issues when multiple sets manage the same resource.
[ ... ]
> @@ -126,7 +134,7 @@ static void *__klp_shadow_get_or_alloc(void *obj, unsigned long id,
>
> /* Look for <obj, id> again under the lock */
> spin_lock_irqsave(&klp_shadow_lock, flags);
> - shadow_data = klp_shadow_get(obj, id);
> + shadow_data = klp_shadow_get(obj, set, id);
> if (unlikely(shadow_data)) {
> /*
> * Shadow variable was found, throw away speculative
> @@ -147,8 +155,8 @@ static void *__klp_shadow_get_or_alloc(void *obj, unsigned long id,
[ ... ]
> }
>
> new_shadow->obj = obj;
> new_shadow->id = id;
Because the id argument is now a 32-bit integer, the upper 32 bits of
new_shadow->id will be zero. Should this be initialized using
klp_shadow_combined_id(set, id) instead?
Lookup functions like klp_shadow_get() and klp_shadow_free() search using
the 64-bit composite ID klp_shadow_combined_id(replace_set, id). When
replace_set is non-zero, this lookup will always fail.
This means klp_shadow_get() will erroneously return NULL, and
klp_shadow_get_or_alloc() could duplicate variables indefinitely, leading
to unbounded memory leaks.
[ ... ]
> diff --git a/kernel/livepatch/state.c b/kernel/livepatch/state.c
> index 43115e8e8453c..6e3d6fb92e64a 100644
> --- a/kernel/livepatch/state.c
> +++ b/kernel/livepatch/state.c
> @@ -203,7 +203,8 @@ void klp_states_post_unpatch(struct klp_patch *patch)
> state->callbacks.post_unpatch(patch, state);
>
> if (state->is_shadow)
> - klp_shadow_free_all(state->id, state->callbacks.shadow_dtor);
> + klp_shadow_free_all(patch->replace_set, state->id,
> + state->callbacks.shadow_dtor);
The shadow variable APIs previously accepted an unsigned long for the id,
allowing 64-bit identifiers. The new klp_shadow_free_all() now accepts an
unsigned int, but struct klp_state still defines id as an unsigned long.
Will the implicit cast from unsigned long to unsigned int silently truncate
the upper 32 bits of state->id on 64-bit platforms?
If the original ID exceeded 32 bits, the core might fail to locate and free
the correct shadow variables, causing a permanent memory leak during
garbage collection.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260513143321.26185-1-laoar.shao@gmail.com?part=6
^ permalink raw reply
* Re: [RFC PATCH 5/6] livepatch: Remove obsolete per-object callbacks
From: sashiko-bot @ 2026-05-14 22:40 UTC (permalink / raw)
To: jikos, joe.lawrence, mbenes, song, pmladek, jpoimboe
Cc: Yafang Shao, live-patching
In-Reply-To: <20260513143321.26185-6-laoar.shao@gmail.com>
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] Out-of-bounds ELF relocations causing memory corruption
- [High] Silent failure of livepatch state callbacks due to missing initialization
- [Low] Build failures due to unmigrated selftests and samples
--
commit deb761bd65e9a96a95798282bde72ddb362e1c0d
Author: Yafang Shao <laoar.shao@gmail.com>
livepatch: Remove obsolete per-object callbacks
This commit removes obsolete per-object callbacks from the livepatch framework
in favor of per-state callbacks. It drops the callback fields from the klp
object structures and updates objtool and the livepatch core scripts.
> All selftests have been migrated to the new per-state callbacks, making the
> per-object callbacks redundant.
Is this description accurate?
The callbacks field was removed from struct klp_object, but if modules like
samples/livepatch/livepatch-callbacks-demo.c or the selftests still use it
in their klp_object declarations, won't this cause build failures?
> diff --git a/scripts/livepatch/init.c b/scripts/livepatch/init.c
> index 9265e0e504e0c..570366b9268c4 100644
> --- a/scripts/livepatch/init.c
> +++ b/scripts/livepatch/init.c
> @@ -63,8 +63,6 @@ static int __init livepatch_mod_init(void)
>
> obj->name = obj_ext->name;
> obj->funcs = funcs;
> -
> - memcpy(&obj->callbacks, &obj_ext->callbacks, sizeof(struct klp_callbacks));
> }
>
> patch->mod = THIS_MODULE;
Will the state callbacks be executed when the patch is enabled?
Looking at livepatch_mod_init() after these changes, patch->states is never
initialized:
scripts/livepatch/init.c:livepatch_mod_init() {
...
patch->mod = THIS_MODULE;
patch->objs = objs;
/* TODO patch->states */
patch->replace_set = KLP_REPLACE_TAG;
return klp_enable_patch(patch);
}
When klp_states_pre_patch() iterates over patch->states, won't it silently
skip the uninitialized callbacks?
> diff --git a/tools/objtool/klp-diff.c b/tools/objtool/klp-diff.c
> index 4d1f9e9977eb9..6e2762de3d512 100644
> --- a/tools/objtool/klp-diff.c
> +++ b/tools/objtool/klp-diff.c
[ ... ]
> @@ -1566,8 +1566,8 @@ static int create_klp_sections(struct elfs *e)
> reloc = find_reloc_by_dest(e->out, sym->sec, sym->offset);
>
> if (!elf_create_reloc(e->out, obj_sec,
> - offsetof(struct klp_object_ext, callbacks) +
> - offsetof(struct klp_callbacks, pre_patch),
> + offsetof(struct klp_state_ext, callbacks) +
> + offsetof(struct klp_state_callbacks, pre_patch),
> reloc->sym, reloc_addend(reloc), R_ABS64))
> return -1;
> }
Does this result in out-of-bounds memory writes during ELF relocation?
Earlier in create_klp_sections(), obj_sec is allocated using the size of
struct klp_object_ext, which is 24 bytes:
tools/objtool/klp-diff.c:create_klp_sections() {
size_t obj_size = sizeof(struct klp_object_ext);
...
obj_sec = elf_create_section_pair(e->out, KLP_OBJECTS_SEC, obj_size, 0, 0);
However, the new relocations are created using offsets from a completely
different structure, struct klp_state_ext.
Because offsetof(struct klp_state_ext, callbacks) is 16, and the callback
offsets go up to 24, this applies relocations at offsets 16, 24, 32, and 40
within obj_sec.
Won't the relocation at offset 16 corrupt klp_object_ext.nr_funcs, and the
subsequent relocations write entirely out of bounds of the 24-byte obj_sec
buffer?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260513143321.26185-1-laoar.shao@gmail.com?part=5
^ permalink raw reply
* Re: [PATCH v3 10/21] objtool: Ignore jumps to the end of the function for checksum runs
From: sashiko-bot @ 2026-05-14 22:30 UTC (permalink / raw)
To: x86
Cc: Mark Rutland, live-patching, linux-arm-kernel, linux-kernel,
Miroslav Benes, Petr Mladek, Will Deacon, Song Liu,
Peter Zijlstra, Joe Lawrence, Catalin Marinas
In-Reply-To: <b3b58101e15e1bb5266e57134f0b65f7d8efdd4b.1778642120.git.jpoimboe@kernel.org>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Missing parentheses in the jump destination check causes objtool to silently ignore all missing jump targets when branch validation is disabled, instead of only jumps to the end of the function.
--
commit aa068d4053e8b92ff17082c7a023821e72f5d625
Author: Josh Poimboeuf <jpoimboe@kernel.org>
objtool: Ignore jumps to the end of the function for checksum runs
Sometimes Clang arm64 code jumps to the end of the function for UB.
This commit ignores these jumps to the end of the function for
checksum runs to avoid errors.
> diff --git a/tools/objtool/check.c b/tools/objtool/check.c
> index 10b18cf9c3608..73451aef68029 100644
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
[ ... ]
> @@ -1593,10 +1609,14 @@ static int add_jump_destinations(struct objtool_file *file)
> /*
> * GCOV/KCOV dead code can jump to the end of
> * the function/section.
> + *
> + * Clang on arm64 also does this sometimes for
> + * undefined behavior.
> */
> - if (file->ignore_unreachables && func &&
> - dest_sec == insn->sec &&
> - dest_off == func->offset + func->len)
> + if (!validate_branch_enabled() ||
> + (file->ignore_unreachables && func &&
> + dest_sec == insn->sec &&
> + dest_off == func->offset + func->len))
> continue;
Does this condition silently ignore all missing jump targets when branch
validation is disabled?
Because of the || without proper parenthetical grouping around the first
two terms, if !validate_branch_enabled() is true (which it is for
--checksum or --noinstr runs), the rest of the condition short-circuits.
This appears to cause objtool to skip the error reporting and silently
leave the jump destination as NULL for any unresolvable jump in the binary,
instead of only those jumping to the end of the function.
Should the logic be grouped to restrict the exception to the end of the
function?
if ((!validate_branch_enabled() || file->ignore_unreachables) &&
func &&
dest_sec == insn->sec &&
dest_off == func->offset + func->len)
--
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1778642120.git.jpoimboe@kernel.org?part=10
^ permalink raw reply
* Re: [PATCH v3 09/21] kbuild: Only run objtool if there is at least one command
From: sashiko-bot @ 2026-05-14 22:25 UTC (permalink / raw)
To: x86
Cc: Peter Zijlstra, Song Liu, Catalin Marinas, linux-arm-kernel,
linux-kernel, Joe Lawrence, Will Deacon, Miroslav Benes,
Petr Mladek, Nicolas Schier, live-patching, Nathan Chancellor,
Mark Rutland
In-Reply-To: <8699672b82fef17e73a5f2e5528478778dbdab31.1778642120.git.jpoimboe@kernel.org>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] Leaking objtool options into vmlinux.o validation causes a fatal build failure (`--cfi requires --prefix`) and silently alters validation behavior.
--
commit 68e454f3a8ccd3d638c67c092eee264996286695
Author: Josh Poimboeuf <jpoimboe@kernel.org>
kbuild: Only run objtool if there is at least one command
This commit splits the objtool arguments into commands and options to
prevent objtool from running when no commands are enabled.
> diff --git a/scripts/Makefile.vmlinux_o b/scripts/Makefile.vmlinux_o
> index 527352c222ff6..09af33203bd8d 100644
> --- a/scripts/Makefile.vmlinux_o
> +++ b/scripts/Makefile.vmlinux_o
[ ... ]
> @@ -36,18 +36,13 @@ endif
> # For !delay-objtool + CONFIG_NOINSTR_VALIDATION, it runs on both translation
> # units and vmlinux.o, with the latter only used for noinstr/unret validation.
>
> -objtool-enabled := $(or $(delay-objtool),$(CONFIG_NOINSTR_VALIDATION))
> -
> -ifeq ($(delay-objtool),y)
> -vmlinux-objtool-args-y += $(objtool-args-y)
> -else
> -vmlinux-objtool-args-$(CONFIG_OBJTOOL_WERROR) += --werror
> +ifneq ($(delay-objtool),y)
> +objtool-cmds-y =
> +objtool-opts-y += --link
Does this code unintentionally retain the options from scripts/Makefile.lib?
Since objtool-cmds-y is cleared here, commands like --prefix are dropped.
However, because objtool-opts-y is only appended to, options like --cfi
remain active.
This causes objtool to fail with a fatal error: "--cfi requires --prefix"
during vmlinux.o validation on configurations with KCFI.
Should objtool-opts-y be assigned directly using = or := instead of
appended?
> endif
--
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1778642120.git.jpoimboe@kernel.org?part=9
^ permalink raw reply
* Re: [RFC PATCH 4/6] livepatch: Remove "data" from struct klp_state
From: sashiko-bot @ 2026-05-14 22:22 UTC (permalink / raw)
To: jpoimboe, pmladek, joe.lawrence, song, mbenes, jikos; +Cc: live-patching
In-Reply-To: <20260513143321.26185-5-laoar.shao@gmail.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Removing `data` from `struct klp_state` breaks compilation of the livepatch selftests and leaves kernel documentation explicitly referencing a non-existent API.
--
commit 792ddcf269c47d4de395b3873cfd5f02cbf282ef
Author: Petr Mladek <pmladek@suse.com>
livepatch: Remove "data" from struct klp_state
The commit removes the data pointer from struct klp_state since its lifetime
is tied to the module rather than the state. This pointer is lost during a
livepatch replacement, so shadow variables are recommended instead.
> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> index fef0c2f970838..56b7e4fe01bc8 100644
> --- a/include/linux/livepatch.h
> +++ b/include/linux/livepatch.h
> @@ -152,14 +152,12 @@ struct klp_state_callbacks {
> * @callbacks: optional callbacks used when enabling or disabling the state
> * @is_shadow: the state handles lifetime of a shadow variable with
> * the same @id
> - * @data: custom data
> */
> struct klp_state {
> unsigned long id;
> unsigned int version;
> struct klp_state_callbacks callbacks;
> bool is_shadow;
> - void *data;
Does this code removal cause a regression by breaking the compilation of
livepatch selftests?
For example, test_klp_state.c and test_klp_state2.c actively use this field
to store and transfer state.
In tools/testing/selftests/livepatch/test_modules/test_klp_state.c:
allocate_loglevel_state() {
...
loglevel_state->data = kzalloc(sizeof(console_loglevel), GFP_KERNEL);
if (!loglevel_state->data)
return -ENOMEM;
...
}
Additionally, does Documentation/livepatch/system-state.rst still reference
this non-existent API? It continues to provide instructions like:
In Documentation/livepatch/system-state.rst:
- Allocate *state->data* when necessary. The allocation might fail
...
- Make sure that *state->data* has all necessary information.
Should the selftests and documentation be updated as part of this change to
prevent build failures and confusion?
> };
>
> /**
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260513143321.26185-1-laoar.shao@gmail.com?part=4
^ permalink raw reply
* Re: [RFC PATCH 3/6] livepatch: Allow to handle lifetime of shadow variables using the livepatch state
From: sashiko-bot @ 2026-05-14 22:07 UTC (permalink / raw)
To: mbenes, jpoimboe, joe.lawrence, pmladek, song, jikos; +Cc: live-patching
In-Reply-To: <20260513143321.26185-4-laoar.shao@gmail.com>
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Shadow variables and state are permanently leaked during an atomic replace if multiple replaced patches share the same state.
- [High] Tying shadow variable lifetime to the patch's global state causes shadow variable leaks and use-after-free risks for loadable module objects.
- [High] The new `shadow_dtor` callback in `struct klp_state_callbacks` executes in atomic context, creating a severe API trap that leads to kernel panics.
--
commit e19c79edd4d439e568084dd175307ac754ab4007
Author: Petr Mladek <pmladek@suse.com>
livepatch: Allow to handle lifetime of shadow variables using the livepatch state
This commit introduces a new mechanism to associate the lifetime of
shadow variables with the livepatch state. Obsolete shadow variables
are automatically freed during state unpatching using a new shadow_dtor
callback, reducing manual cleanup overhead.
> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> index 248e1a388818e..fef0c2f970838 100644
> --- a/include/linux/livepatch.h
> +++ b/include/linux/livepatch.h
[ ... ]
> @@ -135,6 +141,7 @@ struct klp_state_callbacks {
> void (*post_patch)(struct klp_patch *patch, struct klp_state *state);
> void (*pre_unpatch)(struct klp_patch *patch, struct klp_state *state);
> void (*post_unpatch)(struct klp_patch *patch, struct klp_state *state);
> + klp_shadow_dtor_t shadow_dtor;
Will this callback run in an atomic context, unlike the other state callbacks?
All existing state callbacks execute in standard process context and are
permitted to sleep. However, looking at klp_shadow_free_all(), it invokes
the destructor while holding klp_shadow_lock:
kernel/livepatch/shadow.c:klp_shadow_free_all() {
...
spin_lock_irqsave(&klp_shadow_lock, flags);
hash_for_each(klp_shadow_hash, i, shadow, node) {
if (klp_shadow_match(shadow, shadow->obj, id))
klp_shadow_free_struct(shadow, dtor);
}
spin_unlock_irqrestore(&klp_shadow_lock, flags);
}
If a user assumes this callback runs in process context like the other
state callbacks and includes sleeping operations, couldn't this trigger
a scheduling while atomic panic?
> bool pre_patch_succeeded;
> };
>
> @@ -143,12 +150,15 @@ struct klp_state_callbacks {
> * @id: system state identifier (non-zero)
> * @version: version of the change
> * @callbacks: optional callbacks used when enabling or disabling the state
> + * @is_shadow: the state handles lifetime of a shadow variable with
> + * the same @id
Does tying shadow variable lifetime to the global patch state cause leaks when
dealing with loadable modules?
If a livepatch applies to a loadable module, it may allocate shadow variables
keyed to the module's dynamically allocated objects.
When the target module is unloaded, klp_module_going() unpatches the module's
objects. However, because the state is a global property of the patch, no
state callbacks are invoked and the state is not considered obsolete. This
leaves the shadow variables for the unloaded module orphaned in the global
hash table.
If the module is subsequently reloaded and allocates new objects at the same
memory addresses, wouldn't klp_shadow_alloc() fail with a duplicate warning,
or klp_shadow_get() incorrectly return stale data?
> * @data: custom data
> */
[ ... ]
> diff --git a/kernel/livepatch/state.c b/kernel/livepatch/state.c
> index a90c24d79084b..43115e8e8453c 100644
> --- a/kernel/livepatch/state.c
> +++ b/kernel/livepatch/state.c
> @@ -202,6 +202,9 @@ void klp_states_post_unpatch(struct klp_patch *patch)
> if (state->callbacks.post_unpatch)
> state->callbacks.post_unpatch(patch, state);
>
> + if (state->is_shadow)
> + klp_shadow_free_all(state->id, state->callbacks.shadow_dtor);
> +
Can this permanently leak shadow variables and state during an atomic replace?
Livepatch supports stacking, allowing multiple active patches (e.g., Patch A
and Patch B) to define the same state. If an atomic replace patch (Patch C)
is loaded, it replaces both A and B.
During the transition completion, klp_complete_transition() calls
klp_states_post_unpatch_replaced(), which iterates over the replaced
patches and calls klp_states_post_unpatch() for each.
For Patch A, klp_states_post_unpatch() calls is_state_in_other_patches().
Since Patch B is also still in the klp_patches list at this point (they are
not removed until klp_free_replaced_patches_async() runs later), the function
returns true and skips the cleanup block.
For Patch B, the same check finds Patch A in the list, returns true, and
also skips cleanup. Both patches are subsequently removed from the list,
permanently leaking the shadow variables and state cleanup logic.
> state->callbacks.pre_patch_succeeded = 0;
> }
> }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260513143321.26185-1-laoar.shao@gmail.com?part=3
^ permalink raw reply
* Re: [RFC 4/4] livepatch/klp-build: add basic out-of-tree module patching support
From: sashiko-bot @ 2026-05-14 21:20 UTC (permalink / raw)
To: live-patching; +Cc: Petr Mladek, Miroslav Benes, Josh Poimboeuf, Song Liu
In-Reply-To: <20260512221102.2720763-5-joe.lawrence@redhat.com>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The script hardcodes the temporary directory (klp-tmp) and default output file to the kernel root directory ($PWD), which breaks building out-of-tree modules against read-only system headers.
- [Medium] The commit message incorrectly states that 'make modules_prepare' is sufficient to prepare the kernel tree for out-of-tree patching.
--
commit c21e20dbe33c0ce1976f54b57ae6fc8cea53b65b
Author: Joe Lawrence <joe.lawrence@redhat.com>
livepatch/klp-build: add basic out-of-tree module patching support
This commit introduces a module-dir option to klp-build to support
generating livepatches for out-of-tree kernel modules. It performs two
builds of the out-of-tree module to generate the original and patched
objects.
> klp-build is currently limited to patching in-tree kernel modules.
> Introduce a -M/--module-dir option to enable livepatch generation for
> basic out-of-tree (OOT) modules. This requires the associated kernel
> tree to be pre-configured (e.g., 'make modules_prepare').
The commit message states that running 'make modules_prepare' is
sufficient to prepare the tree. Is this description accurate?
Looking at do_init(), the script explicitly checks for Module.symvers:
[[ -f "$PWD/Module.symvers" ]] ||
die "kernel must be built first (no Module.symvers in $PWD)"
Running 'make modules_prepare' on a clean kernel source tree prepares
configuration and headers but does not generate Module.symvers.
Could the commit message be updated to clarify that a full build is
required?
> diff --git a/scripts/livepatch/klp-build b/scripts/livepatch/klp-build
> index c4a7acf8edc3f..aef3b47db4f24 100755
> --- a/scripts/livepatch/klp-build
> +++ b/scripts/livepatch/klp-build
[ ... ]
> @@ -433,8 +451,6 @@ validate_patches() {
> }
>
> do_init() {
> - # We're not yet smart enough to handle anything other than in-tree
> - # builds in pwd.
> [[ ! "$PWD" -ef "$SCRIPT_DIR/../.." ]] && die "please run from the kernel root directory"
[ ... ]
> + if [[ -n "$MODULE_DIR" ]]; then
> + [[ -d "$MODULE_DIR" ]] || die "module directory not found: $MODULE_DIR"
> + MODULE_DIR="$(realpath "$MODULE_DIR")"
> + [[ -f "$MODULE_DIR/Kbuild" || -f "$MODULE_DIR/Makefile" ]] ||
> + die "no Kbuild or Makefile in $MODULE_DIR"
> + [[ -f "$PWD/Module.symvers" ]] ||
> + die "kernel must be built first (no Module.symvers in $PWD)"
> + fi
> +
> (( SHORT_CIRCUIT <= 1 )) && rm -rf "$TMP_DIR"
> mkdir -p "$TMP_DIR"
Does creating the temporary directory in the kernel root directory cause
permission regressions when building out-of-tree modules?
Out-of-tree modules are often built against system-installed kernel headers,
which are typically read-only for standard users. Since the script mandates
running from the kernel root directory, creating the temporary working
directory and default output file inside the present working directory will
cause permission denied errors.
Could the script use the module directory or a user-specified path for
temporary files and the default output instead?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260512221102.2720763-1-joe.lawrence@redhat.com?part=4
^ permalink raw reply
* Re: [RFC 3/4] objtool/klp: add --arch option to display target architecture
From: sashiko-bot @ 2026-05-14 21:10 UTC (permalink / raw)
To: live-patching; +Cc: Song Liu, Petr Mladek, Josh Poimboeuf, Miroslav Benes
In-Reply-To: <20260512221102.2720763-4-joe.lawrence@redhat.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The `--arch` option is inaccessible on non-x86 architectures because the `klp` subcommand is conditionally compiled only for x86.
--
commit bd45d7b713ebb8e72f88be16df165f29efd9fc96
Author: Joe Lawrence <joe.lawrence@redhat.com>
objtool/klp: add --arch option to display target architecture
This commit adds an --arch option to the klp diff subcommand to print the
target architecture objtool was compiled for. This allows scripts to verify
compatibility before processing object files.
> diff --git a/tools/objtool/klp-diff.c b/tools/objtool/klp-diff.c
> --- a/tools/objtool/klp-diff.c
> +++ b/tools/objtool/klp-diff.c
[ ... ]
> @@ -44,6 +44,7 @@ static const char * const klp_diff_usage[] = {
>
> static const struct option klp_diff_options[] = {
> OPT_GROUP("Options:"),
> + OPT_BOOLEAN('a', "arch", &show_arch, "display target architecture"),
> OPT_BOOLEAN('d', "debug", &debug, "enable all debug output"),
Since the klp subcommand is conditionally compiled and only enabled for x86
architectures, will this option be accessible on other architectures?
According to the objtool Makefile, ARCH_HAS_KLP is strictly set to y
only when SRCARCH is x86. For all other architectures, BUILD_KLP is
disabled. This means that on non-x86 architectures, invoking
objtool klp diff --arch will fall back to the weak stub, returning an
"objtool: klp not implemented" error instead of printing the
architecture.
Since the architecture is a global property of the binary, would it make
more sense to expose this via a global flag like objtool --arch instead of
restricting it to the klp subcommand?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260512221102.2720763-1-joe.lawrence@redhat.com?part=3
^ permalink raw reply
* Re: [RFC 2/4] objtool/klp: allow special section entry size overrides
From: sashiko-bot @ 2026-05-14 20:58 UTC (permalink / raw)
To: live-patching; +Cc: Miroslav Benes, Petr Mladek, Josh Poimboeuf, Song Liu
In-Reply-To: <20260512221102.2720763-3-joe.lawrence@redhat.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The custom entry size logic bypasses section size validation, allowing the creation of out-of-bounds ELF symbols.
--
commit 07468f70445bbc595d50f5edb50b5c394dc62f05
Author: Joe Lawrence <joe.lawrence@redhat.com>
objtool/klp: allow special section entry size overrides
This commit allows users to override compiled-in defaults for special
section entry sizes via environment variables. This helps process cached
unit test objects built from different kernel versions where the compiled-in
sizes may not match the actual entry sizes.
> diff --git a/tools/objtool/klp-diff.c b/tools/objtool/klp-diff.c
> index 0774ab312d033..84c00b673085b 100644
> --- a/tools/objtool/klp-diff.c
> +++ b/tools/objtool/klp-diff.c
[ ... ]
> @@ -1700,6 +1716,21 @@ static int create_fake_symbols(struct elf *elf)
> }
>
> entry_size = sec->sh.sh_entsize;
> +
> + /*
> + * Some special sections have multiple relocs per entry,
> + * so the reloc-based heuristic below doesn't work. Use
> + * the arch-defined entry sizes for known special sections.
> + */
> + if (!entry_size) {
> + if (!strcmp(sec->name, ".altinstructions"))
> + entry_size = entry_size_from_env("ALT_ENTRY_SIZE", ALT_ENTRY_SIZE);
> + else if (!strcmp(sec->name, "__jump_table"))
> + entry_size = entry_size_from_env("JUMP_ENTRY_SIZE", JUMP_ENTRY_SIZE);
> + else if (!strcmp(sec->name, "__ex_table"))
> + entry_size = entry_size_from_env("EX_ENTRY_SIZE", EX_ENTRY_SIZE);
> + }
> +
> if (!entry_size) {
> entry_size = arch_reloc_size(sec->rsec->relocs);
> if (sec_size(sec) != entry_size * sec_num_entries(sec->rsec)) {
If entry_size is populated from the environment variables, it skips this
validation check entirely.
If an invalid entry size that isn't a clean divisor of the section size is
provided, could this lead to an out-of-bounds memory access regression?
> ERROR("%s: missing special section entsize or annotations", sec->name);
> return -1;
> }
> }
>
> for (offset = 0; offset < sec_size(sec); offset += entry_size) {
> if (create_fake_symbol(elf, sec, offset, entry_size))
> return -1;
> }
For example, if sec_size(sec) is 20 and entry_size is provided as 12, the
loop will execute for offset 0 and offset 12. During the second iteration,
the fake symbol will extend from 12 to 24, which exceeds the section bounds
and may cause out-of-bounds accesses during subsequent objtool passes.
Should there be an explicit validation check to ensure the overridden
entry_size divides evenly into sec_size(sec) before proceeding to this loop?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260512221102.2720763-1-joe.lawrence@redhat.com?part=2
^ permalink raw reply
* Re: [RFC PATCH 1/6] livepatch: Support scoped atomic replace using replace set
From: sashiko-bot @ 2026-05-14 20:54 UTC (permalink / raw)
To: mbenes, song, jikos, jpoimboe, pmladek, joe.lawrence
Cc: Yafang Shao, live-patching
In-Reply-To: <20260513143321.26185-2-laoar.shao@gmail.com>
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Module reference leak during forced transition.
- [High] Silent semantic reversion for zero-initialized out-of-tree livepatches.
- [High] Sysfs ABI breakage and incorrect type formatting for the `replace` attribute.
- [Medium] Build failure in livepatch selftests.
--
commit 8a05e8a3ebd9241541926ac6aba19452a820c899
Author: Yafang Shao <laoar.shao@gmail.com>
livepatch: Support scoped atomic replace using replace set
This commit converts the boolean replace attribute to a u32 replace_set
to allow livepatches to atomically replace only existing patches within
the same set. Patches with different replace sets can now remain active
simultaneously.
> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> --- a/include/linux/livepatch.h
> +++ b/include/linux/livepatch.h
[ ... ]
> @@ -137,7 +138,7 @@ struct klp_patch {
> struct module *mod;
> struct klp_object *objs;
> struct klp_state *states;
> - bool replace;
> + unsigned int replace_set;
By removing the boolean replace flag, out-of-tree livepatches that
zero-initialize their patch structures will now default to a replace_set of 0.
Will this cause them to unintentionally replace all other zero-initialized
patches instead of stacking safely?
Additionally, does this rename break the livepatch selftests in
tools/testing/selftests/livepatch/test_modules/ (such as test_klp_state2.c),
which still explicitly set patch.replace = true?
[ ... ]
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -454,7 +454,7 @@ static ssize_t replace_show(struct kobject *kobj,
> struct klp_patch *patch;
>
> patch = container_of(kobj, struct klp_patch, kobj);
> - return sysfs_emit(buf, "%d\n", patch->replace);
> + return sysfs_emit(buf, "%d\n", patch->replace_set);
Does changing the sysfs output from a boolean to a replace set ID break
compatibility for userspace tools expecting strict 0 or 1 values from the
replace attribute?
Also, since replace_set is declared as an unsigned int, will using the
signed %d format specifier cause large replace set IDs to overflow and
display as negative numbers?
[ ... ]
> diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
> --- a/kernel/livepatch/transition.c
> +++ b/kernel/livepatch/transition.c
[ ... ]
> @@ -720,11 +720,11 @@ void klp_force_transition(void)
> klp_update_patch_state(idle_task(cpu));
>
> /* Set forced flag for patches being removed. */
> - if (klp_target_state == KLP_TRANSITION_UNPATCHED)
> + if (klp_target_state == KLP_TRANSITION_UNPATCHED) {
> klp_transition_patch->forced = true;
> - else if (klp_transition_patch->replace) {
> + } else {
> klp_for_each_patch(patch) {
> - if (patch != klp_transition_patch)
> + if (patch->replace_set == klp_transition_patch->replace_set)
> patch->forced = true;
> }
> }
Could this inadvertently set forced = true on the klp_transition_patch
itself?
Since klp_transition_patch was already added to the global list during
klp_init_patch(), it will match its own replace_set here. If it is marked
as forced during a patching transition, wouldn't we leak its module reference
in klp_free_patch_finish() when it is cleanly disabled later?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260513143321.26185-1-laoar.shao@gmail.com?part=1
^ permalink raw reply
* Re: [RFC 1/4] objtool/klp: add --symvers option to klp diff
From: sashiko-bot @ 2026-05-14 20:47 UTC (permalink / raw)
To: live-patching; +Cc: Song Liu, Petr Mladek, Josh Poimboeuf, Miroslav Benes
In-Reply-To: <20260512221102.2720763-2-joe.lawrence@redhat.com>
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] NULL pointer dereference and crash when the implicitly generated `--no-symvers` option is passed.
- [Medium] The fallback file resolution logic incorrectly concatenates absolute paths to the kernel root directory, resulting in malformed paths and unintended file resolution.
- [Low] Misleading error diagnostics for custom path failures.
--
commit ad4674adedd0ef34b67a931273e6e044219ae8b0
Author: Joe Lawrence <joe.lawrence@redhat.com>
objtool/klp: add --symvers option to klp diff
This commit adds a --symvers (-s) option to klp diff that allows specifying
a custom path to Module.symvers. The change aims to support out-of-tree
module patching by decoupling the tool from the default Module.symvers
location while preserving the existing fallback detection logic.
> diff --git a/tools/objtool/klp-diff.c b/tools/objtool/klp-diff.c
> --- a/tools/objtool/klp-diff.c
> +++ b/tools/objtool/klp-diff.c
[ ... ]
> @@ -46,6 +47,7 @@ static const struct option klp_diff_options[] = {
> OPT_BOOLEAN('d', "debug", &debug, "enable all debug output"),
> OPT_BOOLEAN(0, "debug-correlate", &debug_correlate, "enable correlation debug output"),
> OPT_BOOLEAN(0, "debug-clone", &debug_clone, "enable cloning debug output"),
> + OPT_STRING('s', "symvers", &symvers_path, "path", "path to Module.symvers (default: Module.symvers)"),
Could this cause a NULL pointer dereference if a user passes --no-symvers?
Since OPT_STRING doesn't include the PARSE_OPT_NONEG flag, the option
parsing machinery will automatically accept --no-symvers and set
symvers_path to NULL. Later in read_exports(), this would result in calling
fopen(NULL, "r").
> OPT_END(),
> };
[ ... ]
> @@ -85,16 +87,15 @@ static char *escape_str(const char *orig)
>
> static int read_exports(void)
> {
> - const char *symvers = "Module.symvers";
> char line[1024], *path = NULL;
> unsigned int line_num = 1;
> FILE *file;
>
> - file = fopen(symvers, "r");
> + file = fopen(symvers_path, "r");
> if (!file) {
> - path = top_level_dir(symvers);
> + path = top_level_dir(symvers_path);
What happens here if the user supplies an absolute path like
/build/out-of-tree/Module.symvers that fails the initial fopen()?
It looks like top_level_dir() will unconditionally append the absolute path
to the kernel root directory, resulting in a malformed path like
/path/to/linux//build/out-of-tree/Module.symvers. Is it intentional to
apply the top-level directory fallback to absolute paths?
> if (!path) {
> - ERROR("can't open '%s', \"objtool diff\" should be run from the kernel tree", symvers);
> + ERROR("can't open '%s', \"objtool diff\" should be run from the kernel tree", symvers_path);
Since the goal of this patch is to enable out-of-tree module patching, does
this error message still make sense when a custom path is provided?
It might be confusing to tell users they must run from the kernel tree when
they explicitly passed an out-of-tree --symvers path.
Additionally, if the fallback fopen() fails later on, the code calls
ERROR_GLIBC("fopen") which prints "fopen: No such file or directory" without
indicating which path failed. Would it be helpful to include the actual file
path in these error diagnostics?
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260512221102.2720763-1-joe.lawrence@redhat.com?part=1
^ permalink raw reply
* Re: Sashiko patch review for live-patching?
From: Joe Lawrence @ 2026-05-14 16:59 UTC (permalink / raw)
To: live-patching
Cc: Jiri Kosina, Josh Poimboeuf, Miroslav Benes, Petr Mladek,
Song Liu
In-Reply-To: <agSjM8dxgnV9QQaf@redhat.com>
On 5/13/26 12:13 PM, Joe Lawrence wrote:
> Hello live-patching maintainers,
>
> I've noticed several references to the Sashiko (https://sashiko.dev/)
> kernel review bot on this list and was wondering if there is interest in
> adding live-patching to the mailing lists Sashiko tracks.
>
> Integration appears straightforward: we can submit an MR to add our
> entry to sashiko-k8s.yaml and customize the bot's email behavior in
> email_policy.toml.
>
> Full Sashiko Maintainer documentation is available here:
> https://github.com/sashiko-dev/sashiko/blob/main/MAINTAINERS_GUIDE.md
>
> Personally, I would vote to set reply_to_author. I don't have a strong
> opinion on the other custom options, provided that the CC list is opt-in
> rather than simply mirrored from the MAINTAINERS::LIVE PATCHING file.
> Either way, I've found the Sashiko web interface very helpful in patch
> review.
>
Sashiko.dev MR for live-patching@vger.kernel.org w/reply-all policy:
https://github.com/sashiko-dev/sashiko/pull/195
--
Joe
^ permalink raw reply
* Re: Sashiko patch review for live-patching?
From: Song Liu @ 2026-05-14 15:45 UTC (permalink / raw)
To: Petr Mladek
Cc: Josh Poimboeuf, Joe Lawrence, live-patching, Jiri Kosina,
Miroslav Benes
In-Reply-To: <agV33GZsjwQjZNGb@pathway.suse.cz>
On Thu, May 14, 2026 at 12:21 AM Petr Mladek <pmladek@suse.com> wrote:
>
> On Wed 2026-05-13 12:47:13, Josh Poimboeuf wrote:
> > On Wed, May 13, 2026 at 10:17:51AM -0700, Song Liu wrote:
> > > Hi Joe,
> > >
> > > On Wed, May 13, 2026 at 9:13 AM Joe Lawrence <joe.lawrence@redhat.com> wrote:
> > > >
> > > > Hello live-patching maintainers,
> > > >
> > > > I've noticed several references to the Sashiko (https://sashiko.dev/)
> > > > kernel review bot on this list and was wondering if there is interest in
> > > > adding live-patching to the mailing lists Sashiko tracks.
> > >
> > > I think it is a great idea. AFAICT, these bots add a lot of values in the
> > > code reviews.
> >
> > +1
> >
> > > > Integration appears straightforward: we can submit an MR to add our
> > > > entry to sashiko-k8s.yaml and customize the bot's email behavior in
> > > > email_policy.toml.
> > > >
> > > > Full Sashiko Maintainer documentation is available here:
> > > > https://github.com/sashiko-dev/sashiko/blob/main/MAINTAINERS_GUIDE.md
> > > >
> > > > Personally, I would vote to set reply_to_author. I don't have a strong
> > > > opinion on the other custom options, provided that the CC list is opt-in
> > > > rather than simply mirrored from the MAINTAINERS::LIVE PATCHING file.
> > > > Either way, I've found the Sashiko web interface very helpful in patch
> > > > review.
> > >
> > > Given the relatively low volume of patches to the livepatch mail list, I
> > > think we can use reply_all. But if folks prefer reply_to_author instead,
> > > we sure can use the cc list.
> >
> > I would vote reply_all. The signal/noise ratio isn't perfect, but it's
> > high enough to be useful in many cases. That way the
> > maintainers/reviewers are aware of any potential issues, and it avoids
> > duplicating review work and fragmenting conversations.
>
> I agree. And it might even motivate us to update the subsystem
> specific review prompts so that the review gets improved over time.
+1 for livepatch specific prompts. We sure have some unique constraints
due to the nature of livepatch.
Thanks,
Song
^ permalink raw reply
* Re: [PATCH] killswitch: add per-function short-circuit mitigation primitive
From: Jiri Olsa @ 2026-05-14 14:35 UTC (permalink / raw)
To: Sasha Levin
Cc: Michal Hocko, Breno Leitao, Andrew Morton, corbet, skhan,
linux-doc, linux-kernel, linux-kselftest, gregkh, akinobu.mita,
live-patching
In-Reply-To: <agIbaeBQAr-RkqYc@laps>
On Mon, May 11, 2026 at 02:09:45PM -0400, Sasha Levin wrote:
SNIP
> > > Even if I'm okay with rebooting that often (and I really really would prefer
> > > not to), this doesn't solve the issues of a larger fleet of servers that can't
> > > just reboot that often.
> > >
> > > What am I missing?
> >
> > For one, you are missing more maintainers of code modification infrastructures.
>
> Happy to add more, but I don't want to be too spammy. I'll add in the
> livepatching ML and the fault injection maintainer (I couldn't find a list).
> Please add any other folks/lists who you think might want to contribute to this
> discussion.
hi,
could you please add bpf (bpf@vger.kernel.org) to the loop?
thanks,
jirka
^ permalink raw reply
* Re: Sashiko patch review for live-patching?
From: Petr Mladek @ 2026-05-14 7:21 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: Song Liu, Joe Lawrence, live-patching, Jiri Kosina,
Miroslav Benes
In-Reply-To: <ju3k5mp22u37l4m27xygqce2sh2nczwckvmk3exkldk5365csx@q53zbumzj33t>
On Wed 2026-05-13 12:47:13, Josh Poimboeuf wrote:
> On Wed, May 13, 2026 at 10:17:51AM -0700, Song Liu wrote:
> > Hi Joe,
> >
> > On Wed, May 13, 2026 at 9:13 AM Joe Lawrence <joe.lawrence@redhat.com> wrote:
> > >
> > > Hello live-patching maintainers,
> > >
> > > I've noticed several references to the Sashiko (https://sashiko.dev/)
> > > kernel review bot on this list and was wondering if there is interest in
> > > adding live-patching to the mailing lists Sashiko tracks.
> >
> > I think it is a great idea. AFAICT, these bots add a lot of values in the
> > code reviews.
>
> +1
>
> > > Integration appears straightforward: we can submit an MR to add our
> > > entry to sashiko-k8s.yaml and customize the bot's email behavior in
> > > email_policy.toml.
> > >
> > > Full Sashiko Maintainer documentation is available here:
> > > https://github.com/sashiko-dev/sashiko/blob/main/MAINTAINERS_GUIDE.md
> > >
> > > Personally, I would vote to set reply_to_author. I don't have a strong
> > > opinion on the other custom options, provided that the CC list is opt-in
> > > rather than simply mirrored from the MAINTAINERS::LIVE PATCHING file.
> > > Either way, I've found the Sashiko web interface very helpful in patch
> > > review.
> >
> > Given the relatively low volume of patches to the livepatch mail list, I
> > think we can use reply_all. But if folks prefer reply_to_author instead,
> > we sure can use the cc list.
>
> I would vote reply_all. The signal/noise ratio isn't perfect, but it's
> high enough to be useful in many cases. That way the
> maintainers/reviewers are aware of any potential issues, and it avoids
> duplicating review work and fragmenting conversations.
I agree. And it might even motivate us to update the subsystem
specific review prompts so that the review gets improved over time.
Best Regards,
Petr
^ permalink raw reply
* Re: Sashiko patch review for live-patching?
From: Miroslav Benes @ 2026-05-14 6:52 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: Song Liu, Joe Lawrence, live-patching, Jiri Kosina, Petr Mladek
In-Reply-To: <ju3k5mp22u37l4m27xygqce2sh2nczwckvmk3exkldk5365csx@q53zbumzj33t>
[-- Attachment #1: Type: text/plain, Size: 1703 bytes --]
On Wed, 13 May 2026, Josh Poimboeuf wrote:
> On Wed, May 13, 2026 at 10:17:51AM -0700, Song Liu wrote:
> > Hi Joe,
> >
> > On Wed, May 13, 2026 at 9:13 AM Joe Lawrence <joe.lawrence@redhat.com> wrote:
> > >
> > > Hello live-patching maintainers,
> > >
> > > I've noticed several references to the Sashiko (https://sashiko.dev/)
> > > kernel review bot on this list and was wondering if there is interest in
> > > adding live-patching to the mailing lists Sashiko tracks.
> >
> > I think it is a great idea. AFAICT, these bots add a lot of values in the
> > code reviews.
>
> +1
>
> > > Integration appears straightforward: we can submit an MR to add our
> > > entry to sashiko-k8s.yaml and customize the bot's email behavior in
> > > email_policy.toml.
> > >
> > > Full Sashiko Maintainer documentation is available here:
> > > https://github.com/sashiko-dev/sashiko/blob/main/MAINTAINERS_GUIDE.md
> > >
> > > Personally, I would vote to set reply_to_author. I don't have a strong
> > > opinion on the other custom options, provided that the CC list is opt-in
> > > rather than simply mirrored from the MAINTAINERS::LIVE PATCHING file.
> > > Either way, I've found the Sashiko web interface very helpful in patch
> > > review.
> >
> > Given the relatively low volume of patches to the livepatch mail list, I
> > think we can use reply_all. But if folks prefer reply_to_author instead,
> > we sure can use the cc list.
>
> I would vote reply_all. The signal/noise ratio isn't perfect, but it's
> high enough to be useful in many cases. That way the
> maintainers/reviewers are aware of any potential issues, and it avoids
> duplicating review work and fragmenting conversations.
+1
Miroslav
^ permalink raw reply
* Re: Sashiko patch review for live-patching?
From: Josh Poimboeuf @ 2026-05-13 19:47 UTC (permalink / raw)
To: Song Liu
Cc: Joe Lawrence, live-patching, Jiri Kosina, Miroslav Benes,
Petr Mladek
In-Reply-To: <CAPhsuW6CgBtEGAK+E0n7+tnLAWNPTEuvnAo0vtQsbMVBb6htFw@mail.gmail.com>
On Wed, May 13, 2026 at 10:17:51AM -0700, Song Liu wrote:
> Hi Joe,
>
> On Wed, May 13, 2026 at 9:13 AM Joe Lawrence <joe.lawrence@redhat.com> wrote:
> >
> > Hello live-patching maintainers,
> >
> > I've noticed several references to the Sashiko (https://sashiko.dev/)
> > kernel review bot on this list and was wondering if there is interest in
> > adding live-patching to the mailing lists Sashiko tracks.
>
> I think it is a great idea. AFAICT, these bots add a lot of values in the
> code reviews.
+1
> > Integration appears straightforward: we can submit an MR to add our
> > entry to sashiko-k8s.yaml and customize the bot's email behavior in
> > email_policy.toml.
> >
> > Full Sashiko Maintainer documentation is available here:
> > https://github.com/sashiko-dev/sashiko/blob/main/MAINTAINERS_GUIDE.md
> >
> > Personally, I would vote to set reply_to_author. I don't have a strong
> > opinion on the other custom options, provided that the CC list is opt-in
> > rather than simply mirrored from the MAINTAINERS::LIVE PATCHING file.
> > Either way, I've found the Sashiko web interface very helpful in patch
> > review.
>
> Given the relatively low volume of patches to the livepatch mail list, I
> think we can use reply_all. But if folks prefer reply_to_author instead,
> we sure can use the cc list.
I would vote reply_all. The signal/noise ratio isn't perfect, but it's
high enough to be useful in many cases. That way the
maintainers/reviewers are aware of any potential issues, and it avoids
duplicating review work and fragmenting conversations.
--
Josh
^ permalink raw reply
* Re: Sashiko patch review for live-patching?
From: Marcos Paulo de Souza @ 2026-05-13 18:43 UTC (permalink / raw)
To: Joe Lawrence, live-patching
Cc: Jiri Kosina, Josh Poimboeuf, Miroslav Benes, Petr Mladek,
Song Liu
In-Reply-To: <agSjM8dxgnV9QQaf@redhat.com>
On Wed, 2026-05-13 at 12:13 -0400, Joe Lawrence wrote:
> Hello live-patching maintainers,
>
> I've noticed several references to the Sashiko (https://sashiko.dev/)
> kernel review bot on this list and was wondering if there is interest
> in
> adding live-patching to the mailing lists Sashiko tracks.
>
> Integration appears straightforward: we can submit an MR to add our
> entry to sashiko-k8s.yaml and customize the bot's email behavior in
> email_policy.toml.
>
> Full Sashiko Maintainer documentation is available here:
> https://github.com/sashiko-dev/sashiko/blob/main/MAINTAINERS_GUIDE.md
>
> Personally, I would vote to set reply_to_author. I don't have a
> strong
> opinion on the other custom options, provided that the CC list is
> opt-in
> rather than simply mirrored from the MAINTAINERS::LIVE PATCHING file.
> Either way, I've found the Sashiko web interface very helpful in
> patch
> review.
We can start with reply_to_author for some time before opting for
reply_all. At least in my last patches, it was quite good and helpful.
>
> --
> Joe
>
^ permalink raw reply
* Re: Sashiko patch review for live-patching?
From: Song Liu @ 2026-05-13 17:17 UTC (permalink / raw)
To: Joe Lawrence
Cc: live-patching, Jiri Kosina, Josh Poimboeuf, Miroslav Benes,
Petr Mladek
In-Reply-To: <agSjM8dxgnV9QQaf@redhat.com>
Hi Joe,
On Wed, May 13, 2026 at 9:13 AM Joe Lawrence <joe.lawrence@redhat.com> wrote:
>
> Hello live-patching maintainers,
>
> I've noticed several references to the Sashiko (https://sashiko.dev/)
> kernel review bot on this list and was wondering if there is interest in
> adding live-patching to the mailing lists Sashiko tracks.
I think it is a great idea. AFAICT, these bots add a lot of values in the
code reviews.
> Integration appears straightforward: we can submit an MR to add our
> entry to sashiko-k8s.yaml and customize the bot's email behavior in
> email_policy.toml.
>
> Full Sashiko Maintainer documentation is available here:
> https://github.com/sashiko-dev/sashiko/blob/main/MAINTAINERS_GUIDE.md
>
> Personally, I would vote to set reply_to_author. I don't have a strong
> opinion on the other custom options, provided that the CC list is opt-in
> rather than simply mirrored from the MAINTAINERS::LIVE PATCHING file.
> Either way, I've found the Sashiko web interface very helpful in patch
> review.
Given the relatively low volume of patches to the livepatch mail list, I
think we can use reply_all. But if folks prefer reply_to_author instead,
we sure can use the cc list.
Thanks,
Song
^ permalink raw reply
* Sashiko patch review for live-patching?
From: Joe Lawrence @ 2026-05-13 16:13 UTC (permalink / raw)
To: live-patching
Cc: Jiri Kosina, Josh Poimboeuf, Miroslav Benes, Petr Mladek,
Song Liu
Hello live-patching maintainers,
I've noticed several references to the Sashiko (https://sashiko.dev/)
kernel review bot on this list and was wondering if there is interest in
adding live-patching to the mailing lists Sashiko tracks.
Integration appears straightforward: we can submit an MR to add our
entry to sashiko-k8s.yaml and customize the bot's email behavior in
email_policy.toml.
Full Sashiko Maintainer documentation is available here:
https://github.com/sashiko-dev/sashiko/blob/main/MAINTAINERS_GUIDE.md
Personally, I would vote to set reply_to_author. I don't have a strong
opinion on the other custom options, provided that the CC list is opt-in
rather than simply mirrored from the MAINTAINERS::LIVE PATCHING file.
Either way, I've found the Sashiko web interface very helpful in patch
review.
--
Joe
^ 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