On Sun 2026-08-30 17:56:07, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s)
> to consider:
>
> New issues:
> - [Medium] Failing `klp_apply_object_relocs()` triggers spurious
> architectural warnings due to unconditional clearing of unapplied relocations
> in the newly added error path.
>
> Pre-existing issues:
> - [High] If `klp_pre_patch_callback()` or `klp_patch_object()` fails during
> `klp_module_coming()`, the successfully initialized state from
> `klp_init_object_loaded()` is leaked. The cleanup function
> `klp_cleanup_module_patches_limited()` intentionally skips the currently
> failing patch, leaving applied relocations in the livepatch module uncleared
> and causing subsequent load attempts of the target module to persistently
> fail.
> --
>
> commit a90c142b7a6f2c5551f83382ec1fcf15614ffbac
> Author: Petr Mladek <[email protected]>
>
> livepatch: Clean up klp_init_object_loaded() when fails
>
> When a module is loaded, klp_module_coming() iterates over patches and
> calls klp_init_object_loaded(). If initialization fails, it delegates
> cleanup to klp_cleanup_module_patches_limited(). This commit updates
> klp_free_object_loaded() to revert all and only changes made by
> klp_init_object_loaded().
>
> Closes: https://lore.kernel.org/r/[email protected]
>
> > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > index a6796cd6b65fd..706a4a7193514 100644
> > --- a/kernel/livepatch/core.c
> > +++ b/kernel/livepatch/core.c
>
> [ ... ]
>
> > @@ -916,11 +920,17 @@ static int klp_init_object_loaded(struct klp_patch
> > *patch,
> > if (!ret) {
> > pr_err("kallsyms size lookup failed for '%s'
> > replacement\n",
> > func->old_name);
> > - return -ENOENT;
> > + ret = -ENOENT;
> > + goto err;
> > }
> > }
> >
> > return 0;
> > +
> > +err:
> > + klp_free_object_loaded(patch, obj);
> > +
> > + return ret;
> > }
>
> [Severity: Medium]
> Does this unconditionally clear unapplied relocations?
Ah, I thought that this should be OK.
> When klp_init_object_loaded() fails (for example, if a symbol lookup fails
> after klp_apply_object_relocs() succeeds, or if klp_apply_object_relocs()
> itself fails), this newly added error path calls klp_free_object_loaded().
>
> This unconditionally invokes klp_clear_object_relocs(), which iterates over
> all .klp.rela sections and attempts to un-apply them via
> clear_relocate_add().
>
> However, for sections that were never applied, the target memory remains
> unmodified (zeroed). On architectures like x86, clear_relocate_add() uses
> memcmp() to verify that the memory contains the expected relocated value.
> Since it does not, it prints an "Invalid relocation target" warning for
> every unapplied relocation entry, resulting in significant spurious warning
> spam in the kernel log.
Sashiko is right. __write_relocate_add() really checks the original
value even when clearing relocation at least on x86_64.
Only klp_write_object_relocs() knows which relocations were updated.
So it has to clear its own mess.
I am going to do the following changes on top of this patch
(just compile tested at the moment):
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 1c217ac49d2b..b6ae3a518ca9 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -342,14 +342,17 @@ int klp_apply_section_relocs(struct module *pmod,
Elf_Shdr *sechdrs,
secndx, objname, true);
}
-static int klp_write_object_relocs(struct klp_patch *patch,
- struct klp_object *obj,
- bool apply)
+static int klp_write_object_relocs_limited(struct klp_patch *patch,
+ struct klp_object *obj,
+ bool apply, int limit)
{
int i, ret;
struct klp_modinfo *info = patch->mod->klp_info;
- for (i = 1; i < info->hdr.e_shnum; i++) {
+ if (!limit || limit > info->hdr.e_shnum)
+ limit = info->hdr.e_shnum;
+
+ for (i = 1; i < limit; i++) {
Elf_Shdr *sec = info->sechdrs + i;
if (!(sec->sh_flags & SHF_RELA_LIVEPATCH))
@@ -359,13 +362,23 @@ static int klp_write_object_relocs(struct klp_patch
*patch,
info->secstrings,
patch->mod->core_kallsyms.strtab,
info->symndx, i, obj->name,
apply);
- if (ret)
+ if (ret) {
+ if (apply)
+ klp_write_object_relocs_limited(patch, obj,
false, i);
return ret;
+ }
}
return 0;
}
+static int klp_write_object_relocs(struct klp_patch *patch,
+ struct klp_object *obj,
+ bool apply)
+{
+ return klp_write_object_relocs_limited(patch, obj, apply, 0);
+}
+
static int klp_apply_object_relocs(struct klp_patch *patch,
struct klp_object *obj)
{
@@ -902,7 +915,7 @@ static int klp_init_object_loaded(struct klp_patch *patch,
*/
ret = klp_apply_object_relocs(patch, obj);
if (ret)
- goto err;
+ return ret;
}
klp_for_each_func(obj, func) {
> [ ... ]
>
> > @@ -1274,8 +1284,8 @@ static void klp_cleanup_module_patches_limited(struct
> > module *mod,
> > klp_unpatch_object(obj);
> >
> > klp_post_unpatch_callback(obj);
> > - klp_clear_object_relocs(patch, obj);
> > - klp_free_object_loaded(obj);
> > + klp_free_object_loaded(patch, obj);
> > + obj->mod = NULL;
> > break;
> > }
> > }
>
> [Severity: High]
> This is a pre-existing issue, but does this cleanup logic leak relocations
> if klp_module_coming() fails after klp_init_object_loaded() succeeds?
>
> If klp_pre_patch_callback() or klp_patch_object() fails during
> klp_module_coming(), the successfully initialized state from
> klp_init_object_loaded() is leaked.
>
> The cleanup function intentionally skips the currently failing patch:
>
> kernel/livepatch/core.c:klp_cleanup_module_patches_limited() {
> ...
> klp_for_each_patch(patch) {
> if (patch == limit)
> break;
> ...
> }
>
> This prevents its newly applied relocations from being cleared. Since these
> relocations modify the livepatch module's memory, they persist even after
> the target module's load attempt is aborted.
>
> Upon a subsequent attempt to load the target module,
> klp_apply_object_relocs() will attempt to apply the relocations again. On
> architectures like x86, apply_relocate_add() verifies that the target
> memory is zero; finding it non-zero, it returns -ENOEXEC. This causes
> klp_module_coming() to fail, permanently preventing the target module
> from being loaded as long as the livepatch is loaded.
Sigh, I got a bit lost in all the cycles. I believe that this should
get fixed the the following changes on top of this patch:
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 1c217ac49d2b..09b6f4aa6217 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -1360,7 +1373,7 @@ int klp_module_coming(struct module *mod)
if (ret) {
pr_warn("pre-patch callback failed for object
'%s'\n",
obj->name);
- goto err;
+ goto err_free_object;
}
ret = klp_patch_object(obj);
@@ -1368,8 +1381,7 @@ int klp_module_coming(struct module *mod)
pr_warn("failed to apply patch '%s' to module
'%s' (%d)\n",
patch->mod->name, obj->mod->name, ret);
- klp_post_unpatch_callback(obj);
- goto err;
+ goto err_unpatch_callback;
}
if (patch != klp_transition_patch)
@@ -1383,6 +1395,10 @@ int klp_module_coming(struct module *mod)
return 0;
+err_unpatch_callback:
+ klp_post_unpatch_callback(obj);
+err_free_object:
+ klp_free_object_loaded(patch, obj);
err:
/*
* If a patch is unsuccessfully applied, return
Note: This is called when it fails in the middle of
klp_for_each_object(patch, obj) {
Naive approach would be to implement another *_limited
variant which would revert the action for all already
proceed "obj" structures.
But this is called in klp_module_coming() so only one
struct object should match. All others are skipped.
This is why it should be enough to revert only the last "obj"
in the err_* goto targets.
We propably should enforce this => add another patch
which would reject livepatches which contain two
struct object for the same object.
Best Regards,
Petr
PS: I am going to wait few more days for a possible feedback.
Then I would v4 of this whole patchset with the additional
changes.
I hope that the 1st patch from Harry won't need more changes.
So, I will only fix my part of the patchset. /o\
Best Regards,
Petr
