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

[Petr Mladek <pmladek@suse.com>]

On Tue 2026-09-08 12:25:48, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] Partial relocations are not reverted when apply_relocate_add() fails midway, leading to permanent state corruption.
> --
> 
> commit 3f6df1b4ed699e839abbdc8919a6c7de5d1850c8
> Author: Petr Mladek <pmladek@suse.com>
> 
> livepatch: Clean up klp_init_object_loaded() when fails
> 
> When loading a module, klp_module_coming() updates all enabled patches.
> If an error occurs, klp_cleanup_module_patches_limited() cleans up fully
> processed patches, but skips the patch that failed midway. 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
> Closes: https://lore.kernel.org/r/20260830175608.4BABB1F000E9@smtp.kernel.org
> 
> > Next, klp_init_object_loaded() has to clear its own changes on
> > failure. It just returns an error when relocations failed because
> > they clear their own mess. It could call klp_free_object_loaded()
> > in other situations because all relocations were done and other
> > values are just cleared.
> 
> [Severity: High]
> Does this actually clear its own mess completely? Looking at how relocations
> are rolled back in klp_write_object_relocs_limited(), which was introduced
> earlier in this series:
> 
> kernel/livepatch/core.c:klp_write_object_relocs_limited() {
>     ...
> 		ret = klp_write_section_relocs(patch->mod, info->sechdrs,
> 					       info->secstrings,
> 					       patch->mod->core_kallsyms.strtab,
> 					       info->symndx, i, obj->name, apply);
> 		if (ret) {
> 			if (apply)
> 				klp_write_object_relocs_limited(patch, obj, false, i);
> 			return ret;
> 		}
>     ...
> }
> 
> By passing i as the limit to the rollback function, the rollback loop:
> 
> 	for (i = 1; i < limit; i++) {
> 
> will terminate at i - 1. Doesn't this skip the rollback for the partially
> applied section i?
> 
> If apply_relocate_add succeeds on relocations 0 to k-1 in section i, but
> fails on relocation k, these partial relocations remain permanently applied
> in the livepatch module's text. 

This is the same problem which was reported for the 4th patch.
It would need to handled in the arch-code, namely
in __write_relocate_add() in arch/x86/kernel/module.c.

Best Regards,
Petr

> Subsequent attempts to load the target module will fail immediately at
> relocation 0 due to non-zero target memory, converting a potentially transient
> address-space layout failure into a permanent -ENOEXEC on retry.