Re: [PATCH 3/3] livepatch: Clean up klp_init_object_loaded() when fails

[Petr Mladek <pmladek@suse.com>]

On Sun 2026-08-30 17:56:07, sashiko-bot@kernel.org 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 <pmladek@suse.com>
> 
> 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/20260823062313.1321B1F000E9@smtp.kernel.org
> 
> > 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